diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md index 95ad6c1c..18de6b03 100644 --- a/doc/api/hooks_server-side.md +++ b/doc/api/hooks_server-side.md @@ -308,7 +308,7 @@ Things in context: This hook is called to handle authentication. Plugins that supply an authenticate function should probably also supply an -authFailure function unless falling back to HTTP basic authentication is +authnFailure function unless falling back to HTTP basic authentication is appropriate upon authentication failure. This hook is only called if either the `requireAuthentication` setting is true @@ -359,10 +359,12 @@ Things in context: 2. res - the response object 3. next - ? +**DEPRECATED:** Use authnFailure or authzFailure instead. + This hook is called to handle an authentication or authorization failure. Plugins that supply an authenticate function should probably also supply an -authFailure function unless falling back to HTTP basic authentication is +authnFailure function unless falling back to HTTP basic authentication is appropriate upon authentication failure. A plugin's authFailure function is only called if all of the following are true: @@ -370,6 +372,10 @@ A plugin's authFailure function is only called if all of the following are true: * There was an authentication or authorization failure. * The failure was not already handled by an authFailure function from another plugin. +* For authentication failures: The failure was not already handled by the + authnFailure hook. +* For authorization failures: The failure was not already handled by the + authzFailure hook. Calling the provided callback with `[true]` tells Etherpad that the failure was handled and no further error handling is required. Calling the callback with @@ -389,6 +395,70 @@ exports.authFailure = (hookName, context, cb) => { }; ``` +## authnFailure +Called from: src/node/hooks/express/webaccess.js + +Things in context: + +1. req - the request object +2. res - the response object + +This hook is called to handle an authentication failure. + +Plugins that supply an authenticate function should probably also supply an +authnFailure function unless falling back to HTTP basic authentication is +appropriate upon authentication failure. + +A plugin's authnFailure function is only called if the authentication failure +was not already handled by an authnFailure function from another plugin. + +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 an authnFailure function from +another plugin (if any, otherwise fall back to the deprecated authFailure hook). + +Example: + +``` +exports.authnFailure = (hookName, context, cb) => { + if (notApplicableToThisPlugin(context)) return cb([]); + context.res.redirect(makeLoginURL(context.req)); + return cb([true]); +}; +``` + +## authzFailure +Called from: src/node/hooks/express/webaccess.js + +Things in context: + +1. req - the request object +2. res - the response object + +This hook is called to handle an authorization failure. + +A plugin's authzFailure function is only called if the authorization failure was +not already handled by an authzFailure function from another plugin. + +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 an authzFailure function from +another plugin (if any, otherwise fall back to the deprecated authFailure hook). + +Example: + +``` +exports.authzFailure = (hookName, context, cb) => { + if (notApplicableToThisPlugin(context)) return cb([]); + if (needsPremiumAccount(context.req) && !context.req.session.user.premium) { + context.res.status(200).send(makeUpgradeToPremiumAccountPage(context.req)); + return cb([true]); + } + // Use the generic 403 forbidden response. + return cb([]); +}; +``` + ## handleMessage Called from: src/node/handler/PadMessageHandler.js diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 01ba8a92..1dfa2412 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -8,6 +8,8 @@ const stats = require('ep_etherpad-lite/node/stats'); const sessionModule = require('express-session'); const cookieParser = require('cookie-parser'); +hooks.deprecationNotices.authFailure = 'use the authnFailure and authzFailure hooks instead'; + exports.normalizeAuthzLevel = (level) => { if (!level) return false; switch (level) { @@ -70,8 +72,8 @@ exports.checkAccess = (req, res, next) => { // 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 - // error handling behavior (e.g., to redirect to a login page). + // 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). let step1PreAuthenticate, step2Authenticate, step3Authorize; @@ -93,14 +95,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) => { + return hooks.aCallFirst('authnFailure', {req, res}, 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); + 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. @@ -127,10 +132,13 @@ exports.checkAccess = (req, res, next) => { }; step3Authorize = () => authorize(() => { - return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => { + return hooks.aCallFirst('authzFailure', {req, res}, hookResultMangle((ok) => { if (ok) return; - // No plugin handled the authorization failure. - res.status(403).send('Forbidden'); + return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => { + if (ok) return; + // No plugin handled the authorization failure. + res.status(403).send('Forbidden'); + })); })); }); diff --git a/tests/backend/specs/webaccess.js b/tests/backend/specs/webaccess.js index 8367429f..029b41dd 100644 --- a/tests/backend/specs/webaccess.js +++ b/tests/backend/specs/webaccess.js @@ -95,30 +95,43 @@ describe('webaccess without any plugins', function() { }); }); -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]); +describe('webaccess with authnFailure, authzFailure, authFailure hooks', function() { + const Handler = class { + constructor(hookName) { + this.hookName = hookName; + this.shouldHandle = false; + this.called = false; + } + handle(hookName, context, cb) { + assert.equal(hookName, this.hookName); + assert(context != null); + assert(context.req != null); + assert(context.res != null); + assert(!this.called); + this.called = true; + if (this.shouldHandle) { + context.res.status(200).send(this.hookName); + return cb([true]); + } + return cb([]); } - if (returnUndef) return cb(); - return cb([]); }; - + const handlers = {}; + const hookNames = ['authnFailure', 'authzFailure', 'authFailure']; const settingsBackup = {}; - let authFailureHooksBackup; - before(function() { + const hooksBackup = {}; + + beforeEach(function() { Object.assign(settingsBackup, settings); - authFailureHooksBackup = plugins.hooks.authFailure; - plugins.hooks.authFailure = [{hook_fn: authFailure}]; + hookNames.forEach((hookName) => { + if (plugins.hooks[hookName] == null) plugins.hooks[hookName] = []; + }); + Object.assign(hooksBackup, plugins.hooks); + hookNames.forEach((hookName) => { + const handler = new Handler(hookName); + handlers[hookName] = handler; + plugins.hooks[hookName] = [{hook_fn: handler.handle.bind(handler)}]; + }); settings.requireAuthentication = true; settings.requireAuthorization = true; settings.users = { @@ -126,41 +139,66 @@ describe('webaccess with authFailure plugin', function() { 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); + Object.assign(settings, settingsBackup); + Object.assign(plugins.hooks, hooksBackup); }); - 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; + // authn failure tests + it('authn fail, no hooks handle -> 401', async function() { await agent.get('/').expect(401); + assert(handlers['authnFailure'].called); + assert(!handlers['authzFailure'].called); + assert(handlers['authFailure'].called); }); - it('authn fail, hook defers (empty list) -> 401', async function() { - await agent.get('/').expect(401); + it('authn fail, authnFailure handles', async function() { + handlers['authnFailure'].shouldHandle = true; + await agent.get('/').expect(200, 'authnFailure'); + assert(handlers['authnFailure'].called); + assert(!handlers['authzFailure'].called); + assert(!handlers['authFailure'].called); }); - it('authz fail, hook handles -> 200', async function() { - handle = true; - await agent.get('/').auth('user', 'user-password').expect(200, /injected content/); + it('authn fail, authFailure handles', async function() { + handlers['authFailure'].shouldHandle = true; + await agent.get('/').expect(200, 'authFailure'); + assert(handlers['authnFailure'].called); + assert(!handlers['authzFailure'].called); + assert(handlers['authFailure'].called); }); - it('authz fail, hook defers (undefined) -> 403', async function() { - returnUndef = true; + it('authnFailure trumps authFailure', async function() { + handlers['authnFailure'].shouldHandle = true; + handlers['authFailure'].shouldHandle = true; + await agent.get('/').expect(200, 'authnFailure'); + assert(handlers['authnFailure'].called); + assert(!handlers['authFailure'].called); + }); + + // authz failure tests + it('authz fail, no hooks handle -> 403', async function() { await agent.get('/').auth('user', 'user-password').expect(403); + assert(!handlers['authnFailure'].called); + assert(handlers['authzFailure'].called); + assert(handlers['authFailure'].called); }); - it('authz fail, hook defers (empty list) -> 403', async function() { - await agent.get('/').auth('user', 'user-password').expect(403); + it('authz fail, authzFailure handles', async function() { + handlers['authzFailure'].shouldHandle = true; + await agent.get('/').auth('user', 'user-password').expect(200, 'authzFailure'); + assert(!handlers['authnFailure'].called); + assert(handlers['authzFailure'].called); + assert(!handlers['authFailure'].called); + }); + it('authz fail, authFailure handles', async function() { + handlers['authFailure'].shouldHandle = true; + await agent.get('/').auth('user', 'user-password').expect(200, 'authFailure'); + assert(!handlers['authnFailure'].called); + assert(handlers['authzFailure'].called); + assert(handlers['authFailure'].called); + }); + it('authzFailure trumps authFailure', async function() { + handlers['authzFailure'].shouldHandle = true; + handlers['authFailure'].shouldHandle = true; + await agent.get('/').auth('user', 'user-password').expect(200, 'authzFailure'); + assert(handlers['authzFailure'].called); + assert(!handlers['authFailure'].called); }); });