fix(authentication): return errors details during authentication inst…

…ead of generic 500s (#857)

packages/agent/src/routes/security/authentication.ts

@@ -1,9 +1,8 @@
-import type { Client } from 'openid-client';
-
 import { ValidationError } from '@forestadmin/datasource-toolkit';
+import { AuthenticationError } from '@forestadmin/forestadmin-client';
 import Router from '@koa/router';
 import jsonwebtoken from 'jsonwebtoken';
-import { Context } from 'koa';
+import { Context, Next } from 'koa';
 import jwt from 'koa-jwt';
 
 import { RouteType } from '../../types';
@@ -12,15 +11,17 @@ import BaseRoute from '../base-route';
 export default class Authentication extends BaseRoute {
   readonly type = RouteType.Authentication;
 
-  private client: Client;
-
   override async bootstrap(): Promise<void> {
-    this.client = await this.options.forestAdminClient.getOpenIdClient();
+    await this.options.forestAdminClient.authService.init();
   }
 
   setupRoutes(router: Router): void {
     router.post('/authentication', this.handleAuthentication.bind(this));
-    router.get('/authentication/callback', this.handleAuthenticationCallback.bind(this));
+    router.get(
+      '/authentication/callback',
+      this.handleError.bind(this),
+      this.handleAuthenticationCallback.bind(this),
+    );
 
     router.use(jwt({ secret: this.options.authSecret, cookie: 'forest_session_token' }));
   }
@@ -31,10 +32,11 @@ export default class Authentication extends BaseRoute {
     const renderingId = Number(body?.renderingId);
     Authentication.checkRenderingId(renderingId);
 
-    const authorizationUrl = this.client.authorizationUrl({
-      scope: 'openid email profile',
-      state: JSON.stringify({ renderingId }),
-    });
+    const authorizationUrl =
+      await this.options.forestAdminClient.authService.generateAuthorizationUrl({
+        scope: 'openid email profile',
+        state: JSON.stringify({ renderingId }),
+      });
 
     context.response.body = { authorizationUrl };
   }
@@ -53,9 +55,15 @@ export default class Authentication extends BaseRoute {
     }
 
     // Retrieve user
-    const tokenSet = await this.client.callback(undefined, query, { state });
-    const accessToken = tokenSet.access_token;
-    const user = await this.options.forestAdminClient.getUserInfo(renderingId, accessToken);
+    const tokenSet = await this.options.forestAdminClient.authService.generateTokens({
+      query,
+      state,
+    });
+    const { accessToken } = tokenSet;
+    const user = await this.options.forestAdminClient.authService.getUserInfo(
+      renderingId,
+      accessToken,
+    );
 
     // Generate final token.
     const token = jsonwebtoken.sign(user, this.options.authSecret, { expiresIn: '1 hours' });
@@ -68,4 +76,23 @@ export default class Authentication extends BaseRoute {
       throw new ValidationError('Rendering id must be a number');
     }
   }
+
+  private async handleError(context: Context, next: Next): Promise<void> {
+    try {
+      await next();
+    } catch (e) {
+      if (e instanceof AuthenticationError) {
+        context.response.status = 401;
+        context.response.body = {
+          error: e.code,
+          error_description: e.description,
+          state: e.state,
+        };
+
+        return;
+      }
+
+      throw e;
+    }
+  }
 }

packages/agent/test/__factories__/forest-admin-client.ts

