From 867fdbd3f92c2d11901b8ca92848cc463167f5f3 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 18 Nov 2020 17:31:18 -0500 Subject: [PATCH] webaccess: Asyncify `checkAccess` --- src/node/hooks/express/webaccess.js | 231 +++++++++++++--------------- 1 file changed, 108 insertions(+), 123 deletions(-) diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 18b2f589d..51d57ae2e 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -48,32 +48,33 @@ exports.userCanModify = (padId, req) => { // Exported so that tests can set this to 0 to avoid unnecessary test slowness. exports.authnFailureDelayMs = 1000; -const checkAccess = (req, res, next) => { - const hookResultMangle = (cb) => { - return (err, data) => { - if (err != null) httpLogger.error(`Error during access check: ${err}`); - return cb(!err && data.length && data[0]); - }; - }; +const checkAccess = async (req, res, next) => { + // Promisified wrapper around hooks.aCallFirst. + const aCallFirst = (hookName, context, pred = null) => new Promise((resolve, reject) => { + hooks.aCallFirst(hookName, context, (err, r) => err != null ? reject(err) : resolve(r), pred); + }); + + const aCallFirst0 = + async (hookName, context, pred = null) => (await aCallFirst(hookName, context, pred))[0]; 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) => { + // 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). + const authorize = async () => { const grant = (level) => { level = exports.normalizeAuthzLevel(level); - if (!level) return fail(); + if (!level) return false; const user = req.session.user; - if (user == null) return next(); // This will happen if authentication is not required. + if (user == null) return true; // This will happen if authentication is not required. const encodedPadId = (req.path.match(/^\/p\/([^/]*)/) || [])[1]; - if (encodedPadId == null) return next(); + if (encodedPadId == null) return true; const padId = decodeURIComponent(encodedPadId); // The user was granted access to a pad. Remember the authorization level in the user's // settings so that SecurityManager can approve or deny specific actions. if (user.padAuthorizations == null) user.padAuthorizations = {}; user.padAuthorizations[padId] = level; - return next(); + return true; }; const isAuthenticated = req.session && req.session.user; if (isAuthenticated && req.session.user.is_admin) return grant('create'); @@ -82,126 +83,110 @@ const checkAccess = (req, res, next) => { 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)); + return grant(await aCallFirst0('authorize', {req, res, next, resource: req.path})); }; - // Access checking is done in four steps: - // - // 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. - // 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. - // 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 preAuthzFailure, authnFailure, and - // authzFailure hooks to override the default error handling behavior (e.g., to redirect to a - // login page). + // /////////////////////////////////////////////////////////////////////////////////////////////// + // Step 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. Plugins can + // use the preAuthzFailure hook to override the default 403 error. + // /////////////////////////////////////////////////////////////////////////////////////////////// - let step1PreAuthorize, step2PreAuthenticate, step3Authenticate, step4Authorize; + let results; + try { + results = await aCallFirst('preAuthorize', {req, res, next}, + // This 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. + (r) => (r != null && r.filter((x) => (!requireAdmin || !x)).length > 0)); + } catch (err) { + httpLogger.error(`Error in preAuthorize hook: ${err.stack || err.toString()}`); + return res.status(500).send('Internal Server Error'); + } + if (staticPathsRE.test(req.path)) 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(); + if (await aCallFirst0('preAuthzFailure', {req, res})) return; + // No plugin handled the pre-authentication authorization failure. + return res.status(403).send('Forbidden'); + } - 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'); - } - if (req.path.match(staticPathsRE)) 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); - }; + // /////////////////////////////////////////////////////////////////////////////////////////////// + // Step 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. + // /////////////////////////////////////////////////////////////////////////////////////////////// - step2PreAuthenticate = () => authorize(step3Authenticate); + if (await authorize()) return next(); - 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 - // given to authn plugins. - const httpBasicAuth = - req.headers.authorization && req.headers.authorization.search('Basic ') === 0; - if (httpBasicAuth) { - const userpass = - Buffer.from(req.headers.authorization.split(' ')[1], 'base64').toString().split(':'); - ctx.username = userpass.shift(); - ctx.password = userpass.join(':'); + // /////////////////////////////////////////////////////////////////////////////////////////////// + // Step 3: Authenticate the user. (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. Plugins can use the + // authnFailure hook to override the default error handling behavior (e.g., to redirect to a login + // page). + // /////////////////////////////////////////////////////////////////////////////////////////////// + + 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 given + // to authn plugins. + const httpBasicAuth = + req.headers.authorization && req.headers.authorization.search('Basic ') === 0; + if (httpBasicAuth) { + const userpass = + Buffer.from(req.headers.authorization.split(' ')[1], 'base64').toString().split(':'); + ctx.username = userpass.shift(); + ctx.password = userpass.join(':'); + } + if (!(await aCallFirst0('authenticate', ctx))) { + // Fall back to HTTP basic auth. + const {[ctx.username]: {password} = {}} = settings.users; + if (!httpBasicAuth || password == null || password !== ctx.password) { + httpLogger.info(`Failed authentication from IP ${req.ip}`); + if (await aCallFirst0('authnFailure', {req, res})) return; + if (await aCallFirst0('authFailure', {req, res, next})) 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. + await new Promise((resolve) => setTimeout(resolve, exports.authnFailureDelayMs)); + res.status(401).send('Authentication Required'); + return; } - hooks.aCallFirst('authenticate', ctx, hookResultMangle((ok) => { - if (!ok) { - // Fall back to HTTP basic auth. - const {[ctx.username]: {password} = {}} = settings.users; - if (!httpBasicAuth || password == null || password !== ctx.password) { - httpLogger.info(`Failed authentication from IP ${req.ip}`); - return hooks.aCallFirst('authnFailure', {req, res}, hookResultMangle((ok) => { - if (ok) return; - 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'); - }, exports.authnFailureDelayMs); - })); - })); - } - settings.users[ctx.username].username = ctx.username; - // Make a shallow copy so that the password property can be deleted (to prevent it from - // appearing in logs or in the database) without breaking future authentication attempts. - req.session.user = {...settings.users[ctx.username]}; - delete req.session.user.password; - } - if (req.session.user == null) { - httpLogger.error('authenticate hook failed to add user settings to session'); - res.status(500).send('Internal Server Error'); - return; - } - let username = req.session.user.username; - username = (username != null) ? username : ''; - httpLogger.info(`Successful authentication from IP ${req.ip} for username ${username}`); - step4Authorize(); - })); - }; + settings.users[ctx.username].username = ctx.username; + // Make a shallow copy so that the password property can be deleted (to prevent it from + // appearing in logs or in the database) without breaking future authentication attempts. + req.session.user = {...settings.users[ctx.username]}; + delete req.session.user.password; + } + if (req.session.user == null) { + httpLogger.error('authenticate hook failed to add user settings to session'); + return res.status(500).send('Internal Server Error'); + } + const {username = ''} = req.session.user; + httpLogger.info(`Successful authentication from IP ${req.ip} for user ${username}`); - step4Authorize = () => authorize(() => { - return hooks.aCallFirst('authzFailure', {req, res}, hookResultMangle((ok) => { - if (ok) return; - return hooks.aCallFirst('authFailure', {req, res, next}, hookResultMangle((ok) => { - if (ok) return; - // No plugin handled the authorization failure. - res.status(403).send('Forbidden'); - })); - })); - }); + // /////////////////////////////////////////////////////////////////////////////////////////////// + // Step 4: Try to access the thing again. If this fails, give the user a 403 error. Plugins can + // use the authzFailure hook to override the default error handling behavior (e.g., to redirect to + // a login page). + // /////////////////////////////////////////////////////////////////////////////////////////////// - step1PreAuthorize(); + if (await authorize()) return next(); + if (await aCallFirst0('authzFailure', {req, res})) return; + if (await aCallFirst0('authFailure', {req, res, next})) return; + // No plugin handled the authorization failure. + res.status(403).send('Forbidden'); }; exports.expressConfigure = (hookName, args, cb) => { - args.app.use(checkAccess); + args.app.use((req, res, next) => { checkAccess(req, res, next).catch(next); }); return cb(); };