From f83350cb2a7d02b526efc5f5fecfb5f1eeaeae14 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 11 Jul 2023 09:20:11 +0200 Subject: [PATCH] refactor: move status codes into classes (#4200) Make each error class have to define its own status code. This makes it easier for developers to see which code an error corresponds to and means less jumping back and forth between files. In other words: improved locality. Unfortunately, the long switch needs to stay in case we get errors thrown that aren't of the Unleash Error type, but we can move it to the `fromLegacyError` file instead. Tradeoff analysis by @kwasniew: + I like the locality of error to code reasoning - now HTTP leaks to the non-HTTP code that throws those errors e.g. application services If we had other delivery mechanisms other than HTTP then it wouldn't make sense to couple error codes to one protocol (HTTP). But since we're mostly doing web it may not be a problem. @thomasheartman's response: This is a good point and something I hadn't considered. The same data was always available on those errors (by using the same property), I've just made the declaration local to each error instead of something that the parent class handles. The idea was to make it easier to create new error classes with their corresponding error codes. Because the errors are intended to be API errors (or at least, I've always considered them to be that), I think that makes sense. Taking your comment into consideration, I still think it's the right thing to do, but I'm not bullish about it. We could always walk it back later if we find that it's not appropriate. The old code is still available and we could easily enough roll back this change if we find that we want to decouple it later. --- src/lib/error/bad-data-error.ts | 2 + src/lib/error/content-type-error.ts | 2 + src/lib/error/disabled-error.ts | 4 +- src/lib/error/feature-has-tag-error.ts | 4 +- src/lib/error/forbidden-error.ts | 4 +- src/lib/error/from-legacy-error.ts | 70 ++++++++++++++++++ src/lib/error/incompatible-project-error.ts | 2 + src/lib/error/invalid-operation-error.ts | 4 +- src/lib/error/invalid-token-error.ts | 2 + .../error/minimum-one-environment-error.ts | 4 +- src/lib/error/name-exists-error.ts | 4 +- src/lib/error/not-implemented-error.ts | 4 +- src/lib/error/notfound-error.ts | 2 + src/lib/error/operation-denied-error.ts | 4 +- src/lib/error/owasp-validation-error.ts | 2 + src/lib/error/password-mismatch.ts | 2 + src/lib/error/password-undefined.ts | 2 + src/lib/error/permission-error.ts | 2 + src/lib/error/project-without-owner-error.ts | 2 + src/lib/error/role-in-use-error.ts | 4 +- src/lib/error/unauthorized-error.ts | 4 +- src/lib/error/unleash-error.ts | 72 ++----------------- src/lib/error/used-token-error.ts | 2 + src/lib/types/authentication-required.ts | 2 + 24 files changed, 130 insertions(+), 76 deletions(-) diff --git a/src/lib/error/bad-data-error.ts b/src/lib/error/bad-data-error.ts index 3fc85e25aa..9c034ca92a 100644 --- a/src/lib/error/bad-data-error.ts +++ b/src/lib/error/bad-data-error.ts @@ -9,6 +9,8 @@ type ValidationErrorDescription = { path?: string; }; class BadDataError extends UnleashError { + statusCode = 400; + details: ValidationErrorDescription[]; constructor( diff --git a/src/lib/error/content-type-error.ts b/src/lib/error/content-type-error.ts index 14d99b21f2..78220c9c48 100644 --- a/src/lib/error/content-type-error.ts +++ b/src/lib/error/content-type-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; class ContentTypeError extends UnleashError { + statusCode = 415; + constructor( acceptedContentTypes: [string, ...string[]], providedContentType?: string, diff --git a/src/lib/error/disabled-error.ts b/src/lib/error/disabled-error.ts index 7fe2fd0000..d946ea86d2 100644 --- a/src/lib/error/disabled-error.ts +++ b/src/lib/error/disabled-error.ts @@ -1,5 +1,7 @@ import { UnleashError } from './unleash-error'; -class DisabledError extends UnleashError {} +class DisabledError extends UnleashError { + statusCode = 422; +} export default DisabledError; diff --git a/src/lib/error/feature-has-tag-error.ts b/src/lib/error/feature-has-tag-error.ts index bcd0592c91..2f13c7c233 100644 --- a/src/lib/error/feature-has-tag-error.ts +++ b/src/lib/error/feature-has-tag-error.ts @@ -1,5 +1,7 @@ import { UnleashError } from './unleash-error'; -class FeatureHasTagError extends UnleashError {} +class FeatureHasTagError extends UnleashError { + statusCode = 409; +} export default FeatureHasTagError; module.exports = FeatureHasTagError; diff --git a/src/lib/error/forbidden-error.ts b/src/lib/error/forbidden-error.ts index d8ca41dcfe..66cd29709f 100644 --- a/src/lib/error/forbidden-error.ts +++ b/src/lib/error/forbidden-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; -class ForbiddenError extends UnleashError {} +class ForbiddenError extends UnleashError { + statusCode = 403; +} export default ForbiddenError; module.exports = ForbiddenError; diff --git a/src/lib/error/from-legacy-error.ts b/src/lib/error/from-legacy-error.ts index c90e09b076..d754b019d6 100644 --- a/src/lib/error/from-legacy-error.ts +++ b/src/lib/error/from-legacy-error.ts @@ -7,6 +7,69 @@ import { UnleashError, } from './unleash-error'; +const getStatusCode = (errorName: string): number => { + switch (errorName) { + case 'ContentTypeError': + return 415; + case 'ValidationError': + return 400; + case 'BadDataError': + return 400; + case 'OwaspValidationError': + return 400; + case 'PasswordUndefinedError': + return 400; + case 'MinimumOneEnvironmentError': + return 400; + case 'InvalidTokenError': + return 401; + case 'NoAccessError': + return 403; + case 'UsedTokenError': + return 403; + case 'InvalidOperationError': + return 403; + case 'IncompatibleProjectError': + return 403; + case 'OperationDeniedError': + return 403; + case 'NotFoundError': + return 404; + case 'NameExistsError': + return 409; + case 'FeatureHasTagError': + return 409; + case 'RoleInUseError': + return 400; + case 'ProjectWithoutOwnerError': + return 409; + case 'UnknownError': + return 500; + case 'InternalError': + return 500; + case 'PasswordMismatch': + return 401; + case 'UnauthorizedError': + return 401; + case 'DisabledError': + return 422; + case 'NotImplementedError': + return 405; + case 'NoAccessError': + return 403; + case 'AuthenticationRequired': + return 401; + case 'ForbiddenError': + return 403; + case 'PermissionError': + return 403; + case 'BadRequestError': //thrown by express; do not remove + return 400; + default: + return 500; + } +}; + export const fromLegacyError = (e: Error): UnleashError => { if (e instanceof UnleashError) { return e; @@ -15,10 +78,13 @@ export const fromLegacyError = (e: Error): UnleashError => { ? (e.name as UnleashApiErrorName) : 'UnknownError'; + const statusCode = getStatusCode(name); + if (name === 'NoAccessError') { return new GenericUnleashError({ name: 'NoAccessError', message: e.message, + statusCode, }); } @@ -30,6 +96,7 @@ export const fromLegacyError = (e: Error): UnleashError => { return new GenericUnleashError({ name: 'BadDataError', message: e.message, + statusCode, }); } @@ -37,6 +104,7 @@ export const fromLegacyError = (e: Error): UnleashError => { return new GenericUnleashError({ name: 'OwaspValidationError', message: e.message, + statusCode, }); } @@ -44,11 +112,13 @@ export const fromLegacyError = (e: Error): UnleashError => { return new GenericUnleashError({ name: 'AuthenticationRequired', message: `You must be authenticated to view this content. Please log in.`, + statusCode, }); } return new GenericUnleashError({ name: name as UnleashApiErrorName, message: e.message, + statusCode, }); }; diff --git a/src/lib/error/incompatible-project-error.ts b/src/lib/error/incompatible-project-error.ts index 1888e21262..a84118e162 100644 --- a/src/lib/error/incompatible-project-error.ts +++ b/src/lib/error/incompatible-project-error.ts @@ -1,6 +1,8 @@ import { ApiErrorSchema, UnleashError } from './unleash-error'; export default class IncompatibleProjectError extends UnleashError { + statusCode = 403; + constructor(targetProject: string) { super(`${targetProject} is not a compatible target`); } diff --git a/src/lib/error/invalid-operation-error.ts b/src/lib/error/invalid-operation-error.ts index 72bd09c89e..741bfed478 100644 --- a/src/lib/error/invalid-operation-error.ts +++ b/src/lib/error/invalid-operation-error.ts @@ -1,5 +1,7 @@ import { UnleashError } from './unleash-error'; -class InvalidOperationError extends UnleashError {} +class InvalidOperationError extends UnleashError { + statusCode = 403; +} export default InvalidOperationError; module.exports = InvalidOperationError; diff --git a/src/lib/error/invalid-token-error.ts b/src/lib/error/invalid-token-error.ts index 00afa79577..a62b82b925 100644 --- a/src/lib/error/invalid-token-error.ts +++ b/src/lib/error/invalid-token-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; class InvalidTokenError extends UnleashError { + statusCode = 401; + constructor() { super('Token was not valid'); } diff --git a/src/lib/error/minimum-one-environment-error.ts b/src/lib/error/minimum-one-environment-error.ts index 5d83741160..dac6f122f1 100644 --- a/src/lib/error/minimum-one-environment-error.ts +++ b/src/lib/error/minimum-one-environment-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; -class MinimumOneEnvironmentError extends UnleashError {} +class MinimumOneEnvironmentError extends UnleashError { + statusCode = 400; +} export default MinimumOneEnvironmentError; module.exports = MinimumOneEnvironmentError; diff --git a/src/lib/error/name-exists-error.ts b/src/lib/error/name-exists-error.ts index 9d1423779f..34b652d9a3 100644 --- a/src/lib/error/name-exists-error.ts +++ b/src/lib/error/name-exists-error.ts @@ -1,5 +1,7 @@ import { UnleashError } from './unleash-error'; -class NameExistsError extends UnleashError {} +class NameExistsError extends UnleashError { + statusCode = 409; +} export default NameExistsError; module.exports = NameExistsError; diff --git a/src/lib/error/not-implemented-error.ts b/src/lib/error/not-implemented-error.ts index 64e1b663ee..a1df5c8a77 100644 --- a/src/lib/error/not-implemented-error.ts +++ b/src/lib/error/not-implemented-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; -class NotImplementedError extends UnleashError {} +class NotImplementedError extends UnleashError { + statusCode = 405; +} export default NotImplementedError; module.exports = NotImplementedError; diff --git a/src/lib/error/notfound-error.ts b/src/lib/error/notfound-error.ts index eaef4bc25a..f5cb1d103d 100644 --- a/src/lib/error/notfound-error.ts +++ b/src/lib/error/notfound-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; class NotFoundError extends UnleashError { + statusCode = 404; + constructor(message: string = 'The requested resource could not be found') { super(message); } diff --git a/src/lib/error/operation-denied-error.ts b/src/lib/error/operation-denied-error.ts index 222216a2f6..3efa89003b 100644 --- a/src/lib/error/operation-denied-error.ts +++ b/src/lib/error/operation-denied-error.ts @@ -1,3 +1,5 @@ import { UnleashError } from './unleash-error'; -export class OperationDeniedError extends UnleashError {} +export class OperationDeniedError extends UnleashError { + statusCode = 403; +} diff --git a/src/lib/error/owasp-validation-error.ts b/src/lib/error/owasp-validation-error.ts index ec950cc46e..42390a7eb9 100644 --- a/src/lib/error/owasp-validation-error.ts +++ b/src/lib/error/owasp-validation-error.ts @@ -7,6 +7,8 @@ type ValidationError = { }; class OwaspValidationError extends UnleashError { + statusCode = 400; + private details: [ValidationError]; constructor(testResult: TestResult) { diff --git a/src/lib/error/password-mismatch.ts b/src/lib/error/password-mismatch.ts index 056baf299b..c6488ad370 100644 --- a/src/lib/error/password-mismatch.ts +++ b/src/lib/error/password-mismatch.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; class PasswordMismatch extends UnleashError { + statusCode = 401; + constructor(message: string = 'Wrong password, try again.') { super(message); } diff --git a/src/lib/error/password-undefined.ts b/src/lib/error/password-undefined.ts index 8a019b07fa..9258e27e81 100644 --- a/src/lib/error/password-undefined.ts +++ b/src/lib/error/password-undefined.ts @@ -1,6 +1,8 @@ import { ApiErrorSchema, UnleashError } from './unleash-error'; export default class PasswordUndefinedError extends UnleashError { + statusCode = 400; + constructor() { super('Password cannot be empty or undefined'); } diff --git a/src/lib/error/permission-error.ts b/src/lib/error/permission-error.ts index 2cbf50bc32..7b17064c1b 100644 --- a/src/lib/error/permission-error.ts +++ b/src/lib/error/permission-error.ts @@ -3,6 +3,8 @@ import { ApiErrorSchema, UnleashError } from './unleash-error'; type Permission = string | string[]; class PermissionError extends UnleashError { + statusCode = 403; + permissions: Permission; constructor(permission: Permission = [], environment?: string) { diff --git a/src/lib/error/project-without-owner-error.ts b/src/lib/error/project-without-owner-error.ts index b38ee5ba62..638543639a 100644 --- a/src/lib/error/project-without-owner-error.ts +++ b/src/lib/error/project-without-owner-error.ts @@ -1,6 +1,8 @@ import { ApiErrorSchema, UnleashError } from './unleash-error'; export default class ProjectWithoutOwnerError extends UnleashError { + statusCode = 409; + constructor() { super('A project must have at least one owner'); } diff --git a/src/lib/error/role-in-use-error.ts b/src/lib/error/role-in-use-error.ts index 922d7d182e..4557a92908 100644 --- a/src/lib/error/role-in-use-error.ts +++ b/src/lib/error/role-in-use-error.ts @@ -1,5 +1,7 @@ import { UnleashError } from './unleash-error'; -class RoleInUseError extends UnleashError {} +class RoleInUseError extends UnleashError { + statusCode = 400; +} export default RoleInUseError; diff --git a/src/lib/error/unauthorized-error.ts b/src/lib/error/unauthorized-error.ts index dbbf9c2318..a1d027a2f6 100644 --- a/src/lib/error/unauthorized-error.ts +++ b/src/lib/error/unauthorized-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; -class UnauthorizedError extends UnleashError {} +class UnauthorizedError extends UnleashError { + statusCode = 401; +} export default UnauthorizedError; module.exports = UnauthorizedError; diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts index 331a37ec75..2f83bee693 100644 --- a/src/lib/error/unleash-error.ts +++ b/src/lib/error/unleash-error.ts @@ -35,75 +35,12 @@ export const UnleashApiErrorTypes = [ export type UnleashApiErrorName = typeof UnleashApiErrorTypes[number]; -const statusCode = (errorName: string): number => { - switch (errorName) { - case 'ContentTypeError': - return 415; - case 'ValidationError': - return 400; - case 'BadDataError': - return 400; - case 'OwaspValidationError': - return 400; - case 'PasswordUndefinedError': - return 400; - case 'MinimumOneEnvironmentError': - return 400; - case 'InvalidTokenError': - return 401; - case 'NoAccessError': - return 403; - case 'UsedTokenError': - return 403; - case 'InvalidOperationError': - return 403; - case 'IncompatibleProjectError': - return 403; - case 'OperationDeniedError': - return 403; - case 'NotFoundError': - return 404; - case 'NameExistsError': - return 409; - case 'FeatureHasTagError': - return 409; - case 'RoleInUseError': - return 400; - case 'ProjectWithoutOwnerError': - return 409; - case 'UnknownError': - return 500; - case 'InternalError': - return 500; - case 'PasswordMismatch': - return 401; - case 'UnauthorizedError': - return 401; - case 'DisabledError': - return 422; - case 'NotImplementedError': - return 405; - case 'NoAccessError': - return 403; - case 'AuthenticationRequired': - return 401; - case 'ForbiddenError': - return 403; - case 'PermissionError': - return 403; - case 'BadRequestError': //thrown by express; do not remove - return 400; - default: - return 500; - } -}; - export abstract class UnleashError extends Error { id: string; name: string; - statusCode: number; + abstract statusCode: number; additionalParameters: object; @@ -112,8 +49,6 @@ export abstract class UnleashError extends Error { this.id = uuidV4(); this.name = name || this.constructor.name; super.message = message; - - this.statusCode = statusCode(this.name); } help(): string { @@ -135,14 +70,19 @@ export abstract class UnleashError extends Error { } export class GenericUnleashError extends UnleashError { + statusCode: number; + constructor({ name, message, + statusCode, }: { name: UnleashApiErrorName; message: string; + statusCode: number; }) { super(message, name); + this.statusCode = statusCode; } } diff --git a/src/lib/error/used-token-error.ts b/src/lib/error/used-token-error.ts index 8493828670..9b16de9ebe 100644 --- a/src/lib/error/used-token-error.ts +++ b/src/lib/error/used-token-error.ts @@ -1,6 +1,8 @@ import { UnleashError } from './unleash-error'; class UsedTokenError extends UnleashError { + statusCode = 403; + constructor(usedAt: Date) { super(`Token was already used at ${usedAt}`); } diff --git a/src/lib/types/authentication-required.ts b/src/lib/types/authentication-required.ts index b9887ac165..1b3a0082c3 100644 --- a/src/lib/types/authentication-required.ts +++ b/src/lib/types/authentication-required.ts @@ -12,6 +12,8 @@ interface IOptions extends IBaseOptions { } class AuthenticationRequired extends UnleashError { + statusCode = 401; + private type: string; private path: string;