-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
feat(core): Add OIDC support for SSO #15988
Conversation
05a9b19
to
e02da5e
Compare
1e58dbc
to
bb6adbd
Compare
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
d8cce48
to
4ec25f1
Compare
There was a problem hiding this 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.
packages/frontend/@n8n/design-system/src/components/N8nUsersList/UsersList.vue
Show resolved
Hide resolved
There was a problem hiding this 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; |
There was a problem hiding this comment.
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<{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 ?
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. |
There was a problem hiding this 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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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 && |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional:
signInType: providerType ? providerType : 'email', | |
signInType: providerType ?? 'email', |
throw new BadRequestError('Email needs to be verified'); | ||
} | ||
|
||
if (!userInfo.email) { |
There was a problem hiding this comment.
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 }, |
There was a problem hiding this comment.
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 ?
There was a problem hiding this 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 👏
✅ All Cypress E2E specs passed |
87c9895
to
86fc1ad
Compare
✅ All Cypress E2E specs passed |
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
release/backport
(if the PR is an urgent fix that needs to be backported)