@@ -11,6 +11,12 @@ const forestAdminClientFactory = ForestAdminClientFactory.define(() => ({
   markScopesAsUpdated: jest.fn(),
   getIpWhitelistConfiguration: jest.fn(),
   postSchema: jest.fn(),
+  authService: {
+    init: jest.fn(),
+    getUserInfo: jest.fn(),
+    generateAuthorizationUrl: jest.fn(),
+    generateTokens: jest.fn(),
+  },
   permissionService: {
     canExecuteSegmentQuery: jest.fn(),
     canApproveCustomAction: jest.fn(),

packages/agent/test/routes/security/authentication.test.ts

@@ -1,5 +1,6 @@
+import { AuthenticationError } from '@forestadmin/forestadmin-client';
 import { createMockContext } from '@shopify/jest-koa-mocks';
-import { ClientAuthMethod, Issuer } from 'openid-client';
+import { errors } from 'openid-client';
 
 import Authentication from '../../../src/routes/security/authentication';
 import { AgentOptionsWithDefaults } from '../../../src/types';
@@ -22,115 +23,192 @@ describe('Authentication', () => {
     route.setupRoutes(router);
 
     expect(router.post).toHaveBeenCalledWith('/authentication', expect.any(Function));
-    expect(router.get).toHaveBeenCalledWith('/authentication/callback', expect.any(Function));
+    expect(router.get).toHaveBeenCalledWith(
+      '/authentication/callback',
+      expect.any(Function),
+      expect.any(Function),
+    );
     expect(router.use).toHaveBeenCalledWith(expect.any(Function));
   });
 
   test('bootstrap should retrieve the openId client', async () => {
-    options.forestAdminClient.getOpenIdClient = jest.fn().mockResolvedValue('value');
-    await route.bootstrap();
-
-    expect(options.forestAdminClient.getOpenIdClient).toHaveBeenCalled();
-  });
+    const init = jest.spyOn(options.forestAdminClient.authService, 'init').mockResolvedValue();
 
-  test('bootstrap should throw if the openid configuration cannot be fetched', async () => {
-    options.forestAdminClient.getOpenIdClient = jest
-      .fn()
-      .mockRejectedValue(new Error('Cannot fetch openid configuration'));
+    await route.bootstrap();
 
-    await expect(route.bootstrap()).rejects.toThrow('Cannot fetch openid configuration');
+    expect(init).toHaveBeenCalled();
   });
 
   describe('when the route is bootstraped', () => {
-    beforeEach(async () => {
-      const issuer = await Issuer.discover('https://accounts.google.com');
-      const client = new issuer.Client({
-        client_id: '123',
-        token_endpoint_auth_method: 'none' as ClientAuthMethod,
-        redirect_uris: ['https://localhost/authentication/callback'],
-      });
+    describe('/authentication', () => {
+      test('responds with auth url if renderingId is correct', async () => {
+        const context = createMockContext({ requestBody: { renderingId: '1' } });
 
-      options.forestAdminClient.getOpenIdClient = jest.fn().mockResolvedValue(client);
+        const authUrl = 'https://accounts.google.com/o/oauth2/v2/auth?client_id=123';
+        jest
+          .spyOn(options.forestAdminClient.authService, 'generateAuthorizationUrl')
+          .mockResolvedValue(authUrl);
 
-      await route.bootstrap();
-    });
-
-    test('/authentication responds with auth url if renderingId is correct', async () => {
-      const context = createMockContext({ requestBody: { renderingId: '1' } });
-
-      // @ts-expect-error: testing private method
-      await route.handleAuthentication(context);
+        // @ts-expect-error: testing private method
+        await route.handleAuthentication(context);
 
-      expect(context.response.body).toEqual({
-        authorizationUrl: expect.stringContaining('https://accounts.google.com/o/oauth2/v2/auth'),
+        expect(context.response.body).toEqual({
+          authorizationUrl: authUrl,
+        });
       });
-    });
 
-    test('/authentication throws if renderingId is incorrect', async () => {
-      const context = createMockContext({ requestBody: { renderingId: 'somethingInvalid' } });
+      test('throws if renderingId is incorrect', async () => {
+        const context = createMockContext({ requestBody: { renderingId: 'somethingInvalid' } });
 
-      // @ts-expect-error: testing private method
-      const fn = () => route.handleAuthentication(context);
+        // @ts-expect-error: testing private method
+        const fn = () => route.handleAuthentication(context);
 
-      await expect(fn).rejects.toThrow('Rendering id must be a number');
-    });
-
-    test('/authentication/callback responds with token if auth is successful', async () => {
-      const context = createMockContext({
-        customProperties: { query: { state: '{"renderingId": 1}' } },
+        await expect(fn).rejects.toThrow('Rendering id must be a number');
       });
+    });
 
-      options.forestAdminClient.getUserInfo = jest.fn().mockResolvedValue({
-        id: 1,
-        email: 'hello@forest.admin',
-        firstName: 'erlich',
-        lastName: 'bachman',
-        team: 'admin',
-        renderingId: '1',
+    describe('/authentication/callback', () => {
+      test('responds with token if auth is successful', async () => {
+        const context = createMockContext({
+          customProperties: { query: { state: '{"renderingId": 1}' } },
+        });
+
+        jest.spyOn(options.forestAdminClient.authService, 'generateTokens').mockResolvedValue({
+          accessToken: '123',
+        });
+
+        jest.spyOn(options.forestAdminClient.authService, 'getUserInfo').mockResolvedValue({
+          id: 1,
+          email: 'hello@forest.admin',
+          firstName: 'erlich',
+          lastName: 'bachman',
+          team: 'admin',
+          renderingId: 1,
+          role: 'admin',
+          tags: {},
+          permissionLevel: 'admin',
+        });
+
+        // @ts-expect-error: testing private method
+        await route.handleAuthenticationCallback(context);
+
+        expect(context.response.body).toEqual({
+          token: expect.any(String),
+          tokenData: {
+            id: 1,
+            email: 'hello@forest.admin',
+            firstName: 'erlich',
+            lastName: 'bachman',
+            team: 'admin',
+            renderingId: 1,
+            role: 'admin',
+            tags: {},
+            permissionLevel: 'admin',
+            exp: expect.any(Number),
+            iat: expect.any(Number),
+          },
+        });
       });
 
-      // @ts-expect-error: testing private method
-      await route.handleAuthenticationCallback(context);
+      test('throws if request.state is invalid', async () => {
+        const context = createMockContext({
+          customProperties: { query: { state: '{"rendeId":' } },
+        });
 
-      expect(context.response.body).toContainAllKeys(['token', 'tokenData']);
-    });
-
-    test('/authentication/callback throws if request.state is invalid', async () => {
-      const context = createMockContext({
-        customProperties: { query: { state: '{"rendeId":' } },
+        // @ts-expect-error: testing private method
+        const fn = () => route.handleAuthenticationCallback(context);
+        await expect(fn).rejects.toThrow('Failed to retrieve renderingId from query[state]');
       });
 
-      // @ts-expect-error: testing private method
-      const fn = () => route.handleAuthenticationCallback(context);
-      await expect(fn).rejects.toThrow('Failed to retrieve renderingId from query[state]');
-    });
+      test('throws if it fails to fetch userinfo (exception)', async () => {
+        jest.spyOn(options.forestAdminClient.authService, 'generateTokens').mockResolvedValue({
+          accessToken: '123',
+        });
+        jest
+          .spyOn(options.forestAdminClient.authService, 'getUserInfo')
+          .mockRejectedValue(new Error('Failed to fetch userinfo'));
 
-    test('/authentication/callback throws if it fails to fetch userinfo (exception)', async () => {
-      options.forestAdminClient.getUserInfo = jest
-        .fn()
-        .mockRejectedValue(new Error('Failed to fetch userinfo'));
+        const context = createMockContext({
+          customProperties: { query: { state: '{"renderingId": 1}' } },
+        });
 
-      const context = createMockContext({
-        customProperties: { query: { state: '{"renderingId": 1}' } },
+        // @ts-expect-error: testing private method
+        const fn = () => route.handleAuthenticationCallback(context);
+
+        await expect(fn).rejects.toThrow('Failed to fetch userinfo');
       });
 
-      // @ts-expect-error: testing private method
-      const fn = () => route.handleAuthenticationCallback(context);
+      test('throws if it fails to fetch userinfo (null)', async () => {
+        options.forestAdminClient.authService.getUserInfo = jest.fn().mockResolvedValue(null);
 
-      await expect(fn).rejects.toThrow('Failed to fetch userinfo');
-    });
+        const context = createMockContext({
+          customProperties: { query: { state: '{"renderingId": 1}' } },
+        });
 
-    test('/authentication/callback throws if it fails to fetch userinfo (null)', async () => {
-      options.forestAdminClient.getUserInfo = jest.fn().mockResolvedValue(null);
+        // @ts-expect-error: testing private method
+        const fn = () => route.handleAuthenticationCallback(context);
 
-      const context = createMockContext({
-        customProperties: { query: { state: '{"renderingId": 1}' } },
+        await expect(fn).rejects.toThrow();
       });
 
-      // @ts-expect-error: testing private method
-      const fn = () => route.handleAuthenticationCallback(context);
-
-      await expect(fn).rejects.toThrow();
+      describe('exception handling', () => {
+        test('returns a translated error if it is an AuthenticationError', async () => {
+          route.setupRoutes(router);
+
+          const getMock = router.get as jest.Mock;
+
+          const errorHandler = getMock.mock.calls[0][1];
+          const callbackHandler = getMock.mock.calls[0][2];
+
+          const context = createMockContext({
+            customProperties: { query: { state: '{"renderingId": 1}' } },
+          });
+
+          jest.spyOn(options.forestAdminClient.authService, 'generateTokens').mockRejectedValue(
+            new AuthenticationError(
+              new errors.OPError({
+                error: 'invalid_request',
+                error_description: 'Invalid request',
+                state: '123',
+              }),
+            ),
+          );
+
+          await errorHandler(context, async () => {
+            await callbackHandler(context);
+          });
+
+          expect(context.response.status).toEqual(401);
+          expect(context.response.body).toEqual({
+            error: 'invalid_request',
+            error_description: 'Invalid request',
+            state: '123',
+          });
+        });
+
+        test('rethrow the error if it is not an AuthenticationError', async () => {
+          route.setupRoutes(router);
+
+          const getMock = router.get as jest.Mock;
+
+          const errorHandler = getMock.mock.calls[0][1];
+          const callbackHandler = getMock.mock.calls[0][2];
+
+          const context = createMockContext({
+            customProperties: { query: { state: '{"renderingId": 1}' } },
+          });
+
+          jest
+            .spyOn(options.forestAdminClient.authService, 'generateTokens')
+            .mockRejectedValue(new Error('Something went wrong'));
+
+          await expect(
+            errorHandler(context, async () => {
+              await callbackHandler(context);
+            }),
+          ).rejects.toThrow('Something went wrong');
+        });
+      });
     });
   });
 });