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:
  * bf35dcfc50
  * 0b1ec20c5c
  * 30544b564e

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

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

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];