From c5d37f18cb3f4d6212fb8f3e1c4e1e027f677ec5 Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Wed, 11 Sep 2019 16:32:44 +0200
Subject: [PATCH] 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
---
app/controllers/admin/accounts_controller.rb | 2 +-
.../admin/report_notes_controller.rb | 9 ++-
.../api/v1/admin/accounts_controller.rb | 2 +-
app/lib/activitypub/activity/delete.rb | 3 +-
app/models/account.rb | 1 +
app/models/admin/account_action.rb | 24 +++++--
app/models/form/account_batch.rb | 2 +-
app/models/form/status_batch.rb | 2 +-
app/models/report.rb | 1 +
app/models/status.rb | 4 ++
app/models/user.rb | 4 ++
app/services/block_domain_service.rb | 2 +-
app/services/remove_status_service.rb | 7 ++-
app/services/suspend_account_service.rb | 62 ++++++++++++++-----
app/services/unallow_domain_service.rb | 2 +-
app/workers/admin/suspension_worker.rb | 2 +-
lib/mastodon/accounts_cli.rb | 4 +-
lib/mastodon/domains_cli.rb | 2 +-
.../reported_statuses_controller_spec.rb | 2 +-
.../admin/statuses_controller_spec.rb | 2 +-
spec/models/form/status_batch_spec.rb | 4 +-
21 files changed, 98 insertions(+), 45 deletions(-)
diff --git a/app/controllers/admin/accounts_controller.rb b/app/controllers/admin/accounts_controller.rb
index 2fa1dfe5fc..68b6352f88 100644
--- a/app/controllers/admin/accounts_controller.rb
+++ b/app/controllers/admin/accounts_controller.rb
@@ -41,7 +41,7 @@ module Admin
def 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
end
diff --git a/app/controllers/admin/report_notes_controller.rb b/app/controllers/admin/report_notes_controller.rb
index bcb3f20260..b816c5b5d4 100644
--- a/app/controllers/admin/report_notes_controller.rb
+++ b/app/controllers/admin/report_notes_controller.rb
@@ -5,10 +5,10 @@ module Admin
before_action :set_report_note, only: [:destroy]
def create
- authorize ReportNote, :create?
+ authorize :report_note, :create?
@report_note = current_account.report_notes.new(resource_params)
- @report = @report_note.report
+ @report = @report_note.report
if @report_note.save
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')
else
- @report_notes = @report.notes.latest
- @report_history = @report.history
- @form = Form::StatusBatch.new
+ @report_notes = (@report.notes.latest + @report.history + @report.target_account.targeted_account_warnings.latest.custom).sort_by(&:created_at)
+ @form = Form::StatusBatch.new
render template: 'admin/reports/show'
end
diff --git a/app/controllers/api/v1/admin/accounts_controller.rb b/app/controllers/api/v1/admin/accounts_controller.rb
index c306180ca2..c35ea5ab25 100644
--- a/app/controllers/api/v1/admin/accounts_controller.rb
+++ b/app/controllers/api/v1/admin/accounts_controller.rb
@@ -58,7 +58,7 @@ class Api::V1::Admin::AccountsController < Api::BaseController
def 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
end
diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb
index 3450604629..dc9ff580c1 100644
--- a/app/lib/activitypub/activity/delete.rb
+++ b/app/lib/activitypub/activity/delete.rb
@@ -13,8 +13,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
def delete_person
lock_or_return("delete_in_progress:#{@account.id}") do
- SuspendAccountService.new.call(@account)
- @account.destroy!
+ SuspendAccountService.new.call(@account, reserve_username: false)
end
end
diff --git a/app/models/account.rb b/app/models/account.rb
index 8c9388b959..55fe53fae1 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -115,6 +115,7 @@ class Account < ApplicationRecord
:approved?,
:pending?,
:disabled?,
+ :unconfirmed_or_pending?,
:role,
:admin?,
:moderator?,
diff --git a/app/models/admin/account_action.rb b/app/models/admin/account_action.rb
index c7da8b52ce..b30a823699 100644
--- a/app/models/admin/account_action.rb
+++ b/app/models/admin/account_action.rb
@@ -83,19 +83,23 @@ class Admin::AccountAction
# A log entry is only interesting if the warning contains
# custom text from someone. Otherwise it's just noise.
+
log_action(:create, warning) if warning.text.present?
end
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)
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
@@ -141,6 +145,16 @@ class Admin::AccountAction
@report.status_ids if @report && include_statuses
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
@warning_preset ||= AccountWarningPreset.find(warning_preset_id) if warning_preset_id.present?
end
diff --git a/app/models/form/account_batch.rb b/app/models/form/account_batch.rb
index f1b7a45664..0b285fde92 100644
--- a/app/models/form/account_batch.rb
+++ b/app/models/form/account_batch.rb
@@ -69,6 +69,6 @@ class Form::AccountBatch
records = accounts.includes(:user)
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
diff --git a/app/models/form/status_batch.rb b/app/models/form/status_batch.rb
index e09cc2594e..c4943a7eab 100644
--- a/app/models/form/status_batch.rb
+++ b/app/models/form/status_batch.rb
@@ -35,7 +35,7 @@ class Form::StatusBatch
def delete_statuses
Status.where(id: status_ids).reorder(nil).find_each do |status|
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)
log_action :destroy, status
end
diff --git a/app/models/report.rb b/app/models/report.rb
index 1e707ff1c4..fb2e040ee8 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -59,6 +59,7 @@ class Report < ApplicationRecord
end
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)
end
diff --git a/app/models/status.rb b/app/models/status.rb
index 9cfaddcecb..471bb03b47 100644
--- a/app/models/status.rb
+++ b/app/models/status.rb
@@ -214,6 +214,10 @@ class Status < ApplicationRecord
!sensitive? && with_media?
end
+ def reported?
+ @reported ||= Report.where(target_account: account).unresolved.where('? = ANY(status_ids)', id).exists?
+ end
+
def emojis
return @emojis if defined?(@emojis)
diff --git a/app/models/user.rb b/app/models/user.rb
index 95f1d8fc5f..78b82a68f9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -171,6 +171,10 @@ class User < ApplicationRecord
confirmed? && approved? && !disabled? && !account.suspended?
end
+ def unconfirmed_or_pending?
+ !(confirmed? && approved?)
+ end
+
def inactive_message
!approved? ? :pending : super
end
diff --git a/app/services/block_domain_service.rb b/app/services/block_domain_service.rb
index 0ec6be5036..ae461abf24 100644
--- a/app/services/block_domain_service.rb
+++ b/app/services/block_domain_service.rb
@@ -53,7 +53,7 @@ class BlockDomainService < BaseService
def suspend_accounts!
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
diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb
index 685c1d4bff..f9352ed3d3 100644
--- a/app/services/remove_status_service.rb
+++ b/app/services/remove_status_service.rb
@@ -8,7 +8,8 @@ class RemoveStatusService < BaseService
# @param [Status] status
# @param [Hash] options
# @option [Boolean] :redraft
- # @options [Boolean] :original_removed
+ # @option [Boolean] :immediate
+ # @option [Boolean] :original_removed
def call(status, **options)
@payload = Oj.dump(event: :delete, payload: status.id.to_s)
@status = status
@@ -31,7 +32,7 @@ class RemoveStatusService < BaseService
remove_from_spam_check
remove_media
- @status.destroy!
+ @status.destroy! if @options[:immediate] || !@status.reported?
else
raise Mastodon::RaceConditionError
end
@@ -150,7 +151,7 @@ class RemoveStatusService < BaseService
end
def remove_media
- return if @options[:redraft]
+ return if @options[:redraft] || (!@options[:immediate] && @status.reported?)
@status.media_attachments.destroy_all
end
diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb
index 85da7e9210..ecc893931d 100644
--- a/app/services/suspend_account_service.rb
+++ b/app/services/suspend_account_service.rb
@@ -15,7 +15,6 @@ class SuspendAccountService < BaseService
favourites
follow_requests
list_accounts
- media_attachments
mute_relationships
muted_by_relationships
notifications
@@ -32,14 +31,26 @@ class SuspendAccountService < BaseService
targeted_reports
).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 [Hash] options
- # @option [Boolean] :including_user Remove the user record as well
- # @option [Boolean] :destroy Remove the account record instead of suspending
+ # @option [Boolean] :reserve_email Keep user record. Only applicable for local accounts
+ # @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)
@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!
purge_user!
@@ -60,27 +71,39 @@ class SuspendAccountService < BaseService
def purge_user!
return if !@account.local? || @account.user.nil?
- if @options[:including_user]
- @options[:destroy] = true if !@account.user_confirmed? || @account.user_pending?
- @account.user.destroy
- else
+ if @options[:reserve_email]
@account.user.disable!
@account.user.invites.where(uses: 0).destroy_all
+ else
+ @account.user.destroy
end
end
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|
- 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
associations_for_destruction.each do |association_name|
destroy_all(@account.public_send(association_name))
end
- @account.destroy if @options[:destroy]
+ @account.destroy unless @options[:reserve_username]
end
def purge_profile!
@@ -88,11 +111,13 @@ class SuspendAccountService < BaseService
# there is no point wasting time updating
# its values first
- return if @options[:destroy]
+ return unless @options[:reserve_username]
@account.silenced_at = nil
@account.suspended_at = @options[:suspended_at] || Time.now.utc
@account.locked = false
+ @account.memorial = false
+ @account.discoverable = false
@account.display_name = ''
@account.note = ''
@account.fields = []
@@ -100,6 +125,7 @@ class SuspendAccountService < BaseService
@account.followers_count = 0
@account.following_count = 0
@account.moved_to_account = nil
+ @account.trust_level = :untrusted
@account.avatar.destroy
@account.header.destroy
@account.save!
@@ -135,11 +161,15 @@ class SuspendAccountService < BaseService
Account.inboxes - delivery_inboxes
end
+ def reported_status_ids
+ @reported_status_ids ||= Report.where(target_account: @account).unresolved.pluck(:status_ids).flatten.uniq
+ end
+
def associations_for_destruction
- if @options[:destroy]
- ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
- else
+ if @options[:reserve_username]
ASSOCIATIONS_ON_SUSPEND
+ else
+ ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
end
end
end
diff --git a/app/services/unallow_domain_service.rb b/app/services/unallow_domain_service.rb
index d4387c1a19..bd1ad328da 100644
--- a/app/services/unallow_domain_service.rb
+++ b/app/services/unallow_domain_service.rb
@@ -3,7 +3,7 @@
class UnallowDomainService < BaseService
def call(domain_allow)
Account.where(domain: domain_allow.domain).find_each do |account|
- SuspendAccountService.new.call(account, destroy: true)
+ SuspendAccountService.new.call(account, reserve_username: false)
end
domain_allow.destroy
diff --git a/app/workers/admin/suspension_worker.rb b/app/workers/admin/suspension_worker.rb
index ae8b24d8c3..83c815efd7 100644
--- a/app/workers/admin/suspension_worker.rb
+++ b/app/workers/admin/suspension_worker.rb
@@ -6,6 +6,6 @@ class Admin::SuspensionWorker
sidekiq_options queue: 'pull'
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
diff --git a/lib/mastodon/accounts_cli.rb b/lib/mastodon/accounts_cli.rb
index b16bf2e389..a09a6ab040 100644
--- a/lib/mastodon/accounts_cli.rb
+++ b/lib/mastodon/accounts_cli.rb
@@ -185,7 +185,7 @@ module Mastodon
end
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)
end
@@ -239,7 +239,7 @@ module Mastodon
end
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
else
# Touch account even during dry run to avoid getting the account into the window again
diff --git a/lib/mastodon/domains_cli.rb b/lib/mastodon/domains_cli.rb
index c612c2d72f..8e52de1c35 100644
--- a/lib/mastodon/domains_cli.rb
+++ b/lib/mastodon/domains_cli.rb
@@ -42,7 +42,7 @@ module Mastodon
end
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
DomainBlock.where(domain: domain).destroy_all unless options[:dry_run]
diff --git a/spec/controllers/admin/reported_statuses_controller_spec.rb b/spec/controllers/admin/reported_statuses_controller_spec.rb
index bd146b7956..2a1598123c 100644
--- a/spec/controllers/admin/reported_statuses_controller_spec.rb
+++ b/spec/controllers/admin/reported_statuses_controller_spec.rb
@@ -47,7 +47,7 @@ describe Admin::ReportedStatusesController do
it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async)
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
diff --git a/spec/controllers/admin/statuses_controller_spec.rb b/spec/controllers/admin/statuses_controller_spec.rb
index 6b06343efb..d9690d83f2 100644
--- a/spec/controllers/admin/statuses_controller_spec.rb
+++ b/spec/controllers/admin/statuses_controller_spec.rb
@@ -65,7 +65,7 @@ describe Admin::StatusesController do
it 'removes a status' do
allow(RemovalWorker).to receive(:perform_async)
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
diff --git a/spec/models/form/status_batch_spec.rb b/spec/models/form/status_batch_spec.rb
index f9c58c90f8..68d84a7379 100644
--- a/spec/models/form/status_batch_spec.rb
+++ b/spec/models/form/status_batch_spec.rb
@@ -41,12 +41,12 @@ describe Form::StatusBatch do
it 'call RemovalWorker' do
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
it 'do not call RemovalWorker' do
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
--
GitLab