diff --git a/imap-core/lib/commands/create.js b/imap-core/lib/commands/create.js index 057c9164..f04a2eb8 100644 --- a/imap-core/lib/commands/create.js +++ b/imap-core/lib/commands/create.js @@ -76,7 +76,7 @@ module.exports = { // do not return actual error to user return callback(null, { response: 'NO', - code: 'TEMPFAIL' + code: err.code || 'TEMPFAIL' }); } diff --git a/lib/api/mailboxes.js b/lib/api/mailboxes.js index 3aed88cc..f72310f2 100644 --- a/lib/api/mailboxes.js +++ b/lib/api/mailboxes.js @@ -296,24 +296,10 @@ module.exports = (db, server, mailboxHandler) => { } let status, id; - try { - let data = await createMailbox(user, path, opts); - status = data.status; - id = data.id; - } catch (err) { - res.status(500); // TODO: use response code specific status - return res.json({ - error: err.message, - code: err.code - }); - } - if (typeof status === 'string') { - res.status(500); - return res.json({ - error: 'Mailbox creation failed with code ' + status - }); - } + let data = await createMailbox(user, path, opts); + status = data.status; + id = data.id; return res.json({ success: !!status, @@ -326,7 +312,7 @@ module.exports = (db, server, mailboxHandler) => { '/users/:user/mailboxes/:mailbox', tools.responseWrapper(async (req, res) => { res.charSet('utf-8'); - const schema = Joi.object().keys({ + const schema = Joi.object({ user: Joi.string().hex().lowercase().length(24).required(), mailbox: Joi.string().hex().lowercase().length(24).allow('resolve').required(), path: Joi.string().regex(/\/{2,}|\/$/, { invert: true }), @@ -506,23 +492,7 @@ module.exports = (db, server, mailboxHandler) => { }); } - let status; - try { - status = await updateMailbox(user, mailbox, updates); - } catch (err) { - res.status(500); // TODO: use response code specific status - return res.json({ - error: err.message, - code: err.code - }); - } - - if (typeof status === 'string') { - res.status(500); - return res.json({ - error: 'Mailbox update failed with code ' + status - }); - } + await updateMailbox(user, mailbox, updates); return res.json({ success: true @@ -566,24 +536,7 @@ module.exports = (db, server, mailboxHandler) => { let user = new ObjectId(result.value.user); let mailbox = new ObjectId(result.value.mailbox); - let status; - try { - status = await deleteMailbox(user, mailbox); - } catch (err) { - res.status(500); // TODO: use response code specific status - return res.json({ - error: err.message, - code: err.code - }); - } - - if (typeof status === 'string') { - res.status(500); // TODO: use response code specific status - return res.json({ - error: 'Mailbox deletion failed with code ' + status, - code: status - }); - } + await deleteMailbox(user, mailbox); return res.json({ success: true diff --git a/lib/mailbox-handler.js b/lib/mailbox-handler.js index 998b0b54..da3b5b68 100644 --- a/lib/mailbox-handler.js +++ b/lib/mailbox-handler.js @@ -32,7 +32,10 @@ class MailboxHandler { return callback(err); } if (mailboxData) { - return callback(null, 'ALREADYEXISTS'); + const err = new Error('Mailbox creation failed with code MailboxAlreadyExists'); + err.code = 'ALREADYEXISTS'; + err.responseCode = 400; + return callback(err, 'ALREADYEXISTS'); } this.users.collection('users').findOne( @@ -50,7 +53,10 @@ class MailboxHandler { } if (!userData) { - return callback(new Error('User not found')); + const err = new Error('This user does not exist'); + err.code = 'UserNotFound'; + err.responseCode = 404; + return callback(err, 'UserNotFound'); } mailboxData = { @@ -74,7 +80,10 @@ class MailboxHandler { this.database.collection('mailboxes').insertOne(mailboxData, { writeConcern: 'majority' }, (err, r) => { if (err) { if (err.code === 11000) { - return callback(null, 'ALREADYEXISTS'); + const err = new Error('Mailbox creation failed with code MailboxAlreadyExists'); + err.code = 'ALREADYEXISTS'; + err.responseCode = 400; + return callback(err, 'ALREADYEXISTS'); } return callback(err); } @@ -116,10 +125,16 @@ class MailboxHandler { return callback(err); } if (!mailboxData) { - return callback(null, 'NONEXISTENT'); + const err = new Error('Mailbox update failed with code NoSuchMailbox'); + err.code = 'NONEXISTENT'; + err.responseCode = 404; + return callback(err, 'NONEXISTENT'); } if (mailboxData.path === 'INBOX' || mailboxData.hidden) { - return callback(null, 'CANNOT'); + const err = new Error('Mailbox update failed with code DisallowedMailboxMethod'); + err.code = 'CANNOT'; + err.responseCode = 400; + return callback(err, 'CANNOT'); } this.database.collection('mailboxes').findOne( { @@ -131,7 +146,10 @@ class MailboxHandler { return callback(err); } if (existing) { - return callback(null, 'ALREADYEXISTS'); + const err = new Error('Mailbox rename failed with code MailboxAlreadyExists'); + err.code = 'ALREADYEXISTS'; + err.responseCode = 400; + return callback(err, 'ALREADYEXISTS'); } let $set = { path: newname }; @@ -157,7 +175,10 @@ class MailboxHandler { if (!item || !item.value) { // was not able to acquire a lock - return callback(null, 'NONEXISTENT'); + const err = new Error('Mailbox update failed with code NoSuchMailbox'); + err.code = 'NONEXISTENT'; + err.responseCode = 404; + return callback(err, 'NONEXISTENT'); } publish(this.redis, { @@ -201,10 +222,16 @@ class MailboxHandler { return callback(err); } if (!mailboxData) { - return callback(null, 'NONEXISTENT'); + const err = new Error('Mailbox deletion failed with code NoSuchMailbox'); + err.code = 'NONEXISTENT'; + err.responseCode = 404; + return callback(err, 'NONEXISTENT'); } if (mailboxData.specialUse || mailboxData.path === 'INBOX' || mailboxData.hidden) { - return callback(null, 'CANNOT'); + const err = new Error('Mailbox deletion failed with code DisallowedMailboxMethod'); + err.code = 'CANNOT'; + err.responseCode = 400; + return callback(err, 'CANNOT'); } this.database.collection('mailboxes').deleteOne( @@ -338,7 +365,10 @@ class MailboxHandler { return callback(err); } if (!mailboxData) { - return callback(null, 'NONEXISTENT'); + const err = new Error('Mailbox update failed with code NoSuchMailbox'); + err.code = 'NONEXISTENT'; + err.responseCode = 404; + return callback(err, 'NONEXISTENT'); } if (updates.path && updates.path !== mailboxData.path) { @@ -373,7 +403,10 @@ class MailboxHandler { } if (!item || !item.value) { - return callback(null, 'NONEXISTENT'); + const err = new Error('Mailbox update failed with code NoSuchMailbox'); + err.code = 'NONEXISTENT'; + err.responseCode = 404; + return callback(err, 'NONEXISTENT'); } return callback(null, true); diff --git a/lib/tools.js b/lib/tools.js index 57f64ddd..98a4df68 100644 --- a/lib/tools.js +++ b/lib/tools.js @@ -661,6 +661,10 @@ module.exports = { err.responseCode = err.responseCode || 404; err.code = 'NoSuchMailbox'; break; + case 'CANNOT': + err.responseCode = err.responseCode || 400; + err.code = 'DisallowedMailboxMethod'; + break; } if (err.responseCode) { diff --git a/test/api/mailboxes-test.js b/test/api/mailboxes-test.js index 08cdf642..87da78c1 100644 --- a/test/api/mailboxes-test.js +++ b/test/api/mailboxes-test.js @@ -120,14 +120,14 @@ describe('Mailboxes tests', function () { expect(response.body.error).to.not.be.empty; }); - // TODO: rewrite when mailboxes.js gets updated status codes it('should POST /users/{user}/mailboxes expect failure / user not found', async () => { const response = await server .post(`/users/${'0'.repeat(24)}/mailboxes`) .send({ path: '/cool2/path2/whatisthis346/cool-drink', hidden: false, retention: 0 }) - .expect(500); + .expect(404); - expect(response.body.error).to.be.equal('User not found'); + expect(response.body.error).to.be.equal('This user does not exist'); + expect(response.body.code).to.be.equal('UserNotFound'); }); it('should POST /users/{user}/mailboxes expect failure / path is missing', async () => { @@ -228,30 +228,21 @@ describe('Mailboxes tests', function () { expect(response3.body.code).to.be.equal('InputValidationError'); expect(response3.body.error).to.be.not.empty; - const response4 = await server - .get(`/users/${user}/mailboxes/${'-'.repeat(24)}`) - - .expect(400); + const response4 = await server.get(`/users/${user}/mailboxes/${'-'.repeat(24)}`).expect(400); expect(response4.body.code).to.be.equal('InputValidationError'); expect(response4.body.error).to.be.not.empty; }); it('should GET /users/{user}/mailboxes/{mailbox} expect failure / mailbox not found', async () => { - const response = await server - .get(`/users/${user}/mailboxes/${'0'.repeat(24)}`) - - .expect(404); + const response = await server.get(`/users/${user}/mailboxes/${'0'.repeat(24)}`).expect(404); expect(response.body.code).to.be.equal('NoSuchMailbox'); expect(response.body.error).to.be.equal('This mailbox does not exist'); }); it('should GET /users/{user}/mailboxes/{mailbox} expect failure / user not found', async () => { - const response = await server - .get(`/users/${'0'.repeat(24)}/mailboxes`) - - .expect(404); + const response = await server.get(`/users/${'0'.repeat(24)}/mailboxes`).expect(404); expect(response.body.error).to.be.equal('This user does not exist'); expect(response.body.code).to.be.equal('UserNotFound'); @@ -335,9 +326,9 @@ describe('Mailboxes tests', function () { const response = await server .put(`/users/${user}/mailboxes/${'0'.repeat(24)}`) .send({ path: 'newPath' }) - .expect(500); + .expect(404); - expect(response.body.error).to.be.equal('Mailbox update failed with code NONEXISTENT'); + expect(response.body.error).to.be.equal('Mailbox update failed with code NoSuchMailbox'); }); it('should PUT /users/{user}/mailboxes/{mailbox} expect failure / user not found', async () => { @@ -346,18 +337,15 @@ describe('Mailboxes tests', function () { const response = await server .put(`/users/${'0'.repeat(24)}/mailboxes/${mailboxes.body.results.at(-1).id}`) .send({ path: 'newPath' }) - .expect(500); + .expect(404); - expect(response.body.error).to.be.equal('Mailbox update failed with code NONEXISTENT'); + expect(response.body.error).to.be.equal('Mailbox update failed with code NoSuchMailbox'); }); it('should PUT /users/{user}/mailboxes/{mailbox} expect failure / nothing was changed', async () => { const mailboxes = await server.get(`/users/${user}/mailboxes`).expect(200); - const response = await server - .put(`/users/${user}/mailboxes/${mailboxes.body.results.at(-1).id}`) - - .expect(400); + const response = await server.put(`/users/${user}/mailboxes/${mailboxes.body.results.at(-1).id}`).expect(400); expect(response.body.error).to.be.equal('Nothing was changed'); }); @@ -367,9 +355,9 @@ describe('Mailboxes tests', function () { const inboxMailbox = mailboxes.body.results.find(el => el.path === 'INBOX'); - const response = await server.put(`/users/${user}/mailboxes/${inboxMailbox.id}`).send({ path: 'newPath/folder123' }).expect(500); + const response = await server.put(`/users/${user}/mailboxes/${inboxMailbox.id}`).send({ path: 'newPath/folder123' }).expect(400); - expect(response.body.error).to.be.equal('Mailbox update failed with code CANNOT'); + expect(response.body.error).to.be.equal('Mailbox update failed with code DisallowedMailboxMethod'); }); it('should DELETE /users/{user}/mailboxes/{mailbox} expect success', async () => { @@ -385,10 +373,10 @@ describe('Mailboxes tests', function () { it('should DELETE /users/{user}/mailboxes/{mailbox} expect failure / protected path', async () => { const mailboxes = await server.get(`/users/${user}/mailboxes`).expect(200); - const response = await server.del(`/users/${user}/mailboxes/${mailboxes.body.results[0].id}`).expect(500); + const response = await server.del(`/users/${user}/mailboxes/${mailboxes.body.results[0].id}`).expect(400); - expect(response.body.error).to.be.equal('Mailbox deletion failed with code CANNOT'); - expect(response.body.code).to.be.equal('CANNOT'); + expect(response.body.error).to.be.equal('Mailbox deletion failed with code DisallowedMailboxMethod'); + expect(response.body.code).to.be.equal('DisallowedMailboxMethod'); }); it('should DELETE /users/{user}/mailboxes/{mailbox} expect failure / incorrect params', async () => { @@ -422,26 +410,18 @@ describe('Mailboxes tests', function () { }); it('should DELETE /users/{user}/mailboxes/{mailbox} expect failure / mailbox not found', async () => { - const response = await server - .del(`/users/${user}/mailboxes/${'0'.repeat(24)}`) - - .expect(500); - - console.log(response.body); + const response = await server.del(`/users/${user}/mailboxes/${'0'.repeat(24)}`).expect(404); - expect(response.body.error).to.be.equal('Mailbox deletion failed with code NONEXISTENT'); - expect(response.body.code).to.be.equal('NONEXISTENT'); + expect(response.body.error).to.be.equal('Mailbox deletion failed with code NoSuchMailbox'); + expect(response.body.code).to.be.equal('NoSuchMailbox'); }); it('should DELETE /users/{user}/mailboxes/{mailbox} expect failure / user not found', async () => { const mailboxes = await server.get(`/users/${user}/mailboxes`).expect(200); - const response = await server - .del(`/users/${'0'.repeat(24)}/mailboxes/${mailboxes.body.results[0].id}`) - - .expect(500); + const response = await server.del(`/users/${'0'.repeat(24)}/mailboxes/${mailboxes.body.results[0].id}`).expect(404); - expect(response.body.error).to.be.equal('Mailbox deletion failed with code NONEXISTENT'); + expect(response.body.error).to.be.equal('Mailbox deletion failed with code NoSuchMailbox'); }); it('should DELETE /users/{user}/mailboxes/{mailbox} expect failure / cannot delete inbox', async () => { @@ -449,9 +429,9 @@ describe('Mailboxes tests', function () { const inboxMailbox = mailboxes.body.results.find(el => el.path === 'INBOX'); - const response = await server.del(`/users/${user}/mailboxes/${inboxMailbox.id}`).expect(500); + const response = await server.del(`/users/${user}/mailboxes/${inboxMailbox.id}`).expect(400); - expect(response.body.error).to.be.equal('Mailbox deletion failed with code CANNOT'); - expect(response.body.code).to.be.equal('CANNOT'); + expect(response.body.error).to.be.equal('Mailbox deletion failed with code DisallowedMailboxMethod'); + expect(response.body.code).to.be.equal('DisallowedMailboxMethod'); }); });