diff --git a/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee b/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee index 2ebca7682f..9a6e9dbaeb 100644 --- a/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee +++ b/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee @@ -12,7 +12,10 @@ module.exports = subscription = req.entity email = EmailHelper.parseEmail(req.body.email) if !email? - return res.sendStatus(400) + return res.status(422).json error: + code: 'invalid_email' + message: req.i18n.translate('invalid_email') + TeamInvitesHandler.createInvite teamManagerId, subscription, email, (err, invite) -> return next(err) if err? diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee index 654d3638de..3276552c1f 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee @@ -2,6 +2,7 @@ AuthenticationController = require('../Authentication/AuthenticationController') UserMembershipHandler = require('./UserMembershipHandler') EntityConfigs = require('./UserMembershipEntityConfigs') Errors = require('../Errors/Errors') +EmailHelper = require("../Helpers/EmailHelper") logger = require("logger-sharelatex") module.exports = @@ -18,13 +19,25 @@ module.exports = add: (req, res, next)-> { entity, entityConfig } = req - email = req.body.email - return res.sendStatus 422 unless email + email = EmailHelper.parseEmail(req.body.email) + if !email? + return res.status(400).json error: + code: 'invalid_email' + message: req.i18n.translate('invalid_email') + if entityConfig.readOnly return next(new Errors.NotFoundError("Cannot add users to entity")) UserMembershipHandler.addUser entity, entityConfig, email, (error, user)-> + if error?.alreadyAdded + return res.status(400).json error: + code: 'user_already_added' + message: req.i18n.translate('user_already_added') + if error?.userNotFound + return res.status(404).json error: + code: 'user_not_found' + message: req.i18n.translate('user_not_found') return next(error) if error? res.json(user: user) @@ -35,7 +48,17 @@ module.exports = if entityConfig.readOnly return next(new Errors.NotFoundError("Cannot remove users from entity")) + loggedInUserId = AuthenticationController.getLoggedInUserId(req) + if loggedInUserId == userId + return res.status(400).json error: + code: 'managers_cannot_remove_self' + message: req.i18n.translate('managers_cannot_remove_self') + UserMembershipHandler.removeUser entity, entityConfig, userId, (error, user)-> + if error?.isAdmin + return res.status(400).json error: + code: 'managers_cannot_remove_admin' + message: req.i18n.translate('managers_cannot_remove_admin') return next(error) if error? res.send() diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee index 015f32c4dc..3dab0c5771 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee @@ -24,13 +24,19 @@ module.exports = addUser: (entity, entityConfig, email, callback = (error, user) ->) -> attribute = entityConfig.fields.write UserGetter.getUserByAnyEmail email, (error, user) -> - error ||= new Errors.NotFoundError("No user found with email #{email}") unless user return callback(error) if error? + unless user + return callback(userNotFound: true) + if entity[attribute].some((managerId) -> managerId.equals(user._id)) + return callback(alreadyAdded: true) + addUserToEntity entity, attribute, user, (error) -> callback(error, UserMembershipViewModel.build(user)) removeUser: (entity, entityConfig, userId, callback = (error) ->) -> attribute = entityConfig.fields.write + if entity.admin_id?.equals(userId) + return callback(isAdmin: true) removeUserFromEntity entity, attribute, userId, callback getPopulatedListOfMembers = (entity, attributes, callback = (error, users)->)-> diff --git a/services/web/app/views/user_membership/index.pug b/services/web/app/views/user_membership/index.pug index 330b56f1f0..11d59ae0dd 100644 --- a/services/web/app/views/user_membership/index.pug +++ b/services/web/app/views/user_membership/index.pug @@ -17,6 +17,12 @@ block content h1 #{translate(translations.title)} .row-spaced-small + div(ng-if="inputs.removeMembers.error", ng-cloak) + div.alert.alert-danger(ng-if="inputs.removeMembers.errorMessage") + | #{translate('error')}: + | {{ inputs.removeMembers.errorMessage }} + div.alert.alert-danger(ng-if="!inputs.removeMembers.errorMessage") + | #{translate('generic_something_went_wrong')} ul.list-unstyled.structured-list( select-all-list, ng-cloak @@ -62,6 +68,12 @@ block content hr div(ng-if="!groupSize || users.length < groupSize", ng-cloak) p.small #{translate("add_more_members")} + div(ng-if="inputs.addMembers.error", ng-cloak) + div.alert.alert-danger(ng-if="inputs.addMembers.errorMessage") + | #{translate('error')}: + | {{ inputs.addMembers.errorMessage }} + div.alert.alert-danger(ng-if="!inputs.addMembers.errorMessage") + | #{translate('generic_something_went_wrong')} form.form .row .col-xs-6 @@ -69,7 +81,7 @@ block content name="email", type="text", placeholder="jane@example.com, joe@example.com", - ng-model="inputs.emails", + ng-model="inputs.addMembers.content", on-enter="addMembers()" ) .col-xs-4 diff --git a/services/web/public/coffee/main/user-membership.coffee b/services/web/public/coffee/main/user-membership.coffee index 1d2df70407..cfb41c2a0f 100644 --- a/services/web/public/coffee/main/user-membership.coffee +++ b/services/web/public/coffee/main/user-membership.coffee @@ -8,7 +8,13 @@ define [ $scope.selectedUsers = [] $scope.inputs = - emails: "" + addMembers: + content: '' + error: false + errorMessage: null + removeMembers: + error: false + errorMessage: null parseEmails = (emailsString)-> regexBySpaceOrComma = /[\s,]+/ @@ -20,7 +26,9 @@ define [ return emails $scope.addMembers = () -> - emails = parseEmails($scope.inputs.emails) + $scope.inputs.addMembers.error = false + $scope.inputs.addMembers.errorMessage = null + emails = parseEmails($scope.inputs.addMembers.content) for email in emails queuedHttp .post(paths.addMember, { @@ -30,9 +38,15 @@ define [ .then (response) -> { data } = response $scope.users.push data.user if data.user? - $scope.inputs.emails = "" + $scope.inputs.addMembers.content = "" + .catch (response) -> + { data } = response + $scope.inputs.addMembers.error = true + $scope.inputs.addMembers.errorMessage = data.error?.message $scope.removeMembers = () -> + $scope.inputs.removeMembers.error = false + $scope.inputs.removeMembers.errorMessage = null for user in $scope.selectedUsers do (user) -> if paths.removeInvite and user.invite and !user._id? @@ -51,7 +65,11 @@ define [ index = $scope.users.indexOf(user) return if index == -1 $scope.users.splice(index, 1) - $scope.selectedUsers = [] + .catch (response) -> + { data } = response + $scope.inputs.removeMembers.error = true + $scope.inputs.removeMembers.errorMessage = data.error?.message + $scope.updateSelectedUsers $scope.updateSelectedUsers = () -> $scope.selectedUsers = $scope.users.filter (user) -> user.selected diff --git a/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee b/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee index bb2012a045..886101936c 100644 --- a/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee +++ b/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee @@ -27,6 +27,7 @@ describe "UserMembershipController", -> @AuthenticationController = getSessionUser: sinon.stub().returns(@user) + getLoggedInUserId: sinon.stub().returns(@user._id) @UserMembershipHandler = getEntity: sinon.stub().yields(null, @subscription) getUsers: sinon.stub().yields(null, @users) @@ -110,6 +111,24 @@ describe "UserMembershipController", -> expect(error).to.be.an.instanceof(Errors.NotFoundError) done() + it 'handle user already added', (done) -> + @UserMembershipHandler.addUser.yields(alreadyAdded: true) + @UserMembershipController.add @req, status: () => json: (payload) => + expect(payload.error.code).to.equal 'user_already_added' + done() + + it 'handle user not found', (done) -> + @UserMembershipHandler.addUser.yields(userNotFound: true) + @UserMembershipController.add @req, status: () => json: (payload) => + expect(payload.error.code).to.equal 'user_not_found' + done() + + it 'handle invalid email', (done) -> + @req.body.email = 'not_valid_email' + @UserMembershipController.add @req, status: () => json: (payload) => + expect(payload.error.code).to.equal 'invalid_email' + done() + describe 'remove', -> beforeEach -> @req.params.userId = @newUser._id @@ -133,6 +152,18 @@ describe "UserMembershipController", -> expect(error).to.be.an.instanceof(Errors.NotFoundError) done() + it 'prevent self removal', (done) -> + @req.params.userId = @user._id + @UserMembershipController.remove @req, status: () => json: (payload) => + expect(payload.error.code).to.equal 'managers_cannot_remove_self' + done() + + it 'prevent admin removal', (done) -> + @UserMembershipHandler.removeUser.yields(isAdmin: true) + @UserMembershipController.remove @req, status: () => json: (payload) => + expect(payload.error.code).to.equal 'managers_cannot_remove_admin' + done() + describe "exportCsv", -> beforeEach -> diff --git a/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee b/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee index a96977af55..55fcb64943 100644 --- a/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee +++ b/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee @@ -12,8 +12,8 @@ EntityConfigs = require("../../../../app/js/Features/UserMembership/UserMembersh describe 'UserMembershipHandler', -> beforeEach -> - @user = _id: 'mock-user-id' - @newUser = _id: 'mock-new-user-id', email: 'new-user-email@foo.bar' + @user = _id: ObjectId() + @newUser = _id: ObjectId(), email: 'new-user-email@foo.bar' @fakeEntityId = ObjectId() @subscription = _id: 'mock-subscription-id' @@ -133,7 +133,14 @@ describe 'UserMembershipHandler', -> @UserGetter.getUserByAnyEmail.yields(null, null) @UserMembershipHandler.addUser @institution, EntityConfigs.institution, @email, (error) => expect(error).to.exist - expect(error).to.be.an.instanceof(Errors.NotFoundError) + expect(error.userNotFound).to.equal true + done() + + it 'handle user already added', (done) -> + @institution.managerIds.push(@newUser._id) + @UserMembershipHandler.addUser @institution, EntityConfigs.institution, @email, (error, users) => + expect(error).to.exist + expect(error.alreadyAdded).to.equal true done() it 'add user to institution', (done) -> @@ -153,3 +160,10 @@ describe 'UserMembershipHandler', -> lastCall = @institution.update.lastCall assertCalledWith(@institution.update, { $pull: managerIds: @newUser._id }) done() + + it 'handle admin', (done) -> + @subscription.admin_id = @newUser._id + @UserMembershipHandler.removeUser @subscription, EntityConfigs.groupManagers, @newUser._id, (error, user) => + expect(error).to.exist + expect(error.isAdmin).to.equal true + done()