From 2dfcfd18047cee93c791b531381eba952883a6ff Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Wed, 21 Jan 2026 15:56:13 +0000 Subject: [PATCH] Merge pull request #30916 from overleaf/ar-allow-read-only-institution-access-for-admins-with-no-modify-capability [web] Add view-only version of institution managers page GitOrigin-RevId: 65d19e42220932fe268d595ea13d0ba0b18c4398 --- .../UserMembershipMiddleware.mjs | 14 +++- .../UserMembership/UserMembershipRouter.mjs | 6 +- .../institution-managers-react.pug | 3 +- .../src/UserMembershipAuthorizationTests.mjs | 65 ++++++++++++++++++- 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs index dfaaee3b3e..6bea53c0a1 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs +++ b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs @@ -95,7 +95,19 @@ const UserMembershipMiddleware = { ]), ], - requireInstitutionManagementAccess: [ + requireInstitutionManagerAccess: [ + AuthenticationController.requireLogin(), + fetchEntityConfig('institution'), + fetchEntity(), + requireEntityOrCreate(), + useAdminCapabilities, + allowAccessIfAny([ + UserMembershipAuthorization.hasEntityAccess(), + UserMembershipAuthorization.hasAdminAccess, + ]), + ], + + requireInstitutionManagerManagement: [ AuthenticationController.requireLogin(), fetchEntityConfig('institution'), fetchEntity(), diff --git a/services/web/app/src/Features/UserMembership/UserMembershipRouter.mjs b/services/web/app/src/Features/UserMembership/UserMembershipRouter.mjs index 798d2cba35..e590e0d174 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipRouter.mjs +++ b/services/web/app/src/Features/UserMembership/UserMembershipRouter.mjs @@ -82,17 +82,17 @@ export default { // institution members routes webRouter.get( '/manage/institutions/:id/managers', - UserMembershipMiddleware.requireInstitutionManagementAccess, + UserMembershipMiddleware.requireInstitutionManagerAccess, UserMembershipController.manageInstitutionManagers ) webRouter.post( '/manage/institutions/:id/managers', - UserMembershipMiddleware.requireInstitutionManagementAccess, + UserMembershipMiddleware.requireInstitutionManagerManagement, UserMembershipController.add ) webRouter.delete( '/manage/institutions/:id/managers/:userId', - UserMembershipMiddleware.requireInstitutionManagementAccess, + UserMembershipMiddleware.requireInstitutionManagerManagement, UserMembershipController.remove ) diff --git a/services/web/app/views/user_membership/institution-managers-react.pug b/services/web/app/views/user_membership/institution-managers-react.pug index e174ae6796..4a6433ddd0 100644 --- a/services/web/app/views/user_membership/institution-managers-react.pug +++ b/services/web/app/views/user_membership/institution-managers-react.pug @@ -4,11 +4,12 @@ block entrypointVar - entrypoint = 'pages/user/subscription/group-management/institution-managers' block append meta + - const hasWriteAccess = entityAccess || (hasAdminAccess() && hasAdminCapability('modify-institution-manager')) meta(name='ol-user' data-type='json' content=user) meta(name='ol-users' data-type='json' content=users) meta(name='ol-groupId' data-type='string' content=groupId) meta(name='ol-groupName' data-type='string' content=name) - meta(name='ol-hasWriteAccess' data-type='boolean' content) + meta(name='ol-hasWriteAccess' data-type='boolean' content=hasWriteAccess) block content main#subscription-manage-group-root.content.content-alt diff --git a/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs b/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs index 5c6f422155..b1f7448cf6 100644 --- a/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs +++ b/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs @@ -90,7 +90,7 @@ describe('UserMembershipAuthorization', function () { }) describe('users management', function () { - it('should allow managers only', function (done) { + it('should allow managers', function (done) { const url = `/manage/institutions/${this.institution.v1Id}/managers` async.series( [ @@ -102,6 +102,54 @@ describe('UserMembershipAuthorization', function () { done ) }) + + it('should allow admin users', function (done) { + const url = `/manage/institutions/${this.institution.v1Id}/managers` + async.series( + [ + this.user.login.bind(this.user), + expectAccess(this.user, url, 403), + cb => this.user.ensureAdmin(cb), + cb => this.user.ensureAdminRole('sales', cb), + this.user.login.bind(this.user), + expectAccess(this.user, url, 200), + ], + done + ) + }) + + it('should not allow "sales" admin to add managers', function (done) { + const url = `/manage/institutions/${this.institution.v1Id}/managers` + async.series( + [ + this.user.login.bind(this.user), + expectPost(this.user, url, { email: this.user.email }, 403), + cb => this.user.ensureAdmin(cb), + cb => this.user.ensureAdminRole('sales', cb), + this.user.login.bind(this.user), + expectPost(this.user, url, { email: this.user.email }, 403), + ], + done + ) + }) + + it('should allow "engineering" admin to add managers', function (done) { + const user = new User() + const url = `/manage/institutions/${this.institution.v1Id}/managers` + async.series( + [ + user.ensureUserExists.bind(user), + this.user.login.bind(this.user), + expectPost(this.user, url, { email: user.email }, 403), + cb => this.user.ensureAdmin(cb), + cb => this.user.ensureAdminRole('engineering', cb), + this.user.login.bind(this.user), + expectPost(this.user, url, { email: user.email }, 200), + cb => this.institution.setManagerIds([], cb), + ], + done + ) + }) }) describe('creation', function () { @@ -195,3 +243,18 @@ function expectAccess(user, url, status, pattern) { }) } } + +function expectPost(user, url, body, status, pattern) { + return callback => { + user.request.post({ url, json: body }, (error, response, body) => { + if (error) { + return callback(error) + } + expect(response.statusCode).to.equal(status) + if (pattern) { + expect(body).to.match(pattern) + } + callback() + }) + } +}