From 0ea8cec120864515bf7d9cd6a2dd9234efc79fc0 Mon Sep 17 00:00:00 2001
From: Michael Leanos <michael.leanos@gmail.com>
Date: Mon, 10 Oct 2016 16:00:24 -0700
Subject: [PATCH] fix(express): Incorrest uses of 400 error codes (#1553)

Fixes incorrest usage of 400 HTTP responses being returned from the
server, in favor of using 422.

Also, changed a few return codes to 401 where it was more appropriate.

See this article for reasoning behind moving to 422, and why 400 isn't
appropriate for these cases.

For ref:
https://github.com/meanjs/mean/commit/6be12f8a061a2f7228e254c314a46ad7a78966d1

Related:
https://github.com/meanjs/mean/pull/1547
https://github.com/meanjs/mean/pull/1510
---
 .../controllers/articles.server.controller.js |  8 +++----
 .../admin.article.server.routes.tests.js      |  2 +-
 .../controllers/admin.server.controller.js    |  6 ++---
 .../users.authentication.server.controller.js |  2 +-
 .../users/users.password.server.controller.js | 16 +++++++-------
 .../users/users.profile.server.controller.js  | 10 ++++-----
 .../tests/server/user.server.routes.tests.js  | 22 +++++++++----------
 7 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/modules/articles/server/controllers/articles.server.controller.js b/modules/articles/server/controllers/articles.server.controller.js
index 18d6a3b959..ddba5fe192 100644
--- a/modules/articles/server/controllers/articles.server.controller.js
+++ b/modules/articles/server/controllers/articles.server.controller.js
@@ -17,7 +17,7 @@ exports.create = function (req, res) {
 
   article.save(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     } else {
@@ -51,7 +51,7 @@ exports.update = function (req, res) {
 
   article.save(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     } else {
@@ -68,7 +68,7 @@ exports.delete = function (req, res) {
 
   article.remove(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     } else {
@@ -83,7 +83,7 @@ exports.delete = function (req, res) {
 exports.list = function (req, res) {
   Article.find().sort('-created').populate('user', 'displayName').exec(function (err, articles) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     } else {
diff --git a/modules/articles/tests/server/admin.article.server.routes.tests.js b/modules/articles/tests/server/admin.article.server.routes.tests.js
index e56119da9e..0322dd14bc 100644
--- a/modules/articles/tests/server/admin.article.server.routes.tests.js
+++ b/modules/articles/tests/server/admin.article.server.routes.tests.js
@@ -170,7 +170,7 @@ describe('Article Admin CRUD tests', function () {
         // Save a new article
         agent.post('/api/articles')
           .send(article)
-          .expect(400)
+          .expect(422)
           .end(function (articleSaveErr, articleSaveRes) {
             // Set message assertion
             (articleSaveRes.body.message).should.match('Title cannot be blank');
diff --git a/modules/users/server/controllers/admin.server.controller.js b/modules/users/server/controllers/admin.server.controller.js
index 3747a06e68..20fc9c72cb 100644
--- a/modules/users/server/controllers/admin.server.controller.js
+++ b/modules/users/server/controllers/admin.server.controller.js
@@ -29,7 +29,7 @@ exports.update = function (req, res) {
 
   user.save(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     }
@@ -46,7 +46,7 @@ exports.delete = function (req, res) {
 
   user.remove(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     }
@@ -61,7 +61,7 @@ exports.delete = function (req, res) {
 exports.list = function (req, res) {
   User.find({}, '-salt -password -providerData').sort('-created').populate('user', 'displayName').exec(function (err, users) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     }
diff --git a/modules/users/server/controllers/users/users.authentication.server.controller.js b/modules/users/server/controllers/users/users.authentication.server.controller.js
index 2089385400..7d96002b08 100644
--- a/modules/users/server/controllers/users/users.authentication.server.controller.js
+++ b/modules/users/server/controllers/users/users.authentication.server.controller.js
@@ -231,7 +231,7 @@ exports.removeOAuthProvider = function (req, res, next) {
 
   user.save(function (err) {
     if (err) {
-      return res.status(400).send({
+      return res.status(422).send({
         message: errorHandler.getErrorMessage(err)
       });
     } else {
diff --git a/modules/users/server/controllers/users/users.password.server.controller.js b/modules/users/server/controllers/users/users.password.server.controller.js
index 77a04dbd27..7f3153ba1f 100644
--- a/modules/users/server/controllers/users/users.password.server.controller.js
+++ b/modules/users/server/controllers/users/users.password.server.controller.js
@@ -50,7 +50,7 @@ exports.forgot = function (req, res, next) {
           }
         });
       } else {
-        return res.status(400).send({
+        return res.status(422).send({
           message: 'Username field must not be blank'
         });
       }
@@ -141,7 +141,7 @@ exports.reset = function (req, res, next) {
 
             user.save(function (err) {
               if (err) {
-                return res.status(400).send({
+                return res.status(422).send({
                   message: errorHandler.getErrorMessage(err)
                 });
               } else {
@@ -161,7 +161,7 @@ exports.reset = function (req, res, next) {
               }
             });
           } else {
-            return res.status(400).send({
+            return res.status(422).send({
               message: 'Passwords do not match'
             });
           }
@@ -217,7 +217,7 @@ exports.changePassword = function (req, res, next) {
 
               user.save(function (err) {
                 if (err) {
-                  return res.status(400).send({
+                  return res.status(422).send({
                     message: errorHandler.getErrorMessage(err)
                   });
                 } else {
@@ -233,12 +233,12 @@ exports.changePassword = function (req, res, next) {
                 }
               });
             } else {
-              res.status(400).send({
+              res.status(422).send({
                 message: 'Passwords do not match'
               });
             }
           } else {
-            res.status(400).send({
+            res.status(422).send({
               message: 'Current password is incorrect'
             });
           }
@@ -249,12 +249,12 @@ exports.changePassword = function (req, res, next) {
         }
       });
     } else {
-      res.status(400).send({
+      res.status(422).send({
         message: 'Please provide a new password'
       });
     }
   } else {
-    res.status(400).send({
+    res.status(401).send({
       message: 'User is not signed in'
     });
   }
diff --git a/modules/users/server/controllers/users/users.profile.server.controller.js b/modules/users/server/controllers/users/users.profile.server.controller.js
index 99f8e7bc99..68e992d780 100644
--- a/modules/users/server/controllers/users/users.profile.server.controller.js
+++ b/modules/users/server/controllers/users/users.profile.server.controller.js
@@ -31,7 +31,7 @@ exports.update = function (req, res) {
 
     user.save(function (err) {
       if (err) {
-        return res.status(400).send({
+        return res.status(422).send({
           message: errorHandler.getErrorMessage(err)
         });
       } else {
@@ -45,7 +45,7 @@ exports.update = function (req, res) {
       }
     });
   } else {
-    res.status(400).send({
+    res.status(401).send({
       message: 'User is not signed in'
     });
   }
@@ -73,10 +73,10 @@ exports.changeProfilePicture = function (req, res) {
         res.json(user);
       })
       .catch(function (err) {
-        res.status(400).send(err);
+        res.status(422).send(err);
       });
   } else {
-    res.status(400).send({
+    res.status(401).send({
       message: 'User is not signed in'
     });
   }
@@ -129,7 +129,7 @@ exports.changeProfilePicture = function (req, res) {
     return new Promise(function (resolve, reject) {
       req.login(user, function (err) {
         if (err) {
-          reject(err);
+          res.status(400).send(err);
         } else {
           resolve();
         }
diff --git a/modules/users/tests/server/user.server.routes.tests.js b/modules/users/tests/server/user.server.routes.tests.js
index 8e6f075e58..9288a53488 100644
--- a/modules/users/tests/server/user.server.routes.tests.js
+++ b/modules/users/tests/server/user.server.routes.tests.js
@@ -328,7 +328,7 @@ describe('User CRUD tests', function () {
         .send({
           username: ''
         })
-        .expect(400)
+        .expect(422)
         .end(function (err, res) {
           // Handle error
           if (err) {
@@ -507,7 +507,7 @@ describe('User CRUD tests', function () {
             verifyPassword: '1234567890-ABC-123-Aa$',
             currentPassword: credentials.password
           })
-          .expect(400)
+          .expect(422)
           .end(function (err, res) {
             if (err) {
               return done(err);
@@ -536,7 +536,7 @@ describe('User CRUD tests', function () {
             verifyPassword: '1234567890Aa$',
             currentPassword: 'some_wrong_passwordAa$'
           })
-          .expect(400)
+          .expect(422)
           .end(function (err, res) {
             if (err) {
               return done(err);
@@ -565,7 +565,7 @@ describe('User CRUD tests', function () {
             verifyPassword: '',
             currentPassword: credentials.password
           })
-          .expect(400)
+          .expect(422)
           .end(function (err, res) {
             if (err) {
               return done(err);
@@ -577,7 +577,7 @@ describe('User CRUD tests', function () {
       });
   });
 
-  it('should not be able to change user own password if no new password is at all given', function (done) {
+  it('should not be able to change user own password if not signed in', function (done) {
 
     // Change password
     agent.post('/api/users/password')
@@ -586,7 +586,7 @@ describe('User CRUD tests', function () {
         verifyPassword: '1234567890Aa$',
         currentPassword: credentials.password
       })
-      .expect(400)
+      .expect(401)
       .end(function (err, res) {
         if (err) {
           return done(err);
@@ -759,7 +759,7 @@ describe('User CRUD tests', function () {
 
           agent.put('/api/users')
             .send(userUpdate)
-            .expect(400)
+            .expect(422)
             .end(function (userInfoErr, userInfoRes) {
               if (userInfoErr) {
                 return done(userInfoErr);
@@ -811,7 +811,7 @@ describe('User CRUD tests', function () {
 
           agent.put('/api/users')
             .send(userUpdate)
-            .expect(400)
+            .expect(422)
             .end(function (userInfoErr, userInfoRes) {
               if (userInfoErr) {
                 return done(userInfoErr);
@@ -888,7 +888,7 @@ describe('User CRUD tests', function () {
 
       agent.put('/api/users')
         .send(userUpdate)
-        .expect(400)
+        .expect(401)
         .end(function (userInfoErr, userInfoRes) {
           if (userInfoErr) {
             return done(userInfoErr);
@@ -906,7 +906,7 @@ describe('User CRUD tests', function () {
 
     agent.post('/api/users/picture')
       .send({})
-      .expect(400)
+      .expect(401)
       .end(function (userInfoErr, userInfoRes) {
         if (userInfoErr) {
           return done(userInfoErr);
@@ -960,7 +960,7 @@ describe('User CRUD tests', function () {
         agent.post('/api/users/picture')
           .attach('fieldThatDoesntWork', './modules/users/client/img/profile/default.png')
           .send(credentials)
-          .expect(400)
+          .expect(422)
           .end(function (userInfoErr, userInfoRes) {
             done(userInfoErr);
           });