From a68518dd35894c369c19c1614f918e492c4f467d Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 5 Jul 2023 10:13:12 +0100 Subject: [PATCH] Merge pull request #13694 from overleaf/revert-13584-bg-managed-users-block-delete-own-account Revert "block account deletion by managed users" GitOrigin-RevId: ece8024b2fac16066abd36af9a9670ba483b3628 --- .../Authorization/PermissionsController.js | 34 +------------------ .../Authorization/PermissionsManager.js | 4 +-- .../Subscription/ManagedUsersPolicy.js | 21 +++--------- .../Subscription/SubscriptionRouter.js | 4 --- .../app/src/Features/User/UserController.js | 5 --- services/web/app/src/models/GroupPolicy.js | 2 +- services/web/app/src/router.js | 5 --- .../test/unit/src/User/UserControllerTests.js | 18 ---------- 8 files changed, 9 insertions(+), 84 deletions(-) diff --git a/services/web/app/src/Features/Authorization/PermissionsController.js b/services/web/app/src/Features/Authorization/PermissionsController.js index fbe5da9d21..e02130e401 100644 --- a/services/web/app/src/Features/Authorization/PermissionsController.js +++ b/services/web/app/src/Features/Authorization/PermissionsController.js @@ -1,38 +1,7 @@ const { ForbiddenError } = require('../Errors/Errors') -const { hasPermission, getUserCapabilities } = require('./PermissionsManager') +const { hasPermission } = require('./PermissionsManager') const ManagedUsersHandler = require('../Subscription/ManagedUsersHandler') -/** - * Function that returns middleware to add an `assertPermission` function to the request object to check if the user has a specific capability. - * @returns {Function} The middleware function that adds the `assertPermission` function to the request object. - */ -function useCapabilities() { - return async function (req, res, next) { - if (!req.user) { - return next() - } - try { - // get the group policy applying to the user - const groupPolicy = - await ManagedUsersHandler.promises.getGroupPolicyForUser(req.user) - // if there is no group policy, the user is not managed - if (!groupPolicy) { - return next() - } - const capabilitySet = getUserCapabilities(groupPolicy) - req.assertPermission = capability => { - if (!capabilitySet.has(capability)) { - throw new ForbiddenError( - `user does not have permission for ${capability}` - ) - } - } - next() - } catch (error) { - next(error) - } - } -} /** * Function that returns middleware to check if the user has permission to access a resource. * @param {[string]} requiredCapabilities - the capabilities required to access the resource. @@ -76,5 +45,4 @@ function requirePermission(...requiredCapabilities) { module.exports = { requirePermission, - useCapabilities, } diff --git a/services/web/app/src/Features/Authorization/PermissionsManager.js b/services/web/app/src/Features/Authorization/PermissionsManager.js index 1f7bfb99b8..6116f10c94 100644 --- a/services/web/app/src/Features/Authorization/PermissionsManager.js +++ b/services/web/app/src/Features/Authorization/PermissionsManager.js @@ -26,7 +26,7 @@ * * 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 + * the `userCannotAddSecondaryEmail` policy would check whether the user has * more than one email address. * * Group Policies: a collection of policies with a setting indicating whether @@ -37,7 +37,7 @@ * * { * "userCannotDeleteOwnAccount": true, // enforced - * "userCannotHaveSecondaryEmail": false // not enforced + * "userCannotAddSecondaryEmail": false // not enforced * } */ diff --git a/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js b/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js index c78897a481..1cf0c99c6b 100644 --- a/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js +++ b/services/web/app/src/Features/Subscription/ManagedUsersPolicy.js @@ -13,12 +13,6 @@ registerCapability('delete-own-account', { default: true }) // Register the capability for a user to add a secondary email to their account. registerCapability('add-secondary-email', { default: true }) -// Register the capability for a user to add an affiliation to their account. -registerCapability('add-affiliation', { default: true }) - -// Register the capability for a user to endorse an email address. -registerCapability('endorse-email', { default: true }) - // Register the capability for a user to sign in with Google to their account registerCapability('link-google-sso', { default: true }) @@ -26,14 +20,11 @@ registerCapability('link-google-sso', { default: true }) registerCapability('link-other-third-party-sso', { default: true }) // Register the capability for a user to leave a managed group subscription. -registerCapability('leave-group-subscription', { default: true }) +registerCapability('leave-managing-group-subscription', { default: true }) // Register the capability for a user to start a subscription. registerCapability('start-subscription', { default: true }) -// Register the capability for a user to join a subscription. -registerCapability('join-subscription', { default: true }) - // Register a policy to prevent a user deleting their own account. registerPolicy('userCannotDeleteOwnAccount', { 'delete-own-account': false, @@ -41,11 +32,9 @@ registerPolicy('userCannotDeleteOwnAccount', { // Register a policy to prevent a user having secondary email addresses on their account. registerPolicy( - 'userCannotHaveSecondaryEmail', + 'userCannotAddSecondaryEmail', { 'add-secondary-email': false, - 'add-affiliation': false, - 'endorse-email': false, }, { validator: async user => { @@ -57,7 +46,7 @@ registerPolicy( // Register a policy to prevent a user leaving the group subscription they are managed by. registerPolicy('userCannotLeaveManagingGroupSubscription', { - 'leave-group-subscription': false, + 'leave-managing-group-subscription': false, }) // Register a policy to prevent a user having third-party SSO linked to their account. @@ -89,7 +78,7 @@ registerPolicy( // Register a policy to prevent a user having an active personal subscription. registerPolicy( 'userCannotHaveSubscription', - { 'start-subscription': false, 'join-subscription': false }, + { 'start-subscription': false }, { validator: async user => { return !(await SubscriptionLocator.promises.getUserIndividualSubscription( @@ -112,7 +101,7 @@ registerPolicy( function getDefaultPolicy() { return { userCannotDeleteOwnAccount: true, - userCannotHaveSecondaryEmail: true, + userCannotAddSecondaryEmail: true, userCannotHaveSubscription: true, userCannotLeaveManagingGroupSubscription: true, userCannotHaveGoogleSSO: false, // we want to allow google SSO by default diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index 503ec9b3b1..114c389a8d 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -1,5 +1,4 @@ const AuthenticationController = require('../Authentication/AuthenticationController') -const PermissionsController = require('../Authorization/PermissionsController') const SubscriptionController = require('./SubscriptionController') const SubscriptionGroupController = require('./SubscriptionGroupController') const TeamInvitesController = require('./TeamInvitesController') @@ -59,7 +58,6 @@ module.exports = { webRouter.delete( '/subscription/group/user', AuthenticationController.requireLogin(), - PermissionsController.requirePermission('leave-group-subscription'), SubscriptionGroupController.removeSelfFromGroup ) @@ -72,7 +70,6 @@ module.exports = { '/subscription/invites/:token/', AuthenticationController.requireLogin(), RateLimiterMiddleware.rateLimit(teamInviteRateLimiter), - PermissionsController.requirePermission('join-subscription'), TeamInvitesController.acceptInvite ) @@ -90,7 +87,6 @@ module.exports = { webRouter.post( '/user/subscription/create', AuthenticationController.requireLogin(), - PermissionsController.requirePermission('start-subscription'), SubscriptionController.createSubscription ) webRouter.post( diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index ceab54cf7e..1c41b47a6a 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -194,11 +194,6 @@ async function ensureAffiliationMiddleware(req, res, next) { } catch (error) { return new Errors.UserNotFoundError({ info: { userId } }) } - try { - req.assertPermission('add-affiliation') - } catch (error) { - return next(error) - } try { await ensureAffiliation(user) } catch (error) { diff --git a/services/web/app/src/models/GroupPolicy.js b/services/web/app/src/models/GroupPolicy.js index 3251ec325f..c03dc7839a 100644 --- a/services/web/app/src/models/GroupPolicy.js +++ b/services/web/app/src/models/GroupPolicy.js @@ -8,7 +8,7 @@ const GroupPolicySchema = new Schema( userCannotDeleteOwnAccount: Boolean, // User can't add a secondary email address, or affiliation - userCannotHaveSecondaryEmail: Boolean, + userCannotAddSecondaryEmail: Boolean, // User can't have an active (currently auto-renewing) personal subscription, nor can they start one userCannotHaveSubscription: Boolean, diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index c1acb706f0..fc50d138b5 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -12,7 +12,6 @@ const UploadsRouter = require('./Features/Uploads/UploadsRouter') const metrics = require('@overleaf/metrics') const ReferalController = require('./Features/Referal/ReferalController') const AuthenticationController = require('./Features/Authentication/AuthenticationController') -const PermissionsController = require('./Features/Authorization/PermissionsController') const SessionManager = require('./Features/Authentication/SessionManager') const TagsController = require('./Features/Tags/TagsController') const NotificationsController = require('./Features/Notifications/NotificationsController') @@ -301,7 +300,6 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( '/user/emails', AuthenticationController.requireLogin(), - PermissionsController.useCapabilities(), UserController.promises.ensureAffiliationMiddleware, UserEmailsController.list ) @@ -334,7 +332,6 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/emails', AuthenticationController.requireLogin(), - PermissionsController.requirePermission('add-secondary-email'), RateLimiterMiddleware.rateLimit(rateLimiters.addEmail), CaptchaMiddleware.validateCaptcha('addEmail'), UserEmailsController.add @@ -353,7 +350,6 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/emails/endorse', AuthenticationController.requireLogin(), - PermissionsController.requirePermission('endorse-email'), RateLimiterMiddleware.rateLimit(rateLimiters.endorseEmail), UserEmailsController.endorse ) @@ -399,7 +395,6 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { '/user/delete', RateLimiterMiddleware.rateLimit(rateLimiters.deleteUser), AuthenticationController.requireLogin(), - PermissionsController.requirePermission('delete-own-account'), UserController.tryDeleteUser ) diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index c41156624a..df108a8778 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -928,18 +928,12 @@ describe('UserController', function () { ] this.Features.hasFeature.withArgs('affiliations').returns(true) this.req.query.ensureAffiliation = true - this.req.assertPermission = sinon.stub() await this.UserController.promises.ensureAffiliationMiddleware( this.req, this.res, this.next ) }) - it('should check the user has permission', function () { - expect(this.req.assertPermission).to.have.been.calledWith( - 'add-affiliation' - ) - }) it('should unflag the emails but not confirm', function () { expect( this.UserUpdater.promises.addAffiliationForNewUser @@ -966,18 +960,12 @@ describe('UserController', function () { ] this.Features.hasFeature.withArgs('affiliations').returns(true) this.req.query.ensureAffiliation = true - this.req.assertPermission = sinon.stub() await this.UserController.promises.ensureAffiliationMiddleware( this.req, this.res, this.next ) }) - it('should check the user has permission', function () { - expect(this.req.assertPermission).to.have.been.calledWith( - 'add-affiliation' - ) - }) it('should add affiliation to v1, unflag and confirm on v2', function () { expect(this.UserUpdater.promises.addAffiliationForNewUser).to.have.not .been.called @@ -1004,18 +992,12 @@ describe('UserController', function () { ] this.Features.hasFeature.withArgs('affiliations').returns(true) this.req.query.ensureAffiliation = true - this.req.assertPermission = sinon.stub() await this.UserController.promises.ensureAffiliationMiddleware( this.req, this.res, this.next ) }) - it('should check the user has permission', function () { - expect(this.req.assertPermission).to.have.been.calledWith( - 'add-affiliation' - ) - }) it('should return the error', function () { expect(this.next).to.be.calledWith(sinon.match.instanceOf(Error)) })