Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix join ratelimiter breaking profile updates and idempotency #8153

Merged
merged 6 commits into from
Aug 24, 2020
Merged

Fix join ratelimiter breaking profile updates and idempotency #8153

merged 6 commits into from
Aug 24, 2020

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Aug 24, 2020 •
edited
Loading

Fixes #8148 by not firing the ratelimiter if the join event isn't actually joining the user to the room (e.g. it's a profile change).

Fixes #8146 by not firing the ratelimiter if the join event is identical to the user's previous join event (in which case we don't actually send the event, to preserve idempotency on the join endpoints).

This fix is only targeting the ratelimiter on local joins, and not the one on remote joins, because we're sure the latter is only actually used to get a user in the room (afterwards we're already in that room and use the local one).

The main downside of this patch is that it doesn't save as many CPU cycles, and time, before ratelimiting as how it was done originally, but it looks to me like all that's done between the location we used to ratelimit and the new location is almost exclusively gathering data to perform these two checks, so it doesn't sound to me like we can do it any differently.

TODO:

  • Write a test

This was linked to issues Aug 24, 2020
Join endpoints are rate-limited even if the user has already joined the room #8146
Closed
Default rate limit on joins breaks profile picture and display name updates to propagate to rooms #8148
Closed
This was referenced Aug 24, 2020
Closed
Closed
@babolivier babolivier requested a review from a team August 24, 2020 16:02
@babolivier babolivier self-assigned this Aug 24, 2020
erikjohnston
erikjohnston approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

I think this works fine.

We could instead honour the ratelimit param, which would help with profile updates no longer working. However that breaks the flow for bots that send a join before every message, as currently the join ratelimiter doesn't honour the ratelimit override.

Since this is a hotfix I vote we land this as is

changelog.d/8153.bugfix Outdated Show resolved Hide resolved
@babolivier @erikjohnston
Update changelog.d/8153.bugfix
11786f4 
Co-authored-by: Erik Johnston <erik@matrix.org>
@babolivier babolivier merged commit 393a811 into release-v1.19.1 Aug 24, 2020
@babolivier babolivier deleted the babolivier/ratelimit_join_fix branch August 24, 2020 17:06
babolivier added a commit that referenced this pull request Aug 25, 2020
@babolivier
Merge tag 'v1.19.1rc1' into develop
6e1c64a 
Synapse 1.19.1rc1 (2020-08-25)
==============================

Bugfixes
--------

