Skip to content
Permalink
Browse files

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.
  • Loading branch information...
gschlager committed Aug 10, 2019
1 parent 9f445be commit e4f14ca3d79b0cc19a12348e330e2165a9364028
@@ -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() {
@@ -236,6 +236,9 @@ module NewTopicDuration
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 @@ def create_reviewable
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
@@ -61,9 +61,14 @@ def can_unsilence_user?(user)
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

@@ -5,15 +5,15 @@
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 @@
end

let :user2 do
Fabricate.build(:user, id: 4)
Fabricate(:user)
end

it "returns all fields for staff" do
@@ -179,4 +179,142 @@
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
@@ -2174,87 +2174,6 @@

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

1 comment on commit e4f14ca

@discoursebot

This comment has been minimized.

Copy link

commented on e4f14ca Aug 13, 2019

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

https://meta.discourse.org/t/malicious-personal-messages/125158/29

Please sign in to comment.
You can’t perform that action at this time.