Change deletes to preserve soft-deleted statuses in unresolved reports (#11805)

Change all account actions except "none" to resolve all unresolved reports

Refactor `SuspendAccountService` to be more readable
This commit is contained in:
Eugen Rochko 2019-09-11 16:32:44 +02:00 committed by GitHub
parent 4fe127664b
commit c5d37f18cb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
21 changed files with 98 additions and 45 deletions

View file

@ -41,7 +41,7 @@ module Admin
def reject def reject
authorize @account.user, :reject? authorize @account.user, :reject?
SuspendAccountService.new.call(@account, including_user: true, destroy: true, skip_distribution: true) SuspendAccountService.new.call(@account, reserve_email: false, reserve_username: false)
redirect_to admin_pending_accounts_path redirect_to admin_pending_accounts_path
end end

View file

@ -5,10 +5,10 @@ module Admin
before_action :set_report_note, only: [:destroy] before_action :set_report_note, only: [:destroy]
def create def create
authorize ReportNote, :create? authorize :report_note, :create?
@report_note = current_account.report_notes.new(resource_params) @report_note = current_account.report_notes.new(resource_params)
@report = @report_note.report @report = @report_note.report
if @report_note.save if @report_note.save
if params[:create_and_resolve] if params[:create_and_resolve]
@ -26,9 +26,8 @@ module Admin
redirect_to admin_report_path(@report), notice: I18n.t('admin.report_notes.created_msg') redirect_to admin_report_path(@report), notice: I18n.t('admin.report_notes.created_msg')
else else
@report_notes = @report.notes.latest @report_notes = (@report.notes.latest + @report.history + @report.target_account.targeted_account_warnings.latest.custom).sort_by(&:created_at)
@report_history = @report.history @form = Form::StatusBatch.new
@form = Form::StatusBatch.new
render template: 'admin/reports/show' render template: 'admin/reports/show'
end end

View file

@ -58,7 +58,7 @@ class Api::V1::Admin::AccountsController < Api::BaseController
def reject def reject
authorize @account.user, :reject? authorize @account.user, :reject?
SuspendAccountService.new.call(@account, including_user: true, destroy: true, skip_distribution: true) SuspendAccountService.new.call(@account, reserve_email: false, reserve_username: false)
render json: @account, serializer: REST::Admin::AccountSerializer render json: @account, serializer: REST::Admin::AccountSerializer
end end

View file

@ -13,8 +13,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
def delete_person def delete_person
lock_or_return("delete_in_progress:#{@account.id}") do lock_or_return("delete_in_progress:#{@account.id}") do
SuspendAccountService.new.call(@account) SuspendAccountService.new.call(@account, reserve_username: false)
@account.destroy!
end end
end end

View file

@ -115,6 +115,7 @@ class Account < ApplicationRecord
:approved?, :approved?,
:pending?, :pending?,
:disabled?, :disabled?,
:unconfirmed_or_pending?,
:role, :role,
:admin?, :admin?,
:moderator?, :moderator?,

View file

@ -83,19 +83,23 @@ class Admin::AccountAction
# A log entry is only interesting if the warning contains # A log entry is only interesting if the warning contains
# custom text from someone. Otherwise it's just noise. # custom text from someone. Otherwise it's just noise.
log_action(:create, warning) if warning.text.present? log_action(:create, warning) if warning.text.present?
end end
def process_reports! def process_reports!
return if report_id.blank? # If we're doing "mark as resolved" on a single report,
# then we want to keep other reports open in case they
# contain new actionable information.
#
# Otherwise, we will mark all unresolved reports about
# the account as resolved.
authorize(report, :update?) reports.each { |report| authorize(report, :update?) }
if type == 'none' reports.each do |report|
log_action(:resolve, report) log_action(:resolve, report)
report.resolve!(current_account) report.resolve!(current_account)
else
Report.where(target_account: target_account).unresolved.update_all(action_taken: true, action_taken_by_account_id: current_account.id)
end end
end end
@ -141,6 +145,16 @@ class Admin::AccountAction
@report.status_ids if @report && include_statuses @report.status_ids if @report && include_statuses
end end
def reports
@reports ||= begin
if type == 'none' && with_report?
[report]
else
Report.where(target_account: target_account).unresolved
end
end
end
def warning_preset def warning_preset
@warning_preset ||= AccountWarningPreset.find(warning_preset_id) if warning_preset_id.present? @warning_preset ||= AccountWarningPreset.find(warning_preset_id) if warning_preset_id.present?
end end

View file

@ -69,6 +69,6 @@ class Form::AccountBatch
records = accounts.includes(:user) records = accounts.includes(:user)
records.each { |account| authorize(account.user, :reject?) } records.each { |account| authorize(account.user, :reject?) }
.each { |account| SuspendAccountService.new.call(account, including_user: true, destroy: true, skip_distribution: true) } .each { |account| SuspendAccountService.new.call(account, reserve_email: false, reserve_username: false) }
end end
end end

View file

@ -35,7 +35,7 @@ class Form::StatusBatch
def delete_statuses def delete_statuses
Status.where(id: status_ids).reorder(nil).find_each do |status| Status.where(id: status_ids).reorder(nil).find_each do |status|
status.discard status.discard
RemovalWorker.perform_async(status.id, redraft: false) RemovalWorker.perform_async(status.id, immediate: true)
Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true) Tombstone.find_or_create_by(uri: status.uri, account: status.account, by_moderator: true)
log_action :destroy, status log_action :destroy, status
end end

