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

Fix bug where remote suspension causes local instance to remove remote follows #27588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShadowJonathan
Copy link
Contributor

This does not have an issue number, but I noticed this bug while looking at the comment.

The comment presumes that this code will only be run when the suspension comes locally, but I've observed the code being triggered when a remote account gets suspended remotely, causing the local server to issue follow rejects to that account.

According to the comment, this is not intended, and any (temporary) local suspension will effectively remove remote follows.

@ShadowJonathan ShadowJonathan changed the title Fix bug where remote suspension causes local to remote remote follows Fix bug where remote suspension causes local instance to remove remote follows Oct 27, 2023
@ShadowJonathan
Copy link
Contributor Author

The bug happens like follows;

Ergo, suspending a local account gets remote servers to send Reject activities to that account's follows.

@renchap renchap added the activitypub Protocol-related changes, federation label Oct 27, 2023
@ClearlyClaire
Copy link
Contributor

This indeed sounds like a bug worth fixing, but I don't think this should be done before ensuring users are offered to and effectively able to sever relationships with suspended users. This is not currently possible from the web interface.

@ShadowJonathan
Copy link
Contributor Author

Ah, you want to fix the frontend first before this?

@ClearlyClaire
Copy link
Contributor

Ah, you want to fix the frontend first before this?

Yes! Also, I need to check whether that's just a front-end thing or if the backend prevents from severing the relationships.

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Nov 2, 2023

I think it was just never necessary, as this code is always triggered on every suspension, so it just simply wasn't ever possible to still be following a suspended user, if the user would get a "this user is suspended" message.

I haven't seen any code which prevents following/unfollowing a suspended user, but I missed this code first time when looking for what happens upon suspension, so....

Btw, I don't know the right files to edit the front-end, so if someone would want to submit a PR to unblock this, or send a PR to pull it into this branch, that'd be appreciated.

@ClearlyClaire
Copy link
Contributor

Looking into it the backend does not prevent unfollowing or blocking suspended accounts, which is a good thing. However, it refuses returning relationships with suspended accounts (see app/controllers/api/v1/accounts/relationships_controller.rb).

This means the UI can't know you are following them or they are following you, and thus cannot present you with the relevant options. This is an easy change, but this will require further change to the Web UI and perhaps other clients as well to properly handle suspended users with relationships without e.g. prompting you to try to follow a suspended account.

@ShadowJonathan
Copy link
Contributor Author

@ClearlyClaire with #27667 merged, could this PR proceed?

ClearlyClaire
ClearlyClaire previously approved these changes Nov 13, 2023
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Dec 26, 2023

@renchap could this PR get some extra eyes after claire's approval last month? We got bitten by this again today

@renchap
Copy link
Member

renchap commented Dec 27, 2023

@ClearlyClaire approved the PR so I guess this is fine, I will let her merge it.

@ClearlyClaire ClearlyClaire dismissed their stale review December 29, 2023 15:08

Needs more consideration on the backend side

@ClearlyClaire
Copy link
Contributor

Sorry, dismissed my review because I need to think more about what happens when you're followed by some remote suspended user. Merging this would cause remote suspended users to still get your posts, both public and followers-only.

@mjankowski
Copy link
Contributor

Does the recent introduction of the severed relationship stuff make this one any more/less reviewable at this point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants