-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Add support for reversible suspensions through ActivityPub #14989
Conversation
9568cae
to
0a8a1d1
Compare
db01b5d
to
8d39c88
Compare
8d39c88
to
c204e8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to do a second review pass, but here are a few inline comments (most of them minor) for now.
More generally, I'm a bit worried about the suspension_origin
enum making the logic a bit more complicated and error-prone (but I also understand it's useful for not having to change how suspended accounts are looked up/checked).
Another thing I'm a bit worried about is the two instances not agreeing on the suspension status of some user, which could result in yet another occurrence of “ghost followers”.
def suspended_permanently? | ||
suspended? && deletion_request.nil? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly worried this logic could fall apart if at some point we (or an admin) end up deciding to cancel some deletion requests without unsuspending the user, but otherwise it seems fine.
def moved? | ||
!object.suspended? && object.moved? | ||
end | ||
|
||
def also_known_as? | ||
!object.also_known_as.empty? | ||
!object.suspended? && !object.also_known_as.empty? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if these could cause subtle issues in Move
handling or whatnot... but it's unlikely.
db/post_migrate/20201017234926_fill_account_suspension_origin.rb
Outdated
Show resolved
Hide resolved
e578ba8
to
34ab90c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do another review pass later today, but it seems mostly good. So far, my only worry is about returning 410 for temporary suspensions, as that might trigger deletion in some implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the inline comments:
DeleteAccountService#purge_profile!
needs to set suspension originBlockDomainService#suspend_accounts!
andBlockDomainService#process_retroactive_updates!
need to (un)set suspension originActivityPub::ProcessAccountService#create_account
needs to set suspension origin on auto_block
Also, I think we could/should federate silencing information as well, this would make some things less hackish. This definitely can be in another PR though.
@@ -42,7 +42,7 @@ def challenge_passed? | |||
end | |||
|
|||
def destroy_account! | |||
current_account.suspend! | |||
current_account.suspend!(origin: :local) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm thinking about that, a self-deletion being displayed as a suspension might be misleading and problematic. Though that's a permanent suspension so I guess it should be fine, just returning 410 with a generic “Gone” page.
f9f2fe2
to
bbf5724
Compare
I have removed the "suspended" profile design from the PR. Temporarily suspended profiles return HTTP 403. |
ade8152
to
f892b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic behind which response to return in case of suspension is a bit hard to follow but I don't really have a better idea on how to implement it right now.
Also, I'm not sure why you removed the special page about the account being suspended, it does seem helpful to me. In any case that can be added back in another PR.
I'd like to see some actual testing of the feature (I might do that later), but overall, the PR looks good to me.
f892b42
to
2da95f9
Compare
Follow-up to #14726
Suspension is expressed through boolean
suspended
attribute on the actor, and changes are distributed using the standardUpdate
activity. This means that handling suspended accounts works pretty much the same if you receive the event live or encounter the suspended account in the wild. Accounts that were suspended locally cannot be unsuspended remotely. On the other hand, we no longer ignoreUpdate
,Undo
,Reject
, andDelete
events for accounts that were suspended.