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

Stop Auth methods from polling the config on every req. #7420

Merged
merged 6 commits into from
May 6, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/7420.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent methods in `synapse.handlers.auth` from polling the homeserver config every request.
34 changes: 22 additions & 12 deletions synapse/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ def __init__(self, hs):
register_cache("cache", "token_cache", self.token_cache)

self._account_validity = hs.config.account_validity
self._track_appservice_user_ips = self.hs.config.track_appservice_user_ips
self._macaroon_secret_key = self.hs.config.macaroon_secret_key
self._server_notices_mxid = self.hs.config.server_notices_mxid
self._hs_disabled = self.hs.config.hs_disabled
self._limit_usage_by_mau = self.hs.config.limit_usage_by_mau
self._hs_disabled_message = self.hs.config.hs_disabled_message
self._admin_contact = self.hs.config.admin_contact
self._limit_usage_by_mau = self.hs.config.limit_usage_by_mau
self._mau_limits_reserved_threepids = (
self.hs.config.mau_limits_reserved_threepids
)
self._max_mau_value = self.hs.config.max_mau_value
Copy link
Member

Choose a reason for hiding this comment

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

can we factor out a separate class rather than add all these fields to this big class?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you mean? Moving the methods that use these to a new class or putting all these config options in a new inline class?

Copy link
Member

Choose a reason for hiding this comment

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

move the methods to a new class. dunno how easy that is

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not too bad to move, I just need to update all the references to it.

I could start with just having Auth.check_auth_blocking call the new method though.

Or I could just go ahead and update all the references. My plan is:

  • Create a new class AuthBlocking
  • It just handles this method
  • Have to add a build_auth_blocking method to the hs in addition to build_auth.

Before I get too far into any of this, does this sound sane or should we back out?

Copy link
Member

Choose a reason for hiding this comment

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

I could start with just having Auth.check_auth_blocking call the new method though.

sounds sensible for now

* Create a new class `AuthBlocking`
* It just handles this method

yes

* Have to add a `build_auth_blocking` method to the hs in addition to `build_auth`.

I'd have Auth.__init__ build the new object

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately there's quite a few tests that changed the hs's config after the hs had already been created, which need fixing up.


@defer.inlineCallbacks
def check_from_context(self, room_version: str, event, context, do_sig_check=True):
Expand Down Expand Up @@ -191,7 +203,7 @@ def get_user_by_req(
opentracing.set_tag("authenticated_entity", user_id)
opentracing.set_tag("appservice_id", app_service.id)

if ip_addr and self.hs.config.track_appservice_user_ips:
if ip_addr and self._track_appservice_user_ips:
yield self.store.insert_client_ip(
user_id=user_id,
access_token=access_token,
Expand Down Expand Up @@ -454,7 +466,7 @@ def validate_macaroon(self, macaroon, type_string, user_id):
# access_tokens include a nonce for uniqueness: any value is acceptable
v.satisfy_general(lambda c: c.startswith("nonce = "))

v.verify(macaroon, self.hs.config.macaroon_secret_key)
v.verify(macaroon, self._macaroon_secret_key)

def _verify_expiry(self, caveat):
prefix = "time < "
Expand Down Expand Up @@ -685,20 +697,20 @@ def check_auth_blocking(self, user_id=None, threepid=None, user_type=None):
# Never fail an auth check for the server notices users or support user
# This can be a problem where event creation is prohibited due to blocking
if user_id is not None:
if user_id == self.hs.config.server_notices_mxid:
if user_id == self._server_notices_mxid:
return
if (yield self.store.is_support_user(user_id)):
return

if self.hs.config.hs_disabled:
if self._hs_disabled:
raise ResourceLimitError(
403,
self.hs.config.hs_disabled_message,
self._hs_disabled_message,
errcode=Codes.RESOURCE_LIMIT_EXCEEDED,
admin_contact=self.hs.config.admin_contact,
admin_contact=self._admin_contact,
limit_type=LimitBlockingTypes.HS_DISABLED,
)
if self.hs.config.limit_usage_by_mau is True:
if self._limit_usage_by_mau is True:
assert not (user_id and threepid)

# If the user is already part of the MAU cohort or a trial user
Expand All @@ -713,21 +725,19 @@ def check_auth_blocking(self, user_id=None, threepid=None, user_type=None):
elif threepid:
# If the user does not exist yet, but is signing up with a
# reserved threepid then pass auth check
if is_threepid_reserved(
self.hs.config.mau_limits_reserved_threepids, threepid
):
if is_threepid_reserved(self._mau_limits_reserved_threepids, threepid):
return
elif user_type == UserTypes.SUPPORT:
# If the user does not exist yet and is of type "support",
# allow registration. Support users are excluded from MAU checks.
return
# Else if there is no room in the MAU bucket, bail
current_mau = yield self.store.get_monthly_active_count()
if current_mau >= self.hs.config.max_mau_value:
if current_mau >= self._max_mau_value:
raise ResourceLimitError(
403,
"Monthly Active User Limit Exceeded",
admin_contact=self.hs.config.admin_contact,
admin_contact=self._admin_contact,
errcode=Codes.RESOURCE_LIMIT_EXCEEDED,
limit_type=LimitBlockingTypes.MONTHLY_ACTIVE_USER,
)