0
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2026-08-04 10:39:43 +08:00
discourse/spec/services/user_destroyer_spec.rb
Alan Guo Xiang Tan b03428cfec
FIX: Resolve all of a deleted spammer's reviewables (#40780)
When a moderator picks "Delete User" or "Delete and Block User" on one
of the spammer's flagged posts, deleting the user is meant to settle all
of their reviewables, since there is no user left to act on. Previously
only part of that happened:

- `UserDestroyer#agree_with_flags` resolved the spammer's other flagged
posts by checking for the `agree_and_keep` action, but a flag on a
hidden post only offers `agree_and_keep_hidden`, so those flags were
skipped and stayed pending forever.
- Queued posts by the spammer were never touched, so they also stayed
pending.
- `UserDestroyer#destroy` skips resolving the spammer's account
reviewable when a `reviewable_id` option is present. That option is
meant to prevent the account reviewable from resolving itself twice when
the deletion starts from it, but the guard only checked presence, so a
deletion starting from a flagged post also skipped the account
reviewable.
- The browser updates review queue rows from message bus broadcasts.
Those can arrive after the page has stopped waiting, so the acting
moderator kept seeing the affected rows as "Pending" until a full
reload. Acting on one of those stale rows returns a 403 because the
target user no longer exists.

This PR resolves every reviewable tied to the deleted user and updates
the acting moderator's review queue without a reload.
2026-06-15 13:34:15 +08:00

630 lines
21 KiB
Ruby
Vendored

