Merge pull request #17289 from overleaf/jel-permissions-controller

[web] Move user permissions check to manager

GitOrigin-RevId: 8c59d053da3d8d452cd424b04baa05f5d7d9057a

services/web/app/src/Features/Authorization/PermissionsController.js

@@ -1,9 +1,9 @@
 const { ForbiddenError, UserNotFoundError } = require('../Errors/Errors')
 const {
-  hasPermission,
   getUserCapabilities,
   getUserRestrictions,
 } = require('./PermissionsManager')
+const { checkUserPermissions } = require('./PermissionsManager').promises
 const Modules = require('../../infrastructure/Modules')
 
 /**
@@ -76,26 +76,7 @@ function requirePermission(...requiredCapabilities) {
       return next(new Error('no user'))
     }
     try {
-      const result =
-        (
-          await Modules.promises.hooks.fire(
-            'getManagedUsersEnrollmentForUser',
-            req.user
-          )
-        )[0] || {}
-      const { groupPolicy, managedUsersEnabled } = result
-      if (!managedUsersEnabled) {
-        return next()
-      }
-      // check that the user has all the required capabilities
-      for (const requiredCapability of requiredCapabilities) {
-        // if the user has the permission, continue
-        if (!hasPermission(groupPolicy, requiredCapability)) {
-          throw new ForbiddenError(
-            `user does not have permission for ${requiredCapability}`
-          )
-        }
-      }
+      await checkUserPermissions(req.user, requiredCapabilities)
       next()
     } catch (error) {
       next(error)

services/web/app/src/Features/Authorization/PermissionsManager.js

@@ -42,6 +42,8 @@
  */
 
 const { callbackify } = require('util')
+const { ForbiddenError } = require('../Errors/Errors')
+const Modules = require('../../infrastructure/Modules')
 
 const POLICY_TO_CAPABILITY_MAP = new Map()
 const POLICY_TO_VALIDATOR_MAP = new Map()
@@ -313,6 +315,38 @@ async function getUserValidationStatus({ user, groupPolicy, subscription }) {
   return userValidationStatus
 }
 
+/**
+ * Checks if a user has permission for a given set of capabilities
+ *
+ * @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<void>}
+ * @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
+  for (const requiredCapability of requiredCapabilities) {
+    // if the user has the permission, continue
+    if (!hasPermission(groupPolicy, requiredCapability)) {
+      throw new ForbiddenError(
+        `user does not have permission for ${requiredCapability}`
+      )
+    }
+  }
+}
+
 module.exports = {
   registerCapability,
   registerPolicy,
@@ -320,5 +354,5 @@ module.exports = {
   getUserCapabilities,
   getUserRestrictions,
   getUserValidationStatus: callbackify(getUserValidationStatus),
-  promises: { getUserValidationStatus },
+  promises: { checkUserPermissions, getUserValidationStatus },
 }

services/web/test/unit/src/Authorization/PermissionsManagerTests.js

@@ -1,12 +1,22 @@
+const sinon = require('sinon')
 const { expect } = require('chai')
 const modulePath =
   '../../../../app/src/Features/Authorization/PermissionsManager.js'
 const SandboxedModule = require('sandboxed-module')
+const { ForbiddenError } = require('../../../../app/src/Features/Errors/Errors')
 
 describe('PermissionsManager', function () {
   beforeEach(function () {
     this.PermissionsManager = SandboxedModule.require(modulePath, {
-      requires: {},
+      requires: {
+        '../../infrastructure/Modules': (this.Modules = {
+          promises: {
+            hooks: {
+              fire: (this.hooksFire = sinon.stub().resolves([{}])),
+            },
+          },
+        }),
+      },
     })
     this.PermissionsManager.registerCapability('capability1', {
       default: true,
@@ -389,4 +399,109 @@ describe('PermissionsManager', function () {
       )
     })
   })
+
+  describe('checkUserPermissions', function () {
+    describe('allowed', function () {
+      it('should not error when managedUsersEnabled is not enabled for user', async function () {
+        const result =
+          await this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['add-secondary-email']
+          )
+        expect(result).to.be.undefined
+      })
+
+      it('should not error when default capability is true', async function () {
+        this.PermissionsManager.registerCapability('some-policy-to-check', {
+          default: true,
+        })
+        this.hooksFire.resolves([
+          {
+            managedUsersEnabled: true,
+            groupPolicy: {},
+          },
+        ])
+        const result =
+          await this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['some-policy-to-check']
+          )
+        expect(result).to.be.undefined
+      })
+
+      it('should not error when default permission is false but user has permission', async function () {
+        this.PermissionsManager.registerCapability('some-policy-to-check', {
+          default: false,
+        })
+        this.PermissionsManager.registerPolicy('userCanDoSomePolicy', {
+          'some-policy-to-check': true,
+        })
+        this.hooksFire.resolves([
+          {
+            managedUsersEnabled: true,
+            groupPolicy: {
+              userCanDoSomePolicy: true,
+            },
+          },
+        ])
+        const result =
+          await this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['some-policy-to-check']
+          )
+        expect(result).to.be.undefined
+      })
+    })
+
+    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 }])
+        await expect(
+          this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['add-secondary-email']
+          )
+        ).to.be.rejectedWith(Error, 'unknown capability: add-secondary-email')
+      })
+
+      it('should return error when default permission is false', async function () {
+        this.PermissionsManager.registerCapability('some-policy-to-check', {
+          default: false,
+        })
+        this.hooksFire.resolves([
+          {
+            managedUsersEnabled: true,
+            groupPolicy: {},
+          },
+        ])
+        await expect(
+          this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['some-policy-to-check']
+          )
+        ).to.be.rejectedWith(ForbiddenError)
+      })
+
+      it('should return error when default permission is true but user does not have permission', async function () {
+        this.PermissionsManager.registerCapability('some-policy-to-check', {
+          default: true,
+        })
+        this.PermissionsManager.registerPolicy('userCannotDoSomePolicy', {
+          'some-policy-to-check': false,
+        })
+        this.hooksFire.resolves([
+          {
+            managedUsersEnabled: true,
+            groupPolicy: { userCannotDoSomePolicy: true },
+          },
+        ])
+        await expect(
+          this.PermissionsManager.promises.checkUserPermissions(
+            { _id: 'user123' },
+            ['some-policy-to-check']
+          )
+        ).to.be.rejectedWith(ForbiddenError)
+      })
+    })
+  })
 })