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/jamid/jami-signal-interfaces.ts b/server/src/jamid/jami-signal-interfaces.ts
index 06dd6ed..a839309 100644
--- a/server/src/jamid/jami-signal-interfaces.ts
+++ b/server/src/jamid/jami-signal-interfaces.ts
@@ -59,7 +59,26 @@
export interface IncomingAccountMessage {
accountId: string;
from: string;
- message: Record<string, string>;
+ payload: Record<string, string>;
+}
+
+export interface AccountMessageStatusChanged {
+ accountId: string;
+ messageId: string;
+ peer: string;
+ state: number; // TODO: Replace state number with enum (see account_const.h)
+}
+
+export interface ContactAdded {
+ accountId: string;
+ contactId: string;
+ confirmed: boolean;
+}
+
+export interface ContactRemoved {
+ accountId: string;
+ contactId: string;
+ banned: boolean;
}
export interface ConversationReady {
diff --git a/server/src/jamid/jamid.ts b/server/src/jamid/jamid.ts
index 283e527..a3db747 100644
--- a/server/src/jamid/jamid.ts
+++ b/server/src/jamid/jamid.ts
@@ -24,6 +24,9 @@
import { JamiSignal } from './jami-signal.js';
import {
AccountDetailsChanged,
+ AccountMessageStatusChanged,
+ ContactAdded,
+ ContactRemoved,
ConversationLoaded,
ConversationReady,
ConversationRemoved,
@@ -93,8 +96,20 @@
onKnownDevicesChanged.next({ accountId, devices });
const onIncomingAccountMessage = new Subject<IncomingAccountMessage>();
- handlers.IncomingAccountMessage = (accountId: string, from: string, message: Record<string, string>) =>
- onIncomingAccountMessage.next({ accountId, from, message });
+ handlers.IncomingAccountMessage = (accountId: string, from: string, payload: Record<string, string>) =>
+ onIncomingAccountMessage.next({ accountId, from, payload });
+
+ const onAccountMessageStatusChanged = new Subject<AccountMessageStatusChanged>();
+ handlers.AccountMessageStatusChanged = (accountId: string, messageId: string, peer: string, state: number) =>
+ onAccountMessageStatusChanged.next({ accountId, messageId, peer, state });
+
+ const onContactAdded = new Subject<ContactAdded>();
+ handlers.ContactAdded = (accountId: string, contactId: string, confirmed: boolean) =>
+ onContactAdded.next({ accountId, contactId, confirmed });
+
+ const onContactRemoved = new Subject<ContactRemoved>();
+ handlers.ContactRemoved = (accountId: string, contactId: string, banned: boolean) =>
+ onContactRemoved.next({ accountId, contactId, banned });
const onConversationReady = new Subject<ConversationReady>();
handlers.ConversationReady = (accountId: string, conversationId: string) =>
@@ -122,6 +137,9 @@
onRegisteredNameFound: onRegisteredNameFound.asObservable(),
onKnownDevicesChanged: onKnownDevicesChanged.asObservable(),
onIncomingAccountMessage: onIncomingAccountMessage.asObservable(),
+ onAccountMessageStatusChanged: onAccountMessageStatusChanged.asObservable(),
+ onContactAdded: onContactAdded.asObservable(),
+ onContactRemoved: onContactRemoved.asObservable(),
onConversationReady: onConversationReady.asObservable(),
onConversationRemoved: onConversationRemoved.asObservable(),
onConversationLoaded: onConversationLoaded.asObservable(),
@@ -351,7 +369,7 @@
this.events.onIncomingAccountMessage.subscribe((signal) => {
log.debug('Received IncomingAccountMessage:', JSON.stringify(signal));
- const message = JSON.parse(signal.message['application/json']);
+ const message = JSON.parse(signal.payload['application/json']);
if (message === undefined) {
log.debug('Undefined account message');
@@ -369,6 +387,18 @@
this.ws.send(signal.accountId, message);
});
+ this.events.onAccountMessageStatusChanged.subscribe((signal) => {
+ log.debug('Received AccountMessageStatusChanged:', JSON.stringify(signal));
+ });
+
+ this.events.onContactAdded.subscribe((signal) => {
+ log.debug('Received ContactAdded:', JSON.stringify(signal));
+ });
+
+ this.events.onContactRemoved.subscribe((signal) => {
+ log.debug('Received ContactRemoved:', JSON.stringify(signal));
+ });
+
this.events.onConversationReady.subscribe((signal) => {
log.debug('Received ConversationReady:', JSON.stringify(signal));
});
diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts
index 715db0a..4f06992 100644
--- a/server/src/middleware/auth.ts
+++ b/server/src/middleware/auth.ts
@@ -52,7 +52,7 @@
res.locals.accountId = payload.id;
next();
} catch (e) {
- res.sendStatus(HttpStatusCode.Unauthorized);
+ res.status(HttpStatusCode.Unauthorized).send('Invalid access token');
}
};
}
diff --git a/server/src/middleware/setup.ts b/server/src/middleware/setup.ts
index 3575fd0..a697b5a 100644
--- a/server/src/middleware/setup.ts
+++ b/server/src/middleware/setup.ts
@@ -27,8 +27,9 @@
const isSetupComplete = adminConfig.get() !== undefined;
if (!isSetupComplete) {
- res.sendStatus(HttpStatusCode.Forbidden);
+ res.status(HttpStatusCode.Forbidden).send('Setup not complete');
return;
}
+
next();
}
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;
}