diff --git a/plugins/auth-backend/src/providers/pinniped/provider.test.ts b/plugins/auth-backend/src/providers/pinniped/provider.test.ts index 4b44bcfb6c..9c2d3d7d63 100644 --- a/plugins/auth-backend/src/providers/pinniped/provider.test.ts +++ b/plugins/auth-backend/src/providers/pinniped/provider.test.ts @@ -14,12 +14,13 @@ * limitations under the License. */ import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; -import { OAuthStartRequest, encodeState } from '../../lib/oauth'; +import { OAuthStartRequest, encodeState, readState } from '../../lib/oauth'; import { PinnipedAuthProvider, PinnipedOptions } from './provider'; import { setupServer } from 'msw/node'; import { rest } from 'msw'; import express from 'express'; import { UnsecuredJWT } from 'jose'; +import { OAuthState } from '../../lib/oauth'; describe('PinnipedAuthProvider', () => { let provider: PinnipedAuthProvider; @@ -75,6 +76,10 @@ describe('PinnipedAuthProvider', () => { .setIssuedAt(iat) .setExpirationTime(exp) .encode(); + const oauthState: OAuthState = { + nonce: 'nonce', + env: 'env', + }; beforeEach(() => { jest.clearAllMocks(); @@ -96,6 +101,7 @@ describe('PinnipedAuthProvider', () => { session: fakeSession, method: 'GET', url: 'test', + state: oauthState, } as unknown as OAuthStartRequest; const handler = jest.fn((_req, res, ctx) => { return res( @@ -114,7 +120,7 @@ describe('PinnipedAuthProvider', () => { }); describe('#start', () => { - it('redirects to authorization endpoint returned from federationDomain config value', async () => { + it('redirects to authorization endpoint returned from OIDC metadata endpoint', async () => { const startResponse = await provider.start(startRequest); const url = new URL(startResponse.url); @@ -123,6 +129,13 @@ describe('PinnipedAuthProvider', () => { expect(url.pathname).toBe('/oauth2/authorize'); }); + it('initiates an authorization code grant', async () => { + const startResponse = await provider.start(startRequest); + const { searchParams } = new URL(startResponse.url); + + expect(searchParams.get('response_type')).toBe('code'); + }); + it('passes client ID from config', async () => { const startResponse = await provider.start(startRequest); const { searchParams } = new URL(startResponse.url); @@ -152,6 +165,16 @@ describe('PinnipedAuthProvider', () => { expect(fakeSession['oidc:pinniped.test'].code_verifier).toBeDefined(); }); + it('requests sufficient scopes for token exchange', async () => { + const startResponse = await provider.start(startRequest); + const { searchParams } = new URL(startResponse.url); + const scopes = searchParams.get('scope')?.split(' ') ?? []; + + expect(scopes).toEqual( + expect.arrayContaining(['pinniped:request-audience', 'username']), + ); + }); + it('fails when request has no session', async () => { return expect( provider.start({ @@ -161,20 +184,13 @@ describe('PinnipedAuthProvider', () => { ).rejects.toThrow('authentication requires session support'); }); - // false passing test: passes because we compare two falsy values undefined and undefined - // need to add the logic that makes this true - it.skip('adds session ID handle to state param', async () => { + it('encodes OAuth state in query param', async () => { const startResponse = await provider.start(startRequest); - // stateParam is empty string - const stateParam = new URL(startResponse.url).searchParams.get('state'); - const state = Object.fromEntries( - new URLSearchParams(Buffer.from(stateParam!, 'hex').toString('utf-8')), - ); - // handle is currently undefined - const { handle } = fakeSession['oidc:pinniped.test'].state; - console.log(`This is the param:`, stateParam); - // state.handle = undefined - expect(state.handle ?? '').toEqual(handle); + const { searchParams } = new URL(startResponse.url); + const stateParam = searchParams.get('state'); + const decodedState = readState(stateParam!); + + expect(decodedState).toMatchObject(oauthState); }); }); diff --git a/plugins/auth-backend/src/providers/pinniped/provider.ts b/plugins/auth-backend/src/providers/pinniped/provider.ts index 8f27072001..9eafb8bde0 100644 --- a/plugins/auth-backend/src/providers/pinniped/provider.ts +++ b/plugins/auth-backend/src/providers/pinniped/provider.ts @@ -31,7 +31,6 @@ import { OAuthStartResponse } from '../types'; import express from 'express'; import { OAuthAdapter, OAuthEnvironmentHandler } from '../../lib/oauth'; import { createAuthProviderIntegration } from '../createAuthProviderIntegration'; -import { InternalOAuthError } from 'passport-oauth2'; type OidcImpl = { strategy: OidcStrategy; @@ -61,7 +60,7 @@ export class PinnipedAuthProvider implements OAuthHandlers { async start(req: OAuthStartRequest): Promise { const { strategy } = await this.implementation; const options: Record = { - scope: req.scope || 'openid profile email', + scope: req.scope || 'pinniped:request-audience username', state: encodeState(req.state), }; return new Promise((resolve, reject) => { @@ -79,7 +78,7 @@ export class PinnipedAuthProvider implements OAuthHandlers { req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken?: string }> { const { strategy } = await this.implementation; - return new Promise((resolve, reject) => { + return new Promise((_, reject) => { strategy.fail = info => { reject(new Error(`Authentication rejected, ${info.message || ''}`)); }; @@ -111,13 +110,7 @@ export class PinnipedAuthProvider implements OAuthHandlers { tokenset: TokenSet, done: PassportDoneCallback<{ tokenset: TokenSet }, PrivateInfo>, ) => { - done( - undefined, - { tokenset }, - { - refreshToken: tokenset.refresh_token, - }, - ); + done(undefined, { tokenset }, {}); }, ); return { strategy, client };