From 613c9193e3ed7dfc093f024621be87c254f317f4 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Fri, 25 May 2018 13:04:09 +0200 Subject: [PATCH 1/5] implement multi emails logic --- .../coffee/Features/User/UserGetter.coffee | 18 +++ .../coffee/Features/User/UserUpdater.coffee | 93 +++++++++++-- services/web/app/coffee/models/User.coffee | 4 + .../coffee/RegistrationTests.coffee | 9 ++ .../acceptance/coffee/SettingsTests.coffee | 28 ++++ .../acceptance/coffee/helpers/User.coffee | 49 ++++++- .../unit/coffee/User/UserGetterTests.coffee | 18 +++ .../unit/coffee/User/UserUpdaterTests.coffee | 126 ++++++++++++++++-- 8 files changed, 312 insertions(+), 33 deletions(-) create mode 100644 services/web/test/acceptance/coffee/SettingsTests.coffee diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 201981625e..10a975613e 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -21,6 +21,10 @@ module.exports = UserGetter = db.users.findOne query, projection, callback + getUserEmail: (userId, callback = (error, email) ->) -> + @getUser userId, { email: 1 }, (error, user) -> + callback(error, user?.email) + getUserByMainEmail: (email, projection, callback = (error, user) ->) -> email = email.trim() if arguments.length == 2 @@ -28,6 +32,18 @@ module.exports = UserGetter = projection = {} db.users.findOne email: email, projection, callback + getUserByAnyEmail: (email, projection, callback = (error, user) ->) -> + email = email.trim() + if arguments.length == 2 + callback = projection + projection = {} + db.users.findOne 'emails.email': email, projection, (error, user) => + return callback(error, user) if error? or user? + + # While multiple emails are being rolled out, check for the main email as + # well + @getUserByMainEmail email, projection, callback + getUsers: (user_ids, projection, callback = (error, users) ->) -> try user_ids = user_ids.map (u) -> ObjectId(u.toString()) @@ -48,7 +64,9 @@ module.exports = UserGetter = [ 'getUser', + 'getUserEmail', 'getUserByMainEmail', + 'getUserByAnyEmail', 'getUsers', 'getUserOrUserStubById' ].map (method) -> diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 174d37bc38..fa9ee24450 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -2,6 +2,7 @@ logger = require("logger-sharelatex") mongojs = require("../../infrastructure/mongojs") metrics = require("metrics-sharelatex") db = mongojs.db +async = require("async") ObjectId = mongojs.ObjectId UserGetter = require("./UserGetter") @@ -11,23 +12,89 @@ module.exports = UserUpdater = query = _id: ObjectId(query) else if query instanceof ObjectId query = _id: query + else if typeof query._id == "string" + query._id = ObjectId(query._id) db.users.update query, update, callback - changeEmailAddress: (user_id, newEmail, callback)-> - self = @ - logger.log user_id:user_id, newEmail:newEmail, "updaing email address of user" - UserGetter.getUserByMainEmail newEmail, (error, user) -> - if user? - return callback({message:"alread_exists"}) - self.updateUser user_id.toString(), { - $set: { "email": newEmail}, - }, (err) -> - if err? - logger.err err:err, "problem updating users email" - return callback(err) + # + # DEPRECATED + # + # Change the user's main email address by adding a new email, switching the + # default email and removing the old email. Prefer manipulating multiple + # emails and the default rather than calling this method directly + # + changeEmailAddress: (userId, newEmail, callback)-> + logger.log userId: userId, newEmail: newEmail, "updaing email address of user" + + oldEmail = null + async.series [ + (cb) -> + UserGetter.getUserEmail userId, (error, email) -> + oldEmail = email + cb(error) + (cb) -> UserUpdater.addEmailAddress userId, newEmail, cb + (cb) -> UserUpdater.setDefaultEmailAddress userId, newEmail, cb + (cb) -> UserUpdater.removeEmailAddress userId, oldEmail, cb + ], callback + + + # 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) => + return callback(error) if error? + + update = $push: emails: email: newEmail, createdAt: new Date() + @updateUser userId, update, (error) -> + if error? + logger.err error: error, 'problem updating users emails' + return callback(error) callback() -metrics.timeAsyncMethod UserUpdater, 'updateUser', 'mongo.UserUpdater', logger + # remove one of the user's email addresses. The email cannot be the user's + # default email address + removeEmailAddress: (userId, email, callback) -> + query = _id: userId, email: $ne: email + update = $pull: emails: email: email + @updateUser query, update, (error, res) -> + if error? + logger.err error:error, 'problem removing users email' + return callback(error) + if res.nMatched == 0 + return callback(new Error('Cannot remove default email')) + callback() + + + # set the default email address by setting the `email` attribute. The email + # must be one of the user's multiple emails (`emails` attribute) + setDefaultEmailAddress: (userId, email, callback) -> + query = _id: userId, 'emails.email': email + update = $set: email: email + @updateUser query, update, (error, res) -> + if error? + logger.err error:error, 'problem setting default emails' + return callback(error) + if res.nMatched == 0 + 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 313722a71b..89d1bf3299 100644 --- a/services/web/test/acceptance/coffee/RegistrationTests.coffee +++ b/services/web/test/acceptance/coffee/RegistrationTests.coffee @@ -146,6 +146,15 @@ describe "LoginViaRegistration", -> describe "[Security] Trying to register/login as another user", -> + it 'should not allow sign in with secondary email', (done) -> + secondaryEmail = "acceptance-test-secondary@example.com" + @user1.addEmail secondaryEmail, (err) => + @user1.loginWith secondaryEmail, (err) => + expect(err?).to.equal false + @user1.isLoggedIn (err, isLoggedIn) -> + expect(isLoggedIn).to.equal false + done() + it 'should have user1 login', (done) -> @user1.login (err) -> expect(err?).to.equal false diff --git a/services/web/test/acceptance/coffee/SettingsTests.coffee b/services/web/test/acceptance/coffee/SettingsTests.coffee new file mode 100644 index 0000000000..bd2942e072 --- /dev/null +++ b/services/web/test/acceptance/coffee/SettingsTests.coffee @@ -0,0 +1,28 @@ +should = require('chai').should() +async = require("async") +User = require "./helpers/User" + +describe 'SettingsPage', -> + + before (done) -> + @user = new User() + async.series [ + @user.ensureUserExists.bind(@user) + @user.login.bind(@user) + @user.activateSudoMode.bind(@user) + ], done + + it 'load settigns page', (done) -> + @user.getUserSettingsPage (err, statusCode) -> + statusCode.should.equal 200 + done() + + it 'update main email address', (done) -> + newEmail = 'foo@bar.com' + @user.updateSettings email: newEmail, (error) => + should.not.exist error + @user.get (error, user) -> + user.email.should.equal newEmail + user.emails.length.should.equal 1 + user.emails[0].email.should.equal newEmail + done() diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 61c2e84a8c..f83780b535 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -3,13 +3,18 @@ _ = require("underscore") settings = require("settings-sharelatex") {db, ObjectId} = require("../../../../app/js/infrastructure/mongojs") UserModel = require("../../../../app/js/models/User").User +UserUpdater = require("../../../../app/js/Features/User/UserUpdater") AuthenticationManager = require("../../../../app/js/Features/Authentication/AuthenticationManager") count = 0 class User constructor: (options = {}) -> - @email = "acceptance-test-#{count}@example.com" + @emails = [ + email: "acceptance-test-#{count}@example.com" + createdAt: new Date() + ] + @email = @emails[0].email @password = "acceptance-test-#{count}-password" count++ @jar = request.jar() @@ -17,14 +22,20 @@ class User jar: @jar }) + get: (callback = (error, user)->) -> + db.users.findOne { _id: ObjectId(@_id) }, callback + login: (callback = (error) ->) -> + @loginWith(@email, callback) + + loginWith: (email, callback = (error) ->) -> @ensureUserExists (error) => return callback(error) if error? @getCsrfToken (error) => return callback(error) if error? @request.post { url: "/login" - json: { @email, @password } + json: { email, @password } }, callback ensureUserExists: (callback = (error) ->) -> @@ -34,11 +45,14 @@ class User return callback(error) if error? AuthenticationManager.setUserPassword user._id, @password, (error) => return callback(error) if error? - @id = user?._id?.toString() - @_id = user?._id?.toString() - @first_name = user?.first_name - @referal_id = user?.referal_id - callback(null, @password) + 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 + + callback(null, @password) setFeatures: (features, callback = (error) ->) -> update = {} @@ -62,6 +76,10 @@ class User @_id = user?._id?.toString() callback() + addEmail: (email, callback = (error) ->) -> + @emails.push(email: email, createdAt: new Date()) + UserUpdater.addEmailAddress @id, email, callback + ensure_admin: (callback = (error) ->) -> db.users.update {_id: ObjectId(@id)}, { $set: { isAdmin: true }}, callback @@ -226,6 +244,23 @@ class User return callback(error) if error? callback(null, response.statusCode) + activateSudoMode: (callback = (error)->) -> + @getCsrfToken (error) => + return callback(error) if error? + @request.post { + uri: '/confirm-password', + json: + password: @password + }, callback + + updateSettings: (newSettings, callback = (error, response, body) ->) -> + @getCsrfToken (error) => + return callback(error) if error? + @request.post { + url: '/user/settings' + json: newSettings + }, callback + getProjectListPage: (callback=(error, statusCode)->) -> @getCsrfToken (error) => return callback(error) if error? diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index 2c32bd8250..cdc22772f3 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -56,3 +56,21 @@ describe "UserGetter", -> @findOne.called.should.equal true @findOne.calledWith(email: email).should.equal true done() + + describe "getUserByAnyEmail", -> + it "query user for any email", (done)-> + email = 'hello@world.com' + projection = emails: 1 + @UserGetter.getUserByAnyEmail " #{email} ", projection, (error, user) => + @findOne.calledWith('emails.email': email, projection).should.equal true + user.should.deep.equal @fakeUser + done() + + it "checks main email as well", (done)-> + @findOne.callsArgWith(2, null, null) + email = 'hello@world.com' + projection = emails: 1 + @UserGetter.getUserByAnyEmail " #{email} ", projection, (error, user) => + @findOne.calledTwice.should.equal true + @findOne.calledWith(email: email, projection).should.equal true + done() diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 202a4f3f1a..b952a688ae 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -15,35 +15,135 @@ describe "UserUpdater", -> db:{} ObjectId:(id)-> return id @UserGetter = - getUserByMainEmail: sinon.stub() + getUserEmail: sinon.stub() + getUserByAnyEmail: sinon.stub() + @logger = err: sinon.stub(), log: -> @UserUpdater = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings - "logger-sharelatex": log:-> + "logger-sharelatex": @logger "./UserGetter": @UserGetter "../../infrastructure/mongojs":@mongojs "metrics-sharelatex": timeAsyncMethod: sinon.stub() @stubbedUser = + _id: "3131231" name:"bob" email:"hello@world.com" - @user_id = "3131231" @newEmail = "bob@bob.com" - describe "changeEmailAddress", -> + describe 'changeEmailAddress', -> beforeEach -> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2) + @UserGetter.getUserEmail.callsArgWith(1, null, @stubbedUser.email) + @UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2) + @UserUpdater.setDefaultEmailAddress = sinon.stub().callsArgWith(2) + @UserUpdater.removeEmailAddress = sinon.stub().callsArgWith(2) - it "should check if the new email already has an account", (done)-> - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @stubbedUser) - @UserUpdater.changeEmailAddress @user_id, @stubbedUser.email, (err)=> - @UserUpdater.updateUser.called.should.equal false + it 'change email', (done)-> + @UserUpdater.changeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @UserUpdater.addEmailAddress.calledWith( + @stubbedUser._id, @newEmail + ).should.equal true + @UserUpdater.setDefaultEmailAddress.calledWith( + @stubbedUser._id, @newEmail + ).should.equal true + @UserUpdater.removeEmailAddress.calledWith( + @stubbedUser._id, @stubbedUser.email + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.removeEmailAddress.callsArgWith(2, new Error('nope')) + @UserUpdater.changeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + describe 'addEmailAddress', -> + beforeEach -> + @UserUpdater._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 + should.not.exist(err) + @UserUpdater.updateUser.calledWith( + @stubbedUser._id, + $push: { emails: { email: @newEmail, createdAt: sinon.match.date } } + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) + + @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> + @logger.err.called.should.equal true + should.exist(err) + done() + + describe 'removeEmailAddress', -> + it 'remove email', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + + @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @UserUpdater.updateUser.calledWith( + { _id: @stubbedUser._id, email: { $ne: @newEmail } }, + $pull: { emails: { email: @newEmail } } + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) + + @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + it 'handle missed update', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + + @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + describe 'setDefaultEmailAddress', -> + it 'set default', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + + @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @UserUpdater.updateUser.calledWith( + { _id: @stubbedUser._id, 'emails.email': @newEmail }, + $set: { email: @newEmail } + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) + + @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + it 'handle missed update', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + + @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> should.exist(err) done() - it "should set the users password", (done)-> - @UserGetter.getUserByMainEmail.callsArgWith(1, null) - @UserUpdater.changeEmailAddress @user_id, @newEmail, (err)=> - @UserUpdater.updateUser.calledWith(@user_id, $set: { "email": @newEmail}).should.equal true + 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() From 1f6fcafce6636a950b6ebc588788a99767298a2b Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 6 Jun 2018 11:11:57 +0200 Subject: [PATCH 2/5] remove default emails attribute on user model --- services/web/app/coffee/models/User.coffee | 4 ---- 1 file changed, 4 deletions(-) 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 : ''} From e4da74825768bf20ae23f9de85453c8ac10d1551 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 6 Jun 2018 14:52:09 +0200 Subject: [PATCH 3/5] add filter to query on emails attribute --- services/web/app/coffee/Features/User/UserGetter.coffee | 3 ++- services/web/test/unit/coffee/User/UserGetterTests.coffee | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 10a975613e..3160258b24 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -37,7 +37,8 @@ module.exports = UserGetter = if arguments.length == 2 callback = projection projection = {} - db.users.findOne 'emails.email': email, projection, (error, user) => + query = emails: { $exists: true }, 'emails.email': email + db.users.findOne query, projection, (error, user) => return callback(error, user) if error? or user? # While multiple emails are being rolled out, check for the main email as diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index cdc22772f3..b25bab87a7 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -60,9 +60,12 @@ describe "UserGetter", -> describe "getUserByAnyEmail", -> it "query user for any email", (done)-> email = 'hello@world.com' + expectedQuery = + emails: { $exists: true } + 'emails.email': email projection = emails: 1 @UserGetter.getUserByAnyEmail " #{email} ", projection, (error, user) => - @findOne.calledWith('emails.email': email, projection).should.equal true + @findOne.calledWith(expectedQuery, projection).should.equal true user.should.deep.equal @fakeUser done() From 7726314e7c8eed9d6e3620f81ca66448b7ec4184 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 6 Jun 2018 15:45:12 +0200 Subject: [PATCH 4/5] add test to explicitely check filter --- services/web/test/unit/coffee/User/UserGetterTests.coffee | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index b25bab87a7..7fb14a7f7d 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -69,6 +69,14 @@ describe "UserGetter", -> user.should.deep.equal @fakeUser done() + it "query contains $exists:true so partial index is used", (done)-> + expectedQuery = + emails: { $exists: true } + 'emails.email': '' + @UserGetter.getUserByAnyEmail '', {}, (error, user) => + @findOne.calledWith(expectedQuery, {}).should.equal true + done() + it "checks main email as well", (done)-> @findOne.callsArgWith(2, null, null) email = 'hello@world.com' From 3cb499a3c2aac549e0dc04f7949083f5519761c8 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 6 Jun 2018 15:46:41 +0200 Subject: [PATCH 5/5] add comment --- services/web/app/coffee/Features/User/UserGetter.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 3160258b24..2e6d4eada6 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -37,6 +37,7 @@ module.exports = UserGetter = if arguments.length == 2 callback = projection projection = {} + # $exists: true MUST be set to use the partial index query = emails: { $exists: true }, 'emails.email': email db.users.findOne query, projection, (error, user) => return callback(error, user) if error? or user?