Merge pull request #3409 from overleaf/jel-inst-leavers-db

Add reconfirmedAt to UserSchema

GitOrigin-RevId: 543b57236bbf964c72c6587362a6b6d6b7b7caa6

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)

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: '' },

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

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()
         }
       )