Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clean up logging a bit #246

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: clean up logging a bit #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
280e30b
clean up logging a bit
andywilkinshmcts Aug 23, 2024
ae5448c
lint
andywilkinshmcts Aug 23, 2024
a2302ff
update axios and other dependencies for CVE
andywilkinshmcts Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@hmcts/rpx-xui-node-lib",
"version": "2.29.2-output-stdout",
"version": "2.29.3-logging",
"description": "Common nodejs library components for XUI",
"main": "dist/index",
"types": "dist/index.d.ts",
Expand Down Expand Up @@ -68,7 +68,9 @@
"eslint-plugin-prettier": "^3.3.1",
"husky": "^8.0.0",
"jest": "^29.7.0",
"jest-environment-jsdom": "^27.5.1",
"jest-environment-jsdom": "^29.7.0",
"jest-mock-axios": "^4.7.3",
"jest-ts-auto-mock": "^2.1.0",
"lint-staged": "^10.2.2",
"prettier": "^2.0.5",
"semantic-release": "^19.0.2",
Expand All @@ -87,16 +89,14 @@
},
"dependencies": {
"@hapi/joi": "^17.1.1",
"axios": "^0.21.1",
"axios": "^1.7.5",
"caller-path": "^3.0.0",
"connect-redis": "^4.0.4",
"csurf": "^1.11.0",
"debug": "^4.1.1",
"deepmerge": "^4.2.2",
"express": "^4.17.1",
"express-session": "^1.17.0",
"jest-mock-axios": "^4.7.3",
"jest-ts-auto-mock": "^2.1.0",
"jwt-decode": "^2.2.0",
"openid-client": "^3.10.0",
"otplib": "^12.0.1",
Expand All @@ -108,8 +108,6 @@
"ttypescript": "^1.5.13"
},
"resolutions": {
"@babel/traverse": "7.23.2",
"minimatch": "3.0.5",
"minimist": "1.2.6"
"@babel/traverse": "7.23.2"
}
}
2 changes: 1 addition & 1 deletion src/auth/messaging.constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export const VERIFY_ERROR_MESSAGE_NO_ACCESS_ROLES = 'User does not have any access roles.'
export const VERIFY_ERROR_MESSAGE_NO_ACCESS_ROLES = 'User does not have any IDAM roles.'
31 changes: 15 additions & 16 deletions src/auth/models/strategy.class.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export abstract class Strategy extends events.EventEmitter {
resolve(true)
})
} else {
this.logger.warn('resolved promise, state not saved')
this.logger.warn('resolved promise, no session key, state not saved')
resolve(false)
}
})
Expand All @@ -123,13 +123,14 @@ export abstract class Strategy extends events.EventEmitter {
state,
} as any,
(error: any, user: any, info: any) => {
this.logger.log('authenticate callback user', JSON.stringify(user))
/* istanbul ignore next */
if (error) {
this.logger.error('error => ', JSON.stringify(error))
this.logger.error('authenticate callback error ', JSON.stringify(error))
}
/* istanbul ignore next */
if (info) {
this.logger.info(info)
this.logger.info('authenticate callback', info)
}
/* istanbul ignore next */
if (!user) {
Expand All @@ -140,7 +141,7 @@ export abstract class Strategy extends events.EventEmitter {
)(req, res, next)
/* istanbul ignore next */
} catch (error) {
this.logger.error(error, this.strategyName)
this.logger.error('authentication exception', error, this.strategyName)
next(error)
return Promise.reject(error)
}
Expand Down Expand Up @@ -196,13 +197,13 @@ export abstract class Strategy extends events.EventEmitter {
}

const redirect = req.query.redirect ? req.query.redirect : AUTH.ROUTE.LOGIN
this.logger.log('redirecting to => ', redirect)
this.logger.log('logout redirecting to ', redirect)
// 401 is when no accessToken
res.redirect(redirect as string)

/* istanbul ignore next */
} catch (e) {
this.logger.error('error => ', e)
this.logger.error('logout exception ', e)
res.status(401).redirect(AUTH.ROUTE.DEFAULT_REDIRECT)
}
this.logger.log('logout end')
Expand Down Expand Up @@ -262,7 +263,7 @@ export abstract class Strategy extends events.EventEmitter {

/* istanbul ignore next */
public callbackHandler = (req: Request, res: Response, next: NextFunction): void => {
this.logger.log('inside callbackHandler')
this.logger.log('inside callbackHandler for ', req.originalUrl)
const INVALID_STATE_ERROR = 'Invalid authorization request state.'
const reqSession = req.session as MySessionData

Expand All @@ -284,8 +285,7 @@ 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':
Expand All @@ -305,8 +305,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('callbackHandler authenticate info', info)
}

if (!user) {
Expand All @@ -315,9 +314,9 @@ export abstract class Strategy extends events.EventEmitter {
this.logger.log(message)

emitAuthenticationFailure(errorMessages)
this.logger.info('redirecting to ' + AUTH.ROUTE.LOGIN)
return res.redirect(AUTH.ROUTE.LOGIN)
}

emitAuthenticationFailure(errorMessages)
this.verifyLogin(req, user, next, res)
},
Expand Down Expand Up @@ -402,13 +401,13 @@ export abstract class Strategy extends events.EventEmitter {
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.`,
)
const re = this.options.allowRolesRegex
const roleStr = roles.join(' ')
this.logger.error('Missing required roles', re, roleStr)
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(`no listener count, redirecting to: ${AUTH.ROUTE.DEFAULT_REDIRECT}`)
res.redirect(AUTH.ROUTE.DEFAULT_REDIRECT)
} else {
req.isRefresh = false
Expand Down
22 changes: 11 additions & 11 deletions src/auth/oidc/models/openid.class.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,17 @@ export class OpenID extends AuthStrategy {
reqsession.passport.user.tokenset = this.convertTokenSet(tokenSet)

if (!this.listenerCount(AUTH.EVENT.AUTHENTICATE_SUCCESS)) {
this.logger.log(`refresh: no listener count: ${AUTH.EVENT.AUTHENTICATE_SUCCESS}`)
this.logger.log(`refresh: no listeners: ${AUTH.EVENT.AUTHENTICATE_SUCCESS}`)
return next()
} else {
req.isRefresh = true
this.logger.log('refresh: success')
this.emit(AUTH.EVENT.AUTHENTICATE_SUCCESS, req, res, next)
return
}
}
} catch (e) {
this.logger.error('refresh error => ', e)
this.logger.error('refresh exception ', e)
next(e)
}
}
Expand Down Expand Up @@ -160,7 +161,7 @@ export class OpenID extends AuthStrategy {
this.logger.warn(VERIFY_ERROR_MESSAGE_NO_ACCESS_ROLES)
return done(null, false, { message: VERIFY_ERROR_MESSAGE_NO_ACCESS_ROLES })
}
this.logger.info('verify okay, user:', userinfo)
this.logger.info('verify success, user:', userinfo)

return done(null, { tokenset: this.convertTokenSet(tokenset), userinfo })
}
Expand Down Expand Up @@ -234,7 +235,7 @@ export class OpenID extends AuthStrategy {
if (req.session && this.options?.sessionKey) {
reqsession[this.options?.sessionKey] = { state }
req.session.save(() => {
this.logger.log('resolved promise, nonce & state saved')
this.logger.log('resolved promise, state saved')
resolve(true)
})
} else {
Expand All @@ -246,7 +247,7 @@ export class OpenID extends AuthStrategy {
try {
await promise

this.logger.log('calling passport authenticate')
this.logger.log('OAuth2 calling passport authenticate')

return passport.authenticate(
this.strategyName,
Expand All @@ -256,28 +257,27 @@ export class OpenID extends AuthStrategy {
state,
} as any,
(error: any, user: any, info: any) => {
this.logger.log('passport authenticate')

this.logger.log('OAuth2 passport authenticate callback')
if (error) {
this.logger.error('loginHandler error: ', JSON.stringify(error))
}
/* istanbul ignore next */
if (info) {
this.logger.info(info)
this.logger.info('OAuth2 info', info)
}
/* istanbul ignore next */
if (user) {
const message = 'loginHandler User details returned by passport authenticate'
const message = 'OAuth2 loginHandler User details from passport authenticate'
this.logger.log(message)
}
if (!user) {
const message = 'loginHandler no User details returned by passport authenticate'
const message = 'OAuth2 loginHandler no User details returned by passport authenticate'
this.logger.log(message)
}
},
)(req, res, next)
} catch (error) {
this.logger.error('this should not throw an error')
this.logger.error('OAuth2 loginHandler unexpected error', error)
throw new Error(`this should not throw an ${error}`)
}
}
Expand Down
Loading
Loading