From 53fd0b4f98bacbd075c4b8a1315d45dc3769c727 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 26 Aug 2020 22:08:07 -0400 Subject: [PATCH] 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 | 3 +- src/node/hooks/express/webaccess.js | 34 +++--- src/node/server.js | 2 +- tests/backend/specs/webaccess.js | 166 ++++++++++++++++++++++++++++ 4 files changed, 188 insertions(+), 17 deletions(-) create mode 100644 tests/backend/specs/webaccess.js diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md index dd0b8599e..370e782ed 100644 --- a/doc/api/hooks_server-side.md +++ b/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: diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index b83fbbd00..fe5a40535 100644 --- a/src/node/hooks/express/webaccess.js +++ b/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(); }; diff --git a/src/node/server.js b/src/node/server.js index c9ef33cc9..a8a567179 100755 --- a/src/node/server.js +++ b/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'); diff --git a/tests/backend/specs/webaccess.js b/tests/backend/specs/webaccess.js new file mode 100644 index 000000000..8367429f0 --- /dev/null +++ b/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); + }); +});