-
-
Notifications
You must be signed in to change notification settings - Fork 30
feat: enhance OIDC configuration handling and discovery process #1848
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
Open
paustint
wants to merge
1
commit into
main
Choose a base branch
from
feat/improve-oidc-form-setup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+268
−110
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
113 changes: 113 additions & 0 deletions
113
apps/jetstream-e2e/src/tests/authentication/team/sso/sso-oidc-config-save.api.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { prisma } from '@jetstream/api-config'; | ||
| import { HTTP } from '@jetstream/shared/constants'; | ||
| import { randomUUID } from 'crypto'; | ||
| import { createServer, Server } from 'http'; | ||
| import { expect, test } from '../../../../fixtures/fixtures'; | ||
|
|
||
| // Fresh signup per test (mirrors team.spec.ts) so the fixture's admin owns the page session. | ||
| test.use({ storageState: { cookies: [], origins: [] } }); | ||
|
|
||
| // Distinct port from sso-oidc-happy.api.spec.ts (5555) so both mock IdPs can run in parallel workers. | ||
| const ISSUER = 'http://127.0.0.1:5556'; | ||
|
|
||
| /** | ||
| * Minimal mock IdP that serves only the OIDC discovery document. The config-save flow resolves and | ||
| * snapshots the endpoints from discovery; it does not call token/jwks/userinfo at save time. | ||
| */ | ||
| function startMockOidcServer(): Server { | ||
| const server = createServer((req, res) => { | ||
| if (req.url?.startsWith('/.well-known/openid-configuration')) { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end( | ||
| JSON.stringify({ | ||
| issuer: ISSUER, | ||
| authorization_endpoint: `${ISSUER}/auth`, | ||
| token_endpoint: `${ISSUER}/token`, | ||
| userinfo_endpoint: `${ISSUER}/userinfo`, | ||
| jwks_uri: `${ISSUER}/jwks`, | ||
| }), | ||
| ); | ||
| return; | ||
| } | ||
| res.writeHead(404); | ||
| res.end(); | ||
| }); | ||
| server.listen(5556); | ||
| return server; | ||
| } | ||
|
|
||
| test.describe('OIDC SSO configuration save', () => { | ||
| let mockServer: Server; | ||
| let createdLoginConfigId: string | undefined; | ||
|
|
||
| test.beforeAll(() => { | ||
| mockServer = startMockOidcServer(); | ||
| }); | ||
|
|
||
| test.afterEach(async () => { | ||
| // The OIDC config hangs off the team's login config, which the fixture's team-only cleanup does | ||
| // not delete, so remove it explicitly to avoid leaking rows between runs. | ||
| if (createdLoginConfigId) { | ||
| await prisma.oidcConfiguration.deleteMany({ where: { loginConfigId: createdLoginConfigId } }).catch(() => undefined); | ||
| createdLoginConfigId = undefined; | ||
| } | ||
| }); | ||
|
|
||
| test.afterAll(() => { | ||
| mockServer?.close(); | ||
| }); | ||
|
|
||
| test('derives OIDC endpoints from the issuer when the client omits them', async ({ page, teamCreationUtils1User: teamCreationUtils }) => { | ||
| const { team } = teamCreationUtils; | ||
| const loginConfigId = team.loginConfig.id; | ||
| createdLoginConfigId = loginConfigId; | ||
|
|
||
| await test.step('Seed a verified domain (configuring SSO requires one)', async () => { | ||
| await prisma.domainVerification.create({ | ||
| data: { | ||
| teamId: team.id, | ||
| domain: `oidc-${randomUUID().slice(0, 8)}.test`, | ||
| status: 'VERIFIED', | ||
| verificationCode: `jetstream-verification=${randomUUID()}`, | ||
| verifiedAt: new Date(), | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| // The /api/teams routes enforce HMAC double-submit CSRF: echo the `jetstream-csrf` cookie set at | ||
| // login back in the X-Csrf-Token header (the app's axios interceptor does this in the real client). | ||
| // The /api/auth/csrf token is a separate legacy token and does NOT satisfy this middleware. | ||
| const cookies = await page.context().cookies(); | ||
| const csrfToken = cookies.find(({ name }) => name.endsWith(HTTP.COOKIE.CSRF_SUFFIX))?.value ?? ''; | ||
| expect(csrfToken).toBeTruthy(); | ||
|
|
||
| await test.step('Save OIDC config with only the issuer + client credentials (no endpoints)', async () => { | ||
| const response = await page.request.post(`/api/teams/${team.id}/sso/oidc/config`, { | ||
| headers: { [HTTP.HEADERS.X_CSRF_TOKEN]: csrfToken }, | ||
| data: { | ||
| name: 'Mock OIDC', | ||
| issuer: ISSUER, | ||
| clientId: 'client-id', | ||
| clientSecret: 'client-secret', | ||
| scopes: ['openid', 'email', 'profile'], | ||
| attributeMapping: { email: 'email', userName: 'email', firstName: 'given_name', lastName: 'family_name' }, | ||
| // Intentionally NO authorizationEndpoint / tokenEndpoint / jwksUri — this is the exact | ||
| // request shape that previously failed validation with a ZodError. | ||
| }, | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(200); | ||
| const body = await response.json(); | ||
| // Secret is encrypted at rest and never returned to the client. | ||
| expect(body?.data?.oidcConfiguration?.clientSecret).toBeNull(); | ||
| }); | ||
|
|
||
| await test.step('Server discovered and persisted the endpoints from the issuer', async () => { | ||
| const stored = await prisma.oidcConfiguration.findFirstOrThrow({ where: { loginConfigId } }); | ||
| expect(stored.authorizationEndpoint).toBe(`${ISSUER}/auth`); | ||
| expect(stored.tokenEndpoint).toBe(`${ISSUER}/token`); | ||
| expect(stored.jwksUri).toBe(`${ISSUER}/jwks`); | ||
| expect(stored.userinfoEndpoint).toBe(`${ISSUER}/userinfo`); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { OidcConfigurationRequestSchema } from '../auth-types'; | ||
|
|
||
| /** | ||
| * OIDC save-request schema. | ||
| * | ||
| * Regression guard for the "OIDC config save fails with a ZodError" bug: an admin providing only | ||
| * the issuer (relying on server-side discovery to resolve the endpoints) must pass validation. The | ||
| * IdP endpoints (authorizationEndpoint / tokenEndpoint / jwksUri / userinfoEndpoint / | ||
| * endSessionEndpoint) are resolved server-side, so they are optional/nullable on the wire. | ||
| */ | ||
| describe('OidcConfigurationRequestSchema', () => { | ||
| const baseRequest = { | ||
| name: 'JumpCloud SSO', | ||
| issuer: 'https://oauth.id.jumpcloud.com/', | ||
| clientId: 'client-abc', | ||
| clientSecret: 'super-secret', | ||
| attributeMapping: { email: 'email', userName: 'email', firstName: 'given_name', lastName: 'family_name' }, | ||
| }; | ||
|
|
||
| it('accepts a request with no endpoints (endpoints are resolved server-side from the issuer)', () => { | ||
| const result = OidcConfigurationRequestSchema.safeParse(baseRequest); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it('accepts a request with null endpoints (client sends null when auto-discover was not run)', () => { | ||
| const result = OidcConfigurationRequestSchema.safeParse({ | ||
| ...baseRequest, | ||
| authorizationEndpoint: null, | ||
| tokenEndpoint: null, | ||
| userinfoEndpoint: null, | ||
| jwksUri: null, | ||
| endSessionEndpoint: null, | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it('accepts a request with valid discovered endpoint URLs (the auto-discover preview path)', () => { | ||
| const result = OidcConfigurationRequestSchema.safeParse({ | ||
| ...baseRequest, | ||
| authorizationEndpoint: 'https://oauth.id.jumpcloud.com/oauth2/auth', | ||
| tokenEndpoint: 'https://oauth.id.jumpcloud.com/oauth2/token', | ||
| jwksUri: 'https://oauth.id.jumpcloud.com/.well-known/jwks.json', | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it('still rejects a non-URL endpoint when one is provided', () => { | ||
| const result = OidcConfigurationRequestSchema.safeParse({ ...baseRequest, authorizationEndpoint: 'not-a-url' }); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| it('still requires the core fields (issuer must be a valid URL)', () => { | ||
| const { issuer, ...withoutIssuer } = baseRequest; | ||
| expect(OidcConfigurationRequestSchema.safeParse(withoutIssuer).success).toBe(false); | ||
| expect(OidcConfigurationRequestSchema.safeParse({ ...baseRequest, issuer: 'not-a-url' }).success).toBe(false); | ||
| expect(OidcConfigurationRequestSchema.safeParse({ ...baseRequest, name: '' }).success).toBe(false); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.