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

mcalinghee · 2025-03-11T13:09:08Z

Fixes Point 1 and 4 in #2089

do nothing, and fail the registration/login (corresponds to allow_existing_users: false in Synapse)

import the link without overriding existing links for this user (corresponds to allow_existing_users: true in Synapse)
import the link if and only if there is no existing link for this user

Introducing new configuration on_conflict in section claims_imports.localpart to set the account linking strategy

upstream_oauth2:
  providers:
    - id: 
      claims_imports:
        localpart:
          action: require 
          on_conflict: add

fail (default value): do nothing, and fail the registration/login (corresponds to allow_existing_users: false in Synapse)
add : import the link without overriding existing links for this user (corresponds to allow_existing_users: true in Synapse)
replace : import the link and replace any existing link for this user => not implemented in this PR
set : import the link if and only if there is no existing link for this user => not implemented in this PR

confirmation page when configuration is on_conflict: add :

crates/handlers/src/upstream_oauth2/link.rs

odelcroi · 2025-05-15T08:18:41Z

Wrong configuration of provider

When activating the option allow_existing_users : true, the claim username is used to check if users exist or not. If the attribute mapper username is set to ignore or suggest , the user existence is not checked. Therefore to enable this option the username attribute mapper should be set to require or force.

documentation should mention it
an error should be raised at startup if the configuration does not respect this condition

crates/config/src/sections/upstream_oauth2.rs

crates/handlers/src/upstream_oauth2/link.rs

docs/reference/configuration.md

mcalinghee · 2025-06-16T13:24:36Z

@sandhose : are you ok for this renaming?

sandhose · 2025-06-16T13:25:52Z

@sandhose : are you ok for this renaming?

Yep, makes sense!

sandhose

We're almost there! A few nitpicks, and I also wonder whether the confirmation screen is needed in this case or not; we can also change that later

crates/config/src/sections/upstream_oauth2.rs

templates/pages/upstream_oauth2/login_link.html

crates/handlers/src/upstream_oauth2/link.rs

sandhose · 2025-07-08T09:07:32Z

crates/handlers/src/upstream_oauth2/link.rs

+                                    // new oauth link is allowed
+                                    let ctx = UpstreamExistingLinkContext::new(existing_user)
+                                        .with_csrf(csrf_token.form_value())
+                                        .with_language(locale);
+
+                                    return Ok((
+                                        cookie_jar,
+                                        Html(templates.render_upstream_oauth2_login_link(&ctx)?)
+                                            .into_response(),
+                                    ));


I'm now wondering if it makes sense to show this intermediate page or not? For the 'active session' linking I think it does, as it's user-initiated and we don't want it to happen automatically, whereas here the intent really is for HS admins to transition from local passwords to upstream IDPs, the users should not care about this.

I'm fine with leaving it like that for now, and I'll clean that up later, as we also would like to make import dialog to be optional in some cases

… OAuth 2.0 logins

Co-authored-by: Quentin Gliech <quenting@element.io>

odelcroi · 2025-07-18T08:46:44Z

@sandhose I'm done with the required modifications, I think we can consider updating the user flow by skipping the screen in a second step. If good with you, we can merge :)

sandhose

Thanks! Almost there, just a few minor stylistic things, but I think this should make the cut for the next RC on Tuesday!

sandhose · 2025-07-18T13:06:31Z

crates/handlers/src/upstream_oauth2/link.rs

+        #[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");
+
+        let subject = format!("{}+{}", "subject", timestamp);


The databases are isolated between each test, no need to add timestamps there!

sandhose · 2025-07-18T13:13:39Z

crates/handlers/src/upstream_oauth2/link.rs

+            // User already exists, but it is not linked, neither logged in
+            // Proceed by associating the link and log in the user
+            // Upstream_session is used to re-render the username as it is the only source
+            // of truth


Just a little reflow and rewording of this comment

Suggested change

// User already exists, but it is not linked, neither logged in

// Proceed by associating the link and log in the user

// Upstream_session is used to re-render the username as it is the only source

// of truth

// There is an existing user with the same username, but no link.

// If the configuration allows it, the user is prompted to link the

// existing account. Note that we cannot trust the user input here,

// which is why we have to re-calculate the localpart, instead of

// passing it through form data.

sandhose · 2025-07-18T13:16:16Z

crates/handlers/src/upstream_oauth2/link.rs

+            let localpart = render_attribute_template(&env, template, &context, true)?;
+
+            let maybe_user = repo.user().find_by_username(&localpart.unwrap()).await?;


Better error out than crash (even if we recover from panics!)

Suggested change

let localpart = render_attribute_template(&env, template, &context, true)?;

let maybe_user = repo.user().find_by_username(&localpart.unwrap()).await?;

let Some(localpart) = render_attribute_template(&env, template, &context, true)? else {

// This should never be the case at this point

return Err(RouteError::InvalidFormAction);

};

let maybe_user = repo.user().find_by_username(&localpart).await?;

sandhose · 2025-07-18T13:18:16Z

crates/handlers/src/upstream_oauth2/link.rs

+            &state.clock,
+            &mut repo,
+            existing_username.clone(),
+            existing_email.clone(),


Why does this test need an email? I don't think the create_user utility is very helpful here

sandhose · 2025-07-18T13:20:50Z

crates/data-model/src/upstream_oauth2/provider.rs

+
+impl OnConflict {
+    #[must_use]
+    pub fn is_add(&self) -> bool {
+        matches!(self, Self::Add)
+    }
+}


I don't see it used?

Suggested change

impl OnConflict {

#[must_use]

pub fn is_add(&self) -> bool {

matches!(self, Self::Add)

}

}

mcalinghee changed the title ~~Allow existing user~~ wip: allow existing user Mar 11, 2025

