From e99165b475d506c8871d5f7fce2be3ec5b5d6fc0 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 10:03:21 +0100 Subject: [PATCH 01/13] Validate password length when registering --- .../app/coffee/Features/User/UserRegistrationHandler.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 1291142dab..0928f64640 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -20,10 +20,13 @@ module.exports = UserRegistrationHandler = hasZeroLength = true return hasZeroLength + isTooShort: (prop, length) -> + return prop.length < length + _registrationRequestIsValid : (body, callback)-> email = EmailHelper.parseEmail(body.email) or '' password = body.password - if @hasZeroLengths([password, email]) + if @hasZeroLengths([password, email]) or @isTooShort(password, 6) return false else return true From bf60fe7f6cba1f4e9df2ddbc0117aa640f3970be Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 10:36:10 +0100 Subject: [PATCH 02/13] Add error handling for InvalidError --- .../web/app/coffee/Features/Errors/ErrorController.coffee | 4 ++++ services/web/app/coffee/Features/Errors/Errors.coffee | 8 ++++++++ 2 files changed, 12 insertions(+) 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 From 1fe8aebf5bdcc1e5d20c709f723d7d023d1f1ca7 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 10:36:22 +0100 Subject: [PATCH 03/13] Add error handling for 400 responses --- services/web/public/coffee/directives/asyncForm.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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" From 44c86b3769dcc6db8c2d6bee5cc767b55400b675 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 12:00:33 +0100 Subject: [PATCH 04/13] Refactor to use password strength options --- .../AuthenticationManager.coffee | 18 +++++++++++++++--- .../User/UserRegistrationHandler.coffee | 15 +++------------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 49bd994b2c..26c31de84c 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -28,13 +28,25 @@ module.exports = AuthenticationManager = else callback null, null - setUserPassword: (user_id, password, callback = (error) ->) -> + validateEmail: (email) -> + if !email?.length + return { message: 'email not set' } + 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") + return { message: 'password is too short' } if (Settings.passwordStrengthOptions?.length?.min? and Settings.passwordStrengthOptions?.length?.min > password.length) - return callback("password is too short") + 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/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 0928f64640..1fa1dc0d79 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -13,20 +13,11 @@ 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 - - isTooShort: (prop, length) -> - return prop.length < length - _registrationRequestIsValid : (body, callback)-> email = EmailHelper.parseEmail(body.email) or '' - password = body.password - if @hasZeroLengths([password, email]) or @isTooShort(password, 6) + invalidEmail = AuthenticationManager.validateEmail(email) + invalidPassword = AuthenticationManager.validatePassword(body.password) + if invalidEmail? or invalidPassword? return false else return true From 676557a05179d73a07ff26a0d13eb4eb8bba94eb Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 13:47:53 +0100 Subject: [PATCH 05/13] Refactor to validate in AuthenticationManager --- .../Features/Authentication/AuthenticationManager.coffee | 8 +++++--- .../coffee/Features/User/UserRegistrationHandler.coffee | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 26c31de84c..bedaf60d79 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 @@ -29,8 +30,9 @@ module.exports = AuthenticationManager = callback null, null validateEmail: (email) -> - if !email?.length - return { message: 'email not set' } + parsed = EmailHelper.parseEmail(email) + if !parsed? + return { message: 'email not valid' } return null validatePassword: (password) -> @@ -45,7 +47,7 @@ module.exports = AuthenticationManager = return null setUserPassword: (user_id, password, callback = (error) ->) -> - validation = validatePassword(password) + validation = @validatePassword(password) return callback(validation.message) if validation? bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) -> diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 1fa1dc0d79..52d731c4bc 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -14,9 +14,8 @@ EmailHelper = require("../Helpers/EmailHelper") module.exports = UserRegistrationHandler = _registrationRequestIsValid : (body, callback)-> - email = EmailHelper.parseEmail(body.email) or '' - invalidEmail = AuthenticationManager.validateEmail(email) - invalidPassword = AuthenticationManager.validatePassword(body.password) + invalidEmail = AuthenticationManager.validateEmail(body.email or '') + invalidPassword = AuthenticationManager.validatePassword(body.password or '') if invalidEmail? or invalidPassword? return false else From 1ef947b1febd7cb38bf4ebd06c7e1d0d0c9779a1 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 13:48:57 +0100 Subject: [PATCH 06/13] Fix tests after refactoring register validation --- .../AuthenticationManagerTests.coffee | 42 +++++++++++++++++++ .../User/UserRegistrationHandlerTests.coffee | 33 +++++++-------- 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee index 0a041a0865..be2ed44979 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee @@ -94,6 +94,48 @@ 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' + 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", -> From 2edca417b180db65c003830fcaae2cbb119df040 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 15:22:25 +0100 Subject: [PATCH 07/13] Reduce padding --- services/web/public/stylesheets/app/homepage.less | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index 795d04488f..3295d769c4 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/services/web/public/stylesheets/app/homepage.less @@ -94,6 +94,9 @@ border-radius: 9999px; } } + .register-banner__password-error { + padding: 2px; + } .screenshot { height: 600px; margin: auto; From af499e45395dc34bf4dfb040e1283ea97207bbe8 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 15:49:42 +0100 Subject: [PATCH 08/13] Style error message on register form --- services/web/public/stylesheets/app/homepage.less | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index 3295d769c4..8087a96dd0 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/services/web/public/stylesheets/app/homepage.less @@ -94,8 +94,12 @@ border-radius: 9999px; } } + .hp-register-password-error { + margin-top: 5px; + } .register-banner__password-error { padding: 2px; + position: relative; } .screenshot { height: 600px; From 8777b0f5f88a5b69fb9a6fcd1c88265e378d4ca5 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 16:40:50 +0100 Subject: [PATCH 09/13] Style error message after moving it above inputs --- services/web/public/stylesheets/app/homepage.less | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index 8087a96dd0..afe1ebf357 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/services/web/public/stylesheets/app/homepage.less @@ -95,10 +95,11 @@ } } .hp-register-password-error { - margin-top: 5px; + margin-bottom: 9px; } .register-banner__password-error { - padding: 2px; + padding: 3px; + border: none; position: relative; } .screenshot { From e37a54e25449ead87554aa331be3535db727f9fd Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 4 Oct 2018 16:51:42 +0100 Subject: [PATCH 10/13] Make error message match other styles --- services/web/public/stylesheets/app/homepage.less | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index afe1ebf357..ff11ef3776 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/services/web/public/stylesheets/app/homepage.less @@ -98,9 +98,9 @@ margin-bottom: 9px; } .register-banner__password-error { - padding: 3px; - border: none; - position: relative; + padding: 5px 9px; + border: none; + border-radius: @btn-border-radius-base; } .screenshot { height: 600px; From 04572f61bb444037414689ec17eb2c32be93b384 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 5 Oct 2018 10:18:53 +0100 Subject: [PATCH 11/13] Fix copy/paste error --- .../coffee/Features/Authentication/AuthenticationManager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index bedaf60d79..64da009387 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -43,7 +43,7 @@ module.exports = AuthenticationManager = return { message: 'password is too short' } if (Settings.passwordStrengthOptions?.length?.min? and Settings.passwordStrengthOptions?.length?.min > password.length) - return { message: "password is too short" } + return { message: "password is too long" } return null setUserPassword: (user_id, password, callback = (error) ->) -> From f26f30e677890d79394d7f40c7f0cdc8fd5d2f0b Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 5 Oct 2018 10:19:04 +0100 Subject: [PATCH 12/13] Fix spaces instead of tabs --- services/web/public/stylesheets/app/homepage.less | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/web/public/stylesheets/app/homepage.less b/services/web/public/stylesheets/app/homepage.less index ff11ef3776..9f60c58cb3 100644 --- a/services/web/public/stylesheets/app/homepage.less +++ b/services/web/public/stylesheets/app/homepage.less @@ -98,9 +98,9 @@ margin-bottom: 9px; } .register-banner__password-error { - padding: 5px 9px; - border: none; - border-radius: @btn-border-radius-base; + padding: 5px 9px; + border: none; + border-radius: @btn-border-radius-base; } .screenshot { height: 600px; From e129172553231a8bd9b310ccb09301aff09b3724 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 8 Oct 2018 11:25:24 +0100 Subject: [PATCH 13/13] Fix ordering of boolean check to be more readable --- .../Features/Authentication/AuthenticationManager.coffee | 8 ++++---- .../Authentication/AuthenticationManagerTests.coffee | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 64da009387..f381735969 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -39,11 +39,11 @@ module.exports = AuthenticationManager = if !password? return { message: 'password not set' } if (Settings.passwordStrengthOptions?.length?.max? and - Settings.passwordStrengthOptions?.length?.max < password.length) - return { message: 'password is too short' } - if (Settings.passwordStrengthOptions?.length?.min? and - Settings.passwordStrengthOptions?.length?.min > password.length) + password.length > Settings.passwordStrengthOptions?.length?.max) return { message: "password is too long" } + if (Settings.passwordStrengthOptions?.length?.min? and + password.length < Settings.passwordStrengthOptions?.length?.min) + return { message: 'password is too short' } return null setUserPassword: (user_id, password, callback = (error) ->) -> diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee index be2ed44979..39880b112e 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee @@ -135,6 +135,8 @@ describe "AuthenticationManager", -> 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 ->