From 17e787bbd25e6d4c3387686d2f59a6b75cf395f3 Mon Sep 17 00:00:00 2001 From: Phil Booth Date: Wed, 31 Oct 2018 07:06:21 +0000 Subject: [PATCH] chore(errors): make email-sending errors a 422 for new addresses In 75815f21, we added an error for email-sending failure to some routes. Quite reasonably, we made it a 500 on the assumption that it probably indicated a problem somewhere in our email-sending infrastructure, but it turns out that in most cases it indicates a user mistyped their email address. That causes Sentry to alert us pretty noisily, so this change seeks to return a 422 instead in cases where the email address is unverified. Some rationale behind that decision: * We still want actual infrastructure errors to be a 500, so I opted to keep the 500 in place on `/account/login` for verified accounts. * For unverified accounts in `/account/login`, `/account/create` and `POST /recovery_email`, I wanted something different to a 400 so that it was distinct from our regular validation errors. The description for 422 seemed like a good appropriation for that. * I avoided adding a new errno because that would require a knock-on change in the content server, and we want to target this as a point release for train 123. --- docs/api.md | 6 +++--- lib/error.js | 16 +++++++++++++--- lib/routes/account.js | 2 +- lib/routes/emails.js | 2 +- lib/routes/utils/signin.js | 5 +++-- test/local/routes/account.js | 4 ++-- test/local/routes/emails.js | 1 + 7 files changed, 24 insertions(+), 12 deletions(-) diff --git a/docs/api.md b/docs/api.md index 62b91b0bd..04970aeaa 100644 --- a/docs/api.md +++ b/docs/api.md @@ -273,7 +273,7 @@ for `code` and `errno` are: This email can not currently be used to login * `code: 400, errno: 150`: Can not resend email code to an email that does not belong to this account -* `code: 500, errno: 151`: +* `code: 422, errno: 151`: Failed to send email * `code: 400, errno: 152`: Invalid token verification code @@ -538,7 +538,7 @@ by the following errors * `code: 400, errno: 144`: Email already exists -* `code: 500, errno: 151`: +* `code: 422, errno: 151`: Failed to send email @@ -1888,7 +1888,7 @@ by the following errors * `code: 400, errno: 141`: Email already exists -* `code: 500, errno: 151`: +* `code: 422, errno: 151`: Failed to send email diff --git a/lib/error.js b/lib/error.js index 7a1090a83..c7de19f98 100644 --- a/lib/error.js +++ b/lib/error.js @@ -734,10 +734,20 @@ AppError.cannotResendEmailCodeToUnownedEmail = function () { }) } -AppError.cannotSendEmail = function () { +AppError.cannotSendEmail = function (isNewAddress) { + let code, error + + if (isNewAddress) { + code = 422 + error = 'Unprocessable Entity' + } else { + code = 500 + error = 'Internal Server Error' + } + return new AppError({ - code: 500, - error: 'Internal Server Error', + code, + error, errno: ERRNO.FAILED_TO_SEND_EMAIL, message: 'Failed to send email' }) diff --git a/lib/routes/account.js b/lib/routes/account.js index 783e353a4..430954102 100644 --- a/lib/routes/account.js +++ b/lib/routes/account.js @@ -305,7 +305,7 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push) // show an error to the user, the account is already created. // the user can come back later and try again. - throw error.cannotSendEmail() + throw error.cannotSendEmail(true) }) } } diff --git a/lib/routes/emails.js b/lib/routes/emails.js index 9028bc614..32ee4cfba 100644 --- a/lib/routes/emails.js +++ b/lib/routes/emails.js @@ -642,7 +642,7 @@ module.exports = (log, db, mailer, config, customs, push) => { log.error({op: 'mailer.sendVerifySecondaryEmail', err: err}) return db.deleteEmail(emailData.uid, emailData.normalizedEmail) .then(() => { - throw error.cannotSendEmail() + throw error.cannotSendEmail(true) }) }) } diff --git a/lib/routes/utils/signin.js b/lib/routes/utils/signin.js index 383a1c934..11dad5298 100644 --- a/lib/routes/utils/signin.js +++ b/lib/routes/utils/signin.js @@ -177,6 +177,7 @@ module.exports = (log, config, customs, db, mailer) => { const redirectTo = request.payload.redirectTo const resume = request.payload.resume const ip = request.app.clientAddress + const isUnverifiedAccount = ! accountRecord.primaryEmail.isVerified let sessions @@ -255,7 +256,7 @@ module.exports = (log, config, customs, db, mailer) => { function sendEmail() { // For unverified accounts, we always re-send the account verification email. - if (! accountRecord.primaryEmail.isVerified) { + if (isUnverifiedAccount) { return sendVerifyAccountEmail() } // If the session needs to be verified, send the sign-in confirmation email. @@ -348,7 +349,7 @@ module.exports = (log, config, customs, db, mailer) => { .catch(function (err) { log.error({op: 'mailer.confirmation.error', err: err}) - throw error.cannotSendEmail() + throw error.cannotSendEmail(isUnverifiedAccount) }) } diff --git a/test/local/routes/account.js b/test/local/routes/account.js index 92c6a81b6..7f7d31157 100644 --- a/test/local/routes/account.js +++ b/test/local/routes/account.js @@ -533,9 +533,9 @@ describe('/account/create', () => { return runTest(route, mockRequest).then(assert.fail, (err) => { assert.equal(err.message, 'Failed to send email') - assert.equal(err.output.payload.code, 500) + assert.equal(err.output.payload.code, 422) assert.equal(err.output.payload.errno, 151) - assert.equal(err.output.payload.error, 'Internal Server Error') + assert.equal(err.output.payload.error, 'Unprocessable Entity') }) }) }) diff --git a/test/local/routes/emails.js b/test/local/routes/emails.js index 6c8fb6e1e..e263377d9 100644 --- a/test/local/routes/emails.js +++ b/test/local/routes/emails.js @@ -820,6 +820,7 @@ describe('/recovery_email', () => { }) .catch((err) => { assert.equal(err.errno, 151, 'failed to send email error') + assert.equal(err.output.payload.code, 422) assert.equal(mockDB.createEmail.callCount, 1, 'call db.createEmail') assert.equal(mockDB.deleteEmail.callCount, 1, 'call db.deleteEmail') assert.equal(mockDB.deleteEmail.args[0][0], mockRequest.auth.credentials.uid, 'correct uid passed')