Allow admins to require a manual approval process before new accounts can be used (using MSC3866)

[babolivier]

Resolves #13520

Apologies about the size of this PR, but I wasn't sure it made sense to split it much more. Happy to discuss this if people have different opinions. It can be reviewed commit by commit though.

---

To enable this feature, add this to Synapse's configuration file:

experimental_features:
  msc3866:
    enabled: true
    require_approval_for_new_accounts: true

Setting require_approval_for_new_accounts to false while keeping enabled to true allows manipulating a user's approval status without actively blocking unapproved users. require_approval_for_new_accounts is ignored if enabled is false.

If require_approval_for_new_accounts is false, all new users are considered already approved. This is to avoid users suddenly losing access to their accounts if the configuration changes to require approval for new users.

Users created by an admin (either by hitting the admin API directly or by using the register_new_matrix_user script) are automatically considered approved. However, it is possible for an admin to create an unapproved user by creating the account using the Create or modify Account admin API and set approved to false in the request's content (note that this parameter is ignored if enabled is false).

If the server is configured to require approval, a new user will be served a 403 error (with ORG.MATRIX.MSC3866_USER_AWAITING_APPROVAL) once the registration process finishes. If they try to log in before they have been approved, they will also be served a 403 error (also with ORG.MATRIX.MSC3866_USER_AWAITING_APPROVAL).

Admins can see a list of users pending approval by using the List Accounts admin API with approved=false (this parameter is also ignored if enabled is false).

Admins can approve (or unapprove) a user by using the Create or modify Account admin API and set approved to true (for approved) or false (for unapproved) in the request's content (this parameter is also ignored if enabled is false).

---

In terms of future evolution for this feature, it would be nice to eg send emails to admins when there's a new user to approve and to figure out a way to contact users when they've been approved. This is out of the scope of this PR though.

[DMRobertson]

My first instinct is that this sounds a little bit bespoke for Synapse. Isn't this something an identity provider would do if Synapse was using OIDC for its login and registration?

(I'll leave O to review)

[reivilibre]

David has a point about this sounding like something that should be outside of Synapse's remit, either now or once OIDC-native is here, but I'm happy to believe that this has already been thought through somewhere.

As a note, I would have been tempted to leave the background job as a separate PR, since the rest of it can stand well on its own and that would have made it smaller to review.

I'm personally quite biased against background jobs (they're complicated, occasionally bugs pop up later down the line when someone upgrades very late and they can leave you with intermediate states which are harder to reason about); not saying it's wrong but on balance I'm not sure this one is worth it. It seems like we have to deal with the tri-state NULL value anyway because it's an intermediate state that the background job leaves you with, so perhaps rather omit the background job and leave NULL as meaning 'approved' implicitly.

[reivilibre]

Does it make sense to have a background job to rewrite when we could set it as a database default, either because NULL means approved (and new users are inserted with approved = false)
or because there's a default value for the column*?
It feels like it would be less complicated. Note that any new users will already need approved = false upon registration because otherwise there's a race where a user can register before your migration is finished and then slip through as approved. Your background job might even be substantially delayed in starting because an earlier one is blocking it.

*This involves rewriting the rows in table, except in Postgres 11+. Postgres 10 has an EOL in 2 months so we're close to being able to require it!
We probably don't need to worry about SQLite's behaviour even if it is rewriting rows (which it may not be actually), because those will be small databases.

that said ... Even on matrix.org-size servers, rewriting the rows in one go is probably not so bad, because:

1. the alternative (this background job) is doing that, just slower
2. the users table isn't that large
3. we can now update the schema before restarting workers, so there's no actual downtime involved

That said I would still be tempted to just leave NULL meaning no approval required.

[babolivier]

Using NULL as approved is an interesting idea, though I'm a bit on the fence with this because imo it's very easy to read NULL on a boolean column as a false value, and it also means it needs a bit of special casing when handling it (because then we need to explicitly make it true) which can end up a bit fiddly. I'd rather not use a default value since, as you point out, it involves rewriting the table in Postgres 10. That version reaching EOL in a couple of months is good news but I'd rather not block this PR on that.

It's also worth noting that this background update is basically a copy of the one we already have for setting the deactivation flag.

[babolivier]

Actually since writing this PR a similar question came up in #13799, to which the decision was to treat NULL as true, which isn't that much of a faff in the end. So let's go with that.

[babolivier]

This docstring change is because I've realised some tests purposefully use this method with existing users, which made the docstring confusing.

[reivilibre]

I know I've left a flurry of comments, but this is basically good at this point.

Really minor nitty stuff. Feel free to ping when you want your re-review, sorry for missing this :(

[reivilibre]

wonder if we should be using the Pydantic models instead of attrs here so you get the validation? For some reason I'm under the impression that this is already done in some of the config, but feel free to punt it for now if it's too much faff.

[babolivier]

Looks like we're only doing it in http handlers. Also I'm not sure I see the point of validation to this extent for experimental, if-you-use-this-without-knowing-exactly-what-you're-doing-you're-on-your-own options.

[DMRobertson]

We're free to use it wherever we want to. The original suggestion was to use it for config and I even prototyped this: #12651 (comment)

We don't have many examples of using Pydantic yet so I wouldn't force it on anyone. We would probably need some glue code for config too?

But I think it is pretty good at what it does, gives better error messages earlier and gives us an opportunity to propagate more types through Synapse.

[reivilibre]

No strong opinion but presumably this won't always be an experimental option, at which point it will just get renamed into something unexperimental? So I don't really buy that we should do things less properly for experimental options, but it's only a minor thought

[babolivier]

Re-requesting a review since I've implemented a recent change in the MSC (matrix-org/matrix-spec-proposals@3df48d3, with matrix-org/matrix-spec-proposals@51fd1ed adding missing unstable IDs). Should be fairly straightforward.