From 271700893a73d0fac6d6a3fe009b6f7bcc6d1144 Mon Sep 17 00:00:00 2001 From: Jimmy Domagala-Tang Date: Thu, 20 Jun 2024 12:59:26 -0400 Subject: [PATCH] Merge pull request #18784 from overleaf/bg-allow-combined-group-policies allow combined group policies GitOrigin-RevId: b23fb0454f794e9094e8e15e732b4322a48ac1ee --- .../Authorization/PermissionsController.js | 36 +-- .../Authorization/PermissionsManager.js | 81 +++++- services/web/app/src/models/GroupPolicy.js | 3 + services/web/app/src/models/Institution.js | 1 + services/web/app/src/models/User.js | 1 + .../Authorization/PermissionsManagerTests.js | 253 ++++++++++++++++-- 6 files changed, 327 insertions(+), 48 deletions(-) diff --git a/services/web/app/src/Features/Authorization/PermissionsController.js b/services/web/app/src/Features/Authorization/PermissionsController.js index 40f23135df..921fbd406c 100644 --- a/services/web/app/src/Features/Authorization/PermissionsController.js +++ b/services/web/app/src/Features/Authorization/PermissionsController.js @@ -2,6 +2,8 @@ const { ForbiddenError, UserNotFoundError } = require('../Errors/Errors') const { getUserCapabilities, getUserRestrictions, + combineGroupPolicies, + combineAllowedProperties, } = require('./PermissionsManager') const { checkUserPermissions } = require('./PermissionsManager').promises const Modules = require('../../infrastructure/Modules') @@ -27,25 +29,29 @@ function useCapabilities() { return next() } try { - const result = ( - await Modules.promises.hooks.fire( - 'getManagedUsersEnrollmentForUser', - req.user - ) - )[0] - if (result) { - // get the group policy applying to the user - const { groupPolicy, managedBy, isManagedGroupAdmin } = result - // attach the subscription ID to the request object - req.managedBy = managedBy - // attach the subscription admin status to the request object - req.isManagedGroupAdmin = isManagedGroupAdmin + let results = await Modules.promises.hooks.fire( + 'getGroupPolicyForUser', + req.user + ) + // merge array of all results from all modules + results = results.flat() + + if (results.length > 0) { + // get the combined group policy applying to the user + const groupPolicies = results.map(result => result.groupPolicy) + const combinedGroupPolicy = combineGroupPolicies(groupPolicies) // attach the new capabilities to the request object - for (const cap of getUserCapabilities(groupPolicy)) { + for (const cap of getUserCapabilities(combinedGroupPolicy)) { req.capabilitySet.add(cap) } // also attach the user's restrictions (the capabilities they don't have) - req.userRestrictions = getUserRestrictions(groupPolicy) + req.userRestrictions = getUserRestrictions(combinedGroupPolicy) + + // attach allowed properties to the request object + const allowedProperties = combineAllowedProperties(results) + for (const [prop, value] of Object.entries(allowedProperties)) { + req[prop] = value + } } next() } catch (error) { diff --git a/services/web/app/src/Features/Authorization/PermissionsManager.js b/services/web/app/src/Features/Authorization/PermissionsManager.js index 4edaaeb0e4..ade3f317c6 100644 --- a/services/web/app/src/Features/Authorization/PermissionsManager.js +++ b/services/web/app/src/Features/Authorization/PermissionsManager.js @@ -48,6 +48,7 @@ const Modules = require('../../infrastructure/Modules') const POLICY_TO_CAPABILITY_MAP = new Map() const POLICY_TO_VALIDATOR_MAP = new Map() const DEFAULT_PERMISSIONS = new Map() +const ALLOWED_PROPERTIES = new Set() /** * Throws an error if the given capability is not registered. @@ -126,6 +127,24 @@ function registerPolicy(name, capabilities, options = {}) { } } +/** + * Registers an allowed property that can be added to the request object. + * + * @param {string} name - The name of the property to register. + * @returns {void} + */ +function registerAllowedProperty(name) { + ALLOWED_PROPERTIES.add(name) +} + +/** + * returns the set of allowed properties that have been registered + * + * @returns {Set} ALLOWED_PROPERTIES + */ +function getAllowedProperties() { + return ALLOWED_PROPERTIES +} /** * Returns an array of policy names that are enforced based on the provided * group policy object. @@ -240,6 +259,41 @@ function getUserCapabilities(groupPolicy) { return userCapabilities } +/** + * Combines an array of group policies into a single policy object. + * + * @param {Array} groupPolicies - An array of group policies. + * @returns {Object} - The combined group policy object. + */ +function combineGroupPolicies(groupPolicies) { + const combinedGroupPolicy = {} + for (const groupPolicy of groupPolicies) { + const enforcedPolicyNames = getEnforcedPolicyNames(groupPolicy) + for (const enforcedPolicyName of enforcedPolicyNames) { + combinedGroupPolicy[enforcedPolicyName] = true + } + } + return combinedGroupPolicy +} + +/** + * Combines the allowed properties from an array of property objects. + * + * @param {Array} propertyObjects - An array of property objects. + * @returns {Object} - An object containing the combined allowed properties. + */ +function combineAllowedProperties(propertyObjects) { + const userProperties = {} + for (const properties of propertyObjects) { + for (const [key, value] of Object.entries(properties)) { + if (ALLOWED_PROPERTIES.has(key)) { + userProperties[key] ??= value + } + } + } + return userProperties +} + /** * Returns a set of capabilities that a user does not have based on their group policy. * @@ -317,6 +371,7 @@ 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 * * @param {Object} user - The user object to retrieve the group policy for. * Only the user's _id is required @@ -325,21 +380,17 @@ async function getUserValidationStatus({ user, groupPolicy, subscription }) { * @throws {Error} If the user does not have permission */ async function checkUserPermissions(user, requiredCapabilities) { - const result = - ( - await Modules.promises.hooks.fire( - 'getManagedUsersEnrollmentForUser', - user - ) - )[0] || {} - const { groupPolicy, managedUsersEnabled } = result - if (!managedUsersEnabled) { - return - } - // check that the user has all the required capabilities + let results = await Modules.promises.hooks.fire('getGroupPolicyForUser', user) + results = results.flat() + + if (!results?.length) return + + // get the combined group policy applying to the user + const groupPolicies = results.map(result => result.groupPolicy) + const combinedGroupPolicy = combineGroupPolicies(groupPolicies) for (const requiredCapability of requiredCapabilities) { // if the user has the permission, continue - if (!hasPermission(groupPolicy, requiredCapability)) { + if (!hasPermission(combinedGroupPolicy, requiredCapability)) { throw new ForbiddenError( `user does not have permission for ${requiredCapability}` ) @@ -350,6 +401,10 @@ async function checkUserPermissions(user, requiredCapabilities) { module.exports = { registerCapability, registerPolicy, + registerAllowedProperty, + combineGroupPolicies, + combineAllowedProperties, + getAllowedProperties, hasPermission, getUserCapabilities, getUserRestrictions, diff --git a/services/web/app/src/models/GroupPolicy.js b/services/web/app/src/models/GroupPolicy.js index f421374993..ffbc3a9ada 100644 --- a/services/web/app/src/models/GroupPolicy.js +++ b/services/web/app/src/models/GroupPolicy.js @@ -21,6 +21,9 @@ const GroupPolicySchema = new Schema( // User can't have other third-party SSO (e.g. ORCID/IEEE) active on their account, nor can they link it to their account userCannotHaveOtherThirdPartySSO: Boolean, + + // User can't use any of our AI features, such as the compile-assistant + userCannotUseAIFeatures: Boolean, }, { minimize: false } ) diff --git a/services/web/app/src/models/Institution.js b/services/web/app/src/models/Institution.js index 5672000d89..2fc2c34bfa 100644 --- a/services/web/app/src/models/Institution.js +++ b/services/web/app/src/models/Institution.js @@ -14,6 +14,7 @@ const InstitutionSchema = new Schema( optedOutUserIds: [{ type: ObjectId, ref: 'User' }], lastSent: { type: Date }, }, + groupPolicy: { type: ObjectId, ref: 'GroupPolicy' }, }, { minimize: false } ) diff --git a/services/web/app/src/models/User.js b/services/web/app/src/models/User.js index 3f85d506aa..503abf7fc5 100644 --- a/services/web/app/src/models/User.js +++ b/services/web/app/src/models/User.js @@ -156,6 +156,7 @@ const UserSchema = new Schema( zotero: { type: Boolean }, referencesSearch: { type: Boolean }, symbolPalette: { type: Boolean }, + compileAssistant: { type: Boolean }, }, }, ], diff --git a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js index 1c79446604..bae8d0a862 100644 --- a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js +++ b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js @@ -12,7 +12,7 @@ describe('PermissionsManager', function () { '../../infrastructure/Modules': (this.Modules = { promises: { hooks: { - fire: (this.hooksFire = sinon.stub().resolves([{}])), + fire: (this.hooksFire = sinon.stub().resolves([[]])), }, }, }), @@ -399,7 +399,6 @@ describe('PermissionsManager', function () { ) }) }) - describe('checkUserPermissions', function () { describe('allowed', function () { it('should not error when managedUsersEnabled is not enabled for user', async function () { @@ -416,10 +415,12 @@ describe('PermissionsManager', function () { default: true, }) this.hooksFire.resolves([ - { - managedUsersEnabled: true, - groupPolicy: {}, - }, + [ + { + managedUsersEnabled: true, + groupPolicy: {}, + }, + ], ]) const result = await this.PermissionsManager.promises.checkUserPermissions( @@ -437,12 +438,14 @@ describe('PermissionsManager', function () { 'some-policy-to-check': true, }) this.hooksFire.resolves([ - { - managedUsersEnabled: true, - groupPolicy: { - userCanDoSomePolicy: true, + [ + { + managedUsersEnabled: true, + groupPolicy: { + userCanDoSomePolicy: true, + }, }, - }, + ], ]) const result = await this.PermissionsManager.promises.checkUserPermissions( @@ -455,7 +458,7 @@ describe('PermissionsManager', function () { describe('not allowed', function () { it('should return error when managedUsersEnabled is enabled for user but there is no group policy', async function () { - this.hooksFire.resolves([{ managedUsersEnabled: true }]) + this.hooksFire.resolves([[{ managedUsersEnabled: true }]]) await expect( this.PermissionsManager.promises.checkUserPermissions( { _id: 'user123' }, @@ -469,10 +472,12 @@ describe('PermissionsManager', function () { default: false, }) this.hooksFire.resolves([ - { - managedUsersEnabled: true, - groupPolicy: {}, - }, + [ + { + managedUsersEnabled: true, + groupPolicy: {}, + }, + ], ]) await expect( this.PermissionsManager.promises.checkUserPermissions( @@ -490,10 +495,12 @@ describe('PermissionsManager', function () { 'some-policy-to-check': false, }) this.hooksFire.resolves([ - { - managedUsersEnabled: true, - groupPolicy: { userCannotDoSomePolicy: true }, - }, + [ + { + managedUsersEnabled: true, + groupPolicy: { userCannotDoSomePolicy: true }, + }, + ], ]) await expect( this.PermissionsManager.promises.checkUserPermissions( @@ -504,4 +511,210 @@ describe('PermissionsManager', function () { }) }) }) + + describe('registerAllowedProperty', function () { + it('allows us to register a property', async function () { + this.PermissionsManager.registerAllowedProperty('metadata1') + const result = await this.PermissionsManager.getAllowedProperties() + expect(result).to.deep.equal(new Set(['metadata1'])) + }) + + // used if multiple modules would require the same prop, since we dont know which will load first, both must register + it('should handle multiple registrations of the same property', async function () { + this.PermissionsManager.registerAllowedProperty('metadata1') + this.PermissionsManager.registerAllowedProperty('metadata1') + const result = await this.PermissionsManager.getAllowedProperties() + expect(result).to.deep.equal(new Set(['metadata1'])) + }) + }) + + describe('combineAllowedProperties', function () { + it('should handle multiple occurences of the same property, preserving the first occurence', async function () { + const policy1 = { + groupPolicy: { + policy: false, + }, + prop1: 'some other value here', + } + const policy2 = { + groupPolicy: { + policy: false, + }, + prop1: 'some value here', + } + + const results = [policy1, policy2] + this.PermissionsManager.registerAllowedProperty('prop1') + + const combinedProps = + this.PermissionsManager.combineAllowedProperties(results) + + expect(combinedProps).to.deep.equal({ + prop1: 'some other value here', + }) + }) + + it('should add registered properties to the set', async function () { + const policy = { + groupPolicy: { + policy: false, + }, + prop1: 'some value here', + propNotMeThough: 'dont copy please', + } + + const policy2 = { + groupPolicy: { + policy: false, + }, + prop2: 'some value here', + } + + const results = [policy, policy2] + this.PermissionsManager.registerAllowedProperty('prop1') + this.PermissionsManager.registerAllowedProperty('prop2') + + const combinedProps = + this.PermissionsManager.combineAllowedProperties(results) + + expect(combinedProps).to.deep.equal({ + prop1: 'some value here', + prop2: 'some value here', + }) + }) + + it('should not add unregistered properties to the req object', async function () { + const policy = { + groupPolicy: { + policy: false, + }, + prop1: 'some value here', + } + + const policy2 = { + groupPolicy: { + policy: false, + }, + prop2: 'some value here', + } + this.PermissionsManager.registerAllowedProperty('prop1') + + const results = [policy, policy2] + + const combinedProps = + this.PermissionsManager.combineAllowedProperties(results) + + expect(combinedProps).to.deep.equal({ prop1: 'some value here' }) + }) + + it('should handle an empty array', async function () { + const results = [] + + const combinedProps = + this.PermissionsManager.combineAllowedProperties(results) + + expect(combinedProps).to.deep.equal({}) + }) + }) + + describe('combineGroupPolicies', function () { + it('should return an empty object when an empty array is passed', async function () { + const results = [] + + const combinedPolicy = + this.PermissionsManager.combineGroupPolicies(results) + expect(combinedPolicy).to.deep.equal({}) + }) + + it('should combine multiple group policies into a single policy object', async function () { + const groupPolicy = { + policy1: true, + } + + const groupPolicy2 = { + policy2: false, + policy3: true, + } + this.PermissionsManager.registerAllowedProperty('prop1') + + const results = [groupPolicy, groupPolicy2] + + const combinedPolicy = + this.PermissionsManager.combineGroupPolicies(results) + + expect(combinedPolicy).to.deep.equal({ + policy1: true, + policy3: true, + }) + }) + + it('should handle duplicate enforced policies across different group policies', async function () { + const groupPolicy = { + policy1: false, + policy2: true, + } + + const groupPolicy2 = { + policy2: true, + policy3: true, + } + this.PermissionsManager.registerAllowedProperty('prop1') + + const results = [groupPolicy, groupPolicy2] + + const combinedPolicy = + this.PermissionsManager.combineGroupPolicies(results) + + expect(combinedPolicy).to.deep.equal({ + policy2: true, + policy3: true, + }) + }) + + it('should handle group policies with no enforced policies', async function () { + const groupPolicy = { + policy1: false, + policy2: false, + } + + const groupPolicy2 = { + policy2: false, + policy3: true, + } + this.PermissionsManager.registerAllowedProperty('prop1') + + const results = [groupPolicy, groupPolicy2] + + const combinedPolicy = + this.PermissionsManager.combineGroupPolicies(results) + + expect(combinedPolicy).to.deep.equal({ policy3: true }) + }) + + it('should choose the stricter option between two policy values', async function () { + const groupPolicy = { + policy1: false, + policy2: true, + policy4: true, + } + + const groupPolicy2 = { + policy2: false, + policy3: true, + policy4: false, + } + this.PermissionsManager.registerAllowedProperty('prop1') + + const results = [groupPolicy, groupPolicy2] + + const combinedPolicy = + this.PermissionsManager.combineGroupPolicies(results) + + expect(combinedPolicy).to.deep.equal({ + policy2: true, + policy3: true, + policy4: true, + }) + }) + }) })