From dbf2cc08fe0bb4b59af4cc90528f9790b039ea79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 10 Feb 2022 10:50:12 +0100 Subject: [PATCH] Merge pull request #6693 from overleaf/jpa-error-is-manager [web] refactor error handling for rejected manager removal GitOrigin-RevId: 48be3bf254de74c2799d1368aee329fd9038dfa6 --- .../Features/UserMembership/UserMembershipController.js | 3 ++- .../src/Features/UserMembership/UserMembershipErrors.js | 7 +++++++ .../src/Features/UserMembership/UserMembershipHandler.js | 4 ++-- .../src/UserMembership/UserMembershipControllerTests.js | 6 +++++- .../unit/src/UserMembership/UserMembershipHandlerTests.js | 6 +++++- 5 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 services/web/app/src/Features/UserMembership/UserMembershipErrors.js diff --git a/services/web/app/src/Features/UserMembership/UserMembershipController.js b/services/web/app/src/Features/UserMembership/UserMembershipController.js index 3a5853a145..23a306ffbb 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipController.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipController.js @@ -15,6 +15,7 @@ const UserMembershipHandler = require('./UserMembershipHandler') const Errors = require('../Errors/Errors') const EmailHelper = require('../Helpers/EmailHelper') const { csvAttachment } = require('../../infrastructure/Response') +const { UserIsManagerError } = require('./UserMembershipErrors') const CSVParser = require('json2csv').Parser module.exports = { @@ -119,7 +120,7 @@ module.exports = { entityConfig, userId, function (error, user) { - if (error != null ? error.isAdmin : undefined) { + if (error && error instanceof UserIsManagerError) { return res.status(400).json({ error: { code: 'managers_cannot_remove_admin', diff --git a/services/web/app/src/Features/UserMembership/UserMembershipErrors.js b/services/web/app/src/Features/UserMembership/UserMembershipErrors.js new file mode 100644 index 0000000000..504f4fa512 --- /dev/null +++ b/services/web/app/src/Features/UserMembership/UserMembershipErrors.js @@ -0,0 +1,7 @@ +const OError = require('@overleaf/o-error') + +class UserIsManagerError extends OError {} + +module.exports = { + UserIsManagerError, +} diff --git a/services/web/app/src/Features/UserMembership/UserMembershipHandler.js b/services/web/app/src/Features/UserMembership/UserMembershipHandler.js index 46b727f350..a8bcb749dc 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipHandler.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipHandler.js @@ -25,6 +25,7 @@ const UserMembershipViewModel = require('./UserMembershipViewModel') const UserGetter = require('../User/UserGetter') const logger = require('@overleaf/logger') const UserMembershipEntityConfigs = require('./UserMembershipEntityConfigs') +const { UserIsManagerError } = require('./UserMembershipErrors') const UserMembershipHandler = { getEntityWithoutAuthorizationCheck(entityId, entityConfig, callback) { @@ -81,8 +82,7 @@ const UserMembershipHandler = { } const attribute = entityConfig.fields.write if (entity.admin_id != null ? entity.admin_id.equals(userId) : undefined) { - const error = { isAdmin: true } - return callback(error) + return callback(new UserIsManagerError()) } return removeUserFromEntity(entity, attribute, userId, callback) }, diff --git a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js b/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js index b983c1b33c..b71ebbacf2 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js +++ b/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js @@ -21,6 +21,9 @@ const MockRequest = require('../helpers/MockRequest') const MockResponse = require('../helpers/MockResponse') const EntityConfigs = require('../../../../app/src/Features/UserMembership/UserMembershipEntityConfigs') const Errors = require('../../../../app/src/Features/Errors/Errors') +const { + UserIsManagerError, +} = require('../../../../app/src/Features/UserMembership/UserMembershipErrors') describe('UserMembershipController', function () { beforeEach(function () { @@ -71,6 +74,7 @@ describe('UserMembershipController', function () { modulePath, { requires: { + './UserMembershipErrors': { UserIsManagerError }, '../Authentication/SessionManager': this.SessionManager, './UserMembershipHandler': this.UserMembershipHandler, }, @@ -263,7 +267,7 @@ describe('UserMembershipController', function () { }) it('prevent admin removal', function (done) { - this.UserMembershipHandler.removeUser.yields({ isAdmin: true }) + this.UserMembershipHandler.removeUser.yields(new UserIsManagerError()) return this.UserMembershipController.remove(this.req, { status: () => ({ json: payload => { diff --git a/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js b/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js index a47d57ed75..42d2a0d244 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js +++ b/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js @@ -21,6 +21,9 @@ const modulePath = const SandboxedModule = require('sandboxed-module') const Errors = require('../../../../app/src/Features/Errors/Errors') const EntityConfigs = require('../../../../app/src/Features/UserMembership/UserMembershipEntityConfigs') +const { + UserIsManagerError, +} = require('../../../../app/src/Features/UserMembership/UserMembershipErrors') describe('UserMembershipHandler', function () { beforeEach(function () { @@ -68,6 +71,7 @@ describe('UserMembershipHandler', function () { return (this.UserMembershipHandler = SandboxedModule.require(modulePath, { requires: { mongodb: { ObjectId }, + './UserMembershipErrors': { UserIsManagerError }, './UserMembershipViewModel': this.UserMembershipViewModel, '../User/UserGetter': this.UserGetter, '../../models/Institution': { @@ -265,7 +269,7 @@ describe('UserMembershipHandler', function () { this.newUser._id, (error, user) => { expect(error).to.exist - expect(error.isAdmin).to.equal(true) + expect(error).to.be.instanceof(UserIsManagerError) return done() } )