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.
This commit is contained in:
Richard Hansen 2020-09-11 17:12:29 -04:00 committed by John McLear
parent 259b8d891d
commit f9087fabd6
5 changed files with 22 additions and 6 deletions

View file

@ -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 * with this token then a new author object is created (including generating an author ID) and
* associated with this token. * associated with this token.
* @param password is the password the user has given to access this pad. It can be null. * @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 * @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 * must use the author ID returned in this object when making any changes associated with the
* author. * 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 * 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). * 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) { if (!padID) {
authLogger.debug('access denied: missing padID'); authLogger.debug('access denied: missing padID');
return DENY; 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 // allow plugins to deny access
const isFalse = (x) => x === false; const isFalse = (x) => x === false;
if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) { if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) {

View file

@ -289,7 +289,9 @@ exports.handleMessage = async function(client, message)
padId = await readOnlyManager.getPadId(padId); 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") { if (accessStatus !== "grant") {
// no access, send the client a message that tells him why // 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); let padIds = await readOnlyManager.getIds(message.padId);
// FIXME: Allow to override readwrite access with readonly // FIXME: Allow to override readwrite access with readonly
const {session: {user} = {}} = client.client.request;
const {accessStatus, authorID} = await securityManager.checkAccess( 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 // no access, send the client a message that tells him why
if (accessStatus !== "grant") { if (accessStatus !== "grant") {

View file

@ -97,7 +97,9 @@ exports.setSocketIO = function(_socket) {
padId = await readOnlyManager.getPadId(message.padId); 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") { if (accessStatus === "grant") {
// access was granted, mark the client as authorized and handle the message // access was granted, mark the client as authorized and handle the message

View file

@ -58,8 +58,9 @@ exports.expressCreateServer = function (hook_name, args, cb) {
return next(); return next();
} }
const {session: {user} = {}} = req;
const {accessStatus, authorID} = await securityManager.checkAccess( 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'); if (accessStatus !== 'grant') return res.status(403).send('Forbidden');
assert(authorID); assert(authorID);

View file

@ -3,7 +3,9 @@ var securityManager = require('./db/SecurityManager');
// checks for padAccess // checks for padAccess
module.exports = async function (req, res) { module.exports = async function (req, res) {
try { 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") { if (accessObj.accessStatus === "grant") {
// there is access, continue // there is access, continue