Unify registrations for local passwords and upstream OAuth registrations #5281
Conversation
This will let us push emails in user registrations using an upstream session
This will allow us creating user registrations from upstream OAuth auth sessions
In case an email is required for password auth, we create a user authentication which we force the user to complete. We used to double-check that the email is required before completing the registration, which was only really useful when the config flipped from not being required to being required, in the 1h window in which running registrations were still valid. We think this is a fine trade-off.
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
61ee8da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54597060.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://quenting-upstream-user-regis.matrix-authentication-service-docs.pages.dev |
sandhose
force-pushed
the
quenting/upstream-user-registration
branch
from
6fd1b7d to
1e69ea8
Compare
There was a problem hiding this comment.
Pull request overview
This PR unifies the registration flow for local password-based registrations and upstream OAuth registrations by introducing support for storing upstream OAuth sessions on user registrations. This enables users to complete registrations that were initiated through OAuth providers, with optional email verification.
Key changes:
- Added
upstream_oauth_authorization_session_idfield to user registrations - Split
complete_authenticationinto two methods:complete_authentication_with_codeandcomplete_authentication_with_upstream - Modified OAuth registration flow to create user registrations instead of directly creating users, allowing multi-step registration flows
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/data-model/src/users.rs |
Added upstream_oauth_authorization_session_id field to UserRegistration struct |
crates/storage/src/user/registration.rs |
Added trait method set_upstream_oauth_authorization_session for associating OAuth sessions with registrations |
crates/storage/src/user/email.rs |
Renamed complete_authentication to complete_authentication_with_code and added complete_authentication_with_upstream method |
crates/storage-pg/src/user/registration.rs |
Implemented set_upstream_oauth_authorization_session with database query and added comprehensive tests |
crates/storage-pg/src/user/email.rs |
Implemented both authentication completion methods with appropriate database updates |
crates/storage-pg/migrations/20251121145458_user_registration_upstream_oauth_session.sql |
Added database column and index for tracking upstream OAuth sessions in registrations |
crates/handlers/src/views/register/steps/finish.rs |
Updated to handle upstream OAuth registrations, verify links aren't already associated, and authenticate users with upstream sessions |
crates/handlers/src/upstream_oauth2/link.rs |
Refactored OAuth registration to use the registration flow instead of directly creating users, enabling multi-step registrations |
crates/handlers/src/views/register/mod.rs |
Exported UserRegistrationSessionsCookie for use in OAuth link handler |
crates/handlers/src/views/register/steps/verify_email.rs |
Updated to use renamed complete_authentication_with_code method |
crates/handlers/src/graphql/mutations/user_email.rs |
Updated to use renamed complete_authentication_with_code method |
crates/storage-pg/src/user/tests.rs |
Updated test cases to use renamed complete_authentication_with_code method |
crates/storage-pg/.sqlx/query-*.json |
Updated SQLx query metadata files to reflect new database schema |
Files not reviewed (1)
- crates/storage-pg/.sqlx/query-4c37988dacca5a83c8b64209042d5f1a8ec44ec8ccccad2d7fce9ac855209883.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
| code: &UserEmailAuthenticationCode, | ||
| ) -> Result<UserEmailAuthentication, Self::Error>; | ||
|
|
||
| /// Complete a [`UserEmailAuthentication`] by using the given upstream oauth |
There was a problem hiding this comment.
is this saying that we accept e.g. Apple's claim of a user's email address instead of them actually verifying their email directly with us?
Maybe this doc would be a bit better if it said the circumstances in which you can do this, but maybe something I'll see in later commits.
There was a problem hiding this comment.
We already assumed that. In the past, we've let the admin choose in the configuration whether to trust it or not, but this lead to more confusion than anything
| CREATE INDEX user_registrations_upstream_oauth_session_id_idx | ||
| ON user_registrations (upstream_oauth_authorization_session_id); |
There was a problem hiding this comment.
is user_registrations small enough that we're happy to do this non-CONCURRENTLY?
There was a problem hiding this comment.
It might, but that's a good point! Fixed in 4c3d2ba
| } | ||
|
|
||
| #[sqlx::test(migrator = "crate::MIGRATOR")] | ||
| async fn test_set_upstream_oauth_link(pool: PgPool) { |
There was a problem hiding this comment.
maybe _link is a slight misnomer / not consistent with the other methods
There was a problem hiding this comment.
That's a leftover for when it was the link that was attached to registrations, not the session! Fixed in 61ee8da
This can be reviewed commit by commit.
The intention of this is to unify how we register user, by using the
UserRegistrationabstraction for upstream OAuth registrations. I intended to do that when I introduced that in #3784 but didn't got around to do it.The nice thing with this is that it gives us registration tokens support for upstream OAuth providers
Fixes #4980