Merge pull request #1489 from mozilla/issue-1451-remove-sendEmailIfUnverified

refactor(email): Remove `sendEmailIfUnverified` r=@shane-tomlinson
2016-10-14 21:04:49 +01:00 · 2016-10-14 21:04:49 +01:00 · 230b85ee8f
--- a/lib/routes/account.js
+++ b/lib/routes/account.js
@ -699,12 +699,9 @@ module.exports = function (

        function sendVerifyAccountEmail() {
          // Delegate sending emails for unverified users to auth-server.
-          // Will be removed once all clients have been updated not to send verify emails.
-          // https://github.com/mozilla/fxa-auth-server/issues/1325
-          var shouldSendVerifyAccountEmail = requestHelper.shouldSendVerifyAccountEmail(emailRecord, request)
          emailSent = false

-          if (shouldSendVerifyAccountEmail) {
+          if (!emailRecord.emailVerified) {
            // Only use tokenVerificationId if it is set, otherwise use the corresponding email code
            // This covers the cases where sign-in confirmation is disabled or not needed.
            var emailCode = tokenVerificationId ? tokenVerificationId : emailRecord.emailCode
--- a/lib/routes/utils/request_helper.js
+++ b/lib/routes/utils/request_helper.js
@ -12,25 +12,6 @@ function wantsKeys (request) {
  return request.query && request.query.keys === 'true'
 }

-/**
- * Returns whether or not to send the verify account email on a login
- * attempt. This never sends a verification email to an already verified email.
- *
- * @param request
- * @returns {boolean}
- */
-function shouldSendVerifyAccountEmail(account, request) {
-
-  var sendEmailIfUnverified = request.query.sendEmailIfUnverified
-
-  // Only the content-server sends metrics context. For all non content-server
-  // requests, send the verification email.
-  var context = !!(request.payload && request.payload.metricsContext)
-
-  return (!context || !!sendEmailIfUnverified) && !account.emailVerified
-}
-
 module.exports = {
-  wantsKeys: wantsKeys,
-  shouldSendVerifyAccountEmail: shouldSendVerifyAccountEmail
+  wantsKeys: wantsKeys
 }
--- a/test/local/account_routes.js
+++ b/test/local/account_routes.js
@ -867,7 +867,7 @@ test('/account/login', function (t) {
    })

    t.test('sign-in unverified account', function (t) {
-      t.plan(2)
+      t.plan(1)
      var emailCode = crypto.randomBytes(16)
      mockDB.emailRecord = function () {
        return P.resolve({
@ -885,23 +885,7 @@ test('/account/login', function (t) {
        })
      }

-      t.test('`sendEmailIfUnverified=false`, don\'t send any code', function (t) {
-        mockRequest.query.sendEmailIfUnverified = false
-        return runTest(route, mockRequest, function (response) {
-          t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
-          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
-          t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-          t.equal(response.verified, false, 'response indicates account is unverified')
-          t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-          t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
-          t.equal(response.emailSent, false, 'email sent')
-        }).then(function () {
-          mockRequest.query.sendEmailIfUnverified = undefined
-        })
-      })
-
-      t.test('`sendEmailIfUnverified=true`, send\'s email code', function (t) {
-        mockRequest.query.sendEmailIfUnverified = true
+      t.test('send\'s email code', function (t) {
        return runTest(route, mockRequest, function (response) {
          t.equal(mockMailer.sendVerifyCode.callCount, 1, 'mailer.sendVerifyCode was called')

@ -916,7 +900,6 @@ test('/account/login', function (t) {
          t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
          t.equal(response.emailSent, true, 'email sent')
        }).then(function () {
-          mockRequest.query.sendEmailIfUnverified = undefined
          mockMailer.sendVerifyCode.reset()
          mockDB.createSessionToken.reset()
          mockMetricsContext.stash.reset()
@ -926,7 +909,7 @@ test('/account/login', function (t) {
  })

  t.test('sign-in confirmation enabled', function (t) {
-    t.plan(9)
+    t.plan(8)
    config.signinConfirmation.enabled = true
    config.signinConfirmation.supportedClients = [ 'fx_desktop_v3' ]
    config.signinConfirmation.enabledEmailAddresses = /.+@mozilla\.com$/
@ -1060,7 +1043,6 @@ test('/account/login', function (t) {
          wrapWrapKb: crypto.randomBytes(32)
        })
      }
-
      return runTest(route, mockRequest, function (response) {
        t.equal(mockDB.createSessionToken.callCount, 1, 'db.createSessionToken was called')
        var tokenData = mockDB.createSessionToken.getCall(0).args[0]
@ -1130,7 +1112,7 @@ test('/account/login', function (t) {
      })
    })

-    t.test('unverified account does not get any confirmation emails', function (t) {
+    t.test('unverified account gets account confirmation email', function (t) {
      config.signinConfirmation.supportedClients = [ 'fx_desktop_v3' ]
      mockRequest.payload.email = 'test@mozilla.com'
      mockDB.emailRecord = function () {
@ -1153,75 +1135,20 @@ test('/account/login', function (t) {
        var tokenData = mockDB.createSessionToken.getCall(0).args[0]
        t.ok(tokenData.mustVerify, 'sessionToken must be verified before use')
        t.ok(tokenData.tokenVerificationId, 'sessionToken was created unverified')
-        t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
+        t.equal(mockMailer.sendVerifyCode.callCount, 1, 'mailer.sendVerifyCode was called')
        t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
        t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
        t.notOk(response.verified, 'response indicates account is not verified')
        t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
        t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
-      })
-    })
-
-    t.test('sign-in with verified account', function (t) {
-      t.plan(4)
-      mockDB.emailRecord = function () {
-        return P.resolve({
-          authSalt: crypto.randomBytes(32),
-          data: crypto.randomBytes(32),
-          email: 'test@mozilla.com',
-          emailVerified: true,
-          kA: crypto.randomBytes(32),
-          lastAuthAt: function () {
-            return Date.now()
-          },
-          uid: uid,
-          wrapWrapKb: crypto.randomBytes(32)
-        })
-      }
-
-      // When signing in with a verified account, only send `VerifyLoginEmail` emails
-      var items = [undefined, true, false]
-      items.forEach(function (item) {
-        mockRequest.query.sendEmailIfUnverified = item
-        t.test('with `sendEmailIfUnverified` param, ' + item, function (t) {
-          return runTest(route, mockRequest, function (response) {
-            t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
-            t.equal(mockMailer.sendVerifyLoginEmail.callCount, 1, 'mailer.sendVerifyLoginEmail was called')
-            t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-            t.notOk(response.verified, 'response indicates account is not verified')
-            t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-            t.equal(response.verificationReason, 'login', 'verificationReason is login')
-            t.equal(response.emailSent, false, 'email not sent')
-          }).then(function () {
-            mockMailer.sendVerifyLoginEmail.reset()
-          })
-        })
-      })
-      t.test('not from content server', function (t) {
-        mockRequest.query.sendEmailIfUnverified = undefined
-        mockRequest.payload.metricsContext = undefined
-        return runTest(route, mockRequest, function (response) {
-          t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
-          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 1, 'mailer.sendVerifyLoginEmail was called')
-          t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-          t.notOk(response.verified, 'response indicates account is not verified')
-          t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-          t.equal(response.verificationReason, 'login', 'verificationReason is login')
-          t.equal(response.emailSent, false, 'email not sent')
-        }).then(function () {
-          mockMailer.sendVerifyLoginEmail.reset()
-          mockRequest.payload.metricsContext = {
-            flowBeginTime: Date.now(),
-            flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
-            entrypoint: 'preferences',
-            utmContent: 'some-content-string'
-          }
-        })
+      }).then(function () {
+        mockMailer.sendVerifyCode.reset()
+        mockDB.createSessionToken.reset()
      })
    })

    t.test('sign-in with unverified account', function (t) {
-      t.plan(4)
+      t.plan(1)
      mockDB.emailRecord = function () {
        return P.resolve({
          authSalt: crypto.randomBytes(32),
@ -1237,53 +1164,7 @@ test('/account/login', function (t) {
        })
      }

-      t.test('without `sendEmailIfUnverified` param', function (t) {
-        mockRequest.query.sendEmailIfUnverified = undefined
-        return runTest(route, mockRequest, function (response) {
-          t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
-          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
-          t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-          t.equal(response.verified, false, 'response indicates account is unverified')
-          t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-          t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
-          t.equal(response.emailSent, false, 'email not sent')
-        })
-      })
-
-      t.test('with `sendEmailIfUnverified` param, true', function (t) {
-        mockRequest.query.sendEmailIfUnverified = true
-        return runTest(route, mockRequest, function (response) {
-          t.equal(mockMailer.sendVerifyCode.callCount, 1, 'mailer.sendVerifyCode was called')
-          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
-          t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-          t.equal(response.verified, false, 'response indicates account is unverified')
-          t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-          t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
-          t.equal(response.emailSent, true, 'email sent')
-        }).then(function () {
-          mockRequest.query.sendEmailIfUnverified = undefined
-          mockMailer.sendVerifyCode.reset()
-        })
-      })
-
-      t.test('with `sendEmailIfUnverified` param, false', function (t) {
-        mockRequest.query.sendEmailIfUnverified = false
-        return runTest(route, mockRequest, function (response) {
-          t.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyCode was not called')
-          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
-          t.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 0, 'mailer.sendNewDeviceLoginNotification was not called')
-          t.equal(response.verified, false, 'response indicates account is unverified')
-          t.equal(response.verificationMethod, 'email', 'verificationMethod is email')
-          t.equal(response.verificationReason, 'signup', 'verificationReason is signup')
-          t.equal(response.emailSent, false, 'email sent')
-        }).then(function () {
-          mockRequest.query.sendEmailIfUnverified = undefined
-        })
-      })
-
-      t.test('not from content server', function (t) {
-        mockRequest.query.sendEmailIfUnverified = undefined
-        mockRequest.payload.metricsContext = undefined
+      t.test('send\'s verify account email', function (t) {
        return runTest(route, mockRequest, function (response) {
          t.equal(mockMailer.sendVerifyCode.callCount, 1, 'mailer.sendVerifyCode was called')
          t.equal(mockMailer.sendVerifyLoginEmail.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
@ -1321,7 +1202,7 @@ test('/account/login', function (t) {
    mockRequest.app.clientAddress = '63.245.221.32'
    return runTest(route, mockRequest, function () {
      t.equal(mockLog.error.callCount, 0, 'log.error was not called')
-    }).then(function() {
+    }).then(function () {
      mockDB.sessions = function () {
        return P.resolve(new Array(201))
      }
@ -1340,7 +1221,7 @@ test('/account/login', function (t) {
  t.test('checks security history', function (t) {
    t.plan(3)
    var record
-    mockLog.info = sinon.spy(function(arg) {
+    mockLog.info = sinon.spy(function (arg) {
      if (arg.op.indexOf('Account.history') === 0) {
        record = arg
      }
--- a/test/local/request_helper_tests.js
+++ b/test/local/request_helper_tests.js
@ -11,9 +11,8 @@ test(
  'interface is correct',
  t => {
    t.equal(typeof requestHelper, 'object', 'object type should be exported')
-    t.equal(Object.keys(requestHelper).length, 2, 'object should have two properties')
+    t.equal(Object.keys(requestHelper).length, 1, 'object should have one properties')
    t.equal(typeof requestHelper.wantsKeys, 'function', 'wantsKeys should be function')
-    t.equal(typeof requestHelper.shouldSendVerifyAccountEmail, 'function', 'shouldSendVerifyAccountEmail should be function')

    t.end()
  }
@ -30,76 +29,3 @@ test(
    t.end()
  }
 )
-
-test(
-  'shouldSendVerifyAccountEmail',
-  t => {
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: false,
-    }, {
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), true, 'should return true when request has no payload and account is not verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: true,
-    }, {
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), false, 'should return false when request has no payload and account is verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: false,
-    }, {
-      payload: {},
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), true, 'should return true when payload has no metrics context and account is not verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: true,
-    }, {
-      payload: {},
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), false, 'should return false when payload has no metrics context and account is verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: false,
-    }, {
-      payload: {
-        metricsContext: {}
-      },
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), true, 'should return true when payload has metrics context and sendEmailIfUnverified is set and account is not verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: true,
-    }, {
-      payload: {
-        metricsContext: {}
-      },
-      query: {
-        sendEmailIfUnverified: 'true'
-      }
-    }), false, 'should return false when payload has metrics context and sendEmailIfUnverified is set and account is verified')
-
-    t.equal(requestHelper.shouldSendVerifyAccountEmail({
-      emailVerified: false,
-    }, {
-      payload: {
-        metricsContext: {}
-      },
-      query: {}
-    }), false, 'should return false when payload has metrics context and sendEmailIfUnverified is not set and account is not verified')
-
-    t.end()
-  }
-)
-