Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Commit

Permalink
fix(express): Incorrest uses of 400 error codes (#1553)
Browse files Browse the repository at this point in the history 
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:
6be12f8

Related:
#1547
#1510
  • Loading branch information
@mleanos
mleanos authored Oct 10, 2016
1 parent 607ed06 commit 0ea8cec
Show file tree
Hide file tree
Showing 7 changed files with 33 additions and 33 deletions.
8 changes: 4 additions & 4 deletions modules/articles/server/controllers/articles.server.controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion modules/articles/tests/server/admin.article.server.routes.tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
6 changes: 3 additions & 3 deletions modules/users/server/controllers/admin.server.controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
});
}
Expand All @@ -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)
});
}
Expand All @@ -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)
});
}
Expand Down
2 changes: 1 addition & 1 deletion modules/users/server/controllers/users/users.authentication.server.controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
16 changes: 8 additions & 8 deletions modules/users/server/controllers/users/users.password.server.controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
});
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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'
});
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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'
});
}
Expand All @@ -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'
});
}
Expand Down
10 changes: 5 additions & 5 deletions modules/users/server/controllers/users/users.profile.server.controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -45,7 +45,7 @@ exports.update = function (req, res) {
}
});
} else {
res.status(400).send({
res.status(401).send({
message: 'User is not signed in'
});
}
Expand Down Expand Up @@ -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'
});
}
Expand Down Expand Up @@ -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();
}
Expand Down
22 changes: 11 additions & 11 deletions modules/users/tests/server/user.server.routes.tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ describe('User CRUD tests', function () {
.send({
username: ''
})
.expect(400)
.expect(422)
.end(function (err, res) {
// Handle error
if (err) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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')
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
});
Expand Down

0 comments on commit 0ea8cec

Please sign in to comment.