webaccess: Move pre-authn authz check to a separate hook

Before this change, the authorize hook was invoked twice: once before
authentication and again after (if settings.requireAuthorization is
true). Now pre-authentication authorization is instead handled by a
new preAuthorize hook, and the authorize hook is only invoked after
the user has authenticated.

Rationale: Without this change it is too easy to write an
authorization plugin that is too permissive. Specifically:

  * If the plugin does not check the path for /admin then a non-admin
    user might be able to access /admin pages.
  * If the plugin assumes that the user has already been authenticated
    by the time the authorize function is called then unauthenticated
    users might be able to gain access to restricted resources.

This change also avoids calling the plugin's authorize function twice
per access, which makes it easier for plugin authors to write an
authorization plugin that is easy to understand.

This change may break existing authorization plugins: After this
change, the authorize hook will no longer be able to authorize
non-admin access to /admin pages. This is intentional. Access to admin
pages should instead be controlled via the `is_admin` user setting,
which can be set in the config file or by an authentication plugin.

Also:
  * Add tests for the authenticate and authorize hooks.
  * Disable the authentication failure delay when testing.

src/node/hooks/express/webaccess.js

@@ -24,6 +24,9 @@ exports.normalizeAuthzLevel = (level) => {
   return false;
 };
 
+// Exported so that tests can set this to 0 to avoid unnecessary test slowness.
+exports.authnFailureDelayMs = 1000;
+
 exports.checkAccess = (req, res, next) => {
   const hookResultMangle = (cb) => {
     return (err, data) => {
@@ -31,12 +34,11 @@ exports.checkAccess = (req, res, next) => {
     };
   };
 
+  const requireAdmin = req.path.toLowerCase().indexOf('/admin') === 0;
+
   // This may be called twice per access: once before authentication is checked and once after (if
   // settings.requireAuthorization is true).
   const authorize = (fail) => {
-    // Do not require auth for static paths and the API...this could be a bit brittle
-    if (req.path.match(/^\/(static|javascripts|pluginfw|api)/)) return next();
-
     const grant = (level) => {
       level = exports.normalizeAuthzLevel(level);
       if (!level) return fail();
@@ -51,35 +53,70 @@ exports.checkAccess = (req, res, next) => {
       user.padAuthorizations[padId] = level;
       return next();
     };
-
-    if (req.path.toLowerCase().indexOf('/admin') !== 0) {
-      if (!settings.requireAuthentication) return grant('create');
-      if (!settings.requireAuthorization && req.session && req.session.user) return grant('create');
-    }
-
-    if (req.session && req.session.user && req.session.user.is_admin) return grant('create');
-
+    const isAuthenticated = req.session && req.session.user;
+    if (isAuthenticated && req.session.user.is_admin) return grant('create');
+    const requireAuthn = requireAdmin || settings.requireAuthentication;
+    if (!requireAuthn) return grant('create');
+    if (!isAuthenticated) return grant(false);
+    if (requireAdmin && !req.session.user.is_admin) return grant(false);
+    if (!settings.requireAuthorization) return grant('create');
     hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle(grant));
   };
 
-  // Access checking is done in three steps:
+  // Access checking is done in four steps:
   //
-  // 1) Try to just access the thing. If access fails (perhaps authentication has not yet completed,
+  // 1) Check the preAuthorize hook for early permit/deny (permit is only allowed for non-admin
+  //    pages). If any plugin explicitly grants or denies access, skip the remaining steps.
+  // 2) Try to just access the thing. If access fails (perhaps authentication has not yet completed,
   //    or maybe different credentials are required), go to the next step.
-  // 2) Try to authenticate. (Or, if already logged in, reauthenticate with different credentials if
+  // 3) Try to authenticate. (Or, if already logged in, reauthenticate with different credentials if
   //    supported by the authn scheme.) If authentication fails, give the user a 401 error to
   //    request new credentials. Otherwise, go to the next step.
-  // 3) Try to access the thing again. If this fails, give the user a 403 error.
+  // 4) Try to access the thing again. If this fails, give the user a 403 error.
   //
   // Plugins can use the 'next' callback (from the hook's context) to break out at any point (e.g.,
-  // to process an OAuth callback). Plugins can use the authnFailure and authzFailure hooks to
-  // override the default error handling behavior (e.g., to redirect to a login page).
+  // to process an OAuth callback). Plugins can use the preAuthzFailure, authnFailure, and
+  // authzFailure hooks to override the default error handling behavior (e.g., to redirect to a
+  // login page).
 
-  let step1PreAuthenticate, step2Authenticate, step3Authorize;
+  let step1PreAuthorize, step2PreAuthenticate, step3Authenticate, step4Authorize;
 
-  step1PreAuthenticate = () => authorize(step2Authenticate);
+  step1PreAuthorize = () => {
+    // This aCallFirst predicate will cause aCallFirst to call the hook functions one at a time
+    // until one of them returns a non-empty list, with an exception: If the request is for an
+    // /admin 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.
+    const predicate = (results) => (results != null &&
+                                    results.filter((x) => (!requireAdmin || !x)).length > 0);
+    hooks.aCallFirst('preAuthorize', {req, res, next}, (err, results) => {
+      if (err != null) {
+        httpLogger.error('Error in preAuthorize hook:', err);
+        return res.status(500).send('Internal Server Error');
+      }
+      // Do not require auth for static paths and the API...this could be a bit brittle
+      if (req.path.match(/^\/(static|javascripts|pluginfw|api)/)) results.push(true);
+      if (requireAdmin) {
+        // Filter out all 'true' entries to prevent plugin authors from accidentally granting admin
+        // privileges to the general public.
+        results = results.filter((x) => !x);
+      }
+      if (results.length > 0) {
+        // Access was explicitly granted or denied. If any value is false then access is denied.
+        if (results.every((x) => x)) return next();
+        return hooks.aCallFirst('preAuthzFailure', {req, res}, hookResultMangle((ok) => {
+          if (ok) return;
+          // No plugin handled the pre-authentication authorization failure.
+          res.status(403).send('Forbidden');
+        }));
+      }
+      step2PreAuthenticate();
+    }, predicate);
+  };
 
-  step2Authenticate = () => {
+  step2PreAuthenticate = () => authorize(step3Authenticate);
+
+  step3Authenticate = () => {
     if (settings.users == null) settings.users = {};
     const ctx = {req, res, users: settings.users, next};
     // If the HTTP basic auth header is present, extract the username and password so it can be
@@ -107,7 +144,7 @@ exports.checkAccess = (req, res, next) => {
               // Delay the error response for 1s to slow down brute force attacks.
               setTimeout(() => {
                 res.status(401).send('Authentication Required');
-              }, 1000);
+              }, exports.authnFailureDelayMs);
             }));
           }));
         }
@@ -122,11 +159,11 @@ exports.checkAccess = (req, res, next) => {
       let username = req.session.user.username;
       username = (username != null) ? username : '<no username>';
       httpLogger.info(`Successful authentication from IP ${req.ip} for username ${username}`);
-      step3Authorize();
+      step4Authorize();
     }));
   };
 
-  step3Authorize = () => authorize(() => {
+  step4Authorize = () => authorize(() => {
     return hooks.aCallFirst('authzFailure', {req, res}, hookResultMangle((ok) => {
       if (ok) return;
       return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => {
@@ -137,7 +174,7 @@ exports.checkAccess = (req, res, next) => {
     }));
   });
 
-  step1PreAuthenticate();
+  step1PreAuthorize();
 };
 
 exports.secret = null;