# frozen_string_literal: true
RSpec.describe UserDestroyer do
fab!(:user) { Fabricate(:user_with_secondary_email, refresh_auto_groups: true) }
fab!(:admin)
describe ".new" do
it "raises an error when user is nil" do
expect { UserDestroyer.new(nil) }.to raise_error(Discourse::InvalidParameters)
end
it "raises an error when user is not a User" do
expect { UserDestroyer.new(5) }.to raise_error(Discourse::InvalidParameters)
end
end
describe "#destroy" do
it "raises an error when user is nil" do
expect { UserDestroyer.new(admin).destroy(nil) }.to raise_error(Discourse::InvalidParameters)
end
it "raises an error when user is not a User" do
expect { UserDestroyer.new(admin).destroy("nothing") }.to raise_error(
Discourse::InvalidParameters,
)
end
it "raises an error when regular user tries to delete another user" do
expect { UserDestroyer.new(user).destroy(Fabricate(:user)) }.to raise_error(
Discourse::InvalidAccess,
)
end
shared_examples "successfully destroy a user" do
it "should delete the user" do
expect { destroy }.to change { User.count }.by(-1)
end
it "should return the deleted user record" do
return_value = destroy
expect(return_value).to eq(user)
expect(return_value).to be_destroyed
end
it "should log the action" do
StaffActionLogger.any_instance.expects(:log_user_deletion).with(user, anything).once
destroy
end
it "should not log the action if quiet is true" do
expect {
UserDestroyer.new(admin).destroy(user, destroy_opts.merge(quiet: true))
}.to_not change { UserHistory.where(action: UserHistory.actions[:delete_user]).count }
end
it "triggers a extensibility event" do
event = DiscourseEvent.track_events { destroy }.last
expect(event[:event_name]).to eq(:user_destroyed)
expect(event[:params].first).to eq(user)
end
end
shared_examples "email block list" do
it "doesn't add email to block list by default" do
ScreenedEmail.expects(:block).never
destroy
end
it "adds emails to block list if block_email is true" do
expect {
UserDestroyer.new(admin).destroy(user, destroy_opts.merge(block_email: true))
}.to change { ScreenedEmail.count }.by(2)
end
it "also blocks associated account emails if block_email is true" do
UserAssociatedAccount.create!(
user: user,
provider_name: "google_oauth2",
provider_uid: "12345",
info: {
email: "oauth@example.com",
},
)
expect {
UserDestroyer.new(admin).destroy(user, destroy_opts.merge(block_email: true))
}.to change { ScreenedEmail.count }.by(3)
expect(ScreenedEmail.exists?(email: "oauth@example.com")).to eq(true)
end
it "does not duplicate block when associated account email matches a user email" do
UserAssociatedAccount.create!(
user: user,
provider_name: "google_oauth2",
provider_uid: "12345",
info: {
email: user.email,
},
)
expect {
UserDestroyer.new(admin).destroy(user, destroy_opts.merge(block_email: true))
}.to change { ScreenedEmail.count }.by(2)
end
end
context "when user deletes self" do
subject(:destroy) { UserDestroyer.new(user).destroy(user, destroy_opts) }
let(:destroy_opts) { { delete_posts: true, context: "/u/username/preferences/account" } }
include_examples "successfully destroy a user"
it "logs context in default locale" do
I18n.locale = :ja
SiteSetting.default_locale = :de
destroy
expect(UserHistory.where(action: UserHistory.actions[:delete_user]).last.context).to eq(
I18n.with_locale(:de) do
I18n.t("staff_action_logs.user_delete_self", url: "/u/username/preferences/account")
end,
)
end
end
context "when context is missing" do
it "logs warning message if context is missing" do
logger = track_log_messages { UserDestroyer.new(admin).destroy(user) }
expect(logger.warnings).to include(/User destroyed without context from:/)
end
end
context "with a reviewable post" do
let!(:reviewable) { Fabricate(:reviewable, created_by: user) }
it "removes the queued post" do
UserDestroyer.new(admin).destroy(user)
expect(Reviewable.where(created_by_id: user.id).count).to eq(0)
end
it "removes the queued post even when it has notes" do
Fabricate(:reviewable_note, reviewable: reviewable)
UserDestroyer.new(admin).destroy(user)
expect(Reviewable.where(created_by_id: user.id).count).to eq(0)
expect(ReviewableNote.where(reviewable_id: reviewable.id).count).to eq(0)
end
end
context "with a reviewable user" do
let(:reviewable) { Fabricate(:reviewable, created_by: admin) }
it "sets the reviewable user as rejected" do
UserDestroyer.new(admin).destroy(reviewable.target)
expect(reviewable.reload).to be_rejected
end
it "links the staff action log to the reviewable when passed via opts" do
expect {
UserDestroyer.new(admin).destroy(reviewable.target, reviewable_id: reviewable.id)
}.to change {
UserHistory.where(
action: UserHistory.actions[:delete_user],
reviewable_id: reviewable.id,
).count
}.by(1)
end
end
context "with a directory item record" do
it "removes the directory item" do
DirectoryItem.create!(
user: user,
period_type: 1,
likes_received: 0,
likes_given: 0,
topics_entered: 0,
topic_count: 0,
post_count: 0,
)
UserDestroyer.new(admin).destroy(user)
expect(DirectoryItem.where(user_id: user.id).count).to eq(0)
end
end
context "with a draft" do
let!(:draft) { Draft.set(user, "test", 0, "test") }
it "removed the draft" do
UserDestroyer.new(admin).destroy(user)
expect(Draft.where(user_id: user.id).count).to eq(0)
end
end
context "when user has posts" do
let!(:topic_starter) { Fabricate(:user) }
let!(:topic) { Fabricate(:topic, user: topic_starter) }
let!(:first_post) { Fabricate(:post, user: topic_starter, topic: topic) }
let!(:post) { Fabricate(:post, user: user, topic: topic) }
context "when delete_posts is false" do
subject(:destroy) { UserDestroyer.new(admin).destroy(user) }
before do
user.stubs(:post_count).returns(1)
user.stubs(:first_post_created_at).returns(Time.zone.now)
end
it "should raise the right error" do
StaffActionLogger.any_instance.expects(:log_user_deletion).never
expect { destroy }.to raise_error(UserDestroyer::PostsExistError)
expect(user.reload.id).to be_present
end
end
context "when delete_posts is true" do
let(:destroy_opts) { { delete_posts: true } }
context "when staff deletes user" do
subject(:destroy) { UserDestroyer.new(admin).destroy(user, destroy_opts) }
include_examples "successfully destroy a user"
include_examples "email block list"
it "deletes the posts" do
destroy
expect(post.reload.deleted_at).not_to eq(nil)
expect(post.user_id).to eq(nil)
end
it "does not delete topics started by others in which the user has replies" do
destroy
expect(topic.reload.deleted_at).to eq(nil)
expect(topic.user_id).not_to eq(nil)
end
it "deletes topics started by the deleted user" do
spammer_topic = Fabricate(:topic, user: user)
Fabricate(:post, user: user, topic: spammer_topic)
destroy
expect(spammer_topic.reload.deleted_at).not_to eq(nil)
expect(spammer_topic.user_id).to eq(nil)
end
context "when delete_as_spammer is true" do
before { destroy_opts[:delete_as_spammer] = true }
it "approves reviewable flags" do
spammer_post = Fabricate(:post, user: user)
reviewable = PostActionCreator.inappropriate(admin, spammer_post).reviewable
expect(reviewable).to be_pending
destroy
reviewable.reload
expect(reviewable).to be_approved
end
it "rejects pending posts" do
post = Fabricate(:post, user: user)
reviewable =
Fabricate(
:reviewable,
type: "ReviewablePost",
target_type: "Post",
target_id: post.id,
created_by: Discourse.system_user,
target_created_by: user,
)
expect(reviewable).to be_pending
destroy
reviewable.reload
expect(reviewable).to be_rejected
end
it "approves reviewable flags on hidden posts" do
spammer_post = Fabricate(:post, user: user)
reviewable = PostActionCreator.inappropriate(admin, spammer_post).reviewable
spammer_post.update!(
hidden: true,
hidden_at: Time.zone.now,
hidden_reason_id: Post.hidden_reasons[:flag_threshold_reached],
)
expect(reviewable).to be_pending
destroy
expect(reviewable.reload).to be_approved
end
it "rejects queued posts" do
reviewable =
Fabricate(
:reviewable_queued_post,
created_by: Discourse.system_user,
target_created_by: user,
)
expect(reviewable).to be_pending
destroy
expect(reviewable.reload).to be_rejected
end
it "rejects the user's account reviewable when reviewable_id is a different reviewable" do
flag_reviewable =
PostActionCreator.inappropriate(admin, Fabricate(:post, user: user)).reviewable
account_reviewable = ReviewableUser.create_for(user)
destroy_opts[:reviewable_id] = flag_reviewable.id
destroy
expect(account_reviewable.reload).to be_rejected
end
it "leaves the user's account reviewable pending when reviewable_id is its id" do
account_reviewable = ReviewableUser.create_for(user)
destroy_opts[:reviewable_id] = account_reviewable.id
destroy
expect(account_reviewable.reload).to be_pending
end
end
end
context "when users deletes self" do
subject(:destroy) { UserDestroyer.new(user).destroy(user, destroy_opts) }
include_examples "successfully destroy a user"
include_examples "email block list"
it "deletes the posts" do
destroy
expect(post.reload.deleted_at).not_to eq(nil)
expect(post.user_id).to eq(nil)
end
end
end
end
context "when user was invited" do
it "should delete the invite of user" do
invite = Fabricate(:invite)
topic_invite = invite.topic_invites.create!(topic: Fabricate(:topic))
invited_group = invite.invited_groups.create!(group: Fabricate(:group))
user = Fabricate(:user)
user.user_emails.create!(email: invite.email)
UserDestroyer.new(admin).destroy(user)
expect(Invite.exists?(invite.id)).to eq(false)
expect(InvitedGroup.exists?(invited_group.id)).to eq(false)
expect(TopicInvite.exists?(topic_invite.id)).to eq(false)
end
it "should delete invites matching associated account emails" do
user = Fabricate(:user)
invite = Fabricate(:invite, email: "oauth@example.com")
UserAssociatedAccount.create!(
user: user,
provider_name: "google_oauth2",
provider_uid: "12345",
info: {
email: "oauth@example.com",
},
)
UserDestroyer.new(admin).destroy(user)
expect(Invite.exists?(invite.id)).to eq(false)
end
end
context "when user created category" do
let!(:topic) { Fabricate(:topic, user: user) }
let!(:first_post) { Fabricate(:post, user: user, topic: topic) }
let!(:second_post) { Fabricate(:post, user: user, topic: topic) }
let!(:category) { Fabricate(:category, user: user, topic_id: topic.id) }
it "changes author of first category post to system user and still deletes second post" do
UserDestroyer.new(admin).destroy(user, delete_posts: true)
expect(first_post.reload.deleted_at).to eq(nil)
expect(first_post.user_id).to eq(Discourse.system_user.id)
expect(second_post.reload.deleted_at).not_to eq(nil)
expect(second_post.user_id).to eq(nil)
end
end
context "when user has no posts, but user_stats table has post_count > 0" do
subject(:destroy) { UserDestroyer.new(user).destroy(user, delete_posts: false) }
let(:destroy_opts) { {} }
before do
# out of sync user_stat data shouldn't break UserDestroyer
user.user_stat.update_attribute(:post_count, 1)
end
include_examples "successfully destroy a user"
end
context "when user has deleted posts" do
let!(:deleted_post) { Fabricate(:post, user: user, deleted_at: 1.hour.ago) }
it "should mark the user's deleted posts as belonging to a nuked user" do
expect { UserDestroyer.new(admin).destroy(user) }.to change { User.count }.by(-1)
expect(deleted_post.reload.user_id).to eq(nil)
end
end
context "when user has no posts" do
context "when destroy succeeds" do
subject(:destroy) { UserDestroyer.new(admin).destroy(user) }
let(:destroy_opts) { {} }
include_examples "successfully destroy a user"
include_examples "email block list"
end
context "when destroy fails" do
subject(:destroy) { UserDestroyer.new(admin).destroy(user) }
it "should not log the action" do
user.stubs(:destroy).returns(false)
StaffActionLogger.any_instance.expects(:log_user_deletion).never
destroy
end
end
end
context "when user has posts with links" do
context "with external links" do
before do
@post = Fabricate(:post_with_external_links, user: user)
TopicLink.extract_from(@post)
end
it "doesn't add ScreenedUrl records by default" do
ScreenedUrl.expects(:watch).never
UserDestroyer.new(admin).destroy(user, delete_posts: true)
end
it "adds ScreenedUrl records when :block_urls is true" do
ScreenedUrl.expects(:watch).with(anything, anything, has_key(:ip_address)).at_least_once
UserDestroyer.new(admin).destroy(user, delete_posts: true, block_urls: true)
end
end
context "with internal links" do
before do
@post = Fabricate(:post_with_external_links, user: user)
TopicLink.extract_from(@post)
TopicLink.where(user: user).update_all(internal: true)
end
it "doesn't add ScreenedUrl records" do
ScreenedUrl.expects(:watch).never
UserDestroyer.new(admin).destroy(user, delete_posts: true, block_urls: true)
end
end
context "with oneboxed links" do
before do
@post = Fabricate(:post_with_youtube, user: user)
TopicLink.extract_from(@post)
end
it "doesn't add ScreenedUrl records" do
ScreenedUrl.expects(:watch).never
UserDestroyer.new(admin).destroy(user, delete_posts: true, block_urls: true)
end
end
end
context "with ip address screening" do
it "doesn't create screened_ip_address records by default" do
ScreenedIpAddress.expects(:watch).never
UserDestroyer.new(admin).destroy(user)
end
context "when block_ip is true" do
it "creates a new screened_ip_address record" do
ScreenedIpAddress.expects(:watch).with(user.ip_address).returns(stub_everything)
UserDestroyer.new(admin).destroy(user, block_ip: true)
end
it "creates two new screened_ip_address records when registration_ip_address is different than last ip_address" do
user.registration_ip_address = "12.12.12.12"
ScreenedIpAddress.expects(:watch).with(user.ip_address).returns(stub_everything)
ScreenedIpAddress
.expects(:watch)
.with(user.registration_ip_address)
.returns(stub_everything)
UserDestroyer.new(admin).destroy(user, block_ip: true)
end
end
end
context "when user created a category" do
let!(:category) { Fabricate(:category_with_definition, user: user) }
it "assigns the system user to the categories" do
UserDestroyer.new(admin).destroy(user, delete_posts: true)
expect(category.reload.user_id).to eq(Discourse.system_user.id)
expect(category.topic).to be_present
expect(category.topic.user_id).to eq(Discourse.system_user.id)
end
end
describe "Destroying a user with security key" do
let!(:security_key) { Fabricate(:user_security_key_with_random_credential, user: user) }
it "removes the security key" do
UserDestroyer.new(admin).destroy(user)
expect(UserSecurityKey.where(user_id: user.id).count).to eq(0)
end
end
describe "Destroying a user with a bookmark" do
let!(:bookmark) { Fabricate(:bookmark, user: user) }
it "removes the bookmark" do
UserDestroyer.new(admin).destroy(user)
expect(Bookmark.where(user_id: user.id).count).to eq(0)
end
end
describe "Destroying a user with a reviewable claimed topic" do
let!(:topic) { Fabricate(:topic) }
let!(:claimed_topic) { Fabricate(:reviewable_claimed_topic, user: user, topic: topic) }
it "removes the reviewable claimed topic" do
UserDestroyer.new(admin).destroy(user)
expect(ReviewableClaimedTopic.exists?(user_id: user.id)).to be false
end
end
context "when user liked things" do
before do
@topic = Fabricate(:topic, user: Fabricate(:user))
@post = Fabricate(:post, user: @topic.user, topic: @topic)
PostActionCreator.like(user, @post)
end
it "should destroy the like" do
expect { UserDestroyer.new(admin).destroy(user, delete_posts: true) }.to change {
PostAction.count
}.by(-1)
expect(@post.reload.like_count).to eq(0)
end
end
context "when user belongs to groups that grant trust level" do
let(:group) { Fabricate(:group, grant_trust_level: 4) }
before { group.add(user) }
it "can delete the user" do
d = UserDestroyer.new(admin)
expect { d.destroy(user) }.to change { User.count }.by(-1)
end
it "can delete the user if they have a manual locked trust level and have no email" do
user.update(manual_locked_trust_level: 3)
UserEmail.where(user: user).delete_all
user.reload
expect { UserDestroyer.new(admin).destroy(user) }.to change { User.count }.by(-1)
end
it "can delete the user if they were to fall into another trust level and have no email" do
g2 = Fabricate(:group, grant_trust_level: 1)
g2.add(user)
UserEmail.where(user: user).delete_all
user.reload
expect { UserDestroyer.new(admin).destroy(user) }.to change { User.count }.by(-1)
end
end
context "when user has staff action logs" do
before do
logger = StaffActionLogger.new(user)
logger.log_site_setting_change(
"site_description",
"Our friendly community",
"My favourite community",
)
logger.log_site_setting_change(
"site_description",
"Our friendly community",
"My favourite community",
details: "existing details",
)
end
it "should keep the staff action log and add the username" do
username = user.username
ids =
UserHistory.staff_action_records(Discourse.system_user, acting_user: username).map(&:id)
UserDestroyer.new(admin).destroy(user, delete_posts: true)
details = UserHistory.where(id: ids).map(&:details)
expect(details).to contain_exactly(
"\nuser_id: #{user.id}\nusername: #{username}",
"existing details\nuser_id: #{user.id}\nusername: #{username}",
)
end
end
context "when user got an email" do
let!(:email_log) { Fabricate(:email_log, user: user) }
it "does not delete the email log" do
expect { UserDestroyer.new(admin).destroy(user, delete_posts: true) }.to_not change {
EmailLog.count
}
end
end
end
end