diff --git a/crates/handlers/src/upstream_oauth2/authorize.rs b/crates/handlers/src/upstream_oauth2/authorize.rs index ea5a35053..364ba1cbb 100644 --- a/crates/handlers/src/upstream_oauth2/authorize.rs +++ b/crates/handlers/src/upstream_oauth2/authorize.rs @@ -91,6 +91,7 @@ pub(crate) async fn start_authorization( cookie_jar: CookieJar, provider: &UpstreamOAuthProvider, post_auth_action: Option, + username: 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 @@ -194,7 +195,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, + username, + ) .save(cookie_jar, clock); Ok((cookie_jar, url)) @@ -233,6 +240,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..abf18258c 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. // @@ -27,6 +28,11 @@ pub struct Payload { state: String, link: Option, post_auth_action: Option, + + /// The username typed on the registration page, when the flow started + /// there with the field filled in + #[serde(skip_serializing_if = "Option::is_none")] + username: Option, } impl Payload { @@ -87,6 +93,7 @@ impl UpstreamSessions { provider: Ulid, state: String, post_auth_action: Option, + username: Option, ) -> Self { self.0.push(Payload { session, @@ -94,6 +101,7 @@ impl UpstreamSessions { state, link: None, post_auth_action, + username, }); self } @@ -131,13 +139,19 @@ impl UpstreamSessions { pub fn lookup_link( &self, link_id: Ulid, - ) -> Result<(Ulid, Option<&PostAuthAction>), UpstreamSessionNotFound> { + ) -> Result<(Ulid, Option<&PostAuthAction>, Option<&str>), 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.username.as_deref(), + ) + }) .ok_or(UpstreamSessionNotFound) } @@ -178,13 +192,19 @@ 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("john".to_owned()), + ); let sessions = sessions.expire(now); assert_eq!( @@ -216,11 +236,32 @@ 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 username it carries + let (session, _post_auth_action, username) = sessions.lookup_link(second_link).unwrap(); + assert_eq!(session, second_session); + assert_eq!(username, 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()); } + + /// A cookie payload written before the username field existed still + /// deserializes, and carries no username + #[test] + fn test_payload_without_username() { + 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 username should deserialize"); + + let link = "01FSHN9AG09NMZYX8MFVH74RP4".parse().unwrap(); + let (_session, _post_auth_action, username) = sessions.lookup_link(link).unwrap(); + assert_eq!(username, None); + } } diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index 51de8ac5a..f2486c5a9 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, username) = sessions_cookie .lookup_link(link_id) .map_err(|_| RouteError::MissingCookie)?; + // Owned copy of the username the user typed on the registration page before + // starting the flow, if any, so we can consume the cookie later. + let carried_username = username.map(ToOwned::to_owned); + let link = repo .upstream_oauth_link() .lookup(link_id) @@ -454,34 +458,47 @@ pub(crate) async fn get( )? }; + let forced_or_required = provider.claims_imports.localpart.is_forced_or_required(); + + // What the user typed on the registration page beats the claim rendered + // from the upstream response, even when that claim is ignored. A provider + // which forces or requires the localpart never lets the user pick one. + 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() { + let localpart = if let Some(username) = carried_username { + username + } else if provider.claims_imports.localpart.ignore() { break 'localpart None; - } + } else { + 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?; @@ -491,7 +508,7 @@ pub(crate) async fn get( upstream_oauth_provider.id = %provider.id, upstream_oauth_link.id = %link.id, user.id = %existing_user.id, - "Upstream provider returned a localpart {localpart:?} which is already used by another user. As the username is just a suggestion, it was ignored." + "Localpart {localpart:?} is already used by another user. As the username is just a suggestion, it was ignored." ); break 'localpart None; } @@ -714,7 +731,7 @@ pub(crate) async fn get( tracing::warn!( upstream_oauth_provider.id = %provider.id, upstream_oauth_link.id = %link.id, - "Upstream provider returned a localpart {localpart:?} which was denied by the policy ({res}). As the username is just a suggestion, it was ignored." + "Localpart {localpart:?} was denied by the policy ({res}). As the username is just a suggestion, it was ignored." ); break 'localpart None; } @@ -748,7 +765,7 @@ pub(crate) async fn get( tracing::warn!( upstream_oauth_provider.id = %provider.id, upstream_oauth_link.id = %link.id, - "Upstream provider returned a localpart {localpart:?} which isn't available on the homeserver. As the username is just a suggestion, it was ignored." + "Localpart {localpart:?} isn't available on the homeserver. As the username is just a suggestion, it was ignored." ); break 'localpart None; } @@ -869,7 +886,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 carried username 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, _username) = sessions_cookie .lookup_link(link_id) .map_err(|_| RouteError::MissingCookie)?; @@ -1256,7 +1275,7 @@ async fn prepare_user_registration( #[cfg(test)] mod tests { - use hyper::{Request, StatusCode, header::CONTENT_TYPE}; + use hyper::{Request, Response, StatusCode, header::CONTENT_TYPE}; use mas_data_model::{ UpstreamOAuthAuthorizationSession, UpstreamOAuthLink, UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderImportPreference, UpstreamOAuthProviderLocalpartPreference, @@ -1399,7 +1418,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 +1616,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 +1750,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 +1874,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 +2064,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 +2195,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 +2333,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 +2376,275 @@ 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, + skip_confirmation: bool, + ) -> UpstreamOAuthLink { + let mut rng = state.rng(); + + let claims_imports = UpstreamOAuthProviderClaimsImports { + localpart: UpstreamOAuthProviderLocalpartPreference { + action: localpart, + template: None, + on_conflict: mas_data_model::UpstreamOAuthProviderOnConflict::default(), + }, + skip_confirmation, + ..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(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", + false, + ) + .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", + false, + ) + .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", + false, + ) + .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", + false, + ) + .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); + assert!(!response.body().contains(r#"value="alice""#)); + assert!(!response.body().contains(r#"value="john""#)); + } + + /// Look up the user registration the response redirects to + async fn lookup_registration( + state: &TestState, + response: &Response, + ) -> UserRegistration { + let location = response.headers().get(hyper::header::LOCATION).unwrap(); + // The location is /register/steps/{id}/finish + let registration_id: Ulid = str::from_utf8(location.as_bytes()) + .unwrap() + .rsplit('/') + .nth(1) + .expect("Location to have two slashes") + .parse() + .expect("last segment of location to be a ULID"); + + let mut repo = state.repository().await.unwrap(); + repo.user_registration() + .lookup(registration_id) + .await + .unwrap() + .expect("user registration exists") + } + + /// A provider which skips the confirmation registers under the carried + /// username, without showing the form + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_carried_username_with_skip_confirmation(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", + true, + ) + .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::SEE_OTHER); + + let registration = lookup_registration(&state, &response).await; + assert_eq!(registration.username, "alice"); + } + + /// The carried username is what makes the registration possible when the + /// provider ignores the localpart claim and skips the confirmation + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_carried_username_with_skip_confirmation_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", + true, + ) + .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::SEE_OTHER); + + let registration = lookup_registration(&state, &response).await; + assert_eq!(registration.username, "alice"); + } }