From 3c71e8983bcd967728e360f49b75c2321f7afe41 Mon Sep 17 00:00:00 2001 From: pcworld <0188801@gmail.com> Date: Sun, 11 Apr 2021 03:59:52 +0200 Subject: [PATCH] Fix read only pad access with authentication Before this commit, webaccess.checkAccess saved the authorization in user.padAuthorizations[padId] with padId being the read-only pad ID, however later stages, e.g. in PadMessageHandler, use the real pad ID for access checks. This led to authorization being denied. This commit fixes it by only storing and comparing the real pad IDs and not read-only pad IDs. This fixes test case "authn user readonly pad -> 200, ok" in src/tests/backend/specs/socketio.js. --- doc/api/hooks_server-side.md | 6 ++++-- src/node/db/SecurityManager.js | 10 ++++++++++ src/node/handler/PadMessageHandler.js | 10 +--------- src/node/hooks/express/webaccess.js | 21 +++++++++++++-------- 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md index e13adfa97..32a09ecf4 100644 --- a/doc/api/hooks_server-side.md +++ b/doc/api/hooks_server-side.md @@ -156,11 +156,13 @@ Called from: src/node/db/SecurityManager.js Things in context: -1. padID - the pad the user wants to access +1. padID - the real ID (never the read-only ID) of the pad the user wants to + access 2. token - the token of the author 3. sessionCookie - the session the use has -This hook gets called when the access to the concrete pad is being checked. Return `false` to deny access. +This hook gets called when the access to the concrete pad is being checked. +Return `false` to deny access. ## padCreate Called from: src/node/db/Pad.js diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index 24d966d18..4851866d5 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -22,6 +22,7 @@ const authorManager = require('./AuthorManager'); const hooks = require('../../static/js/pluginfw/hooks.js'); const padManager = require('./PadManager'); +const readOnlyManager = require('./ReadOnlyManager'); const sessionManager = require('./SessionManager'); const settings = require('../utils/Settings'); const webaccess = require('../hooks/express/webaccess'); @@ -56,6 +57,15 @@ exports.checkAccess = async (padID, sessionCookie, token, userSettings) => { let canCreate = !settings.editOnly; + if (readOnlyManager.isReadOnlyId(padID)) { + canCreate = false; + padID = await readOnlyManager.getPadId(padID); + if (padID == null) { + authLogger.debug('access denied: read-only pad ID for a pad that does not exist'); + return DENY; + } + } + // Authentication and authorization checks. if (settings.loadTest) { console.warn( diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index d2f6fc7ba..0e6869e88 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -218,17 +218,9 @@ exports.handleMessage = async (socket, message) => { return; } - // check if pad is requested via readOnly - let padId = auth.padID; - - if (padId.indexOf('r.') === 0) { - // Pad is readOnly, first get the real Pad ID - padId = await readOnlyManager.getPadId(padId); - } - const {session: {user} = {}} = socket.client.request; const {accessStatus, authorID} = - await securityManager.checkAccess(padId, auth.sessionID, auth.token, user); + await securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, user); if (accessStatus !== 'grant') { // Access denied. Send the reason to the user. socket.json.send({accessStatus}); diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 5ff957a52..99210d9c5 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -70,14 +70,19 @@ const checkAccess = async (req, res, next) => { // 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) => { + const grant = async (level) => { level = exports.normalizeAuthzLevel(level); if (!level) return false; const user = req.session.user; if (user == null) return true; // This will happen if authentication is not required. const encodedPadId = (req.path.match(/^\/p\/([^/]*)/) || [])[1]; if (encodedPadId == null) return true; - const padId = decodeURIComponent(encodedPadId); + let padId = decodeURIComponent(encodedPadId); + if (readOnlyManager.isReadOnlyId(padId)) { + // pad is read-only, first get the real pad ID + padId = await readOnlyManager.getPadId(padId); + if (padId == null) return false; + } // 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 = {}; @@ -85,13 +90,13 @@ const checkAccess = async (req, res, next) => { return true; }; const isAuthenticated = req.session && req.session.user; - if (isAuthenticated && req.session.user.is_admin) return grant('create'); + if (isAuthenticated && req.session.user.is_admin) return await grant('create'); const requireAuthn = requireAdmin || settings.requireAuthentication; - if (!requireAuthn) return grant('create'); - if (!isAuthenticated) return grant(false); - if (requireAdmin && !req.session.user.is_admin) return grant(false); - if (!settings.requireAuthorization) return grant('create'); - return grant(await aCallFirst0('authorize', {req, res, next, resource: req.path})); + if (!requireAuthn) return await grant('create'); + if (!isAuthenticated) return await grant(false); + if (requireAdmin && !req.session.user.is_admin) return await grant(false); + if (!settings.requireAuthorization) return await grant('create'); + return await grant(await aCallFirst0('authorize', {req, res, next, resource: req.path})); }; // ///////////////////////////////////////////////////////////////////////////////////////////////