diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index f4042da6e1b..d16f31721df 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -591,13 +591,9 @@ const User = RestModel.extend({ ); }, - @computed("can_delete_account", "reply_count", "topic_count") - canDeleteAccount(canDeleteAccount, replyCount, topicCount) { - return ( - !Discourse.SiteSettings.enable_sso && - canDeleteAccount && - (replyCount || 0) + (topicCount || 0) <= 1 - ); + @computed("can_delete_account") + canDeleteAccount(canDeleteAccount) { + return !Discourse.SiteSettings.enable_sso && canDeleteAccount; }, delete: function() { diff --git a/app/models/user.rb b/app/models/user.rb index 3e42b504bbe..3c090e66e96 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -236,6 +236,9 @@ class User < ActiveRecord::Base LAST_VISIT = -2 end + MAX_SELF_DELETE_POST_COUNT ||= 1 + MAX_STAFF_DELETE_POST_COUNT ||= 5 + def self.max_password_length 200 end @@ -1246,6 +1249,36 @@ class User < ActiveRecord::Base Jobs.enqueue(:create_user_reviewable, user_id: self.id) end + def has_more_posts_than?(max_post_count) + return true if user_stat && (user_stat.topic_count + user_stat.post_count) > max_post_count + + DB.query_single(<<~SQL, user_id: self.id).first > max_post_count + SELECT COUNT(1) + FROM ( + SELECT 1 + FROM posts p + JOIN topics t ON (p.topic_id = t.id) + WHERE p.user_id = :user_id AND + p.deleted_at IS NULL AND + t.deleted_at IS NULL AND + ( + t.archetype <> 'private_message' OR + EXISTS( + SELECT 1 + FROM topic_allowed_users a + WHERE a.topic_id = t.id AND a.user_id > 0 AND a.user_id <> :user_id + ) OR + EXISTS( + SELECT 1 + FROM topic_allowed_groups g + WHERE g.topic_id = p.topic_id + ) + ) + LIMIT #{max_post_count + 1} + ) x + SQL + end + protected def badge_grant diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index 0d19f64fe70..984969b1f22 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -61,9 +61,14 @@ module UserGuardian def can_delete_user?(user) return false if user.nil? || user.admin? if is_me?(user) - user.post_count <= 1 + !SiteSetting.enable_sso && + !user.has_more_posts_than?(User::MAX_SELF_DELETE_POST_COUNT) else - is_staff? && (user.first_post_created_at.nil? || user.post_count <= 5 || user.first_post_created_at > SiteSetting.delete_user_max_post_age.to_i.days.ago) + is_staff? && ( + user.first_post_created_at.nil? || + !user.has_more_posts_than?(User::MAX_STAFF_DELETE_POST_COUNT) || + user.first_post_created_at > SiteSetting.delete_user_max_post_age.to_i.days.ago + ) end end diff --git a/spec/components/guardian/user_guardian_spec.rb b/spec/components/guardian/user_guardian_spec.rb index 867a9c44c5b..12fbdd02a21 100644 --- a/spec/components/guardian/user_guardian_spec.rb +++ b/spec/components/guardian/user_guardian_spec.rb @@ -5,15 +5,15 @@ require 'rails_helper' describe UserGuardian do let :user do - Fabricate.build(:user, id: 1) + Fabricate(:user) end let :moderator do - Fabricate.build(:moderator, id: 2) + Fabricate(:moderator) end let :admin do - Fabricate.build(:admin, id: 3) + Fabricate(:admin) end let(:user_avatar) do @@ -155,7 +155,7 @@ describe UserGuardian do end let :user2 do - Fabricate.build(:user, id: 4) + Fabricate(:user) end it "returns all fields for staff" do @@ -179,4 +179,142 @@ describe UserGuardian do expect(guardian.allowed_user_field_ids(user)).to contain_exactly(*fields.map(&:id)) end end + + describe "#can_delete_user?" do + shared_examples "can_delete_user examples" do + it "isn't allowed if user is an admin" do + another_admin = Fabricate(:admin) + expect(guardian.can_delete_user?(another_admin)).to eq(false) + end + end + + shared_examples "can_delete_user staff examples" do + it "is allowed when user didn't create a post yet" do + expect(user.first_post_created_at).to be_nil + expect(guardian.can_delete_user?(user)).to eq(true) + end + + context "when user created too many posts" do + before do + (User::MAX_STAFF_DELETE_POST_COUNT + 1).times { Fabricate(:post, user: user) } + end + + it "is allowed when user created the first post within delete_user_max_post_age days" do + SiteSetting.delete_user_max_post_age = 2 + + user.user_stat = UserStat.new(new_since: 3.days.ago, first_post_created_at: 1.day.ago) + expect(guardian.can_delete_user?(user)).to eq(true) + + user.user_stat = UserStat.new(new_since: 3.days.ago, first_post_created_at: 3.day.ago) + expect(guardian.can_delete_user?(user)).to eq(false) + end + end + + context "when user didn't create many posts" do + before do + (User::MAX_STAFF_DELETE_POST_COUNT - 1).times { Fabricate(:post, user: user) } + end + + it "is allowed when even when user created the first post before delete_user_max_post_age days" do + SiteSetting.delete_user_max_post_age = 2 + + user.user_stat = UserStat.new(new_since: 3.days.ago, first_post_created_at: 3.day.ago) + expect(guardian.can_delete_user?(user)).to eq(true) + end + end + end + + context "delete myself" do + let(:guardian) { Guardian.new(user) } + + include_examples "can_delete_user examples" + + it "isn't allowed when SSO is enabled" do + SiteSetting.sso_url = "https://www.example.com/sso" + SiteSetting.enable_sso = true + expect(guardian.can_delete_user?(user)).to eq(false) + end + + it "isn't allowed when user created too many posts" do + Fabricate(:post, user: user) + expect(guardian.can_delete_user?(user)).to eq(true) + + Fabricate(:post, user: user) + expect(guardian.can_delete_user?(user)).to eq(false) + end + + it "isn't allowed when user created too many posts in PM" do + topic = Fabricate(:private_message_topic, user: user) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(false) + end + + it "is allowed when user responded to PM from system user" do + topic = Fabricate(:private_message_topic, user: Discourse.system_user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: Discourse.system_user), + Fabricate.build(:topic_allowed_user, user: user) + ]) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + end + + it "is allowed when user created multiple posts in PMs to themself" do + topic = Fabricate(:private_message_topic, user: user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user) + ]) + + Fabricate(:post, user: user, topic: topic) + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + end + + it "isn't allowed when user created multiple posts in PMs sent to other users" do + topic = Fabricate(:private_message_topic, user: user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user), + Fabricate.build(:topic_allowed_user, user: Fabricate(:user)) + ]) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(false) + end + + it "isn't allowed when user created multiple posts in PMs sent to groups" do + topic = Fabricate(:private_message_topic, user: user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user) + ], topic_allowed_groups: [ + Fabricate.build(:topic_allowed_group, group: Fabricate(:group)), + Fabricate.build(:topic_allowed_group, group: Fabricate(:group)) + ]) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(true) + + Fabricate(:post, user: user, topic: topic) + expect(guardian.can_delete_user?(user)).to eq(false) + end + end + + context "for moderators" do + let(:guardian) { Guardian.new(moderator) } + include_examples "can_delete_user examples" + include_examples "can_delete_user staff examples" + end + + context "for admins" do + let(:guardian) { Guardian.new(admin) } + include_examples "can_delete_user examples" + include_examples "can_delete_user staff examples" + end + end end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index ae6b742e305..24fb33d1f8a 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -2104,87 +2104,6 @@ describe Guardian do end - describe "can_delete_user?" do - it "is false without a logged in user" do - expect(Guardian.new(nil).can_delete_user?(user)).to be_falsey - end - - it "is false without a user to look at" do - expect(Guardian.new(admin).can_delete_user?(nil)).to be_falsey - end - - it "is false for regular users" do - expect(Guardian.new(user).can_delete_user?(coding_horror)).to be_falsey - end - - context "delete myself" do - fab!(:myself) { Fabricate(:user, created_at: 6.months.ago) } - subject { Guardian.new(myself).can_delete_user?(myself) } - - it "is true to delete myself and I have never made a post" do - expect(subject).to be_truthy - end - - it "is true to delete myself and I have only made 1 post" do - myself.stubs(:post_count).returns(1) - expect(subject).to be_truthy - end - - it "is false to delete myself and I have made 2 posts" do - myself.stubs(:post_count).returns(2) - expect(subject).to be_falsey - end - end - - shared_examples "can_delete_user examples" do - it "is true if user is not an admin and has never posted" do - expect(Guardian.new(actor).can_delete_user?(Fabricate.build(:user, created_at: 100.days.ago))).to be_truthy - end - - it "is true if user is not an admin and first post is not too old" do - user = Fabricate.build(:user, created_at: 100.days.ago) - user.stubs(:post_count).returns(10) - user.stubs(:first_post_created_at).returns(9.days.ago) - SiteSetting.delete_user_max_post_age = 10 - expect(Guardian.new(actor).can_delete_user?(user)).to be_truthy - end - - it "is false if user is an admin" do - expect(Guardian.new(actor).can_delete_user?(another_admin)).to be_falsey - end - - it "is false if user's first post is too old" do - user = Fabricate.build(:user, created_at: 100.days.ago) - user.stubs(:post_count).returns(10) - user.stubs(:first_post_created_at).returns(11.days.ago) - SiteSetting.delete_user_max_post_age = 10 - expect(Guardian.new(actor).can_delete_user?(user)).to be_falsey - end - end - - shared_examples "can_delete_user staff examples" do - it "is true if posts are less than or equal to 5" do - user = Fabricate.build(:user, created_at: 100.days.ago) - user.stubs(:post_count).returns(4) - user.stubs(:first_post_created_at).returns(11.days.ago) - SiteSetting.delete_user_max_post_age = 10 - expect(Guardian.new(actor).can_delete_user?(user)).to be_truthy - end - end - - context "for moderators" do - let(:actor) { moderator } - include_examples "can_delete_user examples" - include_examples "can_delete_user staff examples" - end - - context "for admins" do - let(:actor) { admin } - include_examples "can_delete_user examples" - include_examples "can_delete_user staff examples" - end - end - describe "can_delete_all_posts?" do it "is false without a logged in user" do expect(Guardian.new(nil).can_delete_all_posts?(user)).to be_falsey