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.
This commit is contained in:
Gerhard Schlager
parent
9f445bec09
commit
e4f14ca3d7
|
|
@ -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 @@ 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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -2174,87 +2174,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
|
||||
|
|
|
|||
Loading…
Reference in New Issue