From 7666c8a481836f9c60c36ab2d099968d459ccb35 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Fri, 14 Dec 2018 09:45:18 -0600 Subject: [PATCH] Merge pull request #1236 from sharelatex/jel-password-reset Reset password via API request to v1 GitOrigin-RevId: 00b0306ca77df650595a762382a8a63b05a945f6 --- .../AuthenticationManager.coffee | 59 +++++++++++--- .../app/coffee/Features/Errors/Errors.coffee | 16 ++++ .../PasswordResetController.coffee | 13 ++- .../PasswordReset/PasswordResetHandler.coffee | 4 +- .../app/coffee/Features/V1/V1Handler.coffee | 23 ++++++ services/web/app/views/user/setPassword.pug | 19 ++++- services/web/app/views/user/settings.pug | 3 +- .../web/public/src/directives/asyncForm.js | 2 + .../acceptance/coffee/TokenAccessTests.coffee | 7 ++ .../acceptance/coffee/helpers/User.coffee | 2 +- .../acceptance/config/settings.test.coffee | 3 - .../AuthenticationManagerTests.coffee | 79 +++++++++++++------ .../PasswordResetHandlerTests.coffee | 2 +- 13 files changed, 187 insertions(+), 45 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index f381735969..1e4dfb7caf 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -4,9 +4,19 @@ User = require("../../models/User").User crypto = require 'crypto' bcrypt = require 'bcrypt' EmailHelper = require("../Helpers/EmailHelper") +Errors = require("../Errors/Errors") +UserGetter = require("../User/UserGetter") +V1Handler = require '../V1/V1Handler' BCRYPT_ROUNDS = Settings?.security?.bcryptRounds or 12 +_checkWriteResult = (result, callback = (error, updated) ->) -> + # for MongoDB + if result and result.nModified == 1 + callback(null, true) + else + callback(null, false) + module.exports = AuthenticationManager = authenticate: (query, password, callback = (error, user) ->) -> # Using Mongoose for legacy reasons here. The returned User instance @@ -46,10 +56,41 @@ module.exports = AuthenticationManager = return { message: 'password is too short' } return null - setUserPassword: (user_id, password, callback = (error) ->) -> + setUserPassword: (user_id, password, callback = (error, changed) ->) -> validation = @validatePassword(password) return callback(validation.message) if validation? + 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 + # 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? + # v2 user in SL + return callback(new Errors.NotInV2Error("Password Reset Attempt")) + else if !overleafId and !Settings.overleaf? + # SL user in SL, change password in SL + AuthenticationManager._setUserPasswordInV2(user_id, password, callback) + else if !overleafId and Settings.overleaf? + # SL user in v2, should not happen + return callback(new Errors.SLInV2Error("Password Reset Attempt")) + else + return callback(new Error("Password Reset Attempt Failed")) + + checkRounds: (user, hashedPassword, password, callback = (error) ->) -> + # check current number of rounds and rehash if necessary + currentRounds = bcrypt.getRounds hashedPassword + if currentRounds < BCRYPT_ROUNDS + AuthenticationManager.setUserPassword user._id, password, callback + else + callback() + + _setUserPasswordInV2: (user_id, password, callback) -> bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) -> return callback(error) if error? bcrypt.hash password, salt, (error, hash) -> @@ -59,12 +100,12 @@ module.exports = AuthenticationManager = }, { $set: hashedPassword: hash $unset: password: true - }, callback) + }, (updateError, result)-> + return callback(updateError) if updateError? + _checkWriteResult(result, callback) + ) - checkRounds: (user, hashedPassword, password, callback = (error) ->) -> - # check current number of rounds and rehash if necessary - currentRounds = bcrypt.getRounds hashedPassword - if currentRounds < BCRYPT_ROUNDS - AuthenticationManager.setUserPassword user._id, password, callback - else - callback() + _setUserPasswordInV1: (user, callback) -> + V1Handler.doPasswordReset user, (error, reset)-> + return callback(error) if error? + return callback(error, reset) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index a73fcf97ef..cda94d316c 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -89,6 +89,20 @@ AccountMergeError = (message) -> return error AccountMergeError.prototype.__proto__ = Error.prototype +NotInV2Error = (message) -> + error = new Error(message) + error.name = "NotInV2Error" + error.__proto__ = NotInV2Error.prototype + return error +NotInV2Error.prototype.__proto__ = Error.prototype + +SLInV2Error = (message) -> + error = new Error(message) + error.name = "SLInV2Error" + error.__proto__ = SLInV2Error.prototype + return error +SLInV2Error.prototype.__proto__ = Error.prototype + module.exports = Errors = NotFoundError: NotFoundError ServiceNotConfiguredError: ServiceNotConfiguredError @@ -103,3 +117,5 @@ module.exports = Errors = EmailExistsError: EmailExistsError InvalidError: InvalidError AccountMergeError: AccountMergeError + NotInV2Error: NotInV2Error + SLInV2Error: SLInV2Error diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee index 618e9e0a7d..816539b83b 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee @@ -46,8 +46,17 @@ module.exports = return res.sendStatus 400 delete req.session.resetToken PasswordResetHandler.setNewUserPassword passwordResetToken?.trim(), password?.trim(), (err, found, user_id) -> - return next(err) if err? - if found + if err and err.name and err.name == "NotFoundError" + res.status(404).send("NotFoundError") + else if err and err.name and err.name == "NotInV2Error" + res.status(403).send("NotInV2Error") + else if err and err.name and err.name == "SLInV2Error" + res.status(403).send("SLInV2Error") + else if err and err.statusCode and err.statusCode == 500 + res.status(500) + else if err and !err.statusCode + res.status(500) + else if found 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 d383e8b669..d0e7867d97 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee @@ -28,6 +28,6 @@ module.exports = if err then return callback(err) if !user_id? return callback null, false, null - AuthenticationManager.setUserPassword user_id, password, (err) -> + AuthenticationManager.setUserPassword user_id, password, (err, reset) -> if err then return callback(err) - callback null, true, user_id \ No newline at end of file + callback null, reset, user_id diff --git a/services/web/app/coffee/Features/V1/V1Handler.coffee b/services/web/app/coffee/Features/V1/V1Handler.coffee index d4f4286f32..1bfda802d9 100644 --- a/services/web/app/coffee/Features/V1/V1Handler.coffee +++ b/services/web/app/coffee/Features/V1/V1Handler.coffee @@ -25,3 +25,26 @@ module.exports = V1Handler = else 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}, + "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 + } + expectedStatusCodes: [200] + }, (err, response, body) -> + if err? + logger.err {email: userData.email, 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" + callback(null, true) + else + err = new Error("Unexpected status from v1 password reset api: #{response.statusCode}") + callback(err, false) diff --git a/services/web/app/views/user/setPassword.pug b/services/web/app/views/user/setPassword.pug index 3a6a588f34..d983f20049 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -19,8 +19,21 @@ block content .alert.alert-success(ng-show="passwordResetForm.response.success") | #{translate("password_has_been_reset")}. a(href='/login') #{translate("login_here")} - .alert.alert-danger(ng-show="passwordResetForm.response.error") - | #{translate("password_reset_token_expired")} + div(ng-show="passwordResetForm.response.error == true") + .alert.alert-danger(ng-show="passwordResetForm.response.data == 'NotFoundError'") + | #{translate('password_reset_token_expired')} + br + a(href="/user/password/reset" style="text-decoration: underline;") + | Request a new password reset email + .alert.alert-danger(ng-show="passwordResetForm.response.data == 'SLInV2Error'") + a(href=settings.accountMerge.sharelatexHost + "/user/password/reset") + | Please go to ShareLaTeX to reset your password + .alert.alert-danger(ng-show="passwordResetForm.response.data == 'NotInV2Error'") + a(href=settings.accountMerge.betaHost + "/user/password/reset" style="text-decoration: underline;") + | Please go to Overleaf to reset your password + .alert.alert-danger(ng-show="passwordResetForm.response.status == 500") + | #{translate('error_performing_request')} + .form-group input.form-control#passwordField( @@ -47,4 +60,4 @@ block content script(type='text/javascript'). window.usersEmail = "#{getReqQueryParam('email')}" - window.passwordStrengthOptions = !{JSON.stringify(settings.passwordStrengthOptions || {})} \ No newline at end of file + window.passwordStrengthOptions = !{JSON.stringify(settings.passwordStrengthOptions || {})} diff --git a/services/web/app/views/user/settings.pug b/services/web/app/views/user/settings.pug index 6da0d50fb9..23b41e8ec8 100644 --- a/services/web/app/views/user/settings.pug +++ b/services/web/app/views/user/settings.pug @@ -256,7 +256,7 @@ block content div.alert.alert-info | If you can't remember your password, or if you are using Single-Sign-On with another provider | to sign in (such as Twitter or Google), please - | #[a(href='/sign_in_to_v1?return_to=/users/password/new', target='_blank') reset your password], + | #[a(href="/user/password/reset", target='_blank') reset your password], | and try again. .modal-footer button.btn.btn-default( @@ -271,4 +271,3 @@ block content script(type='text/javascript'). window.passwordStrengthOptions = !{JSON.stringify(settings.passwordStrengthOptions || {})} - diff --git a/services/web/public/src/directives/asyncForm.js b/services/web/public/src/directives/asyncForm.js index 2f02bf8ca0..395723b980 100644 --- a/services/web/public/src/directives/asyncForm.js +++ b/services/web/public/src/directives/asyncForm.js @@ -93,6 +93,8 @@ define(['base', 'libs/passfield'], function(App) { scope[attrs.name].inflight = false response.success = false response.error = true + response.status = status + response.data = data const onErrorHandler = scope[attrs.onError] if (onErrorHandler) { diff --git a/services/web/test/acceptance/coffee/TokenAccessTests.coffee b/services/web/test/acceptance/coffee/TokenAccessTests.coffee index 832281f6a3..ce1a00fd7c 100644 --- a/services/web/test/acceptance/coffee/TokenAccessTests.coffee +++ b/services/web/test/acceptance/coffee/TokenAccessTests.coffee @@ -417,6 +417,13 @@ describe 'TokenAccess', -> , done) describe 'unimported v1 project', -> + before -> + settings.overleaf = + host: 'http://localhost:5000' + + after -> + delete settings.overleaf + it 'should redirect read and write token to v1', (done) -> unimportedV1Token = '123abc' try_read_and_write_token_access(@owner, unimportedV1Token, (response, body) => diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 053f3fa119..551d0c8893 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.setUserPassword 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/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 00a9168623..5252f03834 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -126,9 +126,6 @@ module.exports = path: '/destination/get_and_post' } - overleaf: - host: "http://overleaf.test:5000" - redirects: '/redirect/one': '/destination/one', '/redirect/get_and_post': { diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee index 39880b112e..be632a64fb 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee @@ -6,6 +6,7 @@ modulePath = "../../../../app/js/Features/Authentication/AuthenticationManager.j SandboxedModule = require('sandboxed-module') events = require "events" ObjectId = require("mongojs").ObjectId +Errors = require "../../../../app/js/Features/Errors/Errors" describe "AuthenticationManager", -> beforeEach -> @@ -18,6 +19,8 @@ describe "AuthenticationManager", -> ObjectId: ObjectId "bcrypt": @bcrypt = {} "settings-sharelatex": @settings + "../V1/V1Handler": @V1Handler = {} + "../User/UserGetter": @UserGetter = {} @callback = sinon.stub() describe "authenticate", -> @@ -28,7 +31,7 @@ describe "AuthenticationManager", -> email: @email = "USER@sharelatex.com" @unencryptedPassword = "banana" @User.findOne = sinon.stub().callsArgWith(1, null, @user) - + describe "when the hashed password matches", -> beforeEach (done) -> @user.hashedPassword = @hashedPassword = "asdfjadflasdf" @@ -151,7 +154,7 @@ describe "AuthenticationManager", -> describe "too long", -> beforeEach -> @settings.passwordStrengthOptions = - length: + length: max:10 @password = "dsdsadsadsadsadsadkjsadjsadjsadljs" @@ -185,30 +188,62 @@ describe "AuthenticationManager", -> @bcrypt.hash.called.should.equal false done() - describe "successful set", -> - beforeEach -> - @AuthenticationManager.setUserPassword(@user_id, @password, @callback) + describe "password set attempt", -> + describe "with SL user in SL", -> + beforeEach -> + @UserGetter.getUser = sinon.stub().yields(null, { overleaf: null }) + @AuthenticationManager.setUserPassword(@user_id, @password, @callback) - it "should update the user's password in the database", -> - args = @db.users.update.lastCall.args - expect(args[0]).to.deep.equal {_id: ObjectId(@user_id.toString())} - expect(args[1]).to.deep.equal { - $set: { - "hashedPassword": @hashedPassword + it 'should look up the user', -> + @UserGetter.getUser.calledWith(@user_id).should.equal true + + it "should update the user's password in the database", -> + args = @db.users.update.lastCall.args + expect(args[0]).to.deep.equal {_id: ObjectId(@user_id.toString())} + expect(args[1]).to.deep.equal { + $set: { + "hashedPassword": @hashedPassword + } + $unset: password: true } - $unset: password: true - } - it "should hash the password", -> - @bcrypt.genSalt - .calledWith(12) - .should.equal true - @bcrypt.hash - .calledWith(@password, @salt) - .should.equal true + it "should hash the password", -> + @bcrypt.genSalt + .calledWith(12) + .should.equal true + @bcrypt.hash + .calledWith(@password, @salt) + .should.equal true - it "should call the callback", -> - @callback.called.should.equal true + it "should call the callback", -> + @callback.called.should.equal true + describe "with SL user in v2", -> + beforeEach (done) -> + @settings.overleaf = true + @UserGetter.getUser = sinon.stub().yields(null, { overleaf: null }) + @AuthenticationManager.setUserPassword @user_id, @password, (err, changed) => + @callback(err, changed) + done() + it "should error", -> + @callback.calledWith(new Errors.SLInV2Error("Password Reset Attempt")).should.equal true + describe "with v2 user in SL", -> + beforeEach (done) -> + @UserGetter.getUser = sinon.stub().yields(null, { overleaf: {id: 1} }) + @AuthenticationManager.setUserPassword @user_id, @password, (err, changed) => + @callback(err, changed) + done() + it "should error", -> + @callback.calledWith(new Errors.NotInV2Error("Password Reset Attempt")).should.equal true + describe "with v2 user in v2", -> + beforeEach (done) -> + @settings.overleaf = true + @UserGetter.getUser = sinon.stub().yields(null, { overleaf: {id: 1} }) + @V1Handler.doPasswordReset = sinon.stub().yields(null, true) + @AuthenticationManager.setUserPassword @user_id, @password, (err, changed) => + @callback(err, changed) + done() + it "should set the password in v2", -> + @callback.calledWith(null, true).should.equal true diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee index 7ee82fbd7e..14bde68c44 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee @@ -79,7 +79,7 @@ describe "PasswordResetHandler", -> it "should set the user password", (done)-> @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, @user_id) - @AuthenticationManager.setUserPassword.callsArgWith(2) + @AuthenticationManager.setUserPassword.yields(null, true) @PasswordResetHandler.setNewUserPassword @token, @password, (err, found, user_id) => found.should.equal true user_id.should.equal @user_id