Improve register performance

[clokep]

This has a few tweaks to the register (and modify password) endpoints that should improve performance a bit:

1. Pre-stores whether registration is enabled instead of checking the config each time.
2. Only hash passwords when necessary:
   1. If UI auth fails (and might be completed in a subsequent call).
   2. Right before registration, if a password was provided.

It also has a couple of other changes:

1. Re-organizes some code in the register endpoint and clarifies comments.
2. Renames AuthHandler.check_auth to AuthHandler.check_ui_auth to avoid clashes with UserInteractiveAuthChecker.

This should be reviewable commit-by-commit.

Fixes #7956

[richvdh]

lgtm I think.

reminder that we should test this stuff carefully from a range of clients after deploying it, as it's fragile and the sort of thing that we can accidentally break and not notice until our usage stats drop off a few days later.

[richvdh]

I struggled for a while to understand what was going on here, and tbh the comment didn't do much to help.

I think this comment - especially the bit about what happens if the password changes mid-flow - should move down to inside the except InteractiveAuthIncompleteError block at line 533ish, and we can just say "extract the previously-hashed password from the session" here.

[clokep]

I struggled with where to put this, moving it further up sounds like it would be an improvement.

Is there a particular piece of knowledge that should be documented in comments?

[richvdh]

I struggled with where to put this, moving it further up sounds like it would be an improvement.

I'm mostly suggesting the code gets left where it is, and the comment moves down.

Is there a particular piece of knowledge that should be documented in comments?

I don't think so, tbh. I think this code speaks for itself, but I only realised that after I stopped trying to understand the comment, which doesn't really describe what is happening here :)

[clokep]

I re-arranged and re-worded the comments in e05118b, I'm hoping that helps?

[richvdh]

yup, lgtm.

[clokep]

reminder that we should test this stuff carefully from a range of clients after deploying it, as it's fragile and the sort of thing that we can accidentally break and not notice until our usage stats drop off a few days later.

I think I've previously written tests for some of the assumptions in various client applications, but definitely agree this needs careful testing!

[clokep]

I did some testing with this and Element iOS and the registration behavior seems to be the same as with develop, which is not ideal frankly, but unrelated to this PR.

I was using the following configuration:

1. Enable email notifications (email block) and require email for registration (registrations_require_3pid)
2. Patch the email so it just prints instead

I then went through the following steps:

1. Register via the iOS client
2. Copy the link
3. Visit the link

This works and the registration completes OK.

The behavior that isn't great is that if the link is visited on my desktop (so both the iOS app and web app are "running" at the same time) then sometimes only one of the apps completes registration successfully and the other just spins. Other times both complete registration and you immediately get a prompt to verify the other session. I think the two apps essentially race since we delete the threepid validation information after it is validated. This doesn't seem to match the rest of the UI Auth information which we purposefully leave around as "valid" for a period of time.

Note that this behavior is not due to this PR.

[clokep]

And...I was testing the wrong branch. 🤦 Some of the behavior on this branch does seem a little different. On develop I seem to always end up at the case where both sessions end up valid (e.g. the race isn't triggered). I'm going to look a bit more into this.

[clokep]

After some investigation it seems that Element iOS is requesting to be sent back to Element Web (via a next link link https://app.element.io/#/register?client_secret=98BC6BD7-CBA4-43AE-83A1-D968C292F5BC-44237-0000FDA97FBAB43A&hs_url=http%3A%2F%2Flocalhost%3A8081&session_id=gTGEIrTURJBbxcATejjmHsJL&is_url=https%3A%2F%2Fvector.im&sid=xDzfSQZUKctJQsRZ on the requestToken API for validating email addresses). This seems wrong, but is not a Synapse issue.