diff --git a/src/lib/error/bad-data-error.ts b/src/lib/error/bad-data-error.ts index b9c2ff358f..2c1f61aec9 100644 --- a/src/lib/error/bad-data-error.ts +++ b/src/lib/error/bad-data-error.ts @@ -1,4 +1,5 @@ import { ErrorObject } from 'ajv'; +import { ValidationError } from 'joi'; import getProp from 'lodash.get'; import { ApiErrorSchema, UnleashError } from './unleash-error'; @@ -17,7 +18,7 @@ class BadDataError extends UnleashError { const topLevelMessage = 'Request validation failed: your request body contains invalid data' + (errors - ? '. Refer to the `details` list to see what happened.' + ? '. Refer to the `details` list for more information.' : `: ${message}`); super(topLevelMessage); @@ -121,3 +122,29 @@ export const fromOpenApiValidationErrors = ( [firstDetail, ...remainingDetails], ); }; + +export const fromJoiError = (err: ValidationError): BadDataError => { + const details = err.details.map((detail) => { + const messageEnd = detail.context?.value + ? `. You provided ${JSON.stringify(detail.context.value)}.` + : '.'; + const description = detail.message + messageEnd; + return { + description, + message: description, + }; + }); + + const [first, ...rest] = details; + + if (first) { + return new BadDataError( + 'A validation error occurred while processing your request data. Refer to the `details` property for more information.', + [first, ...rest], + ); + } else { + return new BadDataError( + 'A validation error occurred while processing your request data. Please make sure it conforms to the request data schema.', + ); + } +}; diff --git a/src/lib/error/from-legacy-error.ts b/src/lib/error/from-legacy-error.ts new file mode 100644 index 0000000000..c90e09b076 --- /dev/null +++ b/src/lib/error/from-legacy-error.ts @@ -0,0 +1,54 @@ +import { fromJoiError } from './bad-data-error'; +import { ValidationError as JoiValidationError } from 'joi'; +import { + GenericUnleashError, + UnleashApiErrorName, + UnleashApiErrorTypes, + UnleashError, +} from './unleash-error'; + +export const fromLegacyError = (e: Error): UnleashError => { + if (e instanceof UnleashError) { + return e; + } + const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName) + ? (e.name as UnleashApiErrorName) + : 'UnknownError'; + + if (name === 'NoAccessError') { + return new GenericUnleashError({ + name: 'NoAccessError', + message: e.message, + }); + } + + if (e instanceof JoiValidationError) { + return fromJoiError(e); + } + + if (name === 'ValidationError' || name === 'BadDataError') { + return new GenericUnleashError({ + name: 'BadDataError', + message: e.message, + }); + } + + if (name === 'OwaspValidationError') { + return new GenericUnleashError({ + name: 'OwaspValidationError', + message: e.message, + }); + } + + if (name === 'AuthenticationRequired') { + return new GenericUnleashError({ + name: 'AuthenticationRequired', + message: `You must be authenticated to view this content. Please log in.`, + }); + } + + return new GenericUnleashError({ + name: name as UnleashApiErrorName, + message: e.message, + }); +}; diff --git a/src/lib/error/unleash-error.test.ts b/src/lib/error/unleash-error.test.ts index 3369d10840..5f6a03f4cd 100644 --- a/src/lib/error/unleash-error.test.ts +++ b/src/lib/error/unleash-error.test.ts @@ -1,7 +1,7 @@ import owasp from 'owasp-password-strength-test'; import { ErrorObject } from 'ajv'; import AuthenticationRequired from '../types/authentication-required'; -import { ApiErrorSchema, fromLegacyError } from './unleash-error'; +import { ApiErrorSchema } from './unleash-error'; import BadDataError, { fromOpenApiValidationError, fromOpenApiValidationErrors, @@ -12,6 +12,8 @@ import IncompatibleProjectError from './incompatible-project-error'; import PasswordUndefinedError from './password-undefined'; import ProjectWithoutOwnerError from './project-without-owner-error'; import NotFoundError from './notfound-error'; +import { validateString } from '../util/validators/constraint-types'; +import { fromLegacyError } from './from-legacy-error'; describe('v5 deprecation: backwards compatibility', () => { it(`Adds details to error types that don't specify it`, () => { @@ -352,6 +354,32 @@ describe('Error serialization special cases', () => { ], }); }); + + it('Converts Joi errors in a sensible fashion', async () => { + // if the validation doesn't fail, this test does nothing, so ensure + // that an error is thrown. + let validationThrewAnError = false; + try { + await validateString([]); + } catch (e) { + validationThrewAnError = true; + const convertedError = fromLegacyError(e); + + expect(convertedError.toJSON()).toMatchObject({ + message: + expect.stringContaining('validation error') && + expect.stringContaining('details'), + details: [ + { + description: + '"value" must contain at least 1 items. You provided [].', + }, + ], + }); + } + + expect(validationThrewAnError).toBeTruthy(); + }); }); describe('Error serialization special cases', () => { diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts index 8356c87a82..d8ccfafa54 100644 --- a/src/lib/error/unleash-error.ts +++ b/src/lib/error/unleash-error.ts @@ -1,7 +1,7 @@ import { v4 as uuidV4 } from 'uuid'; import { FromSchema } from 'json-schema-to-ts'; -const UnleashApiErrorTypes = [ +export const UnleashApiErrorTypes = [ 'ContentTypeError', 'DisabledError', 'FeatureHasTagError', @@ -32,7 +32,7 @@ const UnleashApiErrorTypes = [ 'InternalError', ] as const; -type UnleashApiErrorName = typeof UnleashApiErrorTypes[number]; +export type UnleashApiErrorName = typeof UnleashApiErrorTypes[number]; const statusCode = (errorName: string): number => { switch (errorName) { @@ -127,7 +127,7 @@ export abstract class UnleashError extends Error { } } -class GenericUnleashError extends UnleashError { +export class GenericUnleashError extends UnleashError { constructor({ name, message, @@ -168,46 +168,4 @@ export const apiErrorSchema = { components: {}, } as const; -export const fromLegacyError = (e: Error): UnleashError => { - if (e instanceof UnleashError) { - return e; - } - const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName) - ? (e.name as UnleashApiErrorName) - : 'UnknownError'; - - if (name === 'NoAccessError') { - return new GenericUnleashError({ - name: 'NoAccessError', - message: e.message, - }); - } - - if (name === 'ValidationError' || name === 'BadDataError') { - return new GenericUnleashError({ - name: 'BadDataError', - message: e.message, - }); - } - - if (name === 'OwaspValidationError') { - return new GenericUnleashError({ - name: 'OwaspValidationError', - message: e.message, - }); - } - - if (name === 'AuthenticationRequired') { - return new GenericUnleashError({ - name: 'AuthenticationRequired', - message: `You must be authenticated to view this content. Please log in.`, - }); - } - - return new GenericUnleashError({ - name: name as UnleashApiErrorName, - message: e.message, - }); -}; - export type ApiErrorSchema = FromSchema; diff --git a/src/lib/routes/admin-api/strategy.test.ts b/src/lib/routes/admin-api/strategy.test.ts index f1d13402f3..62542d33bb 100644 --- a/src/lib/routes/admin-api/strategy.test.ts +++ b/src/lib/routes/admin-api/strategy.test.ts @@ -68,7 +68,7 @@ test('require parameters array when creating a new strategy', async () => { .send({ name: 'TestStrat' }) .expect(400) .expect((res) => { - expect(res.body.details[0].description).toEqual( + expect(res.body.details[0].description).toMatch( '"parameters" is required', ); }); diff --git a/src/lib/routes/util.ts b/src/lib/routes/util.ts index 207eb5c047..e02f8591f5 100644 --- a/src/lib/routes/util.ts +++ b/src/lib/routes/util.ts @@ -1,7 +1,8 @@ import joi from 'joi'; import { Response } from 'express'; import { Logger } from '../logger'; -import { fromLegacyError, UnleashError } from '../error/unleash-error'; +import { UnleashError } from '../error/unleash-error'; +import { fromLegacyError } from '../error/from-legacy-error'; export const customJoi = joi.extend((j) => ({ type: 'isUrlFriendly', diff --git a/src/test/e2e/api/admin/tag-types.e2e.test.ts b/src/test/e2e/api/admin/tag-types.e2e.test.ts index 6f1173a1ce..926d5baa7a 100644 --- a/src/test/e2e/api/admin/tag-types.e2e.test.ts +++ b/src/test/e2e/api/admin/tag-types.e2e.test.ts @@ -77,7 +77,7 @@ test('Invalid tag types gets rejected', async () => { .set('Content-Type', 'application/json') .expect(400) .expect((res) => { - expect(res.body.details[0].description).toBe( + expect(res.body.details[0].description).toMatch( '"name" must be URL friendly', ); }); @@ -151,7 +151,7 @@ test('Invalid tag-types get refused by validator', async () => { .set('Content-Type', 'application/json') .expect(400) .expect((res) => { - expect(res.body.details[0].description).toBe( + expect(res.body.details[0].description).toMatch( '"name" must be URL friendly', ); }); diff --git a/src/test/e2e/api/admin/tags.e2e.test.ts b/src/test/e2e/api/admin/tags.e2e.test.ts index 0e76428cce..fc13372ff0 100644 --- a/src/test/e2e/api/admin/tags.e2e.test.ts +++ b/src/test/e2e/api/admin/tags.e2e.test.ts @@ -86,7 +86,7 @@ test('Can validate a tag', async () => .expect(400) .expect((res) => { expect(res.body.details.length).toBe(1); - expect(res.body.details[0].description).toBe( + expect(res.body.details[0].description).toMatch( '"type" must be URL friendly', ); }));