When site banning a federated user, also remove their content from our local communities. #4464
Conversation
dessalines
commented
Feb 20, 2024
•
dessalines
commented
- This works by:
- When doing a site ban, find all posts and comments to local communities
- Send a federated community ban action for each local comm.
- This also removes their content in the apub receive code.
- Adding back in federated community ban api tests.
- Adding in two more api tests for site bans.
- Fixes [Bug]: Content removal for local communities does not federate out when banning remote users #4118
communities. - This works by: - Before a site ban, find all posts and comments to local communities - Send a federated community ban action for each local comm. - This also removes their content in the apub receive code. - Adding back in federated community ban api tests. - Adding in two more api tests for site bans. - Fixes #4118
dessalines
changed the title
|
Not sure if I understand the logic entirely, please correct me if I don't. Ban/unban are in the same function, just with a bool specifying whether to ban or unban, so the steps below are the same for both banning and unbanning.
As 6. is not currently accepted on receiving instances, 5. is intended as an intermediate/alternative solution until proper handling for receiving 6. exists, which would also be able to deal with future communities on the local instance, which were created after the ban was issued. This does not currently ban those users from communities locally, nor write this to the community ban mod log locally. Is this intended as a workaround to allow a site unban to be processed without effectively unbanning the user from all local communities, just telling other instances the user would be unbanned in them? If this is not intended as a workaround for allowing community unbans to be undone from previous implicit community bans, shouldn't those bans also exist locally and also be in the local mod log for consistency? |
|
Close, but your step 6 only applies to our local communities. Yes it bans them locally. This has nothing to do with workarounds for unbanning. I don't think modlog entries are necessary, you'll already see the site ban. |
| reason: &Option<String>, | ||
| remove_data: &Option<bool>, | ||
| expires: &Option<i64>, |
There was a problem hiding this comment.
If its a temporary site ban then the user would get banned from local communities and never unbanned, right? Also if ban = false, any existing community bans for the user would be removed. Seems safer to remove these two params and use it only in case of permanent site ban.
There was a problem hiding this comment.
Also instead of running this logic on the instance where the ban originates, we could move it to the code which receives remote site bans. Then it would only have to ban the user from all known communities of that instance. This would reduce the amount of activities that have to get federated. However it wont work if a community gets federated later. Though with this implementation, the ban also wont federate to instances where the local communities dont have any followers.
Either way the core problem wont be solved. We only federate bans via activities, so if that doesnt get delivered or an instance fetches the community later, it wont know about the ban. A proper solution would be similar to #4475, federating a collection of users who are banned from each instance/community so that it can also be synchronized later. And for site bans we need to store them in a separate table as table site_ban (instance_id, person_id), to properly track which instance banned the user. All rather complex, so we can use this PR as a temporary, imperfect solution.
There was a problem hiding this comment.
If its a temporary site ban then the user would get banned from local communities and never unbanned, right?
If you look below, the community ban form also includes that expires. So the site ban, and the community bans, would all expire at the same time.
Also if ban = false, any existing community bans for the user would be removed. Seems safer to remove these two params and use it only in case of permanent site ban.
That's true, but I think its still a good idea to federate community unbans. If we didn't run this code for unbans, then if someone was permabanned, then unbanned, their community bans would still be there.
Its a limitation, since community mods might have wanted that user banned, but I think overall its still better than leaving all the bans in place.
Also instead of running this logic on the instance where the ban originates, we could move it to the code which receives remote site bans. Then it would only have to ban the user from all known communities of that instance. This would reduce the amount of activities that have to get federated. However it wont work if a community gets federated later. Though with this implementation, the ban also wont federate to instances where the local communities dont have any followers.
That's up to you, but this code currently does work, and the API tests are passing. I don't think it makes much sense to federate out local site bans.
crates/api/src/lib.rs
Outdated
| // Ignore all errors for these | ||
| CommunityPersonBan::ban(&mut context.pool(), &community_user_ban_form) | ||
| .await | ||
| .with_lemmy_type(LemmyErrorType::CommunityUserAlreadyBanned) |
There was a problem hiding this comment.
No reason to add an error message if errors are ignored. Same below.
api_tests/src/post.spec.ts
Outdated
| expect(banAlphaOnBeta.banned).toBe(true); | ||
|
|
||
| // The beta site ban should NOT be federated to alpha | ||
| let alphaPerson2 = (await getSite(alphaUserHttp)).my_user?.local_user_view |
There was a problem hiding this comment.
I believe you can simply do .my_user!, no need to be so verbose with explicit throw.
