diff --git a/services/web/app/src/Features/Authorization/PermissionsManager.mjs b/services/web/app/src/Features/Authorization/PermissionsManager.mjs index f35d8c4469..85384810ed 100644 --- a/services/web/app/src/Features/Authorization/PermissionsManager.mjs +++ b/services/web/app/src/Features/Authorization/PermissionsManager.mjs @@ -42,6 +42,7 @@ */ import { callbackify } from 'node:util' +import _ from 'lodash' import Errors from '../Errors/Errors.js' import Modules from '../../infrastructure/Modules.mjs' @@ -459,7 +460,11 @@ async function checkUserPermissions(user, requiredCapabilities) { * @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) { + // Deduplicate the user list to avoid checking the same user multiple times + // Convert _id to string for comparison since it can be ObjectId or string + const uniqueUsers = _.uniqBy(userList, user => String(user._id)) + + for (const user of uniqueUsers) { // mimic a user object with only id, since we need it to fetch permissions const allowed = await checkUserPermissions(user, capabilities) if (!allowed) { diff --git a/services/web/test/unit/src/Authorization/PermissionsManager.test.mjs b/services/web/test/unit/src/Authorization/PermissionsManager.test.mjs index b14506c99c..125120263b 100644 --- a/services/web/test/unit/src/Authorization/PermissionsManager.test.mjs +++ b/services/web/test/unit/src/Authorization/PermissionsManager.test.mjs @@ -797,7 +797,7 @@ describe('PermissionsManager', function () { describe('checkUserListPermissions', function () { it('should return true when all users have permissions required', async function (ctx) { - const userList = ['user1', 'user2', 'user3'] + const userList = [{ _id: 'user1' }, { _id: 'user2' }, { _id: 'user3' }] const capabilities = ['capability1', 'capability2'] ctx.hooksFire.onCall(0).resolves(ctx.openPolicyResponseSet) ctx.hooksFire.onCall(1).resolves(ctx.openPolicyResponseSet) @@ -812,7 +812,7 @@ describe('PermissionsManager', function () { }) it('should return false if any user does not have permission', async function (ctx) { - const userList = ['user1', 'user2', 'user3'] + const userList = [{ _id: 'user1' }, { _id: 'user2' }, { _id: 'user3' }] const capabilities = ['capability1', 'capability2'] ctx.hooksFire.onCall(0).resolves(ctx.openPolicyResponseSet) ctx.hooksFire.onCall(1).resolves(ctx.restrictivePolicyResponseSet) @@ -825,5 +825,39 @@ describe('PermissionsManager', function () { ) expect(usersHavePermission).to.equal(false) }) + + it('should deduplicate users with duplicate IDs', async function (ctx) { + const userList = [{ _id: 'user1' }, { _id: 'user1' }, { _id: 'user2' }] + const capabilities = ['capability1', 'capability2'] + ctx.hooksFire.onCall(0).resolves(ctx.openPolicyResponseSet) + ctx.hooksFire.onCall(1).resolves(ctx.openPolicyResponseSet) + + const usersHavePermission = + await ctx.PermissionsManager.promises.checkUserListPermissions( + userList, + capabilities + ) + expect(usersHavePermission).to.equal(true) + // Should only call hooksFire twice (once per unique user), not three times + expect(ctx.hooksFire.callCount).to.equal(2) + }) + + it('should deduplicate users when _id is ObjectId vs string', async function (ctx) { + const mongodb = await import('mongodb-legacy') + const ObjectId = mongodb.default.ObjectId + const userId = new ObjectId() + const userList = [{ _id: userId }, { _id: userId.toString() }] + const capabilities = ['capability1', 'capability2'] + ctx.hooksFire.onCall(0).resolves(ctx.openPolicyResponseSet) + + const usersHavePermission = + await ctx.PermissionsManager.promises.checkUserListPermissions( + userList, + capabilities + ) + expect(usersHavePermission).to.equal(true) + // Should only call hooksFire once since both users are the same + expect(ctx.hooksFire.callCount).to.equal(1) + }) }) })