From 918c3e7e337e0f56988d359456cd3214fca5d288 Mon Sep 17 00:00:00 2001 From: Jimmy Domagala-Tang Date: Thu, 25 Jul 2024 09:03:01 -0400 Subject: [PATCH] Merge pull request #19301 from overleaf/jdt-collaborator-ai-usage Prevent AI usage on projects where collaborators have a blocking policy GitOrigin-RevId: 93bdd9c5accff51a14f0585249e13ed7f1fa4e53 --- .../Authorization/PermissionsManager.js | 4 +- .../src/Features/Project/ProjectController.js | 44 ++++++++----- .../Authorization/PermissionsManagerTests.js | 64 +++++++++++++++++++ 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/services/web/app/src/Features/Authorization/PermissionsManager.js b/services/web/app/src/Features/Authorization/PermissionsManager.js index aafcf9f17b..49c19369ce 100644 --- a/services/web/app/src/Features/Authorization/PermissionsManager.js +++ b/services/web/app/src/Features/Authorization/PermissionsManager.js @@ -422,8 +422,8 @@ async function checkUserPermissions(user, requiredCapabilities) { * 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 + * @function checkUserListPermissions + * @param {Object[]} 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`. */ diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 0fcdb35945..94a8ee84dc 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -42,8 +42,7 @@ const PublicAccessLevels = require('../Authorization/PublicAccessLevels') const TagsHandler = require('../Tags/TagsHandler') const TutorialHandler = require('../Tutorial/TutorialHandler') const UserUpdater = require('../User/UserUpdater') -const { checkUserPermissions } = - require('../Authorization/PermissionsManager').promises +const Modules = require('../../infrastructure/Modules') const UserGetter = require('../User/UserGetter') /** @@ -616,21 +615,34 @@ const _ProjectController = { !showPersonalAccessToken && splitTestAssignments['personal-access-token'].variant === 'enabled' // `?personal-access-token=enabled` - // still allow users to access project if we cant get their permissions, but disable AI feature - let canUseAi - try { - canUseAi = await checkUserPermissions(user, ['use-ai']) - } catch (err) { - canUseAi = false - } + let showAiErrorAssistant = false + if (userId && Features.hasFeature('saas')) { + try { + // exit early if the user couldnt use ai anyways, since permissions checks are expensive + const canUseAiOnProject = + user.features?.aiErrorAssistant && + (privilegeLevel === PrivilegeLevels.READ_AND_WRITE || + privilegeLevel === PrivilegeLevels.OWNER) - const showAiErrorAssistant = - userId && - Features.hasFeature('saas') && - user.features?.aiErrorAssistant && - canUseAi && - (privilegeLevel === PrivilegeLevels.READ_AND_WRITE || - privilegeLevel === PrivilegeLevels.OWNER) + if (canUseAiOnProject) { + // check permissions for user and project owner, to see if they allow AI on the project + const permissionsResults = await Modules.promises.hooks.fire( + 'projectAllowsCapability', + project, + userId, + ['use-ai'] + ) + const aiAllowed = permissionsResults.every( + result => result === true + ) + + showAiErrorAssistant = aiAllowed + } + } catch (err) { + // still allow users to access project if we cant get their permissions, but disable AI feature + showAiErrorAssistant = false + } + } const template = detachRole === 'detached' diff --git a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js index 469c00e0ad..bbf499d147 100644 --- a/services/web/test/unit/src/Authorization/PermissionsManagerTests.js +++ b/services/web/test/unit/src/Authorization/PermissionsManagerTests.js @@ -30,6 +30,38 @@ describe('PermissionsManager', function () { this.PermissionsManager.registerCapability('capability4', { default: false, }) + this.PermissionsManager.registerPolicy('openPolicy', { + capability1: true, + capability2: true, + }) + this.PermissionsManager.registerPolicy('restrictivePolicy', { + capability1: true, + capability2: false, + }) + this.openPolicyResponseSet = [ + [ + { + managedUsersEnabled: true, + groupPolicy: { openPolicy: true }, + }, + { + managedUsersEnabled: true, + groupPolicy: { openPolicy: true }, + }, + ], + ] + this.restrictivePolicyResponseSet = [ + [ + { + managedUsersEnabled: true, + groupPolicy: { openPolicy: true }, + }, + { + managedUsersEnabled: true, + groupPolicy: { restrictivePolicy: true }, + }, + ], + ] }) describe('hasPermission', function () { @@ -717,4 +749,36 @@ describe('PermissionsManager', function () { }) }) }) + + describe('checkUserListPermissions', function () { + it('should return true when all users have permissions required', async function () { + const userList = ['user1', 'user2', 'user3'] + const capabilities = ['capability1', 'capability2'] + this.hooksFire.onCall(0).resolves(this.openPolicyResponseSet) + this.hooksFire.onCall(1).resolves(this.openPolicyResponseSet) + this.hooksFire.onCall(2).resolves(this.openPolicyResponseSet) + + const usersHavePermission = + await this.PermissionsManager.promises.checkUserListPermissions( + userList, + capabilities + ) + expect(usersHavePermission).to.equal(true) + }) + + it('should return false if any user does not have permission', async function () { + const userList = ['user1', 'user2', 'user3'] + const capabilities = ['capability1', 'capability2'] + this.hooksFire.onCall(0).resolves(this.openPolicyResponseSet) + this.hooksFire.onCall(1).resolves(this.restrictivePolicyResponseSet) + this.hooksFire.onCall(2).resolves(this.openPolicyResponseSet) + + const usersHavePermission = + await this.PermissionsManager.promises.checkUserListPermissions( + userList, + capabilities + ) + expect(usersHavePermission).to.equal(false) + }) + }) })