diff --git a/plugins/auth-backend/migrations/20250701120000_oidc_client_registration.js b/plugins/auth-backend/migrations/20250909120000_oidc_client_registration.js similarity index 100% rename from plugins/auth-backend/migrations/20250701120000_oidc_client_registration.js rename to plugins/auth-backend/migrations/20250909120000_oidc_client_registration.js diff --git a/plugins/auth-backend/src/database/OidcDatabase.ts b/plugins/auth-backend/src/database/OidcDatabase.ts index ec9a879803..ebb6619400 100644 --- a/plugins/auth-backend/src/database/OidcDatabase.ts +++ b/plugins/auth-backend/src/database/OidcDatabase.ts @@ -202,14 +202,24 @@ export class OidcDatabase { }); } - const [updated] = await this.db( + const returnedRows = await this.db( 'oauth_authorization_sessions', ) .where('id', session.id) .update(updatedFields) .returning('*'); - return this.rowToAuthorizationSession(updated) as AuthorizationSession; + if (returnedRows.length !== 1) { + throw new Error( + `Failed to retrieve updated authorization session with id ${session.id}`, + ); + } + + const [returnedSession] = returnedRows; + + return this.rowToAuthorizationSession( + returnedSession, + ) as AuthorizationSession; } async getAuthorizationSession({ id }: { id: string }) { @@ -290,17 +300,25 @@ export class OidcDatabase { }); } - const [updated] = await this.db( + const returnedRows = await this.db( 'oidc_authorization_codes', ) .where('code', authorizationCode.code) .update(updatedFields) .returning('*'); - return this.rowToAuthorizationCode(updated) as AuthorizationCode; + if (returnedRows.length !== 1) { + throw new Error( + `Failed to retrieve updated authorization code with code ${authorizationCode.code}`, + ); + } + + const [returnedCode] = returnedRows; + + return this.rowToAuthorizationCode(returnedCode) as AuthorizationCode; } - private rowToClient(row: Partial): Partial { + private rowToClient(row: OidcClientRow): Client { return { clientId: row.client_id, clientName: row.client_name, @@ -332,12 +350,12 @@ export class OidcDatabase { code_challenge_method: session.codeChallengeMethod, nonce: session.nonce, status: session.status, - expires_at: session.expiresAt, + expires_at: toDate(session.expiresAt), }; } private rowToAuthorizationSession( - row: Partial, + row: OAuthAuthorizationSessionRow, ): Partial { return { id: row.id, @@ -361,13 +379,13 @@ export class OidcDatabase { return { code: authorizationCode.code, session_id: authorizationCode.sessionId, - expires_at: authorizationCode.expiresAt, + expires_at: toDate(authorizationCode.expiresAt), used: authorizationCode.used, }; } private rowToAuthorizationCode( - row: Partial, + row: OidcAuthorizationCodeRow, ): Partial { return { code: row.code, diff --git a/plugins/auth-backend/src/migrations.test.ts b/plugins/auth-backend/src/migrations.test.ts index f9575c70fd..7cb6f982ea 100644 --- a/plugins/auth-backend/src/migrations.test.ts +++ b/plugins/auth-backend/src/migrations.test.ts @@ -186,4 +186,144 @@ describe('migrations', () => { await knex.destroy(); }, ); + + it.each(databases.eachSupportedId())( + '20250909120000_oidc_client_registration.js, %p', + async databaseId => { + const knex = await databases.init(databaseId); + + await migrateUntilBefore( + knex, + '20250909120000_oidc_client_registration.js', + ); + await migrateUpOnce(knex); + + await knex + .insert({ + client_id: 'test-client-id', + client_secret: 'test-client-secret', + client_name: 'Test Client', + response_types: JSON.stringify(['code']), + grant_types: JSON.stringify(['authorization_code']), + redirect_uris: JSON.stringify(['https://example.com/callback']), + scope: 'openid profile', + metadata: JSON.stringify({ description: 'Test client' }), + }) + .into('oidc_clients'); + + await expect( + knex('oidc_clients').where('client_id', 'test-client-id').first(), + ).resolves.toEqual({ + client_id: 'test-client-id', + client_secret: 'test-client-secret', + client_name: 'Test Client', + response_types: JSON.stringify(['code']), + grant_types: JSON.stringify(['authorization_code']), + redirect_uris: JSON.stringify(['https://example.com/callback']), + scope: 'openid profile', + metadata: JSON.stringify({ description: 'Test client' }), + }); + + await knex + .insert({ + id: 'test-session-id', + client_id: 'test-client-id', + user_entity_ref: 'user:default/test-user', + redirect_uri: 'https://example.com/callback', + scope: 'openid', + state: 'test-state', + response_type: 'code', + code_challenge: 'test-challenge', + code_challenge_method: 'S256', + nonce: 'test-nonce', + status: 'pending', + expires_at: new Date(Date.now() + 3600000), + }) + .into('oauth_authorization_sessions'); + + await expect( + knex('oauth_authorization_sessions') + .where('id', 'test-session-id') + .first(), + ).resolves.toEqual( + expect.objectContaining({ + id: 'test-session-id', + client_id: 'test-client-id', + user_entity_ref: 'user:default/test-user', + redirect_uri: 'https://example.com/callback', + scope: 'openid', + state: 'test-state', + response_type: 'code', + code_challenge: 'test-challenge', + code_challenge_method: 'S256', + nonce: 'test-nonce', + status: 'pending', + }), + ); + + await knex + .insert({ + code: 'test-auth-code', + session_id: 'test-session-id', + expires_at: new Date(Date.now() + 600000), + used: false, + }) + .into('oidc_authorization_codes'); + + await expect( + knex('oidc_authorization_codes') + .where('code', 'test-auth-code') + .first(), + ).resolves.toEqual( + expect.objectContaining({ + code: 'test-auth-code', + session_id: 'test-session-id', + }), + ); + + await expect( + knex + .insert({ + id: 'invalid-session', + client_id: 'non-existent-client', + redirect_uri: 'https://example.com/callback', + response_type: 'code', + expires_at: new Date(), + }) + .into('oauth_authorization_sessions'), + ).rejects.toThrow(); + + await expect( + knex + .insert({ + code: 'invalid-code', + session_id: 'non-existent-session', + expires_at: new Date(), + }) + .into('oidc_authorization_codes'), + ).rejects.toThrow(); + + await knex('oauth_authorization_sessions') + .where('id', 'test-session-id') + .del(); + + await expect( + knex('oidc_authorization_codes').where('session_id', 'test-session-id'), + ).resolves.toHaveLength(0); + + await migrateDownOnce(knex); + + const tables = [ + 'oidc_clients', + 'oauth_authorization_sessions', + 'oidc_authorization_codes', + ]; + + for (const table of tables) { + await expect(knex.schema.hasTable(table)).resolves.toBe(false); + } + + await knex.destroy(); + }, + ); }); diff --git a/plugins/auth-backend/src/service/OidcRouter.test.ts b/plugins/auth-backend/src/service/OidcRouter.test.ts index 81430ef05e..de865a9c43 100644 --- a/plugins/auth-backend/src/service/OidcRouter.test.ts +++ b/plugins/auth-backend/src/service/OidcRouter.test.ts @@ -24,6 +24,7 @@ import { startTestBackend, TestDatabases, TestDatabaseId, + mockCredentials, } from '@backstage/backend-test-utils'; import request from 'supertest'; import crypto from 'crypto'; @@ -413,13 +414,9 @@ describe('OidcRouter', () => { ], }); - httpAuth.credentials.mockResolvedValueOnce({ - principal: { - type: 'user', - userEntityRef: 'user:default/test-user', - }, - $$type: '@backstage/BackstageCredentials', - }); + httpAuth.credentials.mockResolvedValueOnce( + mockCredentials.user('user:default/test-user'), + ); auth.isPrincipal.mockReturnValueOnce(true); @@ -437,7 +434,7 @@ describe('OidcRouter', () => { it('should reject auth session', async () => { const { - mocks: { service }, + mocks: { service, httpAuth, auth }, router, } = await createRouter(databaseId); @@ -457,6 +454,12 @@ describe('OidcRouter', () => { state: 'test-state', }); + httpAuth.credentials.mockResolvedValueOnce( + mockCredentials.user('user:default/test-user'), + ); + + auth.isPrincipal.mockReturnValueOnce(true); + const { server } = await startTestBackend({ features: [ createBackendPlugin({ @@ -496,13 +499,9 @@ describe('OidcRouter', () => { router, } = await createRouter(databaseId); - httpAuth.credentials.mockResolvedValueOnce({ - principal: { - type: 'user', - userEntityRef: 'user:default/test-user', - }, - $$type: '@backstage/BackstageCredentials', - }); + httpAuth.credentials.mockResolvedValueOnce( + mockCredentials.user('user:default/test-user'), + ); auth.isPrincipal.mockReturnValueOnce(true); @@ -590,13 +589,9 @@ describe('OidcRouter', () => { token: 'mock-access-token-pkce', }); - httpAuth.credentials.mockResolvedValueOnce({ - principal: { - type: 'user', - userEntityRef: 'user:default/test-user-pkce', - }, - $$type: '@backstage/BackstageCredentials', - }); + httpAuth.credentials.mockResolvedValueOnce( + mockCredentials.user('user:default/test-user-pkce'), + ); auth.isPrincipal.mockReturnValueOnce(true); @@ -725,13 +720,9 @@ describe('OidcRouter', () => { token: 'mock-access-token-s256', }); - httpAuth.credentials.mockResolvedValueOnce({ - principal: { - type: 'user', - userEntityRef: 'user:default/test-user-s256', - }, - $$type: '@backstage/BackstageCredentials', - }); + httpAuth.credentials.mockResolvedValueOnce( + mockCredentials.user('user:default/test-user-s256'), + ); auth.isPrincipal.mockReturnValueOnce(true); diff --git a/plugins/auth-backend/src/service/OidcRouter.ts b/plugins/auth-backend/src/service/OidcRouter.ts index f20b5f4ede..9a3308b4eb 100644 --- a/plugins/auth-backend/src/service/OidcRouter.ts +++ b/plugins/auth-backend/src/service/OidcRouter.ts @@ -134,19 +134,19 @@ export class OidcRouter { clientId: clientId as string, redirectUri: redirectUri as string, responseType: responseType as string, - scope: scope as string, - state: state as string, - nonce: nonce as string, - codeChallenge: codeChallenge as string, - codeChallengeMethod: codeChallengeMethod as string, + scope: scope as string | undefined, + state: state as string | undefined, + nonce: nonce as string | undefined, + codeChallenge: codeChallenge as string | undefined, + codeChallengeMethod: codeChallengeMethod as string | undefined, }); // todo(blam): maybe this URL could be overridable by config if // the plugin is mounted somewhere else? // support slashes in baseUrl? const authSessionRedirectUrl = new URL( - `/oauth2/authorize/${result.id}`, - this.appUrl, + `./oauth2/authorize/${result.id}`, + ensureTrailingSlash(this.appUrl), ); return res.redirect(authSessionRedirectUrl.toString()); @@ -171,7 +171,7 @@ export class OidcRouter { }); // Authorization Session request details endpoint - // Returns Authorization Session request details for the frontned + // Returns Authorization Session request details for the frontend router.get('/v1/sessions/:sessionId', async (req, res) => { const { sessionId } = req.params; @@ -263,12 +263,25 @@ export class OidcRouter { }); } + const httpCredentials = await this.httpAuth.credentials(req); + + if (!this.auth.isPrincipal(httpCredentials, 'user')) { + return res.status(401).json({ + error: 'unauthorized', + error_description: 'Authentication required', + }); + } + + const { userEntityRef } = httpCredentials.principal; try { const session = await this.oidc.getAuthorizationSession({ sessionId, }); - await this.oidc.rejectAuthorizationSession({ sessionId }); + await this.oidc.rejectAuthorizationSession({ + sessionId, + userEntityRef, + }); const errorParams = new URLSearchParams(); errorParams.append('error', 'access_denied'); @@ -413,3 +426,9 @@ export class OidcRouter { return router; } } +function ensureTrailingSlash(appUrl: string): string | URL | undefined { + if (appUrl.endsWith('/')) { + return appUrl; + } + return `${appUrl}/`; +} diff --git a/plugins/auth-backend/src/service/OidcService.test.ts b/plugins/auth-backend/src/service/OidcService.test.ts index 7829be3841..328a753e4c 100644 --- a/plugins/auth-backend/src/service/OidcService.test.ts +++ b/plugins/auth-backend/src/service/OidcService.test.ts @@ -466,6 +466,7 @@ describe('OidcService', () => { await service.rejectAuthorizationSession({ sessionId: authSession.id, + userEntityRef: 'user:default/test', }); await expect( @@ -553,6 +554,7 @@ describe('OidcService', () => { await service.rejectAuthorizationSession({ sessionId: authSession.id, + userEntityRef: 'user:default/test', }); await expect( @@ -580,6 +582,7 @@ describe('OidcService', () => { await service.rejectAuthorizationSession({ sessionId: authSession.id, + userEntityRef: 'user:default/test', }); await expect( @@ -595,6 +598,7 @@ describe('OidcService', () => { await expect( service.rejectAuthorizationSession({ sessionId: 'invalid-session', + userEntityRef: 'user:default/test', }), ).rejects.toThrow('Invalid authorization session'); }); @@ -621,6 +625,7 @@ describe('OidcService', () => { await expect( service.rejectAuthorizationSession({ sessionId: authSession.id, + userEntityRef: 'user:default/test', }), ).rejects.toThrow('Authorization session not found or expired'); }); @@ -641,49 +646,15 @@ describe('OidcService', () => { await service.rejectAuthorizationSession({ sessionId: authSession.id, + userEntityRef: 'user:default/test', }); await expect( service.rejectAuthorizationSession({ sessionId: authSession.id, - }), - ).rejects.toThrow('Authorization session not found or expired'); - }); - }); - - describe('authorize', () => { - it('should create direct authorization', async () => { - const { service } = await createOidcService(databaseId); - - const client = await service.registerClient({ - clientName: 'Test Client', - redirectUris: ['https://example.com/callback'], - }); - - const result = await service.authorize({ - clientId: client.clientId, - redirectUri: 'https://example.com/callback', - responseType: 'code', - userEntityRef: 'user:default/test', - state: 'test-state', - }); - - expect(result.redirectUrl).toMatch( - /^https:\/\/example\.com\/callback\?code=.+&state=test-state$/, - ); - }); - - it('should throw error for invalid client', async () => { - const { service } = await createOidcService(databaseId); - - await expect( - service.authorize({ - clientId: 'invalid-client', - redirectUri: 'https://example.com/callback', - responseType: 'code', userEntityRef: 'user:default/test', }), - ).rejects.toThrow('Invalid client_id'); + ).rejects.toThrow('Authorization session not found or expired'); }); }); @@ -698,14 +669,18 @@ describe('OidcService', () => { redirectUris: ['https://example.com/callback'], }); - const authResult = await service.authorize({ + const authSession = await service.createAuthorizationSession({ clientId: client.clientId, redirectUri: 'https://example.com/callback', responseType: 'code', - userEntityRef: 'user:default/test', scope: 'openid', }); + const authResult = await service.approveAuthorizationSession({ + sessionId: authSession.id, + userEntityRef: 'user:default/test', + }); + const code = new URL(authResult.redirectUrl).searchParams.get('code')!; const tokenResult = await service.exchangeCodeForToken({ @@ -751,15 +726,19 @@ describe('OidcService', () => { .update(codeVerifier) .digest('base64url'); - const authResult = await service.authorize({ + const authSession = await service.createAuthorizationSession({ clientId: client.clientId, redirectUri: 'https://example.com/callback', responseType: 'code', - userEntityRef: 'user:default/test', codeChallenge, codeChallengeMethod: 'S256', }); + const authResult = await service.approveAuthorizationSession({ + sessionId: authSession.id, + userEntityRef: 'user:default/test', + }); + const code = new URL(authResult.redirectUrl).searchParams.get('code')!; const tokenResult = await service.exchangeCodeForToken({ @@ -781,15 +760,19 @@ describe('OidcService', () => { }); const codeChallenge = 'test-challenge'; - const authResult = await service.authorize({ + const authSession = await service.createAuthorizationSession({ clientId: client.clientId, redirectUri: 'https://example.com/callback', responseType: 'code', - userEntityRef: 'user:default/test', codeChallenge, codeChallengeMethod: 'S256', }); + const authResult = await service.approveAuthorizationSession({ + sessionId: authSession.id, + userEntityRef: 'user:default/test', + }); + const code = new URL(authResult.redirectUrl).searchParams.get('code')!; await expect( diff --git a/plugins/auth-backend/src/service/OidcService.ts b/plugins/auth-backend/src/service/OidcService.ts index 3139ccc73d..e8a308148a 100644 --- a/plugins/auth-backend/src/service/OidcService.ts +++ b/plugins/auth-backend/src/service/OidcService.ts @@ -300,9 +300,14 @@ export class OidcService { }; } - public async rejectAuthorizationSession(opts: { sessionId: string }) { + public async rejectAuthorizationSession(opts: { + sessionId: string; + userEntityRef: string; + }) { + const { sessionId, userEntityRef } = opts; + const session = await this.oidc.getAuthorizationSession({ - id: opts.sessionId, + id: sessionId, }); if (!session) { @@ -320,94 +325,8 @@ export class OidcService { await this.oidc.updateAuthorizationSession({ id: session.id, status: 'rejected', - }); - } - - public async authorize(opts: { - clientId: string; - redirectUri: string; - responseType: string; - scope?: string; - state?: string; - nonce?: string; - codeChallenge?: string; - codeChallengeMethod?: string; - userEntityRef: string; - }) { - const { - clientId, - redirectUri, - responseType, - scope, - state, - nonce, - codeChallenge, - codeChallengeMethod, userEntityRef, - } = opts; - - if (responseType !== 'code') { - throw new InputError('Only authorization code flow is supported'); - } - - const client = await this.oidc.getClient({ clientId }); - if (!client) { - throw new InputError('Invalid client_id'); - } - - if (!client.redirectUris.includes(redirectUri)) { - throw new InputError('Invalid redirect_uri'); - } - - if (codeChallenge) { - if ( - !codeChallengeMethod || - !['S256', 'plain'].includes(codeChallengeMethod) - ) { - throw new InputError('Invalid code_challenge_method'); - } - } - - const sessionId = crypto.randomUUID(); - const sessionExpiresAt = DateTime.now().plus({ hours: 1 }).toJSDate(); - - await this.oidc.createAuthorizationSession({ - id: sessionId, - clientId, - userEntityRef, - redirectUri, - responseType, - scope, - state, - codeChallenge, - codeChallengeMethod, - nonce, - expiresAt: sessionExpiresAt, }); - - await this.oidc.updateAuthorizationSession({ - id: sessionId, - status: 'approved', - }); - - const authorizationCode = crypto.randomBytes(32).toString('base64url'); - const codeExpiresAt = DateTime.now().plus({ minutes: 10 }).toJSDate(); - - await this.oidc.createAuthorizationCode({ - code: authorizationCode, - sessionId, - expiresAt: codeExpiresAt, - }); - - const redirectUrl = new URL(redirectUri); - redirectUrl.searchParams.append('code', authorizationCode); - if (state) { - redirectUrl.searchParams.append('state', state); - } - - return { - redirectUrl: redirectUrl.toString(), - }; } public async exchangeCodeForToken(params: {