Add a config flag to inhibit M_USER_IN_USE during registration

[babolivier]

This is mostly motivated by the tchap use case, where usernames are automatically generated from the user's email address (in a way that allows figuring out the email address from the username). Therefore, it's an issue if we respond to requests on /register and /register/available with M_USER_IN_USE, because it can potentially leak email addresses (which include the user's real name and place of work).

This PR adds a flag to inhibit the M_USER_IN_USE errors that are raised both by /register/available, and when providing a username early into the registration process. This error will still be raised if the user completes the registration process but the username conflicts. This is particularly useful when using modules (an upcoming PR will add a module callback to set the username of users at registration) or SSO, since they can ensure the username is unique.

More context is available in the PR that introduced this behaviour to synapse-dinsic: matrix-org/synapse-dinsic#48 - as well as the issue in the matrix-dinsic repo: https://github.com/matrix-org/matrix-dinsic/issues/476

[anoadragon453]

I'm wondering if we can break this up into two separate pieces of functionality:

1. The ability to not respond with M_USER_IN_USE if desired
   • Togglable via a config option.
   • This could be useful outside of the generating usernames usecase.
2. Implement the module API, and if implemented by a module, ignore usernames implicity.
   • No need to additionally toggle a config option after installing a module.

What do you think?

[babolivier]

@anoadragon453 The issue I see with your proposed solution is that I'm not sure what we would do if the username is already registered. The main reason we don't want to return M_USER_IN_USE is so we don't leak existing usernames. We're bound to have to return an error somewhere in the process if we're using the username provided by the user and it clashes with an existing one, which would defeat the point.

[anoadragon453]

@babolivier good point! Do you think a use case exists for returning M_USER_IN_USE while usernames are being generated? If so, perhaps an option for whether M_USER_IN_USE is returned is still useful, but it would be conditioned on whether a module has been registered to generate usernames.

Also, does:

Implement the module API, and if implemented by a module, ignore usernames implicity.

sounds reasonable?

[babolivier]

@anoadragon453

Do you think a use case exists for returning M_USER_IN_USE while usernames are being generated?

I don't. afaiu the semantics of this error code is "the username supplied by the client is already used by an existing user". So if we ignore the username supplied by the client I don't see a reason to return this error.

Also, does:

Implement the module API, and if implemented by a module, ignore usernames implicity.

sounds reasonable?

The way I see it the module will be given the parameters the user provided during the registration process so the module can generate a username to use from it (e.g. for Tchap we would extract the email address and just use that). In this case imo whether to ignore the username provided by the client/user should be decided by the module, and Synapse shouldn't e.g. strip the provided username from the parameters it passes through.

[babolivier]

After discussing this with @clokep, we agreed that a better solution would be to instead make the new config flag inhibit M_USER_IN_USE errors in /available requests and before completing the registration process. Since we're not ignoring the client's username anymore, if the username conflicts we'll still raise an error once the client completes the registration if the username conflicts. I've updated the PR's description to reflect this.

[babolivier]

I'm not adding another check later on (e.g. after we've checked that the UIA flow is complete) because the registration handler's register_user (which is called towards the end of this function) already does that.

[DMRobertson]

Still a little anxious about this config option, but the source code changes look reasonable!

[DMRobertson]

Part of me feels uncomfortable lying here. I wondered about returning 400 or 403 with an appropriate error code. But I can imagine a) that clients would break, and b) that would require a spec change.