From 8b4659e7002781fb0ce21f335a32b593474dfd1d Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Mon, 30 Nov 2020 08:57:18 -0600 Subject: [PATCH] Merge pull request #3409 from overleaf/jel-inst-leavers-db Add reconfirmedAt to UserSchema GitOrigin-RevId: 543b57236bbf964c72c6587362a6b6d6b7b7caa6 --- .../web/app/src/Features/User/UserUpdater.js | 93 ++++++++++--------- services/web/app/src/models/User.js | 3 +- .../test/acceptance/src/UserEmailsTests.js | 64 +++++++++++++ .../test/unit/src/User/UserUpdaterTests.js | 57 ++++++++---- 4 files changed, 155 insertions(+), 62 deletions(-) diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 3c33d4e9cf..8cc752e01b 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -155,6 +155,53 @@ async function setDefaultEmailAddress( } } +async function confirmEmail(userId, email) { + // used for initial email confirmation (non-SSO and SSO) + // also used for reconfirmation of non-SSO emails + const confirmedAt = new Date() + email = EmailHelper.parseEmail(email) + if (email == null) { + throw new Error('invalid email') + } + logger.log({ userId, email }, 'confirming user email') + + try { + await InstitutionsAPIPromises.addAffiliation(userId, email, { confirmedAt }) + } catch (error) { + throw OError.tag(error, 'problem adding affiliation while confirming email') + } + + const query = { + _id: userId, + 'emails.email': email + } + const update = { + $set: { + 'emails.$.reconfirmedAt': confirmedAt + } + } + const user = await UserGetter.promises.getUser(userId) + const emailUnconfirmed = user.emails.find(emailData => { + if (emailData.email === email && !emailData.confirmedAt) return true + }) + if (emailUnconfirmed) { + update.$set['emails.$.confirmedAt'] = confirmedAt + } + + if (Features.hasFeature('affiliations')) { + update['$unset'] = { + 'emails.$.affiliationUnchecked': 1 + } + } + + const res = await UserUpdater.promises.updateUser(query, update) + + if (res.n === 0) { + throw new Errors.NotFoundError('user id and email do no match') + } + await FeaturesUpdater.promises.refreshFeatures(userId) +} + const UserUpdater = { addAffiliationForNewUser(userId, email, affiliationOptions, callback) { if (callback == null) { @@ -276,49 +323,7 @@ const UserUpdater = { // must be one of the user's multiple emails (`emails` attribute) setDefaultEmailAddress: callbackify(setDefaultEmailAddress), - confirmEmail(userId, email, confirmedAt, callback) { - if (arguments.length === 3) { - callback = confirmedAt - confirmedAt = new Date() - } - email = EmailHelper.parseEmail(email) - if (email == null) { - return callback(new Error('invalid email')) - } - logger.log({ userId, email }, 'confirming user email') - addAffiliation(userId, email, { confirmedAt }, error => { - if (error != null) { - OError.tag(error, 'problem adding affiliation while confirming email') - return callback(error) - } - - const query = { - _id: userId, - 'emails.email': email - } - const update = { - $set: { - 'emails.$.confirmedAt': confirmedAt - } - } - if (Features.hasFeature('affiliations')) { - update['$unset'] = { - 'emails.$.affiliationUnchecked': 1 - } - } - UserUpdater.updateUser(query, update, (error, res) => { - if (error != null) { - return callback(error) - } - if (res.n === 0) { - return callback( - new Errors.NotFoundError('user id and email do no match') - ) - } - FeaturesUpdater.refreshFeatures(userId, callback) - }) - }) - }, + confirmEmail: callbackify(confirmEmail), removeReconfirmFlag(userId, callback) { UserUpdater.updateUser( @@ -344,7 +349,7 @@ const UserUpdater = { const promises = { addAffiliationForNewUser: promisify(UserUpdater.addAffiliationForNewUser), addEmailAddress, - confirmEmail: promisify(UserUpdater.confirmEmail), + confirmEmail, setDefaultEmailAddress, updateUser: promisify(UserUpdater.updateUser), removeReconfirmFlag: promisify(UserUpdater.removeReconfirmFlag) diff --git a/services/web/app/src/models/User.js b/services/web/app/src/models/User.js index 5af2336b30..38773fc4a0 100644 --- a/services/web/app/src/models/User.js +++ b/services/web/app/src/models/User.js @@ -31,7 +31,8 @@ const UserSchema = new Schema({ }, confirmedAt: { type: Date }, samlProviderId: { type: String }, - affiliationUnchecked: { type: Boolean } + affiliationUnchecked: { type: Boolean }, + reconfirmedAt: { type: Date } } ], first_name: { type: String, default: '' }, diff --git a/services/web/test/acceptance/src/UserEmailsTests.js b/services/web/test/acceptance/src/UserEmailsTests.js index 6e121fc47b..db5d1d79f8 100644 --- a/services/web/test/acceptance/src/UserEmailsTests.js +++ b/services/web/test/acceptance/src/UserEmailsTests.js @@ -6,6 +6,34 @@ const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const MockV1Api = require('./helpers/MockV1Api') const expectErrorResponse = require('./helpers/expectErrorResponse') +async function confirmEmail(userHelper, email) { + let response + // UserHelper.createUser does not create a confirmation token + response = await userHelper.request.post({ + form: { + email + }, + simple: false, + uri: '/user/emails/resend_confirmation' + }) + expect(response.statusCode).to.equal(200) + const tokenData = await db.tokens + .find({ + use: 'email_confirmation', + 'data.user_id': userHelper.user._id.toString(), + usedAt: { $exists: false } + }) + .next() + response = await userHelper.request.post({ + form: { + token: tokenData.token + }, + simple: false, + uri: '/user/emails/confirm' + }) + expect(response.statusCode).to.equal(200) +} + describe('UserEmails', function() { beforeEach(function(done) { this.timeout(20000) @@ -41,7 +69,9 @@ describe('UserEmails', function() { expect(error).to.not.exist expect(response.statusCode).to.equal(200) expect(body[0].confirmedAt).to.not.exist + expect(body[0].reconfirmedAt).to.not.exist expect(body[1].confirmedAt).to.not.exist + expect(body[1].reconfirmedAt).to.not.exist cb() } ) @@ -88,7 +118,10 @@ describe('UserEmails', function() { expect(error).to.not.exist expect(response.statusCode).to.equal(200) expect(body[0].confirmedAt).to.not.exist + expect(body[0].reconfirmedAt).to.not.exist expect(body[1].confirmedAt).to.exist + expect(body[1].reconfirmedAt).to.exist + expect(body[1].reconfirmedAt).to.deep.equal(body[1].confirmedAt) cb() } ) @@ -239,6 +272,37 @@ describe('UserEmails', function() { }) }) + describe('reconfirm an email', function() { + let email, userHelper, confirmedAtDate + beforeEach(async function() { + userHelper = new UserHelper() + email = userHelper.getDefaultEmail() + userHelper = await UserHelper.createUser({ email }) + userHelper = await UserHelper.loginUser({ + email, + password: userHelper.getDefaultPassword() + }) + // original confirmation + await confirmEmail(userHelper, email) + const user = (await UserHelper.getUser({ email })).user + confirmedAtDate = user.emails[0].confirmedAt + expect(user.emails[0].confirmedAt).to.exist + expect(user.emails[0].reconfirmedAt).to.exist + }) + it('should set reconfirmedAt and not reset confirmedAt', async function() { + await confirmEmail(userHelper, email) + const user = (await UserHelper.getUser({ email })).user + expect(user.emails[0].confirmedAt).to.exist + expect(user.emails[0].reconfirmedAt).to.exist + expect(user.emails[0].confirmedAt).to.deep.equal(confirmedAtDate) + expect(user.emails[0].reconfirmedAt).to.not.deep.equal( + user.emails[0].confirmedAt + ) + expect(user.emails[0].reconfirmedAt > user.emails[0].confirmedAt).to.be + .true + }) + }) + describe('with an expired token', function() { it('should not confirm the email', function(done) { let token = null diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index 1edc88d95a..481d068702 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/services/web/test/unit/src/User/UserUpdaterTests.js @@ -72,9 +72,12 @@ describe('UserUpdater', function() { '../../infrastructure/Features': (this.Features = { hasFeature: sinon.stub().returns(false) }), - '../Subscription/FeaturesUpdater': { - refreshFeatures: this.refreshFeatures - }, + '../Subscription/FeaturesUpdater': (this.FeaturesUpdater = { + refreshFeatures: this.refreshFeatures, + promises: { + refreshFeatures: sinon.stub().resolves() + } + }), 'settings-sharelatex': (this.settings = {}), request: (this.request = {}), '../Newsletter/NewsletterManager': this.NewsletterManager, @@ -87,10 +90,16 @@ describe('UserUpdater', function() { } }) + this.stubbedUserEmail = 'hello@world.com' this.stubbedUser = { _id: '3131231', name: 'bob', - email: 'hello@world.com' + email: this.stubbedUserEmail, + emails: [ + { + email: this.stubbedUserEmail + } + ] } this.newEmail = 'bob@bob.com' this.callback = sinon.stub() @@ -708,19 +717,28 @@ describe('UserUpdater', function() { describe('confirmEmail', function() { beforeEach(function() { - this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 }) + this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 1 }) + this.UserGetter.promises.getUser.resolves(this.stubbedUser) }) it('should update the email record', function(done) { this.UserUpdater.confirmEmail( this.stubbedUser._id, - this.newEmail, + this.stubbedUserEmail, err => { should.not.exist(err) - this.UserUpdater.updateUser + this.UserUpdater.promises.updateUser .calledWith( - { _id: this.stubbedUser._id, 'emails.email': this.newEmail }, - { $set: { 'emails.$.confirmedAt': new Date() } } + { + _id: this.stubbedUser._id, + 'emails.email': this.stubbedUserEmail + }, + { + $set: { + 'emails.$.confirmedAt': new Date(), + 'emails.$.reconfirmedAt': new Date() + } + } ) .should.equal(true) done() @@ -734,9 +752,11 @@ describe('UserUpdater', function() { this.newEmail, err => { should.not.exist(err) - this.addAffiliation.calledOnce.should.equal(true) + this.InstitutionsAPI.promises.addAffiliation.calledOnce.should.equal( + true + ) sinon.assert.calledWith( - this.addAffiliation, + this.InstitutionsAPI.promises.addAffiliation, this.stubbedUser._id, this.newEmail, { confirmedAt: new Date() } @@ -747,9 +767,9 @@ describe('UserUpdater', function() { }) it('handle error', function(done) { - this.UserUpdater.updateUser = sinon + this.UserUpdater.promises.updateUser = sinon .stub() - .callsArgWith(2, new Error('nope')) + .throws(new Error('nope')) this.UserUpdater.confirmEmail( this.stubbedUser._id, @@ -762,7 +782,7 @@ describe('UserUpdater', function() { }) it('handle missed update', function(done) { - this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 0 }) + this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 0 }) this.UserUpdater.confirmEmail( this.stubbedUser._id, @@ -782,13 +802,13 @@ describe('UserUpdater', function() { }) it('handle affiliation error', function(done) { - this.addAffiliation.callsArgWith(3, new Error('nope')) + this.InstitutionsAPI.promises.addAffiliation.throws(Error('nope')) this.UserUpdater.confirmEmail( this.stubbedUser._id, this.newEmail, err => { should.exist(err) - this.UserUpdater.updateUser.called.should.equal(false) + this.UserUpdater.promises.updateUser.called.should.equal(false) done() } ) @@ -800,7 +820,10 @@ describe('UserUpdater', function() { this.newEmail, err => { should.not.exist(err) - sinon.assert.calledWith(this.refreshFeatures, this.stubbedUser._id) + sinon.assert.calledWith( + this.FeaturesUpdater.promises.refreshFeatures, + this.stubbedUser._id + ) done() } )