mcalinghee force-pushed the feat/allow_override_user branch 6 times, most recently from 9d35eeb to d50b6c0 Compare March 12, 2025 16:20

mcalinghee changed the title ~~wip: allow existing user~~ allow importing existing users when the localpart matches in upstream OAuth 2.0 logins Mar 12, 2025

mcalinghee marked this pull request as ready for review March 12, 2025 16:21

mcalinghee mentioned this pull request Mar 12, 2025

Make it possible to auto-import existing users when the localpart matches in upstream OAuth 2.0 logins #2089

Open

mcalinghee force-pushed the feat/allow_override_user branch from d50b6c0 to 405abce Compare March 12, 2025 16:47

mcalinghee marked this pull request as draft March 12, 2025 16:52

mcalinghee force-pushed the feat/allow_override_user branch from 405abce to 11f17cc Compare March 12, 2025 17:18

mcalinghee marked this pull request as ready for review March 12, 2025 17:20

mcalinghee force-pushed the feat/allow_override_user branch 3 times, most recently from 8f7d65b to 0307f3c Compare March 17, 2025 10:32

mcalinghee force-pushed the feat/allow_override_user branch from 0307f3c to 63f811a Compare March 25, 2025 15:27

mcalinghee force-pushed the feat/allow_override_user branch 2 times, most recently from fa428d9 to 3d64262 Compare April 11, 2025 06:55

mcalinghee changed the title ~~allow importing existing users when the localpart matches in upstream OAuth 2.0 logins~~ allow account linking for existing users when the localpart matches in upstream OAuth 2.0 logins Apr 14, 2025

mcalinghee force-pushed the feat/allow_override_user branch from 3d64262 to f5a3404 Compare April 14, 2025 13:01

odelcroi force-pushed the feat/allow_override_user branch 2 times, most recently from e2b0077 to 4405e08 Compare May 14, 2025 09:29

odelcroi reviewed May 14, 2025

View reviewed changes

crates/handlers/src/upstream_oauth2/link.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

mcalinghee commented May 15, 2025

View reviewed changes

crates/config/src/sections/upstream_oauth2.rs Outdated Show resolved Hide resolved

odelcroi approved these changes May 19, 2025

View reviewed changes

crates/handlers/src/upstream_oauth2/link.rs Outdated Show resolved Hide resolved

docs/reference/configuration.md Outdated Show resolved Hide resolved

mcalinghee marked this pull request as draft May 19, 2025 07:43

mcalinghee force-pushed the feat/allow_override_user branch 5 times, most recently from 37f772c to 22b3d5d Compare June 13, 2025 15:22

odelcroi force-pushed the feat/allow_override_user branch from 22b3d5d to 39716c9 Compare June 16, 2025 08:39

mcalinghee marked this pull request as ready for review June 16, 2025 10:11

mcalinghee requested a review from sandhose June 16, 2025 13:35

odelcroi force-pushed the feat/allow_override_user branch 2 times, most recently from 93a61bd to 3d4d1a6 Compare July 3, 2025 13:46

sandhose reviewed Jul 8, 2025

View reviewed changes

odelcroi force-pushed the feat/allow_override_user branch 4 times, most recently from e910c49 to 00d8171 Compare July 17, 2025 09:06

odelcroi approved these changes Jul 17, 2025

View reviewed changes

mcalinghee and others added 9 commits July 17, 2025 14:37

allow importing existing users when the localpart matches in upstream…

ec31000

… OAuth 2.0 logins

use of UpstreamOAuthProviderLocalpartPreference

0e03f76

rename is_forced to is_forced_or_required

2eb59bc

use match

10bda44

Co-authored-by: Quentin Gliech <quenting@element.io>

update license

6b91246

Co-authored-by: Quentin Gliech <quenting@element.io>

Update crates/handlers/src/upstream_oauth2/link.rs

94e358f

Co-authored-by: Quentin Gliech <quenting@element.io>

fix code style

31cc23a

Co-authored-by: Quentin Gliech <quenting@element.io>

add semi column

dbdaa79

fix rust style

ea8c8d2

odelcroi force-pushed the feat/allow_override_user branch from b877b9c to ea8c8d2 Compare July 17, 2025 12:37

sandhose approved these changes Jul 18, 2025

View reviewed changes

-            // User already exists, but it is not linked, neither logged in
-            // Proceed by associating the link and log in the user
-            // Upstream_session is used to re-render the username as it is the only source
-            // of truth
+            // There is an existing user with the same username, but no link.
+            // If the configuration allows it, the user is prompted to link the
+            // existing account. Note that we cannot trust the user input here,
+            // which is why we have to re-calculate the localpart, instead of
+            // passing it through form data.

		let localpart = render_attribute_template(&env, template, &context, true)?;

		let maybe_user = repo.user().find_by_username(&localpart.unwrap()).await?;

-            let localpart = render_attribute_template(&env, template, &context, true)?;
-            let maybe_user = repo.user().find_by_username(&localpart.unwrap()).await?;
+            let Some(localpart) = render_attribute_template(&env, template, &context, true)? else {
+                // This should never be the case at this point
+                return Err(RouteError::InvalidFormAction);
+            };
+            let maybe_user = repo.user().find_by_username(&localpart).await?;

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

mcalinghee commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

odelcroi commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Wrong configuration of provider

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

mcalinghee commented Jun 16, 2025

Uh oh!

sandhose commented Jun 16, 2025

Uh oh!

sandhose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandhose Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

odelcroi commented Jul 18, 2025

Uh oh!

sandhose left a comment

Choose a reason for hiding this comment

Uh oh!

sandhose Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandhose Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandhose Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandhose Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandhose Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcalinghee commented Mar 11, 2025 •

edited

Loading

odelcroi commented May 15, 2025 •

edited

Loading