From 55587bc24dc9e9ad8249d876154b2afe6d1db543 Mon Sep 17 00:00:00 2001 From: Jimmy Domagala-Tang Date: Tue, 2 Jul 2024 08:10:29 -0400 Subject: [PATCH] Merge pull request #19152 from overleaf/jdt-project-permissions Allow checking permissions for all users on a project and rename checkPermissions -> AssertPermissions GitOrigin-RevId: 511356cf2fe68367e284347e68e59f6116bd0f80 --- .../Authorization/PermissionsController.js | 4 +- .../Authorization/PermissionsManager.js | 61 ++++++++++++++++--- .../PasswordReset/PasswordResetHandler.js | 6 +- .../Authorization/PermissionsManagerTests.js | 14 ++--- .../PasswordResetHandlerTests.js | 4 +- 5 files changed, 67 insertions(+), 22 deletions(-) diff --git a/services/web/app/src/Features/Authorization/PermissionsController.js b/services/web/app/src/Features/Authorization/PermissionsController.js index 921fbd406c..e8d4aff809 100644 --- a/services/web/app/src/Features/Authorization/PermissionsController.js +++ b/services/web/app/src/Features/Authorization/PermissionsController.js @@ -5,7 +5,7 @@ const { combineGroupPolicies, combineAllowedProperties, } = require('./PermissionsManager') -const { checkUserPermissions } = require('./PermissionsManager').promises +const { assertUserPermissions } = require('./PermissionsManager').promises const Modules = require('../../infrastructure/Modules') const { expressify } = require('@overleaf/promise-utils') @@ -84,7 +84,7 @@ function requirePermission(...requiredCapabilities) { return next(new Error('no user')) } try { - await checkUserPermissions(req.user, requiredCapabilities) + await assertUserPermissions(req.user, requiredCapabilities) next() } catch (error) { next(error) diff --git a/services/web/app/src/Features/Authorization/PermissionsManager.js b/services/web/app/src/Features/Authorization/PermissionsManager.js index ade3f317c6..fca40443d4 100644 --- a/services/web/app/src/Features/Authorization/PermissionsManager.js +++ b/services/web/app/src/Features/Authorization/PermissionsManager.js @@ -370,8 +370,9 @@ async function getUserValidationStatus({ user, groupPolicy, subscription }) { } /** - * Checks if a user has permission for a given set of capabilities - * as set out in both their current group subscription, and any institutions they are affiliated with + * asserts that a user has permission for a given set of capabilities + * as set out in both their current group subscription, and any institutions they are affiliated with, + * throwing an ForbiddenError if they do not * * @param {Object} user - The user object to retrieve the group policy for. * Only the user's _id is required @@ -379,11 +380,31 @@ async function getUserValidationStatus({ user, groupPolicy, subscription }) { * @returns {Promise} * @throws {Error} If the user does not have permission */ +async function assertUserPermissions(user, requiredCapabilities) { + const hasAllPermissions = await checkUserPermissions( + user, + requiredCapabilities + ) + if (!hasAllPermissions) { + throw new ForbiddenError( + `user does not have one or more permissions within ${requiredCapabilities}` + ) + } +} + +/** + * Checks if a user has permission for a given set of capabilities + * as set out in both their current group subscription, and any institutions they are affiliated with + * + * @param {Object} user - The user object to retrieve the group policy for. + * Only the user's _id is required + * @param {Array} capabilities - The list of the capabilities to check permission for. + * @returns {Promise} - true if the user has all permissions, false if not + */ async function checkUserPermissions(user, requiredCapabilities) { let results = await Modules.promises.hooks.fire('getGroupPolicyForUser', user) results = results.flat() - - if (!results?.length) return + if (!results?.length) return true // get the combined group policy applying to the user const groupPolicies = results.map(result => result.groupPolicy) @@ -391,11 +412,30 @@ async function checkUserPermissions(user, requiredCapabilities) { for (const requiredCapability of requiredCapabilities) { // if the user has the permission, continue if (!hasPermission(combinedGroupPolicy, requiredCapability)) { - throw new ForbiddenError( - `user does not have permission for ${requiredCapability}` - ) + return false } } + return true +} + +/** + * checks if all collaborators of a given project have the specified capability, including the owner + * + * @async + * @function checkCollaboratorsPermission + * @param {string} userList - An array of all user to check permissions for + * @param {Array} capabilities - The list of the capabilities to check permission for. + * @returns {Promise} - A promise that resolves to `true` if all collaborators have the specified capability, otherwise `false`. + */ +async function checkUserListPermissions(userList, capabilities) { + for (const user of userList) { + // mimic a user object with only id, since we need it to fetch permissions + const allowed = await checkUserPermissions(user, capabilities) + if (!allowed) { + return false + } + } + return true } module.exports = { @@ -409,5 +449,10 @@ module.exports = { getUserCapabilities, getUserRestrictions, getUserValidationStatus: callbackify(getUserValidationStatus), - promises: { checkUserPermissions, getUserValidationStatus }, + checkCollaboratorsPermission: callbackify(checkUserListPermissions), + promises: { + assertUserPermissions, + getUserValidationStatus, + checkUserListPermissions, + }, } diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index dbc2cf79a1..73cded6d13 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -5,7 +5,7 @@ const OneTimeTokenHandler = require('../Security/OneTimeTokenHandler') const EmailHandler = require('../Email/EmailHandler') const AuthenticationManager = require('../Authentication/AuthenticationManager') const { callbackify, promisify } = require('util') -const { checkUserPermissions } = +const { assertUserPermissions } = require('../Authorization/PermissionsManager').promises const AUDIT_LOG_TOKEN_PREFIX_LENGTH = 10 @@ -21,7 +21,7 @@ async function generateAndEmailResetToken(email) { return 'secondary' } - await checkUserPermissions(user, ['change-password']) + await assertUserPermissions(user, ['change-password']) const data = { user_id: user._id.toString(), email } const token = await OneTimeTokenHandler.promises.getNewToken('password', data) @@ -72,7 +72,7 @@ async function getUserForPasswordResetToken(token) { email: 1, }) - await checkUserPermissions(user, ['change-password']) + await assertUserPermissions(user, ['change-password']) if (user == null) { return { user: null, remainingPeeks: 0 } diff --git a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js index bae8d0a862..469c00e0ad 100644 --- a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js +++ b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js @@ -399,11 +399,11 @@ describe('PermissionsManager', function () { ) }) }) - describe('checkUserPermissions', function () { + describe('assertUserPermissions', function () { describe('allowed', function () { it('should not error when managedUsersEnabled is not enabled for user', async function () { const result = - await this.PermissionsManager.promises.checkUserPermissions( + await this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['add-secondary-email'] ) @@ -423,7 +423,7 @@ describe('PermissionsManager', function () { ], ]) const result = - await this.PermissionsManager.promises.checkUserPermissions( + await this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['some-policy-to-check'] ) @@ -448,7 +448,7 @@ describe('PermissionsManager', function () { ], ]) const result = - await this.PermissionsManager.promises.checkUserPermissions( + await this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['some-policy-to-check'] ) @@ -460,7 +460,7 @@ describe('PermissionsManager', function () { it('should return error when managedUsersEnabled is enabled for user but there is no group policy', async function () { this.hooksFire.resolves([[{ managedUsersEnabled: true }]]) await expect( - this.PermissionsManager.promises.checkUserPermissions( + this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['add-secondary-email'] ) @@ -480,7 +480,7 @@ describe('PermissionsManager', function () { ], ]) await expect( - this.PermissionsManager.promises.checkUserPermissions( + this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['some-policy-to-check'] ) @@ -503,7 +503,7 @@ describe('PermissionsManager', function () { ], ]) await expect( - this.PermissionsManager.promises.checkUserPermissions( + this.PermissionsManager.promises.assertUserPermissions( { _id: 'user123' }, ['some-policy-to-check'] ) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 4dc9583ddc..9a69cba994 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -47,7 +47,7 @@ describe('PasswordResetHandler', function () { '@overleaf/settings': this.settings, '../Authorization/PermissionsManager': (this.PermissionsManager = { promises: { - checkUserPermissions: sinon.stub(), + assertUserPermissions: sinon.stub(), }, }), }, @@ -537,7 +537,7 @@ describe('PasswordResetHandler', function () { it('should returns errors from user permissions', async function () { let error const err = new Error('nope') - this.PermissionsManager.promises.checkUserPermissions.rejects(err) + this.PermissionsManager.promises.assertUserPermissions.rejects(err) try { await this.PasswordResetHandler.promises.getUserForPasswordResetToken( 'abc123'