From 55a271afad926ac01a3dbc7441755180c858d350 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 12 May 2020 14:57:28 -0400 Subject: [PATCH 1/5] Warn, instead of erroring, if the client dict changes UI Auth. --- changelog.d/7483.bugfix | 1 + synapse/handlers/auth.py | 27 +++++++++++++++---------- tests/rest/client/v2_alpha/test_auth.py | 3 +++ 3 files changed, 20 insertions(+), 11 deletions(-) create mode 100644 changelog.d/7483.bugfix diff --git a/changelog.d/7483.bugfix b/changelog.d/7483.bugfix new file mode 100644 index 000000000000..d1693a7f2248 --- /dev/null +++ b/changelog.d/7483.bugfix @@ -0,0 +1 @@ +Ensure that a user inteactive authentication session is tied to a single request. diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 9c717023711e..97fecdb8007b 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -346,25 +346,30 @@ async def check_auth( # Ensure that the queried operation does not vary between stages of # the UI authentication session. This is done by generating a stable - # comparator based on the URI, method, and client dict (minus the - # auth dict) and storing it during the initial query. Subsequent + # comparator and storing it during the initial query. Subsequent # queries ensure that this comparator has not changed. - if validate_clientdict: - session_comparator = (session.uri, session.method, session.clientdict) - comparator = (uri, method, clientdict) - else: - session_comparator = (session.uri, session.method) # type: ignore - comparator = (uri, method) # type: ignore - - if session_comparator != comparator: + # + # The comparator is based on the requested URI and HTTP method. The + # client dict (minus the auth dict) should also be checked, but some + # clients are not spec compliant, just warn for now if the client + # dict changes. + if (session.uri, session.method) != (uri, method): raise SynapseError( 403, "Requested operation has changed during the UI authentication session.", ) + if validate_clientdict: + if session.clientdict != clientdict: + logger.warning( + "Requested operation has changed during the UI " + "authentication session. A future version of Synapse " + "will remove this capability." + ) + # For backwards compatibility the registration endpoint persists # changes to the client dict instead of validating them. - if not validate_clientdict: + else: await self.store.set_ui_auth_clientdict(sid, clientdict) if not authdict: diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index a56c50a5b7ee..a22181aef568 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -316,6 +316,9 @@ def test_cannot_change_body(self): }, ) + # This test is currently skipped since it is allowed for non-spec compliant clients. + test_cannot_change_body.skip = "Skipped to allow for non-compliant clients" # type: ignore + def test_cannot_change_uri(self): """ The initial requested URI cannot be modified during the user interactive authentication session. From 60273fc050f16dd51d5a2c7e337f86e223631bbd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 13 May 2020 07:03:14 -0400 Subject: [PATCH 2/5] Always persist changes to the client dict back into the database. --- changelog.d/7483.bugfix | 2 +- synapse/handlers/auth.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/changelog.d/7483.bugfix b/changelog.d/7483.bugfix index d1693a7f2248..c9b27ad2b2a8 100644 --- a/changelog.d/7483.bugfix +++ b/changelog.d/7483.bugfix @@ -1 +1 @@ -Ensure that a user inteactive authentication session is tied to a single request. +Restore compatibility with non-compliant clients during the user inteactive authentication process. \ No newline at end of file diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 97fecdb8007b..952d150a0d09 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -367,10 +367,10 @@ async def check_auth( "will remove this capability." ) - # For backwards compatibility the registration endpoint persists - # changes to the client dict instead of validating them. - else: - await self.store.set_ui_auth_clientdict(sid, clientdict) + # For backwards compatibility, changes to the client dict are + # persisted as clients modify them throughout their user interactive + # authentication flow. + await self.store.set_ui_auth_clientdict(sid, clientdict) if not authdict: raise InteractiveAuthIncompleteError( From 2be31593df3a5b5f47a710645508ed7537f42810 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 13 May 2020 07:22:05 -0400 Subject: [PATCH 3/5] Remove special casing of registration endpoint. --- synapse/handlers/auth.py | 18 +++----- synapse/rest/client/v2_alpha/register.py | 1 - tests/rest/client/v2_alpha/test_auth.py | 54 +++--------------------- 3 files changed, 12 insertions(+), 61 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 952d150a0d09..5c20e2917114 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -252,7 +252,6 @@ async def check_auth( clientdict: Dict[str, Any], clientip: str, description: str, - validate_clientdict: bool = True, ) -> Tuple[dict, dict, str]: """ Takes a dictionary sent by the client in the login / registration @@ -278,10 +277,6 @@ async def check_auth( description: A human readable string to be displayed to the user that describes the operation happening on their account. - validate_clientdict: Whether to validate that the operation happening - on the account has not changed. If this is false, - the client dict is persisted instead of validated. - Returns: A tuple of (creds, params, session_id). @@ -359,13 +354,12 @@ async def check_auth( "Requested operation has changed during the UI authentication session.", ) - if validate_clientdict: - if session.clientdict != clientdict: - logger.warning( - "Requested operation has changed during the UI " - "authentication session. A future version of Synapse " - "will remove this capability." - ) + if session.clientdict != clientdict: + logger.warning( + "Requested operation has changed during the UI " + "authentication session. A future version of Synapse " + "will remove this capability." + ) # For backwards compatibility, changes to the client dict are # persisted as clients modify them throughout their user interactive diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index e77dd6bf922e..af08cc6cce82 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -516,7 +516,6 @@ async def on_POST(self, request): body, self.hs.get_ip_from_request(request), "register a new account", - validate_clientdict=False, ) # Check that we're not trying to register a denied 3pid. diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index a22181aef568..4fb474170e2e 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -133,47 +133,6 @@ def test_fallback_captcha(self): # We're given a registered user. self.assertEqual(channel.json_body["user_id"], "@user:test") - def test_legacy_registration(self): - """ - Registration allows the parameters to vary through the process. - """ - - # Make the initial request to register. (Later on a different password - # will be used.) - # Returns a 401 as per the spec - channel = self.register( - 401, {"username": "user", "type": "m.login.password", "password": "bar"}, - ) - - # Grab the session - session = channel.json_body["session"] - # Assert our configured public key is being given - self.assertEqual( - channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake" - ) - - # Complete the recaptcha step. - self.recaptcha(session, 200) - - # also complete the dummy auth - self.register(200, {"auth": {"session": session, "type": "m.login.dummy"}}) - - # Now we should have fulfilled a complete auth flow, including - # the recaptcha fallback step. Make the initial request again, but - # with a changed password. This still completes. - channel = self.register( - 200, - { - "username": "user", - "type": "m.login.password", - "password": "foo", # Note that this is different. - "auth": {"session": session}, - }, - ) - - # We're given a registered user. - self.assertEqual(channel.json_body["user_id"], "@user:test") - def test_complete_operation_unknown_session(self): """ Attempting to mark an invalid session as complete should error. @@ -282,9 +241,11 @@ def test_ui_auth(self): }, ) - def test_cannot_change_body(self): + def test_can_change_body(self): """ - The initial requested client dict cannot be modified during the user interactive authentication session. + The client dict can be modified during the user interactive authentication session. + + Note that this is not spec compliant, but is necessary for clients to work. """ # Create a second login. self.login("test", self.user_pass) @@ -302,9 +263,9 @@ def test_cannot_change_body(self): self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) # Make another request providing the UI auth flow, but try to delete the - # second device. This results in an error. + # second device. self.delete_devices( - 403, + 200, { "devices": [device_ids[1]], "auth": { @@ -316,9 +277,6 @@ def test_cannot_change_body(self): }, ) - # This test is currently skipped since it is allowed for non-spec compliant clients. - test_cannot_change_body.skip = "Skipped to allow for non-compliant clients" # type: ignore - def test_cannot_change_uri(self): """ The initial requested URI cannot be modified during the user interactive authentication session. From b66aafadadb1d4524bdf9f3fb4851b309b55dda4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 13 May 2020 12:39:11 -0400 Subject: [PATCH 4/5] Fix a typo in the changelog entry. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/7483.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7483.bugfix b/changelog.d/7483.bugfix index c9b27ad2b2a8..e1bc32461774 100644 --- a/changelog.d/7483.bugfix +++ b/changelog.d/7483.bugfix @@ -1 +1 @@ -Restore compatibility with non-compliant clients during the user inteactive authentication process. \ No newline at end of file +Restore compatibility with non-compliant clients during the user interactive authentication process. From 5fc9804a530b5122a4a287f8910b539763868312 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 13 May 2020 12:41:13 -0400 Subject: [PATCH 5/5] Clarify comment. --- tests/rest/client/v2_alpha/test_auth.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 4fb474170e2e..293ccfba2bb1 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -245,7 +245,11 @@ def test_can_change_body(self): """ The client dict can be modified during the user interactive authentication session. - Note that this is not spec compliant, but is necessary for clients to work. + Note that it is not spec compliant to modify the client dict during a + user interactive authentication session, but many clients currently do. + + When Synapse is updated to be spec compliant, the call to re-use the + session ID should be rejected. """ # Create a second login. self.login("test", self.user_pass)