Add more options to deal with localpart conflicts on upstream OAuth 2.0 logins #5295
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
7bfeef9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://58113ed8.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://quenting-upstream-oauth-bett-eq9x.matrix-authentication-service-docs.pages.dev |
sandhose
added
A-Upstream-OAuth
| mas_config::UpstreamOAuth2OnConflict::Replace => { | ||
| mas_data_model::UpstreamOAuthProviderOnConflict::Replace | ||
| } | ||
| mas_config::UpstreamOAuth2OnConflict::Set => { |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Pull request overview
This PR adds two new conflict resolution options (replace and set) for handling upstream OAuth 2.0 localpart conflicts when an existing user matches the upstream provider's localpart. This provides administrators with more granular control over automatic account linking behavior beyond the existing add and fail options.
- Adds
replaceoption to replace any existing upstream links for the same provider on the matching user - Adds
setoption to link only if no existing upstream links exist for that provider on the matching user - Includes comprehensive test coverage for both new conflict resolution strategies
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/setup/sso.md | Updated documentation to describe the new set and replace conflict resolution options with clearer explanations |
| docs/reference/configuration.md | Added documentation comments for the new replace and set options in the configuration reference |
| docs/config.schema.json | Extended the JSON schema to include replace and set as valid enum values for on_conflict |
| crates/data-model/src/upstream_oauth2/provider.rs | Added Replace and Set variants to the OnConflict enum with updated documentation |
| crates/config/src/sections/upstream_oauth2.rs | Updated validation logic and enum definition to support the new conflict resolution options |
| crates/cli/src/sync.rs | Added mappings for the new Replace and Set conflict resolution options |
| crates/handlers/src/upstream_oauth2/link.rs | Implemented the core logic for replace (removes existing links before adding) and set (adds only if no links exist), plus comprehensive test coverage for both scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Fixes #2089
Follow up from #4193
On top of #5293
This adds more granular options for how to deal when an upstream login matches an existing user.
setwill only add the link if and only if there is no other existing link on the user for that provider. This is likely the safest option in most casesreplacewill replace any existing link on the matching user for that provider