From 07dcd843791cf6dd8e8560488edf3c4ac678dcdb Mon Sep 17 00:00:00 2001
From: Ruben Vallejo <rvallejo@vmware.com>
Date: Mon, 21 Aug 2023 12:26:47 -0400
Subject: [PATCH] Add redirect and error methods to the handlers strategy along
 with unit tests

Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
---
 .../src/providers/pinniped/provider.test.ts   | 15 ++++++
 .../src/providers/pinniped/provider.ts        | 47 ++++++++++++++-----
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/plugins/auth-backend/src/providers/pinniped/provider.test.ts b/plugins/auth-backend/src/providers/pinniped/provider.test.ts
index 46e5bfe630..5299d4e169 100644
--- a/plugins/auth-backend/src/providers/pinniped/provider.test.ts
+++ b/plugins/auth-backend/src/providers/pinniped/provider.test.ts
@@ -222,6 +222,7 @@ describe('PinnipedAuthProvider', () => {
         origin: 'undefined',
       });
 
+      //we want to somehow pass an authentication header in this request for testing purposes
       handlerRequest = {
         method: 'GET',
         url: `https://test?code=authorization_code&state=${testState}&scope=pinniped:request-audience username`,
@@ -294,6 +295,20 @@ describe('PinnipedAuthProvider', () => {
       expect(audience).toEqual('pinniped:request-audience username')
     })
 
+    it('request errors out with missing authorization_code parameter in the request_url', async() => {
+      handlerRequest.url = "test"
+      return expect(provider.handler(handlerRequest)).rejects.toThrow('Unexpected redirect')
+    })
+
+    it('fails when request has no session', async () => {
+      return expect(
+        provider.handler({
+          method: 'GET',
+          url: 'test',
+        } as unknown as OAuthStartRequest),
+      ).rejects.toThrow('authentication requires session support');
+    });
+
     //if no valid key is in the jwks array or even an unsigned jwt
     //have pinniped reject your clientid and secret possibly as a unit test
   });
diff --git a/plugins/auth-backend/src/providers/pinniped/provider.ts b/plugins/auth-backend/src/providers/pinniped/provider.ts
index 3a347db681..5ee92ec6d0 100644
--- a/plugins/auth-backend/src/providers/pinniped/provider.ts
+++ b/plugins/auth-backend/src/providers/pinniped/provider.ts
@@ -59,8 +59,17 @@ export class PinnipedAuthProvider implements OAuthHandlers {
 
   async start(req: OAuthStartRequest): Promise<OAuthStartResponse> {
     const { strategy } = await this.implementation;
+
+    // we are practicing with this scope and it works openid pinniped:request-audience username
+    // whats the bare minimum needed for our request to fail?
+
+    // http://127.0.0.1:7007/api/auth/pinniped/start?env=development&audience=host-cluster
+    // having scope seperated by + results in an error
+
+    // `{"type":"authorization_response","error":{"name":"OPError","message":"invalid_scope (The requested scope is invalid, unknown, or malformed. The OAuth 2.0 Client is not allowed to request scope 'openid+pinniped:request-audience+username'.)"}}`
+
     const options: Record<string, string> = {
-      scope: req.scope || 'pinniped:request-audience username',
+      scope: req.scope || 'openid+pinniped:request-audience+username',
       state: encodeState(req.state),
     };
     return new Promise((resolve, reject) => {
@@ -79,30 +88,42 @@ export class PinnipedAuthProvider implements OAuthHandlers {
   ): Promise<{ response: OAuthResponse; refreshToken?: string }> {
     const { strategy } = await this.implementation;
 
-    //the query string inside the req should contain a code and a state, we can change the stub to reject any auth code, can this query string also include scope??
+    // the query string inside the req should contain a code and a state, we can change the stub to reject any auth code,
+    // can this query string also include scope? It already does get a scope in its redirect url when start is called by default.
+    // but when the fake supervisor hits the handler a scope must be returned by the oauth2/authorize endpoint for it to be present in the req
 
-    const { searchParams } = new URL(req.url, 'https://pinniped.com')
-    const audience = searchParams.get('scope') ?? "none"
+    const { searchParams } = new URL(req.url, 'https://pinniped.com');
+    const audience = searchParams.get('scope') ?? 'none';
 
     return new Promise((resolve, reject) => {
       strategy.success = user => {
-        resolve({ response: {
-          providerInfo: {accessToken: user.tokenset.access_token, scope: audience},
-          profile: {},
-        }})
-      }
+        resolve({
+          response: {
+            providerInfo: {
+              accessToken: user.tokenset.access_token,
+              scope: audience,
+            },
+            profile: {},
+          },
+        });
+      };
       strategy.fail = info => {
         reject(new Error(`Authentication rejected, ${info.message || ''}`));
       };
 
-      //TODO: unit test for provider to state the need for this error handler
-      // strategy.error = reject;
+      strategy.error = (error: Error) => {
+        reject(error);
+      };
+
+      strategy.redirect = () => {
+        reject(new Error('Unexpected redirect'));
+      };
 
       strategy.authenticate(req);
     });
   }
 
-  //will need a refresh method that covers our happy path
+  // will need a refresh method that covers our happy path
 
   private async setupStrategy(options: PinnipedOptions): Promise<OidcImpl> {
     const issuer = await Issuer.discover(
@@ -114,7 +135,7 @@ export class PinnipedAuthProvider implements OAuthHandlers {
       client_secret: options.clientSecret,
       redirect_uris: [options.callbackUrl],
       response_types: ['code'],
-      id_token_signed_response_alg: options.tokenSignedResponseAlg || 'RS256',
+      id_token_signed_response_alg: options.tokenSignedResponseAlg || 'ES256',
       scope: options.scope || '',
     });