- Fix a bug introduced in v1.19.0 where appservices with ratelimiting disabled would still be ratelimited when joining rooms. ([\#8139](#8139))
- Fix a bug introduced in v1.19.0 that would cause e.g. profile updates to fail due to incorrect application of rate limits on join requests. ([\#8153](#8153))
@clokep clokep mentioned this pull request Aug 25, 2020
Merged
anoadragon453 added a commit that referenced this pull request Aug 26, 2020
@anoadragon453
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/use…
7affcd0 
…r_param_ui_auth

* 'develop' of github.com:matrix-org/synapse: (369 commits)
  Add functions to `MultiWriterIdGen` used by events stream (#8164)
  Do not allow send_nonmember_event to be called with shadow-banned users. (#8158)
  Changelog fixes
  1.19.1rc1
  Make StreamIdGen `get_next` and `get_next_mult` async  (#8161)
  Wording fixes to 'name' user admin api filter (#8163)
  Fix missing double-backtick in RST document
  Search in columns 'name' and 'displayname' in the admin users endpoint (#7377)
  Add type hints for state. (#8140)
  Stop shadow-banned users from sending non-member events. (#8142)
  Allow capping a room's retention policy (#8104)
  Add healthcheck for default localhost 8008 port on /health endpoint. (#8147)
  Fix flaky shadow-ban tests. (#8152)
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
  Do not apply ratelimiting on joins to appservices (#8139)
  Don't fail /submit_token requests on incorrect session ID if request_token_inhibit_3pid_errors is turned on (#7991)
  Do not apply ratelimiting on joins to appservices (#8139)
  Micro-optimisations to get_auth_chain_ids (#8132)
  Allow denying or shadow banning registrations via the spam checker (#8034)
  Stop shadow-banned users from sending invites. (#8095)
  ...
anoadragon453 added a commit that referenced this pull request Aug 26, 2020
@anoadragon453
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/pas…
3f4e350 
…sword_reset_confirmation

* 'develop' of github.com:matrix-org/synapse: (22 commits)
  Update the test federation client to handle streaming responses (#8130)
  Do not propagate profile changes of shadow-banned users into rooms. (#8157)
  Make SlavedIdTracker.advance have same interface as MultiWriterIDGenerator (#8171)
  Convert simple_select_one and simple_select_one_onecol to async (#8162)
  Fix rate limiting unit tests. (#8167)
  Add functions to `MultiWriterIdGen` used by events stream (#8164)
  Do not allow send_nonmember_event to be called with shadow-banned users. (#8158)
  Changelog fixes
  1.19.1rc1
  Make StreamIdGen `get_next` and `get_next_mult` async  (#8161)
  Wording fixes to 'name' user admin api filter (#8163)
  Fix missing double-backtick in RST document
  Search in columns 'name' and 'displayname' in the admin users endpoint (#7377)
  Add type hints for state. (#8140)
  Stop shadow-banned users from sending non-member events. (#8142)
  Allow capping a room's retention policy (#8104)
  Add healthcheck for default localhost 8008 port on /health endpoint. (#8147)
  Fix flaky shadow-ban tests. (#8152)
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
  Do not apply ratelimiting on joins to appservices (#8139)
  ...
anoadragon453 added a commit that referenced this pull request Aug 26, 2020
@anoadragon453
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/imp…
ac9d346 
…rove_test_times

* 'develop' of github.com:matrix-org/synapse: (160 commits)
  Update the test federation client to handle streaming responses (#8130)
  Do not propagate profile changes of shadow-banned users into rooms. (#8157)
  Make SlavedIdTracker.advance have same interface as MultiWriterIDGenerator (#8171)
  Convert simple_select_one and simple_select_one_onecol to async (#8162)
  Fix rate limiting unit tests. (#8167)
  Add functions to `MultiWriterIdGen` used by events stream (#8164)
  Do not allow send_nonmember_event to be called with shadow-banned users. (#8158)
  Changelog fixes
  1.19.1rc1
  Make StreamIdGen `get_next` and `get_next_mult` async  (#8161)
  Wording fixes to 'name' user admin api filter (#8163)
  Fix missing double-backtick in RST document
  Search in columns 'name' and 'displayname' in the admin users endpoint (#7377)
  Add type hints for state. (#8140)
  Stop shadow-banned users from sending non-member events. (#8142)
  Allow capping a room's retention policy (#8104)
  Add healthcheck for default localhost 8008 port on /health endpoint. (#8147)
  Fix flaky shadow-ban tests. (#8152)
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
  Do not apply ratelimiting on joins to appservices (#8139)
  ...
anoadragon453 added a commit that referenced this pull request Aug 26, 2020
@anoadragon453
Merge branch 'release-v1.19.1' of github.com:matrix-org/synapse into …
586c8c0 
…anoa/amorgan.xyz

* 'release-v1.19.1' of github.com:matrix-org/synapse: (197 commits)
  Changelog fixes
  1.19.1rc1
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
  Do not apply ratelimiting on joins to appservices (#8139)
  Changelog changes
  1.19.0
  More changelog tweaks
  More changelog tweaks
  Remove unwanted changelog line
  1.19.0rc1
  Convert the roommember database to async/await. (#8070)
  Convert devices database to async/await. (#8069)
  Add type hints to handlers.message and events.builder (#8067)
  Convert account data, device inbox, and censor events databases to async/await (#8063)
  Convert appservice, group server, profile and more databases to async (#8066)
  Fix typing for notifier (#8064)
  Convert tags and metrics databases to async/await (#8062)
  Converts event_federation and registration databases to async/await (#8061)
  Add comment explaining cast
  Update changelog.d/8051.misc
  ...
@csett86 csett86 mentioned this pull request Aug 29, 2020
Closed
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit 'b79d69796' into anoa/dinsic_release_1_21_x
385ab7a 
* commit 'b79d69796':
  1.19.1rc1
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Half-Shot Half-Shot Half-Shot left review comments

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees

@babolivier babolivier

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@babolivier @Half-Shot @erikjohnston