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

Allow Configurable Rate Limiting Per AS #1175

Merged
merged 6 commits into from
Oct 20, 2016
Merged

Allow Configurable Rate Limiting Per AS #1175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b54d51
Allow Configurable Rate Limiting Per AS
Oct 18, 2016
1b17d1a
Use real AS object by passing it through the requester
Oct 20, 2016
8bfd01f
flake8
Oct 20, 2016
f09db23
as_user->app_service, less redundant comments, better positioned comm…
Oct 20, 2016
07caa74
Closing brace on following line
Oct 20, 2016
6fdd319
Style
Oct 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion synapse/appservice/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class ApplicationService(object):
NS_LIST = [NS_USERS, NS_ALIASES, NS_ROOMS]

def __init__(self, token, url=None, namespaces=None, hs_token=None,
sender=None, id=None, protocols=None):
sender=None, id=None, protocols=None, rate_limited=True):
self.token = token
self.url = url
self.hs_token = hs_token
Expand All @@ -95,6 +95,8 @@ def __init__(self, token, url=None, namespaces=None, hs_token=None,
else:
self.protocols = set()

self.rate_limited = rate_limited

def _check_namespaces(self, namespaces):
# Sanity check that it is of the form:
# {
Expand Down Expand Up @@ -234,5 +236,8 @@ def is_exclusive_alias(self, alias):
def is_exclusive_room(self, room_id):
return self._is_exclusive(ApplicationService.NS_ROOMS, room_id)

def is_rate_limited(self):
return self.rate_limited

def __str__(self):
return "ApplicationService: %s" % (self.__dict__,)
6 changes: 6 additions & 0 deletions synapse/config/appservice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ def _load_appservice(hostname, as_info, config_filename):
user = UserID(localpart, hostname)
user_id = user.to_string()

# Rate limiting for users of this AS is on by default (excludes sender)
rate_limited = True
if isinstance(as_info.get("rate_limited"), bool):
rate_limited = as_info.get("rate_limited")

# namespace checks
if not isinstance(as_info.get("namespaces"), dict):
raise KeyError("Requires 'namespaces' object.")
Expand Down Expand Up @@ -155,4 +160,5 @@ def _load_appservice(hostname, as_info, config_filename):
sender=user_id,
id=as_info["id"],
protocols=protocols,
rate_limited=rate_limited
)
14 changes: 14 additions & 0 deletions synapse/handlers/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,24 @@ def ratelimit(self, requester):
time_now = self.clock.time()
user_id = requester.user.to_string()

# Disable rate limiting of users belonging to any AS that is configured
# not to be rate limited in its registration file (rate_limited: true|false).
# The AS user itself is never rate limited.

app_service = self.store.get_app_service_by_user_id(user_id)
if app_service is not None:
return # do not ratelimit app service senders

should_rate_limit = True

for service in self.store.get_app_services():
if service.is_interested_in_user(user_id):
Copy link
Member

Choose a reason for hiding this comment

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

This check looks like it could be relatively expensive and is on a vaguely hot path. (Its also wrong if multiple AS's register non-exclusive interest in it).

I'd rather we did this by checking if the authenticated thing actually doing the sending was an AS. We can probably do this by adding a field to the requester type when we authenticate the sender in auth module and checking that field here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I'll make some changes.

should_rate_limit = service.is_rate_limited()
break
Copy link
Member

Choose a reason for hiding this comment

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

You can probably compact the lot of this and remove should_rate_limit entirely:

for service in self.store.get_app_services():
    if service.is_interested_in_user(user_id) and not service.is_rate_limited():
        return

But I defer to Erik's view on whether this is clearer / what he would prefer.


if not should_rate_limit:
return

allowed, time_allowed = self.ratelimiter.send_message(
user_id, time_now,
msg_rate_hz=self.hs.config.rc_messages_per_second,
Expand Down