webaccess: Return 401 for authn failure, 403 for authz failure

This makes it possible for reverse proxies to transform 403 errors
into something like "upgrade to a premium account to access this
pad".

Also add some webaccess tests.

doc/api/hooks_server-side.md

@@ -370,7 +370,8 @@ A plugin's authFailure function is only called if all of the following are true:
 Calling the provided callback with `[true]` tells Etherpad that the failure was
 handled and no further error handling is required. Calling the callback with
 `[]` or `undefined` defers error handling to the next authFailure plugin (if
-any, otherwise fall back to HTTP basic authentication).
+any, otherwise fall back to HTTP basic authentication for an authentication
+failure or a generic 403 page for an authorization failure).
 
 Example:
 

src/node/hooks/express/webaccess.js

@@ -58,19 +58,6 @@ exports.checkAccess = (req, res, next) => {
     hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle(grant));
   };
 
-  /* Authentication OR authorization failed. */
-  const failure = () => {
-    return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => {
-      if (ok) return;
-      // No plugin handled the authn/authz failure. Fall back to basic authentication.
-      res.header('WWW-Authenticate', 'Basic realm="Protected Area"');
-      // Delay the error response for 1s to slow down brute force attacks.
-      setTimeout(() => {
-        res.status(401).send('Authentication Required');
-      }, 1000);
-    }));
-  };
-
   // Access checking is done in three steps:
   //
   // 1) Try to just access the thing. If access fails (perhaps authentication has not yet completed,
@@ -78,7 +65,7 @@ exports.checkAccess = (req, res, next) => {
   // 2) 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 401 error.
+  // 3) 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 authFailure hook to override the default
@@ -103,6 +90,17 @@ exports.checkAccess = (req, res, next) => {
     }
     hooks.aCallFirst('authenticate', ctx, hookResultMangle((ok) => {
       if (!ok) {
+        const failure = () => {
+          return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => {
+            if (ok) return;
+            // No plugin handled the authentication failure. Fall back to basic authentication.
+            res.header('WWW-Authenticate', 'Basic realm="Protected Area"');
+            // Delay the error response for 1s to slow down brute force attacks.
+            setTimeout(() => {
+              res.status(401).send('Authentication Required');
+            }, 1000);
+          }));
+        };
         // Fall back to HTTP basic auth.
         if (!httpBasicAuth) return failure();
         if (!(ctx.username in settings.users)) {
@@ -126,7 +124,13 @@ exports.checkAccess = (req, res, next) => {
     }));
   };
 
-  step3Authorize = () => authorize(failure);
+  step3Authorize = () => authorize(() => {
+    return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => {
+      if (ok) return;
+      // No plugin handled the authorization failure.
+      res.status(403).send('Forbidden');
+    }));
+  });
 
   step1PreAuthenticate();
 };

src/node/server.js

