From abdee0465d6d9a36ecee9411fbc121992bb6bf3e Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Wed, 9 Aug 2023 11:07:32 +0200 Subject: [PATCH] Merge pull request #14169 from overleaf/revert-14167-revert-14106-ab-ab-group-settings-admin-only Revert "Revert "[web] Restrict group settings page and managed users activation to group admin"" GitOrigin-RevId: 3e622fe3c25dfa9940351450f55c1441634fbd44 --- .../Subscription/SubscriptionController.js | 26 +++++----- .../UserMembershipAuthorization.js | 19 +++---- .../UserMembershipEntityConfigs.js | 49 +++++-------------- .../UserMembershipMiddleware.js | 14 +++++- .../web/test/acceptance/src/DeletionTests.js | 41 +++++++++++++--- 5 files changed, 80 insertions(+), 69 deletions(-) diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index 591c94f198..44a8f13893 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -138,8 +138,8 @@ async function plansPage(req, res) { } /** - * @param {import("express").Request} req - * @param {import("express").Response} res + * @param {import('express').Request} req + * @param {import('express').Response} res * @returns {Promise} */ async function paymentPage(req, res) { @@ -208,8 +208,8 @@ function formatGroupPlansDataForDash() { } /** - * @param {import("express").Request} req - * @param {import("express").Response} res + * @param {import('express').Request} req + * @param {import('express').Response} res * @returns {Promise} */ async function userSubscriptionPage(req, res) { @@ -246,9 +246,13 @@ async function userSubscriptionPage(req, res) { const groupPlansDataForDash = formatGroupPlansDataForDash() + // display the Group Settings button only to admins of group subscriptions with the Managed Users feature available const groupSettingsEnabledFor = (managedGroupSubscriptions || []) - .filter(subscription => - ManagedUsersManager.hasManagedUsersFeature(subscription) + .filter( + subscription => + ManagedUsersManager.hasManagedUsersFeature(subscription) && + (subscription.admin_id._id || subscription.admin_id).toString() === + user._id.toString() ) .map(subscription => subscription._id.toString()) @@ -399,8 +403,8 @@ async function createSubscription(req, res) { } /** - * @param {import("express").Request} req - * @param {import("express").Response} res + * @param {import('express').Request} req + * @param {import('express').Response} res * @returns {Promise} */ async function successfulSubscription(req, res) { @@ -440,9 +444,9 @@ function cancelSubscription(req, res, next) { } /** - * @param {import("express").Request} req - * @param {import("express").Response} res - * @param {import("express").NextFunction} next + * @param {import('express').Request} req + * @param {import('express').Response} res + * @param {import('express').NextFunction} next * @returns {Promise} */ function canceledSubscription(req, res, next) { diff --git a/services/web/app/src/Features/UserMembership/UserMembershipAuthorization.js b/services/web/app/src/Features/UserMembership/UserMembershipAuthorization.js index e3c29d32d6..a7f25c10a5 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipAuthorization.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipAuthorization.js @@ -17,19 +17,12 @@ const UserMembershipAuthorization = { if (!req.entity) { return false } - return req.entity[req.entityConfig.fields.access].some(accessUserId => - accessUserId.equals(req.user._id) - ) - } - }, - - isEntityMember() { - return req => { - if (!req.entity) { - return false - } - return req.entity[req.entityConfig.fields.membership].some(accessUserId => - accessUserId.equals(req.user._id) + const fieldAccess = req.entity[req.entityConfig.fields.access] + const fieldAccessArray = Array.isArray(fieldAccess) + ? fieldAccess + : [fieldAccess.toString()] + return fieldAccessArray.some( + accessUserId => accessUserId.toString() === req.user._id.toString() ) } }, diff --git a/services/web/app/src/Features/UserMembership/UserMembershipEntityConfigs.js b/services/web/app/src/Features/UserMembership/UserMembershipEntityConfigs.js index 94bc01f7f2..8c69032bab 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipEntityConfigs.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipEntityConfigs.js @@ -14,19 +14,6 @@ module.exports = { baseQuery: { groupPlan: true, }, - translations: { - title: 'group_subscription', - subtitle: 'members_management', - remove: 'remove_from_group', - }, - pathsFor(id) { - return { - addMember: `/manage/groups/${id}/invites`, - removeMember: `/manage/groups/${id}/user`, - removeInvite: `/manage/groups/${id}/invites`, - exportMembers: `/manage/groups/${id}/members/export`, - } - }, }, team: { @@ -54,16 +41,20 @@ module.exports = { baseQuery: { groupPlan: true, }, - translations: { - title: 'group_subscription', - subtitle: 'managers_management', - remove: 'remove_manager', + }, + + groupAdmin: { + modelName: 'Subscription', + fields: { + primaryKey: '_id', + read: ['admin_id'], + write: null, + access: 'admin_id', + membership: 'admin_id', + name: 'teamName', }, - pathsFor(id) { - return { - addMember: `/manage/groups/${id}/managers`, - removeMember: `/manage/groups/${id}/managers`, - } + baseQuery: { + groupPlan: true, }, }, @@ -77,16 +68,9 @@ module.exports = { membership: 'member_ids', name: 'name', }, - translations: { - title: 'institution_account', - subtitle: 'managers_management', - remove: 'remove_manager', - }, pathsFor(id) { return { index: `/manage/institutions/${id}/managers`, - addMember: `/manage/institutions/${id}/managers`, - removeMember: `/manage/institutions/${id}/managers`, } }, }, @@ -101,16 +85,9 @@ module.exports = { membership: 'member_ids', name: 'name', }, - translations: { - title: 'publisher_account', - subtitle: 'managers_management', - remove: 'remove_manager', - }, pathsFor(id) { return { index: `/manage/publishers/${id}/managers`, - addMember: `/manage/publishers/${id}/managers`, - removeMember: `/manage/publishers/${id}/managers`, } }, }, diff --git a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js index fc443cf67a..920bd9fbac 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js @@ -64,6 +64,17 @@ const UserMembershipMiddleware = { ]), ], + requireGroupAdminAccess: [ + AuthenticationController.requireLogin(), + fetchEntityConfig('groupAdmin'), + fetchEntity(), + requireEntity(), + allowAccessIfAny([ + UserMembershipAuthorization.hasEntityAccess(), + UserMembershipAuthorization.hasStaffAccess('groupManagement'), + ]), + ], + requireInstitutionMetricsAccess: [ AuthenticationController.requireLogin(), fetchEntityConfig('institution'), @@ -222,12 +233,11 @@ function fetchEntityConfig(entityName) { // fetch the entity with id and config, and set it in the request function fetchEntity() { return expressify(async (req, res, next) => { - const entity = + req.entity = await UserMembershipHandler.promises.getEntityWithoutAuthorizationCheck( req.params.id, req.entityConfig ) - req.entity = entity next() }) } diff --git a/services/web/test/acceptance/src/DeletionTests.js b/services/web/test/acceptance/src/DeletionTests.js index 9b4013f05c..cf97dddd96 100644 --- a/services/web/test/acceptance/src/DeletionTests.js +++ b/services/web/test/acceptance/src/DeletionTests.js @@ -1,4 +1,5 @@ const User = require('./helpers/User') +const Subscription = require('./helpers/Subscription') const request = require('./helpers/request') const async = require('async') const { expect } = require('chai') @@ -21,13 +22,39 @@ before(function () { describe('Deleting a user', function () { beforeEach(function (done) { - this.user = new User() - async.series( - [ - this.user.ensureUserExists.bind(this.user), - this.user.login.bind(this.user), - ], - done + async.auto( + { + user: cb => { + const user = new User() + user.ensureUserExists(() => { + cb(null, user) + }) + }, + login: [ + 'user', + (results, cb) => { + results.user.login(cb) + }, + ], + subscription: [ + 'user', + 'login', + (results, cb) => { + const subscription = new Subscription({ + admin_id: results.user._id, + }) + subscription.ensureExists(err => { + cb(err, subscription) + }) + }, + ], + }, + (err, results) => { + expect(err).not.to.exist + this.user = results.user + this.subscription = results.subscription + done() + } ) })