From d3984aa621be5b7d20e4233814971e25154ce7c0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Jan 2022 18:59:10 -0500 Subject: [PATCH] express: Move `preAuthorize` hook after `express-session` The `ep_openid_connect` plugin needs access to session state before authorization checks are made (to securely redirect the user back to the start page when authentication completes). Now that the `expressPreSession` hook exists, the rationale for moving `preAuthorize` before the `express-session` middleware is gone. This change undoes the following commits: * bf35dcfc5096a77b19bc2b50261b34c98fce7011 * 0b1ec20c5c061c340db2e7feac2229d79d960296 * 30544b564eb7658c6830ba38a31af856fd6509fc --- src/node/hooks/express.js | 17 ++++----- src/node/hooks/express/webaccess.js | 54 ++++++---------------------- src/tests/backend/specs/webaccess.js | 4 +-- 3 files changed, 19 insertions(+), 56 deletions(-) diff --git a/src/node/hooks/express.js b/src/node/hooks/express.js index f7ac91f42..179d37a82 100644 --- a/src/node/hooks/express.js +++ b/src/node/hooks/express.js @@ -204,18 +204,13 @@ exports.restartServer = async () => { }, }); - app.use(webaccess.preAuthorize); - // Give plugins an opportunity to install handlers/middleware after the preAuthorize middleware - // but before the express-session middleware. This allows plugins to avoid creating an - // express-session record in the database when it is not needed (e.g., public static content). + // Give plugins an opportunity to install handlers/middleware before the express-session + // middleware. This allows plugins to avoid creating an express-session record in the database + // when it is not needed (e.g., public static content). await hooks.aCallAll('expressPreSession', {app}); - app.use([ - // If webaccess.preAuthorize explicitly granted access, webaccess.nextRouteIfPreAuthorized will - // call `next('route')` which will skip the remaining middlewares in this list. - webaccess.nextRouteIfPreAuthorized, - exports.sessionMiddleware, - webaccess.checkAccess, - ]); + app.use(exports.sessionMiddleware); + + app.use(webaccess.checkAccess); await Promise.all([ hooks.aCallAll('expressConfigure', {app}), diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 143843488..81ed69b07 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -45,9 +45,8 @@ exports.userCanModify = (padId, req) => { // Exported so that tests can set this to 0 to avoid unnecessary test slowness. exports.authnFailureDelayMs = 1000; -const preAuthorize = async (req, res, next) => { +const checkAccess = async (req, res, next) => { const requireAdmin = req.path.toLowerCase().startsWith('/admin'); - const locals = res.locals._webaccess = {requireAdmin, skip: false}; // /////////////////////////////////////////////////////////////////////////////////////////////// // Step 1: Check the preAuthorize hook for early permit/deny (permit is only allowed for non-admin @@ -56,7 +55,8 @@ const preAuthorize = async (req, res, next) => { // /////////////////////////////////////////////////////////////////////////////////////////////// let results; - const preAuthorizeNext = (...args) => { locals.skip = true; next(...args); }; + let skip = false; + const preAuthorizeNext = (...args) => { skip = true; next(...args); }; try { results = await aCallFirst('preAuthorize', {req, res, next: preAuthorizeNext}, // This predicate will cause aCallFirst to call the hook functions one at a time until one @@ -64,13 +64,13 @@ const preAuthorize = async (req, res, next) => { // page, truthy entries are filtered out before checking to see whether the list is empty. // This prevents plugin authors from accidentally granting admin privileges to the general // public. - (r) => (locals.skip || (r != null && r.filter((x) => (!requireAdmin || !x)).length > 0))); + (r) => (skip || (r != null && r.filter((x) => (!requireAdmin || !x)).length > 0))); } catch (err) { httpLogger.error(`Error in preAuthorize hook: ${err.stack || err.toString()}`); - if (!locals.skip) res.status(500).send('Internal Server Error'); + if (!skip) res.status(500).send('Internal Server Error'); return; } - if (locals.skip) return; + if (skip) return; if (requireAdmin) { // Filter out all 'true' entries to prevent plugin authors from accidentally granting admin // privileges to the general public. @@ -78,22 +78,11 @@ const preAuthorize = async (req, res, next) => { } if (results.length > 0) { // Access was explicitly granted or denied. If any value is false then access is denied. - if (!results.every((x) => x)) { - // Access explicitly denied. - if (await aCallFirst0('preAuthzFailure', {req, res})) return; - // No plugin handled the pre-authentication authorization failure. - return res.status(403).send('Forbidden'); - } - // Access explicitly granted. - locals.skip = true; - return next('route'); + if (results.every((x) => x)) return next(); + if (await aCallFirst0('preAuthzFailure', {req, res})) return; + // No plugin handled the pre-authentication authorization failure. + return res.status(403).send('Forbidden'); } - next(); -}; - -const checkAccess = async (req, res, next) => { - const {locals: {_webaccess: {requireAdmin, skip}}} = res; - if (skip) return next('route'); // This helper is used in steps 2 and 4 below, so it may be called twice per access: once before // authentication is checked and once after (if settings.requireAuthorization is true). @@ -197,30 +186,9 @@ const checkAccess = async (req, res, next) => { res.status(403).send('Forbidden'); }; -/** - * Express middleware that allows plugins to explicitly grant/deny access via the `preAuthorize` - * hook before `checkAccess` is run. If access is explicitly granted: - * - `next('route')` will be called, which can be used to bypass later checks - * - `nextRouteIfPreAuthorized` will simply call `next('route')` - * - `checkAccess` will simply call `next('route')` - */ -exports.preAuthorize = (req, res, next) => { - preAuthorize(req, res, next).catch((err) => next(err || new Error(err))); -}; - -/** - * Express middleware that simply calls `next('route')` if the request has been explicitly granted - * access by `preAuthorize` (otherwise it calls `next()`). This can be used to bypass later checks. - */ -exports.nextRouteIfPreAuthorized = (req, res, next) => { - if (res.locals._webaccess.skip) return next('route'); - next(); -}; - /** * Express middleware to authenticate the user and check authorization. Must be installed after the - * express-session middleware. If the request is pre-authorized, this middleware simply calls - * `next('route')`. + * express-session middleware. */ exports.checkAccess = (req, res, next) => { checkAccess(req, res, next).catch((err) => next(err || new Error(err))); diff --git a/src/tests/backend/specs/webaccess.js b/src/tests/backend/specs/webaccess.js index c55c98ab5..23cd2d889 100644 --- a/src/tests/backend/specs/webaccess.js +++ b/src/tests/backend/specs/webaccess.js @@ -191,11 +191,11 @@ describe(__filename, function () { await agent.get('/').expect(200); assert.deepEqual(callOrder, ['preAuthorize_0']); }); - it('bypasses authenticate and authorize hooks for static content, defers', async function () { + it('static content (expressPreSession) bypasses all auth checks', async function () { settings.requireAuthentication = true; settings.requireAuthorization = true; await agent.get('/static/robots.txt').expect(200); - assert.deepEqual(callOrder, ['preAuthorize_0', 'preAuthorize_1']); + assert.deepEqual(callOrder, []); }); it('cannot grant access to /admin', async function () { handlers.preAuthorize[0].innerHandle = () => [true];