Allow marking users as shadow banned #8028

clokep · 2020-08-04T11:46:48Z

Shadow banned users are able to interact with the CS API (and receive "200 OK" responses), but the state changes aren't stored in the room.

It is worth noting that this PR doesn't give a way to mark users as shadow banned, it would need to be done manually in the database. There are follow-ups that will help here.

The majority of this PR is by @erikjohnston, I've fixed up some formatting and added tests.

clokep · 2020-08-06T14:30:26Z

tests/rest/client/v1/test_rooms.py

+        self.other_user_id = self.register_user("otheruser", "pass")
+        self.other_access_token = self.login("otheruser", "pass")
+
+    def test_invite(self):


Looks like these tests have failed once or twice intermittently? I have no idea why though. They're timing out.

(Originally mentioned this on #8034 (comment))

I still need to look deeper into this.

richvdh

hrm, a few bits and bobs here.

I have a concern though. We are making up REST responses deep in the handler layer, where it is impossible to know which REST endpoint led to that path, and even where it is possible to figure it out, we'll forget about updating these codepaths when we decide to update the shape of the REST response.

We should be faking up the REST bodies close to where we are building the regular REST bodies. it may be appropriate to do so by catching ShadowBanErrors, in which case we should document each function that can raise a ShadowBanError and trace them up the call stack.

changelog.d/8028.feature

synapse/api/errors.py

synapse/handlers/register.py

synapse/handlers/room.py

synapse/replication/http/register.py

synapse/storage/databases/main/registration.py

synapse/storage/databases/main/schema/delta/58/09shadow_ban.sql

Co-authored-by: Richard van der Hoff <[email protected]>

clokep · 2020-08-10T12:27:40Z

I have a concern though. We are making up REST responses deep in the handler layer, where it is impossible to know which REST endpoint led to that path, and even where it is possible to figure it out, we'll forget about updating these codepaths when we decide to update the shape of the REST response.

We should be faking up the REST bodies close to where we are building the regular REST bodies. it may be appropriate to do so by catching ShadowBanErrors, in which case we should document each function that can raise a ShadowBanError and trace them up the call stack.

I agree that this will probably be clearer! I'll re-worker this. I think the current approach was chosen to avoid a bunch of try-excepts where the except essentially converts the ShadowBanError into a SynapeError, but having the returned body close to the REST layer makes sense.

clokep · 2020-08-10T14:43:50Z

I agree that this will probably be clearer! I'll re-worker this. I think the current approach was chosen to avoid a bunch of try-excepts where the except essentially converts the ShadowBanError into a SynapeError, but having the returned body close to the REST layer makes sense.

Looking more deeply at this, I think it is still the proper thing to do, but it does bubble up through a lot of methods.

richvdh · 2020-08-11T17:02:34Z

I believe this fixes matrix-org/matrix-spec-proposals#789 (or at least makes most of it irrelevant)

clokep · 2020-08-11T18:23:03Z

I believe this is ready for another review! Sorry that this grew quite a bit.

Note that I wonder if we should add similar guards to send_nonmember_event. It doesn't seem like any of the places it is called right now would be useful to have it, but it seems like it should make the same checks as create_and_send_nonmember_event.

richvdh

I suspect we need some logging when we do a "fake" response, partly so that we can see if we're seeing requests from shadow-banned users, but also because otherwise we're going to be confused when a user's requests unexpectedly do nothing.

richvdh · 2020-08-14T09:43:49Z

synapse/handlers/room.py

@@ -1171,8 +1190,25 @@ async def shutdown_room(
        if not await self.store.get_room(room_id):
            raise NotFoundError("Unknown room id %s" % (room_id,))

+        # Check if the user is shadow-banned before any mutations occur.


given this can only be called by an admin user, I think it might be a bit redundant to check the shadow-ban.

richvdh · 2020-08-14T09:50:28Z

synapse/handlers/directory.py

+
+            ShadowBanError: if the requester is shadow-banned.


is this coming from _update_canonical_alias? should maybe just ignore the exception if so, as we do with AuthError.

also: would be nice to stick the Raises docstring on _update_canonical_alias, even though it's internal.

richvdh

I'm afraid I'm struggling to follow all of this, and I'm not convinced that all of the code flows have been considered. What happens, for example, if a shadow-banned account is deactivated?

It might be easier to break this up: first land the changes to get the shadow_banned flag into Requester in one PR. Then add a raise ShadowBanError in one place and chase it up through the code, considering the correct behaviour on each path.

Sorry I know that's a load of work but I feel like this is currently a feature which will only work with one very specific set of configuration settings.

richvdh · 2020-08-14T10:01:43Z

synapse/handlers/room.py

+
+        Raises:
+            ShadowBanError if the requester is shadow-banned.


Again: what is the code path by which this happens? Is it just the final PL send? we should maybe ignore the exception there rather than allow it to propagate? or bail out sooner?

richvdh · 2020-08-14T10:54:18Z

synapse/handlers/room_member.py

@@ -281,6 +288,31 @@ async def update_membership(
        content: Optional[dict] = None,
        require_consent: bool = True,
    ) -> Tuple[str, int]:
+        """Update a user's membership in a room.


I note that this is used by auto-join rooms when registering. Does that mean that any server that has auto-join rooms will 500 on registration for a shadow-banned user?

This change was already merged in #8095, which I think makes it clearer that this only affects invites, so it shouldn't cause an issue for auto-join rooms. (Unless I'm missing what the concern was here.)

richvdh · 2020-08-14T11:07:58Z

synapse/handlers/profile.py

+        # Since the requester can get overwritten below, check if the real
+        # requester is shadow-banned.
+        if requester.shadow_banned:
+            # We randomly sleep a bit just to annoy the requester a bit.
+            await self.clock.sleep(random.randint(1, 10))
+            raise ShadowBanError()
+


I'm not sure if this is necessary. Shouldn't we let shadow-banned users change their displaynames if they want? It won't make it into any rooms thanks to the check in update_membership.

Hm....I was under the impression it would still end up in the room, but I think you're right. I'll need to test this.

So the changes to update_membership won't catch this because those only apply to invite requests.

clokep · 2020-08-14T12:39:28Z

It might be easier to break this up: first land the changes to get the shadow_banned flag into Requester in one PR. Then add a raise ShadowBanError in one place and chase it up through the code, considering the correct behaviour on each path.

This sounds like a tractable way to move forward with this. I suspect it would cleaner to start with a new pull request, but would be curious of your thoughts here!

richvdh · 2020-08-14T13:00:01Z

I suspect it would cleaner to start with a new pull request

so do I...

clokep · 2020-08-24T19:27:02Z

This has been split out into individual parts, so I'm going to close this.

clokep added 4 commits August 3, 2020 13:30

Support shadow banning users.

17549a2

Add tests.

b9402e3

Fix a typo.

190eca1

Add a changelog entry.

fc7bbd2

clokep requested a review from a team August 4, 2020 12:05

clokep mentioned this pull request Aug 5, 2020

Allow denying or shadow banning registrations via the spam checker #8034

Merged

clokep added 2 commits August 6, 2020 07:05

Merge branch 'develop' into clokep/shadow-banned

d55f499

Fix name of database variables.

58aad73

clokep commented Aug 6, 2020

View reviewed changes

richvdh suggested changes Aug 7, 2020

View reviewed changes

clokep and others added 2 commits August 7, 2020 10:45

Clarify comments and changelog.

c858898

Co-authored-by: Richard van der Hoff <[email protected]>

Move super call to the top of the function and fix docstrings / types.

1d2ebf8

Add additional comments.

c002070

clokep added 2 commits August 11, 2020 08:40

Move whether the user was shadow banned to the requester.

acd63f8

Create fake responses closer to the real responses.

739be9e

clokep added 3 commits August 11, 2020 13:37

Lint.

505fde2

Merge branch 'develop' into clokep/shadow-banned

4b23b68

Expand comments and add a test.

5adcc56

clokep requested a review from richvdh August 11, 2020 18:22

richvdh suggested changes Aug 14, 2020

View reviewed changes

clokep mentioned this pull request Aug 14, 2020

Add a shadow-banned flag to users #8092

Merged

clokep marked this pull request as draft August 14, 2020 17:28

clokep mentioned this pull request Aug 14, 2020

Stop shadow-banned users from sending invites. #8095

Merged

This was referenced Aug 20, 2020

Stop shadow-banned users from sending non-member events. #8142

Merged

Do not propagate profile changes of shadow-banned users into rooms #8157

Merged

clokep closed this Aug 24, 2020

clokep deleted the clokep/shadow-banned branch August 24, 2020 19:27

richvdh mentioned this pull request Jan 18, 2021

Ability to shadowban users matrix-org/matrix-spec-proposals#789

Closed

clokep mentioned this pull request Jan 22, 2021

Add an admin API for shadow-banning users. #9209

Merged

t3chguy mentioned this pull request May 18, 2022

Ability to shadow ban users element-hq/element-web#6976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow marking users as shadow banned #8028

Allow marking users as shadow banned #8028

clokep commented Aug 4, 2020 •

edited

Loading

clokep Aug 6, 2020

clokep Aug 12, 2020

richvdh left a comment

clokep commented Aug 10, 2020

clokep commented Aug 10, 2020

richvdh commented Aug 11, 2020

clokep commented Aug 11, 2020

richvdh left a comment

richvdh Aug 14, 2020

richvdh Aug 14, 2020

richvdh Aug 14, 2020

richvdh left a comment

richvdh Aug 14, 2020

richvdh Aug 14, 2020

clokep Aug 20, 2020

richvdh Aug 14, 2020

clokep Aug 20, 2020

clokep Aug 24, 2020

clokep commented Aug 14, 2020

richvdh commented Aug 14, 2020

clokep commented Aug 24, 2020

Allow marking users as shadow banned #8028

Allow marking users as shadow banned #8028

Conversation

clokep commented Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

clokep commented Aug 10, 2020

clokep commented Aug 10, 2020

richvdh commented Aug 11, 2020

clokep commented Aug 11, 2020

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Aug 14, 2020

richvdh commented Aug 14, 2020

clokep commented Aug 24, 2020

clokep commented Aug 4, 2020 •

edited

Loading