From 5b7974065d50658f57348953b7d03183fa96590f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 2 May 2019 15:03:36 +0100 Subject: [PATCH] Prevent registration of new accounts with existing secondary emails (#1696) Prevent registration of new accounts with existing secondary emails GitOrigin-RevId: 004cf9d31064fc5b7deb621c95c38f103397ff15 --- .../PasswordResetController.coffee | 6 ++- .../PasswordReset/PasswordResetHandler.coffee | 29 ++++++++----- .../PasswordResetControllerTests.coffee | 16 ++++++-- .../PasswordResetHandlerTests.coffee | 41 ++++++++++++++----- 4 files changed, 64 insertions(+), 28 deletions(-) diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee index e438049b17..a6b32dd39e 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee @@ -24,11 +24,13 @@ module.exports = RateLimiter.addCount opts, (err, canContinue)-> if !canContinue return res.send 429, { message: req.i18n.translate("rate_limit_hit_wait")} - PasswordResetHandler.generateAndEmailResetToken email, (err, exists)-> + PasswordResetHandler.generateAndEmailResetToken email, (err, status)-> if err? res.send 500, {message:err?.message} - else if exists + else if status == 'primary' res.send 200, {message: {text: req.i18n.translate("password_reset_email_sent")}} + else if status == 'secondary' + res.send 404, {message: req.i18n.translate("secondary_email_password_reset")} else res.send 404, {message: req.i18n.translate("cant_find_email")} diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee index 399646418c..6899f6b7e9 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee @@ -9,18 +9,25 @@ V1Api = require("../V1/V1Api") module.exports = PasswordResetHandler = - generateAndEmailResetToken:(email, callback = (error, exists) ->)-> + generateAndEmailResetToken:(email, callback = (error, status) ->)-> 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 - setNewPasswordUrl : "#{settings.siteUrl}/user/password/set?passwordResetToken=#{token}&email=#{encodeURIComponent(email)}" - EmailHandler.sendEmail "passwordResetRequested", emailOptions, (error) -> - return callback(error) if error? - callback null, true + if error? + return callback(error, null) + else if exists + OneTimeTokenHandler.getNewToken 'password', data, (err, token)-> + if err then return callback(err) + emailOptions = + to : email + setNewPasswordUrl : "#{settings.siteUrl}/user/password/set?passwordResetToken=#{token}&email=#{encodeURIComponent(email)}" + EmailHandler.sendEmail "passwordResetRequested", emailOptions, (error) -> + return callback(error) if error? + callback null, 'primary' + else + UserGetter.getUserByAnyEmail email, (err, user) -> + if !user + return callback(error, null) + else + return callback(error, 'secondary') setNewUserPassword: (token, password, callback = (error, found, user_id) ->)-> OneTimeTokenHandler.getValueFromTokenAndExpire 'password', token, (err, data)-> diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee index 4909bdb728..fc5d491fcb 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee @@ -54,7 +54,7 @@ describe "PasswordResetController", -> describe "requestReset", -> it "should error if the rate limit is hit", (done)-> - @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, true) + @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, 'primary') @RateLimiter.addCount.callsArgWith(1, null, false) @res.send = (code)=> code.should.equal 429 @@ -65,7 +65,7 @@ describe "PasswordResetController", -> it "should tell the handler to process that email", (done)-> @RateLimiter.addCount.callsArgWith(1, null, true) - @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, true) + @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, 'primary') @res.send = (code)=> code.should.equal 200 @PasswordResetHandler.generateAndEmailResetToken.calledWith(@email.trim()).should.equal true @@ -82,7 +82,15 @@ describe "PasswordResetController", -> it "should send a 404 if the email doesn't exist", (done)-> @RateLimiter.addCount.callsArgWith(1, null, true) - @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, false) + @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, null) + @res.send = (code)=> + code.should.equal 404 + done() + @PasswordResetController.requestReset @req, @res + + it "should send a 404 if the email is registered as a secondard email", (done)-> + @RateLimiter.addCount.callsArgWith(1, null, true) + @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, 'secondary') @res.send = (code)=> code.should.equal 404 done() @@ -92,7 +100,7 @@ describe "PasswordResetController", -> @email = "UPerCaseEMAIL@example.Com" @req.body.email = @email @RateLimiter.addCount.callsArgWith(1, null, true) - @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, true) + @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, 'primary') @res.send = (code)=> code.should.equal 200 @PasswordResetHandler.generateAndEmailResetToken.calledWith(@email.toLowerCase()).should.equal true diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee index 0da9968d2c..1d90271d95 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee @@ -18,6 +18,7 @@ describe "PasswordResetHandler", -> @UserGetter = getUserByMainEmail:sinon.stub() getUser: sinon.stub() + getUserByAnyEmail: sinon.stub() @EmailHandler = sendEmail:sinon.stub() @AuthenticationManager = @@ -48,29 +49,31 @@ describe "PasswordResetHandler", -> describe "when in ShareLaTeX", -> it "should check the user exists", (done)-> @UserGetter.getUserByMainEmail.callsArgWith(1) + @UserGetter.getUserByAnyEmail.callsArgWith(1) @OneTimeTokenHandler.getNewToken.yields() - @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> - exists.should.equal false + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, status)=> + should.equal(status, null) 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)=> + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, status)=> @EmailHandler.sendEmail.called.should.equal true - exists.should.equal true + status.should.equal 'primary' 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) -> + it "should return exists == null for a holdingAccount", (done) -> @user.holdingAccount = true @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByAnyEmail.callsArgWith(1) @OneTimeTokenHandler.getNewToken.yields() - @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> - exists.should.equal false + @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, status)=> + should.equal(status, null) done() describe "when in overleaf", -> @@ -105,12 +108,13 @@ describe "PasswordResetHandler", -> 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 + it 'should return status == true', -> + @callback.calledWith(null, 'primary').should.equal true describe "when the email doesn't exist", -> beforeEach -> @V1Api.request = sinon.stub().yields(null, { statusCode: 404 }, {}) + @UserGetter.getUserByAnyEmail.callsArgWith(1) @PasswordResetHandler.generateAndEmailResetToken @email, @callback it 'should not set the password token data', -> @@ -120,9 +124,24 @@ describe "PasswordResetHandler", -> 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 status == null', -> + @callback.calledWith(null, null).should.equal true + describe "when the email is a secondary email", -> + beforeEach -> + @V1Api.request = sinon.stub().yields(null, { statusCode: 404 }, {}) + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) + @PasswordResetHandler.generateAndEmailResetToken @email, @callback + + it 'should not set the password token data', -> + @OneTimeTokenHandler.getNewToken + .called.should.equal false + + it 'should not send an email with the token', -> + @EmailHandler.sendEmail.called.should.equal false + + it 'should return status == secondary', -> + @callback.calledWith(null, 'secondary').should.equal true describe "setNewUserPassword", -> describe "when no data is found", ->