View file

@ -59,6 +59,7 @@ class Report < ApplicationRecord
end end
def resolve!(acting_account) def resolve!(acting_account)
RemovalWorker.push_bulk(Status.with_discarded.discarded.where(id: status_ids).pluck(:id)) { |status_id| [status_id, { immediate: true }] }
update!(action_taken: true, action_taken_by_account_id: acting_account.id) update!(action_taken: true, action_taken_by_account_id: acting_account.id)
end end

View file

@ -214,6 +214,10 @@ class Status < ApplicationRecord
!sensitive? && with_media? !sensitive? && with_media?
end end
def reported?
@reported ||= Report.where(target_account: account).unresolved.where('? = ANY(status_ids)', id).exists?
end
def emojis def emojis
return @emojis if defined?(@emojis) return @emojis if defined?(@emojis)

View file

@ -171,6 +171,10 @@ class User < ApplicationRecord
confirmed? && approved? && !disabled? && !account.suspended? confirmed? && approved? && !disabled? && !account.suspended?
end end
def unconfirmed_or_pending?
!(confirmed? && approved?)
end
def inactive_message def inactive_message
!approved? ? :pending : super !approved? ? :pending : super
end end

View file

@ -53,7 +53,7 @@ class BlockDomainService < BaseService
def suspend_accounts! def suspend_accounts!
blocked_domain_accounts.without_suspended.reorder(nil).find_each do |account| blocked_domain_accounts.without_suspended.reorder(nil).find_each do |account|
SuspendAccountService.new.call(account, suspended_at: @domain_block.created_at) SuspendAccountService.new.call(account, reserve_username: true, suspended_at: @domain_block.created_at)
end end
end end

View file

@ -8,7 +8,8 @@ class RemoveStatusService < BaseService
# @param [Status] status # @param [Status] status
# @param [Hash] options # @param [Hash] options
# @option [Boolean] :redraft # @option [Boolean] :redraft
# @options [Boolean] :original_removed # @option [Boolean] :immediate
# @option [Boolean] :original_removed
def call(status, **options) def call(status, **options)
@payload = Oj.dump(event: :delete, payload: status.id.to_s) @payload = Oj.dump(event: :delete, payload: status.id.to_s)
@status = status @status = status
@ -31,7 +32,7 @@ class RemoveStatusService < BaseService
remove_from_spam_check remove_from_spam_check
remove_media remove_media
@status.destroy! @status.destroy! if @options[:immediate] || !@status.reported?
else else
raise Mastodon::RaceConditionError raise Mastodon::RaceConditionError
end end
@ -150,7 +151,7 @@ class RemoveStatusService < BaseService
end end
def remove_media def remove_media
return if @options[:redraft] return if @options[:redraft] || (!@options[:immediate] && @status.reported?)
@status.media_attachments.destroy_all @status.media_attachments.destroy_all
end end

