Skip to content

Commit 61021e4

Browse files
author
welcome-dev
committed
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: meanjs/mean@6be12f8 Related: meanjs/mean#1547 meanjs/mean#1510
1 parent 8124f52 commit 61021e4

7 files changed

+33
-33
lines changed

‎modules/articles/server/controllers/articles.server.controller.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ exports.create = function (req, res) {
1717

1818
article.save(function (err) {
1919
if (err) {
20-
return res.status(400).send({
20+
return res.status(422).send({
2121
message: errorHandler.getErrorMessage(err)
2222
});
2323
} else {
@@ -51,7 +51,7 @@ exports.update = function (req, res) {
5151

5252
article.save(function (err) {
5353
if (err) {
54-
return res.status(400).send({
54+
return res.status(422).send({
5555
message: errorHandler.getErrorMessage(err)
5656
});
5757
} else {
@@ -68,7 +68,7 @@ exports.delete = function (req, res) {
6868

6969
article.remove(function (err) {
7070
if (err) {
71-
return res.status(400).send({
71+
return res.status(422).send({
7272
message: errorHandler.getErrorMessage(err)
7373
});
7474
} else {
@@ -83,7 +83,7 @@ exports.delete = function (req, res) {
8383
exports.list = function (req, res) {
8484
Article.find().sort('-created').populate('user', 'displayName').exec(function (err, articles) {
8585
if (err) {
86-
return res.status(400).send({
86+
return res.status(422).send({
8787
message: errorHandler.getErrorMessage(err)
8888
});
8989
} else {

‎modules/articles/tests/server/admin.article.server.routes.tests.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ describe('Article Admin CRUD tests', function () {
170170
// Save a new article
171171
agent.post('/api/articles')
172172
.send(article)
173-
.expect(400)
173+
.expect(422)
174174
.end(function (articleSaveErr, articleSaveRes) {
175175
// Set message assertion
176176
(articleSaveRes.body.message).should.match('Title cannot be blank');

‎modules/users/server/controllers/admin.server.controller.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ exports.update = function (req, res) {
2929

3030
user.save(function (err) {
3131
if (err) {
32-
return res.status(400).send({
32+
return res.status(422).send({
3333
message: errorHandler.getErrorMessage(err)
3434
});
3535
}
@@ -46,7 +46,7 @@ exports.delete = function (req, res) {
4646

4747
user.remove(function (err) {
4848
if (err) {
49-
return res.status(400).send({
49+
return res.status(422).send({
5050
message: errorHandler.getErrorMessage(err)
5151
});
5252
}
@@ -61,7 +61,7 @@ exports.delete = function (req, res) {
6161
exports.list = function (req, res) {
6262
User.find({}, '-salt -password -providerData').sort('-created').populate('user', 'displayName').exec(function (err, users) {
6363
if (err) {
64-
return res.status(400).send({
64+
return res.status(422).send({
6565
message: errorHandler.getErrorMessage(err)
6666
});
6767
}

‎modules/users/server/controllers/users/users.authentication.server.controller.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ exports.removeOAuthProvider = function (req, res, next) {
231231

232232
user.save(function (err) {
233233
if (err) {
234-
return res.status(400).send({
234+
return res.status(422).send({
235235
message: errorHandler.getErrorMessage(err)
236236
});
237237
} else {

‎modules/users/server/controllers/users/users.password.server.controller.js

+8-8
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ exports.forgot = function (req, res, next) {
5050
}
5151
});
5252
} else {
53-
return res.status(400).send({
53+
return res.status(422).send({
5454
message: 'Username field must not be blank'
5555
});
5656
}
@@ -141,7 +141,7 @@ exports.reset = function (req, res, next) {
141141

142142
user.save(function (err) {
143143
if (err) {
144-
return res.status(400).send({
144+
return res.status(422).send({
145145
message: errorHandler.getErrorMessage(err)
146146
});
147147
} else {
@@ -161,7 +161,7 @@ exports.reset = function (req, res, next) {
161161
}
162162
});
163163
} else {
164-
return res.status(400).send({
164+
return res.status(422).send({
165165
message: 'Passwords do not match'
166166
});
167167
}
@@ -217,7 +217,7 @@ exports.changePassword = function (req, res, next) {
217217

218218
user.save(function (err) {
219219
if (err) {
220-
return res.status(400).send({
220+
return res.status(422).send({
221221
message: errorHandler.getErrorMessage(err)
222222
});
223223
} else {
@@ -233,12 +233,12 @@ exports.changePassword = function (req, res, next) {
233233
}
234234
});
235235
} else {
236-
res.status(400).send({
236+
res.status(422).send({
237237
message: 'Passwords do not match'
238238
});
239239
}
240240
} else {
241-
res.status(400).send({
241+
res.status(422).send({
242242
message: 'Current password is incorrect'
243243
});
244244
}
@@ -249,12 +249,12 @@ exports.changePassword = function (req, res, next) {
249249
}
250250
});
251251
} else {
252-
res.status(400).send({
252+
res.status(422).send({
253253
message: 'Please provide a new password'
254254
});
255255
}
256256
} else {
257-
res.status(400).send({
257+
res.status(401).send({
258258
message: 'User is not signed in'
259259
});
260260
}

‎modules/users/server/controllers/users/users.profile.server.controller.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ exports.update = function (req, res) {
3131

3232
user.save(function (err) {
3333
if (err) {
34-
return res.status(400).send({
34+
return res.status(422).send({
3535
message: errorHandler.getErrorMessage(err)
3636
});
3737
} else {
@@ -45,7 +45,7 @@ exports.update = function (req, res) {
4545
}
4646
});
4747
} else {
48-
res.status(400).send({
48+
res.status(401).send({
4949
message: 'User is not signed in'
5050
});
5151
}
@@ -73,10 +73,10 @@ exports.changeProfilePicture = function (req, res) {
7373
res.json(user);
7474
})
7575
.catch(function (err) {
76-
res.status(400).send(err);
76+
res.status(422).send(err);
7777
});
7878
} else {
79-
res.status(400).send({
79+
res.status(401).send({
8080
message: 'User is not signed in'
8181
});
8282
}
@@ -129,7 +129,7 @@ exports.changeProfilePicture = function (req, res) {
129129
return new Promise(function (resolve, reject) {
130130
req.login(user, function (err) {
131131
if (err) {
132-
reject(err);
132+
res.status(400).send(err);
133133
} else {
134134
resolve();
135135
}

‎modules/users/tests/server/user.server.routes.tests.js

+11-11
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ describe('User CRUD tests', function () {
328328
.send({
329329
username: ''
330330
})
331-
.expect(400)
331+
.expect(422)
332332
.end(function (err, res) {
333333
// Handle error
334334
if (err) {
@@ -507,7 +507,7 @@ describe('User CRUD tests', function () {
507507
verifyPassword: '1234567890-ABC-123-Aa$',
508508
currentPassword: credentials.password
509509
})
510-
.expect(400)
510+
.expect(422)
511511
.end(function (err, res) {
512512
if (err) {
513513
return done(err);
@@ -536,7 +536,7 @@ describe('User CRUD tests', function () {
536536
verifyPassword: '1234567890Aa$',
537537
currentPassword: 'some_wrong_passwordAa$'
538538
})
539-
.expect(400)
539+
.expect(422)
540540
.end(function (err, res) {
541541
if (err) {
542542
return done(err);
@@ -565,7 +565,7 @@ describe('User CRUD tests', function () {
565565
verifyPassword: '',
566566
currentPassword: credentials.password
567567
})
568-
.expect(400)
568+
.expect(422)
569569
.end(function (err, res) {
570570
if (err) {
571571
return done(err);
@@ -577,7 +577,7 @@ describe('User CRUD tests', function () {
577577
});
578578
});
579579

580-
it('should not be able to change user own password if no new password is at all given', function (done) {
580+
it('should not be able to change user own password if not signed in', function (done) {
581581

582582
// Change password
583583
agent.post('/api/users/password')
@@ -586,7 +586,7 @@ describe('User CRUD tests', function () {
586586
verifyPassword: '1234567890Aa$',
587587
currentPassword: credentials.password
588588
})
589-
.expect(400)
589+
.expect(401)
590590
.end(function (err, res) {
591591
if (err) {
592592
return done(err);
@@ -759,7 +759,7 @@ describe('User CRUD tests', function () {
759759

760760
agent.put('/api/users')
761761
.send(userUpdate)
762-
.expect(400)
762+
.expect(422)
763763
.end(function (userInfoErr, userInfoRes) {
764764
if (userInfoErr) {
765765
return done(userInfoErr);
@@ -811,7 +811,7 @@ describe('User CRUD tests', function () {
811811

812812
agent.put('/api/users')
813813
.send(userUpdate)
814-
.expect(400)
814+
.expect(422)
815815
.end(function (userInfoErr, userInfoRes) {
816816
if (userInfoErr) {
817817
return done(userInfoErr);
@@ -888,7 +888,7 @@ describe('User CRUD tests', function () {
888888

889889
agent.put('/api/users')
890890
.send(userUpdate)
891-
.expect(400)
891+
.expect(401)
892892
.end(function (userInfoErr, userInfoRes) {
893893
if (userInfoErr) {
894894
return done(userInfoErr);
@@ -906,7 +906,7 @@ describe('User CRUD tests', function () {
906906

907907
agent.post('/api/users/picture')
908908
.send({})
909-
.expect(400)
909+
.expect(401)
910910
.end(function (userInfoErr, userInfoRes) {
911911
if (userInfoErr) {
912912
return done(userInfoErr);
@@ -960,7 +960,7 @@ describe('User CRUD tests', function () {
960960
agent.post('/api/users/picture')
961961
.attach('fieldThatDoesntWork', './modules/users/client/img/profile/default.png')
962962
.send(credentials)
963-
.expect(400)
963+
.expect(422)
964964
.end(function (userInfoErr, userInfoRes) {
965965
done(userInfoErr);
966966
});

0 commit comments

Comments
 (0)