Trying to set a password during UI Auth while also failing to authenticate during a stage will result in the password not being saved

[anoadragon453]

Related: https://github.com/matrix-org/matrix-doc/issues/2907, #8009

CC: @clokep

DINUM were finding after upgrading to Synapse v1.23.0 that email registration was no longer working due to "Missing params: password". Their flow consisted of the following requests.

Check with the homeserver what the required flows to register are:

POST /register

{}

# get back a sessionID

Request a validation token from Synapse (no Sydent involved here):

POST /register/email/requestToken

{some client_secret and email address info}

# get back a sid (session ID for validating the email)

Request /register again, this time with a password which we want to save. We're doing to do a m.login.email.identity here even though we know that it's going to fail. Note that m.login.email.identity is the only flow the server gives us. We just want to save the password.

POST /register

{
  "auth": {
    "threepid_creds": { threepid_validation stuff that won't succeed because the link in the email hasn't been pressed yet },
    "session": "what_we_got_from_the_first_request",
    "type": "m.login.email.identity"
  },
  "password": "please_save_this_password123"
}

# Get back a 401. Fair, I expected this. But my password_hash has been recorded, right?

The client got a 401 one, but not from a InteractiveAuthIncompleteError exception, but a LoginError coming from here:

synapse/synapse/handlers/ui_auth/checkers.py

Lines 191 to 192 in c9c0ad5

	if not threepid:
	raise LoginError(401, "", errcode=Codes.UNAUTHORIZED)

Now unfortunately, the code that stores password_hash in ui_auth_sessions for subsequent UI Auth requests will only do so if a InteractiveAuthIncompleteError is raised, not LoginError:

synapse/synapse/rest/client/v2_alpha/register.py

Lines 513 to 536 in 789d9eb

	# Check if the user-interactive authentication flows are complete, if
	# not this will raise a user-interactive auth error.
	try:
	auth_result, params, session_id = await self.auth_handler.check_ui_auth(
	self._registration_flows, request, body, "register a new account",
	)
	except InteractiveAuthIncompleteError as e:
	# The user needs to provide more steps to complete auth.
	#
	# Hash the password and store it with the session since the client
	# is not required to provide the password again.
	#
	# If a password hash was previously stored we will not attempt to
	# re-hash and store it for efficiency. This assumes the password
	# does not change throughout the authentication flow, but this
	# should be fine since the data is meant to be consistent.
	if not password_hash and password:
	password_hash = await self.auth_handler.hash(password)
	await self.auth_handler.set_session_data(
	e.session_id,
	UIAuthSessionDataConstants.PASSWORD_HASH,
	password_hash,
	)
	raise

So uh oh. A password_hash never got saved. In the case of DINUM, a user now clicks the link in their email and is brought to a completely separate client which doesn't know what the password is supposed to be.

That client (having validated the threepid) then makes a final request to /register:

POST /register

{
  "auth": {
    "threepid_creds": { threepid_validation stuff that won't succeed because the link in the email hasn't been pressed yet }
  }
}

# Gets back a 400! Missing params: password

And thus registration fails.

---

I think a simple solution here would be to catch LoginError in addition to InteractiveAuthIncompleteError where we store the pasword_hash:

synapse/synapse/rest/client/v2_alpha/register.py

Lines 513 to 536 in 789d9eb

	# Check if the user-interactive authentication flows are complete, if
	# not this will raise a user-interactive auth error.
	try:
	auth_result, params, session_id = await self.auth_handler.check_ui_auth(
	self._registration_flows, request, body, "register a new account",
	)
	except InteractiveAuthIncompleteError as e:
	# The user needs to provide more steps to complete auth.
	#
	# Hash the password and store it with the session since the client
	# is not required to provide the password again.
	#
	# If a password hash was previously stored we will not attempt to
	# re-hash and store it for efficiency. This assumes the password
	# does not change throughout the authentication flow, but this
	# should be fine since the data is meant to be consistent.
	if not password_hash and password:
	password_hash = await self.auth_handler.hash(password)
	await self.auth_handler.set_session_data(
	e.session_id,
	UIAuthSessionDataConstants.PASSWORD_HASH,
	password_hash,
	)
	raise

That way the client would still get the same exact error, but now we're storing the hash when a LoginError is raised. If we go with this, we should also do it in /account/password, which does the same:

synapse/synapse/rest/client/v2_alpha/account.py

Line 196 in 7a2e9b5

except InteractiveAuthIncompleteError as e:

Additionally, the client could just send the password without attempting to complete an auth step, though this isn't really clear from the perspective of the client developer.

[anoadragon453]

Alright, so just catching LoginError where we want to store the password will not work as we require pulling a session ID from the Exception. And you can't store a session ID on LoginError:

synapse/synapse/rest/client/v2_alpha/register.py

Lines 531 to 535 in 789d9eb

	await self.auth_handler.set_session_data(
	e.session_id,
	UIAuthSessionDataConstants.PASSWORD_HASH,
	password_hash,
	)

Instead, I realised that a LoginError is only raised by AuthHandler.check_ui_auth if you fail a m.long.email.identity check, due to this 4 year old tech debt:

synapse/synapse/handlers/auth.py

Lines 569 to 578 in 02070c6

	except LoginError as e:
	if login_type == LoginType.EMAIL_IDENTITY:
	# riot used to have a bug where it would request a new
	# validation token (thus sending a new email) each time it
	# got a 401 with a 'flows' field.
	# (https://github.com/vector-im/vector-web/issues/2447).
	#
	# Grandfather in the old behaviour for now to avoid
	# breaking old riot deployments.
	raise

If we just remove that conditional, check_ui_auth will never raise LoginError and thus the password will be saved.

...I assume it's not controversial to remove that at this point? The issue was fixed in Riot 0.7.4 (released Oct 12, 2016).

[anoadragon453]

The solution of removing the tech-debt indeed solved the issue for DINUM, and the one I recommend we go for. Commit: matrix-org/synapse-dinsic@d36bfdd

[clokep]

I haven't fully looked at the code again but I think the issue is that they should provide the password in the first step instead of a "get auth session step".

[clokep]

So I think the changes in #9265, but I think the client is still doing something unexpected here (and will essentially end up with a second session?).

[anoadragon453]

@clokep The session ID from the first "dummy" request is re-used in subsequent requests, and thus they'll only ever have one session. But you're right in that if the password were provided in the first request then it would be saved and the registration would succeed.

Patching in the server in addition to the client is preferable for DINUM however, so that every client does not need to be updated immediately..