diff --git a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js index 743e191c6e..54668696cb 100644 --- a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js @@ -8,7 +8,8 @@ module.exports = { hasAdminAccess, canRedirectToAdminDomain, getAdminCapabilities, - addHasAdminCapabilityToLocals: expressify(addHasAdminCapabilityToLocals), + useHasAdminCapability, + useAdminCapabilities: expressify(useAdminCapabilities), } function hasAdminAccess(user) { @@ -29,26 +30,40 @@ async function getAdminCapabilities(user) { } } -async function addHasAdminCapabilityToLocals(req, res, next) { +async function useAdminCapabilities(req, res, next) { + if (req.adminCapabilities) { + return next() + } const user = SessionManager.getSessionUser(req.session) if (!hasAdminAccess(user)) { - res.locals.hasAdminCapability = () => false + req.adminCapabilities = [] return next() } try { const { adminCapabilities, adminCapabilitiesAvailable } = await getAdminCapabilities(user) - res.locals.hasAdminCapability = capability => { - if (!adminCapabilitiesAvailable) { - // If admin capabilities are not available, then all admins have all capabilities - return true - } - return adminCapabilities.includes(capability) + req.adminCapabilities = adminCapabilities + req.adminCapabilitiesAvailable = adminCapabilitiesAvailable + } catch (err) { + logger.warn({ err, req }, 'Failed to get admin capabilities') + req.adminCapabilities = [] + // Admin capabilities are likely available because we shouldn't throw otherwise. + req.adminCapabilitiesAvailable = true + } + next() +} + +function useHasAdminCapability(req, res, next) { + res.locals.hasAdminCapability = capability => { + if (!hasAdminAccess(SessionManager.getSessionUser(req.session))) { + return false } - } catch (error) { - logger.warn({ error, req, user }, 'Failed to get admin capabilities') - // A module probably threw so adminCapabilitiesAvailable should be true if we are here so deny to be safe - res.locals.hasAdminCapability = () => false + const { adminCapabilitiesAvailable, adminCapabilities } = req + if (!adminCapabilitiesAvailable) { + // We can't know which capabilities are possible, so we assume all are available for admins. + return true + } + return adminCapabilities.includes(capability) } next() } diff --git a/services/web/app/src/infrastructure/ExpressLocals.js b/services/web/app/src/infrastructure/ExpressLocals.js index 36cd31c9d9..0be0ef313b 100644 --- a/services/web/app/src/infrastructure/ExpressLocals.js +++ b/services/web/app/src/infrastructure/ExpressLocals.js @@ -13,9 +13,10 @@ const PackageVersions = require('./PackageVersions') const Modules = require('./Modules') const Errors = require('../Features/Errors/Errors') const { - addHasAdminCapabilityToLocals, canRedirectToAdminDomain, hasAdminAccess, + useAdminCapabilities, + useHasAdminCapability, } = require('../Features/Helpers/AdminAuthorizationHelper') const { addOptionalCleanupHandlerAfterDrainingConnections, @@ -313,7 +314,9 @@ module.exports = function (webRouter, privateApiRouter, publicApiRouter) { next() }) - webRouter.use(addHasAdminCapabilityToLocals) + webRouter.use(useAdminCapabilities) + + webRouter.use(useHasAdminCapability) webRouter.use(function (req, res, next) { // Clone the nav settings so they can be modified for each request diff --git a/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js b/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js index 6ef8c57111..a14824ad85 100644 --- a/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js +++ b/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js @@ -59,47 +59,7 @@ describe('AdminAuthorizationHelper', function () { }) }) }) - describe('addHasAdminCapabilityToLocals', function () { - describe('when getting capabilities from modules throws an error', function () { - beforeEach(async function () { - this.fireHook.rejects(new Error('Module error')) - - this.req = new MockRequest() - this.res = new MockResponse() - this.next = sinon.stub() - - this.user = { - isAdmin: true, - } - - this.req.logger = { - warn: sinon.stub(), - } - - this.req.session = { - user: this.user, - } - - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( - this.req, - this.res, - this.next - ) - }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') - }) - it('returns false when called with any capability', function () { - expect(this.res.locals.hasAdminCapability('capability1')).to.be.false - }) - it('logs a warning', function () { - expect(this.logger.warn).to.have.been.calledOnce - expect(this.logger.warn.firstCall.args[0]).to.have.property('error') - expect(this.logger.warn.firstCall.args[0].error.message).to.equal( - 'Module error' - ) - }) - }) + describe('useAdminCapabilities', function () { describe('when admin capabilities are not available', function () { describe('user is null', function () { beforeEach(async function () { @@ -111,17 +71,19 @@ describe('AdminAuthorizationHelper', function () { user: null, } - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( + await this.AdminAuthorizationHelper.useAdminCapabilities( this.req, this.res, this.next ) }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') + it('does not define adminCapabilitiesAvailable on req', function () { + expect(this.req).not.to.have.property('adminCapabilitiesAvailable') }) - it('returns false when called with any capability', function () { - expect(this.res.locals.hasAdminCapability('capability1')).to.be.false + it('defines adminCapabilities as an empty array on req', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.be.empty }) }) describe('user is not an admin', function () { @@ -138,17 +100,19 @@ describe('AdminAuthorizationHelper', function () { user: this.user, } - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( + await this.AdminAuthorizationHelper.useAdminCapabilities( this.req, this.res, this.next ) }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') + it('does not define adminCapabilitiesAvailable on req', function () { + expect(this.req).not.to.have.property('adminCapabilitiesAvailable') }) - it('returns false when called with any capability', function () { - expect(this.res.locals.hasAdminCapability('capability1')).to.be.false + it('defines adminCapabilities as an empty array on req', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.be.empty }) }) describe('user is an admin', function () { @@ -165,18 +129,21 @@ describe('AdminAuthorizationHelper', function () { user: this.user, } - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( + await this.AdminAuthorizationHelper.useAdminCapabilities( this.req, this.res, this.next ) }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') + it('defines adminCapabilitiesAvailable as false on req', function () { + expect(this.req).to.have.property('adminCapabilitiesAvailable', false) }) - it('returns true when called with any capability', function () { - expect(this.res.locals.hasAdminCapability('capability1')).to.be.true + + it('defines adminCapabilities as an empty array', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.be.empty }) }) }) @@ -198,20 +165,19 @@ describe('AdminAuthorizationHelper', function () { user: this.user, } - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( + await this.AdminAuthorizationHelper.useAdminCapabilities( this.req, this.res, this.next ) }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') + it('does not define adminCapabilitiesAvailable on req', function () { + expect(this.req).not.to.have.property('adminCapabilitiesAvailable') }) - it('returns false when called with a capability the user has', function () { - expect(this.res.locals.hasAdminCapability('capability1')).to.be.false - }) - it('returns false when called with a capability the user does not have', function () { - expect(this.res.locals.hasAdminCapability('capability3')).to.be.false + it('defines adminCapabilities as an empty array on req', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.be.empty }) }) describe('user is an admin', function () { @@ -228,21 +194,177 @@ describe('AdminAuthorizationHelper', function () { user: this.user, } - await this.AdminAuthorizationHelper.addHasAdminCapabilityToLocals( + await this.AdminAuthorizationHelper.useAdminCapabilities( this.req, this.res, this.next ) }) - it('defines hasAdminCapability on res.locals', function () { - expect(this.res.locals).to.have.property('hasAdminCapability') + it('defines adminCapabilitiesAvailable as true on req', function () { + expect(this.req).to.have.property('adminCapabilitiesAvailable', true) }) - it('returns true when called with a capability the user has', function () { - expect(this.res.locals.hasAdminCapability('capability2')).to.be.true + it('defines adminCapabilities with the capabilities returned from modules', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.include('capability1') + expect(this.req.adminCapabilities).to.include('capability2') }) - it('returns false when called with a capability the user does not have', function () { - expect(this.res.locals.hasAdminCapability('capability3')).to.be.false + }) + }) + describe('when getting capabilities from modules throws an error', function () { + beforeEach(async function () { + this.fireHook.rejects(new Error('Module error')) + + this.req = new MockRequest() + this.res = new MockResponse() + this.next = sinon.stub() + + this.user = { + isAdmin: true, + } + + this.req.logger = { + warn: sinon.stub(), + } + + this.req.session = { + user: this.user, + } + + await this.AdminAuthorizationHelper.useAdminCapabilities( + this.req, + this.res, + this.next + ) + }) + it('logs the error', function () { + expect(this.logger.warn).to.have.been.calledWith( + sinon.match.has('err', sinon.match.instanceOf(Error)) + ) + }) + it('defines adminCapabilitiesAvailable as true on req', function () { + expect(this.req).to.have.property('adminCapabilitiesAvailable', true) + }) + it('defines adminCapabilities as an empty array', function () { + expect(this.req).to.have.property('adminCapabilities') + expect(this.req.adminCapabilities).to.be.an('array') + expect(this.req.adminCapabilities).to.be.empty + }) + }) + }) + describe('useHasAdminCapability', function () { + it('adds hasAdminCapability to res.locals', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals).to.have.property('hasAdminCapability') + expect(res.locals.hasAdminCapability).to.be.a('function') + }) + + describe('when the user is not an admin', function () { + describe('when req.adminCapabilitiesAvailable is true', function () { + it('returns false for any capability', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + req.adminCapabilitiesAvailable = true + req.adminCapabilities = [] + + req.session.user = { isAdmin: false } + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals.hasAdminCapability('capability1')).to.be.false + }) + }) + + describe('when req.adminCapabilitiesAvailable is false', function () { + it('returns false for any capability', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + req.adminCapabilitiesAvailable = false + req.adminCapabilities = [] + + req.session.user = { isAdmin: false } + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals.hasAdminCapability('capability1')).to.be.false + }) + }) + + describe('when req.adminCapabilitiesAvailable is undefined', function () { + it('returns false for any capability', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + req.session.user = { isAdmin: false } + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals.hasAdminCapability('capability1')).to.be.false + }) + }) + }) + + describe('user is an admin', function () { + describe('when req.adminCapabilitiesAvailable is false', function () { + it('returns true for any capability', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + req.session.user = { isAdmin: true } + req.adminCapabilitiesAvailable = false + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals.hasAdminCapability('capability1')).to.be.true + }) + }) + + describe('when req.adminCapabilitiesAvailable is undefined', function () { + it('returns true for any capability', function () { + const req = new MockRequest() + const res = new MockResponse() + const next = sinon.stub() + + req.session.user = { isAdmin: true } + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + + expect(res.locals.hasAdminCapability('capability1')).to.be.true + }) + }) + + describe('when req.adminCapabilitiesAvailable is true', function () { + let req, res, next + beforeEach(function () { + req = new MockRequest() + res = new MockResponse() + next = sinon.stub() + + req.session.user = { isAdmin: true } + req.adminCapabilitiesAvailable = true + req.adminCapabilities = ['capability1', 'capability2'] + + this.AdminAuthorizationHelper.useHasAdminCapability(req, res, next) + }) + + it('returns true for a capability the user has', function () { + expect(res.locals.hasAdminCapability('capability1')).to.be.true + }) + + it('returns false for a capability the user does not have', function () { + expect(res.locals.hasAdminCapability('capability3')).to.be.false }) }) })