From 155102df640266f8802490171688afa2bebaf7bb Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 11 Jun 2018 14:20:35 +0100 Subject: [PATCH] Use Error classes, and ensure invited_emails is cleared on new invite --- .../app/coffee/Features/Errors/Errors.coffee | 2 +- .../Subscription/TeamInvitesController.coffee | 18 ++---- .../Subscription/TeamInvitesHandler.coffee | 30 ++++++--- .../TeamInvitesHandlerTests.coffee | 62 +++++++++++-------- 4 files changed, 62 insertions(+), 50 deletions(-) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index b2ed088d15..af1e437d4b 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -56,7 +56,7 @@ V1HistoryNotSyncedError.prototype.__proto___ = Error.prototype ProjectHistoryDisabledError = (message) -> error = new Error(message) - error.name = "ProjectHistoryDisabledError " + error.name = "ProjectHistoryDisabledError" error.__proto__ = ProjectHistoryDisabledError.prototype return error ProjectHistoryDisabledError.prototype.__proto___ = Error.prototype diff --git a/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee b/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee index f4f0933fdd..475c4684b0 100644 --- a/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee +++ b/services/web/app/coffee/Features/Subscription/TeamInvitesController.coffee @@ -11,7 +11,7 @@ module.exports = email = req.body.email TeamInvitesHandler.createInvite teamManagerId, email, (err, invite) -> - return handleError(err, req, res, next) if err? + return next(err) if err? inviteView = { user: { email: invite.email, sentAt: invite.sentAt, invite: true } } @@ -22,13 +22,13 @@ module.exports = userId = AuthenticationController.getLoggedInUserId(req) TeamInvitesHandler.getInvite token, (err, invite, teamSubscription) -> - return handleError(err, req, res, next) if err? + return next(err) if err? if !invite return ErrorController.notFound(req, res, next) SubscriptionLocator.getUsersSubscription userId, (err, personalSubscription) -> - return handleError(err, req, res, next) if err? + return next(err) if err? res.render "subscriptions/team/invite", inviterName: invite.inviterName @@ -41,8 +41,7 @@ module.exports = userId = AuthenticationController.getLoggedInUserId(req) TeamInvitesHandler.acceptInvite token, userId, (err, results) -> - return handleError(err, req, res, next) if err? - + return next(err) if err? res.sendStatus 204 revokeInvite: (req, res) -> @@ -50,12 +49,5 @@ module.exports = teamManagerId = AuthenticationController.getLoggedInUserId(req) TeamInvitesHandler.revokeInvite teamManagerId, email, (err, results) -> - return handleError(err, req, res, next) if err? - + return next(err) if err? res.sendStatus 204 - -handleError = (err, req, res, next) -> - if err.teamNotFound or err.inviteNoLongerValid - ErrorController.notFound(req, res, next) - else - next(err) diff --git a/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee b/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee index b84cda82f0..b27d0da8a1 100644 --- a/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee @@ -14,12 +14,15 @@ SubscriptionUpdater = require("./SubscriptionUpdater") LimitationsManager = require("./LimitationsManager") EmailHandler = require("../Email/EmailHandler") +EmailHelper = require("../Helpers/EmailHelper") + +Errors = require "../Errors/Errors" module.exports = TeamInvitesHandler = getInvite: (token, callback) -> Subscription.findOne 'teamInvites.token': token, (err, subscription) -> return callback(err) if err? - return callback(teamNotFound: true) unless subscription? + return callback(new Errors.NotFoundError('team not found')) unless subscription? invite = subscription.teamInvites.find (i) -> i.token == token return callback(null, invite, subscription) @@ -37,7 +40,9 @@ module.exports = TeamInvitesHandler = else inviterName = teamManager.email - createInvite(subscription, email, inviterName, callback) + removeLegacyInvite subscription.id, email, (error) -> + return callback(error) if error? + createInvite(subscription, email, inviterName, callback) createDomainInvite: (user, licence, callback) -> logger.log {licence, email: user.email}, "Creating domain team invite" @@ -51,7 +56,7 @@ module.exports = TeamInvitesHandler = logger.log {userId}, "Accepting invite" TeamInvitesHandler.getInvite token, (err, invite, subscription) -> return callback(err) if err? - return callback(inviteNoLongerValid: true) unless invite? + return callback(new Errors.NotFoundError('invite not found')) unless invite? SubscriptionUpdater.addUserToGroup subscription.admin_id, userId, (err) -> return callback(err) if err? @@ -82,8 +87,7 @@ createInvite = (subscription, email, inviterName, callback) -> return callback(error) if error? return callback(reason) unless possible - # TODO: use standard way to canonalise email addresses - email = email.trim().toLowerCase() + email = EmailHelper.parseEmail(email) invite = subscription.teamInvites.find (invite) -> invite.email == email @@ -110,18 +114,24 @@ createInvite = (subscription, email, inviterName, callback) -> return callback(error, invite) removeInviteFromTeam = (subscriptionId, email, callback) -> - email = email.trim().toLowerCase() - + email = EmailHelper.parseEmail(email) searchConditions = { _id: new ObjectId(subscriptionId.toString()) } - removeInvite = { $pull: { teamInvites: { email: email } } } - removeLegacyInvite = { $pull: { invited_emails: email } } + logger.log {subscriptionId, email, searchConditions, removeInvite}, 'removeInviteFromTeam' async.series [ (cb) -> Subscription.update(searchConditions, removeInvite, cb), - (cb) -> Subscription.update(searchConditions, removeLegacyInvite, cb), + (cb) -> removeLegacyInvite(subscriptionId, email, cb), ], callback +removeLegacyInvite = (subscriptionId, email, callback) -> + Subscription.update({ + _id: new ObjectId(subscriptionId.toString()) + }, { + $pull: { + invited_emails: EmailHelper.parseEmail(email) + } + }, callback) checkIfInviteIsPossible = (subscription, email, callback = (error, possible, reason) -> ) -> unless subscription.groupPlan diff --git a/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee index 720c8fadf7..527ba8a143 100644 --- a/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee @@ -6,6 +6,7 @@ querystring = require 'querystring' modulePath = "../../../../app/js/Features/Subscription/TeamInvitesHandler" ObjectId = require("mongojs").ObjectId +Errors = require("../../../../app/js/Features/Errors/Errors") describe "TeamInvitesHandler", -> beforeEach -> @@ -13,13 +14,13 @@ describe "TeamInvitesHandler", -> id: "666666", first_name: "Daenerys" last_name: "Targaryen" - email: "daenerys@motherofdragons.com" + email: "daenerys@example.com" } @token = "aaaaaaaaaaaaaaaaaaaaaa" @teamInvite = { - email: "jorah@mormont.org", + email: "jorah@example.com", token: @token, } @@ -83,6 +84,7 @@ describe "TeamInvitesHandler", -> "./SubscriptionUpdater": @SubscriptionUpdater "./LimitationsManager": @LimitationsManager "../Email/EmailHandler": @EmailHandler + "../Errors/Errors": Errors describe "getInvite", -> it "returns the invite if there's one", (done) -> @@ -96,25 +98,25 @@ describe "TeamInvitesHandler", -> @Subscription.findOne = sinon.stub().yields(null, null) @TeamInvitesHandler.getInvite @token, (err, invite, subscription) -> - expect(err).to.deep.eq(teamNotFound: true) + expect(err).to.be.instanceof(Errors.NotFoundError) done() describe "createInvite", -> it "adds the team invite to the subscription", (done) -> - @TeamInvitesHandler.createInvite @manager.id, "John.Snow@nightwatch.com", (err, invite) => + @TeamInvitesHandler.createInvite @manager.id, "John.Snow@example.com", (err, invite) => expect(err).to.eq(null) expect(invite.token).to.eq(@newToken) - expect(invite.email).to.eq("john.snow@nightwatch.com") - expect(invite.inviterName).to.eq("Daenerys Targaryen (daenerys@motherofdragons.com)") + expect(invite.email).to.eq("john.snow@example.com") + expect(invite.inviterName).to.eq("Daenerys Targaryen (daenerys@example.com)") expect(@subscription.teamInvites).to.deep.include(invite) done() it "sends an email", (done) -> - @TeamInvitesHandler.createInvite @manager.id, "John.Snow@nightwatch.com", (err, invite) => + @TeamInvitesHandler.createInvite @manager.id, "John.Snow@example.com", (err, invite) => @EmailHandler.sendEmail.calledWith("verifyEmailToJoinTeam", sinon.match({ - to: "john.snow@nightwatch.com", - inviterName: "Daenerys Targaryen (daenerys@motherofdragons.com)", + to: "john.snow@example.com", + inviterName: "Daenerys Targaryen (daenerys@example.com)", acceptInviteUrl: "http://example.com/subscription/invites/#{@newToken}/" }) ).should.equal true @@ -136,6 +138,14 @@ describe "TeamInvitesHandler", -> done() + it "removes any legacy invite from the subscription", (done) -> + @TeamInvitesHandler.createInvite @manager.id, "John.Snow@example.com", (err, invite) => + @Subscription.update.calledWith( + { _id: new ObjectId("55153a8014829a865bbf700d") }, + { '$pull': { invited_emails: "john.snow@example.com" } } + ).should.eq true + done() + describe "createDomainInvite", -> beforeEach -> @licence = @@ -143,13 +153,13 @@ describe "TeamInvitesHandler", -> name: "Team Daenerys" @user = - email: "John.Snow@nightwatch.com" + email: "John.Snow@example.com" it "adds the team invite to the subscription", (done) -> @TeamInvitesHandler.createDomainInvite @user, @licence, (err, invite) => expect(err).to.eq(null) expect(invite.token).to.eq(@newToken) - expect(invite.email).to.eq("john.snow@nightwatch.com") + expect(invite.email).to.eq("john.snow@example.com") expect(invite.inviterName).to.eq("Team Daenerys") expect(@subscription.teamInvites).to.deep.include(invite) done() @@ -158,7 +168,7 @@ describe "TeamInvitesHandler", -> @TeamInvitesHandler.createDomainInvite @user, @licence, (err, invite) => @EmailHandler.sendEmail.calledWith("verifyEmailToJoinTeam", sinon.match({ - to: "john.snow@nightwatch.com" + to: "john.snow@example.com" inviterName: "Team Daenerys" acceptInviteUrl: "http://example.com/subscription/invites/#{@newToken}/" }) @@ -171,15 +181,15 @@ describe "TeamInvitesHandler", -> id: "123456789", first_name: "Tyrion", last_name: "Lannister", - email: "tyrion@lannister.com" + email: "tyrion@example.com" } @UserGetter.getUserByAnyEmail.withArgs(@user.email).yields(null, @user) @subscription.teamInvites.push({ - email: "john.snow@nightwatch.com", + email: "john.snow@example.com", token: "dddddddd", - inviterName: "Daenerys Targaryen (daenerys@motherofdragons.com)" + inviterName: "Daenerys Targaryen (daenerys@example.com)" }) it "adds the user to the team", (done) -> @@ -191,37 +201,37 @@ describe "TeamInvitesHandler", -> @TeamInvitesHandler.acceptInvite "dddddddd", @user.id, => @Subscription.update.calledWith( { _id: new ObjectId("55153a8014829a865bbf700d") }, - { '$pull': { teamInvites: { email: 'john.snow@nightwatch.com' } } } + { '$pull': { teamInvites: { email: 'john.snow@example.com' } } } ).should.eq true done() describe "revokeInvite", -> it "removes the team invite from the subscription", (done) -> - @TeamInvitesHandler.revokeInvite @manager.id, "jorah@mormont.org", => + @TeamInvitesHandler.revokeInvite @manager.id, "jorah@example.com", => @Subscription.update.calledWith( { _id: new ObjectId("55153a8014829a865bbf700d") }, - { '$pull': { teamInvites: { email: "jorah@mormont.org" } } } + { '$pull': { teamInvites: { email: "jorah@example.com" } } } ).should.eq true @Subscription.update.calledWith( { _id: new ObjectId("55153a8014829a865bbf700d") }, - { '$pull': { invited_emails: "jorah@mormont.org" } } + { '$pull': { invited_emails: "jorah@example.com" } } ).should.eq true done() describe "createTeamInvitesForLegacyInvitedEmail", (done) -> beforeEach -> - @subscription.invited_emails = ["eddard@stark.com", "robert@stark.com"] + @subscription.invited_emails = ["eddard@example.com", "robert@example.com"] @TeamInvitesHandler.createInvite = sinon.stub().yields(null) @SubscriptionLocator.getGroupsWithEmailInvite = sinon.stub().yields(null, [@subscription]) it "sends an invitation email to addresses in the legacy invited_emails field", (done) -> - @TeamInvitesHandler.createTeamInvitesForLegacyInvitedEmail "eddard@stark.com", (err, invite) => + @TeamInvitesHandler.createTeamInvitesForLegacyInvitedEmail "eddard@example.com", (err, invite) => expect(err).not.to.exist @TeamInvitesHandler.createInvite.calledWith( @subscription.admin_id, - "eddard@stark.com" + "eddard@example.com" ).should.eq true @TeamInvitesHandler.createInvite.callCount.should.eq 1 @@ -231,13 +241,13 @@ describe "TeamInvitesHandler", -> describe "validation", -> it "doesn't create an invite if the team limit has been reached", (done) -> @LimitationsManager.teamHasReachedMemberLimit = sinon.stub().returns(true) - @TeamInvitesHandler.createInvite @manager.id, "John.Snow@nightwatch.com", (err, invite) => + @TeamInvitesHandler.createInvite @manager.id, "John.Snow@example.com", (err, invite) => expect(err).to.deep.equal(limitReached: true) done() it "doesn't create an invite if the subscription is not in a group plan", (done) -> @subscription.groupPlan = false - @TeamInvitesHandler.createInvite @manager.id, "John.Snow@nightwatch.com", (err, invite) => + @TeamInvitesHandler.createInvite @manager.id, "John.Snow@example.com", (err, invite) => expect(err).to.deep.equal(wrongPlan: true) done() @@ -245,13 +255,13 @@ describe "TeamInvitesHandler", -> member = { id: "1a2b", _id: "1a2b", - email: "tyrion@lannister.com" + email: "tyrion@example.com" } @subscription.member_ids = [member.id] @UserGetter.getUserByAnyEmail.withArgs(member.email).yields(null, member) - @TeamInvitesHandler.createInvite @manager.id, "tyrion@lannister.com", (err, invite) => + @TeamInvitesHandler.createInvite @manager.id, "tyrion@example.com", (err, invite) => expect(err).to.deep.equal(alreadyInTeam: true) expect(invite).not.to.exist done()