Merge pull request #999 from sharelatex/as-validate-password-length

Validate password length

services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee

@@ -3,6 +3,7 @@ User = require("../../models/User").User
 {db, ObjectId} = require("../../infrastructure/mongojs")
 crypto = require 'crypto'
 bcrypt = require 'bcrypt'
+EmailHelper = require("../Helpers/EmailHelper")
 
 BCRYPT_ROUNDS = Settings?.security?.bcryptRounds or 12
 
@@ -28,13 +29,26 @@ module.exports = AuthenticationManager =
 			else
 				callback null, null
 
-	setUserPassword: (user_id, password, callback = (error) ->) ->
+	validateEmail: (email) ->
+		parsed = EmailHelper.parseEmail(email)
+		if !parsed?
+			return { message: 'email not valid' }
+		return null
+
+	validatePassword: (password) ->
+		if !password?
+			return { message: 'password not set' }
 		if (Settings.passwordStrengthOptions?.length?.max? and
-				Settings.passwordStrengthOptions?.length?.max < password.length)
-			return callback("password is too long")
+				password.length > Settings.passwordStrengthOptions?.length?.max)
+			return { message: "password is too long" }
 		if (Settings.passwordStrengthOptions?.length?.min? and
-				Settings.passwordStrengthOptions?.length?.min > password.length)
-			return callback("password is too short")
+				password.length < Settings.passwordStrengthOptions?.length?.min)
+			return { message: 'password is too short' }
+		return null
+
+	setUserPassword: (user_id, password, callback = (error) ->) ->
+		validation = @validatePassword(password)
+		return callback(validation.message) if validation?
 
 		bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) ->
 			return callback(error) if error?

services/web/app/coffee/Features/Errors/ErrorController.coffee

@@ -25,6 +25,10 @@ module.exports = ErrorController =
 		else if error instanceof Errors.TooManyRequestsError
 			logger.warn {err: error, url: req.url}, "too many requests error"
 			res.sendStatus(429)
+		else if error instanceof Errors.InvalidError
+			logger.warn {err: error, url: req.url}, "invalid error"
+			res.status(400)
+			res.send(error.message)
 		else if error instanceof Errors.InvalidNameError
 			logger.warn {err: error, url: req.url}, "invalid name error"
 			res.status(400)

services/web/app/coffee/Features/Errors/Errors.coffee

@@ -82,6 +82,13 @@ EmailExistsError = (message) ->
 	return error
 EmailExistsError.prototype.__proto__ = Error.prototype
 
+InvalidError = (message) ->
+	error = new Error(message)
+	error.name = "InvalidError"
+	error.__proto__ = InvalidError.prototype
+	return error
+InvalidError.prototype.__proto__ = Error.prototype
+
 module.exports = Errors =
 	NotFoundError: NotFoundError
 	ServiceNotConfiguredError: ServiceNotConfiguredError
@@ -95,3 +102,4 @@ module.exports = Errors =
 	V1ConnectionError: V1ConnectionError
 	UnconfirmedEmailError: UnconfirmedEmailError
 	EmailExistsError: EmailExistsError
+	InvalidError: InvalidError

services/web/app/coffee/Features/User/UserRegistrationHandler.coffee

@@ -13,17 +13,10 @@ settings = require "settings-sharelatex"
 EmailHelper = require("../Helpers/EmailHelper")
 
 module.exports = UserRegistrationHandler =
-	hasZeroLengths : (props) ->
-		hasZeroLength = false
-		props.forEach (prop) ->
-			if prop.length == 0
-				hasZeroLength = true
-		return hasZeroLength
-
 	_registrationRequestIsValid : (body, callback)->
-		email = EmailHelper.parseEmail(body.email) or ''
-		password = body.password
-		if @hasZeroLengths([password, email])
+		invalidEmail = AuthenticationManager.validateEmail(body.email or '')
+		invalidPassword = AuthenticationManager.validatePassword(body.password or '')
+		if invalidEmail? or invalidPassword?
 			return false
 		else
 			return true

services/web/public/coffee/directives/asyncForm.coffee

@@ -70,7 +70,11 @@ define [
 								onErrorHandler(httpResponse)
 								return
 
-							if status == 403 # Forbidden
+							if status == 400 # Bad Request
+								response.message =
+									text: "Invalid Request. Please correct the data and try again."
+									type: 'error'
+							else if status == 403 # Forbidden
 								response.message =
 									text: "Session error. Please check you have cookies enabled. If the problem persists, try clearing your cache and cookies."
 									type: "error"

services/web/public/stylesheets/app/homepage.less

@@ -94,6 +94,14 @@
 			border-radius: 9999px;
 		}
 	}
