From e8d08537394a49f3e66e9cbea3627e3c25818a7d Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Fri, 13 Nov 2020 16:24:04 +0000 Subject: Generalise _maybe_store_room_on_invite (#8754) There's a handy function called maybe_store_room_on_invite which allows us to create an entry in the rooms table for a room and its version for which we aren't joined to yet, but we can reference when ingesting events about. This is currently used for invites where we receive some stripped state about the room and pass it down via /sync to the client, without us being in the room yet. There is a similar requirement for knocking, where we will eventually do the same thing, and need an entry in the rooms table as well. Thus, reusing this function works, however its name needs to be generalised a bit. Separated out from #6739. --- synapse/handlers/federation.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'synapse/handlers') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index c386957706..69bc5ba44d 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -67,7 +67,7 @@ from synapse.replication.http.devices import ReplicationUserDevicesResyncRestSer from synapse.replication.http.federation import ( ReplicationCleanRoomRestServlet, ReplicationFederationSendEventsRestServlet, - ReplicationStoreRoomOnInviteRestServlet, + ReplicationStoreRoomOnOutlierMembershipRestServlet, ) from synapse.state import StateResolutionStore from synapse.storage.databases.main.events_worker import EventRedactBehaviour @@ -152,12 +152,14 @@ class FederationHandler(BaseHandler): self._user_device_resync = ReplicationUserDevicesResyncRestServlet.make_client( hs ) - self._maybe_store_room_on_invite = ReplicationStoreRoomOnInviteRestServlet.make_client( + self._maybe_store_room_on_outlier_membership = ReplicationStoreRoomOnOutlierMembershipRestServlet.make_client( hs ) else: self._device_list_updater = hs.get_device_handler().device_list_updater - self._maybe_store_room_on_invite = self.store.maybe_store_room_on_invite + self._maybe_store_room_on_outlier_membership = ( + self.store.maybe_store_room_on_outlier_membership + ) # When joining a room we need to queue any events for that room up. # For each room, a list of (pdu, origin) tuples. @@ -1617,7 +1619,7 @@ class FederationHandler(BaseHandler): # keep a record of the room version, if we don't yet know it. # (this may get overwritten if we later get a different room version in a # join dance). - await self._maybe_store_room_on_invite( + await self._maybe_store_room_on_outlier_membership( room_id=event.room_id, room_version=room_version ) -- cgit 1.5.1 From 4f76eef0e8190a1f5b9985853fe645275c9a797c Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 16 Nov 2020 15:37:36 +0000 Subject: Generalise _locally_reject_invite (#8751) `_locally_reject_invite` generates an out-of-band membership event which can be passed to clients, but not other homeservers. This is used when we fail to reject an invite over federation. If this happens, we instead just generate a leave event locally and send it down /sync, allowing clients to reject invites even if we can't reach the remote homeserver. A similar flow needs to be put in place for rescinding knocks. If we're unable to contact any remote server from the room we've tried to knock on, we'd still like to generate and store the leave event locally. Hence the need to reuse, and thus generalise, this method. Separated from #6739. --- changelog.d/8751.misc | 1 + synapse/handlers/room_member.py | 36 +++++++++++++++++++----------------- 2 files changed, 20 insertions(+), 17 deletions(-) create mode 100644 changelog.d/8751.misc (limited to 'synapse/handlers') diff --git a/changelog.d/8751.misc b/changelog.d/8751.misc new file mode 100644 index 0000000000..204c280c0e --- /dev/null +++ b/changelog.d/8751.misc @@ -0,0 +1 @@ +Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. \ No newline at end of file diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 7cd858b7db..fd85e08973 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -1104,32 +1104,34 @@ class RoomMemberMasterHandler(RoomMemberHandler): # logger.warning("Failed to reject invite: %s", e) - return await self._locally_reject_invite( + return await self._generate_local_out_of_band_leave( invite_event, txn_id, requester, content ) - async def _locally_reject_invite( + async def _generate_local_out_of_band_leave( self, - invite_event: EventBase, + previous_membership_event: EventBase, txn_id: Optional[str], requester: Requester, content: JsonDict, ) -> Tuple[str, int]: - """Generate a local invite rejection + """Generate a local leave event for a room - This is called after we fail to reject an invite via a remote server. It - generates an out-of-band membership event locally. + This can be called after we e.g fail to reject an invite via a remote server. + It generates an out-of-band membership event locally. Args: - invite_event: the invite to be rejected + previous_membership_event: the previous membership event for this user txn_id: optional transaction ID supplied by the client - requester: user making the rejection request, according to the access token - content: additional content to include in the rejection event. + requester: user making the request, according to the access token + content: additional content to include in the leave event. Normally an empty dict. - """ - room_id = invite_event.room_id - target_user = invite_event.state_key + Returns: + A tuple containing (event_id, stream_id of the leave event) + """ + room_id = previous_membership_event.room_id + target_user = previous_membership_event.state_key content["membership"] = Membership.LEAVE @@ -1141,12 +1143,12 @@ class RoomMemberMasterHandler(RoomMemberHandler): "state_key": target_user, } - # the auth events for the new event are the same as that of the invite, plus - # the invite itself. + # the auth events for the new event are the same as that of the previous event, plus + # the event itself. # - # the prev_events are just the invite. - prev_event_ids = [invite_event.event_id] - auth_event_ids = invite_event.auth_event_ids() + prev_event_ids + # the prev_events consist solely of the previous membership event. + prev_event_ids = [previous_membership_event.event_id] + auth_event_ids = previous_membership_event.auth_event_ids() + prev_event_ids event, context = await self.event_creation_handler.create_event( requester, -- cgit 1.5.1 From f737368a26bb9eea401fcc3a5bdd7e0b59e91f09 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 17 Nov 2020 10:51:25 +0000 Subject: Add admin API for logging in as a user (#8617) --- changelog.d/8617.feature | 1 + docs/admin_api/user_admin_api.rst | 35 ++++ synapse/api/auth_blocking.py | 33 ++- synapse/handlers/_base.py | 4 +- synapse/handlers/auth.py | 24 ++- synapse/handlers/deactivate_account.py | 5 +- synapse/handlers/message.py | 21 +- synapse/handlers/profile.py | 8 +- synapse/handlers/register.py | 24 ++- synapse/handlers/room.py | 10 +- synapse/handlers/room_member.py | 5 +- synapse/handlers/sync.py | 4 +- synapse/module_api/__init__.py | 5 +- synapse/rest/admin/__init__.py | 2 + synapse/rest/admin/rooms.py | 4 +- synapse/rest/admin/users.py | 54 ++++- synapse/rest/client/v2_alpha/sync.py | 1 + synapse/server_notices/server_notices_manager.py | 13 +- synapse/storage/databases/main/registration.py | 2 + tests/api/test_auth.py | 6 +- tests/handlers/test_sync.py | 14 +- tests/module_api/test_api.py | 11 +- tests/rest/admin/test_user.py | 245 ++++++++++++++++++++++- tests/storage/test_cleanup_extrems.py | 30 --- tests/test_state.py | 1 + 25 files changed, 475 insertions(+), 87 deletions(-) create mode 100644 changelog.d/8617.feature (limited to 'synapse/handlers') diff --git a/changelog.d/8617.feature b/changelog.d/8617.feature new file mode 100644 index 0000000000..4f1e788506 --- /dev/null +++ b/changelog.d/8617.feature @@ -0,0 +1 @@ +Add admin API for logging in as a user. diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst index d4051d0257..95b3da27c4 100644 --- a/docs/admin_api/user_admin_api.rst +++ b/docs/admin_api/user_admin_api.rst @@ -424,6 +424,41 @@ The following fields are returned in the JSON response body: - ``next_token``: integer - Indication for pagination. See above. - ``total`` - integer - Total number of media. +Login as a user +=============== + +Get an access token that can be used to authenticate as that user. Useful for +when admins wish to do actions on behalf of a user. + +The API is:: + + POST /_synapse/admin/v1/users//login + {} + +An optional ``valid_until_ms`` field can be specified in the request body as an +integer timestamp that specifies when the token should expire. By default tokens +do not expire. + +A response body like the following is returned: + +.. code:: json + + { + "access_token": "" + } + + +This API does *not* generate a new device for the user, and so will not appear +their ``/devices`` list, and in general the target user should not be able to +tell they have been logged in as. + +To expire the token call the standard ``/logout`` API with the token. + +Note: The token will expire if the *admin* user calls ``/logout/all`` from any +of their devices, but the token will *not* expire if the target user does the +same. + + User devices ============ diff --git a/synapse/api/auth_blocking.py b/synapse/api/auth_blocking.py index d8fafd7cb8..9c227218e0 100644 --- a/synapse/api/auth_blocking.py +++ b/synapse/api/auth_blocking.py @@ -14,10 +14,12 @@ # limitations under the License. import logging +from typing import Optional from synapse.api.constants import LimitBlockingTypes, UserTypes from synapse.api.errors import Codes, ResourceLimitError from synapse.config.server import is_threepid_reserved +from synapse.types import Requester logger = logging.getLogger(__name__) @@ -33,24 +35,47 @@ class AuthBlocking: self._max_mau_value = hs.config.max_mau_value self._limit_usage_by_mau = hs.config.limit_usage_by_mau self._mau_limits_reserved_threepids = hs.config.mau_limits_reserved_threepids + self._server_name = hs.hostname - async def check_auth_blocking(self, user_id=None, threepid=None, user_type=None): + async def check_auth_blocking( + self, + user_id: Optional[str] = None, + threepid: Optional[dict] = None, + user_type: Optional[str] = None, + requester: Optional[Requester] = None, + ): """Checks if the user should be rejected for some external reason, such as monthly active user limiting or global disable flag Args: - user_id(str|None): If present, checks for presence against existing + user_id: If present, checks for presence against existing MAU cohort - threepid(dict|None): If present, checks for presence against configured + threepid: 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. - user_type(str|None): If present, is used to decide whether to check against + user_type: If present, is used to decide whether to check against certain blocking reasons like MAU. + + requester: If present, and the authenticated entity is a user, checks for + presence against existing MAU cohort. Passing in both a `user_id` and + `requester` is an error. """ + if requester and user_id: + raise Exception( + "Passed in both 'user_id' and 'requester' to 'check_auth_blocking'" + ) + + if requester: + if requester.authenticated_entity.startswith("@"): + user_id = requester.authenticated_entity + elif requester.authenticated_entity == self._server_name: + # We never block the server from doing actions on behalf of + # users. + return # 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 diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index bd8e71ae56..bb81c0e81d 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -169,7 +169,9 @@ class BaseHandler: # and having homeservers have their own users leave keeps more # of that decision-making and control local to the guest-having # homeserver. - requester = synapse.types.create_requester(target_user, is_guest=True) + requester = synapse.types.create_requester( + target_user, is_guest=True, authenticated_entity=self.server_name + ) handler = self.hs.get_room_member_handler() await handler.update_membership( requester, diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 213baea2e3..5163afd86c 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -698,8 +698,12 @@ class AuthHandler(BaseHandler): } async def get_access_token_for_user_id( - self, user_id: str, device_id: Optional[str], valid_until_ms: Optional[int] - ): + self, + user_id: str, + device_id: Optional[str], + valid_until_ms: Optional[int], + puppets_user_id: Optional[str] = None, + ) -> str: """ Creates a new access token for the user with the given user ID. @@ -725,13 +729,25 @@ class AuthHandler(BaseHandler): fmt_expiry = time.strftime( " until %Y-%m-%d %H:%M:%S", time.localtime(valid_until_ms / 1000.0) ) - logger.info("Logging in user %s on device %s%s", user_id, device_id, fmt_expiry) + + if puppets_user_id: + logger.info( + "Logging in user %s as %s%s", user_id, puppets_user_id, fmt_expiry + ) + else: + logger.info( + "Logging in user %s on device %s%s", user_id, device_id, fmt_expiry + ) await self.auth.check_auth_blocking(user_id) access_token = self.macaroon_gen.generate_access_token(user_id) await self.store.add_access_token_to_user( - user_id, access_token, device_id, valid_until_ms + user_id=user_id, + token=access_token, + device_id=device_id, + valid_until_ms=valid_until_ms, + puppets_user_id=puppets_user_id, ) # the device *should* have been registered before we got here; however, diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 4efe6c530a..e808142365 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -39,6 +39,7 @@ class DeactivateAccountHandler(BaseHandler): self._room_member_handler = hs.get_room_member_handler() self._identity_handler = hs.get_identity_handler() self.user_directory_handler = hs.get_user_directory_handler() + self._server_name = hs.hostname # Flag that indicates whether the process to part users from rooms is running self._user_parter_running = False @@ -152,7 +153,7 @@ class DeactivateAccountHandler(BaseHandler): for room in pending_invites: try: await self._room_member_handler.update_membership( - create_requester(user), + create_requester(user, authenticated_entity=self._server_name), user, room.room_id, "leave", @@ -208,7 +209,7 @@ class DeactivateAccountHandler(BaseHandler): logger.info("User parter parting %r from %r", user_id, room_id) try: await self._room_member_handler.update_membership( - create_requester(user), + create_requester(user, authenticated_entity=self._server_name), user, room_id, "leave", diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index c6791fb912..96843338ae 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -472,7 +472,7 @@ class EventCreationHandler: Returns: Tuple of created event, Context """ - await self.auth.check_auth_blocking(requester.user.to_string()) + await self.auth.check_auth_blocking(requester=requester) if event_dict["type"] == EventTypes.Create and event_dict["state_key"] == "": room_version = event_dict["content"]["room_version"] @@ -619,7 +619,13 @@ class EventCreationHandler: if requester.app_service is not None: return - user_id = requester.user.to_string() + user_id = requester.authenticated_entity + if not user_id.startswith("@"): + # The authenticated entity might not be a user, e.g. if it's the + # server puppetting the user. + return + + user = UserID.from_string(user_id) # exempt the system notices user if ( @@ -639,9 +645,7 @@ class EventCreationHandler: if u["consent_version"] == self.config.user_consent_version: return - consent_uri = self._consent_uri_builder.build_user_consent_uri( - requester.user.localpart - ) + consent_uri = self._consent_uri_builder.build_user_consent_uri(user.localpart) msg = self._block_events_without_consent_error % {"consent_uri": consent_uri} raise ConsentNotGivenError(msg=msg, consent_uri=consent_uri) @@ -1252,7 +1256,7 @@ class EventCreationHandler: for user_id in members: if not self.hs.is_mine_id(user_id): continue - requester = create_requester(user_id) + requester = create_requester(user_id, authenticated_entity=self.server_name) try: event, context = await self.create_event( requester, @@ -1273,11 +1277,6 @@ class EventCreationHandler: requester, event, context, ratelimit=False, ignore_shadow_ban=True, ) return True - except ConsentNotGivenError: - logger.info( - "Failed to send dummy event into room %s for user %s due to " - "lack of consent. Will try another user" % (room_id, user_id) - ) except AuthError: logger.info( "Failed to send dummy event into room %s for user %s due to " diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 74a1ddd780..dee0ef45e7 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -206,7 +206,9 @@ class ProfileHandler(BaseHandler): # the join event to update the displayname in the rooms. # This must be done by the target user himself. if by_admin: - requester = create_requester(target_user) + requester = create_requester( + target_user, authenticated_entity=requester.authenticated_entity, + ) await self.store.set_profile_displayname( target_user.localpart, displayname_to_set @@ -286,7 +288,9 @@ class ProfileHandler(BaseHandler): # Same like set_displayname if by_admin: - requester = create_requester(target_user) + requester = create_requester( + target_user, authenticated_entity=requester.authenticated_entity + ) await self.store.set_profile_avatar_url(target_user.localpart, new_avatar_url) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index ed1ff62599..252f700786 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -52,6 +52,7 @@ class RegistrationHandler(BaseHandler): self.ratelimiter = hs.get_registration_ratelimiter() self.macaroon_gen = hs.get_macaroon_generator() self._server_notices_mxid = hs.config.server_notices_mxid + self._server_name = hs.hostname self.spam_checker = hs.get_spam_checker() @@ -317,7 +318,8 @@ class RegistrationHandler(BaseHandler): requires_join = False if self.hs.config.registration.auto_join_user_id: fake_requester = create_requester( - self.hs.config.registration.auto_join_user_id + self.hs.config.registration.auto_join_user_id, + authenticated_entity=self._server_name, ) # If the room requires an invite, add the user to the list of invites. @@ -329,7 +331,9 @@ class RegistrationHandler(BaseHandler): # being necessary this will occur after the invite was sent. requires_join = True else: - fake_requester = create_requester(user_id) + fake_requester = create_requester( + user_id, authenticated_entity=self._server_name + ) # Choose whether to federate the new room. if not self.hs.config.registration.autocreate_auto_join_rooms_federated: @@ -362,7 +366,9 @@ class RegistrationHandler(BaseHandler): # created it, then ensure the first user joins it. if requires_join: await room_member_handler.update_membership( - requester=create_requester(user_id), + requester=create_requester( + user_id, authenticated_entity=self._server_name + ), target=UserID.from_string(user_id), room_id=info["room_id"], # Since it was just created, there are no remote hosts. @@ -370,11 +376,6 @@ class RegistrationHandler(BaseHandler): action="join", ratelimit=False, ) - - except ConsentNotGivenError as e: - # Technically not necessary to pull out this error though - # moving away from bare excepts is a good thing to do. - logger.error("Failed to join new user to %r: %r", r, e) except Exception as e: logger.error("Failed to join new user to %r: %r", r, e) @@ -426,7 +427,8 @@ class RegistrationHandler(BaseHandler): if requires_invite: await room_member_handler.update_membership( requester=create_requester( - self.hs.config.registration.auto_join_user_id + self.hs.config.registration.auto_join_user_id, + authenticated_entity=self._server_name, ), target=UserID.from_string(user_id), room_id=room_id, @@ -437,7 +439,9 @@ class RegistrationHandler(BaseHandler): # Send the join. await room_member_handler.update_membership( - requester=create_requester(user_id), + requester=create_requester( + user_id, authenticated_entity=self._server_name + ), target=UserID.from_string(user_id), room_id=room_id, remote_room_hosts=remote_room_hosts, diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index e73031475f..930047e730 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -587,7 +587,7 @@ class RoomCreationHandler(BaseHandler): """ user_id = requester.user.to_string() - await self.auth.check_auth_blocking(user_id) + await self.auth.check_auth_blocking(requester=requester) if ( self._server_notices_mxid is not None @@ -1257,7 +1257,9 @@ class RoomShutdownHandler: 400, "User must be our own: %s" % (new_room_user_id,) ) - room_creator_requester = create_requester(new_room_user_id) + room_creator_requester = create_requester( + new_room_user_id, authenticated_entity=requester_user_id + ) info, stream_id = await self._room_creation_handler.create_room( room_creator_requester, @@ -1297,7 +1299,9 @@ class RoomShutdownHandler: try: # Kick users from room - target_requester = create_requester(user_id) + target_requester = create_requester( + user_id, authenticated_entity=requester_user_id + ) _, stream_id = await self.room_member_handler.update_membership( requester=target_requester, target=target_requester.user, diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index fd85e08973..70f8966267 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -965,6 +965,7 @@ class RoomMemberMasterHandler(RoomMemberHandler): self.distributor = hs.get_distributor() self.distributor.declare("user_left_room") + self._server_name = hs.hostname async def _is_remote_room_too_complex( self, room_id: str, remote_room_hosts: List[str] @@ -1059,7 +1060,9 @@ class RoomMemberMasterHandler(RoomMemberHandler): return event_id, stream_id # The room is too large. Leave. - requester = types.create_requester(user, None, False, False, None) + requester = types.create_requester( + user, authenticated_entity=self._server_name + ) await self.update_membership( requester=requester, target=user, room_id=room_id, action="leave" ) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 32e53c2d25..9827c7eb8d 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -31,6 +31,7 @@ from synapse.types import ( Collection, JsonDict, MutableStateMap, + Requester, RoomStreamToken, StateMap, StreamToken, @@ -260,6 +261,7 @@ class SyncHandler: async def wait_for_sync_for_user( self, + requester: Requester, sync_config: SyncConfig, since_token: Optional[StreamToken] = None, timeout: int = 0, @@ -273,7 +275,7 @@ class SyncHandler: # not been exceeded (if not part of the group by this point, almost certain # auth_blocking will occur) user_id = sync_config.user.to_string() - await self.auth.check_auth_blocking(user_id) + await self.auth.check_auth_blocking(requester=requester) res = await self.response_cache.wrap( sync_config.request_key, diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 0142542852..72ab5750cc 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -49,6 +49,7 @@ class ModuleApi: self._store = hs.get_datastore() self._auth = hs.get_auth() self._auth_handler = auth_handler + self._server_name = hs.hostname # We expose these as properties below in order to attach a helpful docstring. self._http_client = hs.get_simple_http_client() # type: SimpleHttpClient @@ -336,7 +337,9 @@ class ModuleApi: SynapseError if the event was not allowed. """ # Create a requester object - requester = create_requester(event_dict["sender"]) + requester = create_requester( + event_dict["sender"], authenticated_entity=self._server_name + ) # Create and send the event ( diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index 2a4f7a1740..7a3a5c46ca 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -61,6 +61,7 @@ from synapse.rest.admin.users import ( UserRestServletV2, UsersRestServlet, UsersRestServletV2, + UserTokenRestServlet, WhoisRestServlet, ) from synapse.types import RoomStreamToken @@ -223,6 +224,7 @@ def register_servlets(hs, http_server): UserAdminServlet(hs).register(http_server) UserMediaRestServlet(hs).register(http_server) UserMembershipRestServlet(hs).register(http_server) + UserTokenRestServlet(hs).register(http_server) UserRestServletV2(hs).register(http_server) UsersRestServletV2(hs).register(http_server) DeviceRestServlet(hs).register(http_server) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index f5304ff43d..ee345e12ce 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -309,7 +309,9 @@ class JoinRoomAliasServlet(RestServlet): 400, "%s was not legal room ID or room alias" % (room_identifier,) ) - fake_requester = create_requester(target_user) + fake_requester = create_requester( + target_user, authenticated_entity=requester.authenticated_entity + ) # send invite if room has "JoinRules.INVITE" room_state = await self.state_handler.get_current_state(room_id) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 3638e219f2..fa8d8e6d91 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -16,7 +16,7 @@ import hashlib import hmac import logging from http import HTTPStatus -from typing import Tuple +from typing import TYPE_CHECKING, Tuple from synapse.api.constants import UserTypes from synapse.api.errors import Codes, NotFoundError, SynapseError @@ -37,6 +37,9 @@ from synapse.rest.admin._base import ( ) from synapse.types import JsonDict, UserID +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) _GET_PUSHERS_ALLOWED_KEYS = { @@ -828,3 +831,52 @@ class UserMediaRestServlet(RestServlet): ret["next_token"] = start + len(media) return 200, ret + + +class UserTokenRestServlet(RestServlet): + """An admin API for logging in as a user. + + Example: + + POST /_synapse/admin/v1/users/@test:example.com/login + {} + + 200 OK + { + "access_token": "" + } + """ + + PATTERNS = admin_patterns("/users/(?P[^/]*)/login$") + + def __init__(self, hs: "HomeServer"): + self.hs = hs + self.store = hs.get_datastore() + self.auth = hs.get_auth() + self.auth_handler = hs.get_auth_handler() + + async def on_POST(self, request, user_id): + requester = await self.auth.get_user_by_req(request) + await assert_user_is_admin(self.auth, requester.user) + auth_user = requester.user + + if not self.hs.is_mine_id(user_id): + raise SynapseError(400, "Only local users can be logged in as") + + body = parse_json_object_from_request(request, allow_empty_body=True) + + valid_until_ms = body.get("valid_until_ms") + if valid_until_ms and not isinstance(valid_until_ms, int): + raise SynapseError(400, "'valid_until_ms' parameter must be an int") + + if auth_user.to_string() == user_id: + raise SynapseError(400, "Cannot use admin API to login as self") + + token = await self.auth_handler.get_access_token_for_user_id( + user_id=auth_user.to_string(), + device_id=None, + valid_until_ms=valid_until_ms, + puppets_user_id=user_id, + ) + + return 200, {"access_token": token} diff --git a/synapse/rest/client/v2_alpha/sync.py b/synapse/rest/client/v2_alpha/sync.py index 2b84eb89c0..8e52e4cca4 100644 --- a/synapse/rest/client/v2_alpha/sync.py +++ b/synapse/rest/client/v2_alpha/sync.py @@ -171,6 +171,7 @@ class SyncRestServlet(RestServlet): ) with context: sync_result = await self.sync_handler.wait_for_sync_for_user( + requester, sync_config, since_token=since_token, timeout=timeout, diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index d464c75c03..100dbd5e2c 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -39,6 +39,7 @@ class ServerNoticesManager: self._room_member_handler = hs.get_room_member_handler() self._event_creation_handler = hs.get_event_creation_handler() self._is_mine_id = hs.is_mine_id + self._server_name = hs.hostname self._notifier = hs.get_notifier() self.server_notices_mxid = self._config.server_notices_mxid @@ -72,7 +73,9 @@ class ServerNoticesManager: await self.maybe_invite_user_to_room(user_id, room_id) system_mxid = self._config.server_notices_mxid - requester = create_requester(system_mxid) + requester = create_requester( + system_mxid, authenticated_entity=self._server_name + ) logger.info("Sending server notice to %s", user_id) @@ -145,7 +148,9 @@ class ServerNoticesManager: "avatar_url": self._config.server_notices_mxid_avatar_url, } - requester = create_requester(self.server_notices_mxid) + requester = create_requester( + self.server_notices_mxid, authenticated_entity=self._server_name + ) info, _ = await self._room_creation_handler.create_room( requester, config={ @@ -174,7 +179,9 @@ class ServerNoticesManager: user_id: The ID of the user to invite. room_id: The ID of the room to invite the user to. """ - requester = create_requester(self.server_notices_mxid) + requester = create_requester( + self.server_notices_mxid, authenticated_entity=self._server_name + ) # Check whether the user has already joined or been invited to this room. If # that's the case, there is no need to re-invite them. diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index e5d07ce72a..fedb8a6c26 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1110,6 +1110,7 @@ class RegistrationStore(StatsStore, RegistrationBackgroundUpdateStore): token: str, device_id: Optional[str], valid_until_ms: Optional[int], + puppets_user_id: Optional[str] = None, ) -> int: """Adds an access token for the given user. @@ -1133,6 +1134,7 @@ class RegistrationStore(StatsStore, RegistrationBackgroundUpdateStore): "token": token, "device_id": device_id, "valid_until_ms": valid_until_ms, + "puppets_user_id": puppets_user_id, }, desc="add_access_token_to_user", ) diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 0fd55f428a..ee5217b074 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -282,7 +282,11 @@ class AuthTestCase(unittest.TestCase): ) ) self.store.add_access_token_to_user.assert_called_with( - USER_ID, token, "DEVICE", None + user_id=USER_ID, + token=token, + device_id="DEVICE", + valid_until_ms=None, + puppets_user_id=None, ) def get_user(tok): diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index e178d7765b..e62586142e 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -16,7 +16,7 @@ from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import DEFAULT_FILTER_COLLECTION from synapse.handlers.sync import SyncConfig -from synapse.types import UserID +from synapse.types import UserID, create_requester import tests.unittest import tests.utils @@ -38,6 +38,7 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): user_id1 = "@user1:test" user_id2 = "@user2:test" sync_config = self._generate_sync_config(user_id1) + requester = create_requester(user_id1) self.reactor.advance(100) # So we get not 0 time self.auth_blocking._limit_usage_by_mau = True @@ -45,21 +46,26 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): # Check that the happy case does not throw errors self.get_success(self.store.upsert_monthly_active_user(user_id1)) - self.get_success(self.sync_handler.wait_for_sync_for_user(sync_config)) + self.get_success( + self.sync_handler.wait_for_sync_for_user(requester, sync_config) + ) # Test that global lock works self.auth_blocking._hs_disabled = True e = self.get_failure( - self.sync_handler.wait_for_sync_for_user(sync_config), ResourceLimitError + self.sync_handler.wait_for_sync_for_user(requester, sync_config), + ResourceLimitError, ) self.assertEquals(e.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) self.auth_blocking._hs_disabled = False sync_config = self._generate_sync_config(user_id2) + requester = create_requester(user_id2) e = self.get_failure( - self.sync_handler.wait_for_sync_for_user(sync_config), ResourceLimitError + self.sync_handler.wait_for_sync_for_user(requester, sync_config), + ResourceLimitError, ) self.assertEquals(e.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 9b573ac24d..27206ca3db 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -94,12 +94,13 @@ class ModuleApiTestCase(HomeserverTestCase): self.assertFalse(hasattr(event, "state_key")) self.assertDictEqual(event.content, content) + expected_requester = create_requester( + user_id, authenticated_entity=self.hs.hostname + ) + # Check that the event was sent self.event_creation_handler.create_and_send_nonmember_event.assert_called_with( - create_requester(user_id), - event_dict, - ratelimit=False, - ignore_shadow_ban=True, + expected_requester, event_dict, ratelimit=False, ignore_shadow_ban=True, ) # Create and send a state event @@ -128,7 +129,7 @@ class ModuleApiTestCase(HomeserverTestCase): # Check that the event was sent self.event_creation_handler.create_and_send_nonmember_event.assert_called_with( - create_requester(user_id), + expected_requester, { "type": "m.room.power_levels", "content": content, diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index d74efede06..94cd45066c 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -24,8 +24,8 @@ from mock import Mock import synapse.rest.admin from synapse.api.constants import UserTypes from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError -from synapse.rest.client.v1 import login, profile, room -from synapse.rest.client.v2_alpha import sync +from synapse.rest.client.v1 import login, logout, profile, room +from synapse.rest.client.v2_alpha import devices, sync from tests import unittest from tests.test_utils import make_awaitable @@ -1638,3 +1638,244 @@ class UserMediaRestTestCase(unittest.HomeserverTestCase): self.assertIn("last_access_ts", m) self.assertIn("quarantined_by", m) self.assertIn("safe_from_quarantine", m) + + +class UserTokenRestTestCase(unittest.HomeserverTestCase): + """Test for /_synapse/admin/v1/users//login + """ + + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + sync.register_servlets, + room.register_servlets, + devices.register_servlets, + logout.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + self.other_user = self.register_user("user", "pass") + self.other_user_tok = self.login("user", "pass") + self.url = "/_synapse/admin/v1/users/%s/login" % urllib.parse.quote( + self.other_user + ) + + def _get_token(self) -> str: + request, channel = self.make_request( + "POST", self.url, b"{}", access_token=self.admin_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + return channel.json_body["access_token"] + + def test_no_auth(self): + """Try to login as a user without authentication. + """ + request, channel = self.make_request("POST", self.url, b"{}") + self.render(request) + + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + def test_not_admin(self): + """Try to login as a user as a non-admin user. + """ + request, channel = self.make_request( + "POST", self.url, b"{}", access_token=self.other_user_tok + ) + self.render(request) + + self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + + def test_send_event(self): + """Test that sending event as a user works. + """ + # Create a room. + room_id = self.helper.create_room_as(self.other_user, tok=self.other_user_tok) + + # Login in as the user + puppet_token = self._get_token() + + # Test that sending works, and generates the event as the right user. + resp = self.helper.send_event(room_id, "com.example.test", tok=puppet_token) + event_id = resp["event_id"] + event = self.get_success(self.store.get_event(event_id)) + self.assertEqual(event.sender, self.other_user) + + def test_devices(self): + """Tests that logging in as a user doesn't create a new device for them. + """ + # Login in as the user + self._get_token() + + # Check that we don't see a new device in our devices list + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=self.other_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # We should only see the one device (from the login in `prepare`) + self.assertEqual(len(channel.json_body["devices"]), 1) + + def test_logout(self): + """Test that calling `/logout` with the token works. + """ + # Login in as the user + puppet_token = self._get_token() + + # Test that we can successfully make a request + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Logout with the puppet token + request, channel = self.make_request( + "POST", "logout", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # The puppet token should no longer work + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + + # .. but the real user's tokens should still work + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=self.other_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + def test_user_logout_all(self): + """Tests that the target user calling `/logout/all` does *not* expire + the token. + """ + # Login in as the user + puppet_token = self._get_token() + + # Test that we can successfully make a request + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Logout all with the real user token + request, channel = self.make_request( + "POST", "logout/all", b"{}", access_token=self.other_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # The puppet token should still work + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # .. but the real user's tokens shouldn't + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=self.other_user_tok + ) + self.render(request) + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + + def test_admin_logout_all(self): + """Tests that the admin user calling `/logout/all` does expire the + token. + """ + # Login in as the user + puppet_token = self._get_token() + + # Test that we can successfully make a request + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Logout all with the admin user token + request, channel = self.make_request( + "POST", "logout/all", b"{}", access_token=self.admin_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # The puppet token should no longer work + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=puppet_token + ) + self.render(request) + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + + # .. but the real user's tokens should still work + request, channel = self.make_request( + "GET", "devices", b"{}", access_token=self.other_user_tok + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + @unittest.override_config( + { + "public_baseurl": "https://example.org/", + "user_consent": { + "version": "1.0", + "policy_name": "My Cool Privacy Policy", + "template_dir": "/", + "require_at_registration": True, + "block_events_error": "You should accept the policy", + }, + "form_secret": "123secret", + } + ) + def test_consent(self): + """Test that sending a message is not subject to the privacy policies. + """ + # Have the admin user accept the terms. + self.get_success(self.store.user_set_consent_version(self.admin_user, "1.0")) + + # First, cheekily accept the terms and create a room + self.get_success(self.store.user_set_consent_version(self.other_user, "1.0")) + room_id = self.helper.create_room_as(self.other_user, tok=self.other_user_tok) + self.helper.send_event(room_id, "com.example.test", tok=self.other_user_tok) + + # Now unaccept it and check that we can't send an event + self.get_success(self.store.user_set_consent_version(self.other_user, "0.0")) + self.helper.send_event( + room_id, "com.example.test", tok=self.other_user_tok, expect_code=403 + ) + + # Login in as the user + puppet_token = self._get_token() + + # Sending an event on their behalf should work fine + self.helper.send_event(room_id, "com.example.test", tok=puppet_token) + + @override_config( + {"limit_usage_by_mau": True, "max_mau_value": 1, "mau_trial_days": 0} + ) + def test_mau_limit(self): + # Create a room as the admin user. This will bump the monthly active users to 1. + room_id = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + + # Trying to join as the other user should fail due to reaching MAU limit. + self.helper.join( + room_id, user=self.other_user, tok=self.other_user_tok, expect_code=403 + ) + + # Logging in as the other user and joining a room should work, even + # though the MAU limit would stop the user doing so. + puppet_token = self._get_token() + self.helper.join(room_id, user=self.other_user, tok=puppet_token) diff --git a/tests/storage/test_cleanup_extrems.py b/tests/storage/test_cleanup_extrems.py index 5a1e5c4e66..c13a57dad1 100644 --- a/tests/storage/test_cleanup_extrems.py +++ b/tests/storage/test_cleanup_extrems.py @@ -309,36 +309,6 @@ class CleanupExtremDummyEventsTestCase(HomeserverTestCase): ) self.assertTrue(len(latest_event_ids) < 10, len(latest_event_ids)) - @patch("synapse.handlers.message._DUMMY_EVENT_ROOM_EXCLUSION_EXPIRY", new=0) - def test_send_dummy_event_without_consent(self): - self._create_extremity_rich_graph() - self._enable_consent_checking() - - # Pump the reactor repeatedly so that the background updates have a - # chance to run. Attempt to add dummy event with user that has not consented - # Check that dummy event send fails. - self.pump(10 * 60) - latest_event_ids = self.get_success( - self.store.get_latest_event_ids_in_room(self.room_id) - ) - self.assertTrue(len(latest_event_ids) == self.EXTREMITIES_COUNT) - - # Create new user, and add consent - user2 = self.register_user("user2", "password") - token2 = self.login("user2", "password") - self.get_success( - self.store.user_set_consent_version(user2, self.CONSENT_VERSION) - ) - self.helper.join(self.room_id, user2, tok=token2) - - # Background updates should now cause a dummy event to be added to the graph - self.pump(10 * 60) - - latest_event_ids = self.get_success( - self.store.get_latest_event_ids_in_room(self.room_id) - ) - self.assertTrue(len(latest_event_ids) < 10, len(latest_event_ids)) - @patch("synapse.handlers.message._DUMMY_EVENT_ROOM_EXCLUSION_EXPIRY", new=250) def test_expiry_logic(self): """Simple test to ensure that _expire_rooms_to_exclude_from_dummy_event_insertion() diff --git a/tests/test_state.py b/tests/test_state.py index 80b0ccbc40..6227a3ba95 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -169,6 +169,7 @@ class StateTestCase(unittest.TestCase): "get_state_handler", "get_clock", "get_state_resolution_handler", + "hostname", ] ) hs.config = default_config("tesths", True) -- cgit 1.5.1 From 473dfec1e56cd4115886ad273dba5594ae02ea8a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 17 Nov 2020 09:09:40 -0500 Subject: Use TYPE_CHECKING instead of magic MYPY variable. (#8770) --- changelog.d/8770.misc | 1 + synapse/events/spamcheck.py | 5 ++--- synapse/handlers/presence.py | 5 ++--- synapse/rest/client/v1/room.py | 5 ++--- 4 files changed, 7 insertions(+), 9 deletions(-) create mode 100644 changelog.d/8770.misc (limited to 'synapse/handlers') diff --git a/changelog.d/8770.misc b/changelog.d/8770.misc new file mode 100644 index 0000000000..b5876a82f9 --- /dev/null +++ b/changelog.d/8770.misc @@ -0,0 +1 @@ +Use `TYPE_CHECKING` instead of magic `MYPY` variable. diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index bad18f7fdf..936896656a 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -15,13 +15,12 @@ # limitations under the License. import inspect -from typing import Any, Dict, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import Collection -MYPY = False -if MYPY: +if TYPE_CHECKING: import synapse.events import synapse.server diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 8e014c9bb5..22d1e9d35c 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -25,7 +25,7 @@ The methods that define policy are: import abc import logging from contextlib import contextmanager -from typing import Dict, Iterable, List, Set, Tuple +from typing import TYPE_CHECKING, Dict, Iterable, List, Set, Tuple from prometheus_client import Counter from typing_extensions import ContextManager @@ -46,8 +46,7 @@ from synapse.util.caches.descriptors import cached from synapse.util.metrics import Measure from synapse.util.wheel_timer import WheelTimer -MYPY = False -if MYPY: +if TYPE_CHECKING: from synapse.server import HomeServer logger = logging.getLogger(__name__) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 25d3cc6148..93c06afe27 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -18,7 +18,7 @@ import logging import re -from typing import List, Optional +from typing import TYPE_CHECKING, List, Optional from urllib import parse as urlparse from synapse.api.constants import EventTypes, Membership @@ -48,8 +48,7 @@ from synapse.types import RoomAlias, RoomID, StreamToken, ThirdPartyInstanceID, from synapse.util import json_decoder from synapse.util.stringutils import random_string -MYPY = False -if MYPY: +if TYPE_CHECKING: import synapse.server logger = logging.getLogger(__name__) -- cgit 1.5.1 From ee382025b0c264701fc320133912e9fece40b021 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 17 Nov 2020 09:46:23 -0500 Subject: Abstract shared SSO code. (#8765) De-duplicates code between the SAML and OIDC implementations. --- changelog.d/8765.misc | 1 + synapse/handlers/oidc_handler.py | 92 ++++++++++++++-------------------------- synapse/handlers/saml_handler.py | 77 ++++++++++----------------------- synapse/handlers/sso.py | 90 +++++++++++++++++++++++++++++++++++++++ synapse/server.py | 5 +++ tests/handlers/test_oidc.py | 14 +++--- 6 files changed, 159 insertions(+), 120 deletions(-) create mode 100644 changelog.d/8765.misc create mode 100644 synapse/handlers/sso.py (limited to 'synapse/handlers') diff --git a/changelog.d/8765.misc b/changelog.d/8765.misc new file mode 100644 index 0000000000..053f9acc9c --- /dev/null +++ b/changelog.d/8765.misc @@ -0,0 +1 @@ +Consolidate logic between the OpenID Connect and SAML code. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 331d4e7e96..be8562d47b 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -34,7 +34,8 @@ from typing_extensions import TypedDict from twisted.web.client import readBody from synapse.config import ConfigError -from synapse.http.server import respond_with_html +from synapse.handlers._base import BaseHandler +from synapse.handlers.sso import MappingException from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart @@ -83,17 +84,12 @@ class OidcError(Exception): return self.error -class MappingException(Exception): - """Used to catch errors when mapping the UserInfo object - """ - - -class OidcHandler: +class OidcHandler(BaseHandler): """Handles requests related to the OpenID Connect login flow. """ def __init__(self, hs: "HomeServer"): - self.hs = hs + super().__init__(hs) self._callback_url = hs.config.oidc_callback_url # type: str self._scopes = hs.config.oidc_scopes # type: List[str] self._user_profile_method = hs.config.oidc_user_profile_method # type: str @@ -120,36 +116,13 @@ class OidcHandler: self._http_client = hs.get_proxied_http_client() self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() - self._datastore = hs.get_datastore() - self._clock = hs.get_clock() - self._hostname = hs.hostname # type: str self._server_name = hs.config.server_name # type: str self._macaroon_secret_key = hs.config.macaroon_secret_key - self._error_template = hs.config.sso_error_template # identifier for the external_ids table self._auth_provider_id = "oidc" - def _render_error( - self, request, error: str, error_description: Optional[str] = None - ) -> None: - """Render the error template and respond to the request with it. - - This is used to show errors to the user. The template of this page can - be found under `synapse/res/templates/sso_error.html`. - - Args: - request: The incoming request from the browser. - We'll respond with an HTML page describing the error. - error: A technical identifier for this error. Those include - well-known OAuth2/OIDC error types like invalid_request or - access_denied. - error_description: A human-readable description of the error. - """ - html = self._error_template.render( - error=error, error_description=error_description - ) - respond_with_html(request, 400, html) + self._sso_handler = hs.get_sso_handler() def _validate_metadata(self): """Verifies the provider metadata. @@ -571,7 +544,7 @@ class OidcHandler: Since we might want to display OIDC-related errors in a user-friendly way, we don't raise SynapseError from here. Instead, we call - ``self._render_error`` which displays an HTML page for the error. + ``self._sso_handler.render_error`` which displays an HTML page for the error. Most of the OpenID Connect logic happens here: @@ -609,7 +582,7 @@ class OidcHandler: if error != "access_denied": logger.error("Error from the OIDC provider: %s %s", error, description) - self._render_error(request, error, description) + self._sso_handler.render_error(request, error, description) return # otherwise, it is presumably a successful response. see: @@ -619,7 +592,9 @@ class OidcHandler: session = request.getCookie(SESSION_COOKIE_NAME) # type: Optional[bytes] if session is None: logger.info("No session cookie found") - self._render_error(request, "missing_session", "No session cookie found") + self._sso_handler.render_error( + request, "missing_session", "No session cookie found" + ) return # Remove the cookie. There is a good chance that if the callback failed @@ -637,7 +612,9 @@ class OidcHandler: # Check for the state query parameter if b"state" not in request.args: logger.info("State parameter is missing") - self._render_error(request, "invalid_request", "State parameter is missing") + self._sso_handler.render_error( + request, "invalid_request", "State parameter is missing" + ) return state = request.args[b"state"][0].decode() @@ -651,17 +628,19 @@ class OidcHandler: ) = self._verify_oidc_session_token(session, state) except MacaroonDeserializationException as e: logger.exception("Invalid session") - self._render_error(request, "invalid_session", str(e)) + self._sso_handler.render_error(request, "invalid_session", str(e)) return except MacaroonInvalidSignatureException as e: logger.exception("Could not verify session") - self._render_error(request, "mismatching_session", str(e)) + self._sso_handler.render_error(request, "mismatching_session", str(e)) return # Exchange the code with the provider if b"code" not in request.args: logger.info("Code parameter is missing") - self._render_error(request, "invalid_request", "Code parameter is missing") + self._sso_handler.render_error( + request, "invalid_request", "Code parameter is missing" + ) return logger.debug("Exchanging code") @@ -670,7 +649,7 @@ class OidcHandler: token = await self._exchange_code(code) except OidcError as e: logger.exception("Could not exchange code") - self._render_error(request, e.error, e.error_description) + self._sso_handler.render_error(request, e.error, e.error_description) return logger.debug("Successfully obtained OAuth2 access token") @@ -683,7 +662,7 @@ class OidcHandler: userinfo = await self._fetch_userinfo(token) except Exception as e: logger.exception("Could not fetch userinfo") - self._render_error(request, "fetch_error", str(e)) + self._sso_handler.render_error(request, "fetch_error", str(e)) return else: logger.debug("Extracting userinfo from id_token") @@ -691,7 +670,7 @@ class OidcHandler: userinfo = await self._parse_id_token(token, nonce=nonce) except Exception as e: logger.exception("Invalid id_token") - self._render_error(request, "invalid_token", str(e)) + self._sso_handler.render_error(request, "invalid_token", str(e)) return # Pull out the user-agent and IP from the request. @@ -705,7 +684,7 @@ class OidcHandler: ) except MappingException as e: logger.exception("Could not map user") - self._render_error(request, "mapping_error", str(e)) + self._sso_handler.render_error(request, "mapping_error", str(e)) return # Mapping providers might not have get_extra_attributes: only call this @@ -770,7 +749,7 @@ class OidcHandler: macaroon.add_first_party_caveat( "ui_auth_session_id = %s" % (ui_auth_session_id,) ) - now = self._clock.time_msec() + now = self.clock.time_msec() expiry = now + duration_in_ms macaroon.add_first_party_caveat("time < %d" % (expiry,)) @@ -845,7 +824,7 @@ class OidcHandler: if not caveat.startswith(prefix): return False expiry = int(caveat[len(prefix) :]) - now = self._clock.time_msec() + now = self.clock.time_msec() return now < expiry async def _map_userinfo_to_user( @@ -885,20 +864,14 @@ class OidcHandler: # to be strings. remote_user_id = str(remote_user_id) - logger.info( - "Looking for existing mapping for user %s:%s", - self._auth_provider_id, - remote_user_id, - ) - - registered_user_id = await self._datastore.get_user_by_external_id( + # first of all, check if we already have a mapping for this user + previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id( self._auth_provider_id, remote_user_id, ) + if previously_registered_user_id: + return previously_registered_user_id - if registered_user_id is not None: - logger.info("Found existing mapping %s", registered_user_id) - return registered_user_id - + # Otherwise, generate a new user. try: attributes = await self._user_mapping_provider.map_user_attributes( userinfo, token @@ -917,8 +890,8 @@ class OidcHandler: localpart = map_username_to_mxid_localpart(attributes["localpart"]) - user_id = UserID(localpart, self._hostname).to_string() - users = await self._datastore.get_users_by_id_case_insensitive(user_id) + user_id = UserID(localpart, self.server_name).to_string() + users = await self.store.get_users_by_id_case_insensitive(user_id) if users: if self._allow_existing_users: if len(users) == 1: @@ -942,7 +915,8 @@ class OidcHandler: default_display_name=attributes["display_name"], user_agent_ips=(user_agent, ip_address), ) - await self._datastore.record_user_external_id( + + await self.store.record_user_external_id( self._auth_provider_id, remote_user_id, registered_user_id, ) return registered_user_id diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index fd6c5e9ea8..aee772239a 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -24,7 +24,8 @@ from saml2.client import Saml2Client from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.config.saml2_config import SamlAttributeRequirement -from synapse.http.server import respond_with_html +from synapse.handlers._base import BaseHandler +from synapse.handlers.sso import MappingException from synapse.http.servlet import parse_string from synapse.http.site import SynapseRequest from synapse.module_api import ModuleApi @@ -42,10 +43,6 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -class MappingException(Exception): - """Used to catch errors when mapping the SAML2 response to a user.""" - - @attr.s(slots=True) class Saml2SessionData: """Data we track about SAML2 sessions""" @@ -57,17 +54,13 @@ class Saml2SessionData: ui_auth_session_id = attr.ib(type=Optional[str], default=None) -class SamlHandler: +class SamlHandler(BaseHandler): def __init__(self, hs: "synapse.server.HomeServer"): - self.hs = hs + super().__init__(hs) self._saml_client = Saml2Client(hs.config.saml2_sp_config) - self._auth = hs.get_auth() self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() - self._clock = hs.get_clock() - self._datastore = hs.get_datastore() - self._hostname = hs.hostname self._saml2_session_lifetime = hs.config.saml2_session_lifetime self._grandfathered_mxid_source_attribute = ( hs.config.saml2_grandfathered_mxid_source_attribute @@ -88,26 +81,9 @@ class SamlHandler: self._outstanding_requests_dict = {} # type: Dict[str, Saml2SessionData] # a lock on the mappings - self._mapping_lock = Linearizer(name="saml_mapping", clock=self._clock) - - def _render_error( - self, request, error: str, error_description: Optional[str] = None - ) -> None: - """Render the error template and respond to the request with it. + self._mapping_lock = Linearizer(name="saml_mapping", clock=self.clock) - This is used to show errors to the user. The template of this page can - be found under `synapse/res/templates/sso_error.html`. - - Args: - request: The incoming request from the browser. - We'll respond with an HTML page describing the error. - error: A technical identifier for this error. - error_description: A human-readable description of the error. - """ - html = self._error_template.render( - error=error, error_description=error_description - ) - respond_with_html(request, 400, html) + self._sso_handler = hs.get_sso_handler() def handle_redirect_request( self, client_redirect_url: bytes, ui_auth_session_id: Optional[str] = None @@ -130,7 +106,7 @@ class SamlHandler: # Since SAML sessions timeout it is useful to log when they were created. logger.info("Initiating a new SAML session: %s" % (reqid,)) - now = self._clock.time_msec() + now = self.clock.time_msec() self._outstanding_requests_dict[reqid] = Saml2SessionData( creation_time=now, ui_auth_session_id=ui_auth_session_id, ) @@ -171,12 +147,12 @@ class SamlHandler: # in the (user-visible) exception message, so let's log the exception here # so we can track down the session IDs later. logger.warning(str(e)) - self._render_error( + self._sso_handler.render_error( request, "unsolicited_response", "Unexpected SAML2 login." ) return except Exception as e: - self._render_error( + self._sso_handler.render_error( request, "invalid_response", "Unable to parse SAML2 response: %s." % (e,), @@ -184,7 +160,7 @@ class SamlHandler: return if saml2_auth.not_signed: - self._render_error( + self._sso_handler.render_error( request, "unsigned_respond", "SAML2 response was not signed." ) return @@ -210,7 +186,7 @@ class SamlHandler: # attributes. for requirement in self._saml2_attribute_requirements: if not _check_attribute_requirement(saml2_auth.ava, requirement): - self._render_error( + self._sso_handler.render_error( request, "unauthorised", "You are not authorised to log in here." ) return @@ -226,7 +202,7 @@ class SamlHandler: ) except MappingException as e: logger.exception("Could not map user") - self._render_error(request, "mapping_error", str(e)) + self._sso_handler.render_error(request, "mapping_error", str(e)) return # Complete the interactive auth session or the login. @@ -274,17 +250,11 @@ class SamlHandler: with (await self._mapping_lock.queue(self._auth_provider_id)): # first of all, check if we already have a mapping for this user - logger.info( - "Looking for existing mapping for user %s:%s", - self._auth_provider_id, - remote_user_id, + previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id( + self._auth_provider_id, remote_user_id, ) - registered_user_id = await self._datastore.get_user_by_external_id( - self._auth_provider_id, remote_user_id - ) - if registered_user_id is not None: - logger.info("Found existing mapping %s", registered_user_id) - return registered_user_id + if previously_registered_user_id: + return previously_registered_user_id # backwards-compatibility hack: see if there is an existing user with a # suitable mapping from the uid @@ -294,7 +264,7 @@ class SamlHandler: ): attrval = saml2_auth.ava[self._grandfathered_mxid_source_attribute][0] user_id = UserID( - map_username_to_mxid_localpart(attrval), self._hostname + map_username_to_mxid_localpart(attrval), self.server_name ).to_string() logger.info( "Looking for existing account based on mapped %s %s", @@ -302,11 +272,11 @@ class SamlHandler: user_id, ) - users = await self._datastore.get_users_by_id_case_insensitive(user_id) + users = await self.store.get_users_by_id_case_insensitive(user_id) if users: registered_user_id = list(users.keys())[0] logger.info("Grandfathering mapping to %s", registered_user_id) - await self._datastore.record_user_external_id( + await self.store.record_user_external_id( self._auth_provider_id, remote_user_id, registered_user_id ) return registered_user_id @@ -335,8 +305,8 @@ class SamlHandler: emails = attribute_dict.get("emails", []) # Check if this mxid already exists - if not await self._datastore.get_users_by_id_case_insensitive( - UserID(localpart, self._hostname).to_string() + if not await self.store.get_users_by_id_case_insensitive( + UserID(localpart, self.server_name).to_string() ): # This mxid is free break @@ -348,7 +318,6 @@ class SamlHandler: ) logger.info("Mapped SAML user to local part %s", localpart) - registered_user_id = await self._registration_handler.register_user( localpart=localpart, default_display_name=displayname, @@ -356,13 +325,13 @@ class SamlHandler: user_agent_ips=(user_agent, ip_address), ) - await self._datastore.record_user_external_id( + await self.store.record_user_external_id( self._auth_provider_id, remote_user_id, registered_user_id ) return registered_user_id def expire_sessions(self): - expire_before = self._clock.time_msec() - self._saml2_session_lifetime + expire_before = self.clock.time_msec() - self._saml2_session_lifetime to_expire = set() for reqid, data in self._outstanding_requests_dict.items(): if data.creation_time < expire_before: diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py new file mode 100644 index 0000000000..9cb1866a71 --- /dev/null +++ b/synapse/handlers/sso.py @@ -0,0 +1,90 @@ +# -*- coding: utf-8 -*- +# Copyright 2020 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import logging +from typing import TYPE_CHECKING, Optional + +from synapse.handlers._base import BaseHandler +from synapse.http.server import respond_with_html + +if TYPE_CHECKING: + from synapse.server import HomeServer + +logger = logging.getLogger(__name__) + + +class MappingException(Exception): + """Used to catch errors when mapping the UserInfo object + """ + + +class SsoHandler(BaseHandler): + def __init__(self, hs: "HomeServer"): + super().__init__(hs) + self._error_template = hs.config.sso_error_template + + def render_error( + self, request, error: str, error_description: Optional[str] = None + ) -> None: + """Renders the error template and responds with it. + + This is used to show errors to the user. The template of this page can + be found under `synapse/res/templates/sso_error.html`. + + Args: + request: The incoming request from the browser. + We'll respond with an HTML page describing the error. + error: A technical identifier for this error. + error_description: A human-readable description of the error. + """ + html = self._error_template.render( + error=error, error_description=error_description + ) + respond_with_html(request, 400, html) + + async def get_sso_user_by_remote_user_id( + self, auth_provider_id: str, remote_user_id: str + ) -> Optional[str]: + """ + Maps the user ID of a remote IdP to a mxid for a previously seen user. + + If the user has not been seen yet, this will return None. + + Args: + auth_provider_id: A unique identifier for this SSO provider, e.g. + "oidc" or "saml". + remote_user_id: The user ID according to the remote IdP. This might + be an e-mail address, a GUID, or some other form. It must be + unique and immutable. + + Returns: + The mxid of a previously seen user. + """ + # Check if we already have a mapping for this user. + logger.info( + "Looking for existing mapping for user %s:%s", + auth_provider_id, + remote_user_id, + ) + previously_registered_user_id = await self.store.get_user_by_external_id( + auth_provider_id, remote_user_id, + ) + + # A match was found, return the user ID. + if previously_registered_user_id is not None: + logger.info("Found existing mapping %s", previously_registered_user_id) + return previously_registered_user_id + + # No match. + return None diff --git a/synapse/server.py b/synapse/server.py index 21a232bbd9..12a783de17 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -89,6 +89,7 @@ from synapse.handlers.room_member import RoomMemberMasterHandler from synapse.handlers.room_member_worker import RoomMemberWorkerHandler from synapse.handlers.search import SearchHandler from synapse.handlers.set_password import SetPasswordHandler +from synapse.handlers.sso import SsoHandler from synapse.handlers.stats import StatsHandler from synapse.handlers.sync import SyncHandler from synapse.handlers.typing import FollowerTypingHandler, TypingWriterHandler @@ -390,6 +391,10 @@ class HomeServer(metaclass=abc.ABCMeta): else: return FollowerTypingHandler(self) + @cache_in_self + def get_sso_handler(self) -> SsoHandler: + return SsoHandler(self) + @cache_in_self def get_sync_handler(self) -> SyncHandler: return SyncHandler(self) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 0d51705849..630e6da808 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -154,6 +154,9 @@ class OidcHandlerTestCase(HomeserverTestCase): ) self.handler = OidcHandler(hs) + # Mock the render error method. + self.render_error = Mock(return_value=None) + self.handler._sso_handler.render_error = self.render_error return hs @@ -161,12 +164,12 @@ class OidcHandlerTestCase(HomeserverTestCase): return patch.dict(self.handler._provider_metadata, values) def assertRenderedError(self, error, error_description=None): - args = self.handler._render_error.call_args[0] + args = self.render_error.call_args[0] self.assertEqual(args[1], error) if error_description is not None: self.assertEqual(args[2], error_description) # Reset the render_error mock - self.handler._render_error.reset_mock() + self.render_error.reset_mock() def test_config(self): """Basic config correctly sets up the callback URL and client auth correctly.""" @@ -356,7 +359,6 @@ class OidcHandlerTestCase(HomeserverTestCase): def test_callback_error(self): """Errors from the provider returned in the callback are displayed.""" - self.handler._render_error = Mock() request = Mock(args={}) request.args[b"error"] = [b"invalid_client"] self.get_success(self.handler.handle_oidc_callback(request)) @@ -387,7 +389,6 @@ class OidcHandlerTestCase(HomeserverTestCase): "preferred_username": "bar", } user_id = "@foo:domain.org" - self.handler._render_error = Mock(return_value=None) self.handler._exchange_code = simple_async_mock(return_value=token) self.handler._parse_id_token = simple_async_mock(return_value=userinfo) self.handler._fetch_userinfo = simple_async_mock(return_value=userinfo) @@ -435,7 +436,7 @@ class OidcHandlerTestCase(HomeserverTestCase): userinfo, token, user_agent, ip_address ) self.handler._fetch_userinfo.assert_not_called() - self.handler._render_error.assert_not_called() + self.render_error.assert_not_called() # Handle mapping errors self.handler._map_userinfo_to_user = simple_async_mock( @@ -469,7 +470,7 @@ class OidcHandlerTestCase(HomeserverTestCase): userinfo, token, user_agent, ip_address ) self.handler._fetch_userinfo.assert_called_once_with(token) - self.handler._render_error.assert_not_called() + self.render_error.assert_not_called() # Handle userinfo fetching error self.handler._fetch_userinfo = simple_async_mock(raises=Exception()) @@ -485,7 +486,6 @@ class OidcHandlerTestCase(HomeserverTestCase): def test_callback_session(self): """The callback verifies the session presence and validity""" - self.handler._render_error = Mock(return_value=None) request = Mock(spec=["args", "getCookie", "addCookie"]) # Missing cookie -- cgit 1.5.1 From 51338491c9bedcdfb5f9babad2a34cbfea6b57d2 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 18 Nov 2020 18:54:09 +0000 Subject: Improve appservice handler to send only the most recent read receipts when no stream_id is stored. (#8744) * Make this line debug (it's noisy) * Don't include from_key for presence if we are at 0 * Limit read receipts for all rooms to 100 * changelog.d/8744.bugfix * Allow from_key to be None * Update 8744.bugfix * The from_key is superflous * Update comment --- changelog.d/8744.bugfix | 1 + synapse/handlers/appservice.py | 2 +- synapse/handlers/receipts.py | 3 ++- synapse/storage/databases/main/receipts.py | 7 ++++++- 4 files changed, 10 insertions(+), 3 deletions(-) create mode 100644 changelog.d/8744.bugfix (limited to 'synapse/handlers') diff --git a/changelog.d/8744.bugfix b/changelog.d/8744.bugfix new file mode 100644 index 0000000000..f8f9630bd6 --- /dev/null +++ b/changelog.d/8744.bugfix @@ -0,0 +1 @@ +Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 9fc8444228..5c6458eb52 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -226,7 +226,7 @@ class ApplicationServicesHandler: new_token: Optional[int], users: Collection[Union[str, UserID]], ): - logger.info("Checking interested services for %s" % (stream_key)) + logger.debug("Checking interested services for %s" % (stream_key)) with Measure(self.clock, "notify_interested_services_ephemeral"): for service in services: # Only handle typing if we have the latest token diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index c242c409cf..153cbae7b9 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -158,7 +158,8 @@ class ReceiptEventSource: if from_key == to_key: return [], to_key - # We first need to fetch all new receipts + # Fetch all read receipts for all rooms, up to a limit of 100. This is ordered + # by most recent. rooms_to_events = await self.store.get_linearized_receipts_for_all_rooms( from_key=from_key, to_key=to_key ) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index ca7917c989..1e7949a323 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -278,7 +278,8 @@ class ReceiptsWorkerStore(SQLBaseStore, metaclass=abc.ABCMeta): async def get_linearized_receipts_for_all_rooms( self, to_key: int, from_key: Optional[int] = None ) -> Dict[str, JsonDict]: - """Get receipts for all rooms between two stream_ids. + """Get receipts for all rooms between two stream_ids, up + to a limit of the latest 100 read receipts. Args: to_key: Max stream id to fetch receipts upto. @@ -294,12 +295,16 @@ class ReceiptsWorkerStore(SQLBaseStore, metaclass=abc.ABCMeta): sql = """ SELECT * FROM receipts_linearized WHERE stream_id > ? AND stream_id <= ? + ORDER BY stream_id DESC + LIMIT 100 """ txn.execute(sql, [from_key, to_key]) else: sql = """ SELECT * FROM receipts_linearized WHERE stream_id <= ? + ORDER BY stream_id DESC + LIMIT 100 """ txn.execute(sql, [to_key]) -- cgit 1.5.1 From 950bb0305fc3c114f456eb4e2a806014150545d2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 19 Nov 2020 10:05:33 +0000 Subject: Consistently use room_id from federation request body (#8776) * Consistently use room_id from federation request body Some federation APIs have a redundant `room_id` path param (see https://github.com/matrix-org/matrix-doc/issues/2330). We should make sure we consistently use either the path param or the body param, and the body param is easier. * Kill off some references to "context" Once upon a time, "rooms" were known as "contexts". I think this kills of the last references to "contexts". --- changelog.d/8776.bugfix | 1 + synapse/federation/federation_server.py | 23 +++++------ synapse/federation/transport/server.py | 68 ++++++++++++++++----------------- synapse/handlers/federation.py | 10 ++--- tests/handlers/test_federation.py | 1 - 5 files changed, 49 insertions(+), 54 deletions(-) create mode 100644 changelog.d/8776.bugfix (limited to 'synapse/handlers') diff --git a/changelog.d/8776.bugfix b/changelog.d/8776.bugfix new file mode 100644 index 0000000000..dd7ebbeb86 --- /dev/null +++ b/changelog.d/8776.bugfix @@ -0,0 +1 @@ +Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 23278e36b7..4b6ab470d0 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -49,6 +49,7 @@ from synapse.federation.federation_base import FederationBase, event_from_pdu_js from synapse.federation.persistence import TransactionActions from synapse.federation.units import Edu, Transaction from synapse.http.endpoint import parse_server_name +from synapse.http.servlet import assert_params_in_dict from synapse.logging.context import ( make_deferred_yieldable, nested_logging_context, @@ -391,7 +392,7 @@ class FederationServer(FederationBase): TRANSACTION_CONCURRENCY_LIMIT, ) - async def on_context_state_request( + async def on_room_state_request( self, origin: str, room_id: str, event_id: str ) -> Tuple[int, Dict[str, Any]]: origin_host, _ = parse_server_name(origin) @@ -514,11 +515,12 @@ class FederationServer(FederationBase): return {"event": ret_pdu.get_pdu_json(time_now)} async def on_send_join_request( - self, origin: str, content: JsonDict, room_id: str + self, origin: str, content: JsonDict ) -> Dict[str, Any]: logger.debug("on_send_join_request: content: %s", content) - room_version = await self.store.get_room_version(room_id) + assert_params_in_dict(content, ["room_id"]) + room_version = await self.store.get_room_version(content["room_id"]) pdu = event_from_pdu_json(content, room_version) origin_host, _ = parse_server_name(origin) @@ -547,12 +549,11 @@ class FederationServer(FederationBase): time_now = self._clock.time_msec() return {"event": pdu.get_pdu_json(time_now), "room_version": room_version} - async def on_send_leave_request( - self, origin: str, content: JsonDict, room_id: str - ) -> dict: + async def on_send_leave_request(self, origin: str, content: JsonDict) -> dict: logger.debug("on_send_leave_request: content: %s", content) - room_version = await self.store.get_room_version(room_id) + assert_params_in_dict(content, ["room_id"]) + room_version = await self.store.get_room_version(content["room_id"]) pdu = event_from_pdu_json(content, room_version) origin_host, _ = parse_server_name(origin) @@ -748,12 +749,8 @@ class FederationServer(FederationBase): ) return ret - async def on_exchange_third_party_invite_request( - self, room_id: str, event_dict: Dict - ): - ret = await self.handler.on_exchange_third_party_invite_request( - room_id, event_dict - ) + async def on_exchange_third_party_invite_request(self, event_dict: Dict): + ret = await self.handler.on_exchange_third_party_invite_request(event_dict) return ret async def check_server_matches_acl(self, server_name: str, room_id: str): diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index a0933fae88..b53e7a20ec 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -440,13 +440,13 @@ class FederationEventServlet(BaseFederationServlet): class FederationStateV1Servlet(BaseFederationServlet): - PATH = "/state/(?P[^/]*)/?" + PATH = "/state/(?P[^/]*)/?" - # This is when someone asks for all data for a given context. - async def on_GET(self, origin, content, query, context): - return await self.handler.on_context_state_request( + # This is when someone asks for all data for a given room. + async def on_GET(self, origin, content, query, room_id): + return await self.handler.on_room_state_request( origin, - context, + room_id, parse_string_from_args(query, "event_id", None, required=False), ) @@ -463,16 +463,16 @@ class FederationStateIdsServlet(BaseFederationServlet): class FederationBackfillServlet(BaseFederationServlet): - PATH = "/backfill/(?P[^/]*)/?" + PATH = "/backfill/(?P[^/]*)/?" - async def on_GET(self, origin, content, query, context): + async def on_GET(self, origin, content, query, room_id): versions = [x.decode("ascii") for x in query[b"v"]] limit = parse_integer_from_args(query, "limit", None) if not limit: return 400, {"error": "Did not include limit param"} - return await self.handler.on_backfill_request(origin, context, versions, limit) + return await self.handler.on_backfill_request(origin, room_id, versions, limit) class FederationQueryServlet(BaseFederationServlet): @@ -487,9 +487,9 @@ class FederationQueryServlet(BaseFederationServlet): class FederationMakeJoinServlet(BaseFederationServlet): - PATH = "/make_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/make_join/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, _content, query, context, user_id): + async def on_GET(self, origin, _content, query, room_id, user_id): """ Args: origin (unicode): The authenticated server_name of the calling server @@ -511,16 +511,16 @@ class FederationMakeJoinServlet(BaseFederationServlet): supported_versions = ["1"] content = await self.handler.on_make_join_request( - origin, context, user_id, supported_versions=supported_versions + origin, room_id, user_id, supported_versions=supported_versions ) return 200, content class FederationMakeLeaveServlet(BaseFederationServlet): - PATH = "/make_leave/(?P[^/]*)/(?P[^/]*)" + PATH = "/make_leave/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, content, query, context, user_id): - content = await self.handler.on_make_leave_request(origin, context, user_id) + async def on_GET(self, origin, content, query, room_id, user_id): + content = await self.handler.on_make_leave_request(origin, room_id, user_id) return 200, content @@ -528,7 +528,7 @@ class FederationV1SendLeaveServlet(BaseFederationServlet): PATH = "/send_leave/(?P[^/]*)/(?P[^/]*)" async def on_PUT(self, origin, content, query, room_id, event_id): - content = await self.handler.on_send_leave_request(origin, content, room_id) + content = await self.handler.on_send_leave_request(origin, content) return 200, (200, content) @@ -538,43 +538,43 @@ class FederationV2SendLeaveServlet(BaseFederationServlet): PREFIX = FEDERATION_V2_PREFIX async def on_PUT(self, origin, content, query, room_id, event_id): - content = await self.handler.on_send_leave_request(origin, content, room_id) + content = await self.handler.on_send_leave_request(origin, content) return 200, content class FederationEventAuthServlet(BaseFederationServlet): - PATH = "/event_auth/(?P[^/]*)/(?P[^/]*)" + PATH = "/event_auth/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, content, query, context, event_id): - return await self.handler.on_event_auth(origin, context, event_id) + async def on_GET(self, origin, content, query, room_id, event_id): + return await self.handler.on_event_auth(origin, room_id, event_id) class FederationV1SendJoinServlet(BaseFederationServlet): - PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" - async def on_PUT(self, origin, content, query, context, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + async def on_PUT(self, origin, content, query, room_id, event_id): + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content - content = await self.handler.on_send_join_request(origin, content, context) + content = await self.handler.on_send_join_request(origin, content) return 200, (200, content) class FederationV2SendJoinServlet(BaseFederationServlet): - PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" PREFIX = FEDERATION_V2_PREFIX - async def on_PUT(self, origin, content, query, context, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + async def on_PUT(self, origin, content, query, room_id, event_id): + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content - content = await self.handler.on_send_join_request(origin, content, context) + content = await self.handler.on_send_join_request(origin, content) return 200, content class FederationV1InviteServlet(BaseFederationServlet): - PATH = "/invite/(?P[^/]*)/(?P[^/]*)" + PATH = "/invite/(?P[^/]*)/(?P[^/]*)" - async def on_PUT(self, origin, content, query, context, event_id): + async def on_PUT(self, origin, content, query, room_id, event_id): # We don't get a room version, so we have to assume its EITHER v1 or # v2. This is "fine" as the only difference between V1 and V2 is the # state resolution algorithm, and we don't use that for processing @@ -589,12 +589,12 @@ class FederationV1InviteServlet(BaseFederationServlet): class FederationV2InviteServlet(BaseFederationServlet): - PATH = "/invite/(?P[^/]*)/(?P[^/]*)" + PATH = "/invite/(?P[^/]*)/(?P[^/]*)" PREFIX = FEDERATION_V2_PREFIX - async def on_PUT(self, origin, content, query, context, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + async def on_PUT(self, origin, content, query, room_id, event_id): + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content room_version = content["room_version"] @@ -616,9 +616,7 @@ class FederationThirdPartyInviteExchangeServlet(BaseFederationServlet): PATH = "/exchange_third_party_invite/(?P[^/]*)" async def on_PUT(self, origin, content, query, room_id): - content = await self.handler.on_exchange_third_party_invite_request( - room_id, content - ) + content = await self.handler.on_exchange_third_party_invite_request(content) return 200, content diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 69bc5ba44d..b9799090f7 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -55,6 +55,7 @@ from synapse.events import EventBase from synapse.events.snapshot import EventContext from synapse.events.validator import EventValidator from synapse.handlers._base import BaseHandler +from synapse.http.servlet import assert_params_in_dict from synapse.logging.context import ( make_deferred_yieldable, nested_logging_context, @@ -2688,7 +2689,7 @@ class FederationHandler(BaseHandler): ) async def on_exchange_third_party_invite_request( - self, room_id: str, event_dict: JsonDict + self, event_dict: JsonDict ) -> None: """Handle an exchange_third_party_invite request from a remote server @@ -2696,12 +2697,11 @@ class FederationHandler(BaseHandler): into a normal m.room.member invite. Args: - room_id: The ID of the room. - - event_dict (dict[str, Any]): Dictionary containing the event body. + event_dict: Dictionary containing the event body. """ - room_version = await self.store.get_room_version_id(room_id) + assert_params_in_dict(event_dict, ["room_id"]) + room_version = await self.store.get_room_version_id(event_dict["room_id"]) # NB: event_dict has a particular specced format we might need to fudge # if we change event formats too much. diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 9ef80fe502..bf866dacf3 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -59,7 +59,6 @@ class FederationTestCase(unittest.HomeserverTestCase): ) d = self.handler.on_exchange_third_party_invite_request( - room_id=room_id, event_dict={ "type": EventTypes.Member, "room_id": room_id, -- cgit 1.5.1 From 53a6f5ddf0c6bf2a8c8c3b757fb54a0c7755daf7 Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Thu, 19 Nov 2020 14:57:13 +0000 Subject: SAML: Allow specifying the IdP entityid to use. (#8630) If the SAML metadata includes multiple IdPs it is necessary to specify which IdP to redirect users to for authentication. --- changelog.d/8630.feature | 1 + docs/sample_config.yaml | 8 ++++++++ synapse/config/saml2_config.py | 10 ++++++++++ synapse/handlers/saml_handler.py | 3 ++- 4 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 changelog.d/8630.feature (limited to 'synapse/handlers') diff --git a/changelog.d/8630.feature b/changelog.d/8630.feature new file mode 100644 index 0000000000..706051f131 --- /dev/null +++ b/changelog.d/8630.feature @@ -0,0 +1 @@ +Allow specification of the SAML IdP if the metadata returns multiple IdPs. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index bedc147770..52a1d8b853 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1674,6 +1674,14 @@ saml2_config: # - attribute: department # value: "sales" + # If the metadata XML contains multiple IdP entities then the `idp_entityid` + # option must be set to the entity to redirect users to. + # + # Most deployments only have a single IdP entity and so should omit this + # option. + # + #idp_entityid: 'https://our_idp/entityid' + # Enable OpenID Connect (OIDC) / OAuth 2.0 for registration and login. # diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py index f233854941..c1b8e98ae0 100644 --- a/synapse/config/saml2_config.py +++ b/synapse/config/saml2_config.py @@ -90,6 +90,8 @@ class SAML2Config(Config): "grandfathered_mxid_source_attribute", "uid" ) + self.saml2_idp_entityid = saml2_config.get("idp_entityid", None) + # user_mapping_provider may be None if the key is present but has no value ump_dict = saml2_config.get("user_mapping_provider") or {} @@ -383,6 +385,14 @@ class SAML2Config(Config): # value: "staff" # - attribute: department # value: "sales" + + # If the metadata XML contains multiple IdP entities then the `idp_entityid` + # option must be set to the entity to redirect users to. + # + # Most deployments only have a single IdP entity and so should omit this + # option. + # + #idp_entityid: 'https://our_idp/entityid' """ % { "config_dir_path": config_dir_path } diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index aee772239a..9bf430b656 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -58,6 +58,7 @@ class SamlHandler(BaseHandler): def __init__(self, hs: "synapse.server.HomeServer"): super().__init__(hs) self._saml_client = Saml2Client(hs.config.saml2_sp_config) + self._saml_idp_entityid = hs.config.saml2_idp_entityid self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() @@ -100,7 +101,7 @@ class SamlHandler(BaseHandler): URL to redirect to """ reqid, info = self._saml_client.prepare_for_authenticate( - relay_state=client_redirect_url + entityid=self._saml_idp_entityid, relay_state=client_redirect_url ) # Since SAML sessions timeout it is useful to log when they were created. -- cgit 1.5.1 From 79bfe966e08a2212cc2fae2b00f5efb2c2185543 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 19 Nov 2020 14:25:17 -0500 Subject: Improve error checking for OIDC/SAML mapping providers (#8774) Checks that the localpart returned by mapping providers for SAML and OIDC are valid before registering new users. Extends the OIDC tests for existing users and invalid data. --- UPGRADE.rst | 30 ++++++++++++++ changelog.d/8774.misc | 1 + docs/sso_mapping_providers.md | 9 +++- synapse/handlers/oidc_handler.py | 25 ++++++++--- synapse/handlers/saml_handler.py | 6 +++ synapse/types.py | 6 +-- tests/handlers/test_oidc.py | 89 +++++++++++++++++++++++++++++++--------- 7 files changed, 137 insertions(+), 29 deletions(-) create mode 100644 changelog.d/8774.misc (limited to 'synapse/handlers') diff --git a/UPGRADE.rst b/UPGRADE.rst index 7c19cf2a70..4de1bb5841 100644 --- a/UPGRADE.rst +++ b/UPGRADE.rst @@ -75,6 +75,36 @@ for example: wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb +Upgrading to v1.24.0 +==================== + +Custom OpenID Connect mapping provider breaking change +------------------------------------------------------ + +This release allows the OpenID Connect mapping provider to perform normalisation +of the localpart of the Matrix ID. This allows for the mapping provider to +specify different algorithms, instead of the [default way](https://matrix.org/docs/spec/appendices#mapping-from-other-character-sets). + +If your Synapse configuration uses a custom mapping provider +(`oidc_config.user_mapping_provider.module` is specified and not equal to +`synapse.handlers.oidc_handler.JinjaOidcMappingProvider`) then you *must* ensure +that `map_user_attributes` of the mapping provider performs some normalisation +of the `localpart` returned. To match previous behaviour you can use the +`map_username_to_mxid_localpart` function provided by Synapse. An example is +shown below: + +.. code-block:: python + + from synapse.types import map_username_to_mxid_localpart + + class MyMappingProvider: + def map_user_attributes(self, userinfo, token): + # ... your custom logic ... + sso_user_id = ... + localpart = map_username_to_mxid_localpart(sso_user_id) + + return {"localpart": localpart} + Upgrading to v1.23.0 ==================== diff --git a/changelog.d/8774.misc b/changelog.d/8774.misc new file mode 100644 index 0000000000..57cca8fee5 --- /dev/null +++ b/changelog.d/8774.misc @@ -0,0 +1 @@ +Add additional error checking for OpenID Connect and SAML mapping providers. diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md index 32b06aa2c5..707dd73978 100644 --- a/docs/sso_mapping_providers.md +++ b/docs/sso_mapping_providers.md @@ -15,8 +15,15 @@ where SAML mapping providers come into play. SSO mapping providers are currently supported for OpenID and SAML SSO configurations. Please see the details below for how to implement your own. +It is the responsibility of the mapping provider to normalise the SSO attributes +and map them to a valid Matrix ID. The +[specification for Matrix IDs](https://matrix.org/docs/spec/appendices#user-identifiers) +has some information about what is considered valid. Alternately an easy way to +ensure it is valid is to use a Synapse utility function: +`synapse.types.map_username_to_mxid_localpart`. + External mapping providers are provided to Synapse in the form of an external -Python module. You can retrieve this module from [PyPi](https://pypi.org) or elsewhere, +Python module. You can retrieve this module from [PyPI](https://pypi.org) or elsewhere, but it must be importable via Synapse (e.g. it must be in the same virtualenv as Synapse). The Synapse config is then modified to point to the mapping provider (and optionally provide additional configuration for it). diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index be8562d47b..4bfd8d5617 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -38,7 +38,12 @@ from synapse.handlers._base import BaseHandler from synapse.handlers.sso import MappingException from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable -from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart +from synapse.types import ( + JsonDict, + UserID, + contains_invalid_mxid_characters, + map_username_to_mxid_localpart, +) from synapse.util import json_decoder if TYPE_CHECKING: @@ -885,10 +890,12 @@ class OidcHandler(BaseHandler): "Retrieved user attributes from user mapping provider: %r", attributes ) - if not attributes["localpart"]: - raise MappingException("localpart is empty") - - localpart = map_username_to_mxid_localpart(attributes["localpart"]) + localpart = attributes["localpart"] + if not localpart: + raise MappingException( + "Error parsing OIDC response: OIDC mapping provider plugin " + "did not return a localpart value" + ) user_id = UserID(localpart, self.server_name).to_string() users = await self.store.get_users_by_id_case_insensitive(user_id) @@ -908,6 +915,11 @@ class OidcHandler(BaseHandler): # This mxid is taken raise MappingException("mxid '{}' is already taken".format(user_id)) else: + # Since the localpart is provided via a potentially untrusted module, + # ensure the MXID is valid before registering. + if contains_invalid_mxid_characters(localpart): + raise MappingException("localpart is invalid: %s" % (localpart,)) + # It's the first time this user is logging in and the mapped mxid was # not taken, register the user registered_user_id = await self._registration_handler.register_user( @@ -1076,6 +1088,9 @@ class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]): ) -> UserAttribute: localpart = self._config.localpart_template.render(user=userinfo).strip() + # Ensure only valid characters are included in the MXID. + localpart = map_username_to_mxid_localpart(localpart) + display_name = None # type: Optional[str] if self._config.display_name_template is not None: display_name = self._config.display_name_template.render( diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 9bf430b656..5d9b555b13 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -31,6 +31,7 @@ from synapse.http.site import SynapseRequest from synapse.module_api import ModuleApi from synapse.types import ( UserID, + contains_invalid_mxid_characters, map_username_to_mxid_localpart, mxid_localpart_allowed_characters, ) @@ -318,6 +319,11 @@ class SamlHandler(BaseHandler): "Unable to generate a Matrix ID from the SAML response" ) + # Since the localpart is provided via a potentially untrusted module, + # ensure the MXID is valid before registering. + if contains_invalid_mxid_characters(localpart): + raise MappingException("localpart is invalid: %s" % (localpart,)) + logger.info("Mapped SAML user to local part %s", localpart) registered_user_id = await self._registration_handler.register_user( localpart=localpart, diff --git a/synapse/types.py b/synapse/types.py index 66bb5bac8d..3ab6bdbe06 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -317,14 +317,14 @@ mxid_localpart_allowed_characters = set( ) -def contains_invalid_mxid_characters(localpart): +def contains_invalid_mxid_characters(localpart: str) -> bool: """Check for characters not allowed in an mxid or groupid localpart Args: - localpart (basestring): the localpart to be checked + localpart: the localpart to be checked Returns: - bool: True if there are any naughty characters + True if there are any naughty characters """ return any(c not in mxid_localpart_allowed_characters for c in localpart) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 630e6da808..b4fa02acc4 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -12,7 +12,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - import json from urllib.parse import parse_qs, urlparse @@ -24,12 +23,8 @@ import pymacaroons from twisted.python.failure import Failure from twisted.web._newclient import ResponseDone -from synapse.handlers.oidc_handler import ( - MappingException, - OidcError, - OidcHandler, - OidcMappingProvider, -) +from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider +from synapse.handlers.sso import MappingException from synapse.types import UserID from tests.unittest import HomeserverTestCase, override_config @@ -132,14 +127,13 @@ class OidcHandlerTestCase(HomeserverTestCase): config = self.default_config() config["public_baseurl"] = BASE_URL - oidc_config = {} - oidc_config["enabled"] = True - oidc_config["client_id"] = CLIENT_ID - oidc_config["client_secret"] = CLIENT_SECRET - oidc_config["issuer"] = ISSUER - oidc_config["scopes"] = SCOPES - oidc_config["user_mapping_provider"] = { - "module": __name__ + ".TestMappingProvider", + oidc_config = { + "enabled": True, + "client_id": CLIENT_ID, + "client_secret": CLIENT_SECRET, + "issuer": ISSUER, + "scopes": SCOPES, + "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"}, } # Update this config with what's in the default config so that @@ -705,13 +699,13 @@ class OidcHandlerTestCase(HomeserverTestCase): def test_map_userinfo_to_existing_user(self): """Existing users can log in with OpenID Connect when allow_existing_users is True.""" store = self.hs.get_datastore() - user4 = UserID.from_string("@test_user_4:test") + user = UserID.from_string("@test_user:test") self.get_success( - store.register_user(user_id=user4.to_string(), password_hash=None) + store.register_user(user_id=user.to_string(), password_hash=None) ) userinfo = { - "sub": "test4", - "username": "test_user_4", + "sub": "test", + "username": "test_user", } token = {} mxid = self.get_success( @@ -719,4 +713,59 @@ class OidcHandlerTestCase(HomeserverTestCase): userinfo, token, "user-agent", "10.10.10.10" ) ) - self.assertEqual(mxid, "@test_user_4:test") + self.assertEqual(mxid, "@test_user:test") + + # Register some non-exact matching cases. + user2 = UserID.from_string("@TEST_user_2:test") + self.get_success( + store.register_user(user_id=user2.to_string(), password_hash=None) + ) + user2_caps = UserID.from_string("@test_USER_2:test") + self.get_success( + store.register_user(user_id=user2_caps.to_string(), password_hash=None) + ) + + # Attempting to login without matching a name exactly is an error. + userinfo = { + "sub": "test2", + "username": "TEST_USER_2", + } + e = self.get_failure( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ), + MappingException, + ) + self.assertTrue( + str(e.value).startswith( + "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly:" + ) + ) + + # Logging in when matching a name exactly should work. + user2 = UserID.from_string("@TEST_USER_2:test") + self.get_success( + store.register_user(user_id=user2.to_string(), password_hash=None) + ) + + mxid = self.get_success( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ) + ) + self.assertEqual(mxid, "@TEST_USER_2:test") + + def test_map_userinfo_to_invalid_localpart(self): + """If the mapping provider generates an invalid localpart it should be rejected.""" + userinfo = { + "sub": "test2", + "username": "föö", + } + token = {} + e = self.get_failure( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ), + MappingException, + ) + self.assertEqual(str(e.value), "localpart is invalid: föö") -- cgit 1.5.1 From 59a995f38dab8d83aee09ed7eb3858390ed53a65 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 23 Nov 2020 13:45:23 +0000 Subject: Improve logging of the mapping from SSO IDs to Matrix IDs. (#8773) --- changelog.d/8773.misc | 1 + synapse/handlers/saml_handler.py | 5 +++-- synapse/handlers/sso.py | 12 +++++++++--- 3 files changed, 13 insertions(+), 5 deletions(-) create mode 100644 changelog.d/8773.misc (limited to 'synapse/handlers') diff --git a/changelog.d/8773.misc b/changelog.d/8773.misc new file mode 100644 index 0000000000..62778ba410 --- /dev/null +++ b/changelog.d/8773.misc @@ -0,0 +1 @@ +Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 5d9b555b13..f4e8cbeac8 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -268,7 +268,8 @@ class SamlHandler(BaseHandler): user_id = UserID( map_username_to_mxid_localpart(attrval), self.server_name ).to_string() - logger.info( + + logger.debug( "Looking for existing account based on mapped %s %s", self._grandfathered_mxid_source_attribute, user_id, @@ -324,7 +325,7 @@ class SamlHandler(BaseHandler): if contains_invalid_mxid_characters(localpart): raise MappingException("localpart is invalid: %s" % (localpart,)) - logger.info("Mapped SAML user to local part %s", localpart) + logger.debug("Mapped SAML user to local part %s", localpart) registered_user_id = await self._registration_handler.register_user( localpart=localpart, default_display_name=displayname, diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 9cb1866a71..cf7cb7754a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -71,19 +71,25 @@ class SsoHandler(BaseHandler): Returns: The mxid of a previously seen user. """ - # Check if we already have a mapping for this user. - logger.info( + logger.debug( "Looking for existing mapping for user %s:%s", auth_provider_id, remote_user_id, ) + + # Check if we already have a mapping for this user. previously_registered_user_id = await self.store.get_user_by_external_id( auth_provider_id, remote_user_id, ) # A match was found, return the user ID. if previously_registered_user_id is not None: - logger.info("Found existing mapping %s", previously_registered_user_id) + logger.info( + "Found existing mapping for IdP '%s' and remote_user_id '%s': %s", + auth_provider_id, + remote_user_id, + previously_registered_user_id, + ) return previously_registered_user_id # No match. -- cgit 1.5.1 From 6fde6aa9c02d35e0a908437ea49b275df9b58427 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Nov 2020 13:28:03 -0500 Subject: Properly report user-agent/IP during registration of SSO users. (#8784) This also expands type-hints to the SSO and registration code. Refactors the CAS code to more closely match OIDC/SAML. --- changelog.d/8784.misc | 1 + mypy.ini | 1 + synapse/handlers/cas_handler.py | 71 +++++++++---- synapse/handlers/oidc_handler.py | 2 +- synapse/handlers/register.py | 214 +++++++++++++++++++++------------------ synapse/handlers/saml_handler.py | 6 +- 6 files changed, 173 insertions(+), 122 deletions(-) create mode 100644 changelog.d/8784.misc (limited to 'synapse/handlers') diff --git a/changelog.d/8784.misc b/changelog.d/8784.misc new file mode 100644 index 0000000000..18a4263398 --- /dev/null +++ b/changelog.d/8784.misc @@ -0,0 +1 @@ +Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. diff --git a/mypy.ini b/mypy.ini index fc9f8d8050..0cf7c93f45 100644 --- a/mypy.ini +++ b/mypy.ini @@ -37,6 +37,7 @@ files = synapse/handlers/presence.py, synapse/handlers/profile.py, synapse/handlers/read_marker.py, + synapse/handlers/register.py, synapse/handlers/room.py, synapse/handlers/room_member.py, synapse/handlers/room_member_worker.py, diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index 048a3b3c0b..f4ea0a9767 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -14,7 +14,7 @@ # limitations under the License. import logging import urllib -from typing import Dict, Optional, Tuple +from typing import TYPE_CHECKING, Dict, Optional, Tuple from xml.etree import ElementTree as ET from twisted.web.client import PartialDownloadError @@ -23,6 +23,9 @@ from synapse.api.errors import Codes, LoginError from synapse.http.site import SynapseRequest from synapse.types import UserID, map_username_to_mxid_localpart +if TYPE_CHECKING: + from synapse.app.homeserver import HomeServer + logger = logging.getLogger(__name__) @@ -31,10 +34,10 @@ class CasHandler: Utility class for to handle the response from a CAS SSO service. Args: - hs (synapse.server.HomeServer) + hs """ - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): self.hs = hs self._hostname = hs.hostname self._auth_handler = hs.get_auth_handler() @@ -200,27 +203,57 @@ class CasHandler: args["session"] = session username, user_display_name = await self._validate_ticket(ticket, args) - localpart = map_username_to_mxid_localpart(username) - user_id = UserID(localpart, self._hostname).to_string() - registered_user_id = await self._auth_handler.check_user_exists(user_id) + # Pull out the user-agent and IP from the request. + user_agent = request.get_user_agent("") + ip_address = self.hs.get_ip_from_request(request) + + # Get the matrix ID from the CAS username. + user_id = await self._map_cas_user_to_matrix_user( + username, user_display_name, user_agent, ip_address + ) if session: await self._auth_handler.complete_sso_ui_auth( - registered_user_id, session, request, + user_id, session, request, ) - else: - if not registered_user_id: - # Pull out the user-agent and IP from the request. - user_agent = request.get_user_agent("") - ip_address = self.hs.get_ip_from_request(request) - - registered_user_id = await self._registration_handler.register_user( - localpart=localpart, - default_display_name=user_display_name, - user_agent_ips=(user_agent, ip_address), - ) + # If this not a UI auth request than there must be a redirect URL. + assert client_redirect_url await self._auth_handler.complete_sso_login( - registered_user_id, request, client_redirect_url + user_id, request, client_redirect_url ) + + async def _map_cas_user_to_matrix_user( + self, + remote_user_id: str, + display_name: Optional[str], + user_agent: str, + ip_address: str, + ) -> str: + """ + Given a CAS username, retrieve the user ID for it and possibly register the user. + + Args: + remote_user_id: The username from the CAS response. + display_name: The display name from the CAS response. + user_agent: The user agent of the client making the request. + ip_address: The IP address of the client making the request. + + Returns: + The user ID associated with this response. + """ + + localpart = map_username_to_mxid_localpart(remote_user_id) + user_id = UserID(localpart, self._hostname).to_string() + registered_user_id = await self._auth_handler.check_user_exists(user_id) + + # If the user does not exist, register it. + if not registered_user_id: + registered_user_id = await self._registration_handler.register_user( + localpart=localpart, + default_display_name=display_name, + user_agent_ips=[(user_agent, ip_address)], + ) + + return registered_user_id diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 4bfd8d5617..34de9109ea 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -925,7 +925,7 @@ class OidcHandler(BaseHandler): registered_user_id = await self._registration_handler.register_user( localpart=localpart, default_display_name=attributes["display_name"], - user_agent_ips=(user_agent, ip_address), + user_agent_ips=[(user_agent, ip_address)], ) await self.store.record_user_external_id( diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 252f700786..0d85fd0868 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -15,10 +15,12 @@ """Contains functions for registering clients.""" import logging +from typing import TYPE_CHECKING, List, Optional, Tuple from synapse import types from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType from synapse.api.errors import AuthError, Codes, ConsentNotGivenError, SynapseError +from synapse.appservice import ApplicationService from synapse.config.server import is_threepid_reserved from synapse.http.servlet import assert_params_in_dict from synapse.replication.http.login import RegisterDeviceReplicationServlet @@ -32,16 +34,14 @@ from synapse.types import RoomAlias, UserID, create_requester from ._base import BaseHandler +if TYPE_CHECKING: + from synapse.app.homeserver import HomeServer + logger = logging.getLogger(__name__) class RegistrationHandler(BaseHandler): - def __init__(self, hs): - """ - - Args: - hs (synapse.server.HomeServer): - """ + def __init__(self, hs: "HomeServer"): super().__init__(hs) self.hs = hs self.auth = hs.get_auth() @@ -71,7 +71,10 @@ class RegistrationHandler(BaseHandler): self.session_lifetime = hs.config.session_lifetime async def check_username( - self, localpart, guest_access_token=None, assigned_user_id=None + self, + localpart: str, + guest_access_token: Optional[str] = None, + assigned_user_id: Optional[str] = None, ): if types.contains_invalid_mxid_characters(localpart): raise SynapseError( @@ -140,39 +143,45 @@ class RegistrationHandler(BaseHandler): async def register_user( self, - localpart=None, - password_hash=None, - guest_access_token=None, - make_guest=False, - admin=False, - threepid=None, - user_type=None, - default_display_name=None, - address=None, - bind_emails=[], - by_admin=False, - user_agent_ips=None, - ): + localpart: Optional[str] = None, + password_hash: Optional[str] = None, + guest_access_token: Optional[str] = None, + make_guest: bool = False, + admin: bool = False, + threepid: Optional[dict] = None, + user_type: Optional[str] = None, + default_display_name: Optional[str] = None, + address: Optional[str] = None, + bind_emails: List[str] = [], + by_admin: bool = False, + user_agent_ips: Optional[List[Tuple[str, str]]] = None, + ) -> str: """Registers a new client on the server. Args: localpart: The local part of the user ID to register. If None, one will be generated. - password_hash (str|None): The hashed password to assign to this user so they can + password_hash: The hashed password to assign to this user so they can login again. This can be None which means they cannot login again via a password (e.g. the user is an application service user). - user_type (str|None): type of user. One of the values from + guest_access_token: The access token used when this was a guest + account. + make_guest: True if the the new user should be guest, + false to add a regular user account. + admin: True if the user should be registered as a server admin. + threepid: The threepid used for registering, if any. + user_type: type of user. One of the values from api.constants.UserTypes, or None for a normal user. - default_display_name (unicode|None): if set, the new user's displayname + default_display_name: if set, the new user's displayname will be set to this. Defaults to 'localpart'. - address (str|None): the IP address used to perform the registration. - bind_emails (List[str]): list of emails to bind to this account. - by_admin (bool): True if this registration is being made via the + address: the IP address used to perform the registration. + bind_emails: list of emails to bind to this account. + by_admin: True if this registration is being made via the admin api, otherwise False. - user_agent_ips (List[(str, str)]): Tuples of IP addresses and user-agents used + user_agent_ips: Tuples of IP addresses and user-agents used during the registration process. Returns: - str: user_id + The registere user_id. Raises: SynapseError if there was a problem registering. """ @@ -236,8 +245,10 @@ class RegistrationHandler(BaseHandler): else: # autogen a sequential user ID fail_count = 0 - user = None - while not user: + # If a default display name is not given, generate one. + generate_display_name = default_display_name is None + # This breaks on successful registration *or* errors after 10 failures. + while True: # Fail after being unable to find a suitable ID a few times if fail_count > 10: raise SynapseError(500, "Unable to find a suitable guest user ID") @@ -246,7 +257,7 @@ class RegistrationHandler(BaseHandler): user = UserID(localpart, self.hs.hostname) user_id = user.to_string() self.check_user_id_not_appservice_exclusive(user_id) - if default_display_name is None: + if generate_display_name: default_display_name = localpart try: await self.register_with_store( @@ -262,8 +273,6 @@ class RegistrationHandler(BaseHandler): break except SynapseError: # if user id is taken, just generate another - user = None - user_id = None fail_count += 1 if not self.hs.config.user_consent_at_registration: @@ -295,7 +304,7 @@ class RegistrationHandler(BaseHandler): return user_id - async def _create_and_join_rooms(self, user_id: str): + async def _create_and_join_rooms(self, user_id: str) -> None: """ Create the auto-join rooms and join or invite the user to them. @@ -379,7 +388,7 @@ class RegistrationHandler(BaseHandler): except Exception as e: logger.error("Failed to join new user to %r: %r", r, e) - async def _join_rooms(self, user_id: str): + async def _join_rooms(self, user_id: str) -> None: """ Join or invite the user to the auto-join rooms. @@ -425,6 +434,9 @@ class RegistrationHandler(BaseHandler): # Send the invite, if necessary. if requires_invite: + # If an invite is required, there must be a auto-join user ID. + assert self.hs.config.registration.auto_join_user_id + await room_member_handler.update_membership( requester=create_requester( self.hs.config.registration.auto_join_user_id, @@ -456,7 +468,7 @@ class RegistrationHandler(BaseHandler): except Exception as e: logger.error("Failed to join new user to %r: %r", r, e) - async def _auto_join_rooms(self, user_id: str): + async def _auto_join_rooms(self, user_id: str) -> None: """Automatically joins users to auto join rooms - creating the room in the first place if the user is the first to be created. @@ -479,16 +491,16 @@ class RegistrationHandler(BaseHandler): else: await self._join_rooms(user_id) - async def post_consent_actions(self, user_id): + async def post_consent_actions(self, user_id: str) -> None: """A series of registration actions that can only be carried out once consent has been granted Args: - user_id (str): The user to join + user_id: The user to join """ await self._auto_join_rooms(user_id) - async def appservice_register(self, user_localpart, as_token): + async def appservice_register(self, user_localpart: str, as_token: str) -> str: user = UserID(user_localpart, self.hs.hostname) user_id = user.to_string() service = self.store.get_app_service_by_token(as_token) @@ -513,7 +525,9 @@ class RegistrationHandler(BaseHandler): ) return user_id - def check_user_id_not_appservice_exclusive(self, user_id, allowed_appservice=None): + def check_user_id_not_appservice_exclusive( + self, user_id: str, allowed_appservice: Optional[ApplicationService] = None + ) -> None: # don't allow people to register the server notices mxid if self._server_notices_mxid is not None: if user_id == self._server_notices_mxid: @@ -537,12 +551,12 @@ class RegistrationHandler(BaseHandler): errcode=Codes.EXCLUSIVE, ) - def check_registration_ratelimit(self, address): + def check_registration_ratelimit(self, address: Optional[str]) -> None: """A simple helper method to check whether the registration rate limit has been hit for a given IP address Args: - address (str|None): the IP address used to perform the registration. If this is + address: the IP address used to perform the registration. If this is None, no ratelimiting will be performed. Raises: @@ -553,42 +567,39 @@ class RegistrationHandler(BaseHandler): self.ratelimiter.ratelimit(address) - def register_with_store( + async def register_with_store( self, - user_id, - password_hash=None, - was_guest=False, - make_guest=False, - appservice_id=None, - create_profile_with_displayname=None, - admin=False, - user_type=None, - address=None, - shadow_banned=False, - ): + user_id: str, + password_hash: Optional[str] = None, + was_guest: bool = False, + make_guest: bool = False, + appservice_id: Optional[str] = None, + create_profile_with_displayname: Optional[str] = None, + admin: bool = False, + user_type: Optional[str] = None, + address: Optional[str] = None, + shadow_banned: bool = False, + ) -> None: """Register user in the datastore. Args: - user_id (str): The desired user ID to register. - password_hash (str|None): Optional. The password hash for this user. - was_guest (bool): Optional. Whether this is a guest account being + user_id: The desired user ID to register. + password_hash: Optional. The password hash for this user. + was_guest: Optional. Whether this is a guest account being upgraded to a non-guest account. - make_guest (boolean): True if the the new user should be guest, + make_guest: True if the the new user should be guest, false to add a regular user account. - appservice_id (str|None): The ID of the appservice registering the user. - create_profile_with_displayname (unicode|None): Optionally create a + appservice_id: The ID of the appservice registering the user. + create_profile_with_displayname: Optionally create a profile for the user, setting their displayname to the given value - admin (boolean): is an admin user? - user_type (str|None): type of user. One of the values from + admin: is an admin user? + user_type: type of user. One of the values from api.constants.UserTypes, or None for a normal user. - address (str|None): the IP address used to perform the registration. - shadow_banned (bool): Whether to shadow-ban the user - - Returns: - Awaitable + address: the IP address used to perform the registration. + shadow_banned: Whether to shadow-ban the user """ if self.hs.config.worker_app: - return self._register_client( + await self._register_client( user_id=user_id, password_hash=password_hash, was_guest=was_guest, @@ -601,7 +612,7 @@ class RegistrationHandler(BaseHandler): shadow_banned=shadow_banned, ) else: - return self.store.register_user( + await self.store.register_user( user_id=user_id, password_hash=password_hash, was_guest=was_guest, @@ -614,22 +625,24 @@ class RegistrationHandler(BaseHandler): ) async def register_device( - self, user_id, device_id, initial_display_name, is_guest=False - ): + self, + user_id: str, + device_id: Optional[str], + initial_display_name: Optional[str], + is_guest: bool = False, + ) -> Tuple[str, str]: """Register a device for a user and generate an access token. The access token will be limited by the homeserver's session_lifetime config. Args: - user_id (str): full canonical @user:id - device_id (str|None): The device ID to check, or None to generate - a new one. - initial_display_name (str|None): An optional display name for the - device. - is_guest (bool): Whether this is a guest account + user_id: full canonical @user:id + device_id: The device ID to check, or None to generate a new one. + initial_display_name: An optional display name for the device. + is_guest: Whether this is a guest account Returns: - tuple[str, str]: Tuple of device ID and access token + Tuple of device ID and access token """ if self.hs.config.worker_app: @@ -649,7 +662,7 @@ class RegistrationHandler(BaseHandler): ) valid_until_ms = self.clock.time_msec() + self.session_lifetime - device_id = await self.device_handler.check_device_registered( + registered_device_id = await self.device_handler.check_device_registered( user_id, device_id, initial_display_name ) if is_guest: @@ -659,20 +672,21 @@ class RegistrationHandler(BaseHandler): ) else: access_token = await self._auth_handler.get_access_token_for_user_id( - user_id, device_id=device_id, valid_until_ms=valid_until_ms + user_id, device_id=registered_device_id, valid_until_ms=valid_until_ms ) - return (device_id, access_token) + return (registered_device_id, access_token) - async def post_registration_actions(self, user_id, auth_result, access_token): + async def post_registration_actions( + self, user_id: str, auth_result: dict, access_token: Optional[str] + ) -> None: """A user has completed registration Args: - user_id (str): The user ID that consented - auth_result (dict): The authenticated credentials of the newly - registered user. - access_token (str|None): The access token of the newly logged in - device, or None if `inhibit_login` enabled. + user_id: The user ID that consented + auth_result: The authenticated credentials of the newly registered user. + access_token: The access token of the newly logged in device, or + None if `inhibit_login` enabled. """ if self.hs.config.worker_app: await self._post_registration_client( @@ -698,19 +712,20 @@ class RegistrationHandler(BaseHandler): if auth_result and LoginType.TERMS in auth_result: await self._on_user_consented(user_id, self.hs.config.user_consent_version) - async def _on_user_consented(self, user_id, consent_version): + async def _on_user_consented(self, user_id: str, consent_version: str) -> None: """A user consented to the terms on registration Args: - user_id (str): The user ID that consented. - consent_version (str): version of the policy the user has - consented to. + user_id: The user ID that consented. + consent_version: version of the policy the user has consented to. """ logger.info("%s has consented to the privacy policy", user_id) await self.store.user_set_consent_version(user_id, consent_version) await self.post_consent_actions(user_id) - async def _register_email_threepid(self, user_id, threepid, token): + async def _register_email_threepid( + self, user_id: str, threepid: dict, token: Optional[str] + ) -> None: """Add an email address as a 3pid identifier Also adds an email pusher for the email address, if configured in the @@ -719,10 +734,9 @@ class RegistrationHandler(BaseHandler): Must be called on master. Args: - user_id (str): id of user - threepid (object): m.login.email.identity auth response - token (str|None): access_token for the user, or None if not logged - in. + user_id: id of user + threepid: m.login.email.identity auth response + token: access_token for the user, or None if not logged in. """ reqd = ("medium", "address", "validated_at") if any(x not in threepid for x in reqd): @@ -748,6 +762,8 @@ class RegistrationHandler(BaseHandler): # up when the access token is saved, but that's quite an # invasive change I'd rather do separately. user_tuple = await self.store.get_user_by_access_token(token) + # The token better still exist. + assert user_tuple token_id = user_tuple.token_id await self.pusher_pool.add_pusher( @@ -762,14 +778,14 @@ class RegistrationHandler(BaseHandler): data={}, ) - async def _register_msisdn_threepid(self, user_id, threepid): + async def _register_msisdn_threepid(self, user_id: str, threepid: dict) -> None: """Add a phone number as a 3pid identifier Must be called on master. Args: - user_id (str): id of user - threepid (object): m.login.msisdn auth response + user_id: id of user + threepid: m.login.msisdn auth response """ try: assert_params_in_dict(threepid, ["medium", "address", "validated_at"]) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index f4e8cbeac8..37ab42f050 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -39,7 +39,7 @@ from synapse.util.async_helpers import Linearizer from synapse.util.iterutils import chunk_seq if TYPE_CHECKING: - import synapse.server + from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -56,7 +56,7 @@ class Saml2SessionData: class SamlHandler(BaseHandler): - def __init__(self, hs: "synapse.server.HomeServer"): + def __init__(self, hs: "HomeServer"): super().__init__(hs) self._saml_client = Saml2Client(hs.config.saml2_sp_config) self._saml_idp_entityid = hs.config.saml2_idp_entityid @@ -330,7 +330,7 @@ class SamlHandler(BaseHandler): localpart=localpart, default_display_name=displayname, bind_emails=emails, - user_agent_ips=(user_agent, ip_address), + user_agent_ips=[(user_agent, ip_address)], ) await self.store.record_user_external_id( -- cgit 1.5.1 From 4fd222ad704767e08c41a60690c4b499ed788b63 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 25 Nov 2020 10:04:22 -0500 Subject: Support trying multiple localparts for OpenID Connect. (#8801) Abstracts the SAML and OpenID Connect code which attempts to regenerate the localpart of a matrix ID if it is already in use. --- changelog.d/8801.feature | 1 + docs/sso_mapping_providers.md | 11 ++- synapse/handlers/oidc_handler.py | 120 +++++++++++++----------------- synapse/handlers/saml_handler.py | 91 +++++++---------------- synapse/handlers/sso.py | 155 ++++++++++++++++++++++++++++++++++++++- tests/handlers/test_oidc.py | 88 +++++++++++++++++++++- 6 files changed, 330 insertions(+), 136 deletions(-) create mode 100644 changelog.d/8801.feature (limited to 'synapse/handlers') diff --git a/changelog.d/8801.feature b/changelog.d/8801.feature new file mode 100644 index 0000000000..77f7fe4e5d --- /dev/null +++ b/changelog.d/8801.feature @@ -0,0 +1 @@ +Add support for re-trying generation of a localpart for OpenID Connect mapping providers. diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md index 707dd73978..dee53b5d40 100644 --- a/docs/sso_mapping_providers.md +++ b/docs/sso_mapping_providers.md @@ -63,13 +63,22 @@ A custom mapping provider must specify the following methods: information from. - This method must return a string, which is the unique identifier for the user. Commonly the ``sub`` claim of the response. -* `map_user_attributes(self, userinfo, token)` +* `map_user_attributes(self, userinfo, token, failures)` - This method must be async. - Arguments: - `userinfo` - A `authlib.oidc.core.claims.UserInfo` object to extract user information from. - `token` - A dictionary which includes information necessary to make further requests to the OpenID provider. + - `failures` - An `int` that represents the amount of times the returned + mxid localpart mapping has failed. This should be used + to create a deduplicated mxid localpart which should be + returned instead. For example, if this method returns + `john.doe` as the value of `localpart` in the returned + dict, and that is already taken on the homeserver, this + method will be called again with the same parameters but + with failures=1. The method should then return a different + `localpart` value, such as `john.doe1`. - Returns a dictionary with two keys: - localpart: A required string, used to generate the Matrix ID. - displayname: An optional string, the display name for the user. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 34de9109ea..78c4e94a9d 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -12,6 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import inspect import logging from typing import TYPE_CHECKING, Dict, Generic, List, Optional, Tuple, TypeVar from urllib.parse import urlencode @@ -35,15 +36,10 @@ from twisted.web.client import readBody from synapse.config import ConfigError from synapse.handlers._base import BaseHandler -from synapse.handlers.sso import MappingException +from synapse.handlers.sso import MappingException, UserAttributes from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable -from synapse.types import ( - JsonDict, - UserID, - contains_invalid_mxid_characters, - map_username_to_mxid_localpart, -) +from synapse.types import JsonDict, map_username_to_mxid_localpart from synapse.util import json_decoder if TYPE_CHECKING: @@ -869,73 +865,51 @@ class OidcHandler(BaseHandler): # to be strings. remote_user_id = str(remote_user_id) - # first of all, check if we already have a mapping for this user - previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id( - self._auth_provider_id, remote_user_id, + # Older mapping providers don't accept the `failures` argument, so we + # try and detect support. + mapper_signature = inspect.signature( + self._user_mapping_provider.map_user_attributes ) - if previously_registered_user_id: - return previously_registered_user_id + supports_failures = "failures" in mapper_signature.parameters - # Otherwise, generate a new user. - try: - attributes = await self._user_mapping_provider.map_user_attributes( - userinfo, token - ) - except Exception as e: - raise MappingException( - "Could not extract user attributes from OIDC response: " + str(e) - ) + async def oidc_response_to_user_attributes(failures: int) -> UserAttributes: + """ + Call the mapping provider to map the OIDC userinfo and token to user attributes. - logger.debug( - "Retrieved user attributes from user mapping provider: %r", attributes - ) + This is backwards compatibility for abstraction for the SSO handler. + """ + if supports_failures: + attributes = await self._user_mapping_provider.map_user_attributes( + userinfo, token, failures + ) + else: + # If the mapping provider does not support processing failures, + # do not continually generate the same Matrix ID since it will + # continue to already be in use. Note that the error raised is + # arbitrary and will get turned into a MappingException. + if failures: + raise RuntimeError( + "Mapping provider does not support de-duplicating Matrix IDs" + ) - localpart = attributes["localpart"] - if not localpart: - raise MappingException( - "Error parsing OIDC response: OIDC mapping provider plugin " - "did not return a localpart value" - ) + attributes = await self._user_mapping_provider.map_user_attributes( # type: ignore + userinfo, token + ) - user_id = UserID(localpart, self.server_name).to_string() - users = await self.store.get_users_by_id_case_insensitive(user_id) - if users: - if self._allow_existing_users: - if len(users) == 1: - registered_user_id = next(iter(users)) - elif user_id in users: - registered_user_id = user_id - else: - raise MappingException( - "Attempted to login as '{}' but it matches more than one user inexactly: {}".format( - user_id, list(users.keys()) - ) - ) - else: - # This mxid is taken - raise MappingException("mxid '{}' is already taken".format(user_id)) - else: - # Since the localpart is provided via a potentially untrusted module, - # ensure the MXID is valid before registering. - if contains_invalid_mxid_characters(localpart): - raise MappingException("localpart is invalid: %s" % (localpart,)) - - # It's the first time this user is logging in and the mapped mxid was - # not taken, register the user - registered_user_id = await self._registration_handler.register_user( - localpart=localpart, - default_display_name=attributes["display_name"], - user_agent_ips=[(user_agent, ip_address)], - ) + return UserAttributes(**attributes) - await self.store.record_user_external_id( - self._auth_provider_id, remote_user_id, registered_user_id, + return await self._sso_handler.get_mxid_from_sso( + self._auth_provider_id, + remote_user_id, + user_agent, + ip_address, + oidc_response_to_user_attributes, + self._allow_existing_users, ) - return registered_user_id -UserAttribute = TypedDict( - "UserAttribute", {"localpart": str, "display_name": Optional[str]} +UserAttributeDict = TypedDict( + "UserAttributeDict", {"localpart": str, "display_name": Optional[str]} ) C = TypeVar("C") @@ -978,13 +952,15 @@ class OidcMappingProvider(Generic[C]): raise NotImplementedError() async def map_user_attributes( - self, userinfo: UserInfo, token: Token - ) -> UserAttribute: + self, userinfo: UserInfo, token: Token, failures: int + ) -> UserAttributeDict: """Map a `UserInfo` object into user attributes. Args: userinfo: An object representing the user given by the OIDC provider token: A dict with the tokens returned by the provider + failures: How many times a call to this function with this + UserInfo has resulted in a failure. Returns: A dict containing the ``localpart`` and (optionally) the ``display_name`` @@ -1084,13 +1060,17 @@ class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]): return userinfo[self._config.subject_claim] async def map_user_attributes( - self, userinfo: UserInfo, token: Token - ) -> UserAttribute: + self, userinfo: UserInfo, token: Token, failures: int + ) -> UserAttributeDict: localpart = self._config.localpart_template.render(user=userinfo).strip() # Ensure only valid characters are included in the MXID. localpart = map_username_to_mxid_localpart(localpart) + # Append suffix integer if last call to this function failed to produce + # a usable mxid. + localpart += str(failures) if failures else "" + display_name = None # type: Optional[str] if self._config.display_name_template is not None: display_name = self._config.display_name_template.render( @@ -1100,7 +1080,7 @@ class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]): if display_name == "": display_name = None - return UserAttribute(localpart=localpart, display_name=display_name) + return UserAttributeDict(localpart=localpart, display_name=display_name) async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict: extras = {} # type: Dict[str, str] diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 37ab42f050..34db10ffe4 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -25,13 +25,12 @@ from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.config.saml2_config import SamlAttributeRequirement from synapse.handlers._base import BaseHandler -from synapse.handlers.sso import MappingException +from synapse.handlers.sso import MappingException, UserAttributes from synapse.http.servlet import parse_string from synapse.http.site import SynapseRequest from synapse.module_api import ModuleApi from synapse.types import ( UserID, - contains_invalid_mxid_characters, map_username_to_mxid_localpart, mxid_localpart_allowed_characters, ) @@ -250,14 +249,26 @@ class SamlHandler(BaseHandler): "Failed to extract remote user id from SAML response" ) - with (await self._mapping_lock.queue(self._auth_provider_id)): - # first of all, check if we already have a mapping for this user - previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id( - self._auth_provider_id, remote_user_id, + async def saml_response_to_remapped_user_attributes( + failures: int, + ) -> UserAttributes: + """ + Call the mapping provider to map a SAML response to user attributes and coerce the result into the standard form. + + This is backwards compatibility for abstraction for the SSO handler. + """ + # Call the mapping provider. + result = self._user_mapping_provider.saml_response_to_user_attributes( + saml2_auth, failures, client_redirect_url + ) + # Remap some of the results. + return UserAttributes( + localpart=result.get("mxid_localpart"), + display_name=result.get("displayname"), + emails=result.get("emails"), ) - if previously_registered_user_id: - return previously_registered_user_id + with (await self._mapping_lock.queue(self._auth_provider_id)): # backwards-compatibility hack: see if there is an existing user with a # suitable mapping from the uid if ( @@ -284,59 +295,13 @@ class SamlHandler(BaseHandler): ) return registered_user_id - # Map saml response to user attributes using the configured mapping provider - for i in range(1000): - attribute_dict = self._user_mapping_provider.saml_response_to_user_attributes( - saml2_auth, i, client_redirect_url=client_redirect_url, - ) - - logger.debug( - "Retrieved SAML attributes from user mapping provider: %s " - "(attempt %d)", - attribute_dict, - i, - ) - - localpart = attribute_dict.get("mxid_localpart") - if not localpart: - raise MappingException( - "Error parsing SAML2 response: SAML mapping provider plugin " - "did not return a mxid_localpart value" - ) - - displayname = attribute_dict.get("displayname") - emails = attribute_dict.get("emails", []) - - # Check if this mxid already exists - if not await self.store.get_users_by_id_case_insensitive( - UserID(localpart, self.server_name).to_string() - ): - # This mxid is free - break - else: - # Unable to generate a username in 1000 iterations - # Break and return error to the user - raise MappingException( - "Unable to generate a Matrix ID from the SAML response" - ) - - # Since the localpart is provided via a potentially untrusted module, - # ensure the MXID is valid before registering. - if contains_invalid_mxid_characters(localpart): - raise MappingException("localpart is invalid: %s" % (localpart,)) - - logger.debug("Mapped SAML user to local part %s", localpart) - registered_user_id = await self._registration_handler.register_user( - localpart=localpart, - default_display_name=displayname, - bind_emails=emails, - user_agent_ips=[(user_agent, ip_address)], - ) - - await self.store.record_user_external_id( - self._auth_provider_id, remote_user_id, registered_user_id + return await self._sso_handler.get_mxid_from_sso( + self._auth_provider_id, + remote_user_id, + user_agent, + ip_address, + saml_response_to_remapped_user_attributes, ) - return registered_user_id def expire_sessions(self): expire_before = self.clock.time_msec() - self._saml2_session_lifetime @@ -451,11 +416,11 @@ class DefaultSamlMappingProvider: ) # Use the configured mapper for this mxid_source - base_mxid_localpart = self._mxid_mapper(mxid_source) + localpart = self._mxid_mapper(mxid_source) # Append suffix integer if last call to this function failed to produce - # a usable mxid - localpart = base_mxid_localpart + (str(failures) if failures else "") + # a usable mxid. + localpart += str(failures) if failures else "" # Retrieve the display name from the saml response # If displayname is None, the mxid_localpart will be used instead diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index cf7cb7754a..d963082210 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -13,10 +13,13 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional + +import attr from synapse.handlers._base import BaseHandler from synapse.http.server import respond_with_html +from synapse.types import UserID, contains_invalid_mxid_characters if TYPE_CHECKING: from synapse.server import HomeServer @@ -29,9 +32,20 @@ class MappingException(Exception): """ +@attr.s +class UserAttributes: + localpart = attr.ib(type=str) + display_name = attr.ib(type=Optional[str], default=None) + emails = attr.ib(type=List[str], default=attr.Factory(list)) + + class SsoHandler(BaseHandler): + # The number of attempts to ask the mapping provider for when generating an MXID. + _MAP_USERNAME_RETRIES = 1000 + def __init__(self, hs: "HomeServer"): super().__init__(hs) + self._registration_handler = hs.get_registration_handler() self._error_template = hs.config.sso_error_template def render_error( @@ -94,3 +108,142 @@ class SsoHandler(BaseHandler): # No match. return None + + async def get_mxid_from_sso( + self, + auth_provider_id: str, + remote_user_id: str, + user_agent: str, + ip_address: str, + sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]], + allow_existing_users: bool = False, + ) -> str: + """ + Given an SSO ID, retrieve the user ID for it and possibly register the user. + + This first checks if the SSO ID has previously been linked to a matrix ID, + if it has that matrix ID is returned regardless of the current mapping + logic. + + The mapping function is called (potentially multiple times) to generate + a localpart for the user. + + If an unused localpart is generated, the user is registered from the + given user-agent and IP address and the SSO ID is linked to this matrix + ID for subsequent calls. + + If allow_existing_users is true the mapping function is only called once + and results in: + + 1. The use of a previously registered matrix ID. In this case, the + SSO ID is linked to the matrix ID. (Note it is possible that + other SSO IDs are linked to the same matrix ID.) + 2. An unused localpart, in which case the user is registered (as + discussed above). + 3. An error if the generated localpart matches multiple pre-existing + matrix IDs. Generally this should not happen. + + Args: + auth_provider_id: A unique identifier for this SSO provider, e.g. + "oidc" or "saml". + remote_user_id: The unique identifier from the SSO provider. + user_agent: The user agent of the client making the request. + ip_address: The IP address of the client making the request. + sso_to_matrix_id_mapper: A callable to generate the user attributes. + The only parameter is an integer which represents the amount of + times the returned mxid localpart mapping has failed. + allow_existing_users: True if the localpart returned from the + mapping provider can be linked to an existing matrix ID. + + Returns: + The user ID associated with the SSO response. + + Raises: + MappingException if there was a problem mapping the response to a user. + RedirectException: some mapping providers may raise this if they need + to redirect to an interstitial page. + + """ + # first of all, check if we already have a mapping for this user + previously_registered_user_id = await self.get_sso_user_by_remote_user_id( + auth_provider_id, remote_user_id, + ) + if previously_registered_user_id: + return previously_registered_user_id + + # Otherwise, generate a new user. + for i in range(self._MAP_USERNAME_RETRIES): + try: + attributes = await sso_to_matrix_id_mapper(i) + except Exception as e: + raise MappingException( + "Could not extract user attributes from SSO response: " + str(e) + ) + + logger.debug( + "Retrieved user attributes from user mapping provider: %r (attempt %d)", + attributes, + i, + ) + + if not attributes.localpart: + raise MappingException( + "Error parsing SSO response: SSO mapping provider plugin " + "did not return a localpart value" + ) + + # Check if this mxid already exists + user_id = UserID(attributes.localpart, self.server_name).to_string() + users = await self.store.get_users_by_id_case_insensitive(user_id) + # Note, if allow_existing_users is true then the loop is guaranteed + # to end on the first iteration: either by matching an existing user, + # raising an error, or registering a new user. See the docstring for + # more in-depth an explanation. + if users and allow_existing_users: + # If an existing matrix ID is returned, then use it. + if len(users) == 1: + previously_registered_user_id = next(iter(users)) + elif user_id in users: + previously_registered_user_id = user_id + else: + # Do not attempt to continue generating Matrix IDs. + raise MappingException( + "Attempted to login as '{}' but it matches more than one user inexactly: {}".format( + user_id, users + ) + ) + + # Future logins should also match this user ID. + await self.store.record_user_external_id( + auth_provider_id, remote_user_id, previously_registered_user_id + ) + + return previously_registered_user_id + + elif not users: + # This mxid is free + break + else: + # Unable to generate a username in 1000 iterations + # Break and return error to the user + raise MappingException( + "Unable to generate a Matrix ID from the SSO response" + ) + + # Since the localpart is provided via a potentially untrusted module, + # ensure the MXID is valid before registering. + if contains_invalid_mxid_characters(attributes.localpart): + raise MappingException("localpart is invalid: %s" % (attributes.localpart,)) + + logger.debug("Mapped SSO user to local part %s", attributes.localpart) + registered_user_id = await self._registration_handler.register_user( + localpart=attributes.localpart, + default_display_name=attributes.display_name, + bind_emails=attributes.emails, + user_agent_ips=[(user_agent, ip_address)], + ) + + await self.store.record_user_external_id( + auth_provider_id, remote_user_id, registered_user_id + ) + return registered_user_id diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index b4fa02acc4..e880d32be6 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -89,6 +89,14 @@ class TestMappingProviderExtra(TestMappingProvider): return {"phone": userinfo["phone"]} +class TestMappingProviderFailures(TestMappingProvider): + async def map_user_attributes(self, userinfo, token, failures): + return { + "localpart": userinfo["username"] + (str(failures) if failures else ""), + "display_name": None, + } + + def simple_async_mock(return_value=None, raises=None): # AsyncMock is not available in python3.5, this mimics part of its behaviour async def cb(*args, **kwargs): @@ -152,6 +160,9 @@ class OidcHandlerTestCase(HomeserverTestCase): self.render_error = Mock(return_value=None) self.handler._sso_handler.render_error = self.render_error + # Reduce the number of attempts when generating MXIDs. + self.handler._sso_handler._MAP_USERNAME_RETRIES = 3 + return hs def metadata_edit(self, values): @@ -693,7 +704,10 @@ class OidcHandlerTestCase(HomeserverTestCase): ), MappingException, ) - self.assertEqual(str(e.value), "mxid '@test_user_3:test' is already taken") + self.assertEqual( + str(e.value), + "Could not extract user attributes from SSO response: Mapping provider does not support de-duplicating Matrix IDs", + ) @override_config({"oidc_config": {"allow_existing_users": True}}) def test_map_userinfo_to_existing_user(self): @@ -703,6 +717,8 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success( store.register_user(user_id=user.to_string(), password_hash=None) ) + + # Map a user via SSO. userinfo = { "sub": "test", "username": "test_user", @@ -715,6 +731,23 @@ class OidcHandlerTestCase(HomeserverTestCase): ) self.assertEqual(mxid, "@test_user:test") + # Note that a second SSO user can be mapped to the same Matrix ID. (This + # requires a unique sub, but something that maps to the same matrix ID, + # in this case we'll just use the same username. A more realistic example + # would be subs which are email addresses, and mapping from the localpart + # of the email, e.g. bob@foo.com and bob@bar.com -> @bob:test.) + userinfo = { + "sub": "test1", + "username": "test_user", + } + token = {} + mxid = self.get_success( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ) + ) + self.assertEqual(mxid, "@test_user:test") + # Register some non-exact matching cases. user2 = UserID.from_string("@TEST_user_2:test") self.get_success( @@ -762,6 +795,7 @@ class OidcHandlerTestCase(HomeserverTestCase): "username": "föö", } token = {} + e = self.get_failure( self.handler._map_userinfo_to_user( userinfo, token, "user-agent", "10.10.10.10" @@ -769,3 +803,55 @@ class OidcHandlerTestCase(HomeserverTestCase): MappingException, ) self.assertEqual(str(e.value), "localpart is invalid: föö") + + @override_config( + { + "oidc_config": { + "user_mapping_provider": { + "module": __name__ + ".TestMappingProviderFailures" + } + } + } + ) + def test_map_userinfo_to_user_retries(self): + """The mapping provider can retry generating an MXID if the MXID is already in use.""" + store = self.hs.get_datastore() + self.get_success( + store.register_user(user_id="@test_user:test", password_hash=None) + ) + userinfo = { + "sub": "test", + "username": "test_user", + } + token = {} + mxid = self.get_success( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ) + ) + # test_user is already taken, so test_user1 gets registered instead. + self.assertEqual(mxid, "@test_user1:test") + + # Register all of the potential users for a particular username. + self.get_success( + store.register_user(user_id="@tester:test", password_hash=None) + ) + for i in range(1, 3): + self.get_success( + store.register_user(user_id="@tester%d:test" % i, password_hash=None) + ) + + # Now attempt to map to a username, this will fail since all potential usernames are taken. + userinfo = { + "sub": "tester", + "username": "tester", + } + e = self.get_failure( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ), + MappingException, + ) + self.assertEqual( + str(e.value), "Unable to generate a Matrix ID from the SSO response" + ) -- cgit 1.5.1 From d963c69ba56ea45276ec3d11d191a20e8a38881d Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Wed, 25 Nov 2020 20:06:13 +0000 Subject: Speed up remote invite rejection database call (#8815) This is another PR that grew out of #6739. The existing code for checking whether a user is currently invited to a room when they want to leave the room looks like the following: https://github.com/matrix-org/synapse/blob/f737368a26bb9eea401fcc3a5bdd7e0b59e91f09/synapse/handlers/room_member.py#L518-L540 It calls `get_invite_for_local_user_in_room`, which will actually query *all* rooms the user has been invited to, before iterating over them and matching via the room ID. It will then return a tuple of a lot of information which we pull the event ID out of. I need to do a similar check for knocking, but this code wasn't very efficient. I then tried to write a different implementation using `StateHandler.get_current_state` but this actually didn't work as we haven't *joined* the room yet - we've only been invited to it. That means that only certain tables in Synapse have our desired `invite` membership state. One of those tables is `local_current_membership`. So I wrote a store method that just queries that table instead --- changelog.d/8815.misc | 1 + synapse/handlers/room_member.py | 16 +++++++++---- synapse/storage/databases/main/roommember.py | 34 +++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 changelog.d/8815.misc (limited to 'synapse/handlers') diff --git a/changelog.d/8815.misc b/changelog.d/8815.misc new file mode 100644 index 0000000000..647edeb568 --- /dev/null +++ b/changelog.d/8815.misc @@ -0,0 +1 @@ +Optimise the lookup for an invite from another homeserver when trying to reject it. \ No newline at end of file diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 70f8966267..13a793c05a 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -31,7 +31,6 @@ from synapse.api.errors import ( from synapse.api.ratelimiting import Ratelimiter from synapse.events import EventBase from synapse.events.snapshot import EventContext -from synapse.storage.roommember import RoomsForUser from synapse.types import JsonDict, Requester, RoomAlias, RoomID, StateMap, UserID from synapse.util.async_helpers import Linearizer from synapse.util.distributor import user_left_room @@ -515,10 +514,16 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): elif effective_membership_state == Membership.LEAVE: if not is_host_in_room: # perhaps we've been invited - invite = await self.store.get_invite_for_local_user_in_room( - user_id=target.to_string(), room_id=room_id - ) # type: Optional[RoomsForUser] - if not invite: + ( + current_membership_type, + current_membership_event_id, + ) = await self.store.get_local_current_membership_for_user_in_room( + target.to_string(), room_id + ) + if ( + current_membership_type != Membership.INVITE + or not current_membership_event_id + ): logger.info( "%s sent a leave request to %s, but that is not an active room " "on this server, and there is no pending invite", @@ -528,6 +533,7 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): raise SynapseError(404, "Not a known room") + invite = await self.store.get_event(current_membership_event_id) logger.info( "%s rejects invite to %s from %s", target, room_id, invite.sender ) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 01d9dbb36f..dcdaf09682 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -14,7 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import TYPE_CHECKING, Dict, FrozenSet, Iterable, List, Optional, Set +from typing import TYPE_CHECKING, Dict, FrozenSet, Iterable, List, Optional, Set, Tuple from synapse.api.constants import EventTypes, Membership from synapse.events import EventBase @@ -350,6 +350,38 @@ class RoomMemberWorkerStore(EventsWorkerStore): return results + async def get_local_current_membership_for_user_in_room( + self, user_id: str, room_id: str + ) -> Tuple[Optional[str], Optional[str]]: + """Retrieve the current local membership state and event ID for a user in a room. + + Args: + user_id: The ID of the user. + room_id: The ID of the room. + + Returns: + A tuple of (membership_type, event_id). Both will be None if a + room_id/user_id pair is not found. + """ + # Paranoia check. + if not self.hs.is_mine_id(user_id): + raise Exception( + "Cannot call 'get_local_current_membership_for_user_in_room' on " + "non-local user %s" % (user_id,), + ) + + results_dict = await self.db_pool.simple_select_one( + "local_current_membership", + {"room_id": room_id, "user_id": user_id}, + ("membership", "event_id"), + allow_none=True, + desc="get_local_current_membership_for_user_in_room", + ) + if not results_dict: + return None, None + + return results_dict.get("membership"), results_dict.get("event_id") + @cached(max_entries=500000, iterable=True) async def get_rooms_for_user_with_stream_ordering( self, user_id: str -- cgit 1.5.1