From 52465d779e8cd9e249407c2e31136f7470985ce8 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 19 Jul 2023 15:05:27 +0100 Subject: [PATCH] Merge pull request #13907 from overleaf/bg-managed-users-allow-cancelled-subscriptions allow cancelled subscriptions for managed users GitOrigin-RevId: 56262ce4bd4cc93d4e5ea92222c76a874d6cad1e --- .../Authorization/PermissionsController.js | 12 ++- .../Subscription/ManagedUsersHandler.js | 21 +++-- .../Subscription/ManagedUsersPolicy.js | 50 +++++++++-- .../Features/Subscription/RecurlyClient.js | 6 ++ .../Subscription/SubscriptionController.js | 10 +++ .../Subscription/SubscriptionRouter.js | 1 + .../acceptance/src/helpers/Subscription.js | 4 +- .../SubscriptionControllerTests.js | 84 +++++++++++++++---- 8 files changed, 153 insertions(+), 35 deletions(-) diff --git a/services/web/app/src/Features/Authorization/PermissionsController.js b/services/web/app/src/Features/Authorization/PermissionsController.js index 978337c866..db7d55dc25 100644 --- a/services/web/app/src/Features/Authorization/PermissionsController.js +++ b/services/web/app/src/Features/Authorization/PermissionsController.js @@ -27,8 +27,12 @@ function useCapabilities() { } try { // get the group policy applying to the user - const groupPolicy = - await ManagedUsersHandler.promises.getGroupPolicyForUser(req.user) + const { groupPolicy, managedBy, isManagedGroupAdmin } = + await ManagedUsersHandler.promises.getEnrollmentForUser(req.user) + // attach the subscription ID to the request object + req.managedBy = managedBy + // attach the subscription admin status to the request object + req.isManagedGroupAdmin = isManagedGroupAdmin // attach the new capabilities to the request object for (const cap of getUserCapabilities(groupPolicy)) { req.capabilitySet.add(cap) @@ -66,8 +70,8 @@ function requirePermission(...requiredCapabilities) { } try { // get the group policy applying to the user - const groupPolicy = - await ManagedUsersHandler.promises.getGroupPolicyForUser(req.user) + const { groupPolicy } = + await ManagedUsersHandler.promises.getEnrollmentForUser(req.user) // if there is no group policy, the user is not managed if (!groupPolicy) { return next() diff --git a/services/web/app/src/Features/Subscription/ManagedUsersHandler.js b/services/web/app/src/Features/Subscription/ManagedUsersHandler.js index f85df06255..6f99614413 100644 --- a/services/web/app/src/Features/Subscription/ManagedUsersHandler.js +++ b/services/web/app/src/Features/Subscription/ManagedUsersHandler.js @@ -42,9 +42,9 @@ async function enableManagedUsers(subscriptionId) { * @function * @param {Object} user - The user object to retrieve the group policy for. * @returns {Promise} - A Promise that resolves with the group policy - * object for the user's enrollment, or undefined if it does not exist. + * and subscription objects for the user's enrollment, or null if it does not exist. */ -async function getGroupPolicyForUser(requestedUser) { +async function getEnrollmentForUser(requestedUser) { // Don't rely on the user being populated, it may be a session user without // the enrollment property. Force the user to be loaded from mongo. const user = await User.findById(requestedUser._id, 'enrollment') @@ -53,7 +53,7 @@ async function getGroupPolicyForUser(requestedUser) { } // Now we are sure the user exists and we have the full contents if (user.enrollment?.managedBy == null) { - return + return {} } // retrieve the subscription and the group policy (without the _id field) const subscription = await Subscription.findById(user.enrollment.managedBy) @@ -64,11 +64,20 @@ async function getGroupPolicyForUser(requestedUser) { info: { subscriptionId: user.enrollment.managedBy, userId: user._id }, }) } + + // check whether the user is an admin of the subscription + const isManagedGroupAdmin = user._id.equals(subscription.admin_id) + // return the group policy as a plain object (without the __v field) const groupPolicy = subscription.groupPolicy.toObject({ versionKey: false, }) - return groupPolicy + + return { + groupPolicy, + managedBy: user.enrollment.managedBy, + isManagedGroupAdmin, + } } async function enrollInSubscription(userId, subscription) { @@ -109,10 +118,10 @@ async function enrollInSubscription(userId, subscription) { module.exports = { promises: { enableManagedUsers, - getGroupPolicyForUser, + getEnrollmentForUser, enrollInSubscription, }, enableManagedUsers: callbackify(enableManagedUsers), - getGroupPolicyForUser: callbackify(getGroupPolicyForUser), + getEnrollmentForUser: callbackify(getEnrollmentForUser), enrollInSubscription: callbackify(enrollInSubscription), } diff --git a/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js b/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js index a75d403ee0..82b979ef38 100644 --- a/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js +++ b/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js @@ -4,6 +4,7 @@ const { } = require('../Authorization/PermissionsManager') const { getUsersSubscription, getGroupSubscriptionsMemberOf } = require('./SubscriptionLocator').promises +const { subscriptionIsCanceledOrExpired } = require('./RecurlyClient') // This file defines the capabilities and policies that are used to // determine what managed users can and cannot do. @@ -35,6 +36,9 @@ registerCapability('start-subscription', { default: true }) // Register the capability for a user to join a subscription. registerCapability('join-subscription', { default: true }) +// Register the capability for a user to reactivate a subscription. +registerCapability('reactivate-subscription', { default: true }) + // Register a policy to prevent a user deleting their own account. registerPolicy('userCannotDeleteOwnAccount', { 'delete-own-account': false, @@ -91,20 +95,54 @@ registerPolicy( // being a member of another group subscription. registerPolicy( 'userCannotHaveSubscription', - { 'start-subscription': false, 'join-subscription': false }, + { + 'start-subscription': false, + 'join-subscription': false, + 'reactivate-subscription': false, + }, { validator: async ({ user, subscription }) => { const usersSubscription = await getUsersSubscription(user) - const userHasSubscription = Boolean(usersSubscription) + + // The user can be enrolled if: + // 1. they do not have a subscription and are not a member of another subscription (apart from the managed group subscription) + // 2. they have a subscription and it is canceled or expired + // 3. they have a subscription and it is the subscription they are trying to join as a managed user + // The last case is to allow the admin to join their own subscription as a managed user + + const userHasSubscription = + Boolean(usersSubscription) && + !subscriptionIsCanceledOrExpired(usersSubscription) + + const userIsThisGroupAdmin = + Boolean(usersSubscription) && + usersSubscription._id.toString() === subscription._id.toString() + 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() + sub => { + // ignore the subscription of the managed group itself + if (sub._id.toString() === subscription._id.toString()) { + return false + } + // ignore the user's own subscription + if ( + usersSubscription && + sub._id.toString() === usersSubscription._id.toString() + ) { + return false + } + return true + } + ) + + return ( + (!userHasSubscription || userIsThisGroupAdmin) && + !isMemberOfOtherSubscriptions ) - return !userHasSubscription && !isMemberOfOtherSubscriptions }, } ) diff --git a/services/web/app/src/Features/Subscription/RecurlyClient.js b/services/web/app/src/Features/Subscription/RecurlyClient.js index ab8f7c7db3..3c4002209e 100644 --- a/services/web/app/src/Features/Subscription/RecurlyClient.js +++ b/services/web/app/src/Features/Subscription/RecurlyClient.js @@ -90,6 +90,11 @@ async function cancelSubscriptionByUuid(subscriptionUuid) { } } +function subscriptionIsCanceledOrExpired(subscription) { + const state = subscription?.recurlyStatus?.state + return state === 'canceled' || state === 'expired' +} + module.exports = { errors: recurly.errors, @@ -102,6 +107,7 @@ module.exports = { removeSubscriptionChangeByUuid: callbackify(removeSubscriptionChangeByUuid), reactivateSubscriptionByUuid: callbackify(reactivateSubscriptionByUuid), cancelSubscriptionByUuid: callbackify(cancelSubscriptionByUuid), + subscriptionIsCanceledOrExpired, promises: { getSubscription, diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index 352162747a..312c244003 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -574,6 +574,16 @@ function updateAccountEmailAddress(req, res, next) { function reactivateSubscription(req, res, next) { const user = SessionManager.getSessionUser(req.session) logger.debug({ userId: user._id }, 'reactivating subscription') + try { + if (req.isManagedGroupAdmin) { + // allow admins to reactivate subscriptions + } else { + // otherwise require the user to have the reactivate-subscription permission + req.assertPermission('reactivate-subscription') + } + } catch (error) { + return next(error) + } SubscriptionHandler.reactivateSubscription(user, function (err) { if (err) { OError.tag(err, 'something went wrong reactivating subscription', { diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index 503ec9b3b1..3795d17c8c 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -111,6 +111,7 @@ module.exports = { webRouter.post( '/user/subscription/reactivate', AuthenticationController.requireLogin(), + PermissionsController.useCapabilities(), SubscriptionController.reactivateSubscription ) diff --git a/services/web/test/acceptance/src/helpers/Subscription.js b/services/web/test/acceptance/src/helpers/Subscription.js index 800e28c0ff..d5b9beaa59 100644 --- a/services/web/test/acceptance/src/helpers/Subscription.js +++ b/services/web/test/acceptance/src/helpers/Subscription.js @@ -66,8 +66,8 @@ class Subscription { ManagedUsersHandler.enableManagedUsers(this._id, callback) } - getGroupPolicyForUser(user, callback) { - ManagedUsersHandler.getGroupPolicyForUser(user, callback) + getEnrollmentForUser(user, callback) { + ManagedUsersHandler.getEnrollmentForUser(user, callback) } getCapabilities(groupPolicy) { diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index 0da31405b0..88db5404b9 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -653,26 +653,76 @@ describe('SubscriptionController', function () { }) describe('reactivateSubscription', function () { - beforeEach(function (done) { - this.res = { - redirect() { + describe('when the user has permission', function () { + beforeEach(function (done) { + this.res = { + redirect() { + done() + }, + } + this.req.assertPermission = sinon.stub() + this.next = sinon.stub().callsFake(error => { + done(error) + }) + sinon.spy(this.res, 'redirect') + this.SubscriptionController.reactivateSubscription( + this.req, + this.res, + this.next + ) + }) + + it('should assert the user has permission to reactivate their subscription', function (done) { + this.req.assertPermission + .calledWith('reactivate-subscription') + .should.equal(true) + done() + }) + + it('should tell the handler to reactivate this user', function (done) { + this.SubscriptionHandler.reactivateSubscription + .calledWith(this.user) + .should.equal(true) + done() + }) + + it('should redurect to the subscription page', function (done) { + this.res.redirect.calledWith('/user/subscription').should.equal(true) + done() + }) + }) + + describe('when the user does not have permission', function () { + beforeEach(function (done) { + this.res = { + redirect() { + done() + }, + } + this.req.assertPermission = sinon.stub().throws() + this.next = sinon.stub().callsFake(() => { done() - }, - } - sinon.spy(this.res, 'redirect') - this.SubscriptionController.reactivateSubscription(this.req, this.res) - }) + }) + sinon.spy(this.res, 'redirect') + this.SubscriptionController.reactivateSubscription( + this.req, + this.res, + this.next + ) + }) - it('should tell the handler to reactivate this user', function (done) { - this.SubscriptionHandler.reactivateSubscription - .calledWith(this.user) - .should.equal(true) - done() - }) + it('should not reactivate the user', function (done) { + this.req.assertPermission = sinon.stub().throws() + this.SubscriptionHandler.reactivateSubscription.called.should.equal( + false + ) + done() + }) - it('should redurect to the subscription page', function (done) { - this.res.redirect.calledWith('/user/subscription').should.equal(true) - done() + it('should call next with an error', function (done) { + this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) + done() + }) }) })