Split adminCapabilities middleware into two

GitOrigin-RevId: 093e455e33459cae2e3da236958cb991f128299e

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()
 }

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

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
         })
       })
     })