diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee index 5618da7db7..26da21d9db 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee @@ -55,6 +55,7 @@ module.exports = CollaboratorsInviteController = token = req.params.token currentUser = req.session.user _renderInvalidPage = () -> + logger.log {projectId, token}, "invite not valid, rendering not-valid page" res.render "project/invite/not-valid" # get the target project Project.findOne {_id: projectId}, {owner_ref: 1, name: 1, collaberator_refs: 1, readOnly_refs: 1}, (err, project) -> @@ -64,21 +65,20 @@ module.exports = CollaboratorsInviteController = if !project logger.log {projectId}, "no project found" return _renderInvalidPage() + # check if user is already a member of the project, redirect to project if so + allMembers = (project.collaberator_refs || []).concat(project.readOnly_refs || []).map((oid) -> oid.toString()) + if currentUser._id in allMembers + logger.log {projectId, userId: currentUser._id}, "user is already a member of this project, redirecting" + return res.redirect "/project/#{projectId}" # get the invite CollaboratorsInviteHandler.getInviteByToken projectId, token, (err, invite) -> if err? logger.err {projectId, token}, "error getting invite by token" return next(err) - # check if invite is gone + # check if invite is gone, or otherwise non-existent if !invite logger.log {projectId, token}, "no invite found for this token" - # check if user is already a member of the project, redirect to project if so - allMembers = (project.collaberator_refs || []).concat(project.readOnly_refs || []).map((oid) -> oid.toString()) - if currentUser._id in allMembers - logger.log {projectId, userId: currentUser._id}, "user is already a member of this project, redirecting" - return res.redirect "/project/#{projectId}" - else - return _renderInvalidPage() + return _renderInvalidPage() # check the user who sent the invite exists User.findOne {_id: invite.sendingUserId}, {email: 1, first_name: 1, last_name: 1}, (err, owner) -> if err? diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee index 0950c7efaa..8118f7b135 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee @@ -79,6 +79,15 @@ module.exports = CollaboratorsInviteHandler = logger.log {err, projectId, inviteId, tokenString}, "no matching invite found" return callback(err) + # check if user is already a member of this project, + # return early if so + existing_users = (project.collaberator_refs or []) + existing_users = existing_users.concat(project.readOnly_refs or []) + existing_users = existing_users.map (u) -> u.toString() + if existing_users.indexOf(user._id.toString()) > -1 + logger.log {projectId, userId: user._id}, "user already member of project, returning" + return callback null + # build an update to be applied with $addToSet, user is added to either # `collaberator_refs` or `readOnly_refs` privilegeLevel = invite.privileges @@ -93,9 +102,7 @@ module.exports = CollaboratorsInviteHandler = ContactManager.addContact invite.sendingUserId, user._id - # Update the project, adding the new member. We don't check if the user is already a member of the project, - # because even if they are we still want to have them 'accept' the invite and go through the usual process, - # despite the $addToSet operation having no meaningful effect + # Update the project, adding the new member. Project.update { _id: project._id }, { $addToSet: level }, (error) -> return callback(error) if error? # Flush to TPDS in background to add files to collaborator's Dropbox diff --git a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee index 3de79d9fad..9cf4db291e 100644 --- a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee @@ -329,8 +329,8 @@ describe "CollaboratorsInviteController", -> it 'should call Project.findOne', -> @Project.findOne.callCount.should.equal 1 - it 'should call getInviteByToken', -> - @CollaboratorsInviteHandler.getInviteByToken.callCount.should.equal 1 + it 'should not call getInviteByToken', -> + @CollaboratorsInviteHandler.getInviteByToken.callCount.should.equal 0 it 'should not call User.findOne', -> @User.findOne.callCount.should.equal 0 diff --git a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee index 1af35edc90..d144a3dcb9 100644 --- a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee @@ -343,6 +343,41 @@ describe "CollaboratorsInviteHandler", -> @ProjectInvite.remove.callCount.should.equal 0 done() + describe 'when user is already a member of project', -> + + beforeEach -> + @fakeProject.collaberator_refs = [ObjectId(), @user._id, ObjectId(), ObjectId()] + @Project.findOne.callsArgWith(1, null, @fakeProject) + + it 'should not produce an error', (done) -> + @call (err) => + expect(err).to.not.be.instanceof Error + expect(err).to.be.oneOf [null, undefined] + done() + + it 'should have called Project.findOne', (done) -> + @call (err) => + @Project.findOne.callCount.should.equal 1 + @Project.findOne.calledWith({_id: @projectId}).should.equal true + done() + + it 'should have called ProjectInvite.findOne', (done) -> + @call (err) => + @ProjectInvite.findOne.callCount.should.equal 1 + @ProjectInvite.findOne.calledWith({_id: @inviteId, projectId: @projectId, token: @token}).should.equal true + done() + + it 'should have returned early, not have called Project.update', (done) -> + @call (err) => + @Project.update.callCount.should.equal 0 + done() + + it 'should not have called ProjectInvite.remove', (done) -> + @call (err) => + @ProjectInvite.remove.callCount.should.equal 0 + done() + + describe 'when ProjectInvite.findOne does not find an invite', -> beforeEach ->