From 7cdcd725fd6cce231b933acba313ba93a1ec0c73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 7 Jun 2018 18:44:59 +0200 Subject: [PATCH] Revert "Use Multiple Emails" --- .../CollaboratorsInviteController.coffee | 2 +- .../CollaboratorsInviteHandler.coffee | 2 +- .../SubscriptionGroupHandler.coffee | 2 +- .../coffee/Features/User/UserCreator.coffee | 4 --- .../coffee/Features/User/UserGetter.coffee | 9 +------ .../User/UserRegistrationHandler.coffee | 2 +- .../coffee/Features/User/UserUpdater.coffee | 11 +++++++- services/web/app/coffee/models/User.coffee | 4 --- .../coffee/RegistrationTests.coffee | 14 ---------- .../acceptance/coffee/helpers/User.coffee | 27 ++++--------------- .../CollaboratorsInviteControllerTests.coffee | 12 ++++----- .../CollaboratorsInviteHandlerTests.coffee | 18 ++++++------- .../SubscriptionGroupHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserCreatorTests.coffee | 8 ------ .../unit/coffee/User/UserGetterTests.coffee | 17 ------------ .../User/UserRegistrationHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserUpdaterTests.coffee | 17 +++++++++--- 17 files changed, 57 insertions(+), 108 deletions(-) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee index 8b1b481994..f74a144bac 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee @@ -27,7 +27,7 @@ module.exports = CollaboratorsInviteController = _checkShouldInviteEmail: (email, callback=(err, shouldAllowInvite)->) -> if Settings.restrictInvitesToExistingAccounts == true logger.log {email}, "checking if user exists with this email" - UserGetter.getUserByAnyEmail email, {_id: 1}, (err, user) -> + UserGetter.getUserByMainEmail email, {_id: 1}, (err, user) -> return callback(err) if err? userExists = user? and user?._id? callback(null, userExists) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee index 58121dae2c..b511f56e53 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee @@ -32,7 +32,7 @@ module.exports = CollaboratorsInviteHandler = _trySendInviteNotification: (projectId, sendingUser, invite, callback=(err)->) -> email = invite.email - UserGetter.getUserByAnyEmail email, {_id: 1}, (err, existingUser) -> + UserGetter.getUserByMainEmail email, {_id: 1}, (err, existingUser) -> if err? logger.err {projectId, email}, "error checking if user exists" return callback(err) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index 9463538250..d6ce0dde59 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -21,7 +21,7 @@ module.exports = SubscriptionGroupHandler = if limitReached logger.err adminUserId:adminUserId, newEmail:newEmail, "group subscription limit reached not adding user to group" return callback(limitReached:limitReached) - UserGetter.getUserByAnyEmail newEmail, (err, user)-> + UserGetter.getUserByMainEmail newEmail, (err, user)-> return callback(err) if err? if user? SubscriptionUpdater.addUserToGroup adminUserId, user._id, (err)-> diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 4b4dd30ae2..0a0cc8641e 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -18,10 +18,6 @@ module.exports = UserCreator = user.ace.syntaxValidation = true user.featureSwitches?.pdfng = true - user.emails = [ - email: user.email - createdAt: new Date() - ] user.save (err)-> callback(err, user) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 4ba750be17..2e6d4eada6 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -64,19 +64,12 @@ module.exports = UserGetter = return callback(null, user) if user? db.userstubs.findOne query, projection, callback - # check for duplicate email address. This is also enforced at the DB level - ensureUniqueEmailAddress: (newEmail, callback) -> - @getUserByAnyEmail newEmail, (error, user) -> - return callback(message: 'alread_exists') if user? - callback(error) - [ 'getUser', 'getUserEmail', 'getUserByMainEmail', 'getUserByAnyEmail', 'getUsers', - 'getUserOrUserStubById', - 'ensureUniqueEmailAddress', + 'getUserOrUserStubById' ].map (method) -> metrics.timeAsyncMethod UserGetter, method, 'mongo.UserGetter', logger diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 4a42d7f5fa..fab438ffa6 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -48,7 +48,7 @@ module.exports = UserRegistrationHandler = if !requestIsValid return callback(new Error("request is not valid")) userDetails.email = userDetails.email?.trim()?.toLowerCase() - UserGetter.getUserByAnyEmail userDetails.email, (err, user) => + UserGetter.getUserByMainEmail userDetails.email, (err, user) => if err? return callback err if user?.holdingAccount == false diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 22b31239bd..fa9ee24450 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -43,7 +43,7 @@ module.exports = UserUpdater = # Add a new email address for the user. Email cannot be already used by this # or any other user addEmailAddress: (userId, newEmail, callback) -> - UserGetter.ensureUniqueEmailAddress newEmail, (error) => + @_ensureUniqueEmailAddress newEmail, (error) => return callback(error) if error? update = $push: emails: email: newEmail, createdAt: new Date() @@ -81,11 +81,20 @@ module.exports = UserUpdater = return callback(new Error('Default email does not belong to user')) callback() + + # check for duplicate email address. This is also enforced at the DB level + _ensureUniqueEmailAddress: (newEmail, callback) -> + UserGetter.getUserByAnyEmail newEmail, (error, user) -> + return callback(message: 'alread_exists') if user? + callback() + + [ 'updateUser' 'changeEmailAddress' 'setDefaultEmailAddress' 'addEmailAddress' 'removeEmailAddress' + '_ensureUniqueEmailAddress' ].map (method) -> metrics.timeAsyncMethod(UserUpdater, method, 'mongo.UserUpdater', logger) diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index 71982ba40b..009f582b2a 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -8,10 +8,6 @@ ObjectId = Schema.ObjectId UserSchema = new Schema email : {type : String, default : ''} - emails: [{ - email: { type : String, default : '' }, - createdAt: { type : Date, default: () -> new Date() } - }], first_name : {type : String, default : ''} last_name : {type : String, default : ''} role : {type : String, default : ''} diff --git a/services/web/test/acceptance/coffee/RegistrationTests.coffee b/services/web/test/acceptance/coffee/RegistrationTests.coffee index 8bcf8c3685..89d1bf3299 100644 --- a/services/web/test/acceptance/coffee/RegistrationTests.coffee +++ b/services/web/test/acceptance/coffee/RegistrationTests.coffee @@ -128,20 +128,6 @@ describe "CSRF protection", -> expect(response.statusCode).to.equal 403 done() -describe "Register", -> - before -> - @user = new User() - - it 'Set emails attribute', (done) -> - @user.register (error, user) => - expect(error).to.not.exist - user.email.should.equal @user.email - user.emails.should.exist - user.emails.should.be.a 'array' - user.emails.length.should.equal 1 - user.emails[0].email.should.equal @user.email - done() - describe "LoginViaRegistration", -> before (done) -> diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 793ac6cd9f..f83780b535 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -22,30 +22,9 @@ class User jar: @jar }) - setExtraAttributes: (user) -> - throw new Error("User does not exist") unless user?._id? - @id = user._id.toString() - @_id = user._id.toString() - @first_name = user.first_name - @referal_id = user.referal_id - get: (callback = (error, user)->) -> db.users.findOne { _id: ObjectId(@_id) }, callback - register: (callback = (error, user) ->) -> - return callback(new Error('User already registered')) if @_id? - @getCsrfToken (error) => - return callback(error) if error? - @request.post { - url: '/register' - json: { @email, @password } - }, (error, response, body) => - return callback(error) if error? - db.users.findOne { email: @email }, (error, user) => - return callback(error) if error? - @setExtraAttributes user - callback(null, user) - login: (callback = (error) ->) -> @loginWith(@email, callback) @@ -68,7 +47,11 @@ class User return callback(error) if error? UserUpdater.updateUser user._id, $set: emails: @emails, (error) => return callback(error) if error? - @setExtraAttributes user + @id = user?._id?.toString() + @_id = user?._id?.toString() + @first_name = user?.first_name + @referal_id = user?.referal_id + callback(null, @password) setFeatures: (features, callback = (error) ->) -> diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee index affef2dc31..4b57ff3697 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee @@ -25,7 +25,7 @@ describe "CollaboratorsInviteController", -> @LimitationsManager = {} @UserGetter = - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() getUser: sinon.stub() @CollaboratorsInviteController = SandboxedModule.require modulePath, requires: @@ -716,7 +716,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = {_id: ObjectId().toString()} - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `true`', (done) -> @call (err, shouldAllow) => @@ -728,7 +728,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = null - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `false`', (done) -> @call (err, shouldAllow) => @@ -738,15 +738,15 @@ describe "CollaboratorsInviteController", -> it 'should have called getUser', (done) -> @call (err, shouldAllow) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@email, {_id: 1}).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@email, {_id: 1}).should.equal true done() describe 'when getUser produces an error', -> beforeEach -> @user = null - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should callback with an error', (done) -> @call (err, shouldAllow) => diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee index edad39a637..58b373f61f 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee @@ -605,7 +605,7 @@ describe "CollaboratorsInviteHandler", -> _id: ObjectId() first_name: "jim" @existingUser = {_id: ObjectId()} - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @existingUser) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @existingUser) @fakeProject = _id: @project_id name: "some project" @@ -626,8 +626,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should call getProject', (done) -> @@ -671,7 +671,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the user does not exist', -> beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, null) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, null) it 'should not produce an error', (done) -> @call (err) => @@ -680,8 +680,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> @@ -698,7 +698,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the getUser produces an error', -> beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should produce an error', (done) -> @call (err) => @@ -707,8 +707,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee index 9a70ca0411..bca9ac7600 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee @@ -32,7 +32,7 @@ describe "SubscriptionGroupHandler", -> @UserGetter = getUser: sinon.stub() - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() @LimitationsManager = hasGroupMembersLimitReached: sinon.stub() @@ -71,11 +71,11 @@ describe "SubscriptionGroupHandler", -> describe "addUserToGroup", -> beforeEach -> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, false, @subscription) - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) it "should find the user", (done)-> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> - @UserGetter.getUserByAnyEmail.calledWith(@newEmail).should.equal true + @UserGetter.getUserByMainEmail.calledWith(@newEmail).should.equal true done() it "should add the user to the group", (done)-> @@ -102,7 +102,7 @@ describe "SubscriptionGroupHandler", -> done() it "should add an email invite if no user is found", (done) -> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, null) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, null) @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @SubscriptionUpdater.addEmailInviteToGroup.calledWith(@adminUser_id, @newEmail).should.equal true done() diff --git a/services/web/test/unit/coffee/User/UserCreatorTests.coffee b/services/web/test/unit/coffee/User/UserCreatorTests.coffee index 3764c8a765..cc2b1ec150 100644 --- a/services/web/test/unit/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/unit/coffee/User/UserCreatorTests.coffee @@ -70,11 +70,3 @@ describe "UserCreator", -> assert.equal user.holdingAccount, true assert.equal user.last_name, "lastNammmmeee" done() - - it "should set emails attribute", (done)-> - @UserCreator.createNewUser email: @email, (err, user)=> - user.email.should.equal @email - user.emails.length.should.equal 1 - user.emails[0].email.should.equal @email - user.emails[0].createdAt.should.be.a 'date' - done() diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index fbea0e8b53..7fb14a7f7d 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -85,20 +85,3 @@ describe "UserGetter", -> @findOne.calledTwice.should.equal true @findOne.calledWith(email: email, projection).should.equal true done() - - describe 'ensureUniqueEmailAddress', -> - beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub() - - it 'should return error if existing user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @fakeUser) - @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> - should.exist(err) - err.message.should.equal 'alread_exists' - done() - - it 'should return null if no user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1) - @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> - should.not.exist(err) - done() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index e738947171..9411059022 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee @@ -14,7 +14,7 @@ describe "UserRegistrationHandler", -> @User = update: sinon.stub().callsArgWith(2) @UserGetter = - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() @UserCreator = createNewUser:sinon.stub().callsArgWith(1, null, @user) @AuthenticationManager = @@ -72,7 +72,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @user.holdingAccount = true @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) it "should not create a new user if there is a holding account there", (done)-> @handler.registerNewUser @passingRequest, (err)=> @@ -96,7 +96,7 @@ describe "UserRegistrationHandler", -> done() it "should return email registered in the error if there is a non holdingAccount there", (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user = {holdingAccount:false}) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user = {holdingAccount:false}) @handler.registerNewUser @passingRequest, (err, user)=> err.should.deep.equal new Error("EmailAlreadyRegistered") user.should.deep.equal @user @@ -105,7 +105,7 @@ describe "UserRegistrationHandler", -> describe "validRequest", -> beforeEach -> @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByAnyEmail.callsArgWith 1 + @UserGetter.getUserByMainEmail.callsArgWith 1 it "should create a new user", (done)-> @handler.registerNewUser @passingRequest, (err)=> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 7b3be3df2b..b952a688ae 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -17,7 +17,6 @@ describe "UserUpdater", -> @UserGetter = getUserEmail: sinon.stub() getUserByAnyEmail: sinon.stub() - ensureUniqueEmailAddress: sinon.stub() @logger = err: sinon.stub(), log: -> @UserUpdater = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings @@ -61,13 +60,13 @@ describe "UserUpdater", -> describe 'addEmailAddress', -> beforeEach -> - @UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) + @UserUpdater._ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) it 'add email', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> - @UserGetter.ensureUniqueEmailAddress.called.should.equal true + @UserUpdater._ensureUniqueEmailAddress.called.should.equal true should.not.exist(err) @UserUpdater.updateUser.calledWith( @stubbedUser._id, @@ -136,3 +135,15 @@ describe "UserUpdater", -> done() + describe '_ensureUniqueEmailAddress', -> + it 'should return error if existing user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @stubbedUser) + @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> + should.exist(err) + done() + + it 'should return null if no user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1) + @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> + should.not.exist(err) + done()