From 2c6b883006e85889d29cdf56076cbde1b8faf35f Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Wed, 20 May 2020 11:27:35 +0200 Subject: [PATCH 1/6] tests for makeProvider --- .../auth-backend/src/providers/factories.ts | 22 +++++ .../auth-backend/src/providers/index.test.ts | 80 +++++++++++++++++++ plugins/auth-backend/src/providers/index.ts | 15 ++-- plugins/auth-backend/src/service/router.ts | 2 + 4 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 plugins/auth-backend/src/providers/factories.ts create mode 100644 plugins/auth-backend/src/providers/index.test.ts diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts new file mode 100644 index 0000000000..218f7995e1 --- /dev/null +++ b/plugins/auth-backend/src/providers/factories.ts @@ -0,0 +1,22 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { AuthProviderFactories } from './types'; +import { GoogleAuthProvider } from './google/provider'; + +export const providerFactories: AuthProviderFactories = { + google: GoogleAuthProvider, +}; diff --git a/plugins/auth-backend/src/providers/index.test.ts b/plugins/auth-backend/src/providers/index.test.ts new file mode 100644 index 0000000000..a6d6c221a9 --- /dev/null +++ b/plugins/auth-backend/src/providers/index.test.ts @@ -0,0 +1,80 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import passport from 'passport'; +import express from 'express'; +import { makeProvider } from '.'; +import { AuthProvider, AuthProviderRouteHandlers } from './types'; + +class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { + strategy(): passport.Strategy { + return new passport.Strategy(); + } + start( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): Promise { + return new Promise((res, rej) => res()); + } + frameHandler( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): express.Response { + return res.send('frameHandler'); + } + logout( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): express.Response { + return res.send('logout'); + } +} + +const providerFactories = { + a: MyAuthProvider, +}; + +const providerConfig = { + provider: 'a', + options: { + somekey: 'somevalue', + }, +}; + +const providerConfigInvalid = { + provider: 'b', + options: { + somekey: 'somevalue', + }, +}; + +describe('makeProvider', () => { + it('makes a provider for Myauthprovider', () => { + const provider = makeProvider(providerFactories, providerConfig); + expect(provider.providerId).toEqual('a'); + expect(provider.strategy).toBeDefined(); + expect(provider.providerRouter).toBeDefined(); + }); + + it('throws an error when provider implementation does not exist', () => { + expect(() => { + makeProvider(providerFactories, providerConfigInvalid); + }).toThrow('Provider Implementation missing for : b auth provider'); + }); +}); diff --git a/plugins/auth-backend/src/providers/index.ts b/plugins/auth-backend/src/providers/index.ts index a26176381a..433c7483b3 100644 --- a/plugins/auth-backend/src/providers/index.ts +++ b/plugins/auth-backend/src/providers/index.ts @@ -21,12 +21,6 @@ import { AuthProviderConfig, } from './types'; -import { GoogleAuthProvider } from './google/provider'; - -const providerFactories: AuthProviderFactories = { - google: GoogleAuthProvider, -}; - export const defaultRouter = (provider: AuthProviderRouteHandlers) => { const router = Router(); router.get('/start', provider.start); @@ -38,11 +32,16 @@ export const defaultRouter = (provider: AuthProviderRouteHandlers) => { return router; }; -export const makeProvider = (config: AuthProviderConfig) => { +export const makeProvider = ( + providerFactories: AuthProviderFactories, + config: any, +) => { const providerId = config.provider; const ProviderImpl = providerFactories[providerId]; if (!ProviderImpl) { - throw Error(`Provider Implementation missing for provider: ${providerId}`); + throw Error( + `Provider Implementation missing for : ${providerId} auth provider`, + ); } const providerInstance = new ProviderImpl(config); const strategy = providerInstance.strategy(); diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index cdde95d9d0..18ec5ad81d 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -19,6 +19,7 @@ import Router from 'express-promise-router'; import passport from 'passport'; import { Logger } from 'winston'; import { providers } from './../providers/config'; +import { providerFactories } from './../providers/factories'; import { makeProvider } from '../providers'; export interface RouterOptions { @@ -35,6 +36,7 @@ export async function createRouter( // configure all the providers for (const providerConfig of providers) { const { providerId, strategy, providerRouter } = makeProvider( + providerFactories, providerConfig, ); logger.info(`Configuring provider: ${providerId}`); From e2179d979c233d1da33ae94e42e4b77fa1bd5f18 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Thu, 21 May 2020 14:55:58 +0200 Subject: [PATCH 2/6] Add tests for providers/index, factories and google provider --- .../src/providers/factories.test.ts | 72 +++++++++++++++++++ .../auth-backend/src/providers/factories.ts | 18 ++++- .../src/providers/google/provider.test.ts | 42 +++++++++++ .../src/providers/google/provider.ts | 2 +- .../auth-backend/src/providers/index.test.ts | 71 ++++++++++++++---- plugins/auth-backend/src/providers/index.ts | 19 ++--- plugins/auth-backend/src/service/router.ts | 2 - 7 files changed, 193 insertions(+), 33 deletions(-) create mode 100644 plugins/auth-backend/src/providers/factories.test.ts create mode 100644 plugins/auth-backend/src/providers/google/provider.test.ts diff --git a/plugins/auth-backend/src/providers/factories.test.ts b/plugins/auth-backend/src/providers/factories.test.ts new file mode 100644 index 0000000000..ef9bb6256f --- /dev/null +++ b/plugins/auth-backend/src/providers/factories.test.ts @@ -0,0 +1,72 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import express from 'express'; +import passport from 'passport'; +import { AuthProvider, AuthProviderRouteHandlers } from './types'; +import { ProviderFactories } from './factories'; + +class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { + strategy(): passport.Strategy { + return new passport.Strategy(); + } + start( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): Promise { + return new Promise(resolve => { + res.send('start'); + resolve(); + }); + } + frameHandler( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): Promise { + return new Promise(resolve => { + res.send('frameHandler'); + resolve(); + }); + } + logout( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): Promise { + return new Promise(resolve => { + res.send('logout'); + resolve(); + }); + } +} + +describe('getProviderFactory', () => { + it('makes a provider for MyAuthProvider', () => { + jest + .spyOn(ProviderFactories, 'getProviderFactory') + .mockReturnValueOnce(MyAuthProvider); + const provider = ProviderFactories.getProviderFactory('a'); + expect(provider).toBeDefined(); + }); + + it('throws an error when provider implementation does not exist', () => { + expect(() => { + ProviderFactories.getProviderFactory('b'); + }).toThrow('Provider Implementation missing for : b auth provider'); + }); +}); diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts index 218f7995e1..6947f0ebd2 100644 --- a/plugins/auth-backend/src/providers/factories.ts +++ b/plugins/auth-backend/src/providers/factories.ts @@ -17,6 +17,18 @@ import { AuthProviderFactories } from './types'; import { GoogleAuthProvider } from './google/provider'; -export const providerFactories: AuthProviderFactories = { - google: GoogleAuthProvider, -}; +export class ProviderFactories { + private static readonly providerFactories: AuthProviderFactories = { + google: GoogleAuthProvider, + }; + + public static getProviderFactory(providerId: string) { + const ProviderImpl = ProviderFactories.providerFactories.providerId; + if (!ProviderImpl) { + throw Error( + `Provider Implementation missing for : ${providerId} auth provider`, + ); + } + return ProviderImpl; + } +} diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts new file mode 100644 index 0000000000..bb35d0a47b --- /dev/null +++ b/plugins/auth-backend/src/providers/google/provider.test.ts @@ -0,0 +1,42 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { GoogleAuthProvider } from './provider'; +import passport from 'passport'; + +const googleAuthProviderConfig = { + provider: 'google', + options: {}, +}; + +const googleAuthProviderConfigInvalid = { + provider: 'google', +}; + +describe('GoogleAuthProvider', () => { + describe('create a new provider', () => { + it('should succeed with valid config', () => { + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + expect(googleAuthProvider).toBeDefined(); + expect(googleAuthProvider.start).toBeDefined(); + expect(googleAuthProvider.logout).toBeDefined(); + expect(googleAuthProvider.frameHandler).toBeDefined(); + expect(googleAuthProvider.strategy).toBeDefined(); + }); + }); +}); diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 5d715724bf..86a490df91 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -62,7 +62,7 @@ export class GoogleAuthProvider } logout(_req: express.Request, res: express.Response) { - return new Promise((resolve) => { + return new Promise(resolve => { res.send('logout!'); resolve(); }); diff --git a/plugins/auth-backend/src/providers/index.test.ts b/plugins/auth-backend/src/providers/index.test.ts index a6d6c221a9..97b40d07dd 100644 --- a/plugins/auth-backend/src/providers/index.test.ts +++ b/plugins/auth-backend/src/providers/index.test.ts @@ -16,10 +16,20 @@ import passport from 'passport'; import express from 'express'; -import { makeProvider } from '.'; -import { AuthProvider, AuthProviderRouteHandlers } from './types'; +import { makeProvider, defaultRouter } from '.'; +import { + AuthProvider, + AuthProviderRouteHandlers, + AuthProviderConfig, +} from './types'; +import { ProviderFactories } from './factories'; class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { + private readonly providerConfig: AuthProviderConfig; + constructor(providerConfig: AuthProviderConfig) { + this.providerConfig = providerConfig; + } + strategy(): passport.Strategy { return new passport.Strategy(); } @@ -28,27 +38,45 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { res: express.Response, next: express.NextFunction, ): Promise { - return new Promise((res, rej) => res()); + return new Promise(resolve => { + res.send('start'); + resolve(); + }); } frameHandler( req: express.Request, res: express.Response, next: express.NextFunction, - ): express.Response { - return res.send('frameHandler'); + ): Promise { + return new Promise(resolve => { + res.send('frameHandler'); + resolve(); + }); } logout( req: express.Request, res: express.Response, next: express.NextFunction, - ): express.Response { - return res.send('logout'); + ): Promise { + return new Promise(resolve => { + res.send('logout'); + resolve(); + }); } } -const providerFactories = { - a: MyAuthProvider, -}; +class MyAuthProviderWithRefresh extends MyAuthProvider { + refresh( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ): Promise { + return new Promise(resolve => { + res.send('logout'); + resolve(); + }); + } +} const providerConfig = { provider: 'a', @@ -66,7 +94,10 @@ const providerConfigInvalid = { describe('makeProvider', () => { it('makes a provider for Myauthprovider', () => { - const provider = makeProvider(providerFactories, providerConfig); + jest + .spyOn(ProviderFactories, 'getProviderFactory') + .mockReturnValueOnce(MyAuthProvider); + const provider = makeProvider(providerConfig); expect(provider.providerId).toEqual('a'); expect(provider.strategy).toBeDefined(); expect(provider.providerRouter).toBeDefined(); @@ -74,7 +105,23 @@ describe('makeProvider', () => { it('throws an error when provider implementation does not exist', () => { expect(() => { - makeProvider(providerFactories, providerConfigInvalid); + makeProvider(providerConfigInvalid); }).toThrow('Provider Implementation missing for : b auth provider'); }); }); + +describe('defaultRouter', () => { + it('make router for auth provider without refresh', () => { + expect( + defaultRouter(new MyAuthProvider({ provider: 'a', options: {} })), + ).toBeDefined(); + }); + + it('make router for auth provider with refresh', () => { + expect( + defaultRouter( + new MyAuthProviderWithRefresh({ provider: 'b', options: {} }), + ), + ).toBeDefined(); + }); +}); diff --git a/plugins/auth-backend/src/providers/index.ts b/plugins/auth-backend/src/providers/index.ts index 433c7483b3..6658299033 100644 --- a/plugins/auth-backend/src/providers/index.ts +++ b/plugins/auth-backend/src/providers/index.ts @@ -15,11 +15,8 @@ */ import Router from 'express-promise-router'; -import { - AuthProviderRouteHandlers, - AuthProviderFactories, - AuthProviderConfig, -} from './types'; +import { AuthProviderRouteHandlers, AuthProviderConfig } from './types'; +import { ProviderFactories } from './factories'; export const defaultRouter = (provider: AuthProviderRouteHandlers) => { const router = Router(); @@ -32,17 +29,9 @@ export const defaultRouter = (provider: AuthProviderRouteHandlers) => { return router; }; -export const makeProvider = ( - providerFactories: AuthProviderFactories, - config: any, -) => { +export const makeProvider = (config: AuthProviderConfig) => { const providerId = config.provider; - const ProviderImpl = providerFactories[providerId]; - if (!ProviderImpl) { - throw Error( - `Provider Implementation missing for : ${providerId} auth provider`, - ); - } + const ProviderImpl = ProviderFactories.getProviderFactory(providerId); const providerInstance = new ProviderImpl(config); const strategy = providerInstance.strategy(); const providerRouter = defaultRouter(providerInstance); diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 18ec5ad81d..cdde95d9d0 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -19,7 +19,6 @@ import Router from 'express-promise-router'; import passport from 'passport'; import { Logger } from 'winston'; import { providers } from './../providers/config'; -import { providerFactories } from './../providers/factories'; import { makeProvider } from '../providers'; export interface RouterOptions { @@ -36,7 +35,6 @@ export async function createRouter( // configure all the providers for (const providerConfig of providers) { const { providerId, strategy, providerRouter } = makeProvider( - providerFactories, providerConfig, ); logger.info(`Configuring provider: ${providerId}`); From 64c743edde81e77092066dbe55a1b7b9c1bc76c5 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Thu, 21 May 2020 20:01:27 +0200 Subject: [PATCH 3/6] Add tests for google auth provider --- .../src/providers/google/provider.test.ts | 165 +++++++++++++++++- .../src/providers/google/provider.ts | 1 + plugins/auth-backend/src/setupTests.ts | 2 + yarn.lock | 43 +++-- 4 files changed, 188 insertions(+), 23 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index bb35d0a47b..e9b6243fa1 100644 --- a/plugins/auth-backend/src/providers/google/provider.test.ts +++ b/plugins/auth-backend/src/providers/google/provider.test.ts @@ -16,17 +16,46 @@ import { GoogleAuthProvider } from './provider'; import passport from 'passport'; +import express from 'express'; +import * as passportGoogleAuth20 from 'passport-google-oauth20'; +// import * as utils from './../utils'; const googleAuthProviderConfig = { + provider: 'google', + options: { + clientID: 'a', + clientSecret: 'b', + callbackURL: 'c', + }, +}; + +const googleAuthProviderConfigFromEnv = { + provider: 'google', + options: { + clientID: process.env.AUTH_GOOGLE_CLIENT_ID, + clientSecret: process.env.AUTH_GOOGLE_CLIENT_SECRET, + callbackURL: 'c', + }, +}; + +const googleAuthProviderConfigMissingInEnv = { + provider: 'google', + options: { + clientID: process.env.MISSING_CLIENT_ID, + clientSecret: process.env.MISSING_CLIENT_SECRET, + callbackURL: 'c', + }, +}; + +const googleAuthProviderConfigInvalidOptions = { provider: 'google', options: {}, }; -const googleAuthProviderConfigInvalid = { - provider: 'google', -}; - describe('GoogleAuthProvider', () => { + afterEach(() => { + jest.clearAllMocks(); + }); describe('create a new provider', () => { it('should succeed with valid config', () => { const googleAuthProvider = new GoogleAuthProvider( @@ -39,4 +68,132 @@ describe('GoogleAuthProvider', () => { expect(googleAuthProvider.strategy).toBeDefined(); }); }); + + describe('start authentication handler', () => { + const mockResponse: any = ({} as unknown) as express.Response; + const mockNext: express.NextFunction = jest.fn(); + + it('should initiate authenticate request with provided scopes', () => { + const mockRequest = ({ + query: { + scopes: 'a,b', + }, + } as unknown) as express.Request; + + const spyPassport = jest + .spyOn(passport, 'authenticate') + .mockImplementation(() => jest.fn()); + + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + googleAuthProvider.start(mockRequest, mockResponse, mockNext); + expect(spyPassport).toBeCalledTimes(1); + expect(spyPassport).toBeCalledWith('google', { + scope: ['a', 'b'], + accessType: 'offline', + prompt: 'consent', + }); + }); + + it('should throw error if no scopes provided', () => { + const mockRequest = ({ + query: {}, + } as unknown) as express.Request; + + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + expect(() => { + googleAuthProvider.start(mockRequest, mockResponse, mockNext); + }).toThrowError('Scopes should be specified'); + }); + }); + + describe('logout handler', () => { + const mockRequest = ({} as unknown) as express.Request; + + it('should perform logout and respond with 200', () => { + const mockResponse: any = ({ + send: jest.fn(), + } as unknown) as express.Response; + + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + + const spyResponse = jest + .spyOn(mockResponse, 'send') + .mockImplementation(() => jest.fn()); + + googleAuthProvider.logout(mockRequest, mockResponse); + expect(spyResponse).toBeCalledTimes(1); + expect(spyResponse).toBeCalledWith('logout!'); + }); + }); + + describe('redirect frame handler', () => { + const mockRequest = ({} as unknown) as express.Request; + const mockResponse: any = ({ + send: jest.fn(), + } as unknown) as express.Response; + const mockNext: express.NextFunction = jest.fn(); + + it('should call authenticate and post a response ', () => { + // TODO: Unable to verify if post message is being called + // const spyPostMessage = jest + // .spyOn(utils, 'postMessageResponse') + // .mockImplementation(() => jest.fn()); + + const spyPassport = jest + .spyOn(passport, 'authenticate') + .mockImplementation(() => jest.fn()); + + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + + googleAuthProvider.frameHandler(mockRequest, mockResponse, mockNext); + expect(spyPassport).toBeCalledTimes(1); + // expect(spyPostMessage).toBeCalledTimes(1); + }); + }); + + describe('strategy handler', () => { + it('should return a valid passport strategy', () => { + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfig, + ); + + // TODO: how to test the callback function? + expect(googleAuthProvider.strategy()).toBeInstanceOf(passport.Strategy); + }); + + it('should return a valid passport strategy if secrets in env', () => { + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfigFromEnv, + ); + + expect(googleAuthProvider.strategy()).toBeInstanceOf(passport.Strategy); + }); + + // TODO: The two tests below should throw errors because either + // the options are missing in env or not present at all + // but they aren't throwing errors + it('should throw an error if secrets missing in env', () => { + expect(() => { + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfigMissingInEnv, + ); + }); + }); + + it('should throw an error for invalid options', () => { + expect(() => { + const googleAuthProvider = new GoogleAuthProvider( + googleAuthProviderConfigInvalidOptions, + ); + }); + }); + }); }); diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 86a490df91..a6b8b0ebec 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -69,6 +69,7 @@ export class GoogleAuthProvider } strategy(): passport.Strategy { + // TODO: throw error if env variables not set? return new GoogleStrategy( { ...this.providerConfig.options }, ( diff --git a/plugins/auth-backend/src/setupTests.ts b/plugins/auth-backend/src/setupTests.ts index 3fa7cb04b4..07f6384b3b 100644 --- a/plugins/auth-backend/src/setupTests.ts +++ b/plugins/auth-backend/src/setupTests.ts @@ -15,3 +15,5 @@ */ require('jest-fetch-mock').enableMocks(); +process.env.AUTH_GOOGLE_CLIENT_ID = 'a'; +process.env.AUTH_GOOGLE_CLIENT_SECRET = 'b'; diff --git a/yarn.lock b/yarn.lock index 73a7b17bb4..1b90a0b933 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9,7 +9,7 @@ dependencies: "@babel/highlight" "^7.0.0" -"@babel/code-frame@7.8.3", "@babel/code-frame@^7.0.0", "@babel/code-frame@^7.5.5", "@babel/code-frame@^7.8.3": +"@babel/code-frame@7.8.3", "@babel/code-frame@^7.0.0", "@babel/code-frame@^7.8.3": version "7.8.3" resolved "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.8.3.tgz#33e25903d7481181534e12ec0a25f16b6fcf419e" integrity sha512-a9gxpmdXtZEInkCSHUJDLHZVBgb1QS0jhss4cPP93EW7s+uC5bikET2twEF3KV+7rDblJcmNvTR7VJejqd2C2g== @@ -3961,9 +3961,9 @@ "@types/uglify-js" "*" "@types/html-webpack-plugin@*", "@types/html-webpack-plugin@^3.2.2": - version "3.2.3" - resolved "https://registry.npmjs.org/@types/html-webpack-plugin/-/html-webpack-plugin-3.2.3.tgz#865323e30e82560c0ca898dbf9f6f9d1c541cd7f" - integrity sha512-Y7dsVhTn75IaD4lMIY02UP1L8e0ou8KQu8DKPJAegEFKdJR28/8ejayDG8ykfR0DtYCx3dCEHIkdpN8AOB6txQ== + version "3.2.2" + resolved "https://registry.npmjs.org/@types/html-webpack-plugin/-/html-webpack-plugin-3.2.2.tgz#f552121f3c0a3972dda9a425de1e0029069b2907" + integrity sha512-KsL5cHtNWhOQF9Cu+Dpn7GemzQRxdKhe1/LgZUSku33B5L4Cx2/p3DX6YbeRNOoI552MNbB/VNbCDNEYU//iAw== dependencies: "@types/html-minifier" "*" "@types/tapable" "*" @@ -4069,9 +4069,9 @@ integrity sha512-ijGqzZt/b7BfzcK9vTrS6MFljQRPn5BFWOx8oE0GYxribu6uV+aA9zZuXI1zc/etK9E8nrgdoF2+LgUw7+9tJQ== "@types/lodash@^4.14.151": - version "4.14.152" - resolved "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.152.tgz#7e7679250adce14e749304cdb570969f77ec997c" - integrity sha512-Vwf9YF2x1GE3WNeUMjT5bTHa2DqgUo87ocdgTScupY2JclZ5Nn7W2RLM/N0+oreexUk8uaVugR81NnTY/jNNXg== + version "4.14.151" + resolved "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.151.tgz#7d58cac32bedb0ec37cb7f99094a167d6176c9d5" + integrity sha512-Zst90IcBX5wnwSu7CAS0vvJkTjTELY4ssKbHiTnGcJgi170uiS8yQDdc3v6S77bRqYQIN1App5a1Pc2lceE5/g== "@types/mime@*": version "2.0.1" @@ -5370,7 +5370,7 @@ aws4@^1.8.0: resolved "https://registry.npmjs.org/aws4/-/aws4-1.9.1.tgz#7e33d8f7d449b3f673cd72deb9abdc552dbe528e" integrity sha512-wMHVg2EOHaMRxbzgFJ9gtjOOCrI80OHLG14rxi28XwOW8ux6IiEbRCGGGqCtdAIg4FQCbW20k9RsT4y3gJlFug== -axios@^0.19.0: +axios@^0.19.0, axios@^0.19.2: version "0.19.2" resolved "https://registry.npmjs.org/axios/-/axios-0.19.2.tgz#3ea36c5d8818d0d5f8a8a97a6d36b86cdc00cb27" integrity sha512-fjgm5MvRHLhx+osE2xoekY70AhARk3a6hkN+3Io1jc00jtquGvxYlKlsFUhmUET0V5te6CcZI7lcv2Ym61mjHA== @@ -9749,11 +9749,11 @@ fork-ts-checker-webpack-plugin@3.1.1: worker-rpc "^0.1.0" fork-ts-checker-webpack-plugin@^4.0.5: - version "4.1.4" - resolved "https://registry.npmjs.org/fork-ts-checker-webpack-plugin/-/fork-ts-checker-webpack-plugin-4.1.4.tgz#f0dc3ece19ec5b792d7b8ecd2a7f43509a5285ce" - integrity sha512-R0nTlZSyV0uCCzYe1kgR7Ve8mXyDvMm1pJwUFb6zzRVF5rTNb24G6gn2DFQy+W5aJYp2eq8aexpCOO+1SCyCSA== + version "4.1.0" + resolved "https://registry.npmjs.org/fork-ts-checker-webpack-plugin/-/fork-ts-checker-webpack-plugin-4.1.0.tgz#62bffe704426770fee33f15f0c0d56c86297fefd" + integrity sha512-2DLwUVUR/AdNmMD2utfmSR8r4qHRFhnfL6QQDQS5q4g5uBZzXYDgg8MXPIbu0HzyLjyvbogqjBNKILG5fufwzg== dependencies: - "@babel/code-frame" "^7.5.5" + babel-code-frame "^6.22.0" chalk "^2.4.1" micromatch "^3.1.10" minimatch "^3.0.4" @@ -20444,10 +20444,15 @@ typedarray@^0.0.6: resolved "https://registry.npmjs.org/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777" integrity sha1-hnrHTjhkGHsdPUfZlqeOxciDB3c= -typescript@^3.7.4, typescript@^3.9.2: - version "3.9.3" - resolved "https://registry.npmjs.org/typescript/-/typescript-3.9.3.tgz#d3ac8883a97c26139e42df5e93eeece33d610b8a" - integrity sha512-D/wqnB2xzNFIcoBG9FG8cXRDjiqSTbG2wd8DMZeQyJlP1vfTkIxH4GKveWaEBYySKIg+USu+E+EDIR47SqnaMQ== +typescript@^3.7.4: + version "3.8.3" + resolved "https://registry.npmjs.org/typescript/-/typescript-3.8.3.tgz#409eb8544ea0335711205869ec458ab109ee1061" + integrity sha512-MYlEfn5VrLNsgudQTVJeNaQFUAI7DkhnOjdpAp4T+ku1TfQClewlbSuTVHiA+8skNBgaf02TL/kLOvig4y3G8w== + +typescript@^3.9.2: + version "3.9.2" + resolved "https://registry.npmjs.org/typescript/-/typescript-3.9.2.tgz#64e9c8e9be6ea583c54607677dd4680a1cf35db9" + integrity sha512-q2ktq4n/uLuNNShyayit+DTobV2ApPEo/6so68JaD5ojvc/6GClBipedB9zNWYxRSAlZXAe405Rlijzl6qDiSw== uc.micro@^1.0.1, uc.micro@^1.0.5: version "1.0.6" @@ -20884,9 +20889,9 @@ uuid@^7.0.3: integrity sha512-DPSke0pXhTZgoF/d+WSt2QaKMCFSfx7QegxEWT+JOuHF5aWrKEn0G+ztjuJg/gG8/ItK+rbPCD/yNv8yyih6Cg== uuid@^8.0.0: - version "8.1.0" - resolved "https://registry.npmjs.org/uuid/-/uuid-8.1.0.tgz#6f1536eb43249f473abc6bd58ff983da1ca30d8d" - integrity sha512-CI18flHDznR0lq54xBycOVmphdCYnQLKn8abKn7PXUiKUGdEd+/l9LWNJmugXel4hXq7S+RMNl34ecyC9TntWg== + version "8.0.0" + resolved "https://registry.npmjs.org/uuid/-/uuid-8.0.0.tgz#bc6ccf91b5ff0ac07bbcdbf1c7c4e150db4dbb6c" + integrity sha512-jOXGuXZAWdsTH7eZLtyXMqUb9EcWMGZNbL9YcGBJl4MH4nrxHmZJhEHvyLFrkxo+28uLb/NYRcStH48fnD0Vzw== v8-compile-cache@^2.0.3: version "2.1.0" From e190463539e03c48598e0ad2aca45ec4326effd9 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Fri, 22 May 2020 16:01:35 +0200 Subject: [PATCH 4/6] update tests --- .../src/providers/google/provider.test.ts | 42 ++++++------------- .../src/providers/google/provider.ts | 4 +- plugins/auth-backend/src/setupTests.ts | 2 - 3 files changed, 14 insertions(+), 34 deletions(-) diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index e9b6243fa1..4881b1dd93 100644 --- a/plugins/auth-backend/src/providers/google/provider.test.ts +++ b/plugins/auth-backend/src/providers/google/provider.test.ts @@ -18,7 +18,7 @@ import { GoogleAuthProvider } from './provider'; import passport from 'passport'; import express from 'express'; import * as passportGoogleAuth20 from 'passport-google-oauth20'; -// import * as utils from './../utils'; +import * as utils from './../utils'; const googleAuthProviderConfig = { provider: 'google', @@ -76,7 +76,7 @@ describe('GoogleAuthProvider', () => { it('should initiate authenticate request with provided scopes', () => { const mockRequest = ({ query: { - scopes: 'a,b', + scope: 'a,b', }, } as unknown) as express.Request; @@ -90,7 +90,7 @@ describe('GoogleAuthProvider', () => { googleAuthProvider.start(mockRequest, mockResponse, mockNext); expect(spyPassport).toBeCalledTimes(1); expect(spyPassport).toBeCalledWith('google', { - scope: ['a', 'b'], + scope: 'a,b', accessType: 'offline', prompt: 'consent', }); @@ -106,7 +106,7 @@ describe('GoogleAuthProvider', () => { ); expect(() => { googleAuthProvider.start(mockRequest, mockResponse, mockNext); - }).toThrowError('Scopes should be specified'); + }).toThrowError('missing scope parameter'); }); }); @@ -140,10 +140,9 @@ describe('GoogleAuthProvider', () => { const mockNext: express.NextFunction = jest.fn(); it('should call authenticate and post a response ', () => { - // TODO: Unable to verify if post message is being called - // const spyPostMessage = jest - // .spyOn(utils, 'postMessageResponse') - // .mockImplementation(() => jest.fn()); + const spyPostMessage = jest + .spyOn(utils, 'postMessageResponse') + .mockImplementation(() => jest.fn()); const spyPassport = jest .spyOn(passport, 'authenticate') @@ -154,8 +153,10 @@ describe('GoogleAuthProvider', () => { ); googleAuthProvider.frameHandler(mockRequest, mockResponse, mockNext); + const callbackFunc = spyPassport.mock.calls[0][1] as Function; + callbackFunc(); expect(spyPassport).toBeCalledTimes(1); - // expect(spyPostMessage).toBeCalledTimes(1); + expect(spyPostMessage).toBeCalledTimes(1); }); }); @@ -165,35 +166,16 @@ describe('GoogleAuthProvider', () => { googleAuthProviderConfig, ); - // TODO: how to test the callback function? expect(googleAuthProvider.strategy()).toBeInstanceOf(passport.Strategy); }); - it('should return a valid passport strategy if secrets in env', () => { - const googleAuthProvider = new GoogleAuthProvider( - googleAuthProviderConfigFromEnv, - ); - - expect(googleAuthProvider.strategy()).toBeInstanceOf(passport.Strategy); - }); - - // TODO: The two tests below should throw errors because either - // the options are missing in env or not present at all - // but they aren't throwing errors - it('should throw an error if secrets missing in env', () => { - expect(() => { - const googleAuthProvider = new GoogleAuthProvider( - googleAuthProviderConfigMissingInEnv, - ); - }); - }); - it('should throw an error for invalid options', () => { expect(() => { const googleAuthProvider = new GoogleAuthProvider( googleAuthProviderConfigInvalidOptions, ); - }); + googleAuthProvider.strategy(); + }).toThrow(); }); }); }); diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index a6b8b0ebec..7f3ae85089 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -77,9 +77,9 @@ export class GoogleAuthProvider refreshToken: any, params: any, profile: any, - cb: any, + done: any, ) => { - cb(undefined, { + done(undefined, { profile, idToken: params.id_token, accessToken, diff --git a/plugins/auth-backend/src/setupTests.ts b/plugins/auth-backend/src/setupTests.ts index 07f6384b3b..3fa7cb04b4 100644 --- a/plugins/auth-backend/src/setupTests.ts +++ b/plugins/auth-backend/src/setupTests.ts @@ -15,5 +15,3 @@ */ require('jest-fetch-mock').enableMocks(); -process.env.AUTH_GOOGLE_CLIENT_ID = 'a'; -process.env.AUTH_GOOGLE_CLIENT_SECRET = 'b'; From c496c0e9994f7012a958d025a53b6c6b0a57df12 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Fri, 22 May 2020 16:12:49 +0200 Subject: [PATCH 5/6] fix lint issues --- .../src/providers/factories.test.ts | 18 ++-------- .../src/providers/google/provider.test.ts | 19 ----------- .../auth-backend/src/providers/index.test.ts | 34 ++++++------------- 3 files changed, 14 insertions(+), 57 deletions(-) diff --git a/plugins/auth-backend/src/providers/factories.test.ts b/plugins/auth-backend/src/providers/factories.test.ts index ef9bb6256f..e2e8420c10 100644 --- a/plugins/auth-backend/src/providers/factories.test.ts +++ b/plugins/auth-backend/src/providers/factories.test.ts @@ -23,31 +23,19 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { strategy(): passport.Strategy { return new passport.Strategy(); } - start( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + start(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('start'); resolve(); }); } - frameHandler( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + frameHandler(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('frameHandler'); resolve(); }); } - logout( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + logout(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('logout'); resolve(); diff --git a/plugins/auth-backend/src/providers/google/provider.test.ts b/plugins/auth-backend/src/providers/google/provider.test.ts index 4881b1dd93..845c46a4da 100644 --- a/plugins/auth-backend/src/providers/google/provider.test.ts +++ b/plugins/auth-backend/src/providers/google/provider.test.ts @@ -17,7 +17,6 @@ import { GoogleAuthProvider } from './provider'; import passport from 'passport'; import express from 'express'; -import * as passportGoogleAuth20 from 'passport-google-oauth20'; import * as utils from './../utils'; const googleAuthProviderConfig = { @@ -29,24 +28,6 @@ const googleAuthProviderConfig = { }, }; -const googleAuthProviderConfigFromEnv = { - provider: 'google', - options: { - clientID: process.env.AUTH_GOOGLE_CLIENT_ID, - clientSecret: process.env.AUTH_GOOGLE_CLIENT_SECRET, - callbackURL: 'c', - }, -}; - -const googleAuthProviderConfigMissingInEnv = { - provider: 'google', - options: { - clientID: process.env.MISSING_CLIENT_ID, - clientSecret: process.env.MISSING_CLIENT_SECRET, - callbackURL: 'c', - }, -}; - const googleAuthProviderConfigInvalidOptions = { provider: 'google', options: {}, diff --git a/plugins/auth-backend/src/providers/index.test.ts b/plugins/auth-backend/src/providers/index.test.ts index 97b40d07dd..c13b840948 100644 --- a/plugins/auth-backend/src/providers/index.test.ts +++ b/plugins/auth-backend/src/providers/index.test.ts @@ -22,6 +22,7 @@ import { AuthProviderRouteHandlers, AuthProviderConfig, } from './types'; +import * as passportGoogleOAuth20 from 'passport-google-oauth20'; import { ProviderFactories } from './factories'; class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { @@ -31,33 +32,24 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { } strategy(): passport.Strategy { - return new passport.Strategy(); + return new passportGoogleOAuth20.Strategy( + this.providerConfig.options, + () => {}, + ); } - start( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + start(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('start'); resolve(); }); } - frameHandler( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + frameHandler(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('frameHandler'); resolve(); }); } - logout( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + logout(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('logout'); resolve(); @@ -66,11 +58,7 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { } class MyAuthProviderWithRefresh extends MyAuthProvider { - refresh( - req: express.Request, - res: express.Response, - next: express.NextFunction, - ): Promise { + refresh(_: express.Request, res: express.Response): Promise { return new Promise(resolve => { res.send('logout'); resolve(); @@ -81,14 +69,14 @@ class MyAuthProviderWithRefresh extends MyAuthProvider { const providerConfig = { provider: 'a', options: { - somekey: 'somevalue', + clientID: 'somevalue', }, }; const providerConfigInvalid = { provider: 'b', options: { - somekey: 'somevalue', + clientID: 'somevalue', }, }; From 3f4bd136d68153d617332ad686df9c8d6d18bb10 Mon Sep 17 00:00:00 2001 From: Raghunandan Date: Sat, 23 May 2020 10:20:02 +0200 Subject: [PATCH 6/6] fix pr review --- .../src/providers/factories.test.ts | 21 ++++---------- .../auth-backend/src/providers/factories.ts | 6 ++-- .../src/providers/google/provider.ts | 7 ++--- .../auth-backend/src/providers/index.test.ts | 28 ++++++------------- plugins/auth-backend/src/providers/types.ts | 8 ++++-- 5 files changed, 24 insertions(+), 46 deletions(-) diff --git a/plugins/auth-backend/src/providers/factories.test.ts b/plugins/auth-backend/src/providers/factories.test.ts index e2e8420c10..1647f62682 100644 --- a/plugins/auth-backend/src/providers/factories.test.ts +++ b/plugins/auth-backend/src/providers/factories.test.ts @@ -23,23 +23,14 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { strategy(): passport.Strategy { return new passport.Strategy(); } - start(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('start'); - resolve(); - }); + async start(_: express.Request, res: express.Response): Promise { + res.send('start'); } - frameHandler(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('frameHandler'); - resolve(); - }); + async frameHandler(_: express.Request, res: express.Response): Promise { + res.send('frameHandler'); } - logout(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('logout'); - resolve(); - }); + async logout(_: express.Request, res: express.Response): Promise { + res.send('logout'); } } diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts index 6947f0ebd2..077d45076e 100644 --- a/plugins/auth-backend/src/providers/factories.ts +++ b/plugins/auth-backend/src/providers/factories.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { AuthProviderFactories } from './types'; +import { AuthProviderFactories, AuthProviderFactory } from './types'; import { GoogleAuthProvider } from './google/provider'; export class ProviderFactories { @@ -22,8 +22,8 @@ export class ProviderFactories { google: GoogleAuthProvider, }; - public static getProviderFactory(providerId: string) { - const ProviderImpl = ProviderFactories.providerFactories.providerId; + public static getProviderFactory(providerId: string): AuthProviderFactory { + const ProviderImpl = ProviderFactories.providerFactories[providerId]; if (!ProviderImpl) { throw Error( `Provider Implementation missing for : ${providerId} auth provider`, diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 7f3ae85089..0ef7e19d3e 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -61,11 +61,8 @@ export class GoogleAuthProvider })(req, res, next); } - logout(_req: express.Request, res: express.Response) { - return new Promise(resolve => { - res.send('logout!'); - resolve(); - }); + async logout(_req: express.Request, res: express.Response) { + res.send('logout!'); } strategy(): passport.Strategy { diff --git a/plugins/auth-backend/src/providers/index.test.ts b/plugins/auth-backend/src/providers/index.test.ts index c13b840948..e42cd32edc 100644 --- a/plugins/auth-backend/src/providers/index.test.ts +++ b/plugins/auth-backend/src/providers/index.test.ts @@ -37,32 +37,20 @@ class MyAuthProvider implements AuthProvider, AuthProviderRouteHandlers { () => {}, ); } - start(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('start'); - resolve(); - }); + async start(_: express.Request, res: express.Response): Promise { + res.send('start'); } - frameHandler(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('frameHandler'); - resolve(); - }); + async frameHandler(_: express.Request, res: express.Response): Promise { + res.send('frameHandler'); } - logout(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('logout'); - resolve(); - }); + async logout(_: express.Request, res: express.Response): Promise { + res.send('logout'); } } class MyAuthProviderWithRefresh extends MyAuthProvider { - refresh(_: express.Request, res: express.Response): Promise { - return new Promise(resolve => { - res.send('logout'); - resolve(); - }); + async refresh(_: express.Request, res: express.Response): Promise { + res.send('logout'); } } diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index eaff539c04..4350f36200 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -51,9 +51,11 @@ export interface AuthProviderRouteHandlers { } export type AuthProviderFactories = { - [key: string]: { - new (providerConfig: any): AuthProvider & AuthProviderRouteHandlers; - }; + [key: string]: AuthProviderFactory; +}; + +export type AuthProviderFactory = { + new (providerConfig: any): AuthProvider & AuthProviderRouteHandlers; }; export type AuthInfo = {