refine requirements for start method

Now the unit tests for the start method should render the '#start' describe
in index.test.ts redundant.

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
Co-authored-by: Ruben Vallejo <rvallejo@vmware.com>

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);
     });
   });
 

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<undefined, Client>;
@@ -61,7 +60,7 @@ export class PinnipedAuthProvider implements OAuthHandlers {
   async start(req: OAuthStartRequest): Promise<OAuthStartResponse> {
     const { strategy } = await this.implementation;
     const options: Record<string, string> = {
-      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 };