Add an admin API to manage ratelimit for a specific user

[dklimpel]

Fixes: #6286

A few questions:

1. Is the RoomWorkerStore the right class for get_ratelimit_for_user, set_ratelimit_for_user, and delete_ratelimit_for_user?
   Should I move it to RegistrationWorkerStore or anywhere else?
2. Is the best solution to delete a custom ratelimit with http DELETE. Or do you prefer to set to null with http POST?
3. When get status with http GET and no custom ratelimit is set, do you prefer return an empty dict ({}) or values with null ( "messages_per_second": null, "burst_count": null)

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[erkinalp]

Spec counterpart: matrix-org/matrix-spec-proposals#3059

[erikjohnston]

A few questions:

1. Is the RoomWorkerStore the right class for get_ratelimit_for_user, set_ratelimit_for_user, and delete_ratelimit_for_user?
   Should I move it to RegistrationWorkerStore or anywhere else?

We should probably move it out to its own file, but its probably fine where it is.

2. Is the best solution to delete a custom ratelimit with http DELETE. Or do you prefer to set to null with http POST?

DELETE seems like a sensible choice

3. When get status with http GET and no custom ratelimit is set, do you prefer return an empty dict ({}) or values with null ( "messages_per_second": null, "burst_count": null)

So, looking at the code values or either 0 or null can mean "disable ratelimit for user", so we shouldn't use values with null to mean no ratelimitng is set.

[erikjohnston]

Looks good otherwise!

[erikjohnston]

Can we document that 0 or null mean ratelimiting is disabled? Or we should replace 0 with null (or vice versa) before returning the values, for consistency sake?

[dklimpel]

If i read the code correctly, then null is better for deactivating, as it breaks the processing sooner!?
But I do not like that a null value disable ratelimit. 0 is more understandable.
What's your opinion?

Should I update if not override.messages_per_second: to if not override.messages_per_second or override.messages_per_second == 0:?

synapse/synapse/handlers/_base.py

Lines 102 to 108 in 5b26899

	# Check if there is a per user override in the DB.
	override = await self.store.get_ratelimit_for_user(user_id)
	if override:
	# If overridden with a null Hz then ratelimiting has been entirely
	# disabled for the user
	if not override.messages_per_second:
	return

[erikjohnston]

if not override.messages_per_second: and if not override.messages_per_second or override.messages_per_second == 0: are equivalent here (since both None and 0 are falsey in python), which is why we've ended up in a situation where 0 and None mean the same thing anyway.

[erikjohnston]

I think the only thing we might want to do here is to change null to 0 (or vice versa) when we return the values from the API, for consistency. I don't really mind though.

(The fact that we treat 0 and null the same is a bit awful, but untangling that may be a bit of a mess)

[clokep]

You can probably just cast to an int to always return 0?

[dklimpel]

Yes, I can do it.
Should I do the conversion from None to 0 only in admin API or direct in store get_ratelimit_for_user?

[erikjohnston]

I'd probably do it in the API for now, just to avoid unnecessarily changing things used by lots of places.

updated

[erikjohnston]

I think this LGTM, but will wait to merge until we resolve the outstanding thread about 0 vs null

[erikjohnston]

Sorry this has taken so long to get through review!