From df68be700d46e5095921ce9d3a783273a41bbd34 Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Tue, 29 Jul 2025 15:28:10 +0100 Subject: [PATCH] Move hasAdminCapability middleware into helpers This is so that we can test it more easily than embedding it would allow GitOrigin-RevId: be23d945bc7c816d32b18b4990ecd9e0a6592eb5 --- .../Helpers/AdminAuthorizationHelper.js | 30 ++++ .../app/src/infrastructure/ExpressLocals.js | 32 +--- .../AdminAuthorizationHelperTests.js | 167 ++++++++++++++++++ 3 files changed, 199 insertions(+), 30 deletions(-) diff --git a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js index 216921e58b..afccb6a01b 100644 --- a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js @@ -1,10 +1,14 @@ const Settings = require('@overleaf/settings') const Modules = require('../../infrastructure/Modules') +const { expressify } = require('@overleaf/promise-utils') +const SessionManager = require('../Authentication/SessionManager') +const logger = require('@overleaf/logger') module.exports = { hasAdminAccess, canRedirectToAdminDomain, getAdminCapabilities, + addHasAdminCapabilityToLocals: expressify(addHasAdminCapabilityToLocals), } function hasAdminAccess(user) { @@ -25,6 +29,32 @@ async function getAdminCapabilities(user) { } } +async function addHasAdminCapabilityToLocals(req, res, next) { + const user = SessionManager.getSessionUser(req.session) + try { + const { adminCapabilities, adminCapabilitiesAvailable } = + await getAdminCapabilities(user) + res.locals.hasAdminCapability = capability => { + if (!hasAdminAccess(user)) { + return false + } + if (!adminCapabilitiesAvailable) { + // If admin capabilities are not available, then all admins have all capabilities + return true + } + return adminCapabilities.includes(capability) + } + } catch (error) { + if (user) { + // This is unexpected, it probably means that the session user does not exist. + 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 + } + next() +} + function canRedirectToAdminDomain(user) { if (Settings.adminPrivilegeAvailable) return false if (!Settings.adminUrl) return false diff --git a/services/web/app/src/infrastructure/ExpressLocals.js b/services/web/app/src/infrastructure/ExpressLocals.js index 0032b2c682..36cd31c9d9 100644 --- a/services/web/app/src/infrastructure/ExpressLocals.js +++ b/services/web/app/src/infrastructure/ExpressLocals.js @@ -13,15 +13,14 @@ const PackageVersions = require('./PackageVersions') const Modules = require('./Modules') const Errors = require('../Features/Errors/Errors') const { + addHasAdminCapabilityToLocals, canRedirectToAdminDomain, - getAdminCapabilities, hasAdminAccess, } = require('../Features/Helpers/AdminAuthorizationHelper') const { addOptionalCleanupHandlerAfterDrainingConnections, } = require('./GracefulShutdown') const { sanitizeSessionUserForFrontEnd } = require('./FrontEndUser') -const { expressify } = require('@overleaf/promise-utils') const IEEE_BRAND_ID = Settings.ieeeBrandId @@ -314,34 +313,7 @@ module.exports = function (webRouter, privateApiRouter, publicApiRouter) { next() }) - webRouter.use( - expressify(async function (req, res, next) { - const user = SessionManager.getSessionUser(req.session) - try { - const { adminCapabilities, adminCapabilitiesAvailable } = - await getAdminCapabilities(user) - res.locals.hasAdminCapability = capability => { - if (!hasAdminAccess(user)) { - return false - } - if (!adminCapabilitiesAvailable) { - // If admin capabilities are not available, then all admins have all capabilities - return true - } - return adminCapabilities.includes(capability) - } - } catch (error) { - if (user) { - // This is unexpected, it probably means that the session user does not exist. - logger.warn({ error, req, user }, 'Failed to get admin capabilities') - } - // adminCapabilitiesAvailable should be true if we are here so deny to be safe - res.locals.hasAdminCapability = () => false - } - - next() - }) - ) + webRouter.use(addHasAdminCapabilityToLocals) 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 ba4b532f8f..a25bf985c1 100644 --- a/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js +++ b/services/web/test/unit/src/HelperFiles/AdminAuthorizationHelperTests.js @@ -1,6 +1,8 @@ const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const MockRequest = require('../helpers/MockRequest') +const MockResponse = require('../helpers/MockResponse') const modulePath = '../../../../app/src/Features/Helpers/AdminAuthorizationHelper' @@ -57,4 +59,169 @@ 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: false, + } + + 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('when admin capabilities are not available', function () { + describe('user is not an admin', function () { + beforeEach(async function () { + this.req = new MockRequest() + this.res = new MockResponse() + this.next = sinon.stub() + + this.user = { + isAdmin: false, + } + + 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 + }) + }) + describe('user is an admin', function () { + beforeEach(async function () { + this.req = new MockRequest() + this.res = new MockResponse() + this.next = sinon.stub() + + this.user = { + isAdmin: true, + } + + 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 true when called with any capability', function () { + expect(this.res.locals.hasAdminCapability('capability1')).to.be.true + }) + }) + }) + describe('when admin capabilities are available', function () { + beforeEach(function () { + this.fireHook.resolves(['capability1', 'capability2']) + }) + describe('user is not an admin', function () { + beforeEach(async function () { + this.req = new MockRequest() + this.res = new MockResponse() + this.next = sinon.stub() + + this.user = { + isAdmin: false, + } + + 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 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 + }) + }) + describe('user is an admin', function () { + beforeEach(async function () { + this.req = new MockRequest() + this.res = new MockResponse() + this.next = sinon.stub() + + this.user = { + isAdmin: true, + } + + 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 true when called with a capability the user has', function () { + expect(this.res.locals.hasAdminCapability('capability2')).to.be.true + }) + it('returns false when called with a capability the user does not have', function () { + expect(this.res.locals.hasAdminCapability('capability3')).to.be.false + }) + }) + }) + }) })