diff --git a/changelog.d/7483.bugfix b/changelog.d/7483.bugfix
new file mode 100644
index 0000000000..e1bc324617
--- /dev/null
+++ b/changelog.d/7483.bugfix
@@ -0,0 +1 @@
+Restore compatibility with non-compliant clients during the user interactive authentication process.
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 9c71702371..5c20e29171 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -252,7 +252,6 @@ class AuthHandler(BaseHandler):
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 @@ class AuthHandler(BaseHandler):
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).
@@ -346,26 +341,30 @@ class AuthHandler(BaseHandler):
# 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.",
)
- # For backwards compatibility the registration endpoint persists
- # changes to the client dict instead of validating them.
- if not validate_clientdict:
- await self.store.set_ui_auth_clientdict(sid, 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, 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(
diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py
index e77dd6bf92..af08cc6cce 100644
--- a/synapse/rest/client/v2_alpha/register.py
+++ b/synapse/rest/client/v2_alpha/register.py
@@ -516,7 +516,6 @@ class RegisterRestServlet(RestServlet):
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 a56c50a5b7..293ccfba2b 100644
--- a/tests/rest/client/v2_alpha/test_auth.py
+++ b/tests/rest/client/v2_alpha/test_auth.py
@@ -133,47 +133,6 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
# 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,15 @@ class UIAuthTests(unittest.HomeserverTestCase):
},
)
- 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 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)
@@ -302,9 +267,9 @@ class UIAuthTests(unittest.HomeserverTestCase):
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": {
|
