Skip to content

Commit

Permalink
fix: Additional logging (#253)
Browse files Browse the repository at this point in the history 
* Additional logging

* fix: update version

* typo

* lint

* Additional tests for sonar

* lint

* enable and fix tests in the vain hope of satisfying sonar

* more tests

* lint

* reenable and fix tests

* lint

* stupid sonar

* updated version for lib node package

---------

Co-authored-by: Munish Sharma <Munish.Sharma@HMCTS.NET>
  • Loading branch information
@andywilkinshmcts @MunishSharmaHMCTS
andywilkinshmcts and MunishSharmaHMCTS authored Nov 26, 2024
1 parent 37cdd22 commit 1402bd3
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 58 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@hmcts/rpx-xui-node-lib",
"version": "2.29.5",
"version": "2.29.6",
"description": "Common nodejs library components for XUI",
"main": "dist/index",
"types": "dist/index.d.ts",
Expand Down
36 changes: 21 additions & 15 deletions src/auth/models/strategy.class.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,13 @@ export abstract class Strategy extends events.EventEmitter {
const promise = new Promise((resolve) => {
if (req.session && this.options?.sessionKey) {
reqSession[this.options?.sessionKey] = { state }
this.logger.log('saving state in session')
req.session.save(() => {
this.logger.log('resolved promise, state saved')
this.logger.log('state saved in session')
resolve(true)
})
} else {
this.logger.warn('resolved promise, state not saved')
this.logger.warn('sessionKey not available state not saved')
resolve(false)
}
})
Expand All @@ -125,22 +126,24 @@ export abstract class Strategy extends events.EventEmitter {
(error: any, user: any, info: any) => {
/* istanbul ignore next */
if (error) {
this.logger.error('error => ', JSON.stringify(error))
this.logger.error('passport authenticate error ', JSON.stringify(error))
}
/* istanbul ignore next */
if (info) {
this.logger.info(info)
this.logger.info('passport authenticate info', JSON.stringify(info))
}
/* istanbul ignore next */
if (!user) {
const message = 'No user details returned by the authentication service, redirecting to login'
this.logger.log(message)
} else {
this.logger.log('User details from passport.authenticate ' + user.userInfo?.email)
}
},
)(req, res, next)
/* istanbul ignore next */
} catch (error) {
this.logger.error(error, this.strategyName)
this.logger.error('Exception in passport.authenticate', error, this.strategyName)
next(error)
return Promise.reject(error)
}
Expand Down Expand Up @@ -271,7 +274,7 @@ export abstract class Strategy extends events.EventEmitter {

if (!logMessages.length) return

this.logger.log(`emitAuthenticationFailure logMessages ${logMessages}`)
this.logger.log(`emitAuthenticationFailure logMessages ${logMessages.join('\n')}`)

res.locals.message = logMessages.join('\n')
this.emit(AUTH.EVENT.AUTHENTICATE_FAILURE, req, res, next)
Expand All @@ -284,16 +287,14 @@ export abstract class Strategy extends events.EventEmitter {
} as any,
(error: any, user: any, info: any) => {
const errorMessages: string[] = []
this.logger.log('inside passport authenticate')

this.logger.log('in passport authenticate callback')
if (error) {
switch (error.name) {
case 'TimeoutError':
const timeoutErrorMessage = `${error.name}: timeout awaiting ${error.url} for ${error.gotOptions.gotTimeout.request}ms`
errorMessages.push(timeoutErrorMessage)
this.logger.error(error)
break

default:
errorMessages.push(error)
this.logger.error(error)
Expand All @@ -305,8 +306,7 @@ export abstract class Strategy extends events.EventEmitter {
if (info.message === INVALID_STATE_ERROR) {
errorMessages.push(INVALID_STATE_ERROR)
}

this.logger.info(info)
this.logger.info('Authenticate callback info', info)
}

if (!user) {
Expand All @@ -317,7 +317,6 @@ export abstract class Strategy extends events.EventEmitter {
emitAuthenticationFailure(errorMessages)
return res.redirect(AUTH.ROUTE.LOGIN)
}

emitAuthenticationFailure(errorMessages)
this.verifyLogin(req, user, next, res)
},
Expand Down Expand Up @@ -404,16 +403,19 @@ export abstract class Strategy extends events.EventEmitter {
req.logIn(user, (err) => {
const roles = user.userinfo.roles
if (err) {
this.logger.error('verifyLogin error', err)
return next(err)
}
if (this.options.allowRolesRegex && !arrayPatternMatch(roles, this.options.allowRolesRegex)) {
this.logger.error(
`User has no application access, as they do not have a ${this.options.allowRolesRegex} role.`,
`User has no application access, as they do not have a role that matches ${this.options.allowRolesRegex}.`,
)
return this.logout(req, res)
}
if (!this.listenerCount(AUTH.EVENT.AUTHENTICATE_SUCCESS)) {
this.logger.log(`redirecting, no listener count: ${AUTH.EVENT.AUTHENTICATE_SUCCESS}`)
this.logger.log(
`redirecting, no listener count: ${AUTH.EVENT.AUTHENTICATE_SUCCESS}, user: ${user.email}`,
)
res.redirect(AUTH.ROUTE.DEFAULT_REDIRECT)
} else {
req.isRefresh = false
Expand Down Expand Up @@ -539,6 +541,7 @@ export abstract class Strategy extends events.EventEmitter {
done: (err: any, id?: any) => void,
): void => {
if (!this.listenerCount(eventName)) {
this.logger.error('no listeners for event ' + eventName)
done(null, eventObject)
} else {
this.emit(eventName, eventObject, done)
Expand All @@ -555,6 +558,7 @@ export abstract class Strategy extends events.EventEmitter {
const idamClient = options.clientID
return `grant_type=password&password=${userPassword}&username=${userName}&scope=${scope}&client_id=${idamClient}&client_secret=${clientSecret}`
}
const msg = 'options.routeCredential missing values'
throw new Error('options.routeCredential missing values')
}

Expand All @@ -563,6 +567,8 @@ export abstract class Strategy extends events.EventEmitter {
if (options.routeCredential) {
return `${options.logoutURL}/o/token`
}
throw new Error('missing routeCredential in options')
const msg = 'missing routeCredential in options'
this.logger.error('msg')
throw new Error(msg)
}
}
48 changes: 41 additions & 7 deletions src/auth/oauth2/models/oauth2.class.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,32 @@ describe('OAUTH2 Auth', () => {
},
}

xtest('it should be defined', () => {
test('it should be defined', () => {
expect(oauth2).toBeDefined()
})

xtest('it should be configurable', () => {
test('initialiseStrategy', () => {
const options = {
authorizationURL: 'someauthurl',
authorizationURL: 'http://localhost/someauthurl',
tokenURL: 'sometokenUrl',
clientID: 'client',
clientSecret: 'secret',
callbackURL: 'callbackUrl',
scope: 'scope',
sessionKey: 'node-lib',
useRoutes: false,
logoutURL: 'logoutUrl',
discoveryEndpoint: 'http://localhost/someEndpoint',
issuerURL: 'string',
responseTypes: [''],
tokenEndpointAuthMethod: 'string',
}
oauth2.initialiseStrategy(options)
expect(oauth2.isInitialised()).toBeTruthy()
})
test('it should be configurable', () => {
const options = {
authorizationURL: 'http://localhost/someauthurl',
tokenURL: 'sometokenUrl',
clientID: 'client',
clientSecret: 'secret',
Expand All @@ -60,7 +79,7 @@ describe('OAUTH2 Auth', () => {
expect(handler).toBeTruthy()
})

xtest('loginHandler with session and sessionKey', async () => {
test('loginHandler with session and sessionKey', async () => {
const mockRouter = createMock<Router>()
const options = createMock<AuthOptions>()
const logger = {
Expand All @@ -70,6 +89,9 @@ describe('OAUTH2 Auth', () => {
} as unknown as XuiLogger
options.sessionKey = 'test'
options.discoveryEndpoint = 'http://localhost/someEndpoint'
options.authorizationURL = 'http://localhost/someAuthorizationURL'
options.tokenURL = 'http://localhost/someTokenURL'
options.clientID = 'clientID1234'
const spy = jest.spyOn(passport, 'authenticate').mockImplementation(() => () => true)
const oAuth2 = new OAuth2(mockRouter, logger)
jest.spyOn(oAuth2, 'validateOptions')
Expand Down Expand Up @@ -97,8 +119,20 @@ describe('OAUTH2 Auth', () => {
expect(spy).toHaveBeenCalled()
})

xtest('loginHandler with session and no sessionKey', async () => {
test('loginHandler with session and no sessionKey', async () => {
const mockRouter = createMock<Router>()
const options = createMock<AuthOptions>()
options.sessionKey = 'test'
options.discoveryEndpoint = 'http://localhost/someEndpoint'
options.authorizationURL = 'http://localhost/someAuthorizationURL'
options.tokenURL = 'http://localhost/someTokenURL'
options.clientID = 'clientID1234'
options.callbackURL = 'http://localhost/callback'
options.clientSecret = 'secret123'
options.issuerURL = 'http://localhost/someEndpoint'
options.logoutURL = 'http://localhost/testUrl'
options.tokenEndpointAuthMethod = 'string'
options.scope = 'periscope'
const logger = {
log: jest.fn(),
error: jest.fn(),
Expand Down Expand Up @@ -128,7 +162,7 @@ describe('OAUTH2 Auth', () => {
expect(spy).toHaveBeenCalled()
})

xtest('setCallbackURL', () => {
test('setCallbackURL', () => {
const mockRequest = {
...mockRequestRequired,
body: {},
Expand All @@ -148,7 +182,7 @@ describe('OAUTH2 Auth', () => {
expect(next).toHaveBeenCalled()
})

xtest('setHeaders should set auth headers', () => {
test('setHeaders should set auth headers', () => {
const roles = ['test', 'test1']
const authToken = 'Bearer abc123'
const mockRequest = {
Expand Down
5 changes: 5 additions & 0 deletions src/auth/oauth2/models/oauth2.class.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,16 @@ export class OAuth2 extends Strategy {
}

public initialiseStrategy = async (authOptions: AuthOptions): Promise<void> => {
this.logger.log('initialiseStrategy start')
const options = this.getOAuthOptions(authOptions)
passport.use(this.strategyName, new XUIOAuth2Strategy(options, this.verify))
this.logger.log('initialiseStrategy end')
}

public isInitialised(): boolean {
return passport.strategies != null
}

public verify = (
accessToken: string,
refreshToken: string,
Expand Down
Loading

0 comments on commit 1402bd3

Please sign in to comment.