diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index eacbc33991..570687c873 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -57,54 +57,70 @@ const PasswordResetHandler = { }, setNewUserPassword(token, password, callback) { - OneTimeTokenHandler.getValueFromTokenAndExpire('password', token, function( - err, - data - ) { - if (err) { - return callback(err) - } - if (data == null) { - return callback(null, false, null) - } - 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 != null) { - AuthenticationManager.setUserPassword(data.user_id, password, function( - err, - reset - ) { - if (err) { - return callback(err) - } - callback(null, reset, data.user_id) - }) - } else if (data.v1_user_id != null) { - AuthenticationManager.setUserPasswordInV1( - data.v1_user_id, + PasswordResetHandler.getUserForPasswordResetToken( + token, + (err, user, version) => { + if (err != null) { + return callback(err) + } + if (user == null) { + return callback(null, false, null) + } + AuthenticationManager.setUserPassword( + user._id, password, - function(error, reset) { - if (error != null) { - return callback(error) + (err, reset) => { + if (err) { + return callback(err) } - UserGetter.getUser( - { 'overleaf.id': data.v1_user_id }, - { _id: 1 }, - function(error, user) { - if (error != null) { - return callback(error) - } - callback(null, reset, user != null ? user._id : undefined) - } - ) + callback(null, reset, user._id) } ) } - }) + ) + }, + + getUserForPasswordResetToken(token, callback) { + OneTimeTokenHandler.getValueFromTokenAndExpire( + 'password', + token, + (err, data) => { + if (err != null) { + if (err.name === 'NotFoundError') { + return callback(null, null) + } else { + return callback(err) + } + } + if (data == null || data.email == null) { + return callback(null, null) + } + UserGetter.getUserByMainEmail( + data.email, + { _id: 1, 'overleaf.id': 1 }, + (err, user) => { + if (err != null) { + callback(err) + } else if (user == null) { + callback(null, null) + } else if ( + data.user_id != null && + data.user_id === user._id.toString() + ) { + callback(null, user, 'v2') + } else if ( + data.v1_user_id != null && + user.overleaf != null && + data.v1_user_id === user.overleaf.id + ) { + callback(null, user, 'v1') + } else { + callback(null, null) + } + } + ) + } + ) }, _getPasswordResetData(email, callback) { diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index a1d2a206ff..4542f86907 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -1,4 +1,4 @@ -const should = require('chai').should() +const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const path = require('path') const sinon = require('sinon') @@ -21,9 +21,7 @@ describe('PasswordResetHandler', function() { } this.EmailHandler = { sendEmail: sinon.stub() } this.AuthenticationManager = { - setUserPassword: sinon.stub(), - setUserPasswordInV1: sinon.stub(), - setUserPasswordInV2: sinon.stub() + setUserPassword: sinon.stub() } this.V1Api = { request: sinon.stub() } this.PasswordResetHandler = SandboxedModule.require(modulePath, { @@ -44,9 +42,11 @@ describe('PasswordResetHandler', function() { } }) this.token = '12312321i' - this.user_id = 'user_id_here' this.email = 'bob@bob.com' - this.user = { _id: this.user_id, email: this.email } + this.user = { + _id: 'user_id_here', + email: this.email + } this.password = 'my great secret password' this.callback = sinon.stub() }) @@ -63,7 +63,7 @@ describe('PasswordResetHandler', function() { if (err) { return done(err) } - should.equal(status, null) + expect(status).to.be.null done() } ) @@ -83,7 +83,7 @@ describe('PasswordResetHandler', function() { this.OneTimeTokenHandler.getNewToken.should.have.been.calledWith( 'password', { - user_id: this.user_id, + user_id: this.user._id, email: this.email } ) @@ -111,7 +111,7 @@ describe('PasswordResetHandler', function() { if (err) { return done(err) } - should.equal(status, null) + expect(status).to.be.null done() } ) @@ -257,75 +257,188 @@ describe('PasswordResetHandler', function() { }) describe('setNewUserPassword', function() { - describe('when no data is found', function() { + describe('when no token is found', function() { beforeEach(function() { this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null) + }) + + it('should return found == false when', function(done) { this.PasswordResetHandler.setNewUserPassword( this.token, this.password, - this.callback + (err, found) => { + if (err != null) { + return done(err) + } + expect(found).to.be.false + done() + } ) }) - - it('should return exists == false', function() { - this.callback.calledWith(null, false).should.equal(true) - }) }) - describe('when the data is an old style user_id', function() { + describe('when the token has a user_id and email', function() { beforeEach(function() { - this.AuthenticationManager.setUserPassword.yields( - null, - true, - this.user_id - ) - this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields( - null, - this.user_id - ) - this.PasswordResetHandler.setNewUserPassword( - this.token, - this.password, - this.callback - ) - }) - - it('should call setUserPasswordInV2', function() { + this.OneTimeTokenHandler.getValueFromTokenAndExpire + .withArgs('password', this.token) + .yields(null, { + user_id: this.user._id, + email: this.email + }) this.AuthenticationManager.setUserPassword - .calledWith(this.user_id, this.password) - .should.equal(true) + .withArgs(this.user._id, this.password) + .yields(null, true, this.user._id) }) - it('should reset == true and the user_id', function() { - this.callback.calledWith(null, true, this.user_id).should.equal(true) - }) - }) - - describe('when the data is a new style user_id', function() { - beforeEach(function() { - this.AuthenticationManager.setUserPassword.yields( - null, - true, - this.user_id - ) - this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, { - user_id: this.user_id + describe('when no user is found with this email', function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail + .withArgs(this.email) + .yields(null, null) + }) + + it('should return found == false', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found) => { + if (err != null) { + return done(err) + } + expect(found).to.be.false + done() + } + ) }) - this.PasswordResetHandler.setNewUserPassword( - this.token, - this.password, - this.callback - ) }) - it('should call setUserPasswordInV2', function() { + describe("when the email and user don't match", function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail + .withArgs(this.email) + .yields(null, { _id: 'not-the-same', email: this.email }) + }) + + it('should return found == false', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found) => { + if (err != null) { + return done(err) + } + expect(found).to.be.false + done() + } + ) + }) + }) + + describe('when the email and user match', function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail + .withArgs(this.email) + .yields(null, this.user) + }) + + it('should return found == true and the user id', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found, userId) => { + if (err != null) { + return done(err) + } + expect(found).to.be.true + expect(userId).to.equal(this.user._id) + done() + } + ) + }) + }) + }) + + describe('when the token has a v1_user_id and email', function() { + beforeEach(function() { + this.user.overleaf = { id: 184 } + this.OneTimeTokenHandler.getValueFromTokenAndExpire + .withArgs('password', this.token) + .yields(null, { + v1_user_id: this.user.overleaf.id, + email: this.email + }) this.AuthenticationManager.setUserPassword - .calledWith(this.user_id, this.password) - .should.equal(true) + .withArgs(this.user._id, this.password) + .yields(null, true) }) - it('should reset == true and the user_id', function() { - this.callback.calledWith(null, true, this.user_id).should.equal(true) + describe('when no user is found with this email', function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail + .withArgs(this.email) + .yields(null, null) + }) + + it('should return found == false', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found) => { + if (err != null) { + return done(err) + } + expect(found).to.be.false + done() + } + ) + }) + }) + + describe("when the email and user don't match", function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail.withArgs(this.email).yields(null, { + _id: this.user._id, + email: this.email, + overleaf: { id: 'not-the-same' } + }) + }) + + it('should return found == false', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found) => { + if (err != null) { + return done(err) + } + expect(found).to.be.false + done() + } + ) + }) + }) + + describe('when the email and user match', function() { + beforeEach(function() { + this.UserGetter.getUserByMainEmail + .withArgs(this.email) + .yields(null, this.user) + }) + + it('should return found == true and the user id', function(done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + (err, found, userId) => { + if (err != null) { + return done(err) + } + expect(found).to.be.true + expect(userId).to.equal(this.user._id) + done() + } + ) + }) }) }) })