View file

@ -15,7 +15,6 @@ class SuspendAccountService < BaseService
favourites favourites
follow_requests follow_requests
list_accounts list_accounts
media_attachments
mute_relationships mute_relationships
muted_by_relationships muted_by_relationships
notifications notifications
@ -32,14 +31,26 @@ class SuspendAccountService < BaseService
targeted_reports targeted_reports
).freeze ).freeze
# Suspend an account and remove as much of its data as possible # Suspend or remove an account and remove as much of its data
# as possible. If it's a local account and it has not been confirmed
# or never been approved, then side effects are skipped and both
# the user and account records are removed fully. Otherwise,
# it is controlled by options.
# @param [Account] # @param [Account]
# @param [Hash] options # @param [Hash] options
# @option [Boolean] :including_user Remove the user record as well # @option [Boolean] :reserve_email Keep user record. Only applicable for local accounts
# @option [Boolean] :destroy Remove the account record instead of suspending # @option [Boolean] :reserve_username Keep account record
# @option [Boolean] :skip_side_effects Side effects are ActivityPub and streaming API payloads
# @option [Time] :suspended_at Only applicable when :reserve_username is true
def call(account, **options) def call(account, **options)
@account = account @account = account
@options = options @options = { reserve_username: true, reserve_email: true }.merge(options)
if @account.local? && @account.user_unconfirmed_or_pending?
@options[:reserve_email] = false
@options[:reserve_username] = false
@options[:skip_side_effects] = true
end
reject_follows! reject_follows!
purge_user! purge_user!
@ -60,27 +71,39 @@ class SuspendAccountService < BaseService
def purge_user! def purge_user!
return if !@account.local? || @account.user.nil? return if !@account.local? || @account.user.nil?
if @options[:including_user] if @options[:reserve_email]
@options[:destroy] = true if !@account.user_confirmed? || @account.user_pending?
@account.user.destroy
else
@account.user.disable! @account.user.disable!
@account.user.invites.where(uses: 0).destroy_all @account.user.invites.where(uses: 0).destroy_all
else
@account.user.destroy
end end
end end
def purge_content! def purge_content!
distribute_delete_actor! if @account.local? && !@options[:skip_distribution] distribute_delete_actor! if @account.local? && !@options[:skip_side_effects]
@account.statuses.reorder(nil).find_in_batches do |statuses| @account.statuses.reorder(nil).find_in_batches do |statuses|
BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:destroy]) statuses.reject! { |status| reported_status_ids.include?(status.id) } if @options[:reserve_username]
BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:skip_side_effects])
end
@account.media_attachments.reorder(nil).find_each do |media_attachment|
next if @options[:reserve_username] && reported_status_ids.include?(media_attachment.status_id)
media_attachment.destroy
end
@account.polls.reorder(nil).find_each do |poll|
next if @options[:reserve_username] && reported_status_ids.include?(poll.status_id)
poll.destroy
end end
associations_for_destruction.each do |association_name| associations_for_destruction.each do |association_name|
destroy_all(@account.public_send(association_name)) destroy_all(@account.public_send(association_name))
end end
@account.destroy if @options[:destroy] @account.destroy unless @options[:reserve_username]
end end
def purge_profile! def purge_profile!
@ -88,11 +111,13 @@ class SuspendAccountService < BaseService
# there is no point wasting time updating # there is no point wasting time updating
# its values first # its values first
return if @options[:destroy] return unless @options[:reserve_username]
@account.silenced_at = nil @account.silenced_at = nil
@account.suspended_at = @options[:suspended_at] || Time.now.utc @account.suspended_at = @options[:suspended_at] || Time.now.utc
@account.locked = false @account.locked = false
@account.memorial = false
@account.discoverable = false
@account.display_name = '' @account.display_name = ''
@account.note = '' @account.note = ''
@account.fields = [] @account.fields = []
@ -100,6 +125,7 @@ class SuspendAccountService < BaseService
@account.followers_count = 0 @account.followers_count = 0
@account.following_count = 0 @account.following_count = 0
@account.moved_to_account = nil @account.moved_to_account = nil
@account.trust_level = :untrusted
@account.avatar.destroy @account.avatar.destroy
@account.header.destroy @account.header.destroy
@account.save! @account.save!
@ -135,11 +161,15 @@ class SuspendAccountService < BaseService
Account.inboxes - delivery_inboxes Account.inboxes - delivery_inboxes
end end
def reported_status_ids
@reported_status_ids ||= Report.where(target_account: @account).unresolved.pluck(:status_ids).flatten.uniq
end
def associations_for_destruction def associations_for_destruction
if @options[:destroy] if @options[:reserve_username]
ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
else
ASSOCIATIONS_ON_SUSPEND ASSOCIATIONS_ON_SUSPEND
else
ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
end end
end end
end end

