FIX: Disallow user self-delete when user posted in PMs

FIX: Disallow user self-delete when user posted in PMs

All posts created by the user are counted unless they are deleted, belong to a PM sent between a non-human user and the user or belong to a PM created by the user which doesn’t have any other recipients.

It also makes the guardian prevent self-deletes when SSO is enabled.

diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6
index d225e5c..84ac5ef 100644
--- a/app/assets/javascripts/discourse/models/user.js.es6
+++ b/app/assets/javascripts/discourse/models/user.js.es6
@@ -630,13 +630,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 bfcd565..040c3fa 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
@@ -1251,6 +1254,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 0d19f64..984969b 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 867a9c4..12fbdd0 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),

[... diff too long, it was truncated ...]

GitHub sha: e4f14ca3

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there: