From f9087fabd684cd555883ef99cb207219fd34a681 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 11 Sep 2020 17:12:29 -0400 Subject: [PATCH] security: Check authentication in SecurityManager checkAccess In addition to providing defense in depth, this change makes it easier to implement future enhancements such as support for read-only users. --- src/node/db/SecurityManager.js | 10 +++++++++- src/node/handler/PadMessageHandler.js | 7 +++++-- src/node/handler/SocketIORouter.js | 4 +++- src/node/hooks/express/importexport.js | 3 ++- src/node/padaccess.js | 4 +++- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index fd381f3a8..f4c7af880 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -42,6 +42,7 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'}); * with this token then a new author object is created (including generating an author ID) and * associated with this token. * @param password is the password the user has given to access this pad. It can be null. + * @param userSettings is the settings.users[username] object (or equivalent from an authn plugin). * @return {accessStatus: grant|deny|wrongPassword|needPassword, authorID: a.xxxxxx}. The caller * must use the author ID returned in this object when making any changes associated with the * author. @@ -49,13 +50,20 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'}); * WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate * each other (which might allow them to gain privileges). */ -exports.checkAccess = async function(padID, sessionCookie, token, password) +exports.checkAccess = async function(padID, sessionCookie, token, password, userSettings) { if (!padID) { authLogger.debug('access denied: missing padID'); return DENY; } + // Make sure the user has authenticated if authentication is required. The caller should have + // already performed this check, but it is repeated here just in case. + if (settings.requireAuthentication && userSettings == null) { + authLogger.debug('access denied: authentication is required'); + return DENY; + } + // allow plugins to deny access const isFalse = (x) => x === false; if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) { diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 77f6d39ce..624db6829 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -289,7 +289,9 @@ exports.handleMessage = async function(client, message) padId = await readOnlyManager.getPadId(padId); } - let { accessStatus } = await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password); + const {session: {user} = {}} = client.client.request; + const {accessStatus} = + await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password, user); if (accessStatus !== "grant") { // no access, send the client a message that tells him why @@ -896,8 +898,9 @@ async function handleClientReady(client, message) let padIds = await readOnlyManager.getIds(message.padId); // FIXME: Allow to override readwrite access with readonly + const {session: {user} = {}} = client.client.request; const {accessStatus, authorID} = await securityManager.checkAccess( - padIds.padId, message.sessionID, message.token, message.password); + padIds.padId, message.sessionID, message.token, message.password, user); // no access, send the client a message that tells him why if (accessStatus !== "grant") { diff --git a/src/node/handler/SocketIORouter.js b/src/node/handler/SocketIORouter.js index 077a62beb..a5220d2f4 100644 --- a/src/node/handler/SocketIORouter.js +++ b/src/node/handler/SocketIORouter.js @@ -97,7 +97,9 @@ exports.setSocketIO = function(_socket) { padId = await readOnlyManager.getPadId(message.padId); } - let { accessStatus } = await securityManager.checkAccess(padId, message.sessionID, message.token, message.password); + const {session: {user} = {}} = client.client.request; + const {accessStatus} = await securityManager.checkAccess( + padId, message.sessionID, message.token, message.password, user); if (accessStatus === "grant") { // access was granted, mark the client as authorized and handle the message diff --git a/src/node/hooks/express/importexport.js b/src/node/hooks/express/importexport.js index 85ab6e129..4aa06ecb8 100644 --- a/src/node/hooks/express/importexport.js +++ b/src/node/hooks/express/importexport.js @@ -58,8 +58,9 @@ exports.expressCreateServer = function (hook_name, args, cb) { return next(); } + const {session: {user} = {}} = req; const {accessStatus, authorID} = await securityManager.checkAccess( - req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password); + req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user); if (accessStatus !== 'grant') return res.status(403).send('Forbidden'); assert(authorID); diff --git a/src/node/padaccess.js b/src/node/padaccess.js index 3449f7d16..6e294403e 100644 --- a/src/node/padaccess.js +++ b/src/node/padaccess.js @@ -3,7 +3,9 @@ var securityManager = require('./db/SecurityManager'); // checks for padAccess module.exports = async function (req, res) { try { - let accessObj = await securityManager.checkAccess(req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password); + const {session: {user} = {}} = req; + const accessObj = await securityManager.checkAccess( + req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user); if (accessObj.accessStatus === "grant") { // there is access, continue