From e139abb11001a02f15e66ad462ec1cf8713adbb6 Mon Sep 17 00:00:00 2001 From: Paulo Jorge Reis Date: Tue, 18 Dec 2018 11:14:41 +0000 Subject: [PATCH] Merge pull request #1273 from sharelatex/ja-password-reset-v1 Handle v1-only users in v2 password reset flow GitOrigin-RevId: 38ce8e9aebd3330b980e73640a23661d8015d4f3 --- .../AuthenticationManager.coffee | 30 +-- .../PasswordResetController.coffee | 1 + .../PasswordReset/PasswordResetHandler.coffee | 56 ++++-- .../app/coffee/Features/V1/V1Handler.coffee | 13 +- .../acceptance/coffee/helpers/User.coffee | 2 +- .../PasswordResetControllerTests.coffee | 9 +- .../PasswordResetHandlerTests.coffee | 173 ++++++++++++++---- 7 files changed, 208 insertions(+), 76 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 1e4dfb7caf..e2531332f2 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -62,21 +62,17 @@ module.exports = AuthenticationManager = UserGetter.getUser user_id, { email:1, overleaf: 1 }, (error, user) -> return callback(error) if error? - overleafId = user.overleaf?.id? - if overleafId and Settings.overleaf? # v2 user in v2 + v1IdExists = user.overleaf?.id? + if v1IdExists and Settings.overleaf? # v2 user in v2 # v2 user in v2, change password in v1 - AuthenticationManager._setUserPasswordInV1({ - v1Id: user.overleaf.id, - email: user.email, - password: password - }, callback) - else if overleafId and !Settings.overleaf? + AuthenticationManager.setUserPasswordInV1(user.overleaf.id, password, callback) + else if v1IdExists and !Settings.overleaf? # v2 user in SL return callback(new Errors.NotInV2Error("Password Reset Attempt")) - else if !overleafId and !Settings.overleaf? + else if !v1IdExists and !Settings.overleaf? # SL user in SL, change password in SL - AuthenticationManager._setUserPasswordInV2(user_id, password, callback) - else if !overleafId and Settings.overleaf? + AuthenticationManager.setUserPasswordInV2(user_id, password, callback) + else if !v1IdExists and Settings.overleaf? # SL user in v2, should not happen return callback(new Errors.SLInV2Error("Password Reset Attempt")) else @@ -90,7 +86,10 @@ module.exports = AuthenticationManager = else callback() - _setUserPasswordInV2: (user_id, password, callback) -> + setUserPasswordInV2: (user_id, password, callback) -> + validation = @validatePassword(password) + return callback(validation.message) if validation? + bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) -> return callback(error) if error? bcrypt.hash password, salt, (error, hash) -> @@ -105,7 +104,10 @@ module.exports = AuthenticationManager = _checkWriteResult(result, callback) ) - _setUserPasswordInV1: (user, callback) -> - V1Handler.doPasswordReset user, (error, reset)-> + setUserPasswordInV1: (v1_user_id, password, callback) -> + validation = @validatePassword(password) + return callback(validation.message) if validation? + + V1Handler.doPasswordReset v1_user_id, password, (error, reset)-> return callback(error) if error? return callback(error, reset) diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee index 816539b83b..bc15576396 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee @@ -57,6 +57,7 @@ module.exports = else if err and !err.statusCode res.status(500) else if found + return res.sendStatus 200 if !user_id? # will not exist for v1-only users UserSessionsManager.revokeAllUserSessions {_id: user_id}, [], (err) -> return next(err) if err? if req.body.login_after diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee index d0e7867d97..399646418c 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee @@ -5,16 +5,15 @@ OneTimeTokenHandler = require("../Security/OneTimeTokenHandler") EmailHandler = require("../Email/EmailHandler") AuthenticationManager = require("../Authentication/AuthenticationManager") logger = require("logger-sharelatex") +V1Api = require("../V1/V1Api") -module.exports = +module.exports = PasswordResetHandler = generateAndEmailResetToken:(email, callback = (error, exists) ->)-> - UserGetter.getUserByMainEmail email, (err, user)-> - if err then return callback(err) - if !user? or user.holdingAccount - logger.err email:email, "user could not be found for password reset" - return callback(null, false) - OneTimeTokenHandler.getNewToken 'password', user._id, (err, token)-> + PasswordResetHandler._getPasswordResetData email, (error, exists, data) -> + if error? or !exists + return callback(error, exists) + OneTimeTokenHandler.getNewToken 'password', data, (err, token)-> if err then return callback(err) emailOptions = to : email @@ -24,10 +23,45 @@ module.exports = callback null, true setNewUserPassword: (token, password, callback = (error, found, user_id) ->)-> - OneTimeTokenHandler.getValueFromTokenAndExpire 'password', token, (err, user_id)-> + OneTimeTokenHandler.getValueFromTokenAndExpire 'password', token, (err, data)-> if err then return callback(err) - if !user_id? + if !data? return callback null, false, null - AuthenticationManager.setUserPassword user_id, password, (err, reset) -> + if typeof data == "string" + # Backwards compatible with old format. + # Tokens expire after 1h, so this can be removed soon after deploy. + # Possibly we should keep this until we do an onsite release too. + data = { user_id: data } + if data.user_id? + AuthenticationManager.setUserPassword data.user_id, password, (err, reset) -> + if err then return callback(err) + callback null, reset, data.user_id + else if data.v1_user_id? + AuthenticationManager.setUserPasswordInV1 data.v1_user_id, password, (error, reset) -> + return callback(error) if error? + UserGetter.getUser { 'overleaf.id': data.v1_user_id }, {_id:1}, (error, user) -> + return callback(error) if error? + callback null, reset, user?._id + + _getPasswordResetData: (email, callback = (error, exists, data) ->) -> + if settings.overleaf? + # Overleaf v2 + V1Api.request { + url: "/api/v1/sharelatex/user_emails" + qs: + email: email + expectedStatusCodes: [404] + }, (error, response, body) -> + return callback(error) if error? + if response.statusCode == 404 + return callback null, false + else + return callback null, true, { v1_user_id: body.user_id } + else + # ShareLaTeX + UserGetter.getUserByMainEmail email, (err, user)-> if err then return callback(err) - callback null, reset, user_id + if !user? or user.holdingAccount or user.overleaf? + logger.err email:email, "user could not be found for password reset" + return callback(null, false) + return callback null, true, { user_id: user._id } diff --git a/services/web/app/coffee/Features/V1/V1Handler.coffee b/services/web/app/coffee/Features/V1/V1Handler.coffee index 1bfda802d9..92ed0456d3 100644 --- a/services/web/app/coffee/Features/V1/V1Handler.coffee +++ b/services/web/app/coffee/Features/V1/V1Handler.coffee @@ -26,24 +26,23 @@ module.exports = V1Handler = err = new Error("Unexpected status from v1 login api: #{response.statusCode}") callback(err) - doPasswordReset: (userData, callback=(err, created)->) -> - logger.log({v1Id: userData.v1Id, email: userData.email}, + doPasswordReset: (v1_user_id, password, callback=(err, created)->) -> + logger.log({v1_user_id}, "sending password reset request to v1 login api") V1Api.request { method: 'POST' url: "/api/v1/sharelatex/reset_password" json: { - user_id: userData.v1Id, - email: userData.email, - password: userData.password + user_id: v1_user_id, + password: password } expectedStatusCodes: [200] }, (err, response, body) -> if err? - logger.err {email: userData.email, err}, "error while talking to v1 password reset api" + logger.err {v1_user_id, err}, "error while talking to v1 password reset api" return callback(err, false) if response.statusCode in [200] - logger.log {email: userData.email, changed: true}, "got success response from v1 password reset api" + logger.log {v1_user_id, changed: true}, "got success response from v1 password reset api" callback(null, true) else err = new Error("Unexpected status from v1 password reset api: #{response.statusCode}") diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 551d0c8893..7740c9189b 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -70,7 +70,7 @@ class User options = {upsert: true, new: true, setDefaultsOnInsert: true} UserModel.findOneAndUpdate filter, {}, options, (error, user) => return callback(error) if error? - AuthenticationManager._setUserPasswordInV2 user._id, @password, (error) => + AuthenticationManager.setUserPasswordInV2 user._id, @password, (error) => return callback(error) if error? UserUpdater.updateUser user._id, $set: emails: @emails, (error) => return callback(error) if error? diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee index d11507361c..fd118e9c88 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee @@ -29,6 +29,7 @@ describe "PasswordResetController", -> "../User/UserSessionsManager": @UserSessionsManager @email = "bob@bob.com " + @user_id = 'mock-user-id' @token = "my security token that was emailed to me" @password = "my new password" @req = @@ -98,7 +99,7 @@ describe "PasswordResetController", -> @req.session.resetToken = @token it "should tell the user handler to reset the password", (done)-> - @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true) + @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true, @user_id) @res.sendStatus = (code)=> code.should.equal 200 @PasswordResetHandler.setNewUserPassword.calledWith(@token, @password).should.equal true @@ -106,7 +107,7 @@ describe "PasswordResetController", -> @PasswordResetController.setNewUserPassword @req, @res it "should send 404 if the token didn't work", (done)-> - @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, false) + @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, false, @user_id) @res.sendStatus = (code)=> code.should.equal 404 done() @@ -131,7 +132,7 @@ describe "PasswordResetController", -> @PasswordResetController.setNewUserPassword @req, @res it "should clear the session.resetToken", (done) -> - @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true) + @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true, @user_id) @res.sendStatus = (code)=> code.should.equal 200 @req.session.should.not.have.property 'resetToken' @@ -139,7 +140,7 @@ describe "PasswordResetController", -> @PasswordResetController.setNewUserPassword @req, @res it 'should clear sessions', (done) -> - @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true) + @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true, @user_id) @res.sendStatus = (code)=> @UserSessionsManager.revokeAllUserSessions.callCount.should.equal 1 done() diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee index 14bde68c44..0da9968d2c 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee @@ -17,15 +17,21 @@ describe "PasswordResetHandler", -> getValueFromTokenAndExpire:sinon.stub() @UserGetter = getUserByMainEmail:sinon.stub() + getUser: sinon.stub() @EmailHandler = sendEmail:sinon.stub() @AuthenticationManager = setUserPassword:sinon.stub() + setUserPasswordInV1:sinon.stub() + setUserPasswordInV2:sinon.stub() + @V1Api = + request: sinon.stub() @PasswordResetHandler = SandboxedModule.require modulePath, requires: "../User/UserGetter": @UserGetter "../Security/OneTimeTokenHandler": @OneTimeTokenHandler "../Email/EmailHandler":@EmailHandler "../Authentication/AuthenticationManager":@AuthenticationManager + "../V1/V1Api": @V1Api "settings-sharelatex": @settings "logger-sharelatex": log:-> @@ -33,56 +39,145 @@ describe "PasswordResetHandler", -> @token = "12312321i" @user_id = "user_id_here" @user = - email :"bob@bob.com" + email : @email = "bob@bob.com" @password = "my great secret password" + @callback = sinon.stub() describe "generateAndEmailResetToken", -> + describe "when in ShareLaTeX", -> + it "should check the user exists", (done)-> + @UserGetter.getUserByMainEmail.callsArgWith(1) + @OneTimeTokenHandler.getNewToken.yields() + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> + exists.should.equal false + done() - it "should check the user exists", (done)-> - @UserGetter.getUserByMainEmail.callsArgWith(1) - @OneTimeTokenHandler.getNewToken.yields() - @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> - exists.should.equal false - done() + it "should send the email with the token", (done)-> + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) + @OneTimeTokenHandler.getNewToken.yields(null, @token) + @EmailHandler.sendEmail.callsArgWith(2) + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> + @EmailHandler.sendEmail.called.should.equal true + exists.should.equal true + args = @EmailHandler.sendEmail.args[0] + args[0].should.equal "passwordResetRequested" + args[1].setNewPasswordUrl.should.equal "#{@settings.siteUrl}/user/password/set?passwordResetToken=#{@token}&email=#{encodeURIComponent(@user.email)}" + done() + it "should return exists = false for a holdingAccount", (done) -> + @user.holdingAccount = true + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) + @OneTimeTokenHandler.getNewToken.yields() + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> + exists.should.equal false + done() - it "should send the email with the token", (done)-> + describe "when in overleaf", -> + beforeEach -> + @settings.overleaf = true - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.yields(null, @token) - @EmailHandler.sendEmail.callsArgWith(2) - @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> - @EmailHandler.sendEmail.called.should.equal true - exists.should.equal true - args = @EmailHandler.sendEmail.args[0] - args[0].should.equal "passwordResetRequested" - args[1].setNewPasswordUrl.should.equal "#{@settings.siteUrl}/user/password/set?passwordResetToken=#{@token}&email=#{encodeURIComponent(@user.email)}" - done() + describe "when the email exists", -> + beforeEach -> + @V1Api.request.yields(null, {}, { user_id: 42 }) + @OneTimeTokenHandler.getNewToken.yields(null, @token) + @EmailHandler.sendEmail.yields() + @PasswordResetHandler.generateAndEmailResetToken @email, @callback + + it 'should call the v1 api for the user', -> + @V1Api.request.calledWith({ + url: "/api/v1/sharelatex/user_emails" + qs: + email: @email + expectedStatusCodes: [404] + }).should.equal true + + it 'should set the password token data to the user id and email', -> + @OneTimeTokenHandler.getNewToken + .calledWith('password', { + v1_user_id: 42 + }) + .should.equal true + + it 'should send an email with the token', -> + @EmailHandler.sendEmail.called.should.equal true + args = @EmailHandler.sendEmail.args[0] + args[0].should.equal "passwordResetRequested" + args[1].setNewPasswordUrl.should.equal "#{@settings.siteUrl}/user/password/set?passwordResetToken=#{@token}&email=#{encodeURIComponent(@user.email)}" + + it 'should return exists == true', -> + @callback.calledWith(null, true).should.equal true + + describe "when the email doesn't exist", -> + beforeEach -> + @V1Api.request = sinon.stub().yields(null, { statusCode: 404 }, {}) + @PasswordResetHandler.generateAndEmailResetToken @email, @callback + + it 'should not set the password token data', -> + @OneTimeTokenHandler.getNewToken + .called.should.equal false + + it 'should send an email with the token', -> + @EmailHandler.sendEmail.called.should.equal false + + it 'should return exists == false', -> + @callback.calledWith(null, false).should.equal true - it "should return exists = false for a holdingAccount", (done) -> - @user.holdingAccount = true - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.yields() - @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> - exists.should.equal false - done() describe "setNewUserPassword", -> + describe "when no data is found", -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null) + @PasswordResetHandler.setNewUserPassword @token, @password, @callback - it "should return false if no user id can be found", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.yields() - @PasswordResetHandler.setNewUserPassword @token, @password, (err, found) => - found.should.equal false - @AuthenticationManager.setUserPassword.called.should.equal false - done() + it 'should return exists == false', -> + @callback.calledWith(null, false).should.equal true - it "should set the user password", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, @user_id) - @AuthenticationManager.setUserPassword.yields(null, true) - @PasswordResetHandler.setNewUserPassword @token, @password, (err, found, user_id) => - found.should.equal true - user_id.should.equal @user_id - @AuthenticationManager.setUserPassword.calledWith(@user_id, @password).should.equal true - done() + describe 'when the data is an old style user_id', -> + beforeEach -> + @AuthenticationManager.setUserPassword.yields(null, true, @user_id) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, @user_id) + @PasswordResetHandler.setNewUserPassword @token, @password, @callback + it 'should call setUserPasswordInV2', -> + @AuthenticationManager.setUserPassword + .calledWith(@user_id, @password) + .should.equal true + + it 'should reset == true and the user_id', -> + @callback.calledWith(null, true, @user_id).should.equal true + + describe 'when the data is a new style user_id', -> + beforeEach -> + @AuthenticationManager.setUserPassword.yields(null, true, @user_id) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, {@user_id}) + @PasswordResetHandler.setNewUserPassword @token, @password, @callback + + it 'should call setUserPasswordInV2', -> + @AuthenticationManager.setUserPassword + .calledWith(@user_id, @password) + .should.equal true + + it 'should reset == true and the user_id', -> + @callback.calledWith(null, true, @user_id).should.equal true + + describe 'when the data is v1 id', -> + beforeEach -> + @v1_user_id = 2345 + @AuthenticationManager.setUserPasswordInV1.yields(null, true) + @UserGetter.getUser.withArgs({'overleaf.id': @v1_user_id}).yields(null, { _id: @user_id }) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, {@v1_user_id}) + @PasswordResetHandler.setNewUserPassword @token, @password, @callback + + it 'should call setUserPasswordInV1', -> + @AuthenticationManager.setUserPasswordInV1 + .calledWith(@v1_user_id, @password) + .should.equal true + + it 'should look up the user by v1 id for the v2 user id', -> + @UserGetter.getUser + .calledWith({'overleaf.id': @v1_user_id}) + .should.equal true + + it 'should reset == true and the user_id', -> + @callback.calledWith(null, true, @user_id).should.equal true