Subscribers: Error Handling for adding subscribers

no issue
- do not expose information about adding subscribers

core/server/api/subscribers.js

@@ -80,7 +80,7 @@ subscribers = {
                 return {subscribers: [result.toJSON(options)]};
             }
 
-            return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.subscriber.subscriberNotFound')));
+            return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.subscribers.subscriberNotFound')));
         });
     },
 
@@ -92,13 +92,6 @@ subscribers = {
     add: function add(object, options) {
         var tasks;
 
-        function cleanError(error) {
-            if (error.message.toLowerCase().indexOf('unique') !== -1) {
-                return new errors.DataImportError('Email already exists.');
-            }
-            return error;
-        }
-
         /**
          * ### Model Query
          * Make the call to the Model layer
@@ -106,13 +99,23 @@ subscribers = {
          * @returns {Object} options
          */
         function doQuery(options) {
-            return dataProvider.Subscriber.add(options.data.subscribers[0], _.omit(options, ['data'])).catch(function (error) {
-                if (error.code) {
-                    // DB error
-                    return Promise.reject(cleanError(error));
-                }
-                return Promise.reject(error[0]);
-            });
+            return dataProvider.Subscriber.getByEmail(options.data.subscribers[0].email)
+                .then(function (subscriber) {
+                    if (subscriber && options.context.external) {
+                        // we don't expose this information
+                        return Promise.resolve(subscriber);
+                    } else if (subscriber) {
+                        return Promise.reject(new errors.ValidationError(i18n.t('errors.api.subscribers.subscriberAlreadyExist')));
+                    }
+
+                    return dataProvider.Subscriber.add(options.data.subscribers[0], _.omit(options, ['data'])).catch(function (error) {
+                        if (error.code && error.message.toLowerCase().indexOf('unique') !== -1) {
+                            return Promise.reject(new errors.ValidationError(i18n.t('errors.api.subscribers.subscriberAlreadyExist')));
+                        }
+
+                        return Promise.reject(error);
+                    });
+                });
         }
 
         // Push all of our tasks into a `tasks` array in the correct order
@@ -125,7 +128,6 @@ subscribers = {
         // Pipeline calls each task passing the result of one to be the arguments for the next
         return pipeline(tasks, object, options).then(function formatResponse(result) {
             var subscriber = result.toJSON(options);
-
             return {subscribers: [subscriber]};
         });
     },
@@ -165,7 +167,7 @@ subscribers = {
                 return {subscribers: [subscriber]};
             }
 
-            return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.subscriber.subscriberNotFound')));
+            return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.subscribers.subscriberNotFound')));
         });
     },
 
@@ -297,7 +299,7 @@ subscribers = {
                     if (inspection.isFulfilled()) {
                         fulfilled = fulfilled + 1;
                     } else {
-                        if (inspection.reason() instanceof errors.DataImportError) {
+                        if (inspection.reason() instanceof errors.ValidationError) {
                             duplicates = duplicates + 1;
                         } else {
                             invalid = invalid + 1;

core/server/apps/subscribers/lib/router.js

@@ -1,5 +1,6 @@
 var path                = require('path'),
     express             = require('express'),
+    _                   = require('lodash'),
     subscribeRouter     = express.Router(),
 
     // Dirty requires
@@ -69,13 +70,19 @@ function handleSource(req, res, next) {
 function storeSubscriber(req, res, next) {
     req.body.status = 'subscribed';
 
+    if (_.isEmpty(req.body.email)) {
+        return next(new errors.ValidationError('Email cannot be blank.'));
+    }
+
     return api.subscribers.add({subscribers: [req.body]}, {context: {external: true}})
         .then(function () {
             res.locals.success = true;
             next();
         })
-        .catch(function (error) {
-            next(error);
+        .catch(function () {
+            // we do not expose any information
+            res.locals.success = true;
+            next();
         });
 }
 

core/server/errors/index.js

@@ -152,6 +152,7 @@ errors = {
             context = i18n.t('errors.errors.databaseIsReadOnly');
             help = i18n.t('errors.errors.checkDatabase');
         }
+
         // TODO: Logging framework hookup
         // Eventually we'll have better logging which will know about envs
         if ((process.env.NODE_ENV === 'development' ||

core/server/models/subscriber.js

@@ -73,6 +73,22 @@ Subscriber = ghostBookshelf.Model.extend({
         }
 
         return Promise.reject(new errors.NoPermissionError(i18n.t('errors.models.subscriber.notEnoughPermission')));
+    },
+
+    // TODO: This is a copy paste of models/user.js!
+    getByEmail: function getByEmail(email, options) {
+        options = options || {};
+        options.require = true;
+
+        return Subscribers.forge(options).fetch(options).then(function then(subscribers) {
+            var subscriberWithEmail = subscribers.find(function findSubscriber(subscriber) {
+                return subscriber.get('email').toLowerCase() === email.toLowerCase();
+            });
+
+            if (subscriberWithEmail) {
+                return subscriberWithEmail;
+            }
+        });
     }
 });
 

core/server/translations/en.json

@@ -336,8 +336,9 @@
             "tags": {
                 "tagNotFound": "Tag not found."
             },
-            "subscriber": {
-                "subscriberNotFound": "Subscriber not found."
+            "subscribers": {
+                "subscriberNotFound": "Subscriber not found.",
+                "subscriberAlreadyExist": "Email already exist."
             },
             "themes": {
                 "noPermissionToBrowseThemes": "You do not have permission to browse themes.",

core/test/integration/api/api_subscription_spec.js

@@ -67,6 +67,18 @@ describe('Subscribers API', function () {
                 }).catch(done);
         });
 
+        it('duplicate subscriber', function (done) {
+            SubscribersAPI.add({subscribers: [newSubscriber]}, testUtils.context.external)
+                .then(function () {
+                    SubscribersAPI.add({subscribers: [newSubscriber]}, testUtils.context.external)
+                        .then(function () {
+                            return done();
+                        })
+                        .catch(done);
+                })
+                .catch(done);
+        });
+
         it('CANNOT add subscriber without context', function (done) {
             SubscribersAPI.add({subscribers: [newSubscriber]})
                 .then(function () {