View file

@ -3,7 +3,7 @@
class UnallowDomainService < BaseService class UnallowDomainService < BaseService
def call(domain_allow) def call(domain_allow)
Account.where(domain: domain_allow.domain).find_each do |account| Account.where(domain: domain_allow.domain).find_each do |account|
SuspendAccountService.new.call(account, destroy: true) SuspendAccountService.new.call(account, reserve_username: false)
end end
domain_allow.destroy domain_allow.destroy

View file

@ -6,6 +6,6 @@ class Admin::SuspensionWorker
sidekiq_options queue: 'pull' sidekiq_options queue: 'pull'
def perform(account_id, remove_user = false) def perform(account_id, remove_user = false)
SuspendAccountService.new.call(Account.find(account_id), including_user: remove_user) SuspendAccountService.new.call(Account.find(account_id), reserve_username: true, reserve_email: !remove_user)
end end
end end

View file

@ -185,7 +185,7 @@ module Mastodon
end end
say("Deleting user with #{account.statuses_count} statuses, this might take a while...") say("Deleting user with #{account.statuses_count} statuses, this might take a while...")
SuspendAccountService.new.call(account, including_user: true) SuspendAccountService.new.call(account, reserve_email: false)
say('OK', :green) say('OK', :green)
end end
@ -239,7 +239,7 @@ module Mastodon
end end
if [404, 410].include?(code) if [404, 410].include?(code)
SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run] SuspendAccountService.new.call(account, reserve_username: false) unless options[:dry_run]
1 1
else else
# Touch account even during dry run to avoid getting the account into the window again # Touch account even during dry run to avoid getting the account into the window again

View file

@ -42,7 +42,7 @@ module Mastodon
end end
processed, = parallelize_with_progress(scope) do |account| processed, = parallelize_with_progress(scope) do |account|
SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run] SuspendAccountService.new.call(account, reserve_username: false, skip_side_effects: true) unless options[:dry_run]
end end
DomainBlock.where(domain: domain).destroy_all unless options[:dry_run] DomainBlock.where(domain: domain).destroy_all unless options[:dry_run]

View file

@ -47,7 +47,7 @@ describe Admin::ReportedStatusesController do
it 'removes a status' do it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async) allow(RemovalWorker).to receive(:perform_async)
subject.call subject.call
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false) expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, immediate: true)
end end
end end

View file

@ -65,7 +65,7 @@ describe Admin::StatusesController do
it 'removes a status' do it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async) allow(RemovalWorker).to receive(:perform_async)
subject.call subject.call
expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, redraft: false) expect(RemovalWorker).to have_received(:perform_async).with(status_ids.first, immediate: true)
end end
end end

View file

@ -41,12 +41,12 @@ describe Form::StatusBatch do
it 'call RemovalWorker' do it 'call RemovalWorker' do
form.save form.save
expect(RemovalWorker).to have_received(:perform_async).with(status.id, redraft: false) expect(RemovalWorker).to have_received(:perform_async).with(status.id, immediate: true)
end end
it 'do not call RemovalWorker' do it 'do not call RemovalWorker' do
form.save form.save
expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id, redraft: false) expect(RemovalWorker).not_to have_received(:perform_async).with(another_status.id, immediate: true)
end end
end end
end end