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.

app/assets/javascripts/discourse/models/user.js.es6

@@ -591,13 +591,9 @@ const User = RestModel.extend({
     );
   },
 
-  @computed("can_delete_account", "reply_count", "topic_count")
+  @computed("can_delete_account")
-  canDeleteAccount(canDeleteAccount, replyCount, topicCount) {
+  canDeleteAccount(canDeleteAccount) {
-    return (
+    return !Discourse.SiteSettings.enable_sso && canDeleteAccount;
-      !Discourse.SiteSettings.enable_sso &&
-      canDeleteAccount &&
-      (replyCount || 0) + (topicCount || 0) <= 1
-    );
   },
 
   delete: function() {

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

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
 

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

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