Merge pull request #1273 from sharelatex/ja-password-reset-v1

Handle v1-only users in v2 password reset flow

GitOrigin-RevId: 38ce8e9aebd3330b980e73640a23661d8015d4f3

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)

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

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 }

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}")

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?

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()

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