Do not validate that the client dict is stable during UI Auth. (#7483)

This backs out some of the validation for the client dictionary and logs if
this changes during a user interactive authentication session instead.
This commit is contained in:
Patrick Cloke 2020-05-13 14:26:44 -04:00 committed by GitHub
parent edd3b0747c
commit 5d64fefd6c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 29 additions and 65 deletions

1
changelog.d/7483.bugfix Normal file
View File

@ -0,0 +1 @@
Restore compatibility with non-compliant clients during the user interactive authentication process.

View File

@ -252,7 +252,6 @@ class AuthHandler(BaseHandler):
clientdict: Dict[str, Any], clientdict: Dict[str, Any],
clientip: str, clientip: str,
description: str, description: str,
validate_clientdict: bool = True,
) -> Tuple[dict, dict, str]: ) -> Tuple[dict, dict, str]:
""" """
Takes a dictionary sent by the client in the login / registration 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 description: A human readable string to be displayed to the user that
describes the operation happening on their account. 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: Returns:
A tuple of (creds, params, session_id). 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 # Ensure that the queried operation does not vary between stages of
# the UI authentication session. This is done by generating a stable # the UI authentication session. This is done by generating a stable
# comparator based on the URI, method, and client dict (minus the # comparator and storing it during the initial query. Subsequent
# auth dict) and storing it during the initial query. Subsequent
# queries ensure that this comparator has not changed. # queries ensure that this comparator has not changed.
if validate_clientdict: #
session_comparator = (session.uri, session.method, session.clientdict) # The comparator is based on the requested URI and HTTP method. The
comparator = (uri, method, clientdict) # client dict (minus the auth dict) should also be checked, but some
else: # clients are not spec compliant, just warn for now if the client
session_comparator = (session.uri, session.method) # type: ignore # dict changes.
comparator = (uri, method) # type: ignore if (session.uri, session.method) != (uri, method):
if session_comparator != comparator:
raise SynapseError( raise SynapseError(
403, 403,
"Requested operation has changed during the UI authentication session.", "Requested operation has changed during the UI authentication session.",
) )
# For backwards compatibility the registration endpoint persists if session.clientdict != clientdict:
# changes to the client dict instead of validating them. logger.warning(
if not validate_clientdict: "Requested operation has changed during the UI "
await self.store.set_ui_auth_clientdict(sid, clientdict) "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: if not authdict:
raise InteractiveAuthIncompleteError( raise InteractiveAuthIncompleteError(

View File

@ -516,7 +516,6 @@ class RegisterRestServlet(RestServlet):
body, body,
self.hs.get_ip_from_request(request), self.hs.get_ip_from_request(request),
"register a new account", "register a new account",
validate_clientdict=False,
) )
# Check that we're not trying to register a denied 3pid. # Check that we're not trying to register a denied 3pid.

View File

@ -133,47 +133,6 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
# We're given a registered user. # We're given a registered user.
self.assertEqual(channel.json_body["user_id"], "@user:test") 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): def test_complete_operation_unknown_session(self):
""" """
Attempting to mark an invalid session as complete should error. 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. # Create a second login.
self.login("test", self.user_pass) self.login("test", self.user_pass)
@ -302,9 +267,9 @@ class UIAuthTests(unittest.HomeserverTestCase):
self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
# Make another request providing the UI auth flow, but try to delete the # 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( self.delete_devices(
403, 200,
{ {
"devices": [device_ids[1]], "devices": [device_ids[1]],
"auth": { "auth": {