fix(auth): login first time not working (#101)

fix(session store): ensured session saved before login redirect

Also increased security of oauth2 strategy by providing the state key as a query parameter

test(oidc): fixed test

LoginHandler test failing due to rewrite of method in inheritance class

test(oidc): login handler coveage
test(auth): increased coverage

src/auth/models/strategy.class.ts

@@ -7,6 +7,7 @@ import { arrayPatternMatch, http, XuiLogger, getLogger } from '../../common'
 import { AuthOptions } from './authOptions.interface'
 import Joi from '@hapi/joi'
 import * as URL from 'url'
+import { generators } from 'openid-client'
 
 export abstract class Strategy extends events.EventEmitter {
     public readonly strategyName: string
@@ -75,15 +76,44 @@ export abstract class Strategy extends events.EventEmitter {
         this.options = options
     }
 
-    public loginHandler = (req: Request, res: Response, next: NextFunction): RequestHandler => {
-        this.logger.log('loginHandler Hit')
+    /**
+     * The login route handler, will attempt to setup security state param and redirect user if not authenticated
+     * @param req Request
+     * @param res Response
+     * @param next NextFunction
+     */
+    public loginHandler = async (req: Request, res: Response, next: NextFunction): Promise<RequestHandler> => {
+        this.logger.log('Base loginHandler Hit')
+
+        // we are using oidc generator but it's just a helper, rather than installing another library to provide this
+        const state = generators.state()
+
+        const promise = new Promise((resolve) => {
+            if (req.session && this.options?.sessionKey) {
+                req.session[this.options?.sessionKey] = { state }
+                req.session.save(() => {
+                    this.logger.log('resolved promise, state saved')
+                    resolve(true)
+                })
+            } else {
+                this.logger.warn('resolved promise, state not saved')
+                resolve(false)
+            }
+        })
+
+        await promise
+
+        this.logger.log('calling passport authenticate')
+
         return passport.authenticate(this.strategyName, {
             // eslint-disable-next-line @typescript-eslint/camelcase
             redirect_uri: req.session?.callbackURL,
+            state,
         } as any)(req, res, next)
     }
 
     public setCallbackURL = (req: Request, res: Response, next: NextFunction): void => {
+        /* istanbul ignore else */
         if (req.session && !req.session.callbackURL) {
             req.app.set('trust proxy', true)
             req.session.callbackURL = URL.format({

src/auth/oauth2/models/oauth2.class.spec.ts

@@ -1,25 +1,132 @@
-import { oauth2 } from './oauth2.class'
+import { oauth2, OAuth2 } from './oauth2.class'
+import passport from 'passport'
+import { createMock } from 'ts-auto-mock'
+import { Request, Response, Router } from 'express'
+import { AuthOptions } from '../../models'
 
-test('OIDC Auth', () => {
-    expect(oauth2).toBeDefined()
-})
+describe('OAUTH2 Auth', () => {
+    test('it should be defined', () => {
+        expect(oauth2).toBeDefined()
+    })
+
+    test('it should be configurable', () => {
+        const options = {
+            authorizationURL: 'someauthurl',
+            tokenURL: 'sometokenUrl',
+            clientID: 'client',
+            clientSecret: 'secret',
+            callbackURL: 'callbackUrl',
+            scope: 'scope',
+            sessionKey: 'node-lib',
+            useRoutes: false,
+            logoutURL: 'logoutUrl',
+            discoveryEndpoint: 'string',
+            issuerURL: 'string',
+            responseTypes: [''],
+            tokenEndpointAuthMethod: 'string',
+        }
+        const handler = oauth2.configure(options)
+        expect(handler).toBeTruthy()
+    })
+
+    test('loginHandler with session and sessionKey', async () => {
+        const mockRouter = createMock<Router>()
+        const options = createMock<AuthOptions>()
+        const logger = createMock<typeof console>()
+        options.sessionKey = 'test'
+        const spy = jest.spyOn(passport, 'authenticate').mockImplementation(() => () => true)
+        const oAuth2 = new OAuth2(mockRouter, logger)
+        spyOn(oAuth2, 'validateOptions')
+        spyOn(oAuth2, 'serializeUser')
+        spyOn(oAuth2, 'deserializeUser')
+        spyOn(oAuth2, 'initializePassport')
+        spyOn(oAuth2, 'initializeSession')
+        spyOn(oAuth2, 'initialiseStrategy')
+        options.useRoutes = true
+        oAuth2.configure(options)
+
+        const mockRequest = ({
+            body: {},
+            session: {
+                save: (callback: any): void => callback(),
+            },
+        } as unknown) as Request
+        const mockResponse = {} as Response
+        const next = jest.fn()
+
+        await oAuth2.loginHandler(mockRequest, mockResponse, next)
+        expect(spy).toBeCalled()
+    })
+
+    test('loginHandler with session and no sessionKey', async () => {
+        const mockRouter = createMock<Router>()
+        const options = createMock<AuthOptions>()
+        const logger = createMock<typeof console>()
+        const spy = jest.spyOn(passport, 'authenticate').mockImplementation(() => () => true)
+        const oAuth2 = new OAuth2(mockRouter, logger)
+        spyOn(oAuth2, 'validateOptions')
+        spyOn(oAuth2, 'serializeUser')
+        spyOn(oAuth2, 'deserializeUser')
+        spyOn(oAuth2, 'initializePassport')
+        spyOn(oAuth2, 'initializeSession')
+        spyOn(oAuth2, 'initialiseStrategy')
+        options.useRoutes = true
+        oAuth2.configure(options)
+
+        const mockRequest = ({
+            body: {},
+            session: {
+                save: (callback: any): void => callback(),
+            },
+        } as unknown) as Request
+        const mockResponse = {} as Response
+        const next = jest.fn()
+
+        await oAuth2.loginHandler(mockRequest, mockResponse, next)
+        expect(spy).toBeCalled()
+    })
+
+    test('setCallbackURL', () => {
+        const mockRequest = ({
+            body: {},
+            session: {},
+            app: {
+                set: jest.fn(),
+            },
+            protocol: 'http',
+            get: jest.fn().mockImplementation(() => 'localhost'),
+        } as unknown) as Request
+        const mockResponse = {} as Response
+        const next = jest.fn()
+        oauth2.setCallbackURL(mockRequest, mockResponse, next)
+        expect(mockRequest.app.set).toBeCalledWith('trust proxy', true)
+        expect(mockRequest.get).toBeCalledWith('host')
+        expect(mockRequest.session?.callbackURL).toEqual('http://localhost/callbackUrl')
+        expect(next).toBeCalled()
+    })
 
-test('OAuth2 configure', () => {
-    const options = {
-        authorizationURL: 'someauthurl',
-        tokenURL: 'sometokenUrl',
-        clientID: 'client',
-        clientSecret: 'secret',
-        callbackURL: 'callbackUrl',
-        scope: 'scope',
-        sessionKey: 'node-lib',
-        useRoutes: false,
-        logoutURL: 'logoutUrl',
-        discoveryEndpoint: 'string',
-        issuerURL: 'string',
-        responseTypes: [''],
-        tokenEndpointAuthMethod: 'string',
-    }
-    const handler = oauth2.configure(options)
-    expect(handler).toBeTruthy()
+    test('setHeaders should set auth headers', () => {
+        const roles = ['test', 'test1']
+        const authToken = 'Bearer abc123'
+        const mockRequest = ({
+            body: {},
+            session: {
+                passport: {
+                    user: {
+                        userinfo: {
+                            roles,
+                        },
+                    },
+                },
+            },
+            headers: {},
+        } as unknown) as Request
+        const mockResponse = {} as Response
+        const next = jest.fn()
+        jest.spyOn(oauth2, 'makeAuthorization').mockImplementation(() => authToken)
+        oauth2.setHeaders(mockRequest, mockResponse, next)
+        expect(mockRequest.headers['user-roles']).toEqual(roles.join())
+        expect(mockRequest.headers.Authorization).toEqual(authToken)
+        expect(next).toBeCalled()
+    })
 })

src/auth/oauth2/models/oauth2.class.ts

@@ -13,8 +13,19 @@ export class OAuth2 extends Strategy {
         super(OAUTH2.STRATEGY_NAME, router, logger)
     }
 
+    /**
+     * Retrieve transformed AuthOptions
+     * @param authOptions
+     * @return OAuth2Metadata
+     */
     public getOAuthOptions = (authOptions: AuthOptions): OAuth2Metadata => {
-        const options = { ...authOptions, ...{ logoutUrl: authOptions.logoutURL } }
+        const options = {
+            ...authOptions,
+            ...{
+                logoutUrl: authOptions.logoutURL,
+                state: '1', // this is required to have oauth strategy chose the right sessionStore
+            },
+        }
         delete options.logoutURL
         return options
     }

src/auth/oidc/models/openid.class.spec.ts

@@ -34,15 +34,44 @@ test('OIDC configure deserializeUser', () => {
     expect(spy).toBeCalled()
 })
 
-test('OIDC loginHandler', () => {
+test('OIDC loginHandler', async () => {
     const spy = jest.spyOn(passport, 'authenticate')
     const mockRequest = {
         body: {},
     } as Request
     const mockResponse = {} as Response
     const next = jest.fn()
 
-    oidc.loginHandler(mockRequest, mockResponse, next)
+    await oidc.loginHandler(mockRequest, mockResponse, next)
+    expect(spy).toBeCalled()
+})
+
+test('OIDC loginHandler with session', async () => {
+    const mockRouter = createMock<Router>()
+    const options = createMock<AuthOptions>()
+    options.sessionKey = 'test'
+    const logger = createMock<typeof console>()
+    const openId = new OpenID(mockRouter, logger)
+    spyOn(openId, 'validateOptions')
+    spyOn(openId, 'serializeUser')
+    spyOn(openId, 'deserializeUser')
+    spyOn(openId, 'initializePassport')
+    spyOn(openId, 'initializeSession')
+    spyOn(openId, 'initialiseStrategy')
+    options.useRoutes = true
+    openId.configure(options)
+
+    const spy = jest.spyOn(passport, 'authenticate')
+    const mockRequest = ({
+        body: {},
+        session: {
+            save: (callback: any): void => callback(),
+        },
+    } as unknown) as Request
+    const mockResponse = {} as Response
+    const next = jest.fn()
+
+    await openId.loginHandler(mockRequest, mockResponse, next)
     expect(spy).toBeCalled()
 })
 
@@ -448,6 +477,7 @@ test('configure with useRoutes', () => {
     const spyOnDeSer = spyOn(openId, 'deserializeUser')
     const spyOnPass = spyOn(openId, 'initializePassport')
     const spyOnSes = spyOn(openId, 'initializeSession')
+    spyOn(openId, 'initialiseStrategy')
     options.useRoutes = true
     openId.configure(options)
     expect(spyOnValidateOptions).toBeCalled()
@@ -468,6 +498,7 @@ test('configure without useRoutes', () => {
     const spyOnDeSer = spyOn(openId, 'deserializeUser')
     const spyOnPass = spyOn(openId, 'initializePassport')
     const spyOnSes = spyOn(openId, 'initializeSession')
+    spyOn(openId, 'initialiseStrategy')
 
     options.useRoutes = false
     openId.configure(options)

src/auth/oidc/models/openid.class.ts

@@ -1,5 +1,14 @@
-import { NextFunction, Request, Response, Router } from 'express'
-import { Client, ClientAuthMethod, Issuer, ResponseType, Strategy, TokenSet, UserinfoResponse } from 'openid-client'
+import { NextFunction, Request, RequestHandler, Response, Router } from 'express'
+import {
+    Client,
+    ClientAuthMethod,
+    Issuer,
+    ResponseType,
+    Strategy,
+    TokenSet,
+    UserinfoResponse,
+    generators,
+} from 'openid-client'
 import passport from 'passport'
 import { OIDC } from '../oidc.constants'
 import { OpenIDMetadata } from './OpenIDMetadata.interface'
@@ -164,6 +173,43 @@ export class OpenID extends AuthStrategy {
     public getClient = (): Client | undefined => {
         return this.client
     }
+
+    /**
+     * The login route handler, will attempt to setup security state and nonce param and redirect user if not authenticated
+     * @param req Request
+     * @param res Response
+     * @param next NextFunction
+     */
+    public loginHandler = async (req: Request, res: Response, next: NextFunction): Promise<RequestHandler> => {
+        this.logger.log('OIDC loginHandler Hit')
+
+        const nonce = generators.nonce()
+        const state = generators.state()
+
+        const promise = new Promise((resolve) => {
+            if (req.session && this.options?.sessionKey) {
+                req.session[this.options.sessionKey] = { nonce, state }
+                req.session.save(() => {
+                    this.logger.log('resolved promise, nonce & state saved')
+                    resolve(true)
+                })
+            } else {
+                this.logger.warn('resolved promise, nonce & state not saved!')
+                resolve(false)
+            }
+        })
+
+        await promise
+
+        this.logger.log('calling passport authenticate')
+
+        return passport.authenticate(this.strategyName, {
+            // eslint-disable-next-line @typescript-eslint/camelcase
+            redirect_uri: req.session?.callbackURL,
+            nonce,
+            state,
+        } as any)(req, res, next)
+    }
 }
 
 export const oidc = new OpenID()