From b019a20f317eb79184489dc83d0e4431da9e16e0 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 16 Oct 2023 11:48:07 +0100 Subject: [PATCH] Fix and rename Authentication plugin (#464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://eaflood.atlassian.net/browse/WATER-4085 In [Create authentication plugin](https://github.com/DEFRA/water-abstraction-system/pull/351) we added the ability to authenticate and authorise requests to **water-abstraction-system** using the same data the rest of the service relies on, and the cookie [water-abstraction-ui](https://github.com/DEFRA/water-abstraction-ui) will pass through. One of the nifty things in it was the use of [server.dependency()](https://hapi.dev/api/?v=21.3.2#-serverdependencydependencies-after). Currently, we rely on plugins being registered in a particular order to prevent catastrophe. What this PR tried to demonstrate was that by using `server.dependency()` we could break the dependence on the order they are registered. We're now ready to enable authentication by default but when we tried with `server.auth.default('session')` we kept getting an error. Hapi kept telling us it didn't recognise that strategy. No matter where we made the call we got the error. When we removed the call to `server.dependency()` and re-ordered the plugins in `server.js` the error went away and the default auth on our routes started working. Reading posts like [Handling plugin dependencies](https://hapipal.com/best-practices/handling-plugin-dependencies) highlights there is a lot to think about when it comes to removing the dependence on plugin registration order. If we were maintainers of a plugin, we would need to nail this. But as we are just registering our own for use solely in our own project it looks like considerable overhead and complexity we don't really need. If we screw up the order it becomes obvious pretty quickly that something is broken. So, in this change, we're going back to being non-clever with our plugins! 😁 On the second point, a re-read of the plugin and the associated service highlighted that these enable both authentication and authorisation in Hapi for our routes. Because of this, we're going to rename everything to `auth` so if folks in the future are looking for authorisation-related code they don't overlook the plugin and service. --- ...uthentication.plugin.js => auth.plugin.js} | 14 +++++------ app/server.js | 7 +++--- ...hentication.service.js => auth.service.js} | 7 +++--- ...n.service.test.js => auth.service.test.js} | 24 +++++++++---------- 4 files changed, 27 insertions(+), 25 deletions(-) rename app/plugins/{authentication.plugin.js => auth.plugin.js} (89%) rename app/services/plugins/{authentication.service.js => auth.service.js} (89%) rename test/services/plugins/{authentication.service.test.js => auth.service.test.js} (75%) diff --git a/app/plugins/authentication.plugin.js b/app/plugins/auth.plugin.js similarity index 89% rename from app/plugins/authentication.plugin.js rename to app/plugins/auth.plugin.js index 4e151f8d08..1e2154cd35 100644 --- a/app/plugins/authentication.plugin.js +++ b/app/plugins/auth.plugin.js @@ -1,13 +1,13 @@ 'use strict' /** - * Plugin to authenticate users - * @module AuthenticationPlugin + * Plugin to authenticate and authorise users + * @module AuthPlugin */ -const AuthenticationConfig = require('../../config/authentication.config.js') +const AuthService = require('../services/plugins/auth.service.js') -const AuthenticationService = require('../services/plugins/authentication.service.js') +const AuthenticationConfig = require('../../config/authentication.config.js') const TWO_HOURS_IN_MS = 2 * 60 * 60 * 1000 @@ -33,7 +33,7 @@ const TWO_HOURS_IN_MS = 2 * 60 * 60 * 1000 * More info on authorisation and scope can be found at https://hapi.dev/api/?v=21.3.2#-routeoptionsauthaccessscope */ -const AuthenticationPlugin = { +const AuthPlugin = { name: 'authentication', register: async (server, _options) => { // We wait for @hapi/cookie to be registered before setting up the authentication strategy @@ -49,11 +49,11 @@ const AuthenticationPlugin = { }, redirectTo: '/signin', validate: async (_request, session) => { - return AuthenticationService.go(session.userId) + return AuthService.go(session.userId) } }) }) } } -module.exports = AuthenticationPlugin +module.exports = AuthPlugin diff --git a/app/server.js b/app/server.js index 0ea1831bbc..dab7677783 100644 --- a/app/server.js +++ b/app/server.js @@ -3,7 +3,7 @@ const Hapi = require('@hapi/hapi') const AirbrakePlugin = require('./plugins/airbrake.plugin.js') -const AuthenticationPlugin = require('./plugins/authentication.plugin.js') +const AuthPlugin = require('./plugins/auth.plugin.js') const BlippPlugin = require('./plugins/blipp.plugin.js') const ChargingModuleTokenCachePlugin = require('./plugins/charging-module-token-cache.plugin.js') const ErrorPagesPlugin = require('./plugins/error-pages.plugin.js') @@ -19,11 +19,12 @@ const ViewsPlugin = require('./plugins/views.plugin.js') const ServerConfig = require('../config/server.config.js') const registerPlugins = async (server) => { - // Register the remaining plugins + // NOTE: This order matters to some plugins we register. Inserting into the order should be fine. But if you reorder + // any existing plugin registration double-check you haven't broken anything! await server.register(StopPlugin) await server.register(require('@hapi/inert')) - await server.register(AuthenticationPlugin) await server.register(require('@hapi/cookie')) + await server.register(AuthPlugin) await server.register(RouterPlugin) await server.register(HapiPinoPlugin()) await server.register(AirbrakePlugin) diff --git a/app/services/plugins/authentication.service.js b/app/services/plugins/auth.service.js similarity index 89% rename from app/services/plugins/authentication.service.js rename to app/services/plugins/auth.service.js index f50cdb73be..6ef69c5dfe 100644 --- a/app/services/plugins/authentication.service.js +++ b/app/services/plugins/auth.service.js @@ -1,14 +1,14 @@ 'use strict' /** - * Used by `AuthenticationPlugin` to authenticate and authorise users - * @module AuthenticationService + * Used by `AuthPlugin` to authenticate and authorise users + * @module AuthService */ const FetchUserRolesAndGroupsService = require('../idm/fetch-user-roles-and-groups.service.js') /** - * This service is intended to be used by our `AuthenticationPlugin` to authenticate and authorise users. + * This service is intended to be used by our `AuthPlugin` to authenticate and authorise users. * * We take a user id and look it up in the `idm` schema using `FetchUserRolesAndGroupsService`. This gives us a user * object along with arrays of role objects and group objects that the user has been assigned to. @@ -19,6 +19,7 @@ const FetchUserRolesAndGroupsService = require('../idm/fetch-user-roles-and-grou * array of strings then the user's scope array must contain at least one of the strings. * * @param {Number} userId The user id to be authenticated + * * @returns {Object} response * @returns {Boolean} response.isValid Indicates whether the user was found * @returns {Object} response.credentials User credentials found in the IDM diff --git a/test/services/plugins/authentication.service.test.js b/test/services/plugins/auth.service.test.js similarity index 75% rename from test/services/plugins/authentication.service.test.js rename to test/services/plugins/auth.service.test.js index 3c6b0a3c6e..2a09bdbf73 100644 --- a/test/services/plugins/authentication.service.test.js +++ b/test/services/plugins/auth.service.test.js @@ -12,9 +12,9 @@ const { expect } = Code const FetchUserRolesAndGroupsService = require('../../../app/services/idm/fetch-user-roles-and-groups.service.js') // Thing under test -const AuthenticationService = require('../../../app/services/plugins/authentication.service.js') +const AuthService = require('../../../app/services/plugins/auth.service.js') -describe('Authentication service', () => { +describe('Auth service', () => { afterEach(() => { Sinon.restore() }) @@ -30,31 +30,31 @@ describe('Authentication service', () => { }) it('returns isValid as `true`', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.isValid).to.be.true() }) it('returns the user in credentials.user', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.user).to.equal({ name: 'User' }) }) it('returns the roles in credentials.roles', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.roles).to.equal([{ role: 'Role' }]) }) it('returns the groups in credentials.groups', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.groups).to.equal([{ group: 'Group' }]) }) it('returns the role names in credentials.scope', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.scope).to.equal(['Role']) }) @@ -71,31 +71,31 @@ describe('Authentication service', () => { }) it('returns isValid as `false`', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.isValid).to.be.false() }) it('returns `null` in credentials.user', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.user).to.be.null() }) it('returns an empty array in credentials.roles', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.roles).to.be.empty() }) it('returns an empty array in credentials.groups', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.groups).to.be.empty() }) it('returns an empty array in credentials.scope', async () => { - const result = await AuthenticationService.go(12345) + const result = await AuthService.go(12345) expect(result.credentials.scope).to.be.empty() })