Skip to content

allow account linking for existing users when the localpart matches in upstream OAuth 2.0 logins #4193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/cli/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ pub async fn config_sync(
fetch_userinfo: provider.fetch_userinfo,
userinfo_signed_response_alg: provider.userinfo_signed_response_alg,
response_mode,
allow_existing_users: provider.allow_existing_users,
additional_authorization_parameters: provider
.additional_authorization_parameters
.into_iter()
Expand Down
19 changes: 19 additions & 0 deletions crates/config/src/sections/upstream_oauth2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,17 @@ impl ConfigurationSection for UpstreamOAuth2Config {
}
}
}

if provider.allow_existing_users
&& !matches!(
provider.claims_imports.localpart.action,
ImportAction::Force | ImportAction::Require
)
{
return annotate(figment::Error::custom(
"When `allow_existing_users` is true, localpart claim import must be either `force` or `require`",
));
}
}

Ok(())
Expand Down Expand Up @@ -411,6 +422,7 @@ fn is_default_scope(scope: &str) -> bool {
/// Configuration for one upstream OAuth 2 provider.
#[skip_serializing_none]
#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
#[allow(clippy::struct_excessive_bools)]
pub struct Provider {
/// Whether this provider is enabled.
///
Expand Down Expand Up @@ -571,6 +583,13 @@ pub struct Provider {
#[serde(default, skip_serializing_if = "ClaimsImports::is_default")]
pub claims_imports: ClaimsImports,

/// Whether to allow a user logging in via OIDC to match a pre-existing
/// account instead of failing. This could be used if switching from
/// password logins to OIDC.
//Defaults to false.
#[serde(default)]
pub allow_existing_users: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with #2089, I would like the configuration flag to not just be a boolean, but rather an enum with:

  • fail (the default) which fails the registration on conflict
  • add/append which adds the link, regardless of whether there is an existing link or not
  • replace/overwrite which overwrites any existing link
  • set/add-if-missing/set-new which adds a link only if there are no existing link for that provider on the user

Not 100% sure about the naming, and I'm fine with only having the fail and add options for now

I think it also makes sense to put that in the claims_imports.localpart section; as this is stored as a JSON blob in the database, it even avoids the database migration for this

claims_imports:
  localpart:
    on_conflict: add

What do you think?


/// Additional parameters to include in the authorization request
///
/// Orders of the keys are not preserved.
Expand Down
1 change: 1 addition & 0 deletions crates/data-model/src/upstream_oauth2/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ pub struct UpstreamOAuthProvider {
pub created_at: DateTime<Utc>,
pub disabled_at: Option<DateTime<Utc>>,
pub claims_imports: ClaimsImports,
pub allow_existing_users: bool,
pub additional_authorization_parameters: Vec<(String, String)>,
pub forward_login_hint: bool,
}
Expand Down
1 change: 1 addition & 0 deletions crates/handlers/src/admin/v1/upstream_oauth_links/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod test_utils {
token_endpoint_override: None,
userinfo_endpoint_override: None,
jwks_uri_override: None,
allow_existing_users: true,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
ui_order: 0,
Expand Down
1 change: 1 addition & 0 deletions crates/handlers/src/upstream_oauth2/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ mod tests {
created_at: clock.now(),
disabled_at: None,
claims_imports: UpstreamOAuthProviderClaimsImports::default(),
allow_existing_users: false,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
};
Expand Down
243 changes: 233 additions & 10 deletions crates/handlers/src/upstream_oauth2/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,9 @@ pub(crate) async fn get(
.await
.map_err(RouteError::HomeserverConnection)?;

if maybe_existing_user.is_some() || !is_available {
if !provider.allow_existing_users
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is a bit hard to follow. In case we link to an existing user, I don't think we'd want to import the email address (we might end up with duplicate email addresses to deal with) or display name, just link the user.

This means I would rather branch out here with something like:

if let Some(existing_user) {
    if provider.allow_existing_users {
        // Render the UI to link the user
        return}

    // Show the error about upstream account not being linked to this account
}

if !is_available {
    // Show an error about the user not being available on Synapse (could be an AS namespace for example)
}

This also implies its own view, rather than reusing the do_register template

It also means we don't run the 'registration' policy for an account link, which I think makes sense

&& (maybe_existing_user.is_some() || !is_available)
{
if let Some(existing_user) = maybe_existing_user {
// The mapper returned a username which already exists, but isn't
// linked to this upstream user.
Expand Down Expand Up @@ -749,15 +751,16 @@ pub(crate) async fn post(
mas_templates::UpstreamRegisterFormField::Username,
FieldError::Required,
);
} else if repo.user().exists(&username).await? {
} else if !provider.allow_existing_users && repo.user().exists(&username).await? {
form_state.add_error_on_field(
mas_templates::UpstreamRegisterFormField::Username,
FieldError::Exists,
);
} else if !homeserver
.is_localpart_available(&username)
.await
.map_err(RouteError::HomeserverConnection)?
} else if !provider.allow_existing_users
&& !homeserver
.is_localpart_available(&username)
.await
.map_err(RouteError::HomeserverConnection)?
{
// The user already exists on the homeserver
tracing::warn!(
Expand Down Expand Up @@ -837,10 +840,22 @@ pub(crate) async fn post(
.into_response());
}

REGISTRATION_COUNTER.add(1, &[KeyValue::new(PROVIDER, provider.id.to_string())]);

// Now we can create the user
let user = repo.user().add(&mut rng, &clock, username).await?;
let user = if provider.allow_existing_users {
// If the provider allows existing users, we can use the existing user
let existing_user = repo.user().find_by_username(&username).await?;
if existing_user.is_some() {
existing_user.unwrap()
} else {
REGISTRATION_COUNTER
.add(1, &[KeyValue::new(PROVIDER, provider.id.to_string())]);
// This case should not happen
repo.user().add(&mut rng, &clock, username).await?
}
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal up there would remove all this complexity (and the potential email conflict)

REGISTRATION_COUNTER.add(1, &[KeyValue::new(PROVIDER, provider.id.to_string())]);
// Now we can create the user
repo.user().add(&mut rng, &clock, username).await?
};

if let Some(terms_url) = &site_config.tos_uri {
repo.user_terms()
Expand Down Expand Up @@ -982,6 +997,7 @@ mod tests {
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
response_mode: None,
allow_existing_users: true,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
ui_order: 0,
Expand Down Expand Up @@ -1095,4 +1111,211 @@ mod tests {

assert_eq!(email.email, "[email protected]");
}

#[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
async fn test_link_existing_account(pool: PgPool) {
#[allow(clippy::disallowed_methods)]
let timestamp = chrono::Utc::now().timestamp_millis();

//suffix timestamp to generate unique test data
let existing_username = format!("{}{}", "john",timestamp);
let existing_email = format!("{}@{}", existing_username, "example.com");

//existing username matches oidc username
let oidc_username = existing_username.clone();

//oidc email is different from existing email
let oidc_email: String = format!("{}{}@{}", "any_email", timestamp,"example.com");

//generate unique subject
let subject = format!("{}+{}", "subject", timestamp);

setup();
let state = TestState::from_pool(pool).await.unwrap();
let mut rng = state.rng();
let cookies = CookieHelper::new();

let claims_imports = UpstreamOAuthProviderClaimsImports {
localpart: UpstreamOAuthProviderImportPreference {
action: mas_data_model::UpstreamOAuthProviderImportAction::Require,
template: None,
},
email: UpstreamOAuthProviderImportPreference {
action: mas_data_model::UpstreamOAuthProviderImportAction::Require,
template: None,
},
..UpstreamOAuthProviderClaimsImports::default()
};

let id_token = serde_json::json!({
"preferred_username": oidc_username,
"email": oidc_email,
"email_verified": true,
});

// Grab a key to sign the id_token
// We could generate a key on the fly, but because we have one available here,
// why not use it?
let key = state
.key_store
.signing_key_for_algorithm(&JsonWebSignatureAlg::Rs256)
.unwrap();

let signer = key
.params()
.signing_key_for_alg(&JsonWebSignatureAlg::Rs256)
.unwrap();
let header = JsonWebSignatureHeader::new(JsonWebSignatureAlg::Rs256);
let id_token = Jwt::sign_with_rng(&mut rng, header, id_token, &signer).unwrap();

// Provision a provider and a link
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,
allow_existing_users: true,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
ui_order: 0,
},
)
.await
.unwrap();

let session = repo
.upstream_oauth_session()
.add(
&mut rng,
&state.clock,
&provider,
"state".to_owned(),
None,
Some("nonce".to_owned()),
)
.await
.unwrap();

let link = repo
.upstream_oauth_link()
.add(
&mut rng,
&state.clock,
&provider,
subject.clone(),
None,
)
.await
.unwrap();

let session = repo
.upstream_oauth_session()
.complete_with_link(
&state.clock,
session,
&link,
Some(id_token.into_string()),
None,
None,
)
.await
.unwrap();

//create a user with an email
let user = repo
.user()
.add(&mut rng, &state.clock, existing_username.clone())
.await
.unwrap();

let _user_email = repo
.user_email()
.add(&mut rng, &state.clock, &user, existing_email.clone())
.await;

repo.save().await.unwrap();

let cookie_jar = state.cookie_jar();
let upstream_sessions = UpstreamSessionsCookie::default()
.add(session.id, provider.id, "state".to_owned(), None)
.add_link_to_session(session.id, link.id)
.unwrap();
let cookie_jar = upstream_sessions.save(cookie_jar, &state.clock);
cookies.import(cookie_jar);

let request = Request::get(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).empty();
let request = cookies.with_cookies(request);
let response = state.request(request).await;
cookies.save_cookies(&response);
response.assert_status(StatusCode::OK);
response.assert_header_value(CONTENT_TYPE, "text/html; charset=utf-8");

// Extract the CSRF token from the response body
let csrf_token = response
.body()
.split("name=\"csrf\" value=\"")
.nth(1)
.unwrap()
.split('\"')
.next()
.unwrap();

let request = Request::post(&*mas_router::UpstreamOAuth2Link::new(link.id).path()).form(
serde_json::json!({
"csrf": csrf_token,
"action": "register",
"import_email": "on",
"accept_terms": "on",
}),
);
let request = cookies.with_cookies(request);
let response = state.request(request).await;
cookies.save_cookies(&response);
response.assert_status(StatusCode::SEE_OTHER);

// Check that the existing user has the oidc link
let mut repo = state.repository().await.unwrap();

let link = repo
.upstream_oauth_link()
.find_by_subject(&provider, &subject)
.await
.unwrap()
.expect("link exists");

assert_eq!(link.user_id, Some(user.id));

let page = repo
.user_email()
.list(UserEmailFilter::new().for_user(&user), Pagination::first(1))
.await
.unwrap();

//check that the existing user email is updated by oidc email
assert_eq!(page.edges.len(), 1);
let email = page.edges.first().expect("email exists");

assert_eq!(email.email, oidc_email);
}
}
2 changes: 2 additions & 0 deletions crates/handlers/src/views/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,7 @@ mod test {
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
response_mode: None,
allow_existing_users: true,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
ui_order: 0,
Expand Down Expand Up @@ -539,6 +540,7 @@ mod test {
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
response_mode: None,
allow_existing_users: true,
additional_authorization_parameters: Vec::new(),
forward_login_hint: false,
ui_order: 1,
Expand Down
Loading