fix(emails): Can create secondary email if it is unverified in anothe…

…r account (#1892) r=vladikoff,seanmonstar Fixes https://github.com/mozilla/fxa-bugzilla-mirror/issues/275
mozilla · May 15, 2017 · 34e3841 · 34e3841
1 parent 495acd6
commit 34e3841
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 4 deletions.
diff --git a/lib/db.js b/lib/db.js
@@ -863,6 +863,9 @@ module.exports = (
     })
 
     return this.pool.get('/email/' + Buffer(email, 'utf8').toString('hex'))
+      .then((body) => {
+        return bufferize(body)
+      })
       .catch((err) => {
         if (isNotFoundError(err)) {
           throw error.unknownSecondaryEmail()

diff --git a/lib/routes/account.js b/lib/routes/account.js
@@ -158,7 +158,7 @@ module.exports = (
                 throw error.verifiedSecondaryEmailAlreadyExists()
               }
 
-              return db.deleteEmail(Buffer(secondaryEmailRecord.uid, 'hex'), secondaryEmailRecord.email)
+              return db.deleteEmail(Buffer.from(secondaryEmailRecord.uid, 'hex'), secondaryEmailRecord.email)
             })
             .catch((err) => {
               if (err.errno !== error.ERRNO.SECONDARY_EMAIL_UNKNOWN) {
@@ -2132,7 +2132,8 @@ module.exports = (
         }
 
         customs.check(request, primaryEmail, 'createEmail')
-          .then(checkEmail)
+          .then(deleteAccountIfUnverified)
+          .then(deleteSecondaryEmailIfUnverified)
           .then(generateRandomValues)
           .then(createEmail)
           .then(sendEmailVerification)
@@ -2143,7 +2144,7 @@ module.exports = (
             reply
           )
 
-        function checkEmail() {
+        function deleteAccountIfUnverified() {
           return db.emailRecord(email)
             .then((emailRecord) => {
               if (emailRecord.emailVerified) {
@@ -2169,6 +2170,22 @@ module.exports = (
             })
         }
 
+        function deleteSecondaryEmailIfUnverified() {
+          return db.getSecondaryEmail(email)
+            .then((secondaryEmailRecord) => {
+              // Only delete secondary email if it is unverified and does not belong
+              // to the current user.
+              if (! secondaryEmailRecord.isVerified && ! butil.buffersAreEqual(secondaryEmailRecord.uid, uid)) {
+                return db.deleteEmail(Buffer.from(secondaryEmailRecord.uid, 'hex'), secondaryEmailRecord.email)
+              }
+            })
+            .catch((err) => {
+              if (err.errno !== error.ERRNO.SECONDARY_EMAIL_UNKNOWN) {
+                throw err
+              }
+            })
+        }
+
         function generateRandomValues() {
           return random(16)
             .then(bytes => {

diff --git a/test/remote/recovery_email_emails.js b/test/remote/recovery_email_emails.js
@@ -87,6 +87,53 @@ describe('remote emails', function () {
       }
     )
 
+    it('can create email if email is unverified on another account', () => {
+      let client2
+      const clientEmail = server.uniqueEmail()
+      const secondEmail = server.uniqueEmail()
+      return client.createEmail(secondEmail)
+        .then((res) => {
+          assert.ok(res, 'ok response')
+          return server.mailbox.waitForEmail(secondEmail)
+        })
+        .then(() => {
+          return client.accountEmails()
+        })
+        .then((res) => {
+          assert.equal(res.length, 2, 'returns number of emails')
+          assert.equal(res[1].email, secondEmail, 'returns correct email')
+          assert.equal(res[1].isPrimary, false, 'returns correct isPrimary')
+          assert.equal(res[1].verified, false, 'returns correct verified')
+          return Client.createAndVerify(config.publicUrl, clientEmail, password, server.mailbox)
+            .catch(assert.fail)
+        })
+        .then((x) => {
+          client2 = x
+          assert.equal(client2.email, clientEmail, 'account created with email')
+          return client2.createEmail(secondEmail)
+        })
+        .then((res) => {
+          assert.ok(res, 'ok response')
+          return client.accountEmails()
+        })
+        .then((res) => {
+          // Secondary email on first account should have been deleted
+          assert.equal(res.length, 1, 'returns number of emails')
+          assert.equal(res[0].email, client.email, 'returns correct email')
+          assert.equal(res[0].isPrimary, true, 'returns correct isPrimary')
+          assert.equal(res[0].verified, true, 'returns correct verified')
+          return client2.accountEmails()
+        })
+        .then((res) => {
+          // Secondary email should be on the second account
+          assert.equal(res.length, 2, 'returns number of emails')
+          assert.equal(res[1].email, secondEmail, 'returns correct email')
+          assert.equal(res[1].isPrimary, false, 'returns correct isPrimary')
+          assert.equal(res[1].verified, false, 'returns correct verified')
+        })
+    })
+
+
     it(
       'fails create when email is user primary email',
       () => {
@@ -119,7 +166,7 @@ describe('remote emails', function () {
     )
 
     it(
-      'fails create when email exists in other user account',
+      'fails create when verified secondary email exists in other user account',
       () => {
         const anotherUserEmail = server.uniqueEmail()
         const anotherUserSecondEmail = server.uniqueEmail()
@@ -130,6 +177,13 @@ describe('remote emails', function () {
             assert.ok(client.authAt, 'authAt was set')
             return anotherClient.createEmail(anotherUserSecondEmail)
           })
+          .then(() => {
+            return server.mailbox.waitForEmail(anotherUserSecondEmail)
+          })
+          .then((emailData) => {
+            const emailCode = emailData['headers']['x-verify-code']
+            return anotherClient.verifySecondaryEmail(emailCode, anotherUserSecondEmail)
+          })
           .then((res) => {
             assert.ok(res, 'ok response')
             return client.createEmail(anotherUserSecondEmail)