Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change account suspensions to be reversible by default #14726

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Sep 3, 2020

Fix #6954

Instead of instantly deleting all account-owned data when suspending, hide it in the API layer. Instead of returning a HTTP 410 error for accounts that have been suspended, return an empty account with a new attribute suspended. Add a "suspended" account view to the web UI. Do not immediately send Delete activities to other servers.

This allows suspensions to be losslessly reversible, which helps with accidents, mistakes and appeals. It also allows providing the suspended user with a backup for GDPR compliance. Schedule real deletion after 30 days, while allowing admins to still override and speed up that process.

image

Federation-wise, this PR does not include any changes. This means that a locally suspended account continues to be visible on other servers like nothing happened for 30 days (but obviously cannot post or interact with anyone anymore). Federation support needs to be added in a future PR.


Extracted into separate PRs:

@Gargron Gargron force-pushed the feature-reversible-suspensions branch 14 times, most recently from 9b342f0 to f9ad22d Compare September 9, 2020 23:29
@Gargron Gargron force-pushed the feature-reversible-suspensions branch 4 times, most recently from b4b13ca to 7f4a778 Compare September 12, 2020 18:25
@Gargron Gargron marked this pull request as ready for review September 12, 2020 18:29
@Gargron Gargron force-pushed the feature-reversible-suspensions branch 3 times, most recently from 813767c to 37034d9 Compare September 12, 2020 22:49
@ClearlyClaire ClearlyClaire self-requested a review September 13, 2020 18:22
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly ok with this (except for the missing AP support and #14775 but those can be fixed in later PR) except for concerns about code being split from SuspendAccountService into DeleteAccountService (which is a good idea nevertheless):

  • SuspendAccountService doesn't set suspended_at anymore, which seems counterintuitive and may not be fine with all callers
  • I believe some parts of the code are calling SuspendAccountService when they should really be calling DeleteAccountService:
    • lib/mastodon/domains_cli.rb
    • lib/mastodon/accounts_cli.rb
    • app/lib/activitypub/activity/delete.rb
    • app/models/form/account_batch.rb

app/controllers/admin/accounts_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin/accounts_controller.rb Outdated Show resolved Hide resolved
app/policies/user_policy.rb Outdated Show resolved Hide resolved
config/locales/simple_form.en.yml Show resolved Hide resolved
return unless @options[:reserve_username]

@account.silenced_at = nil
@account.suspended_at = @options[:suspended_at] || Time.now.utc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about SuspendService not setting suspended_at anymore… this basically requires the caller to mark the account as suspended. Maybe this is fine? I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An issue I have run into before is I suspended someone and then I wanted to cancel it. I unsuspended quickly, but when the background job kicked in, it reset the account back to suspended. This is something I wanted to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that still be an issue with the service removing stuff from TLs? And wouldn't that be fixed anyway by UnsuspendAccountService nullifying the suspended_at attribute?

app/services/suspend_account_service.rb Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-reversible-suspensions branch from 37034d9 to a4d6b00 Compare September 14, 2020 12:58
@Gargron Gargron force-pushed the feature-reversible-suspensions branch from a4d6b00 to f9c5407 Compare September 14, 2020 20:30
@Gargron Gargron merged commit ed099d8 into master Sep 15, 2020
@Gargron Gargron deleted the feature-reversible-suspensions branch September 15, 2020 12:38
@Gargron
Copy link
Member Author

Gargron commented Sep 15, 2020

🦀 🦀 🦀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspending an account should not delete it
2 participants