Skip to content

Add more options to deal with localpart conflicts on upstream OAuth 2.0 logins #5295

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.

Sign up for GitHub

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

Merged
sandhose merged 6 commits into from
Dec 3, 2025
Merged

Add more options to deal with localpart conflicts on upstream OAuth 2.0 logins #5295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffb86f6
Add more options to deal with localpart conflicts on upstream OAuth 2…
sandhose Nov 28, 2025
9650dc1
Add tests for the new on_conflict options
sandhose Nov 28, 2025
f0d84a4
Document the new conflict options
sandhose Nov 28, 2025
c5ba1f6
Check for the new on_conflict options & update docs
sandhose Nov 28, 2025
8384a5a
Merge branch 'main' into quenting/upstream-oauth/better-conflict-options
sandhose Nov 28, 2025
7bfeef9
Typos and error message rewording
sandhose Dec 1, 2025
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
6 changes: 6 additions & 0 deletions crates/cli/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ fn map_import_on_conflict(
mas_config::UpstreamOAuth2OnConflict::Add => {
mas_data_model::UpstreamOAuthProviderOnConflict::Add
}
mas_config::UpstreamOAuth2OnConflict::Replace => {
mas_data_model::UpstreamOAuthProviderOnConflict::Replace
}
mas_config::UpstreamOAuth2OnConflict::Set => {
Copy link
Contributor

@reivilibre reivilibre Nov 28, 2025

Choose a reason for hiding this comment

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

Not the hugest fan of calling this Set because it doesn't give off any vibe as to what it means. Maybe SetNew, kinda like OpenOptions::create_new?
Or SetIfNotExists kinda like SQL CREATE IF NOT EXISTS (even though now that I think about it, that's not great grammar :D)?

Copy link
Member Author

@sandhose sandhose Nov 28, 2025

Choose a reason for hiding this comment

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

Errr I don't know; at the same time, set is probably the option which makes most sense in most cases, so I don't want it to look long and complicated

Copy link
Member Author

@sandhose sandhose Dec 3, 2025

Choose a reason for hiding this comment

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

I think that as long as it's documented, it's fine… I can't really think of another single word to better express it?

mas_data_model::UpstreamOAuthProviderOnConflict::Set
}
mas_config::UpstreamOAuth2OnConflict::Fail => {
mas_data_model::UpstreamOAuthProviderOnConflict::Fail
}
Expand Down
19 changes: 13 additions & 6 deletions crates/config/src/sections/upstream_oauth2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,14 @@ impl ConfigurationSection for UpstreamOAuth2Config {

if matches!(
provider.claims_imports.localpart.on_conflict,
OnConflict::Add
OnConflict::Add | OnConflict::Replace | OnConflict::Set
) && !matches!(
provider.claims_imports.localpart.action,
ImportAction::Force | ImportAction::Require
) {
return Err(annotate(figment::Error::custom(
"The field `action` must be either `force` or `require` when `on_conflict` is set to `add`",
)).into());
"The field `action` must be either `force` or `require` when `on_conflict` is set to `add`, `replace` or `set`",
)).with_path("claims_imports.localpart").into());
}
}

Expand Down Expand Up @@ -206,13 +206,20 @@ impl ImportAction {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default, JsonSchema)]
#[serde(rename_all = "lowercase")]
pub enum OnConflict {
/// Fails the sso login on conflict
/// Fails the upstream OAuth 2.0 login on conflict
#[default]
Fail,

/// Adds the oauth identity link, regardless of whether there is an existing
/// link or not
/// Adds the upstream OAuth 2.0 identity link, regardless of whether there
/// is an existing link or not
Add,

/// Replace any existing upstream OAuth 2.0 identity link
Replace,

/// Adds the upstream OAuth 2.0 identity link *only* if there is no existing
/// link for this provider on the matching user
Set,
}

impl OnConflict {
Expand Down
13 changes: 10 additions & 3 deletions crates/data-model/src/upstream_oauth2/provider.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,11 +415,18 @@ impl ImportAction {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
#[serde(rename_all = "lowercase")]
pub enum OnConflict {
/// Fails the upstream OAuth 2.0 login
/// Fails the upstream OAuth 2.0 login on conflict
#[default]
Fail,

/// Adds the upstream account link, regardless of whether there is an
/// existing link or not
/// Adds the upstream OAuth 2.0 identity link, regardless of whether there
/// is an existing link or not
Add,

/// Replace any existing upstream OAuth 2.0 identity link
Replace,

/// Adds the upstream OAuth 2.0 identity link *only* if there is no existing
/// link for this provider on the matching user
Set,
}
Loading
Loading