+	.hp-register-password-error {
+		margin-bottom: 9px;
+	}
+	.register-banner__password-error {
+		padding: 5px 9px;
+		border: none;
+		border-radius: @btn-border-radius-base;
+	}
 	.screenshot {
 		height: 600px;
 		margin: auto;

services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee

@@ -94,6 +94,50 @@ describe "AuthenticationManager", ->
 			it "should not return a user", ->
 				@callback.calledWith(null, null).should.equal true
 
+	describe "validateEmail", ->
+		describe "valid", ->
+			it "should return null", ->
+				result = @AuthenticationManager.validateEmail 'foo@example.com'
+				expect(result).to.equal null
+
+		describe "invalid", ->
+			it "should return validation error object for no email", ->
+				result = @AuthenticationManager.validateEmail ''
+				expect(result).to.not.equal null
+				expect(result.message).to.equal 'email not valid'
+
+			it "should return validation error object for invalid", ->
+				result = @AuthenticationManager.validateEmail 'notanemail'
+				expect(result).to.not.equal null
+				expect(result.message).to.equal 'email not valid'
+
+	describe "validatePassword", ->
+		it "should return null if valid", ->
+			result = @AuthenticationManager.validatePassword 'banana'
+			expect(result).to.equal null
+
+		describe "invalid", ->
+			beforeEach ->
+				@settings.passwordStrengthOptions =
+					length:
+						max:10
+						min:6
+
+			it "should return validation error object if not set", ->
+				result = @AuthenticationManager.validatePassword()
+				expect(result).to.not.equal null
+				expect(result.message).to.equal 'password not set'
+
+			it "should return validation error object if too short", ->
+				result = @AuthenticationManager.validatePassword 'dsd'
+				expect(result).to.not.equal null
+				expect(result.message).to.equal 'password is too short'
+
+			it "should return validation error object if too long", ->
+				result = @AuthenticationManager.validatePassword 'dsdsadsadsadsadsadkjsadjsadjsadljs'
+				expect(result).to.not.equal null
+				expect(result.message).to.equal 'password is too long'
+
 	describe "setUserPassword", ->
 		beforeEach ->
 			@user_id = ObjectId()

services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee

@@ -19,6 +19,8 @@ describe "UserRegistrationHandler", ->
 		@UserCreator = 
 			createNewUser:sinon.stub().callsArgWith(1, null, @user)
 		@AuthenticationManager =
+			validateEmail: sinon.stub().returns(null)
+			validatePassword: sinon.stub().returns(null)
 			setUserPassword: sinon.stub().callsArgWith(2)
 		@NewsLetterManager =
 			subscribe: sinon.stub().callsArgWith(1)
@@ -44,28 +46,25 @@ describe "UserRegistrationHandler", ->
 
 
 	describe 'validate Register Request', ->
-
-
-		it 'allow working account through', ->
+		it 'allows passing validation through', ->
 			result = @handler._registrationRequestIsValid @passingRequest
 			result.should.equal true
-		
-		it 'not allow not valid email through ', ()->
-			@passingRequest.email = "notemail"
-			result = @handler._registrationRequestIsValid @passingRequest
-			result.should.equal false
 
-		it 'not allow no email through ', ->
-			@passingRequest.email = ""
-			result = @handler._registrationRequestIsValid @passingRequest
-			result.should.equal false
-		
-		it 'not allow no password through ', ()->
-			@passingRequest.password= ""
-			result = @handler._registrationRequestIsValid @passingRequest
-			result.should.equal false
+		describe 'failing email validation', ->
+			beforeEach ->
+				@AuthenticationManager.validateEmail.returns({ message: 'email not set' })
 
+			it 'does not allow through', ->
+				result = @handler._registrationRequestIsValid @passingRequest
+				result.should.equal false
+
+		describe 'failing password validation', ->
+			beforeEach ->
+				@AuthenticationManager.validatePassword.returns({ message: 'password is too short' })
 
+			it 'does not allow through', ->
+				result = @handler._registrationRequestIsValid @passingRequest
+				result.should.equal false
 
 	describe "registerNewUser", ->