From 7c71b693195c7b4573944c24772d74cfc053c1ee Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sat, 21 Oct 2023 13:00:06 +0100 Subject: [PATCH 1/8] Implement nav bar for internal users https://eaflood.atlassian.net/browse/WATER-4155 So, pass through authentication should be all sorted. We can now read the cookie passed in my the legacy UI, confirm the user is authenticated and authorised to access a page. We pass through auth, user and scope details in the global context so all view templates have access to the information. We also have our first use of this; deciding whether to display the header links to **Change password** and **Sign out**. The final piece that all our pages should handle is the nav bar. This is where internal users can switch between **Search**, **Bill runs** and **Manage** assuming they have the right permissions. This change implements the nav bar. From 330a197fae78d3efd12a396e38f789d23b987b92 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 22 Oct 2023 23:09:05 +0100 Subject: [PATCH 2/8] Add new permission object to auth credentials All users get to see the search nav bar option. But the 'Bill runs' and 'Manage' options are determined by what roles they have. We don't want to pollute our Nunjucks templates with anything more than if/else kind of logic. So, somewhere in the app we need to work out which options a user has access to. We could do it in the controllers, and pass the result to the view via the context there. But that means repeating the same logic lots of times (or at least calling the same logic from lots of places). This is something we'll need to know for most requests. So, we could determine the permissions in our ViewsPlugin global context builder. But it seems inconsistent that pull most details about the user from the auth object in the request but then work out this final piece in the views plugin. So, we came to the conclusion that this should sit in the `AuthService` and be returned as part of the `credentials` object that gets added to all requests by Hapi. --- app/services/plugins/auth.service.js | 50 +++++++++++++++++++++- test/services/plugins/auth.service.test.js | 44 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/app/services/plugins/auth.service.js b/app/services/plugins/auth.service.js index 6ef69c5dfe..1b72f43235 100644 --- a/app/services/plugins/auth.service.js +++ b/app/services/plugins/auth.service.js @@ -18,6 +18,8 @@ const FetchUserRolesAndGroupsService = require('../idm/fetch-user-roles-and-grou * of the roles the user has. This is used by hapi when authorising the user against a route; if the route has a scope * array of strings then the user's scope array must contain at least one of the strings. * + * Finally, we return a 'permission' object. This is used to determine which nav bar menu items a user sees. + * * @param {Number} userId The user id to be authenticated * * @returns {Object} response @@ -27,6 +29,8 @@ const FetchUserRolesAndGroupsService = require('../idm/fetch-user-roles-and-grou * @returns {RoleModel[]} response.credentials.roles Objects representing the roles the user has * @returns {GroupModel[]} response.credentials.groups Objects representing the groups the user is assigned to * @returns {String[]} response.credentials.scope The names of the roles the user has, for route authorisation purposes + * @returns {Object} response.credentials.permission Object with each top level permission as a key and true or false + * whether the user has authorisation to access the area */ async function go (userId) { const { user, roles, groups } = await FetchUserRolesAndGroupsService.go(userId) @@ -36,7 +40,51 @@ async function go (userId) { return role.role }) - return { isValid: !!user, credentials: { user, roles, groups, scope } } + const permission = _permission(scope) + + return { isValid: !!user, credentials: { user, roles, groups, scope, permission } } +} + +/** + * Determine top level permissions + * + * The legacy service uses 'roles' to determine what a user can or cannot do. These roles are added to routes as 'scope' + * and used to determine if a users is authorized to access a particular page. + * + * These 'roles' are very granular, for example, in the Manage page what links you see is dependent on the roles you + * have. Because of the current way the UI is designed we need to know whether you can access one of the top level areas + * of the site; Bill runs and Manage. + * + * For simplicity we call these 'permissions'. We determine them in this function and add them to the credentials this + * service returns. + * + * They are used by the nav bar to determine which menu items should be visible. + * + * @param {String[]} scope All the scopes (roles) a user has access to + * + * @returns {Object} Each top level permissions is a key. The value is true or false as to whether the user has + * permission to access that area of the service + */ +function _permission (scope = []) { + const billRuns = scope.includes('billing') + + const manageRoles = [ + 'ar_approver', + 'billing', + 'bulk_return_notifications', + 'hof_notifications', + 'manage_accounts', + 'renewal_notifications', + 'returns' + ] + const manage = scope.some((role) => { + return manageRoles.includes(role) + }) + + return { + billRuns, + manage + } } module.exports = { diff --git a/test/services/plugins/auth.service.test.js b/test/services/plugins/auth.service.test.js index 2a09bdbf73..dbf98df7b0 100644 --- a/test/services/plugins/auth.service.test.js +++ b/test/services/plugins/auth.service.test.js @@ -58,6 +58,50 @@ describe('Auth service', () => { expect(result.credentials.scope).to.equal(['Role']) }) + + it('returns the top level permissions in credentials.permission', async () => { + const result = await AuthService.go(12345) + + expect(result.credentials.permission).to.equal({ billRuns: false, manage: false }) + }) + }) + + describe('when the user has a top-level permission role', () => { + describe("such as 'billing'", () => { + beforeEach(() => { + Sinon.stub(FetchUserRolesAndGroupsService, 'go') + .resolves({ + user: { name: 'User' }, + roles: [{ role: 'billing' }], + groups: [{ group: 'Group' }] + }) + }) + + it('returns the matching top level permission as true', async () => { + const result = await AuthService.go(12345) + + // NOTE: Access to bill runs is granted for users with the 'billing' role. They also get access to the manage + // page. So, there currently isn't a scenario where a user would see the 'Bill runs' option but not 'Manage'. + expect(result.credentials.permission).to.equal({ billRuns: true, manage: true }) + }) + }) + + describe("such as 'returns'", () => { + beforeEach(() => { + Sinon.stub(FetchUserRolesAndGroupsService, 'go') + .resolves({ + user: { name: 'User' }, + roles: [{ role: 'returns' }], + groups: [{ group: 'Group' }] + }) + }) + + it('returns the matching top level permission as true', async () => { + const result = await AuthService.go(12345) + + expect(result.credentials.permission).to.equal({ billRuns: false, manage: true }) + }) + }) }) describe('when the user id is not found', () => { From fbceae2159e2465bdccded0c44bd5a451e257441 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 22 Oct 2023 23:16:55 +0100 Subject: [PATCH 3/8] Add new permission object to global context Now all templates will have access to this information. --- app/plugins/views.plugin.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/plugins/views.plugin.js b/app/plugins/views.plugin.js index 7324d38de0..c1e3aec8f8 100644 --- a/app/plugins/views.plugin.js +++ b/app/plugins/views.plugin.js @@ -95,7 +95,8 @@ function context (request) { authenticated: request.auth.isAuthenticated, authorized: request.auth.isAuthorized, user: request.auth.credentials?.user, - scope: request.auth.credentials?.scope + scope: request.auth.credentials?.scope, + permission: request.auth.credentials?.permission } } } From 5f9a461778197aa4ed6aeb89ae7780116a4c81f3 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 22 Oct 2023 23:17:54 +0100 Subject: [PATCH 4/8] Add the nav bar template The html for the layout and the SCSS all come from the [prototype](https://github.com/defra-design/wrls-prototype). We assume they are right! One feature to note is the UI expects to highlight one of the nav bar options when on a page. The legacy service had it's own `ViewContext` plugin that created the global context from a variety of sources. Which nav bar item to highlight was one of them and they chose to set this in the config options of a route, along with the page title. We think it will be clearer if data relevant to a page is route dependent, _all_ the data should come from the controller. For example ```javascript return h.view('info.njk', { pageTitle: 'Info', activeNavBar: 'bill-runs', ...pageData }) ``` That way we know there are only 2 sources; the global context and the context we define in a controller. --- app/views/includes/nav-bar.njk | 27 +++++++++++++++ app/views/layout.njk | 4 +++ client/sass/application.scss | 60 ++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 app/views/includes/nav-bar.njk diff --git a/app/views/includes/nav-bar.njk b/app/views/includes/nav-bar.njk new file mode 100644 index 0000000000..c2bf69a9d6 --- /dev/null +++ b/app/views/includes/nav-bar.njk @@ -0,0 +1,27 @@ + +
+ +
diff --git a/app/views/layout.njk b/app/views/layout.njk index 6422f1c21c..c65992e814 100644 --- a/app/views/layout.njk +++ b/app/views/layout.njk @@ -33,6 +33,10 @@ serviceUrl: "/", navigation: navigationLinks }) }} + + {% if auth.authenticated %} + {% include "includes/nav-bar.njk" %} + {% endif %} {% endblock %} {% block content %} diff --git a/client/sass/application.scss b/client/sass/application.scss index a6d659881b..5f85418d4e 100644 --- a/client/sass/application.scss +++ b/client/sass/application.scss @@ -1,3 +1,63 @@ $govuk-global-styles: true; @import "node_modules/govuk-frontend/govuk/all"; + +// ========== ========== ========== ========== ========== +// Nav bar +// ========== ========== ========== ========== ========== + +// nav bar container +.navbar { + border-bottom: 1px solid $govuk-border-colour; +} + +.navbar__list { + margin: 0; + padding: 0; + list-style: none; + margin-top: 15px 0px; + + @media (min-width: 768px) { + font-size: 19px; + margin-top: 0; + position:relative; + left:-15px; + } +} + +// nav bar list +.navbar__item { + font-weight: 700; + margin-bottom: 5px; + display: block; + @media (min-width: 768px) { + margin-bottom: 0px; + display: inline-block; + font-size: 19px; + } +} + +.navbar__link { + display: inline-block; + text-decoration: none; + padding: 10px 5px 5px 0px; + font-size: 16px; + @media (min-width: 768px) { + padding: 15px; + font-size: 19px; + } +} + +// active link for nav bar +.navbar__link--active { + border-left: 4px solid $govuk-link-colour; + margin-left: -10px; + padding-left: 5px; + @media (min-width: 768px) { + border-left: none; + border-bottom: 4px solid $govuk-link-colour; + margin-bottom: -1px; + margin-left:0; + padding-left:15px; + } +} From 03c04283615563d054238ec29e35b2d48fbd2709 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 23 Oct 2023 15:22:08 +0100 Subject: [PATCH 5/8] Add additional permission Confirmed we need to include Digitise! in the nav-bar like the existing service does (wasn't in the prototype). To support this we need to add another top level permission check. --- app/services/plugins/auth.service.js | 9 +++++++++ test/services/plugins/auth.service.test.js | 23 +++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/app/services/plugins/auth.service.js b/app/services/plugins/auth.service.js index 1b72f43235..5fdb7d495e 100644 --- a/app/services/plugins/auth.service.js +++ b/app/services/plugins/auth.service.js @@ -81,7 +81,16 @@ function _permission (scope = []) { return manageRoles.includes(role) }) + const abstractionReformRoles = [ + 'ar_user', + 'ar_approver' + ] + const abstractionReform = scope.some((role) => { + return abstractionReformRoles.includes(role) + }) + return { + abstractionReform, billRuns, manage } diff --git a/test/services/plugins/auth.service.test.js b/test/services/plugins/auth.service.test.js index dbf98df7b0..a1f8f3bc5a 100644 --- a/test/services/plugins/auth.service.test.js +++ b/test/services/plugins/auth.service.test.js @@ -62,11 +62,28 @@ describe('Auth service', () => { it('returns the top level permissions in credentials.permission', async () => { const result = await AuthService.go(12345) - expect(result.credentials.permission).to.equal({ billRuns: false, manage: false }) + expect(result.credentials.permission).to.equal({ abstractionReform: false, billRuns: false, manage: false }) }) }) describe('when the user has a top-level permission role', () => { + describe("such as 'ar_user'", () => { + beforeEach(() => { + Sinon.stub(FetchUserRolesAndGroupsService, 'go') + .resolves({ + user: { name: 'User' }, + roles: [{ role: 'ar_user' }], + groups: [{ group: 'Group' }] + }) + }) + + it('returns the matching top level permission as true', async () => { + const result = await AuthService.go(12345) + + expect(result.credentials.permission).to.equal({ abstractionReform: true, billRuns: false, manage: false }) + }) + }) + describe("such as 'billing'", () => { beforeEach(() => { Sinon.stub(FetchUserRolesAndGroupsService, 'go') @@ -82,7 +99,7 @@ describe('Auth service', () => { // NOTE: Access to bill runs is granted for users with the 'billing' role. They also get access to the manage // page. So, there currently isn't a scenario where a user would see the 'Bill runs' option but not 'Manage'. - expect(result.credentials.permission).to.equal({ billRuns: true, manage: true }) + expect(result.credentials.permission).to.equal({ abstractionReform: false, billRuns: true, manage: true }) }) }) @@ -99,7 +116,7 @@ describe('Auth service', () => { it('returns the matching top level permission as true', async () => { const result = await AuthService.go(12345) - expect(result.credentials.permission).to.equal({ billRuns: false, manage: true }) + expect(result.credentials.permission).to.equal({ abstractionReform: false, billRuns: false, manage: true }) }) }) }) From 699fd54fa9f9892819e03132f9ecb335d6ceffcf Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 23 Oct 2023 15:23:06 +0100 Subject: [PATCH 6/8] Add the digitise nav bar option --- app/views/includes/nav-bar.njk | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/views/includes/nav-bar.njk b/app/views/includes/nav-bar.njk index c2bf69a9d6..fc24872a9a 100644 --- a/app/views/includes/nav-bar.njk +++ b/app/views/includes/nav-bar.njk @@ -2,7 +2,6 @@