From e28047db1bba17a5d82448c09030258aad859d86 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Wed, 17 Dec 2014 13:26:45 -0300 Subject: [PATCH 1/4] remove profile on email send fail --- plugins/emailstore.js | 57 +++++++++++++++++++++++++---------------- test/test.EmailStore.js | 46 ++++++++++++++++++++++++++++++--- 2 files changed, 78 insertions(+), 25 deletions(-) diff --git a/plugins/emailstore.js b/plugins/emailstore.js index 0973081..a8f5534 100644 --- a/plugins/emailstore.js +++ b/plugins/emailstore.js @@ -52,6 +52,10 @@ code: 406, message: 'User quota exceeded', }, + COULD_NOT_CREATE: { + code: 500, + message: 'Could not create profile', + }, }; var EMAIL_TO_PASSPHRASE = 'email-to-passphrase-'; @@ -141,7 +145,7 @@ * @param {string} email - the user's email * @param {string} secret - the verification secret */ - emailPlugin.sendVerificationEmail = function(email, secret) { + emailPlugin.sendVerificationEmail = function(email, secret, callback) { var confirmUrl = emailPlugin.makeConfirmUrl(email, secret); logger.debug('ConfirmUrl:',confirmUrl); @@ -176,9 +180,10 @@ emailPlugin.email.sendMail(mailOptions, function(err, info) { if (err) { logger.error('An error occurred when trying to send email to ' + email, err); - } else { - logger.info('Message sent: ', info ? info : ''); + return callback(err); } + logger.info('Message sent: ', info ? info : ''); + return callback(err, info); }); }); }; @@ -351,7 +356,12 @@ return callback(emailPlugin.errors.INTERNAL_ERROR); } if (secret) { - emailPlugin.sendVerificationEmail(email, secret); + emailPlugin.sendVerificationEmail(email, secret, function (err, res) { + if (err) { + logger.error('error sending verification email', email, secret, err); + return callback(emailPlugin.errors.INTERNAL_ERROR); + } + }); } callback(); }); @@ -478,7 +488,9 @@ }; emailPlugin.processPost = function(request, response, email, key, passphrase, record) { + var isNewProfile = true; var isConfirmed = false; + var errorCreating = false; async.series([ /** @@ -486,18 +498,14 @@ */ function(callback) { emailPlugin.exists(email, function(err, exists) { - if (err) { - return callback(err); - } else if (exists) { + if (err) return callback(err); + + if (exists) { emailPlugin.checkPassphrase(email, passphrase, function(err, match) { - if (err) { - return callback(err); - } - if (match) { - return callback(); - } else { - return callback(emailPlugin.errors.EMAIL_TAKEN); - } + if (err) return callback(err); + if (!match) return callback(emailPlugin.errors.EMAIL_TAKEN); + isNewProfile = false; + return callback(); }); } else { emailPlugin.savePassphrase(email, passphrase, function(err) { @@ -538,20 +546,25 @@ * Create and store the verification secret. If successful, send a verification email. */ function(callback) { + if (!isNewProfile || isConfirmed) return callback(); + emailPlugin.createVerificationSecretAndSendEmail(email, function(err) { if (err) { - callback({ - code: 500, - message: err - }); - } else { - callback(); + errorCreating = true; + return callback(err); } + return callback(); }); - } + }, ], function(err) { if (err) { + if (isNewProfile && !isConfirmed && errorCreating) { + emailPlugin.deleteWholeProfile(email, function(err) { + return emailPlugin.returnError(emailPlugin.errors.COULD_NOT_CREATE, response); + }); + } + emailPlugin.returnError(err, response); } else { response.json({ diff --git a/test/test.EmailStore.js b/test/test.EmailStore.js index 759b9f7..a5253ae 100644 --- a/test/test.EmailStore.js +++ b/test/test.EmailStore.js @@ -268,7 +268,6 @@ describe('emailstore test', function() { }); describe('on registration', function() { - var emailParam = 'email'; var secretParam = 'secret'; var keyParam = 'key'; @@ -336,7 +335,6 @@ describe('emailstore test', function() { plugin.saveEncryptedData = sinon.stub(); plugin.saveEncryptedData.onFirstCall().callsArg(3); plugin.createVerificationSecretAndSendEmail = sinon.stub(); - plugin.createVerificationSecretAndSendEmail.onFirstCall().callsArg(1); response.send.onFirstCall().returnsThis(); plugin.save(request, response); @@ -347,9 +345,51 @@ describe('emailstore test', function() { assert(plugin.saveEncryptedData.firstCall.args[0] === emailParam); assert(plugin.saveEncryptedData.firstCall.args[1] === keyParam); assert(plugin.saveEncryptedData.firstCall.args[2] === recordParam); - assert(plugin.createVerificationSecretAndSendEmail.firstCall.args[0] === emailParam); + plugin.createVerificationSecretAndSendEmail.called.should.be.false; plugin.getCredentialsFromRequest = originalCredentials; }); + + it('should delete profile on error sending verification email', function() { + var originalCredentials = plugin.getCredentialsFromRequest; + plugin.getCredentialsFromRequest = sinon.mock(); + plugin.getCredentialsFromRequest.onFirstCall().returns({ + email: emailParam, + passphrase: secretParam + }); + plugin.exists = sinon.stub(); + plugin.exists.onFirstCall().callsArgWith(1, null, false); + plugin.savePassphrase = sinon.stub(); + plugin.savePassphrase.onFirstCall().callsArg(2); + plugin.isConfirmed = sinon.stub(); + plugin.isConfirmed.onFirstCall().callsArgWith(1, null, false); + plugin.checkSizeQuota = sinon.stub(); + plugin.checkSizeQuota.onFirstCall().callsArgWith(3, null); + plugin.checkAndUpdateItemQuota = sinon.stub(); + plugin.checkAndUpdateItemQuota.onFirstCall().callsArgWith(3, null); + plugin.saveEncryptedData = sinon.stub(); + plugin.saveEncryptedData.onFirstCall().callsArg(3); + plugin.createVerificationSecretAndSendEmail = sinon.stub(); + plugin.createVerificationSecretAndSendEmail.onFirstCall().callsArgWith(1, 'error'); + var deleteWholeProfile = sinon.stub(plugin, 'deleteWholeProfile'); + deleteWholeProfile.onFirstCall().callsArg(1); + response.send.onFirstCall().returnsThis(); + + plugin.save(request, response); + + assert(plugin.exists.firstCall.args[0] === emailParam); + assert(plugin.savePassphrase.firstCall.args[0] === emailParam); + assert(plugin.savePassphrase.firstCall.args[1] === secretParam); + assert(plugin.saveEncryptedData.firstCall.args[0] === emailParam); + assert(plugin.saveEncryptedData.firstCall.args[1] === keyParam); + assert(plugin.saveEncryptedData.firstCall.args[2] === recordParam); + assert(plugin.createVerificationSecretAndSendEmail.firstCall.args[0] === emailParam); + assert(deleteWholeProfile.firstCall.args[0] === emailParam); + plugin.getCredentialsFromRequest = originalCredentials; + }); + + after(function () { + plugin.deleteWholeProfile.restore(); + }); }); describe('when validating email', function() { From 2f90d7bf3069e783c59776af542206b0ca358835 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Wed, 17 Dec 2014 16:33:16 -0300 Subject: [PATCH 2/4] special message for error sending email --- plugins/emailstore.js | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/plugins/emailstore.js b/plugins/emailstore.js index a8f5534..1b6f51d 100644 --- a/plugins/emailstore.js +++ b/plugins/emailstore.js @@ -52,9 +52,9 @@ code: 406, message: 'User quota exceeded', }, - COULD_NOT_CREATE: { - code: 500, - message: 'Could not create profile', + ERROR_SENDING_EMAIL: { + code: 501, + message: 'Could not send verification email', }, }; @@ -359,11 +359,11 @@ emailPlugin.sendVerificationEmail(email, secret, function (err, res) { if (err) { logger.error('error sending verification email', email, secret, err); - return callback(emailPlugin.errors.INTERNAL_ERROR); + return callback(emailPlugin.errors.ERROR_SENDING_EMAIL); } + return callback(); }); } - callback(); }); }; @@ -536,10 +536,7 @@ */ function(callback) { emailPlugin.saveEncryptedData(email, key, record, function(err) { - if (err) { - return callback(err); - } - return callback(); + return callback(err); }); }, /** @@ -551,17 +548,16 @@ emailPlugin.createVerificationSecretAndSendEmail(email, function(err) { if (err) { errorCreating = true; - return callback(err); } - return callback(); + return callback(err); }); }, ], function(err) { if (err) { if (isNewProfile && !isConfirmed && errorCreating) { - emailPlugin.deleteWholeProfile(email, function(err) { - return emailPlugin.returnError(emailPlugin.errors.COULD_NOT_CREATE, response); + emailPlugin.deleteWholeProfile(email, function() { + return emailPlugin.returnError(err, response); }); } From c61b8f44488d6e0fe37ccabb5dfd2c56561a4f79 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 18 Dec 2014 11:23:47 -0300 Subject: [PATCH 3/4] changed default values for isNewProfile & isConfirmed --- plugins/emailstore.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/emailstore.js b/plugins/emailstore.js index 1b6f51d..3620f71 100644 --- a/plugins/emailstore.js +++ b/plugins/emailstore.js @@ -488,8 +488,8 @@ }; emailPlugin.processPost = function(request, response, email, key, passphrase, record) { - var isNewProfile = true; - var isConfirmed = false; + var isNewProfile = false; + var isConfirmed = true; var errorCreating = false; async.series([ @@ -504,10 +504,10 @@ emailPlugin.checkPassphrase(email, passphrase, function(err, match) { if (err) return callback(err); if (!match) return callback(emailPlugin.errors.EMAIL_TAKEN); - isNewProfile = false; return callback(); }); } else { + isNewProfile = true; emailPlugin.savePassphrase(email, passphrase, function(err) { return callback(err); }); From f7a682612aa59a25cd90efcbabd9ca66751975dc Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 18 Dec 2014 15:17:01 -0300 Subject: [PATCH 4/4] improved tests --- plugins/emailstore.js | 18 ++++++++---------- test/test.EmailStore.js | 13 ++++--------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/plugins/emailstore.js b/plugins/emailstore.js index 3620f71..95d48b5 100644 --- a/plugins/emailstore.js +++ b/plugins/emailstore.js @@ -351,19 +351,17 @@ emailPlugin.createVerificationSecretAndSendEmail = function(email, callback) { emailPlugin.createVerificationSecret(email, function(err, secret) { - if (err) { + if (err || !secret) { logger.error('error saving verification secret', email, secret, err); return callback(emailPlugin.errors.INTERNAL_ERROR); } - if (secret) { - emailPlugin.sendVerificationEmail(email, secret, function (err, res) { - if (err) { - logger.error('error sending verification email', email, secret, err); - return callback(emailPlugin.errors.ERROR_SENDING_EMAIL); - } - return callback(); - }); - } + emailPlugin.sendVerificationEmail(email, secret, function (err, res) { + if (err) { + logger.error('error sending verification email', email, secret, err); + return callback(emailPlugin.errors.ERROR_SENDING_EMAIL); + } + return callback(); + }); }); }; diff --git a/test/test.EmailStore.js b/test/test.EmailStore.js index a5253ae..5dded7f 100644 --- a/test/test.EmailStore.js +++ b/test/test.EmailStore.js @@ -22,7 +22,7 @@ describe('emailstore test', function() { leveldb_stub.get = sinon.stub(); leveldb_stub.del = sinon.stub(); var email_stub = sinon.stub(); - email_stub.sendMail = sinon.stub(); + email_stub.sendMail = sinon.stub().callsArg(1); var cryptoMock = { randomBytes: sinon.stub() @@ -199,7 +199,6 @@ describe('emailstore test', function() { }); describe('creating verification secret', function() { - var sendVerificationEmail = sinon.stub(plugin, 'sendVerificationEmail'); var fakeEmail = 'fake@email.com'; var fakeRandom = 'fakerandom'; var randomBytes = { @@ -211,8 +210,8 @@ describe('emailstore test', function() { beforeEach(function() { leveldb_stub.get.reset(); leveldb_stub.put.reset(); + plugin.email.sendMail.reset(); - sendVerificationEmail.reset(); cryptoMock.randomBytes = sinon.stub(); cryptoMock.randomBytes.onFirstCall().returns(randomBytes); }); @@ -235,11 +234,11 @@ describe('emailstore test', function() { done(); }); }); - it('calls the function to verify the email', function(done) { + it('sends verification email', function(done) { setupLevelDb(); plugin.createVerificationSecretAndSendEmail(fakeEmail, function(err) { - sendVerificationEmail.calledOnce; + plugin.email.sendMail.calledOnce.should.be.true; done(); }); }); @@ -260,10 +259,6 @@ describe('emailstore test', function() { done(); }); }); - - after(function() { - plugin.sendVerificationEmail.restore(); - }); }); });