Fix #111 (better error code handling)

Tietokilta · Sep 24, 2023 · 1286b40 · 1286b40
1 parent 029cf2a
commit 1286b40
Show file tree

Hide file tree

Showing 15 changed files with 80 additions and 51 deletions.
diff --git a/packages/ilmomasiina-backend/src/authentication/adminAuthSession.ts b/packages/ilmomasiina-backend/src/authentication/adminAuthSession.ts
@@ -2,9 +2,9 @@ import {
   createSigner, createVerifier, SignerSync, VerifierSync,
 } from 'fast-jwt';
 import { FastifyRequest } from 'fastify';
-import { Unauthorized } from 'http-errors';
 
 import type { UserID, UserSchema } from '@tietokilta/ilmomasiina-models';
+import { BadSession } from './errors';
 
 export interface AdminTokenData {
   user: UserID;
@@ -34,15 +34,15 @@ export default class AdminAuthSession {
 
   /**
    * Verifies the incoming request authorization.
-   * Throws an Unauthorized error if session is not valid.
+   * Throws a BadSession error if session is not valid.
    *
    * @param request incoming request
    */
   verifySession(request: FastifyRequest): AdminTokenData {
     const header = request.headers.authorization; // Yes, Fastify converts header names to lowercase :D
 
     if (!header) {
-      throw new Unauthorized('Missing Authorization header');
+      throw new BadSession('Missing Authorization header');
     }
 
     const token = Array.isArray(header) ? header[0] : header;
@@ -51,7 +51,7 @@ export default class AdminAuthSession {
       const data = this.verify(token);
       return { user: parseInt(data.user), email: data.email || '' };
     } catch {
-      throw new Unauthorized('Invalid session');
+      throw new BadSession('Invalid session');
     }
   }
 }
diff --git a/packages/ilmomasiina-backend/src/authentication/errors.ts b/packages/ilmomasiina-backend/src/authentication/errors.ts
@@ -0,0 +1,9 @@
+import { ErrorCode } from '@tietokilta/ilmomasiina-models';
+import CustomError from '../util/customError';
+
+// eslint-disable-next-line import/prefer-default-export
+export class BadSession extends CustomError {
+  constructor(message: string) {
+    super(401, ErrorCode.BAD_SESSION, message);
+  }
+}
diff --git a/packages/ilmomasiina-backend/src/routes/admin/events/errors.ts b/packages/ilmomasiina-backend/src/routes/admin/events/errors.ts
@@ -1,18 +1,8 @@
 /* eslint-disable max-classes-per-file */
-import type {
-  EditConflictError, QuestionID, QuotaID, WouldMoveSignupsToQueueError,
+import {
+  EditConflictError, ErrorCode, QuestionID, QuotaID, WouldMoveSignupsToQueueError,
 } from '@tietokilta/ilmomasiina-models';
-
-export abstract class CustomError extends Error {
-  public readonly statusCode: number;
-  public readonly code: string;
-
-  protected constructor(statusCode: number, code: string, message: string) {
-    super(message);
-    this.statusCode = statusCode;
-    this.code = code;
-  }
-}
+import CustomError from '../../../util/customError';
 
 export class EditConflict extends CustomError implements EditConflictError {
   public readonly updatedAt: string; // in date-time format
@@ -23,7 +13,7 @@ export class EditConflict extends CustomError implements EditConflictError {
     const updatedAtStr = updatedAt.toISOString();
     super(
       409,
-      'EditConflict',
+      ErrorCode.EDIT_CONFLICT,
       `the event was updated separately at ${updatedAtStr}`,
     );
     this.updatedAt = updatedAtStr;
@@ -38,7 +28,7 @@ export class WouldMoveSignupsToQueue extends CustomError implements WouldMoveSig
   constructor(count: number) {
     super(
       409,
-      'WouldMoveSignupsToQueue',
+      ErrorCode.WOULD_MOVE_SIGNUPS_TO_QUEUE,
       `this change would move ${count} signups into the queue`,
     );
     this.count = count;

diff --git a/packages/ilmomasiina-backend/src/routes/admin/users/changePassword.ts b/packages/ilmomasiina-backend/src/routes/admin/users/changePassword.ts
@@ -1,10 +1,16 @@
 import { FastifyReply, FastifyRequest } from 'fastify';
-import { BadRequest, NotFound, Unauthorized } from 'http-errors';
+import { BadRequest, NotFound } from 'http-errors';
 
-import type { UserChangePasswordSchema } from '@tietokilta/ilmomasiina-models';
-import { AuditEvent } from '@tietokilta/ilmomasiina-models';
+import { AuditEvent, ErrorCode, UserChangePasswordSchema } from '@tietokilta/ilmomasiina-models';
 import AdminPasswordAuth from '../../../authentication/adminPasswordAuth';
 import { User } from '../../../models/user';
+import CustomError from '../../../util/customError';
+
+class WrongOldPassword extends CustomError {
+  constructor(message: string) {
+    super(401, ErrorCode.WRONG_OLD_PASSWORD, message);
+  }
+}
 
 export default async function changePassword(
   request: FastifyRequest<{ Body: UserChangePasswordSchema }>,
@@ -26,7 +32,7 @@ export default async function changePassword(
     } else {
       // Verify old password
       if (!AdminPasswordAuth.verifyHash(request.body.oldPassword, existing.password)) {
-        throw new Unauthorized('Incorrect password');
+        throw new WrongOldPassword('Incorrect password');
       }
       // Update user with a new password
       await existing.update(

diff --git a/packages/ilmomasiina-backend/src/routes/authentication/adminLogin.ts b/packages/ilmomasiina-backend/src/routes/authentication/adminLogin.ts
@@ -5,6 +5,7 @@ import type { AdminLoginBody, AdminLoginResponse } from '@tietokilta/ilmomasiina
 import AdminAuthSession, { AdminTokenData } from '../../authentication/adminAuthSession';
 import AdminPasswordAuth from '../../authentication/adminPasswordAuth';
 import { User } from '../../models/user';
+import CustomError from '../../util/customError';
 
 export function adminLogin(session: AdminAuthSession) {
   return async (
@@ -62,7 +63,7 @@ export function requireAdmin(session: AdminAuthSession, fastify: FastifyInstance
       } catch (err) {
         // Throwing inside hook is not safe, so the errors must be converted to actual reply here
         fastify.log.error(err);
-        if (err instanceof HttpError) {
+        if (err instanceof HttpError || err instanceof CustomError) {
           reply.code(err.statusCode).send(err);
         } else {
           reply.internalServerError('Session validation failed');

diff --git a/packages/ilmomasiina-backend/src/util/customError.ts b/packages/ilmomasiina-backend/src/util/customError.ts
@@ -0,0 +1,12 @@
+import { ErrorCode } from '@tietokilta/ilmomasiina-models';
+
+export default abstract class CustomError extends Error {
+  public readonly statusCode: number;
+  public readonly code: ErrorCode;
+
+  protected constructor(statusCode: number, code: ErrorCode, message: string) {
+    super(message);
+    this.statusCode = statusCode;
+    this.code = code;
+  }
+}
diff --git a/packages/ilmomasiina-components/src/api.ts b/packages/ilmomasiina-components/src/api.ts
@@ -1,3 +1,5 @@
+import { ErrorCode } from '@tietokilta/ilmomasiina-models';
+
 export interface FetchOptions {
   method?: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE';
   body?: any;
@@ -8,7 +10,7 @@ export interface FetchOptions {
 
 export class ApiError extends Error {
   status: number;
-  code?: string;
+  code?: ErrorCode;
   response?: any;
 
   constructor(status: number, response: any) {
@@ -31,10 +33,6 @@ export class ApiError extends Error {
     }
     return error;
   }
-
-  get isUnauthenticated() {
-    return this.status === 401;
-  }
 }
 
 let apiUrl = '/api';

diff --git a/packages/ilmomasiina-frontend/src/api.ts b/packages/ilmomasiina-frontend/src/api.ts
@@ -1,4 +1,5 @@
 import { ApiError, apiFetch, FetchOptions } from '@tietokilta/ilmomasiina-components';
+import { ErrorCode } from '@tietokilta/ilmomasiina-models';
 import { loginExpired } from './modules/auth/actions';
 import type { DispatchAction } from './store/types';
 
@@ -9,7 +10,7 @@ export default async function adminApiFetch(uri: string, opts: FetchOptions, dis
   try {
     return await apiFetch(uri, opts);
   } catch (err) {
-    if (err instanceof ApiError && err.isUnauthenticated) {
+    if (err instanceof ApiError && err.code === ErrorCode.BAD_SESSION) {
       dispatch(loginExpired());
     }
     throw err;

diff --git a/packages/ilmomasiina-frontend/src/locales/en.json b/packages/ilmomasiina-frontend/src/locales/en.json
@@ -58,6 +58,7 @@
         "adminUsers.changePassword.errors.verifyMatch": "The passwords do not match",
         "adminUsers.changePassword.success": "Password changed successfully.",
         "adminUsers.changePassword.failed": "Failed to change password.",
+        "adminUsers.changePassword.wrongPassword": "Old password is incorrect.",
         "adminUsers.returnToEvents": "Go back",
         "auditLog.title": "Audit log",
         "auditLog.loadFailed": "Failed to load logs",

diff --git a/packages/ilmomasiina-frontend/src/locales/fi.json b/packages/ilmomasiina-frontend/src/locales/fi.json
@@ -60,6 +60,7 @@
         "adminUsers.changePassword.errors.verifyMatch": "Salasanat eivät täsmää",
         "adminUsers.changePassword.success": "Salasana vaihdettiin onnistuneesti.",
         "adminUsers.changePassword.failed": "Salasanan vaihto epäonnistui.",
+        "adminUsers.changePassword.wrongPassword": "Vanha salasana on väärä.",
         "adminUsers.returnToEvents": "Takaisin",
         "auditLog.title": "Toimintoloki",
         "auditLog.loadFailed": "Lokien lataus epäonnistui",

diff --git a/packages/ilmomasiina-frontend/src/modules/adminUsers/actions.ts b/packages/ilmomasiina-frontend/src/modules/adminUsers/actions.ts
@@ -108,14 +108,9 @@ export const changePassword = (data: UserChangePasswordSchema) => async (
 ) => {
   const { accessToken } = getState().auth;
 
-  try {
-    await adminApiFetch('admin/users/self/changepassword', {
-      accessToken,
-      method: 'POST',
-      body: data,
-    }, dispatch);
-    return true;
-  } catch (e) {
-    return false;
-  }
+  await adminApiFetch('admin/users/self/changepassword', {
+    accessToken,
+    method: 'POST',
+    body: data,
+  }, dispatch);
 };
diff --git a/packages/ilmomasiina-frontend/src/modules/editor/actions.ts b/packages/ilmomasiina-frontend/src/modules/editor/actions.ts
@@ -1,6 +1,7 @@
 import { ApiError } from '@tietokilta/ilmomasiina-components';
-import type {
-  AdminEventResponse, CategoriesResponse, CheckSlugResponse, EditConflictError, EventID, EventUpdateBody, SignupID,
+import {
+  AdminEventResponse, CategoriesResponse, CheckSlugResponse, EditConflictError, ErrorCode, EventID, EventUpdateBody,
+  SignupID,
 } from '@tietokilta/ilmomasiina-models';
 import adminApiFetch from '../../api';
 import type { DispatchAction, GetState } from '../../store/types';
@@ -274,11 +275,11 @@ export const publishEventUpdate = (
     dispatch(loaded(response));
     return response;
   } catch (e) {
-    if (e instanceof ApiError && e.code === 'WouldMoveSignupsToQueue') {
+    if (e instanceof ApiError && e.code === ErrorCode.WOULD_MOVE_SIGNUPS_TO_QUEUE) {
       dispatch(moveToQueueWarning(e.response!.count));
       return null;
     }
-    if (e instanceof ApiError && e.code === 'EditConflict') {
+    if (e instanceof ApiError && e.code === ErrorCode.EDIT_CONFLICT) {
       dispatch(editConflictDetected(e.response!));
       return null;
     }

diff --git a/packages/ilmomasiina-frontend/src/routes/AdminUsers/ChangePasswordForm.tsx b/packages/ilmomasiina-frontend/src/routes/AdminUsers/ChangePasswordForm.tsx
@@ -7,6 +7,8 @@ import {
 import { useTranslation } from 'react-i18next';
 import { toast } from 'react-toastify';
 
+import { ApiError } from '@tietokilta/ilmomasiina-components';
+import { ErrorCode } from '@tietokilta/ilmomasiina-models';
 import i18n from '../../i18n';
 import { changePassword } from '../../modules/adminUsers/actions';
 import { useTypedDispatch } from '../../store/reducers';
@@ -42,15 +44,19 @@ const ChangePasswordForm = () => {
   const { t } = useTranslation();
 
   const onSubmit = async (data: FormData, { setSubmitting, resetForm }: FormikHelpers<FormData>) => {
-    // TODO: better error handling
-    const success = await dispatch(changePassword(data));
-    if (success) {
+    try {
+      await dispatch(changePassword(data));
       resetForm();
       toast.success(t('adminUsers.changePassword.success'), { autoClose: 5000 });
-    } else {
-      toast.error(t('adminUsers.changePassword.failed'), { autoClose: 5000 });
+    } catch (err) {
+      if (err instanceof ApiError && err.code === ErrorCode.WRONG_OLD_PASSWORD) {
+        toast.error(t('adminUsers.changePassword.wrongPassword'), { autoClose: 5000 });
+      } else {
+        toast.error(t('adminUsers.changePassword.failed'), { autoClose: 5000 });
+      }
+    } finally {
+      setSubmitting(false);
     }
-    setSubmitting(false);
   };
 
   return (

diff --git a/packages/ilmomasiina-models/src/enum.ts b/packages/ilmomasiina-models/src/enum.ts
@@ -29,3 +29,10 @@ export enum AuditEvent {
   RESET_PASSWORD = 'user.resetpassword',
   CHANGE_PASSWORD = 'user.changepassword',
 }
+
+export enum ErrorCode {
+  BAD_SESSION = 'BadSession',
+  EDIT_CONFLICT = 'EditConflict',
+  WOULD_MOVE_SIGNUPS_TO_QUEUE = 'WouldMoveSignupsToQueue',
+  WRONG_OLD_PASSWORD = 'WrongOldPassword',
+}
diff --git a/packages/ilmomasiina-models/src/schema/errors/index.ts b/packages/ilmomasiina-models/src/schema/errors/index.ts
@@ -1,12 +1,13 @@
 import { Static, Type } from '@sinclair/typebox';
 
+import { ErrorCode } from '../../enum';
 import { questionID } from '../question/attributes';
 import { quotaID } from '../quota/attributes';
 
 /** Response schema for a generic error. */
 export const errorResponse = Type.Object({
   statusCode: Type.Number(),
-  code: Type.String(),
+  code: Type.Optional(Type.Enum(ErrorCode)),
   message: Type.String(),
 });