From 18a3721ce441dcc4c1c838b8f09da357b9436485 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 9 Dec 2020 19:18:37 +0000 Subject: [PATCH 1/6] Remove references to handler._auth_handler (and replace them with hs.get_auth_handler) --- tests/handlers/test_oidc.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 1d99a45436d7..85fe677ad56f 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -383,7 +383,8 @@ def test_callback(self): self.handler._parse_id_token = simple_async_mock(return_value=userinfo) self.handler._fetch_userinfo = simple_async_mock(return_value=userinfo) self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) - self.handler._auth_handler.complete_sso_login = simple_async_mock() + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() request = Mock( spec=[ "args", @@ -417,7 +418,7 @@ def test_callback(self): self.get_success(self.handler.handle_oidc_callback(request)) - self.handler._auth_handler.complete_sso_login.assert_called_once_with( + auth_handler.complete_sso_login.assert_called_once_with( user_id, request, client_redirect_url, {}, ) self.handler._exchange_code.assert_called_once_with(code) @@ -441,7 +442,7 @@ def test_callback(self): self.get_success(self.handler.handle_oidc_callback(request)) self.assertRenderedError("invalid_token") - self.handler._auth_handler.complete_sso_login.reset_mock() + auth_handler.complete_sso_login.reset_mock() self.handler._exchange_code.reset_mock() self.handler._parse_id_token.reset_mock() self.handler._map_userinfo_to_user.reset_mock() @@ -451,7 +452,7 @@ def test_callback(self): self.handler._scopes = [] # do not ask the "openid" scope self.get_success(self.handler.handle_oidc_callback(request)) - self.handler._auth_handler.complete_sso_login.assert_called_once_with( + auth_handler.complete_sso_login.assert_called_once_with( user_id, request, client_redirect_url, {}, ) self.handler._exchange_code.assert_called_once_with(code) @@ -615,7 +616,8 @@ def test_extra_attributes(self): self.handler._exchange_code = simple_async_mock(return_value=token) self.handler._parse_id_token = simple_async_mock(return_value=userinfo) self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) - self.handler._auth_handler.complete_sso_login = simple_async_mock() + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() request = Mock( spec=[ "args", @@ -645,7 +647,7 @@ def test_extra_attributes(self): self.get_success(self.handler.handle_oidc_callback(request)) - self.handler._auth_handler.complete_sso_login.assert_called_once_with( + auth_handler.complete_sso_login.assert_called_once_with( user_id, request, client_redirect_url, {"phone": "1234567"}, ) From ad08164bc02a4d7c32c0e6a3529dced86605d39f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 9 Dec 2020 19:25:38 +0000 Subject: [PATCH 2/6] Factor out a utility function for building Requests --- tests/handlers/test_oidc.py | 84 +++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 85fe677ad56f..b482771d3153 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -385,16 +385,6 @@ def test_callback(self): self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) auth_handler = self.hs.get_auth_handler() auth_handler.complete_sso_login = simple_async_mock() - request = Mock( - spec=[ - "args", - "getCookie", - "addCookie", - "requestHeaders", - "getClientIP", - "get_user_agent", - ] - ) code = "code" state = "state" @@ -402,19 +392,15 @@ def test_callback(self): client_redirect_url = "http://client/redirect" user_agent = "Browser" ip_address = "10.0.0.1" - request.getCookie.return_value = self.handler._generate_oidc_session_token( + session = self.handler._generate_oidc_session_token( state=state, nonce=nonce, client_redirect_url=client_redirect_url, ui_auth_session_id=None, ) - - request.args = {} - request.args[b"code"] = [code.encode("utf-8")] - request.args[b"state"] = [state.encode("utf-8")] - - request.getClientIP.return_value = ip_address - request.get_user_agent.return_value = user_agent + request = self._build_callback_request( + code, state, session, user_agent=user_agent, ip_address=ip_address + ) self.get_success(self.handler.handle_oidc_callback(request)) @@ -618,32 +604,16 @@ def test_extra_attributes(self): self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) auth_handler = self.hs.get_auth_handler() auth_handler.complete_sso_login = simple_async_mock() - request = Mock( - spec=[ - "args", - "getCookie", - "addCookie", - "requestHeaders", - "getClientIP", - "get_user_agent", - ] - ) state = "state" client_redirect_url = "http://client/redirect" - request.getCookie.return_value = self.handler._generate_oidc_session_token( + session = self.handler._generate_oidc_session_token( state=state, nonce="nonce", client_redirect_url=client_redirect_url, ui_auth_session_id=None, ) - - request.args = {} - request.args[b"code"] = [b"code"] - request.args[b"state"] = [state.encode("utf-8")] - - request.getClientIP.return_value = "10.0.0.1" - request.get_user_agent.return_value = "Browser" + request = self._build_callback_request("code", state, session) self.get_success(self.handler.handle_oidc_callback(request)) @@ -849,3 +819,45 @@ def test_map_userinfo_to_user_retries(self): self.assertEqual( str(e.value), "Unable to generate a Matrix ID from the SSO response" ) + + def _build_callback_request( + self, + code: str, + state: str, + session: str, + user_agent: str = "Browser", + ip_address: str = "10.0.0.1", + ): + """Builds a fake SynapseRequest to mock the browser callback + + Returns a Mock object which looks like the SynapseRequest we get from a browser + after SSO (before we return to the client) + + Args: + code: the authorization code which would have been returned by the OIDC + provider + state: the "state" param which would have been passed around in the + query param. Should be the same as was embedded in the session in + _build_oidc_session. + session: the "session" which would have been passed around in the cookie. + user_agent: the user-agent to present + ip_address: the IP address to pretend the request came from + """ + request = Mock( + spec=[ + "args", + "getCookie", + "addCookie", + "requestHeaders", + "getClientIP", + "get_user_agent", + ] + ) + + request.getCookie.return_value = session + request.args = {} + request.args[b"code"] = [code.encode("utf-8")] + request.args[b"state"] = [state.encode("utf-8")] + request.getClientIP.return_value = ip_address + request.get_user_agent.return_value = user_agent + return request From 78a05c70b06b18ade1ed9f0c2144970f51277fea Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 9 Dec 2020 19:40:02 +0000 Subject: [PATCH 3/6] Remove mocks of `OidcHandler._map_userinfo_to_user` This method is going away, so mocking it out is no longer a valid approach. Instead, we mock out lower-level methods (eg _remote_id_from_userinfo), or simply allow the regular implementation to proceed and update the expectations accordingly. --- tests/handlers/test_oidc.py | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index b482771d3153..bd83a025fea0 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -374,15 +374,15 @@ def test_callback(self): "id_token": "id_token", "access_token": "access_token", } + username = "bar" userinfo = { "sub": "foo", - "preferred_username": "bar", + "username": username, } - user_id = "@foo:domain.org" + expected_user_id = "@%s:%s" % (username, self.hs.hostname) 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) - self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) auth_handler = self.hs.get_auth_handler() auth_handler.complete_sso_login = simple_async_mock() @@ -405,23 +405,21 @@ def test_callback(self): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - user_id, request, client_redirect_url, {}, + expected_user_id, request, client_redirect_url, {}, ) self.handler._exchange_code.assert_called_once_with(code) self.handler._parse_id_token.assert_called_once_with(token, nonce=nonce) - self.handler._map_userinfo_to_user.assert_called_once_with( - userinfo, token, user_agent, ip_address - ) self.handler._fetch_userinfo.assert_not_called() self.render_error.assert_not_called() # Handle mapping errors - self.handler._map_userinfo_to_user = simple_async_mock( - raises=MappingException() - ) - self.get_success(self.handler.handle_oidc_callback(request)) - self.assertRenderedError("mapping_error") - self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) + with patch.object( + self.handler, + "_remote_id_from_userinfo", + new=Mock(side_effect=MappingException()), + ): + self.get_success(self.handler.handle_oidc_callback(request)) + self.assertRenderedError("mapping_error") # Handle ID token errors self.handler._parse_id_token = simple_async_mock(raises=Exception()) @@ -431,7 +429,6 @@ def test_callback(self): auth_handler.complete_sso_login.reset_mock() self.handler._exchange_code.reset_mock() self.handler._parse_id_token.reset_mock() - self.handler._map_userinfo_to_user.reset_mock() self.handler._fetch_userinfo.reset_mock() # With userinfo fetching @@ -439,13 +436,10 @@ def test_callback(self): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - user_id, request, client_redirect_url, {}, + expected_user_id, request, client_redirect_url, {}, ) self.handler._exchange_code.assert_called_once_with(code) self.handler._parse_id_token.assert_not_called() - self.handler._map_userinfo_to_user.assert_called_once_with( - userinfo, token, user_agent, ip_address - ) self.handler._fetch_userinfo.assert_called_once_with(token) self.render_error.assert_not_called() @@ -596,12 +590,11 @@ def test_extra_attributes(self): } userinfo = { "sub": "foo", + "username": "foo", "phone": "1234567", } - user_id = "@foo:domain.org" self.handler._exchange_code = simple_async_mock(return_value=token) self.handler._parse_id_token = simple_async_mock(return_value=userinfo) - self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id) auth_handler = self.hs.get_auth_handler() auth_handler.complete_sso_login = simple_async_mock() @@ -618,7 +611,7 @@ def test_extra_attributes(self): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - user_id, request, client_redirect_url, {"phone": "1234567"}, + "@foo:test", request, client_redirect_url, {"phone": "1234567"}, ) def test_map_userinfo_to_user(self): From e7bdf1ed999ba2b88da426102b6def185beacd2e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 9 Dec 2020 19:44:31 +0000 Subject: [PATCH 4/6] Remove references to `OidcHandler._map_userinfo_to_user` from tests This method is going away, so we can no longer use it as a test point. Instead we build mock "callback" requests which we pass into `handle_oidc_callback`, and verify correct behaviour by mocking out `AuthHandler.complete_sso_login`. --- tests/handlers/test_oidc.py | 153 ++++++++++++++++++------------------ 1 file changed, 75 insertions(+), 78 deletions(-) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index bd83a025fea0..f5502265053c 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -15,7 +15,7 @@ import json from urllib.parse import parse_qs, urlparse -from mock import Mock, patch +from mock import ANY, Mock, patch import pymacaroons @@ -82,7 +82,7 @@ async def map_user_attributes(self, userinfo, token, failures): } -def simple_async_mock(return_value=None, raises=None): +def simple_async_mock(return_value=None, raises=None) -> Mock: # AsyncMock is not available in python3.5, this mimics part of its behaviour async def cb(*args, **kwargs): if raises: @@ -160,6 +160,7 @@ def assertRenderedError(self, error, error_description=None): self.assertEqual(args[2], error_description) # Reset the render_error mock self.render_error.reset_mock() + return args def test_config(self): """Basic config correctly sets up the callback URL and client auth correctly.""" @@ -616,30 +617,29 @@ def test_extra_attributes(self): def test_map_userinfo_to_user(self): """Ensure that mapping the userinfo returned from a provider to an MXID works properly.""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + userinfo = { "sub": "test_user", "username": "test_user", } - # The token doesn't matter with the default user mapping provider. - token = {} - mxid = self.get_success( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ) + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user:test", ANY, ANY, {} ) - self.assertEqual(mxid, "@test_user:test") + auth_handler.complete_sso_login.reset_mock() # Some providers return an integer ID. userinfo = { "sub": 1234, "username": "test_user_2", } - mxid = self.get_success( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ) + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user_2:test", ANY, ANY, {} ) - self.assertEqual(mxid, "@test_user_2:test") + auth_handler.complete_sso_login.reset_mock() # Test if the mxid is already taken store = self.hs.get_datastore() @@ -648,14 +648,11 @@ def test_map_userinfo_to_user(self): store.register_user(user_id=user3.to_string(), password_hash=None) ) userinfo = {"sub": "test3", "username": "test_user_3"} - e = self.get_failure( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ), - MappingException, - ) - self.assertEqual( - str(e.value), "Mapping provider does not support de-duplicating Matrix IDs", + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_not_called() + self.assertRenderedError( + "mapping_error", + "Mapping provider does not support de-duplicating Matrix IDs", ) @override_config({"oidc_config": {"allow_existing_users": True}}) @@ -667,26 +664,26 @@ def test_map_userinfo_to_existing_user(self): store.register_user(user_id=user.to_string(), password_hash=None) ) + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # Map a user via SSO. 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" - ) + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + user.to_string(), ANY, ANY, {}, ) - self.assertEqual(mxid, "@test_user:test") + auth_handler.complete_sso_login.reset_mock() # Subsequent calls should map to the same mxid. - mxid = self.get_success( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ) + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + user.to_string(), ANY, ANY, {}, ) - self.assertEqual(mxid, "@test_user:test") + auth_handler.complete_sso_login.reset_mock() # 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, @@ -697,13 +694,11 @@ def test_map_userinfo_to_existing_user(self): "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._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + user.to_string(), ANY, ANY, {}, ) - self.assertEqual(mxid, "@test_user:test") + auth_handler.complete_sso_login.reset_mock() # Register some non-exact matching cases. user2 = UserID.from_string("@TEST_user_2:test") @@ -720,14 +715,11 @@ def test_map_userinfo_to_existing_user(self): "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._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_not_called() + args = self.assertRenderedError("mapping_error") self.assertTrue( - str(e.value).startswith( + args[2].startswith( "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly:" ) ) @@ -738,28 +730,15 @@ def test_map_userinfo_to_existing_user(self): 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._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_called_once_with( + "@TEST_USER_2:test", ANY, ANY, {}, ) - 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öö") + self._make_callback_with_userinfo({"sub": "test2", "username": "föö"}) + self.assertRenderedError("mapping_error", "localpart is invalid: föö") @override_config( { @@ -772,6 +751,9 @@ def test_map_userinfo_to_invalid_localpart(self): ) def test_map_userinfo_to_user_retries(self): """The mapping provider can retry generating an MXID if the MXID is already in use.""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + store = self.hs.get_datastore() self.get_success( store.register_user(user_id="@test_user:test", password_hash=None) @@ -780,14 +762,13 @@ def test_map_userinfo_to_user_retries(self): "sub": "test", "username": "test_user", } - token = {} - mxid = self.get_success( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ) - ) + self._make_callback_with_userinfo(userinfo) + # test_user is already taken, so test_user1 gets registered instead. - self.assertEqual(mxid, "@test_user1:test") + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user1:test", ANY, ANY, {}, + ) + auth_handler.complete_sso_login.reset_mock() # Register all of the potential mxids for a particular OIDC username. self.get_success( @@ -803,15 +784,31 @@ def test_map_userinfo_to_user_retries(self): "sub": "tester", "username": "tester", } - e = self.get_failure( - self.handler._map_userinfo_to_user( - userinfo, token, "user-agent", "10.10.10.10" - ), - MappingException, + self._make_callback_with_userinfo(userinfo) + auth_handler.complete_sso_login.assert_not_called() + self.assertRenderedError( + "mapping_error", "Unable to generate a Matrix ID from the SSO response" ) - self.assertEqual( - str(e.value), "Unable to generate a Matrix ID from the SSO response" + + def _make_callback_with_userinfo( + self, userinfo: dict, client_redirect_url="http://client/redirect" + ) -> None: + self.handler._exchange_code = simple_async_mock(return_value={}) + self.handler._parse_id_token = simple_async_mock(return_value=userinfo) + self.handler._fetch_userinfo = simple_async_mock(return_value=userinfo) + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + + state = "state" + session = self.handler._generate_oidc_session_token( + state=state, + nonce="nonce", + client_redirect_url=client_redirect_url, + ui_auth_session_id=None, ) + request = self._build_callback_request("code", state, session) + + self.get_success(self.handler.handle_oidc_callback(request)) def _build_callback_request( self, From efa531e62d0a5b19f313c54e256a5bc34e2b2a65 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 9 Dec 2020 19:51:59 +0000 Subject: [PATCH 5/6] changelog --- changelog.d/8911.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8911.feature diff --git a/changelog.d/8911.feature b/changelog.d/8911.feature new file mode 100644 index 000000000000..d450ef4998d9 --- /dev/null +++ b/changelog.d/8911.feature @@ -0,0 +1 @@ +Add support for allowing users to pick their own user ID during a single-sign-on login. From 60dbbf049ed8db8d2e324809a308aabe56dd2714 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 14 Dec 2020 10:48:51 +0000 Subject: [PATCH 6/6] Update tests/handlers/test_oidc.py Co-authored-by: Patrick Cloke --- tests/handlers/test_oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index f5502265053c..9878527babd7 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -791,7 +791,7 @@ def test_map_userinfo_to_user_retries(self): ) def _make_callback_with_userinfo( - self, userinfo: dict, client_redirect_url="http://client/redirect" + self, userinfo: dict, client_redirect_url: str = "http://client/redirect" ) -> None: self.handler._exchange_code = simple_async_mock(return_value={}) self.handler._parse_id_token = simple_async_mock(return_value=userinfo)