@@ -45,7 +45,7 @@ let started = false;
 let stopped = false;
 
 exports.start = async () => {
-  if (started) return;
+  if (started) return express.server;
   started = true;
   if (stopped) throw new Error('restart not supported');
 

tests/backend/specs/webaccess.js

@@ -0,0 +1,166 @@
+function m(mod) { return __dirname + '/../../../src/' + mod; }
+
+const assert = require('assert').strict;
+const log4js = require(m('node_modules/log4js'));
+const plugins = require(m('static/js/pluginfw/plugin_defs'));
+const server = require(m('node/server'));
+const settings = require(m('node/utils/Settings'));
+const supertest = require(m('node_modules/supertest'));
+
+let agent;
+const logger = log4js.getLogger('test');
+
+before(async function() {
+  settings.port = 0;
+  settings.ip = 'localhost';
+  const httpServer = await server.start();
+  const baseUrl = `http://localhost:${httpServer.address().port}`;
+  logger.debug(`HTTP server at ${baseUrl}`);
+  agent = supertest(baseUrl);
+});
+
+after(async function() {
+  await server.stop();
+});
+
+describe('webaccess without any plugins', function() {
+  const backup = {};
+
+  before(async function() {
+    Object.assign(backup, settings);
+    settings.users = {
+      admin: {password: 'admin-password', is_admin: true},
+      user: {password: 'user-password'},
+    };
+  });
+
+  after(async function() {
+    Object.assign(settings, backup);
+  });
+
+  it('!authn !authz anonymous / -> 200', async function() {
+    settings.requireAuthentication = false;
+    settings.requireAuthorization = false;
+    await agent.get('/').expect(200);
+  });
+  it('!authn !authz anonymous /admin/ -> 401', async function() {
+    settings.requireAuthentication = false;
+    settings.requireAuthorization = false;
+    await agent.get('/admin/').expect(401);
+  });
+  it('authn !authz anonymous / -> 401', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    await agent.get('/').expect(401);
+  });
+  it('authn !authz user / -> 200', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    await agent.get('/').auth('user', 'user-password').expect(200);
+  });
+  it('authn !authz user /admin/ -> 403', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    await agent.get('/admin/').auth('user', 'user-password').expect(403);
+  });
+  it('authn !authz admin / -> 200', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    await agent.get('/').auth('admin', 'admin-password').expect(200);
+  });
+  it('authn !authz admin /admin/ -> 200', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    await agent.get('/admin/').auth('admin', 'admin-password').expect(200);
+  });
+  it('authn authz user / -> 403', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = true;
+    await agent.get('/').auth('user', 'user-password').expect(403);
+  });
+  it('authn authz user /admin/ -> 403', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = true;
+    await agent.get('/admin/').auth('user', 'user-password').expect(403);
+  });
+  it('authn authz admin / -> 200', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = true;
+    await agent.get('/').auth('admin', 'admin-password').expect(200);
+  });
+  it('authn authz admin /admin/ -> 200', async function() {
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = true;
+    await agent.get('/admin/').auth('admin', 'admin-password').expect(200);
+  });
+});
+
+describe('webaccess with authFailure plugin', function() {
+  let handle, returnUndef, status, called;
+  const authFailure = (hookName, context, cb) => {
+    assert.equal(hookName, 'authFailure');
+    assert(context != null);
+    assert(context.req != null);
+    assert(context.res != null);
+    assert(context.next != null);
+    assert(!called);
+    called = true;
+    if (handle) {
+      context.res.status(status).send('injected content');
+      return cb([true]);
+    }
+    if (returnUndef) return cb();
+    return cb([]);
+  };
+
+  const settingsBackup = {};
+  let authFailureHooksBackup;
+  before(function() {
+    Object.assign(settingsBackup, settings);
+    authFailureHooksBackup = plugins.hooks.authFailure;
+    plugins.hooks.authFailure = [{hook_fn: authFailure}];
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = true;
+    settings.users = {
+      admin: {password: 'admin-password', is_admin: true},
+      user: {password: 'user-password'},
+    };
+  });
+  after(function() {
+    Object.assign(settings, settingsBackup);
+    plugins.hooks.authFailure = authFailureHooksBackup;
+  });
+
+  beforeEach(function() {
+    handle = false;
+    returnUndef = false;
+    status = 200;
+    called = false;
+  });
+  afterEach(function() {
+    assert(called);
+  });
+
+  it('authn fail, hook handles -> 200', async function() {
+    handle = true;
+    await agent.get('/').expect(200, /injected content/);
+  });
+  it('authn fail, hook defers (undefined) -> 401', async function() {
+    returnUndef = true;
+    await agent.get('/').expect(401);
+  });
+  it('authn fail, hook defers (empty list) -> 401', async function() {
+    await agent.get('/').expect(401);
+  });
+  it('authz fail, hook handles -> 200', async function() {
+    handle = true;
+    await agent.get('/').auth('user', 'user-password').expect(200, /injected content/);
+  });
+  it('authz fail, hook defers (undefined) -> 403', async function() {
+    returnUndef = true;
+    await agent.get('/').auth('user', 'user-password').expect(403);
+  });
+  it('authz fail, hook defers (empty list) -> 403', async function() {
+    await agent.get('/').auth('user', 'user-password').expect(403);
+  });
+});