feat: Add OIDC SSO#84
Conversation
serendipty01
force-pushed
the
add-oidc-sso
branch
7 times, most recently
from
97bba63 to
515762c
Compare
serendipty01
force-pushed
the
add-oidc-sso
branch
2 times, most recently
from
298aa43 to
3cd5807
Compare
serendipty01
force-pushed
the
add-oidc-sso
branch
2 times, most recently
from
6bcf124 to
87e92bd
Compare
serendipty01
requested review from
a team,
cassidyjames,
dom-notion,
juanmrad,
julietshen,
pawiecz and
vinaysrao1
as code owners
serendipty01
force-pushed
the
add-oidc-sso
branch
4 times, most recently
from
4da4829 to
daacab4
Compare
juanmrad
left a comment
juanmrad
left a comment
There was a problem hiding this comment.
a couple more comments. 😅 Thanks for all the hard work.
|
@serendipty01 left you a couple more comments. let me know if you need help, and again thanks for contributing. |
serendipty01
force-pushed
the
add-oidc-sso
branch
5 times, most recently
from
658ed70 to
a0d56a8
Compare
|
|
||
| permissions: | ||
| pull-requests: write | ||
| issues: write |
There was a problem hiding this comment.
this is for resolving the failing checklist job.
Should i remove this or raise a separate PR for this ?
|
|
||
| export const DateStringArbitrary = fc | ||
| .date() | ||
| .date({ noInvalidDate: true }) |
There was a problem hiding this comment.
this change is for fixing a test failure
serendipty01
force-pushed
the
add-oidc-sso
branch
2 times, most recently
from
f224b1a to
c65e0b8
Compare
… from schema, use ConfigService for apiUrl
serendipty01
force-pushed
the
add-oidc-sso
branch
from
c65e0b8 to
c3950d2
Compare
juanmrad
left a comment
juanmrad
left a comment
There was a problem hiding this comment.
@serendipty01 Just to doubly confirm you manually tested this change correct?
ThisIsMissEm
left a comment
ThisIsMissEm
left a comment
There was a problem hiding this comment.
Left some comments that may be worth addressing before merge.
|
|
||
| // Reconstruct callback URL with code/state from IdP redirect. | ||
| // authorizationCodeGrant needs: base = registered redirect_uri, query = code+state from IdP. | ||
| const currentUrl = new URL(`${deps.ConfigService.apiUrl}/api/v1/oidc/login/callback`); |
There was a problem hiding this comment.
wouldn't new URL(req.url, deps.ConfigService.apiUrl) work here instead of manually copying the search parameters? Or, perhaps better: req.originalUrl
| if (!user || user.orgId !== orgId) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('[OIDC] User not found or org mismatch', { email, orgId, userOrgId: user?.orgId }); | ||
| return res.redirect(`${deps.ConfigService.uiUrl}/login/sso?error=user_not_found`); |
There was a problem hiding this comment.
no email and user_not_found should probably both be the same error as to try to stop credential guessing through oauth
There was a problem hiding this comment.
Use sso_login_failed for both of these.
| if (!user.loginMethods.includes('oidc')) { | ||
| user.loginMethods = [...user.loginMethods, 'oidc']; | ||
| await user.save(); | ||
| } |
There was a problem hiding this comment.
This is probably an unsafe upgrade to OIDC for authentication, and should only be done if the user is completing a flow for connecting their local account with an OIDC account; otherwise, it should not be done, as this would allow an attacker gaining control over the OIDC server to then access another internal tool as long as they know someone's email that likely has an account on said tool.
| } | ||
|
|
||
| const callbackUrl = deps.SSOService.getSSOOidcCallbackUrl(); | ||
| const issuerUrl = normalizeIssuerUrl(oidcSettings.issuer_url); |
There was a problem hiding this comment.
normalize on storage, not on read
| const state = oidcClient.randomState(); | ||
| redirectTo.searchParams.set('state', state); |
There was a problem hiding this comment.
you can move the state generation up to along with codeVerifier and challenge, and then just pass in params as a URLSearchParams({ ... }) with a state key/property. buildAuthorizationUrl should automatically include the state then in the authorization redirect URL
| redirectTo.searchParams.set('state', state); | ||
|
|
||
| // Store PKCE data + orgId in session for callback | ||
| req.session.oidc = { code_verifier: codeVerifier, state, org_id: orgId }; |
There was a problem hiding this comment.
double check that this is stored securely, and not in a plaintext cookie or something.
3 participants
Context & Requests for Reviewers
fixes #335
Currently, Coop only supports SAML for Single Sign-On (SSO). This PR adds OpenID Connect (OIDC) support to enable modern authentication flows and provide better compatibility with identity providers that support OIDC.
This PR is using openid-client library
TODO
client_secret_basicorclient_secret_postTests
Tested locally using Authelia
will add screenshots/video later
(Optional) Rollout Plan