Merge pull request #13662 from overleaf/bg-managed-users-fix-subscription-validator

fix subscription validator for managed users

GitOrigin-RevId: 765c1c11850090f57327fc8b4255d41a16514472

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

@@ -24,10 +24,10 @@
  * capability will be removed. A policy can remove more than one capability, and
  * more than one policy could apply to a user.
  *
- * Validator: a function that takes a user and returns a boolean indicating
- * whether the user satisfies the policy or not. For example, a validator for
- * the `userCannotHaveSecondaryEmail` policy would check whether the user has
- * more than one email address.
+ * Validator: a function that takes an object with user and subscription properties
+ * and returns a boolean indicating whether the user satisfies the policy or not.
+ * For example, a validator for the `userCannotHaveSecondaryEmail` policy would
+ * check whether the user has more than one email address.
  *
  * Group Policies: a collection of policies with a setting indicating whether
  * they are enforced or not. Used to place restrictions on managed users in a
@@ -265,8 +265,8 @@ function hasPermission(groupPolicy, capability) {
 
 /**
  * Asynchronously checks which policies a user complies with using the
- * applicable validators. Each validator is an async function that takes a user
- * and returns a boolean.
+ * applicable validators. Each validator is an async function that takes a object
+ * with user and subscription properties and returns a boolean.
  *
  * @param {Object} user - The user object to check.
  * @param {Object} groupPolicy - The group policy object to check.
@@ -275,7 +275,7 @@ function hasPermission(groupPolicy, capability) {
  *   enforced policy names, and the values are booleans indicating whether the
  *   user complies with the policy.
  */
-async function getUserValidationStatus(user, groupPolicy) {
+async function getUserValidationStatus(user, groupPolicy, subscription) {
   // find all the enforced policies for the user
   const enforcedPolicyNames = getEnforcedPolicyNames(groupPolicy)
   // for each enforced policy, we have a list of capabilities with expected values
@@ -285,7 +285,10 @@ async function getUserValidationStatus(user, groupPolicy) {
   for (const enforcedPolicyName of enforcedPolicyNames) {
     const validator = getValidatorFromPolicy(enforcedPolicyName)
     if (validator) {
-      userValidationStatus.set(enforcedPolicyName, await validator(user))
+      userValidationStatus.set(
+        enforcedPolicyName,
+        await validator({ user, subscription })
+      )
     }
   }
   return userValidationStatus

services/web/app/src/Features/Subscription/ManagedUsersPolicy.js

@@ -2,7 +2,8 @@ const {
   registerCapability,
   registerPolicy,
 } = require('../Authorization/PermissionsManager')
-const SubscriptionLocator = require('./SubscriptionLocator')
+const { getUsersSubscription, getGroupSubscriptionsMemberOf } =
+  require('./SubscriptionLocator').promises
 
 // This file defines the capabilities and policies that are used to
 // determine what managed users can and cannot do.
@@ -48,7 +49,7 @@ registerPolicy(
     'endorse-email': false,
   },
   {
-    validator: async user => {
+    validator: async ({ user }) => {
       // return true if the user does not have any secondary emails
       return user.emails.length === 1
     },
@@ -66,7 +67,7 @@ registerPolicy(
   { 'link-google-sso': false },
   {
     // return true if the user does not have Google SSO linked
-    validator: async user =>
+    validator: async ({ user }) =>
       !user.thirdPartyIdentifiers?.some(
         identifier => identifier.providerId === 'google'
       ),
@@ -79,22 +80,31 @@ registerPolicy(
   { 'link-other-third-party-sso': false },
   {
     // return true if the user does not have any other third party SSO linked
-    validator: async user =>
+    validator: async ({ user }) =>
       !user.thirdPartyIdentifiers?.some(
         identifier => identifier.providerId !== 'google'
       ),
   }
 )
 
-// Register a policy to prevent a user having an active personal subscription.
+// Register a policy to prevent a user having an active subscription or
+// being a member of another group subscription.
 registerPolicy(
   'userCannotHaveSubscription',
   { 'start-subscription': false, 'join-subscription': false },
   {
-    validator: async user => {
-      return !(await SubscriptionLocator.promises.getUserIndividualSubscription(
+    validator: async ({ user, subscription }) => {
+      const usersSubscription = await getUsersSubscription(user)
+      const userHasSubscription = Boolean(usersSubscription)
+      const userMemberOfSubscriptions = await getGroupSubscriptionsMemberOf(
         user
-      ))
+      )
+      // filter out the subscription of the managed group itself
+      // the user will always be a member of this subscription
+      const isMemberOfOtherSubscriptions = userMemberOfSubscriptions.some(
+        sub => sub._id.toString() !== subscription._id.toString()
+      )
+      return !userHasSubscription && !isMemberOfOtherSubscriptions
     },
   }
 )

services/web/test/acceptance/src/helpers/Subscription.js

@@ -74,8 +74,13 @@ class Subscription {
     return PermissionsManager.getUserCapabilities(groupPolicy)
   }
 
-  getUserValidationStatus(user, groupPolicy, callback) {
-    PermissionsManager.getUserValidationStatus(user, groupPolicy, callback)
+  getUserValidationStatus(user, groupPolicy, subscription, callback) {
+    PermissionsManager.getUserValidationStatus(
+      user,
+      groupPolicy,
+      subscription,
+      callback
+    )
   }
 
   enrollManagedUser(user, callback) {

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

@@ -277,8 +277,8 @@ describe('PermissionsManager', function () {
         'policy',
         {},
         {
-          validator: async user => {
-            return user.prop === 'allowed'
+          validator: async ({ user, subscription }) => {
+            return user.prop === 'allowed' && subscription.prop === 'managed'
           },
         }
       )
@@ -286,10 +286,12 @@ describe('PermissionsManager', function () {
         policy: true,
       }
       const user = { prop: 'allowed' }
+      const subscription = { prop: 'managed' }
       const result =
         await this.PermissionsManager.promises.getUserValidationStatus(
           user,
-          groupPolicy
+          groupPolicy,
+          subscription
         )
       expect(result).to.deep.equal(new Map([['policy', true]]))
     })
@@ -299,8 +301,8 @@ describe('PermissionsManager', function () {
         'policy',
         {},
         {
-          validator: async user => {
-            return user.prop === 'allowed'
+          validator: async ({ user, subscription }) => {
+            return user.prop === 'allowed' && subscription.prop === 'managed'
           },
         }
       )
@@ -308,10 +310,12 @@ describe('PermissionsManager', function () {
         policy: true,
       }
       const user = { prop: 'not allowed' }
+      const subscription = { prop: 'managed' }
       const result =
         await this.PermissionsManager.promises.getUserValidationStatus(
           user,
-          groupPolicy
+          groupPolicy,
+          subscription
         )
       expect(result).to.deep.equal(new Map([['policy', false]]))
     })
@@ -320,8 +324,8 @@ describe('PermissionsManager', function () {
         'policy1',
         {},
         {
-          validator: async user => {
-            return user.prop === 'allowed'
+          validator: async ({ user, subscription }) => {
+            return user.prop === 'allowed' && subscription.prop === 'managed'
           },
         }
       )
@@ -329,8 +333,8 @@ describe('PermissionsManager', function () {
         'policy2',
         {},
         {
-          validator: async user => {
-            return user.prop === 'other'
+          validator: async ({ user, subscription }) => {
+            return user.prop === 'other' && subscription.prop === 'managed'
           },
         }
       )
@@ -338,8 +342,8 @@ describe('PermissionsManager', function () {
         'policy3',
         {},
         {
-          validator: async user => {
-            return user.prop === 'allowed'
+          validator: async ({ user, subscription }) => {
+            return user.prop === 'allowed' && subscription.prop === 'managed'
           },
         }
       )
@@ -350,10 +354,12 @@ describe('PermissionsManager', function () {
         policy3: false, // this policy is not enforced
       }
       const user = { prop: 'allowed' }
+      const subscription = { prop: 'managed' }
       const result =
         await this.PermissionsManager.promises.getUserValidationStatus(
           user,
-          groupPolicy
+          groupPolicy,
+          subscription
         )
       expect(result).to.deep.equal(
         new Map([