From d9b59416429dc0dea90ea35abfc8b54b30f4b031 Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Mon, 22 Jul 2019 12:55:35 -0400 Subject: [PATCH] do not use v1 for setting default email GitOrigin-RevId: 00d4610ae55c0a90699d4bc79e7d08d432087abe --- .../src/Features/User/UserEmailsController.js | 16 +-- .../web/app/src/Features/User/UserUpdater.js | 127 +++++------------- .../test/acceptance/src/UserEmailsTests.js | 24 ++-- .../src/User/UserEmailsControllerTests.js | 12 +- .../test/unit/src/User/UserUpdaterTests.js | 82 +++++++++-- 5 files changed, 117 insertions(+), 144 deletions(-) diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 9f564260e1..9d0efa5119 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -87,18 +87,12 @@ module.exports = UserEmailsController = { if (email == null) { return res.sendStatus(422) } - - return UserUpdater.updateV1AndSetDefaultEmailAddress( - userId, - email, - function(error) { - if (error != null) { - return UserEmailsController._handleEmailError(error, req, res, next) - } else { - return res.sendStatus(200) - } + UserUpdater.setDefaultEmailAddress(userId, email, err => { + if (err) { + return UserEmailsController._handleEmailError(err, req, res, next) } - ) + res.sendStatus(200) + }) }, endorse(req, res, next) { diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index dffd8e05b1..8d108fb29e 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -28,8 +28,6 @@ const { const FeaturesUpdater = require('../Subscription/FeaturesUpdater') const EmailHelper = require('../Helpers/EmailHelper') const Errors = require('../Errors/Errors') -const Settings = require('settings-sharelatex') -const request = require('request') const NewsletterManager = require('../Newsletter/NewsletterManager') module.exports = UserUpdater = { @@ -71,7 +69,7 @@ module.exports = UserUpdater = { return cb(error) }), cb => UserUpdater.addEmailAddress(userId, newEmail, cb), - cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, cb), + cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, true, cb), cb => UserUpdater.removeEmailAddress(userId, oldEmail, cb) ], callback @@ -156,112 +154,47 @@ module.exports = UserUpdater = { // set the default email address by setting the `email` attribute. The email // must be one of the user's multiple emails (`emails` attribute) - setDefaultEmailAddress(userId, email, callback) { + setDefaultEmailAddress(userId, email, allowUnconfirmed, callback) { + if (typeof allowUnconfirmed === 'function') { + callback = allowUnconfirmed + allowUnconfirmed = false + } email = EmailHelper.parseEmail(email) if (email == null) { return callback(new Error('invalid email')) } - return UserGetter.getUserEmail(userId, (error, oldEmail) => { - if (typeof err !== 'undefined' && err !== null) { - return callback(error) + UserGetter.getUser(userId, { email: 1, emails: 1 }, (err, user) => { + if (err) { + return callback(err) + } + if (!user) { + return callback(new Error('invalid userId')) + } + const oldEmail = user.email + const userEmail = user.emails.find(e => e.email === email) + if (!userEmail) { + return callback(new Error('Default email does not belong to user')) + } + if (!userEmail.confirmedAt && !allowUnconfirmed) { + return callback(new Errors.UnconfirmedEmailError()) } const query = { _id: userId, 'emails.email': email } const update = { $set: { email } } - return this.updateUser(query, update, function(error, res) { - if (error != null) { - logger.warn({ error }, 'problem setting default emails') - return callback(error) - } else if (res.n === 0) { - // TODO: Check n or nMatched? - return callback(new Error('Default email does not belong to user')) - } else { - NewsletterManager.changeEmail(oldEmail, email, function() {}) - return callback() + UserUpdater.updateUser(query, update, (err, res) => { + if (err) { + return callback(err) } + // this should not happen + if (res.n === 0) { + return callback(new Error('email update error')) + } + // do not wait - this will log its own errors + NewsletterManager.changeEmail(oldEmail, email, () => {}) + callback() }) }) }, - updateV1AndSetDefaultEmailAddress(userId, email, callback) { - return this.updateEmailAddressInV1(userId, email, error => { - if (error != null) { - return callback(error) - } - return this.setDefaultEmailAddress(userId, email, callback) - }) - }, - - updateEmailAddressInV1(userId, newEmail, callback) { - if (!Settings.apis.v1.url) { - return callback() - } - return UserGetter.getUser(userId, { 'overleaf.id': 1, emails: 1 }, function( - error, - user - ) { - let email - if (error != null) { - return callback(error) - } - if (user == null) { - return callback(new Errors.NotFoundError('no user found')) - } - if ((user.overleaf != null ? user.overleaf.id : undefined) == null) { - return callback() - } - let newEmailIsConfirmed = false - for (email of Array.from(user.emails)) { - if (email.email === newEmail && email.confirmedAt != null) { - newEmailIsConfirmed = true - break - } - } - if (!newEmailIsConfirmed) { - return callback( - new Errors.UnconfirmedEmailError( - "can't update v1 with unconfirmed email" - ) - ) - } - return request( - { - baseUrl: Settings.apis.v1.url, - url: `/api/v1/sharelatex/users/${user.overleaf.id}/email`, - method: 'PUT', - auth: { - user: Settings.apis.v1.user, - pass: Settings.apis.v1.pass, - sendImmediately: true - }, - json: { - user: { - email: newEmail - } - }, - timeout: 5 * 1000 - }, - function(error, response, body) { - if (error != null) { - if (error.code === 'ECONNREFUSED') { - error = new Errors.V1ConnectionError('No V1 connection') - } - return callback(error) - } - if (response.statusCode === 409) { - // Conflict - return callback(new Errors.EmailExistsError('email exists in v1')) - } else if (response.statusCode >= 200 && response.statusCode < 300) { - return callback() - } else { - return callback( - new Error(`non-success code from v1: ${response.statusCode}`) - ) - } - } - ) - }) - }, - confirmEmail(userId, email, confirmedAt, callback) { if (arguments.length === 3) { callback = confirmedAt diff --git a/services/web/test/acceptance/src/UserEmailsTests.js b/services/web/test/acceptance/src/UserEmailsTests.js index a6576c994c..dc18882965 100644 --- a/services/web/test/acceptance/src/UserEmailsTests.js +++ b/services/web/test/acceptance/src/UserEmailsTests.js @@ -653,7 +653,7 @@ describe('UserEmails', function() { ) }) - it('should update the email in v1 if confirmed', function(done) { + it('should not update the email in v1', function(done) { const token = null return async.series( [ @@ -725,18 +725,13 @@ describe('UserEmails', function() { if (error != null) { return done(error) } - expect( - MockV1Api.updateEmail.calledWith( - 42, - 'new-confirmed-default-in-v1@example.com' - ) - ).to.equal(true) - return done() + expect(MockV1Api.updateEmail.callCount).to.equal(0) + done() } ) }) - it('should return an error if the email exists in v1', function(done) { + it('should not return an error if the email exists in v1', function(done) { MockV1Api.existingEmails.push('exists-in-v1@example.com') return async.series( [ @@ -798,11 +793,8 @@ describe('UserEmails', function() { if (error != null) { return done(error) } - expect(response.statusCode).to.equal(409) - expect(body).to.deep.equal({ - message: 'This email is already registered' - }) - return cb() + expect(response.statusCode).to.equal(200) + cb() } ) }, @@ -810,8 +802,8 @@ describe('UserEmails', function() { return this.user.request( { url: '/user/emails', json: true }, function(error, response, body) { - expect(body[0].default).to.equal(true) - expect(body[1].default).to.equal(false) + expect(body[0].default).to.equal(false) + expect(body[1].default).to.equal(true) return cb() } ) diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 37a8517e1c..6073ffa9cb 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -187,22 +187,22 @@ describe('UserEmailsController', function() { beforeEach(function() { this.email = 'email_to_set_default@bar.com' this.req.body.email = this.email - return this.EmailHelper.parseEmail.returns(this.email) + this.EmailHelper.parseEmail.returns(this.email) }) it('sets default email', function(done) { - this.UserUpdater.updateV1AndSetDefaultEmailAddress.callsArgWith(2, null) + this.UserUpdater.setDefaultEmailAddress.yields() - return this.UserEmailsController.setDefault(this.req, { + this.UserEmailsController.setDefault(this.req, { sendStatus: code => { code.should.equal(200) assertCalledWith(this.EmailHelper.parseEmail, this.email) assertCalledWith( - this.UserUpdater.updateV1AndSetDefaultEmailAddress, + this.UserUpdater.setDefaultEmailAddress, this.user._id, this.email ) - return done() + done() } }) }) @@ -210,7 +210,7 @@ describe('UserEmailsController', function() { it('handles email parse error', function(done) { this.EmailHelper.parseEmail.returns(null) - return this.UserEmailsController.setDefault(this.req, { + this.UserEmailsController.setDefault(this.req, { sendStatus: code => { code.should.equal(422) assertNotCalled(this.UserUpdater.setDefaultEmailAddress) diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index b5fb3eaf0d..f206f0c7d0 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/services/web/test/unit/src/User/UserUpdaterTests.js @@ -74,7 +74,8 @@ describe('UserUpdater', function() { name: 'bob', email: 'hello@world.com' } - return (this.newEmail = 'bob@bob.com') + this.newEmail = 'bob@bob.com' + this.callback = sinon.stub() }) afterEach(() => tk.reset()) @@ -83,7 +84,7 @@ describe('UserUpdater', function() { beforeEach(function() { this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email) this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2) - this.UserUpdater.setDefaultEmailAddress = sinon.stub().callsArgWith(2) + this.UserUpdater.setDefaultEmailAddress = sinon.stub().yields() return (this.UserUpdater.removeEmailAddress = sinon .stub() .callsArgWith(2)) @@ -325,14 +326,20 @@ describe('UserUpdater', function() { describe('setDefaultEmailAddress', function() { beforeEach(function() { - this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email) - return this.NewsletterManager.changeEmail.callsArgWith(2, null) + this.stubbedUser.emails = [ + { + email: this.newEmail, + confirmedAt: new Date() + } + ] + this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser) + this.NewsletterManager.changeEmail.callsArgWith(2, null) }) it('set default', function(done) { this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 }) - return this.UserUpdater.setDefaultEmailAddress( + this.UserUpdater.setDefaultEmailAddress( this.stubbedUser._id, this.newEmail, err => { @@ -343,7 +350,7 @@ describe('UserUpdater', function() { { $set: { email: this.newEmail } } ) .should.equal(true) - return done() + done() } ) }) @@ -351,7 +358,7 @@ describe('UserUpdater', function() { it('set changed the email in newsletter', function(done) { this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 }) - return this.UserUpdater.setDefaultEmailAddress( + this.UserUpdater.setDefaultEmailAddress( this.stubbedUser._id, this.newEmail, err => { @@ -359,7 +366,7 @@ describe('UserUpdater', function() { this.NewsletterManager.changeEmail .calledWith(this.stubbedUser.email, this.newEmail) .should.equal(true) - return done() + done() } ) }) @@ -369,12 +376,12 @@ describe('UserUpdater', function() { .stub() .callsArgWith(2, new Error('nope')) - return this.UserUpdater.setDefaultEmailAddress( + this.UserUpdater.setDefaultEmailAddress( this.stubbedUser._id, this.newEmail, err => { should.exist(err) - return done() + done() } ) }) @@ -382,26 +389,73 @@ describe('UserUpdater', function() { it('handle missed update', function(done) { this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 0 }) - return this.UserUpdater.setDefaultEmailAddress( + this.UserUpdater.setDefaultEmailAddress( this.stubbedUser._id, this.newEmail, err => { should.exist(err) - return done() + done() } ) }) it('validates email', function(done) { - return this.UserUpdater.setDefaultEmailAddress( + this.UserUpdater.setDefaultEmailAddress( this.stubbedUser._id, '.edu', err => { should.exist(err) - return done() + done() } ) }) + + describe('when email not confirmed', () => { + beforeEach(function() { + this.stubbedUser.emails = [ + { + email: this.newEmail, + confirmedAt: null + } + ] + this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser) + this.UserUpdater.updateUser = sinon.stub() + this.NewsletterManager.changeEmail = sinon.stub() + }) + + it('should callback with error', function() { + this.UserUpdater.setDefaultEmailAddress( + this.stubbedUser._id, + this.newEmail, + this.callback + ) + this.callback.firstCall.args[0].name.should.equal( + 'UnconfirmedEmailError' + ) + this.UserUpdater.updateUser.callCount.should.equal(0) + this.NewsletterManager.changeEmail.callCount.should.equal(0) + }) + }) + + describe('when email does not belong to user', () => { + beforeEach(function() { + this.stubbedUser.emails = [] + this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser) + this.UserUpdater.updateUser = sinon.stub() + this.NewsletterManager.changeEmail = sinon.stub() + }) + + it('should callback with error', function() { + this.UserUpdater.setDefaultEmailAddress( + this.stubbedUser._id, + this.newEmail, + this.callback + ) + this.callback.firstCall.args[0].name.should.equal('Error') + this.UserUpdater.updateUser.callCount.should.equal(0) + this.NewsletterManager.changeEmail.callCount.should.equal(0) + }) + }) }) describe('confirmEmail', function() {