diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 49bd994b2c..f381735969 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/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? diff --git a/services/web/app/coffee/Features/Errors/ErrorController.coffee b/services/web/app/coffee/Features/Errors/ErrorController.coffee index c4f3089fe0..50f0ba3c06 100644 --- a/services/web/app/coffee/Features/Errors/ErrorController.coffee +++ b/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) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index 94aeaa2a90..3239bbbb58 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/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 diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 1291142dab..52d731c4bc 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/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 diff --git a/services/web/public/coffee/directives/asyncForm.coffee b/services/web/public/coffee/directives/asyncForm.coffee index acafec563d..9baadfd1f2 100644 --- a/services/web/public/coffee/directives/asyncForm.coffee +++ b/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" diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index 795d04488f..9f60c58cb3 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/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; diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee index 0a041a0865..39880b112e 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/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() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index f8bcce30ce..7fcd2147a5 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/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", ->