Skip to content

feat(core): Add OIDC support for SSO #15988

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

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Jun 3, 2025

Summary

Implement OIDC support for single sign on.

Related Linear tickets, Github issues, and Community forum posts

closes https://linear.app/n8n/issue/PAY-2836/spike-implement-support-for-oidc-authentication-method-for-sso

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 3, 2025
@RicardoE105 RicardoE105 force-pushed the pay-2836-spike-implement-support-for-oidc-authentication-method-for branch 2 times, most recently from 05a9b19 to e02da5e Compare June 3, 2025 18:24
@RicardoE105 RicardoE105 changed the title OIDC MVP feat: Add OIDC support Jun 3, 2025
@afitzek afitzek force-pushed the pay-2836-spike-implement-support-for-oidc-authentication-method-for branch 2 times, most recently from 1e58dbc to bb6adbd Compare June 11, 2025 15:42
@afitzek afitzek changed the title feat: Add OIDC support feat(core): Add OIDC support for SSO Jun 12, 2025
@afitzek afitzek force-pushed the pay-2836-spike-implement-support-for-oidc-authentication-method-for branch 2 times, most recently from d8cce48 to 4ec25f1 Compare June 12, 2025 08:33
@afitzek afitzek marked this pull request as ready for review June 12, 2025 10:09
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic found 7 issues across 41 files. Review them in cubic.dev

React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.

Copy link
Contributor

@guillaumejacquart guillaumejacquart left a comment

Choose a reason for hiding this comment

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

Some comments
I was wondering: we don’t handle refresh tokens — is that a deliberate choice?

const isEmailBeingChanged = email !== currentEmail;
const isFirstNameChanged = firstName !== currentFirstName;
Copy link
Contributor

Choose a reason for hiding this comment

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

(completely optional, just if you think it makes sense)
Maybe we could have some kind of map of auth method => check if can change profile, in order to merge both ldap / oids and saml check into one (because it's doing the same business logic)

}

private cachedOidcConfiguration:
| Promise<{
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I see the point of having a promise here

Copy link
Contributor

Choose a reason for hiding this comment

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

A the promise is in the wrong place. the idea is that we can have a code path in getOidcConfiguration that doesn't yield to avoid race conditions.

@@ -32,6 +33,10 @@ export function getSamlLoginLabel(): string {
return config.getEnv(SAML_LOGIN_LABEL);
}

export function isOidcLoginEnabled(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have a function called isOidcLoginEnabled is the saml-helper file ?

@afitzek
Copy link
Contributor

afitzek commented Jun 13, 2025

Some comments I was wondering: we don’t handle refresh tokens — is that a deliberate choice?

I don't see why we need refresh tokens. We are just using OIDC for a one time login to fetch the id token from the provider, after that we authenticate the user with our internal mechanism.

Copy link
Contributor

@guillaumejacquart guillaumejacquart left a comment

Choose a reason for hiding this comment

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

Well done on the fixes
Some more comments !

!(
user &&
(hasGlobalScope(user, 'user:resetPassword') || user.settings?.allowSSOManualLogin === true)
)
) {
const currentAuthenticationMethod = isSamlCurrentAuthenticationMethod() ? 'SAML' : 'OIDC';
this.logger.debug(
'Request to send password reset email failed because login is handled by SAML',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Request to send password reset email failed because login is handled by SAML',
`Request to send password reset email failed because login is handled by ${currentAuthenticationMethod}`,

@@ -76,17 +79,18 @@ export class PasswordResetController {
}

if (
isSamlCurrentAuthenticationMethod() &&
(isSamlCurrentAuthenticationMethod() || isOidcCurrentAuthenticationMethod()) &&
!(
user &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth checking is user exist at that point. code would already have failed before if user was falsy


let publicUser: PublicUser = {
...rest,
signInType: ldapIdentity ? 'ldap' : 'email',
signInType: providerType ? providerType : 'email',
Copy link
Contributor

Choose a reason for hiding this comment

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

optional:

Suggested change
signInType: providerType ? providerType : 'email',
signInType: providerType ?? 'email',

throw new BadRequestError('Email needs to be verified');
}

if (!userInfo.email) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth checking the email exist before it is verified (in case user wants to debug his login issues)

}

const openidUser = await this.authIdentityRepository.findOne({
where: { providerId: claims.sub },
Copy link
Contributor

Choose a reason for hiding this comment

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

just for future proofing here, could we also add to the where the provider type ?

Copy link
Contributor

@guillaumejacquart guillaumejacquart left a comment

Choose a reason for hiding this comment

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

Looks good to me 👏

Copy link
Contributor

✅ All Cypress E2E specs passed

@afitzek afitzek force-pushed the pay-2836-spike-implement-support-for-oidc-authentication-method-for branch from 87c9895 to 86fc1ad Compare June 13, 2025 13:46
Copy link
Contributor

✅ All Cypress E2E specs passed

@afitzek afitzek merged commit 30148df into master Jun 13, 2025
47 of 48 checks passed
@afitzek afitzek deleted the pay-2836-spike-implement-support-for-oidc-authentication-method-for branch June 13, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants