Add more robust error handling to server
Changes:
- Use the proper RESTful HTTP status codes for all endpoints (e.g. 204 rather than 200 when the response body is empty)
- Add consistent and more helpful error messages
- Handle invalid route parameters by checking if jamid returns empty objects (e.g. invalid contact IDs)
- Use res.send() rather than res.json() for consistency
- Handle three new signals
GitLab: #111
Change-Id: I1d48dc4629995ab9a96bb2086a9aa91f81889598
diff --git a/server/src/routers/account-router.ts b/server/src/routers/account-router.ts
index 7bccea7..4e1722d 100644
--- a/server/src/routers/account-router.ts
+++ b/server/src/routers/account-router.ts
@@ -30,9 +30,6 @@
accountRouter.use(authenticateToken);
-// TODO: If tokens can be generated on one daemon and used on another (transferrable between daemons),
-// then add middleware to check that the currently logged-in accountId is stored in this daemon instance
-
// TODO: Do we really need this route to return the default moderators?
// It would be cleaner just to GET /default-moderators for this
accountRouter.get(
@@ -51,7 +48,7 @@
});
}
- res.json({
+ res.send({
id: accountId,
details: jamid.getAccountDetails(accountId),
volatileDetails: jamid.getVolatileAccountDetails(accountId),
@@ -63,18 +60,22 @@
accountRouter.patch('/', (req, res) => {
const accountId = res.locals.accountId;
+
const currentAccountDetails = jamid.getAccountDetails(accountId);
const newAccountDetails: AccountDetails = { ...currentAccountDetails, ...req.body };
jamid.setAccountDetails(accountId, newAccountDetails);
+
res.sendStatus(HttpStatusCode.NoContent);
});
+// TODO: Should this endpoint be removed?
accountRouter.post('/send-account-message', (req: Request<ParamsDictionary, any, AccountTextMessage>, res) => {
const { from, to, message } = req.body;
- if (!from || !to || !message) {
- res.status(HttpStatusCode.BadRequest).send('Missing arguments in request');
+ if (from === undefined || to === undefined || message === undefined) {
+ res.status(HttpStatusCode.BadRequest).send('Missing from, to, or message in body');
return;
}
+
jamid.sendAccountTextMessage(from, to, JSON.stringify(message));
- res.end();
+ res.sendStatus(HttpStatusCode.NoContent);
});
diff --git a/server/src/routers/auth-router.ts b/server/src/routers/auth-router.ts
index 086f640..f04529e 100644
--- a/server/src/routers/auth-router.ts
+++ b/server/src/routers/auth-router.ts
@@ -42,8 +42,13 @@
'/new-account',
asyncHandler(async (req: Request<ParamsDictionary, string, Credentials>, res, _next) => {
const { username, password } = req.body;
- if (!username || !password) {
- res.status(HttpStatusCode.BadRequest).send('Missing username or password');
+ if (username === undefined || password === undefined) {
+ res.status(HttpStatusCode.BadRequest).send('Missing username or password in body');
+ return;
+ }
+
+ if (password === '') {
+ res.status(HttpStatusCode.BadRequest).send('Password may not be empty');
return;
}
@@ -82,8 +87,8 @@
'/login',
asyncHandler(async (req: Request<ParamsDictionary, { accessToken: string } | string, Credentials>, res, _next) => {
const { username, password } = req.body;
- if (!username || !password) {
- res.status(HttpStatusCode.BadRequest).send('Missing username or password');
+ if (username === undefined || password === undefined) {
+ res.status(HttpStatusCode.BadRequest).send('Missing username or password in body');
return;
}
@@ -99,13 +104,15 @@
// TODO: load the password from Jami
const hashedPassword = creds.get(username);
if (!hashedPassword) {
- res.status(HttpStatusCode.NotFound).send('Password not found');
+ res
+ .status(HttpStatusCode.NotFound)
+ .send('Password not found (the account does not have a password set on the server)');
return;
}
const isPasswordVerified = await argon2.verify(hashedPassword, password);
if (!isPasswordVerified) {
- res.sendStatus(HttpStatusCode.Unauthorized);
+ res.status(HttpStatusCode.Unauthorized).send('Incorrect password');
return;
}
diff --git a/server/src/routers/call-router.ts b/server/src/routers/call-router.ts
index 835bf66..96cd7ce 100644
--- a/server/src/routers/call-router.ts
+++ b/server/src/routers/call-router.ts
@@ -16,6 +16,7 @@
* <https://www.gnu.org/licenses/>.
*/
import { Router } from 'express';
+import { HttpStatusCode } from 'jami-web-common';
import { Container } from 'typedi';
import { Jamid } from '../jamid/jamid.js';
@@ -34,5 +35,11 @@
callRouter.get('/:callId', (req, res) => {
const callDetails = jamid.getCallDetails(res.locals.accountId, req.params.callId);
+
+ if (Object.keys(callDetails).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such call found');
+ return;
+ }
+
res.send(callDetails);
});
diff --git a/server/src/routers/contacts-router.ts b/server/src/routers/contacts-router.ts
index 8d51d2b..d757d52 100644
--- a/server/src/routers/contacts-router.ts
+++ b/server/src/routers/contacts-router.ts
@@ -35,6 +35,12 @@
contactsRouter.get('/:contactId', (req, res) => {
const contactDetails = jamid.getContactDetails(res.locals.accountId, req.params.contactId);
+
+ if (Object.keys(contactDetails).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such contact found');
+ return;
+ }
+
res.send(contactDetails);
});
@@ -45,6 +51,11 @@
jamid.addContact(accountId, contactId);
const contactDetails = jamid.getContactDetails(accountId, contactId);
+ if (Object.keys(contactDetails).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such contact found');
+ return;
+ }
+
res.send(contactDetails);
});
diff --git a/server/src/routers/conversation-router.ts b/server/src/routers/conversation-router.ts
index 164ea3e..224962c 100644
--- a/server/src/routers/conversation-router.ts
+++ b/server/src/routers/conversation-router.ts
@@ -37,6 +37,10 @@
// TODO: Create interface for return type in common/ when Records and interfaces are refactored
async function createConversationResponseObject(accountId: string, accountUri: string, conversationId: string) {
const infos = jamid.getConversationInfos(accountId, conversationId);
+ if (Object.keys(infos).length === 0) {
+ return undefined;
+ }
+
const members = jamid.getConversationMembers(accountId, conversationId);
const namedMembers = [];
@@ -94,21 +98,29 @@
})
);
-conversationRouter.post('/', (req: Request<ParamsDictionary, Record<string, string>, ConversationMembers>, res) => {
- const accountId = res.locals.accountId;
+conversationRouter.post(
+ '/',
+ (req: Request<ParamsDictionary, Record<string, string> | string, ConversationMembers>, res) => {
+ const { members } = req.body;
+ if (members === undefined || members.length !== 1) {
+ res.status(HttpStatusCode.BadRequest).send('Missing members or more than one member in body');
+ return;
+ }
- const { members } = req.body;
- if (members === undefined || members.length !== 1) {
- res.sendStatus(HttpStatusCode.BadRequest);
- return;
+ const accountId = res.locals.accountId;
+
+ const contactId = members[0];
+ jamid.addContact(accountId, contactId);
+
+ const contactDetails = jamid.getContactDetails(accountId, contactId);
+ if (Object.keys(contactDetails).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such member found');
+ return;
+ }
+
+ res.send(contactDetails);
}
-
- const contactId = members[0];
- jamid.addContact(accountId, contactId);
-
- const contactDetails = jamid.getContactDetails(accountId, contactId);
- res.send(contactDetails);
-});
+);
// TODO: Check if we actually need this endpoint to return messages.
// Verify by checking what is truly needed in the client when migrating, to clean up the API.
@@ -124,13 +136,12 @@
// Retrieve the URI of the current account (Account.username actually stores the URI rather than the username)
const accountUri = jamid.getAccountDetails(accountId)['Account.username'];
- const conversationIds = jamid.getConversationIds(accountId);
- if (!conversationIds.includes(conversationId)) {
- res.sendStatus(HttpStatusCode.NotFound);
+ const conversation = await createConversationResponseObject(accountId, accountUri, conversationId);
+ if (conversation === undefined) {
+ res.status(HttpStatusCode.NotFound).send('No such conversation found');
return;
}
- const conversation = await createConversationResponseObject(accountId, accountUri, conversationId);
res.send(conversation);
})
);
@@ -141,9 +152,9 @@
const accountId = res.locals.accountId;
const conversationId = req.params.conversationId;
- const conversationIds = jamid.getConversationIds(accountId);
- if (!conversationIds.includes(conversationId)) {
- res.sendStatus(HttpStatusCode.NotFound);
+ const infos = jamid.getConversationInfos(accountId, conversationId);
+ if (Object.keys(infos).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such conversation found');
return;
}
@@ -157,11 +168,20 @@
(req: Request<ParamsDictionary, any, ConversationMessage>, res) => {
const { message } = req.body;
if (message === undefined) {
- res.sendStatus(HttpStatusCode.BadRequest);
+ res.status(HttpStatusCode.BadRequest).send('Missing message in body');
return;
}
- jamid.sendConversationMessage(res.locals.accountId, req.params.conversationId, message);
- res.end();
+ const accountId = res.locals.accountId;
+ const conversationId = req.params.conversationId;
+
+ const infos = jamid.getConversationInfos(accountId, conversationId);
+ if (Object.keys(infos).length === 0) {
+ res.status(HttpStatusCode.NotFound).send('No such conversation found');
+ return;
+ }
+
+ jamid.sendConversationMessage(accountId, conversationId, message);
+ res.sendStatus(HttpStatusCode.NoContent);
}
);
diff --git a/server/src/routers/default-moderators-router.ts b/server/src/routers/default-moderators-router.ts
index 797aac3..06d79ca 100644
--- a/server/src/routers/default-moderators-router.ts
+++ b/server/src/routers/default-moderators-router.ts
@@ -51,7 +51,7 @@
defaultModeratorsRouter.put('/:contactId', (req, res) => {
jamid.addDefaultModerator(res.locals.accountId, req.params.contactId);
- res.end();
+ res.sendStatus(HttpStatusCode.NoContent);
});
defaultModeratorsRouter.delete('/:contactId', (req, res) => {
diff --git a/server/src/routers/nameserver-router.ts b/server/src/routers/nameserver-router.ts
index 6e49b52..2ead8ee 100644
--- a/server/src/routers/nameserver-router.ts
+++ b/server/src/routers/nameserver-router.ts
@@ -35,13 +35,13 @@
const result = await jamid.lookupUsername(req.params.username, res.locals.accountId);
switch (result.state) {
case 0:
- res.json(result);
+ res.send(result);
break;
case 1:
- res.sendStatus(HttpStatusCode.BadRequest);
+ res.status(HttpStatusCode.BadRequest).send('Invalid username');
break;
default:
- res.sendStatus(HttpStatusCode.NotFound);
+ res.status(HttpStatusCode.NotFound).send('No such username found');
break;
}
})
@@ -53,13 +53,13 @@
const result = await jamid.lookupAddress(req.params.address, res.locals.accountId);
switch (result.state) {
case 0:
- res.json(result);
+ res.send(result);
break;
case 1:
- res.sendStatus(HttpStatusCode.BadRequest);
+ res.status(HttpStatusCode.BadRequest).send('Invalid address');
break;
default:
- res.sendStatus(HttpStatusCode.NotFound);
+ res.status(HttpStatusCode.NotFound).send('No such address found');
break;
}
})
diff --git a/server/src/routers/setup-router.ts b/server/src/routers/setup-router.ts
index d155ef5..7dfa24f 100644
--- a/server/src/routers/setup-router.ts
+++ b/server/src/routers/setup-router.ts
@@ -40,19 +40,25 @@
setupRouter.post(
'/admin/create',
asyncHandler(async (req: Request<ParamsDictionary, string, { password?: string }>, res, _next) => {
- const isAdminCreated = adminConfig.get() !== undefined;
- if (isAdminCreated) {
- res.sendStatus(HttpStatusCode.BadRequest);
+ const { password } = req.body;
+ if (password === undefined) {
+ res.status(HttpStatusCode.BadRequest).send('Missing password in body');
return;
}
- const { password } = req.body;
- if (!password) {
- res.status(HttpStatusCode.BadRequest).send('Missing password');
+ if (password === '') {
+ res.status(HttpStatusCode.BadRequest).send('Password may not be empty');
+ return;
+ }
+
+ const isAdminCreated = adminConfig.get() !== undefined;
+ if (isAdminCreated) {
+ res.status(HttpStatusCode.Conflict).send('Admin already exists');
return;
}
const hashedPassword = await argon2.hash(password, { type: argon2.argon2id });
+
adminConfig.set(hashedPassword);
await adminConfig.save();
@@ -62,7 +68,7 @@
// Every request handler after this line will be submitted to this middleware
// in order to ensure that the admin account is set up before proceeding with
-// setup related requests.
+// setup related requests
setupRouter.use(checkAdminSetup);
setupRouter.post(
@@ -70,15 +76,16 @@
asyncHandler(
async (req: Request<ParamsDictionary, { accessToken: string } | string, { password: string }>, res, _next) => {
const { password } = req.body;
- if (!password) {
- res.status(HttpStatusCode.BadRequest).send('Missing password');
+ if (password === undefined) {
+ res.status(HttpStatusCode.BadRequest).send('Missing password in body');
return;
}
const hashedPassword = adminConfig.get();
+
const isPasswordVerified = await argon2.verify(hashedPassword, password);
if (!isPasswordVerified) {
- res.sendStatus(HttpStatusCode.Forbidden);
+ res.status(HttpStatusCode.Forbidden).send('Incorrect password');
return;
}