dandri/matrix-authentication-service
1
0
Fork 0
mirror of https://github.com/element-hq/matrix-authentication-service.git synced 2026-10-15 15:31:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

Carry the username typed on the registration page through the upstream OAuth 2.0 flow

Browse Source 
It is stashed in the upstream sessions cookie and used as the localpart
when the user comes back, unless the provider forces or requires its
own.
This commit is contained in:
Quentin Gliech
2026-09-18 18:11:15 +02:00
parent b6c6e97461
commit 877e279fbf
3 changed files with 378 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/handlers/src/upstream_oauth2/authorize.rs
+9 -1
View File
@@ -91,6 +91,7 @@ pub(crate) async fn start_authorization(
cookie_jar: CookieJar,
provider: &UpstreamOAuthProvider,
post_auth_action: Option<PostAuthAction>,
username: Option<String>,
) -> 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?;
crates/handlers/src/upstream_oauth2/cookie.rs
+47 -6
View File
@@ -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<Ulid>,
post_auth_action: Option<PostAuthAction>,
/// 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<String>,
}
impl Payload {
@@ -87,6 +93,7 @@ impl UpstreamSessions {
provider: Ulid,
state: String,
post_auth_action: Option<PostAuthAction>,
username: Option<String>,
) -> 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);
}
}
crates/handlers/src/upstream_oauth2/link.rs
+322 -32
View File
@@ -245,10 +245,14 @@ pub(crate) async fn get(
) -> Result<impl IntoResponse, RouteError> {
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<String>,
) -> 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");
}
}