From c5530163f54893e3d6ca9cf8aae29ac9b1af3c88 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 28 May 2018 16:08:37 +0200 Subject: [PATCH] add emails attribute on user creation --- .../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 +++++++++++++++---- .../unit/coffee/User/UserCreatorTests.coffee | 8 ++++++ .../unit/coffee/User/UserGetterTests.coffee | 17 ++++++++++++ .../User/UserRegistrationHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserUpdaterTests.coffee | 17 +++--------- 11 files changed, 86 insertions(+), 35 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 0a0cc8641e..4b4dd30ae2 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -18,6 +18,10 @@ 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 10a975613e..2d55d0e432 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -62,12 +62,19 @@ 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' + 'getUserOrUserStubById', + 'ensureUniqueEmailAddress', ].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 fab438ffa6..4a42d7f5fa 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.getUserByMainEmail userDetails.email, (err, user) => + UserGetter.getUserByAnyEmail 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 fa9ee24450..22b31239bd 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) -> - @_ensureUniqueEmailAddress newEmail, (error) => + UserGetter.ensureUniqueEmailAddress newEmail, (error) => return callback(error) if error? update = $push: emails: email: newEmail, createdAt: new Date() @@ -81,20 +81,11 @@ 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 009f582b2a..71982ba40b 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -8,6 +8,10 @@ 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 89d1bf3299..8bcf8c3685 100644 --- a/services/web/test/acceptance/coffee/RegistrationTests.coffee +++ b/services/web/test/acceptance/coffee/RegistrationTests.coffee @@ -128,6 +128,20 @@ 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 f83780b535..793ac6cd9f 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -22,9 +22,30 @@ 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) @@ -47,11 +68,7 @@ class User return callback(error) if error? UserUpdater.updateUser user._id, $set: emails: @emails, (error) => return callback(error) if error? - @id = user?._id?.toString() - @_id = user?._id?.toString() - @first_name = user?.first_name - @referal_id = user?.referal_id - + @setExtraAttributes user callback(null, @password) setFeatures: (features, callback = (error) ->) -> diff --git a/services/web/test/unit/coffee/User/UserCreatorTests.coffee b/services/web/test/unit/coffee/User/UserCreatorTests.coffee index cc2b1ec150..3764c8a765 100644 --- a/services/web/test/unit/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/unit/coffee/User/UserCreatorTests.coffee @@ -70,3 +70,11 @@ 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 cdc22772f3..9ce4c843ff 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -74,3 +74,20 @@ 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 9411059022..e738947171 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 = - getUserByMainEmail: sinon.stub() + getUserByAnyEmail: 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.getUserByMainEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByAnyEmail.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.getUserByMainEmail.callsArgWith(1, null, @user = {holdingAccount:false}) + @UserGetter.getUserByAnyEmail.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.getUserByMainEmail.callsArgWith 1 + @UserGetter.getUserByAnyEmail.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 b952a688ae..7b3be3df2b 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -17,6 +17,7 @@ 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 @@ -60,13 +61,13 @@ describe "UserUpdater", -> describe 'addEmailAddress', -> beforeEach -> - @UserUpdater._ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) + @UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) it 'add email', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> - @UserUpdater._ensureUniqueEmailAddress.called.should.equal true + @UserGetter.ensureUniqueEmailAddress.called.should.equal true should.not.exist(err) @UserUpdater.updateUser.calledWith( @stubbedUser._id, @@ -135,15 +136,3 @@ 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()