From f52a2333d4728fe476aa3ed32b96da58940d42e8 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 11 Aug 2026 15:03:24 +0200 Subject: [PATCH 1/3] Extract a helper to start an upstream OAuth 2.0 authorization flow --- .../handlers/src/upstream_oauth2/authorize.rs | 112 +++++++++++++----- 1 file changed, 83 insertions(+), 29 deletions(-) diff --git a/crates/handlers/src/upstream_oauth2/authorize.rs b/crates/handlers/src/upstream_oauth2/authorize.rs index 5807f5430..72d65e896 100644 --- a/crates/handlers/src/upstream_oauth2/authorize.rs +++ b/crates/handlers/src/upstream_oauth2/authorize.rs @@ -26,6 +26,7 @@ use mas_storage::{ use minijinja::context; use thiserror::Error; use ulid::Ulid; +use url::Url; use super::{UpstreamSessionsCookie, cache::LazyProviderInfos, template::environment}; use crate::{ @@ -42,9 +43,8 @@ pub(crate) enum RouteError { Internal(Box), } -impl_from_error_for_route!(mas_oidc_client::error::DiscoveryError); -impl_from_error_for_route!(mas_oidc_client::error::AuthorizationError); impl_from_error_for_route!(mas_storage::RepositoryError); +impl_from_error_for_route!(StartAuthorizationError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { @@ -57,22 +57,44 @@ impl IntoResponse for RouteError { } } +/// Errors which can happen while starting an upstream OAuth 2.0 authorization +/// flow. They are all internal errors, as the caller is responsible for +/// checking that the provider exists and is enabled. +#[derive(Debug, Error)] +pub(crate) enum StartAuthorizationError { + #[error(transparent)] + Discovery(#[from] mas_oidc_client::error::DiscoveryError), + + #[error(transparent)] + Authorization(#[from] mas_oidc_client::error::AuthorizationError), + + #[error(transparent)] + Repository(#[from] mas_storage::RepositoryError), +} + +/// Start an upstream OAuth 2.0 authorization flow with the given provider. +/// +/// This discovers the provider metadata if needed, records an +/// `upstream_oauth_authorization_sessions` row and stashes it in the browser's +/// upstream sessions cookie, along with the action to perform once the user +/// comes back. It returns the authorization URL to redirect the browser to; it +/// is up to the caller to commit the repository. #[tracing::instrument( - name = "handlers.upstream_oauth2.authorize.get", - fields(upstream_oauth_provider.id = %provider_id), + name = "handlers.upstream_oauth2.authorize.start", + fields(upstream_oauth_provider.id = %provider.id), skip_all, )] -pub(crate) async fn get( - mut rng: BoxRng, - clock: BoxClock, - State(metadata_cache): State, - mut repo: BoxRepository, - State(url_builder): State, - State(http_client): State, +pub(crate) async fn start_authorization( + rng: &mut BoxRng, + clock: &BoxClock, + metadata_cache: &MetadataCache, + http_client: &reqwest::Client, + url_builder: &UrlBuilder, + repo: &mut BoxRepository, cookie_jar: CookieJar, - Path(provider_id): Path, - Query(query): Query, -) -> Result { + provider: &UpstreamOAuthProvider, + post_auth_action: Option, +) -> Result<(CookieJar, Url), StartAuthorizationError> { // Load the session info from the cookie jar. We use this to know whether // the browser recently signed out, which we expose to the // `additional_authorization_parameters` templates as `logged_out` so that @@ -83,17 +105,10 @@ pub(crate) async fn get( .logged_out_at() .is_some_and(|t| clock.now().signed_duration_since(t) < Duration::minutes(5)); - let provider = repo - .upstream_oauth_provider() - .lookup(provider_id) - .await? - .filter(UpstreamOAuthProvider::enabled) - .ok_or(RouteError::ProviderNotFound)?; - // First, discover the provider // This is done lazyly according to provider.discovery_mode and the various // endpoint overrides - let mut lazy_metadata = LazyProviderInfos::new(&metadata_cache, &provider, &http_client); + let mut lazy_metadata = LazyProviderInfos::new(metadata_cache, provider, http_client); lazy_metadata.maybe_discover().await?; let redirect_uri = url_builder.upstream_oauth_callback(provider.id); @@ -114,7 +129,7 @@ pub(crate) async fn get( // `forward_login_hint` shortcut working for deployments that haven't // re-run config sync yet. let downstream_grant = - if let Some(PostAuthAction::ContinueAuthorizationGrant { id }) = &query.post_auth_action { + if let Some(PostAuthAction::ContinueAuthorizationGrant { id }) = &post_auth_action { repo.oauth2_authorization_grant().lookup(*id).await? } else { None @@ -152,7 +167,7 @@ pub(crate) async fn get( let (mut url, data) = mas_oidc_client::requests::authorization_code::build_authorization_url( lazy_metadata.authorization_endpoint().await?.clone(), data, - &mut rng, + rng, )?; // Render the templated `additional_authorization_parameters` and @@ -172,9 +187,9 @@ pub(crate) async fn get( let session = repo .upstream_oauth_session() .add( - &mut rng, - &clock, - &provider, + rng, + clock, + provider, data.state.clone(), data.code_challenge_verifier, data.nonce, @@ -182,8 +197,47 @@ pub(crate) async fn get( .await?; let cookie_jar = UpstreamSessionsCookie::load(&cookie_jar) - .add(session.id, provider.id, data.state, query.post_auth_action) - .save(cookie_jar, &clock); + .add(session.id, provider.id, data.state, post_auth_action) + .save(cookie_jar, clock); + + Ok((cookie_jar, url)) +} + +#[tracing::instrument( + name = "handlers.upstream_oauth2.authorize.get", + fields(upstream_oauth_provider.id = %provider_id), + skip_all, +)] +pub(crate) async fn get( + mut rng: BoxRng, + clock: BoxClock, + State(metadata_cache): State, + mut repo: BoxRepository, + State(url_builder): State, + State(http_client): State, + cookie_jar: CookieJar, + Path(provider_id): Path, + Query(query): Query, +) -> Result { + let provider = repo + .upstream_oauth_provider() + .lookup(provider_id) + .await? + .filter(UpstreamOAuthProvider::enabled) + .ok_or(RouteError::ProviderNotFound)?; + + let (cookie_jar, url) = start_authorization( + &mut rng, + &clock, + &metadata_cache, + &http_client, + &url_builder, + &mut repo, + cookie_jar, + &provider, + query.post_auth_action, + ) + .await?; repo.save().await?; From 60dfac6e17c8c436b5ec68f63ace2d8b8ab0d98d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 11 Aug 2026 15:11:11 +0200 Subject: [PATCH 2/3] Carry the username typed on the registration page through the upstream OAuth 2.0 flow --- .../handlers/src/upstream_oauth2/authorize.rs | 19 +- crates/handlers/src/upstream_oauth2/cookie.rs | 65 +++- crates/handlers/src/upstream_oauth2/link.rs | 282 ++++++++++++++++-- crates/handlers/src/upstream_oauth2/mod.rs | 2 + 4 files changed, 328 insertions(+), 40 deletions(-) diff --git a/crates/handlers/src/upstream_oauth2/authorize.rs b/crates/handlers/src/upstream_oauth2/authorize.rs index 72d65e896..5c925cfbf 100644 --- a/crates/handlers/src/upstream_oauth2/authorize.rs +++ b/crates/handlers/src/upstream_oauth2/authorize.rs @@ -28,7 +28,9 @@ use thiserror::Error; use ulid::Ulid; use url::Url; -use super::{UpstreamSessionsCookie, cache::LazyProviderInfos, template::environment}; +use super::{ + UpstreamSessionContext, UpstreamSessionsCookie, cache::LazyProviderInfos, template::environment, +}; use crate::{ impl_from_error_for_route, upstream_oauth2::cache::MetadataCache, views::shared::OptionalPostAuthAction, @@ -77,8 +79,9 @@ pub(crate) enum StartAuthorizationError { /// This discovers the provider metadata if needed, records an /// `upstream_oauth_authorization_sessions` row and stashes it in the browser's /// upstream sessions cookie, along with the action to perform once the user -/// comes back. It returns the authorization URL to redirect the browser to; it -/// is up to the caller to commit the repository. +/// comes back and the context carried from the page which started the flow. It +/// returns the authorization URL to redirect the browser to; it is up to the +/// caller to commit the repository. #[tracing::instrument( name = "handlers.upstream_oauth2.authorize.start", fields(upstream_oauth_provider.id = %provider.id), @@ -94,6 +97,7 @@ pub(crate) async fn start_authorization( cookie_jar: CookieJar, provider: &UpstreamOAuthProvider, post_auth_action: Option, + context: Option, ) -> Result<(CookieJar, Url), StartAuthorizationError> { // Load the session info from the cookie jar. We use this to know whether // the browser recently signed out, which we expose to the @@ -197,7 +201,13 @@ pub(crate) async fn start_authorization( .await?; let cookie_jar = UpstreamSessionsCookie::load(&cookie_jar) - .add(session.id, provider.id, data.state, post_auth_action) + .add( + session.id, + provider.id, + data.state, + post_auth_action, + context, + ) .save(cookie_jar, clock); Ok((cookie_jar, url)) @@ -236,6 +246,7 @@ pub(crate) async fn get( cookie_jar, &provider, query.post_auth_action, + None, ) .await?; diff --git a/crates/handlers/src/upstream_oauth2/cookie.rs b/crates/handlers/src/upstream_oauth2/cookie.rs index cc5ae2943..90b08eb19 100644 --- a/crates/handlers/src/upstream_oauth2/cookie.rs +++ b/crates/handlers/src/upstream_oauth2/cookie.rs @@ -1,3 +1,4 @@ +// Copyright 2025, 2026 Element Creations Ltd. // Copyright 2024, 2025 New Vector Ltd. // Copyright 2022-2024 The Matrix.org Foundation C.I.C. // @@ -20,6 +21,16 @@ static COOKIE_NAME: &str = "upstream-oauth2-sessions"; /// Sessions expire after 10 minutes static SESSION_MAX_TIME: Duration = Duration::microseconds(10 * 60 * 1000 * 1000); +/// Context gathered by the page which started the upstream OAuth 2.0 flow, +/// carried through it so that it can be used on the way back. +#[derive(Serialize, Deserialize, Default, Debug, Clone, PartialEq, Eq)] +pub struct UpstreamSessionContext { + /// The username the user typed on the registration page, used as a + /// candidate localpart if they end up registering an account + #[serde(default, skip_serializing_if = "Option::is_none")] + pub username: Option, +} + #[derive(Serialize, Deserialize, Debug)] pub struct Payload { session: Ulid, @@ -27,6 +38,11 @@ pub struct Payload { state: String, link: Option, post_auth_action: Option, + + /// Cookies saved by older versions of MAS don't have this field, hence the + /// `default` + #[serde(default, skip_serializing_if = "Option::is_none")] + context: Option, } impl Payload { @@ -87,6 +103,7 @@ impl UpstreamSessions { provider: Ulid, state: String, post_auth_action: Option, + context: Option, ) -> Self { self.0.push(Payload { session, @@ -94,6 +111,7 @@ impl UpstreamSessions { state, link: None, post_auth_action, + context, }); self } @@ -131,13 +149,20 @@ impl UpstreamSessions { pub fn lookup_link( &self, link_id: Ulid, - ) -> Result<(Ulid, Option<&PostAuthAction>), UpstreamSessionNotFound> { + ) -> Result< + ( + Ulid, + Option<&PostAuthAction>, + Option<&UpstreamSessionContext>, + ), + UpstreamSessionNotFound, + > { self.0 .iter() .filter(|p| p.link == Some(link_id)) // Find the session with the highest ID, aka. the most recent one .reduce(|a, b| if a.session > b.session { a } else { b }) - .map(|p| (p.session, p.post_auth_action.as_ref())) + .map(|p| (p.session, p.post_auth_action.as_ref(), p.context.as_ref())) .ok_or(UpstreamSessionNotFound) } @@ -178,13 +203,21 @@ mod tests { let first_session = Ulid::from_datetime_with_rng(now, &mut rng); let first_state = "first-state"; - let sessions = sessions.add(first_session, provider_a, first_state.into(), None); + let sessions = sessions.add(first_session, provider_a, first_state.into(), None, None); let now = now + Duration::microseconds(5 * 60 * 1000 * 1000); let second_session = Ulid::from_datetime_with_rng(now, &mut rng); let second_state = "second-state"; - let sessions = sessions.add(second_session, provider_b, second_state.into(), None); + let sessions = sessions.add( + second_session, + provider_b, + second_state.into(), + None, + Some(UpstreamSessionContext { + username: Some("john".to_owned()), + }), + ); let sessions = sessions.expire(now); assert_eq!( @@ -216,11 +249,31 @@ mod tests { // Now the session can't be found with its state assert!(sessions.find_session(provider_b, second_state).is_err()); - // But it can be looked up by its link - assert_eq!(sessions.lookup_link(second_link).unwrap().0, second_session); + // But it can be looked up by its link, along with the context it carries + let (session, _post_auth_action, context) = sessions.lookup_link(second_link).unwrap(); + assert_eq!(session, second_session); + assert_eq!(context.and_then(|c| c.username.as_deref()), Some("john")); + // And it can be consumed let sessions = sessions.consume_link(second_link).unwrap(); // But only once assert!(sessions.consume_link(second_link).is_err()); } + + /// Cookies saved before we started carrying a context should still parse + #[test] + fn test_payload_without_context() { + let sessions: UpstreamSessions = serde_json::from_value(serde_json::json!([{ + "session": "01FSHN9AG0AJ6AC5HQ9X6H4RP4", + "provider": "01FSHN9AG0MZAA6S4AF7CTV32E", + "state": "state", + "link": "01FSHN9AG09NMZYX8MFVH74RP4", + "post_auth_action": null, + }])) + .expect("payload without a context should deserialize"); + + let link = "01FSHN9AG09NMZYX8MFVH74RP4".parse().unwrap(); + let (_session, _post_auth_action, context) = sessions.lookup_link(link).unwrap(); + assert_eq!(context, None); + } } diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index 51de8ac5a..a877bcb65 100644 --- a/crates/handlers/src/upstream_oauth2/link.rs +++ b/crates/handlers/src/upstream_oauth2/link.rs @@ -245,10 +245,14 @@ pub(crate) async fn get( ) -> Result { let user_agent = user_agent.map(|ua| ua.as_str().to_owned()); let sessions_cookie = UpstreamSessionsCookie::load(&cookie_jar); - let (session_id, post_auth_action) = sessions_cookie + let (session_id, post_auth_action, context) = sessions_cookie .lookup_link(link_id) .map_err(|_| RouteError::MissingCookie)?; + // The username the user typed on the registration page before starting the + // flow, if any. Cloned out of the cookie so we can consume the cookie later. + let carried_username = context.and_then(|context| context.username.clone()); + let link = repo .upstream_oauth_link() .lookup(link_id) @@ -454,34 +458,50 @@ pub(crate) async fn get( )? }; + let forced_or_required = provider.claims_imports.localpart.is_forced_or_required(); + + // A username carried from the registration page is MAS-side user intent, + // so it takes precedence over the claim rendered from the upstream + // response, and applies even when the claim is ignored. Providers which + // force or require the claim ignore it entirely. + let carried_username = if forced_or_required { + None + } else { + carried_username + }; + // We do a bunch of checks for the localpart. Instead of using nested ifs all // the way, we use a labelled block, and use `break` for 'exiting' early when // needed let localpart = 'localpart: { - if provider.claims_imports.localpart.ignore() { - break 'localpart None; - } + let localpart = if let Some(username) = carried_username { + username + } else { + if provider.claims_imports.localpart.ignore() { + break 'localpart None; + } - let template = provider - .claims_imports - .localpart - .template - .as_deref() - .unwrap_or(DEFAULT_LOCALPART_TEMPLATE); + let template = provider + .claims_imports + .localpart + .template + .as_deref() + .unwrap_or(DEFAULT_LOCALPART_TEMPLATE); - let Some(localpart) = render_attribute_template( - &env, - template, - &context, - provider.claims_imports.localpart.is_required(), - )? - else { - break 'localpart None; + let Some(localpart) = render_attribute_template( + &env, + template, + &context, + provider.claims_imports.localpart.is_required(), + )? + else { + break 'localpart None; + }; + + localpart }; - let forced_or_required = provider.claims_imports.localpart.is_forced_or_required(); - - // We got a localpart from the template. We need to check if it's + // We got a localpart candidate. We need to check if it's // available, and if it's not apply the conflict resolution setup in // the config let maybe_existing_user = repo.user().find_by_username(&localpart).await?; @@ -869,7 +889,9 @@ pub(crate) async fn post( let form = cookie_jar.verify_form(&clock, form)?; let sessions_cookie = UpstreamSessionsCookie::load(&cookie_jar); - let (session_id, post_auth_action) = sessions_cookie + // The context carried through the flow doesn't matter here: the username the + // user picked, if they were allowed to pick one, comes from the form + let (session_id, post_auth_action, _context) = sessions_cookie .lookup_link(link_id) .map_err(|_| RouteError::MissingCookie)?; @@ -1274,7 +1296,10 @@ mod tests { use ulid::Ulid; use super::UpstreamSessionsCookie; - use crate::test_utils::{CookieHelper, RequestBuilderExt, ResponseExt, TestState, setup}; + use crate::{ + test_utils::{CookieHelper, RequestBuilderExt, ResponseExt, TestState, setup}, + upstream_oauth2::UpstreamSessionContext, + }; #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] async fn test_register(pool: PgPool) { @@ -1399,7 +1424,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -1597,7 +1622,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -1731,7 +1756,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -1855,7 +1880,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -2045,7 +2070,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -2176,7 +2201,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -2314,7 +2339,7 @@ mod tests { let cookie_jar = state.cookie_jar(); let upstream_sessions = UpstreamSessionsCookie::default() - .add(session.id, provider.id, "state".to_owned(), None) + .add(session.id, provider.id, "state".to_owned(), None, None) .add_link_to_session(session.id, link.id) .unwrap(); let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock); @@ -2357,4 +2382,201 @@ mod tests { assert!(old_link_result.is_some(), "Old link should still exist"); assert_eq!(old_link_result.unwrap().user_id, Some(user.id)); } + + /// Provision a provider with the given claims imports, along with a + /// completed upstream authorization session and its link, and load the + /// upstream sessions cookie carrying the given username in the given cookie + /// jar + async fn carried_username_setup( + state: &TestState, + cookies: &CookieHelper, + localpart: mas_data_model::UpstreamOAuthProviderImportAction, + carried_username: &str, + ) -> UpstreamOAuthLink { + let mut rng = state.rng(); + + let claims_imports = UpstreamOAuthProviderClaimsImports { + localpart: UpstreamOAuthProviderLocalpartPreference { + action: localpart, + template: None, + on_conflict: mas_data_model::UpstreamOAuthProviderOnConflict::default(), + }, + ..UpstreamOAuthProviderClaimsImports::default() + }; + + let id_token_claims = serde_json::json!({ + "preferred_username": "john", + }); + let id_token = sign_token(&mut rng, &state.key_store, id_token_claims.clone()).unwrap(); + + let mut repo = state.repository().await.unwrap(); + let provider = repo + .upstream_oauth_provider() + .add( + &mut rng, + &state.clock, + UpstreamOAuthProviderParams { + issuer: Some("https://example.com/".to_owned()), + human_name: Some("Example Ltd.".to_owned()), + brand_name: None, + scope: Scope::from_iter([OPENID]), + token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None, + token_endpoint_signing_alg: None, + id_token_signed_response_alg: JsonWebSignatureAlg::Rs256, + client_id: "client".to_owned(), + encrypted_client_secret: None, + claims_imports, + authorization_endpoint_override: None, + token_endpoint_override: None, + userinfo_endpoint_override: None, + fetch_userinfo: false, + userinfo_signed_response_alg: None, + jwks_uri_override: None, + discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, + pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, + response_mode: None, + additional_authorization_parameters: Vec::new(), + forward_login_hint: false, + ui_order: 0, + on_backchannel_logout: + mas_data_model::UpstreamOAuthProviderOnBackchannelLogout::DoNothing, + registration_token_required: false, + }, + ) + .await + .unwrap(); + + let (link, session) = add_linked_upstream_session( + &mut rng, + &state.clock, + &mut repo, + &provider, + "subject", + &id_token.into_string(), + id_token_claims, + ) + .await + .unwrap(); + + repo.save().await.unwrap(); + + let upstream_sessions = UpstreamSessionsCookie::default() + .add( + session.id, + provider.id, + "state".to_owned(), + None, + Some(UpstreamSessionContext { + username: Some(carried_username.to_owned()), + }), + ) + .add_link_to_session(session.id, link.id) + .unwrap(); + cookies.import(upstream_sessions.save(state.cookie_jar(), &state.clock)); + + link + } + + /// A username carried through the flow prefills the registration form, and + /// takes precedence over the username suggested by the provider + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_register_with_carried_username(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let link = carried_username_setup( + &state, + &cookies, + mas_data_model::UpstreamOAuthProviderImportAction::Suggest, + "alice", + ) + .await; + + let request = Request::get(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).empty(); + let response = state.request(cookies.with_cookies(request)).await; + response.assert_status(StatusCode::OK); + assert!(response.body().contains(r#"value="alice""#)); + assert!(!response.body().contains(r#"value="john""#)); + } + + /// A carried username prefills the form even when the provider is + /// configured to ignore the localpart claim: it isn't a claim, it is what + /// the user typed + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_register_with_carried_username_and_ignored_claim(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let link = carried_username_setup( + &state, + &cookies, + mas_data_model::UpstreamOAuthProviderImportAction::Ignore, + "alice", + ) + .await; + + let request = Request::get(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).empty(); + let response = state.request(cookies.with_cookies(request)).await; + response.assert_status(StatusCode::OK); + assert!(response.body().contains(r#"value="alice""#)); + } + + /// Providers which force the localpart ignore the carried username + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_carried_username_ignored_when_localpart_forced(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let link = carried_username_setup( + &state, + &cookies, + mas_data_model::UpstreamOAuthProviderImportAction::Force, + "alice", + ) + .await; + + let request = Request::get(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).empty(); + let response = state.request(cookies.with_cookies(request)).await; + response.assert_status(StatusCode::OK); + // The localpart is enforced by the provider, and displayed as a full MXID + assert!(response.body().contains(r#"value="@john:"#)); + assert!(!response.body().contains("alice")); + } + + /// A carried username which is already taken is dropped, and the user gets + /// to pick another one + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_carried_username_dropped_when_taken(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let link = carried_username_setup( + &state, + &cookies, + mas_data_model::UpstreamOAuthProviderImportAction::Suggest, + "alice", + ) + .await; + + let mut rng = state.rng(); + let mut repo = state.repository().await.unwrap(); + repo.user() + .add(&mut rng, &state.clock, "alice".to_owned()) + .await + .unwrap(); + repo.save().await.unwrap(); + + let request = Request::get(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).empty(); + let response = state.request(cookies.with_cookies(request)).await; + response.assert_status(StatusCode::OK); + // The username field is left empty: we don't fall back to the username + // suggested by the provider + assert!(response.body().contains(r#"value="""#)); + assert!(!response.body().contains(r#"value="alice""#)); + assert!(!response.body().contains(r#"value="john""#)); + } } diff --git a/crates/handlers/src/upstream_oauth2/mod.rs b/crates/handlers/src/upstream_oauth2/mod.rs index dab366094..f8adcb938 100644 --- a/crates/handlers/src/upstream_oauth2/mod.rs +++ b/crates/handlers/src/upstream_oauth2/mod.rs @@ -1,3 +1,4 @@ +// Copyright 2025, 2026 Element Creations Ltd. // Copyright 2024, 2025 New Vector Ltd. // Copyright 2022-2024 The Matrix.org Foundation C.I.C. // @@ -23,6 +24,7 @@ mod cookie; pub(crate) mod link; mod template; +pub(crate) use self::cookie::UpstreamSessionContext; use self::cookie::UpstreamSessions as UpstreamSessionsCookie; #[derive(Debug, Error)] From 9a005459c4a9e19878c763b4aad39bb252b3d749 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 11 Aug 2026 15:22:15 +0200 Subject: [PATCH 3/3] Start upstream OAuth 2.0 flows with a form POST from the registration page --- crates/handlers/src/lib.rs | 2 +- crates/handlers/src/views/register/mod.rs | 357 +++++++++++++++++++++- crates/router/src/endpoints.rs | 7 + templates/pages/register/index.html | 10 +- translations/en.json | 14 +- 5 files changed, 373 insertions(+), 17 deletions(-) diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index 66ad96530..1190c0edc 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -393,7 +393,7 @@ where .route(mas_router::Logout::route(), post(self::views::logout::post)) .route( mas_router::Register::route(), - get(self::views::register::get), + get(self::views::register::get).post(self::views::register::post), ) .route( mas_router::PasswordRegister::route(), diff --git a/crates/handlers/src/views/register/mod.rs b/crates/handlers/src/views/register/mod.rs index 6a51852ae..f45abfabb 100644 --- a/crates/handlers/src/views/register/mod.rs +++ b/crates/handlers/src/views/register/mod.rs @@ -1,21 +1,34 @@ +// Copyright 2025, 2026 Element Creations Ltd. // Copyright 2024, 2025 New Vector Ltd. // // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. use axum::{ + Form, extract::State, - response::{Html, IntoResponse, Response}, + response::{Html, IntoResponse, Redirect, Response}, }; use axum_extra::extract::Query; -use mas_axum_utils::{InternalError, SessionInfoExt, cookies::CookieJar, csrf::CsrfExt as _}; -use mas_data_model::{BoxClock, BoxRng, SiteConfig}; +use hyper::StatusCode; +use mas_axum_utils::{ + GenericError, InternalError, SessionInfoExt, + cookies::CookieJar, + csrf::{CsrfExt as _, ProtectedForm}, +}; +use mas_data_model::{BoxClock, BoxRng, SiteConfig, UpstreamOAuthProvider}; use mas_router::{PasswordRegister, UpstreamOAuth2Authorize, UrlBuilder}; -use mas_storage::BoxRepository; +use mas_storage::{BoxRepository, upstream_oauth2::UpstreamOAuthProviderRepository}; use mas_templates::{RegisterContext, TemplateContext, Templates}; +use serde::{Deserialize, Serialize}; +use thiserror::Error; +use ulid::Ulid; use super::shared::OptionalPostAuthAction; -use crate::{BoundActivityTracker, PreferredLanguage}; +use crate::{ + BoundActivityTracker, MetadataCache, PreferredLanguage, impl_from_error_for_route, + upstream_oauth2::{UpstreamSessionContext, authorize::start_authorization}, +}; mod cookie; pub(crate) mod password; @@ -23,6 +36,30 @@ pub(crate) mod steps; pub use self::cookie::UserRegistrationSessions as UserRegistrationSessionsCookie; +#[derive(Debug, Error)] +pub(crate) enum RouteError { + #[error("Provider not found")] + ProviderNotFound, + + #[error(transparent)] + Internal(Box), +} + +impl_from_error_for_route!(mas_axum_utils::csrf::CsrfError); +impl_from_error_for_route!(mas_storage::RepositoryError); +impl_from_error_for_route!(crate::upstream_oauth2::authorize::StartAuthorizationError); + +impl IntoResponse for RouteError { + fn into_response(self) -> Response { + match self { + e @ Self::ProviderNotFound => { + GenericError::new(StatusCode::NOT_FOUND, e).into_response() + } + Self::Internal(e) => InternalError::new(e).into_response(), + } + } +} + #[tracing::instrument(name = "handlers.views.register.get", skip_all)] pub(crate) async fn get( mut rng: BoxRng, @@ -93,3 +130,313 @@ pub(crate) async fn get( Ok((cookie_jar, Html(content)).into_response()) } + +#[derive(Debug, Deserialize, Serialize)] +pub(crate) struct RegisterForm { + #[serde(default)] + username: String, + + /// Which upstream provider the user chose, if any: each provider has its + /// own submit button + #[serde(default)] + provider: Option, + + #[serde(flatten)] + action: OptionalPostAuthAction, +} + +#[tracing::instrument(name = "handlers.views.register.post", skip_all)] +pub(crate) async fn post( + mut rng: BoxRng, + clock: BoxClock, + State(metadata_cache): State, + State(url_builder): State, + State(http_client): State, + mut repo: BoxRepository, + cookie_jar: CookieJar, + Form(form): Form>, +) -> Result { + let form = cookie_jar.verify_form(&clock, form)?; + let username = form.username.trim(); + let post_auth_action = form.action.post_auth_action; + + // The user chose an upstream provider: start an authorization flow with it, + // carrying the username they typed along so that we can prefill it if they + // get to choose one when they come back + if let Some(provider_id) = form.provider { + let provider_id: Ulid = provider_id + .parse() + .map_err(|_| RouteError::ProviderNotFound)?; + + let provider = repo + .upstream_oauth_provider() + .lookup(provider_id) + .await? + .filter(UpstreamOAuthProvider::enabled) + .ok_or(RouteError::ProviderNotFound)?; + + let context = (!username.is_empty()).then(|| UpstreamSessionContext { + username: Some(username.to_owned()), + }); + + let (cookie_jar, url) = start_authorization( + &mut rng, + &clock, + &metadata_cache, + &http_client, + &url_builder, + &mut repo, + cookie_jar, + &provider, + post_auth_action, + context, + ) + .await?; + + repo.save().await?; + + return Ok((cookie_jar, Redirect::to(url.as_str())).into_response()); + } + + // Else the user wants to register with a password: redirect to that page, + // carrying the username in the query string so that the page can be + // reloaded or bookmarked + let mut destination = PasswordRegister::from(post_auth_action); + if !username.is_empty() { + destination = destination.with_username(username.to_owned()); + } + + Ok((cookie_jar, url_builder.redirect(&destination)).into_response()) +} + +#[cfg(test)] +mod tests { + use hyper::{Request, StatusCode, header::LOCATION}; + use mas_data_model::{ + Clock, UlidExt, UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderDiscoveryMode, + UpstreamOAuthProviderOnBackchannelLogout, UpstreamOAuthProviderPkceMode, + UpstreamOAuthProviderTokenAuthMethod, + }; + use mas_iana::jose::JsonWebSignatureAlg; + use mas_storage::{ + RepositoryAccess, + upstream_oauth2::{UpstreamOAuthProviderParams, UpstreamOAuthSessionRepository}, + }; + use oauth2_types::scope::{OPENID, Scope}; + use sqlx::PgPool; + use ulid::Ulid; + + use crate::test_utils::{CookieHelper, RequestBuilderExt, ResponseExt, TestState, setup}; + + /// Provision an upstream provider which needs no network access to start an + /// authorization flow: discovery is disabled and the authorization endpoint + /// is set explicitly + async fn provider(state: &TestState) -> Ulid { + let mut rng = state.rng(); + let mut repo = state.repository().await.unwrap(); + let provider = repo + .upstream_oauth_provider() + .add( + &mut rng, + &state.clock, + UpstreamOAuthProviderParams { + issuer: Some("https://upstream.example.com/".to_owned()), + human_name: Some("Upstream Ltd.".to_owned()), + brand_name: None, + scope: Scope::from_iter([OPENID]), + token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None, + token_endpoint_signing_alg: None, + id_token_signed_response_alg: JsonWebSignatureAlg::Rs256, + client_id: "client".to_owned(), + encrypted_client_secret: None, + claims_imports: UpstreamOAuthProviderClaimsImports::default(), + authorization_endpoint_override: Some( + "https://upstream.example.com/authorize".parse().unwrap(), + ), + token_endpoint_override: None, + userinfo_endpoint_override: None, + fetch_userinfo: false, + userinfo_signed_response_alg: None, + jwks_uri_override: None, + discovery_mode: UpstreamOAuthProviderDiscoveryMode::Disabled, + pkce_mode: UpstreamOAuthProviderPkceMode::Disabled, + response_mode: None, + additional_authorization_parameters: Vec::new(), + forward_login_hint: false, + ui_order: 0, + on_backchannel_logout: UpstreamOAuthProviderOnBackchannelLogout::DoNothing, + registration_token_required: false, + }, + ) + .await + .unwrap(); + repo.save().await.unwrap(); + provider.id + } + + /// Render the registration page, saving its cookies and returning its CSRF + /// token and body + async fn render_page(state: &TestState, cookies: &CookieHelper) -> (String, String) { + let request = cookies.with_cookies(Request::get("/register").empty()); + let response = state.request(request).await; + cookies.save_cookies(&response); + response.assert_status(StatusCode::OK); + + let csrf_token = response + .body() + .split("name=\"csrf\" value=\"") + .nth(1) + .expect("the page should have a CSRF token") + .split('\"') + .next() + .unwrap() + .to_owned(); + + (csrf_token, response.body().clone()) + } + + /// Decode the upstream sessions cookie set by the given response, if any + fn upstream_sessions( + state: &TestState, + response: &hyper::Response, + ) -> Option { + let cookies = CookieHelper::new(); + cookies.save_cookies(response); + let request = cookies.with_cookies(Request::get("/").empty()); + state + .cookie_manager + .cookie_jar_from_headers(request.headers()) + .load("upstream-oauth2-sessions") + .expect("the upstream sessions cookie should decode") + } + + /// Submitting the form with a provider starts an upstream authorization + /// flow, carrying the username along in the cookie + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_post_with_provider(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let provider_id = provider(&state).await; + let (csrf_token, body) = render_page(&state, &cookies).await; + + // The provider is rendered as a submit button of the form + assert!(body.contains(&format!(r#"name="provider" value="{provider_id}""#))); + + let request = cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": csrf_token, + "username": "alice", + "provider": provider_id.to_string(), + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::SEE_OTHER); + + let location = response.headers().get(LOCATION).unwrap().to_str().unwrap(); + assert!( + location.starts_with("https://upstream.example.com/authorize?"), + "unexpected location: {location}" + ); + + // The upstream sessions cookie carries both the session and the username + let sessions = upstream_sessions(&state, &response) + .expect("the upstream sessions cookie should be set"); + assert_eq!(sessions[0]["context"]["username"], "alice"); + + // And we recorded the session it points to + let session_id: Ulid = sessions[0]["session"].as_str().unwrap().parse().unwrap(); + let mut repo = state.repository().await.unwrap(); + let session = repo + .upstream_oauth_session() + .lookup(session_id) + .await + .unwrap() + .expect("the upstream authorization session should exist"); + assert_eq!(session.provider_id, provider_id); + } + + /// Submitting the form without a provider redirects to the password + /// registration page, keeping the username and the post-auth action + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_post_without_provider(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + provider(&state).await; + let (csrf_token, _body) = render_page(&state, &cookies).await; + + let request = cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": csrf_token, + "username": "alice", + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::SEE_OTHER); + response.assert_header_value(LOCATION, "/register/password?username=alice"); + + // The post-auth action travels in the form, as hidden inputs + let request = cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": csrf_token, + "username": "alice", + "kind": "change_password", + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::SEE_OTHER); + response.assert_header_value( + LOCATION, + "/register/password?username=alice&kind=change_password", + ); + + // An empty username is not carried over + let request = cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": csrf_token, + "username": " ", + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::SEE_OTHER); + response.assert_header_value(LOCATION, "/register/password"); + } + + /// A form submitted with an invalid CSRF token is rejected + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_post_invalid_csrf(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + let provider_id = provider(&state).await; + let (csrf_token, _body) = render_page(&state, &cookies).await; + + let request = cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": format!("{csrf_token}invalid"), + "username": "alice", + "provider": provider_id.to_string(), + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::INTERNAL_SERVER_ERROR); + assert!(upstream_sessions(&state, &response).is_none()); + } + + /// Submitting a provider which doesn't exist gives a 404 + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_post_with_unknown_provider(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + let cookies = CookieHelper::new(); + + provider(&state).await; + let (csrf_token, _body) = render_page(&state, &cookies).await; + + let unknown = Ulid::from_datetime_with_rng(state.clock.now(), &mut state.rng()); + for provider in [unknown.to_string(), "not-a-ulid".to_owned()] { + let request = + cookies.with_cookies(Request::post("/register").form(serde_json::json!({ + "csrf": csrf_token, + "username": "alice", + "provider": provider, + }))); + let response = state.request(request).await; + response.assert_status(StatusCode::NOT_FOUND); + } + } +} diff --git a/crates/router/src/endpoints.rs b/crates/router/src/endpoints.rs index 3e60de10c..9bddff410 100644 --- a/crates/router/src/endpoints.rs +++ b/crates/router/src/endpoints.rs @@ -347,6 +347,13 @@ impl PasswordRegister { self } + /// Prefill the form with the given username + #[must_use] + pub fn with_username(mut self, username: String) -> Self { + self.username = Some(username); + self + } + #[must_use] pub fn and_continue_grant(mut self, data: Ulid) -> Self { self.post_auth_action = Some(PostAuthAction::continue_grant(data)); diff --git a/templates/pages/register/index.html b/templates/pages/register/index.html index dc9fd61ec..e19092b01 100644 --- a/templates/pages/register/index.html +++ b/templates/pages/register/index.html @@ -1,4 +1,5 @@ {# +Copyright 2025, 2026 Element Creations Ltd. Copyright 2024, 2025 New Vector Ltd. Copyright 2021-2024 The Matrix.org Foundation C.I.C. @@ -11,7 +12,7 @@ Please see LICENSE files in the repository root for full details. {% from "components/idp_brand.html" import logo %} {% block content %} -
+
{{ icon.user_profile_solid() }} @@ -36,6 +37,8 @@ Please see LICENSE files in the repository root for full details. {% endif %}
+ + {% for key, value in next["params"] | default({}) | items %} {% endfor %} @@ -49,13 +52,12 @@ Please see LICENSE files in the repository root for full details. {% endif %} {% if providers %} - {% set params = next["params"] | default({}) | to_params(prefix="?") %} {% for provider in providers %} {% set name = provider.human_name or (provider.issuer | simplify_url(keep_path=True)) or provider.id %} - + {% endfor %} {% endif %} diff --git a/translations/en.json b/translations/en.json index e7df4b636..3a421887c 100644 --- a/translations/en.json +++ b/translations/en.json @@ -99,7 +99,7 @@ }, "username": "Username", "@username": { - "context": "pages/login.html:51:39-59, pages/register/index.html:30:35-55, pages/register/password.html:37:33-53, pages/upstream_oauth2/do_register.html:101:35-55, pages/upstream_oauth2/do_register.html:107:39-59" + "context": "pages/login.html:51:39-59, pages/register/index.html:31:35-55, pages/register/password.html:37:33-53, pages/upstream_oauth2/do_register.html:101:35-55, pages/upstream_oauth2/do_register.html:107:39-59" } }, "error": { @@ -457,7 +457,7 @@ }, "continue_with_provider": "Continue with %(provider)s", "@continue_with_provider": { - "context": "pages/login.html:81:15-67, pages/register/index.html:57:15-67", + "context": "pages/login.html:81:15-67, pages/register/index.html:59:15-67", "description": "Button to log in with an upstream provider" }, "description": "Please sign in to continue:", @@ -660,25 +660,25 @@ "register": { "call_to_login": "Already have an account?", "@call_to_login": { - "context": "pages/register/index.html:63:35-66, pages/register/password.html:83:33-64", + "context": "pages/register/index.html:65:35-66, pages/register/password.html:83:33-64", "description": "Displayed on the registration page to suggest to log in instead" }, "continue_with_email": "Continue with email address", "@continue_with_email": { - "context": "pages/register/index.html:45:32-69" + "context": "pages/register/index.html:48:32-69" }, "continue_with_password": "Continue with password", "@continue_with_password": { - "context": "pages/register/index.html:47:32-72" + "context": "pages/register/index.html:50:32-72" }, "create_account": { "description": "Choose a username to continue.", "@description": { - "context": "pages/register/index.html:24:29-73" + "context": "pages/register/index.html:25:29-73" }, "heading": "Create an account", "@heading": { - "context": "pages/register/index.html:21:29-69, pages/register/password.html:21:27-67" + "context": "pages/register/index.html:22:29-69, pages/register/password.html:21:27-67" } }, "terms_of_service": "I agree to the Terms and Conditions",