Deduplicate users in checkUserListPermissions to avoid redundant permission checks (#29461)

* Fix duplicate permission checks for same user

Deduplicate user list in checkUserListPermissions before running expensive checks.
Handles ObjectId vs string comparison by converting to string.
Adds tests to verify deduplication works correctly.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: thomas- <2176518+thomas-@users.noreply.github.com>
GitOrigin-RevId: 96eede1cbeb18b807deaca7d4c370aef5c48c4bc

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<boolean>} - 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) {

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)
+    })
   })
 })