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

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)

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<void>}
  * @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<Boolean>} - 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<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) {
+    // 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,
+  },
 }

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 }

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']
           )

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'