Add config option to selectively delegate different operations to account threepid delegates

[babolivier]

Add a registration_only configuration flag to account_threepid_delegates to allow only delegating 3PID registration, and letting Synapse handle password resets.

---

This PR adds a delegate_for config option to account_threepid_delegates which allows you to decide which services should be delegated to configured account threepid delegates, and which should remain as operations for the local Synapse to handle.

For context: account_threepid_delegates are intended to allow Synapse to ask a third-party process (usually an identity service) to handle sending emails/text messages for it. More information is available in the config option comment.

The reason this PR is so chunky is due to some refactoring around the complicated configuration handling of the account_threepid_delegates config option. This could probably be taken out into a separate PR, though it's a little fiddly.

I've tried my best to make this easy to review by commit, with explanations in each commit message.

~anoa

[babolivier]

Hmm, I think I've identified a bug with this approach which is that we currently don't seem to store 3PIDs that have been registered with a delegated config, so password resets happening on Synapse might not work. I'll investigate this.

[anoadragon453]

On the wording, we chose "bind" for binding a 3pid to an IS, and "add" for adding a 3pid to a user's account on a HS.

[clokep]

This is waiting for some feedback, @anoadragon453 said on #synapse-dev:matrix.org:

Ok, I am a bit stuck on this actually.

As part of #7611 we need to decide whether to contact an identity server to validate emails, or do it ourselves, based on whether the user is doing a password reset or registration. Unfortunately, both password reset and registration call these methods.

I eventually need to choose whether to use an IS or not here:

synapse/synapse/handlers/ui_auth/checkers.py

Lines 161 to 166 in a3f1156

	if self.hs.config.threepid_behaviour_email == ThreepidBehaviour.REMOTE:
	assert self.hs.config.account_threepid_delegate_email
	threepid = await identity_handler.threepid_from_creds(
	self.hs.config.account_threepid_delegate_email, threepid_creds
	)
	elif self.hs.config.threepid_behaviour_email == ThreepidBehaviour.LOCAL:

All of these methods are meant to be pretty generic, so threading something that's specific to the email checker is a bit tricky. Anyone have any thoughts on a simpler way to do this?

[anoadragon453]

Hrmm, we have access to the session ID. I wonder if we could store metadata about a UI Auth session and then read it later.

[clokep]

@anoadragon453 Maybe the following are useful?

synapse/synapse/handlers/auth.py

Lines 452 to 482 in d0a43d4

	async def set_session_data(self, session_id: str, key: str, value: Any) -> None:
	"""
	Store a key-value pair into the sessions data associated with this
	request. This data is stored server-side and cannot be modified by
	the client.
	Args:
	session_id: The ID of this session as returned from check_auth
	key: The key to store the data under
	value: The data to store
	"""
	try:
	await self.store.set_ui_auth_session_data(session_id, key, value)
	except StoreError:
	raise SynapseError(400, "Unknown session ID: %s" % (session_id,))
	async def get_session_data(
	self, session_id: str, key: str, default: Optional[Any] = None
	) -> Any:
	"""
	Retrieve data stored with set_session_data
	Args:
	session_id: The ID of this session as returned from check_auth
	key: The key to store the data under
	default: Value to return if the key has not been set
	"""
	try:
	return await self.store.get_ui_auth_session_data(session_id, key, default)
	except StoreError:
	raise SynapseError(400, "Unknown session ID: %s" % (session_id,))

[anoadragon453]

@clokep Ooo, generic, I love it, thank you!

[clokep]

@clokep Ooo, generic, I love it, thank you!

It does persist things into a database though so consider:

• Whether it is OK to store unhashed
• Whether it is JSON serializable

[anoadragon453]

@clokep I'm envsioning just "register" and "password_reset", so should be fine 😁

[anoadragon453]

As I will be checking this value during UI Auth, I need to classify the ui auth type while creating the session. As such, it doesn't look like I should use server_dict here (as we would always be inserting it into the database filled already).

I initially thought about using description to check instead, but that's intended to be a human-readable string that could change at any point, even across upgrades, so relying on that is a no-go.

My next thought is to add an operation column to ui_auth_sessions. This would contain a constant which identifies the purpose of the UI auth session, depending on where it is created. We can then check this later on.

I think this works well, though a slight niggle is backwards compatibility for all the existing sessions in that table. I could technically check the session description during a db migration and retroactively infer the operation of each one... however this would also be subject to description never changing.

We could also just wipe all ongoing UI auth sessions for an update.

What do you think?

[clokep]

My next thought is to add an operation column to ui_auth_sessions. This would contain a constant which identifies the purpose of the UI auth session, depending on where it is created. We can then check this later on.

Isn't this basically what the method + URI is used for?

In terms of modifying this table, I think we've generally considered it OK to be backwards incompatible. It is not ideal but the user impact is fairly low in this case.

[anoadragon453]

Isn't this basically what the method + URI is used for?

@clokep hmm, this could work, but may be slightly iffy with URI's that include variables like:

synapse/synapse/rest/client/v2_alpha/devices.py

Line 98 in 7d28243

PATTERNS = client_patterns("/devices/(?P<device_id>[^/]*)$")

[anoadragon453]

Also uh oh:

synapse/synapse/handlers/auth.py

Lines 305 to 306 in 5d64fef

	uri = request.uri.decode("utf-8")
	method = request.uri.decode("utf-8")

[clokep]

Also uh oh:

synapse/synapse/handlers/auth.py

Lines 305 to 306 in 5d64fef

	uri = request.uri.decode("utf-8")
	method = request.uri.decode("utf-8")

Already fixed: #7689

[anoadragon453]

This is now ready for review.

@clokep I ended up attaching a service to threepid_validation_session in the DB instead of adding an operation to UIAuth sessions, as this classification was only relevant to the threepid checker of UIAuth anyways. Additionally, when users submit a threepid validation token, they don't submit the UIAuth session, so we couldn't check the auth session's operation anyways :)

[anoadragon453]

We've decided to go with matrix-org/synapse-dinsic#51 as a solution to DINUM's problem instead. Additionally, this will hopefully allow for the deprecation of account_threepid_delegates.email instead of further complicating it.