From ea068d6f3cd5ed1bc9a39b2fd43e19d6d40f18da Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 10:49:14 +0100 Subject: [PATCH 1/7] fix bug where preserved threepid user comes to sign up and server is mau blocked --- synapse/api/auth.py | 10 +++++++++- synapse/handlers/register.py | 3 ++- synapse/rest/client/v1_only/register.py | 6 +++++- synapse/rest/client/v2_alpha/register.py | 5 +++++ tests/api/test_auth.py | 17 +++++++++++++++++ 5 files changed, 38 insertions(+), 3 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index a7e3f7a7ac..9c207b9537 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -775,7 +775,7 @@ class Auth(object): ) @defer.inlineCallbacks - def check_auth_blocking(self, user_id=None): + def check_auth_blocking(self, user_id=None, threepid=None): """Checks if the user should be rejected for some external reason, such as monthly active user limiting or global disable flag @@ -806,6 +806,14 @@ class Auth(object): is_trial = yield self.store.is_trial_user(user_id) if is_trial: return + elif threepid: + # If the user does not exist yet, but is signing up with a + # reserved threepid then pass auth check + for tp in self.hs.config.mau_limits_reserved_threepids: + if (threepid['medium'] == tp['medium'] + and threepid['address'] == tp['address']): + 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: diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index f03ee1476b..1e53f2c635 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -125,6 +125,7 @@ class RegistrationHandler(BaseHandler): guest_access_token=None, make_guest=False, admin=False, + threepid=None, ): """Registers a new client on the server. @@ -145,7 +146,7 @@ class RegistrationHandler(BaseHandler): RegistrationError if there was a problem registering. """ - yield self.auth.check_auth_blocking() + yield self.auth.check_auth_blocking(threepid=threepid) password_hash = None if password: password_hash = yield self.auth_handler().hash(password) diff --git a/synapse/rest/client/v1_only/register.py b/synapse/rest/client/v1_only/register.py index 5e99cffbcb..2c7bbcb171 100644 --- a/synapse/rest/client/v1_only/register.py +++ b/synapse/rest/client/v1_only/register.py @@ -281,11 +281,15 @@ class RegisterRestServlet(ClientV1RestServlet): register_json["user"].encode("utf-8") if "user" in register_json else None ) + threepid = None + if session[LoginType.EMAIL_IDENTITY]: + threepid = session["threepidCreds"] handler = self.handlers.registration_handler (user_id, token) = yield handler.register( localpart=desired_user_id, - password=password + password=password, + threepid=threepid, ) if session[LoginType.EMAIL_IDENTITY]: diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 2f64155d13..45113e5386 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -395,11 +395,16 @@ class RegisterRestServlet(RestServlet): if desired_username is not None: desired_username = desired_username.lower() + threepid = None + if auth_result: + threepid = auth_result.get(LoginType.EMAIL_IDENTITY) + (registered_user_id, _) = yield self.registration_handler.register( localpart=desired_username, password=new_password, guest_access_token=guest_access_token, generate_token=False, + threepid=threepid, ) # remember that we've now registered that user account, and with diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 54e396d19d..f65a27e5f1 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -467,6 +467,23 @@ class AuthTestCase(unittest.TestCase): ) yield self.auth.check_auth_blocking() + @defer.inlineCallbacks + def test_reserved_threepid(self): + self.hs.config.limit_usage_by_mau = True + self.hs.config.max_mau_value = 1 + threepid = {'medium': 'email', 'address': 'reserved@server.com'} + unknown_threepid = {'medium': 'email', 'address': 'unreserved@server.com'} + self.hs.config.mau_limits_reserved_threepids = [threepid] + + yield self.store.register(user_id='user1', token="123", password_hash=None) + with self.assertRaises(ResourceLimitError): + yield self.auth.check_auth_blocking() + + with self.assertRaises(ResourceLimitError): + yield self.auth.check_auth_blocking(threepid=unknown_threepid) + + yield self.auth.check_auth_blocking(threepid=threepid) + @defer.inlineCallbacks def test_hs_disabled(self): self.hs.config.hs_disabled = True From 3d6aa06577e170a8af5f0e19a3102d4210730c8f Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 10:56:37 +0100 Subject: [PATCH 2/7] news fragemnt --- changelog.d/3777.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3777.bugfix diff --git a/changelog.d/3777.bugfix b/changelog.d/3777.bugfix new file mode 100644 index 0000000000..93b5b8fa57 --- /dev/null +++ b/changelog.d/3777.bugfix @@ -0,0 +1 @@ +Fix fix bug where preserved threepid user comes to sign up and server is mau blocked From 09f3cf1a7ef0c533d052a5c87257503b710093c6 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 15:42:51 +0100 Subject: [PATCH 3/7] ensure post registration auth checks do not fail erroneously --- synapse/api/auth.py | 7 ++----- synapse/rest/client/v1_only/register.py | 4 ++++ synapse/rest/client/v2_alpha/register.py | 4 ++++ synapse/storage/monthly_active_users.py | 15 ++++++++++++++- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 9c207b9537..6a97c06110 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -809,11 +809,8 @@ class Auth(object): elif threepid: # If the user does not exist yet, but is signing up with a # reserved threepid then pass auth check - for tp in self.hs.config.mau_limits_reserved_threepids: - if (threepid['medium'] == tp['medium'] - and threepid['address'] == tp['address']): - return - + if is_threepid_reserved(threepid): + 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: diff --git a/synapse/rest/client/v1_only/register.py b/synapse/rest/client/v1_only/register.py index 2c7bbcb171..95873e03d5 100644 --- a/synapse/rest/client/v1_only/register.py +++ b/synapse/rest/client/v1_only/register.py @@ -291,6 +291,10 @@ class RegisterRestServlet(ClientV1RestServlet): password=password, threepid=threepid, ) + # Necessary due to auth checks prior to the threepid being + # written to the db + if self.store.is_threepid_reserved(threepid): + self.store.upsert_monthly_active_user(registered_user_id) if session[LoginType.EMAIL_IDENTITY]: logger.debug("Binding emails %s to %s" % ( diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 45113e5386..f22b7577ea 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -406,6 +406,10 @@ class RegisterRestServlet(RestServlet): generate_token=False, threepid=threepid, ) + # Necessary due to auth checks prior to the threepid being + # written to the db + if self.store.is_threepid_reserved(threepid): + self.store.upsert_monthly_active_user(registered_user_id) # remember that we've now registered that user account, and with # what user ID (since the user may not have specified) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index d178f5c5ba..173867c4b1 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -36,7 +36,6 @@ class MonthlyActiveUsersStore(SQLBaseStore): @defer.inlineCallbacks def initialise_reserved_users(self, threepids): - # TODO Why can't I do this in init? store = self.hs.get_datastore() reserved_user_list = [] @@ -220,3 +219,17 @@ class MonthlyActiveUsersStore(SQLBaseStore): yield self.upsert_monthly_active_user(user_id) elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY: yield self.upsert_monthly_active_user(user_id) + + def is_threepid_reserved(self, threepid): + """Check the threepid against the reserved threepid config + Args: + threepid(dict) - The threepid to test for + Returns: + boolean Is the threepid undertest reserved_user + """ + for tp in self.hs.config.mau_limits_reserved_threepids: + if (threepid['medium'] == tp['medium'] + and threepid['address'] == tp['address']): + return True + else: + return False From a796bdd35efbd17c4a1fa48e1245f6d57403b232 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 15:57:33 +0100 Subject: [PATCH 4/7] typo --- changelog.d/3777.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/3777.bugfix b/changelog.d/3777.bugfix index 93b5b8fa57..46efc543a7 100644 --- a/changelog.d/3777.bugfix +++ b/changelog.d/3777.bugfix @@ -1 +1 @@ -Fix fix bug where preserved threepid user comes to sign up and server is mau blocked +Fix bug where preserved threepid user comes to sign up and server is mau blocked From e8e540630ec96d6ed856fb143dbb62b91e15084d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 16:09:15 +0100 Subject: [PATCH 5/7] fix reference to is_threepid_reserved --- synapse/api/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 6a97c06110..a36fb6b3bd 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -809,7 +809,7 @@ class Auth(object): 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(threepid): + if self.store.is_threepid_reserved(threepid): return # Else if there is no room in the MAU bucket, bail current_mau = yield self.store.get_monthly_active_count() From 0b01281e77aee7e69925f36dbb6a798772a98a45 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 17:11:11 +0100 Subject: [PATCH 6/7] move threepid checker to config, add missing yields --- synapse/api/auth.py | 13 +++++++++++-- synapse/config/server.py | 17 +++++++++++++++++ synapse/rest/client/v1_only/register.py | 7 ++++--- synapse/rest/client/v2_alpha/register.py | 5 +++-- synapse/storage/monthly_active_users.py | 14 -------------- tests/utils.py | 6 ++++++ 6 files changed, 41 insertions(+), 21 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index a36fb6b3bd..a89687f420 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -26,6 +26,7 @@ import synapse.types from synapse import event_auth from synapse.api.constants import EventTypes, JoinRules, Membership from synapse.api.errors import AuthError, Codes, ResourceLimitError +from synapse.config.server import is_threepid_reserved from synapse.types import UserID from synapse.util.caches import CACHE_SIZE_FACTOR, register_cache from synapse.util.caches.lrucache import LruCache @@ -782,6 +783,11 @@ class Auth(object): Args: user_id(str|None): If present, checks for presence against existing MAU cohort + threepid(dict|None): If present, checks for presence against configured + reserved threepid. Used in cases where the user is trying register + with a MAU blocked server, normally they would be rejected but their + threepid is on the reserved list. user_id and + threepid should never be set at the same time. """ # Never fail an auth check for the server notices users @@ -797,6 +803,10 @@ class Auth(object): limit_type=self.hs.config.hs_disabled_limit_type ) if self.hs.config.limit_usage_by_mau is True: + + if user_id and threepid: + logger.warn("Called with both user_id and threepid, this shoudn't happen") + # If the user is already part of the MAU cohort or a trial user if user_id: timestamp = yield self.store.user_last_seen_monthly_active(user_id) @@ -809,14 +819,13 @@ class Auth(object): elif threepid: # If the user does not exist yet, but is signing up with a # reserved threepid then pass auth check - if self.store.is_threepid_reserved(threepid): + if is_threepid_reserved(self.hs.config, threepid): 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: raise ResourceLimitError( 403, "Monthly Active User Limit Exceeded", - admin_contact=self.hs.config.admin_contact, errcode=Codes.RESOURCE_LIMIT_EXCEEDED, limit_type="monthly_active_user" diff --git a/synapse/config/server.py b/synapse/config/server.py index 2faf472189..c1c7c0105e 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -404,6 +404,23 @@ class ServerConfig(Config): " service on the given port.") +def is_threepid_reserved(config, threepid): + """Check the threepid against the reserved threepid config + Args: + config(ServerConfig) - to access server config attributes + threepid(dict) - The threepid to test for + + Returns: + boolean Is the threepid undertest reserved_user + """ + + for tp in config.mau_limits_reserved_threepids: + if (threepid['medium'] == tp['medium'] + and threepid['address'] == tp['address']): + return True + return False + + def read_gc_thresholds(thresholds): """Reads the three integer thresholds for garbage collection. Ensures that the thresholds are integers if thresholds are supplied. diff --git a/synapse/rest/client/v1_only/register.py b/synapse/rest/client/v1_only/register.py index 95873e03d5..dadb376b02 100644 --- a/synapse/rest/client/v1_only/register.py +++ b/synapse/rest/client/v1_only/register.py @@ -23,6 +23,7 @@ from twisted.internet import defer import synapse.util.stringutils as stringutils from synapse.api.constants import LoginType from synapse.api.errors import Codes, SynapseError +from synapse.config.server import is_threepid_reserved from synapse.http.servlet import assert_params_in_dict, parse_json_object_from_request from synapse.rest.client.v1.base import ClientV1RestServlet from synapse.types import create_requester @@ -282,7 +283,7 @@ class RegisterRestServlet(ClientV1RestServlet): if "user" in register_json else None ) threepid = None - if session[LoginType.EMAIL_IDENTITY]: + if session.get(LoginType.EMAIL_IDENTITY): threepid = session["threepidCreds"] handler = self.handlers.registration_handler @@ -293,8 +294,8 @@ class RegisterRestServlet(ClientV1RestServlet): ) # Necessary due to auth checks prior to the threepid being # written to the db - if self.store.is_threepid_reserved(threepid): - self.store.upsert_monthly_active_user(registered_user_id) + if is_threepid_reserved(self.hs.config, threepid): + yield self.store.upsert_monthly_active_user(user_id) if session[LoginType.EMAIL_IDENTITY]: logger.debug("Binding emails %s to %s" % ( diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index f22b7577ea..2fb4d43ccb 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -26,6 +26,7 @@ import synapse import synapse.types from synapse.api.constants import LoginType from synapse.api.errors import Codes, SynapseError, UnrecognizedRequestError +from synapse.config.server import is_threepid_reserved from synapse.http.servlet import ( RestServlet, assert_params_in_dict, @@ -408,8 +409,8 @@ class RegisterRestServlet(RestServlet): ) # Necessary due to auth checks prior to the threepid being # written to the db - if self.store.is_threepid_reserved(threepid): - self.store.upsert_monthly_active_user(registered_user_id) + if is_threepid_reserved(self.hs.config, threepid): + yield self.store.upsert_monthly_active_user(registered_user_id) # remember that we've now registered that user account, and with # what user ID (since the user may not have specified) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 173867c4b1..c7899d7fd2 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -219,17 +219,3 @@ class MonthlyActiveUsersStore(SQLBaseStore): yield self.upsert_monthly_active_user(user_id) elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY: yield self.upsert_monthly_active_user(user_id) - - def is_threepid_reserved(self, threepid): - """Check the threepid against the reserved threepid config - Args: - threepid(dict) - The threepid to test for - Returns: - boolean Is the threepid undertest reserved_user - """ - for tp in self.hs.config.mau_limits_reserved_threepids: - if (threepid['medium'] == tp['medium'] - and threepid['address'] == tp['address']): - return True - else: - return False diff --git a/tests/utils.py b/tests/utils.py index 179b592501..63e30dc6c0 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -26,6 +26,7 @@ from twisted.internet import defer, reactor from synapse.api.constants import EventTypes from synapse.api.errors import CodeMessageException, cs_error +from synapse.config.server import ServerConfig from synapse.federation.transport import server from synapse.http.server import HttpServer from synapse.server import HomeServer @@ -158,6 +159,11 @@ def setup_test_homeserver( # background, which upsets the test runner. config.update_user_directory = False + def is_threepid_reserved(threepid): + return ServerConfig.is_threepid_reserved(config, threepid) + + config.is_threepid_reserved.side_effect = is_threepid_reserved + config.use_frozen_dicts = True config.ldap_enabled = False From 301cb60d0b2de55f7feac24a043b2624ad3c8733 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 31 Aug 2018 17:29:35 +0100 Subject: [PATCH 7/7] assert rather than warn --- synapse/api/auth.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index a89687f420..34382e4e3c 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -783,6 +783,7 @@ class Auth(object): Args: user_id(str|None): If present, checks for presence against existing MAU cohort + threepid(dict|None): If present, checks for presence against configured reserved threepid. Used in cases where the user is trying register with a MAU blocked server, normally they would be rejected but their @@ -803,9 +804,7 @@ class Auth(object): limit_type=self.hs.config.hs_disabled_limit_type ) if self.hs.config.limit_usage_by_mau is True: - - if user_id and threepid: - logger.warn("Called with both user_id and threepid, this shoudn't happen") + assert not (user_id and threepid) # If the user is already part of the MAU cohort or a trial user if user_id: