diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index a0776a1813..3c1818979f 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -225,17 +225,30 @@ async function tryDeleteUser(req, res, next) { const { password } = req.body req.logger.addFields({ userId }) + logger.debug({ userId }, 'trying to delete user account') if (password == null || password === '') { logger.err({ userId }, 'no password supplied for attempt to delete account') return res.sendStatus(403) } - const { user } = await AuthenticationManager.promises.authenticate( - { _id: userId }, - password, - null, - { enforceHIBPCheck: false } - ) + let user + try { + user = ( + await AuthenticationManager.promises.authenticate( + { _id: userId }, + password, + null, + { enforceHIBPCheck: false } + ) + ).user + } catch (err) { + throw OError.tag( + err, + 'error authenticating during attempt to delete account', + { userId } + ) + } + if (!user) { logger.err({ userId }, 'auth failed during attempt to delete account') return res.sendStatus(403) @@ -265,10 +278,12 @@ async function tryDeleteUser(req, res, next) { errorData.info.public ) } else { - throw err + throw OError.tag(err, errorData.message, errorData.info) } } + await Modules.promises.hooks.fire('tryDeleteV1Account', user) + const sessionId = req.sessionID if (typeof req.logout === 'function') { diff --git a/services/web/modules/launchpad/test/unit/src/LaunchpadControllerTests.js b/services/web/modules/launchpad/test/unit/src/LaunchpadControllerTests.js index 1a40bfa8a6..f56b0e0e0d 100644 --- a/services/web/modules/launchpad/test/unit/src/LaunchpadControllerTests.js +++ b/services/web/modules/launchpad/test/unit/src/LaunchpadControllerTests.js @@ -696,7 +696,6 @@ describe('LaunchpadController', function () { describe('when overleaf', function () { beforeEach(function () { this.Settings.overleaf = { one: 1 } - this.Settings.createV1AccountOnLogin = true this._atLeastOneAdminExists.resolves(false) this.email = 'someone@example.com' this.password = 'a_really_bad_password' diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 2089ec63da..874220f3cd 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -133,6 +133,14 @@ describe('UserController', function () { promises: { expireAllTokensForUser: sinon.stub().resolves() }, } + this.Modules = { + promises: { + hooks: { + fire: sinon.stub().resolves(), + }, + }, + } + this.UserController = SandboxedModule.require(modulePath, { requires: { '../Helpers/UrlHelper': this.UrlHelper, @@ -156,6 +164,7 @@ describe('UserController', function () { '../Security/OneTimeTokenHandler': this.OneTimeTokenHandler, '../../infrastructure/RequestContentTypeDetection': this.RequestContentTypeDetection, + '../../infrastructure/Modules': this.Modules, }, }) @@ -215,6 +224,17 @@ describe('UserController', function () { this.UserController.tryDeleteUser(this.req, this.res, this.next) }) + it('should call hook to try to delete v1 account', function (done) { + this.res.sendStatus = code => { + expect(this.Modules.promises.hooks.fire).to.have.been.calledWith( + 'tryDeleteV1Account', + this.user + ) + done() + } + this.UserController.tryDeleteUser(this.req, this.res, this.next) + }) + describe('when no password is supplied', function () { beforeEach(function () { this.req.body.password = '' @@ -873,6 +893,7 @@ describe('UserController', function () { } this.EmailHandler.promises.sendEmail.rejects(anError) }) + it('should not return error but should log it', function (done) { this.res.json.callsFake(result => { expect(result.message.type).to.equal('success') @@ -901,16 +922,19 @@ describe('UserController', function () { this.next ) }) + it('should not run affiliation check', function () { expect(this.UserGetter.promises.getUser).to.not.have.been.called expect(this.UserUpdater.promises.confirmEmail).to.not.have.been.called expect(this.UserUpdater.promises.addAffiliationForNewUser).to.not.have .been.called }) + it('should not return an error', function () { expect(this.next).to.be.calledWith() }) }) + describe('without ensureAffiliation query parameter', function () { beforeEach(async function () { this.Features.hasFeature.withArgs('affiliations').returns(true) @@ -920,16 +944,19 @@ describe('UserController', function () { this.next ) }) + it('should not run middleware', function () { expect(this.UserGetter.promises.getUser).to.not.have.been.called expect(this.UserUpdater.promises.confirmEmail).to.not.have.been.called expect(this.UserUpdater.promises.addAffiliationForNewUser).to.not.have .been.called }) + it('should not return an error', function () { expect(this.next).to.be.calledWith() }) }) + describe('no flagged email', function () { beforeEach(async function () { const email = 'unit-test@overleaf.com' @@ -947,19 +974,23 @@ describe('UserController', function () { this.next ) }) + it('should get the user', function () { expect(this.UserGetter.promises.getUser).to.have.been.calledWith( this.user._id ) }) + it('should not try to add affiliation or update user', function () { expect(this.UserUpdater.promises.addAffiliationForNewUser).to.not.have .been.called }) + it('should not return an error', function () { expect(this.next).to.be.calledWith() }) }) + describe('flagged non-SSO email', function () { let emailFlagged beforeEach(async function () { @@ -980,11 +1011,13 @@ describe('UserController', function () { this.next ) }) + it('should check the user has permission', function () { expect(this.req.assertPermission).to.have.been.calledWith( 'add-affiliation' ) }) + it('should unflag the emails but not confirm', function () { expect( this.UserUpdater.promises.addAffiliationForNewUser @@ -993,10 +1026,12 @@ describe('UserController', function () { this.UserUpdater.promises.confirmEmail ).to.not.have.been.calledWith(this.user._id, emailFlagged) }) + it('should not return an error', function () { expect(this.next).to.be.calledWith() }) }) + describe('flagged SSO email', function () { let emailFlagged beforeEach(async function () { @@ -1018,11 +1053,13 @@ describe('UserController', function () { this.next ) }) + it('should check the user has permission', function () { expect(this.req.assertPermission).to.have.been.calledWith( 'add-affiliation' ) }) + it('should add affiliation to v1, unflag and confirm on v2', function () { expect(this.UserUpdater.promises.addAffiliationForNewUser).to.have.not .been.called @@ -1031,10 +1068,12 @@ describe('UserController', function () { emailFlagged ) }) + it('should not return an error', function () { expect(this.next).to.be.calledWith() }) }) + describe('when v1 returns an error', function () { let emailFlagged beforeEach(async function () { @@ -1056,11 +1095,13 @@ describe('UserController', function () { this.next ) }) + it('should check the user has permission', function () { expect(this.req.assertPermission).to.have.been.calledWith( 'add-affiliation' ) }) + it('should return the error', function () { expect(this.next).to.be.calledWith(sinon.match.instanceOf(Error)) })