diff --git a/src/lib/error/bad-data-error.ts b/src/lib/error/bad-data-error.ts index d5dd4792b2..3fc85e25aa 100644 --- a/src/lib/error/bad-data-error.ts +++ b/src/lib/error/bad-data-error.ts @@ -83,27 +83,46 @@ const genericErrorMessage = ( }; }; +const oneOfMessage = ( + propertyName: string, + errorMessage: string = 'is invalid', +) => { + const errorPosition = + propertyName === '' ? 'root object' : `${propertyName} property`; + + const description = `The ${errorPosition} ${errorMessage}. The data you provided matches more than one option in the schema. These options are mutually exclusive. Please refer back to the schema and remove any excess properties.`; + + return { + description, + message: description, + path: propertyName, + }; +}; + export const fromOpenApiValidationError = (requestBody: object) => (validationError: ErrorObject): ValidationErrorDescription => { // @ts-expect-error Unsure why, but the `dataPath` isn't listed on the type definition for error objects. However, it's always there. Suspect this is a bug in the library. - const dataPath = validationError.dataPath.substring('.body.'.length); + const dataPath = validationError.dataPath; + const propertyName = dataPath.substring('.body.'.length); switch (validationError.keyword) { case 'required': return missingRequiredPropertyMessage( - dataPath, + propertyName, validationError.params.missingProperty, ); case 'additionalProperties': return additionalPropertiesMessage( - dataPath, + propertyName, validationError.params.additionalProperty, ); + case 'oneOf': + return oneOfMessage(propertyName, validationError.message); default: return genericErrorMessage( requestBody, - dataPath, + propertyName, validationError.message, ); } diff --git a/src/lib/error/unleash-error.test.ts b/src/lib/error/unleash-error.test.ts index 85027a511b..f122cb15a8 100644 --- a/src/lib/error/unleash-error.test.ts +++ b/src/lib/error/unleash-error.test.ts @@ -99,6 +99,46 @@ describe('OpenAPI error conversion', () => { }); }); + it.each(['.body', '.body.subObject'])( + 'Gives useful error messages for oneOf errors in %s', + (dataPath) => { + const error = { + keyword: 'oneOf', + instancePath: '', + dataPath, + schemaPath: '#/components/schemas/createApiTokenSchema/oneOf', + params: { + passingSchemas: null, + }, + message: 'should match exactly one schema in oneOf', + }; + + const result = fromOpenApiValidationError({ + secret: 'blah', + username: 'string2', + type: 'admin', + })(error); + + expect(result).toMatchObject({ + description: + // it provides the message + expect.stringContaining(error.message), + path: dataPath.substring('.body.'.length), + }); + + // it tells the user what happened + expect(result.description).toContain( + 'matches more than one option', + ); + // it tells the user what part of the request body this pertains to + expect(result.description).toContain( + dataPath === '.body' + ? 'root object' + : `${dataPath.substring('.body.'.length)} property`, + ); + }, + ); + it('Gives useful pattern error messages', () => { const error = { instancePath: '', diff --git a/src/lib/openapi/meta-schema-rules.test.ts b/src/lib/openapi/meta-schema-rules.test.ts index 1aa0c73622..a198fb8c79 100644 --- a/src/lib/openapi/meta-schema-rules.test.ts +++ b/src/lib/openapi/meta-schema-rules.test.ts @@ -93,7 +93,6 @@ const metaRules: Rule[] = [ 'batchFeaturesSchema', 'batchStaleSchema', 'cloneFeatureSchema', - 'createApiTokenSchema', 'createFeatureSchema', 'createInvitedUserSchema', 'environmentsSchema', @@ -136,7 +135,6 @@ const metaRules: Rule[] = [ 'tagTypesSchema', 'tagWithVersionSchema', 'uiConfigSchema', - 'updateApiTokenSchema', 'updateFeatureSchema', 'updateFeatureStrategySchema', 'updateTagTypeSchema', @@ -166,7 +164,6 @@ const metaRules: Rule[] = [ 'batchFeaturesSchema', 'batchStaleSchema', 'cloneFeatureSchema', - 'createApiTokenSchema', 'createFeatureSchema', 'createFeatureStrategySchema', 'createInvitedUserSchema', @@ -211,7 +208,6 @@ const metaRules: Rule[] = [ 'tagTypesSchema', 'tagWithVersionSchema', 'uiConfigSchema', - 'updateApiTokenSchema', 'updateFeatureSchema', 'updateFeatureStrategySchema', 'updateTagTypeSchema', diff --git a/src/lib/openapi/spec/api-tokens-schema.ts b/src/lib/openapi/spec/api-tokens-schema.ts index e4d4ca1b98..843688395f 100644 --- a/src/lib/openapi/spec/api-tokens-schema.ts +++ b/src/lib/openapi/spec/api-tokens-schema.ts @@ -4,16 +4,17 @@ import { apiTokenSchema } from './api-token-schema'; export const apiTokensSchema = { $id: '#/components/schemas/apiTokensSchema', type: 'object', + description: + 'An object with [Unleash API tokens](https://docs.getunleash.io/reference/api-tokens-and-client-keys)', additionalProperties: false, required: ['tokens'], - description: 'Contains a list of API tokens.', properties: { tokens: { type: 'array', + description: 'A list of Unleash API tokens.', items: { $ref: '#/components/schemas/apiTokenSchema', }, - description: 'A list of API tokens.', }, }, components: { diff --git a/src/lib/openapi/spec/create-api-token-schema.ts b/src/lib/openapi/spec/create-api-token-schema.ts index 4a6c65f114..150c06d4c3 100644 --- a/src/lib/openapi/spec/create-api-token-schema.ts +++ b/src/lib/openapi/spec/create-api-token-schema.ts @@ -1,5 +1,78 @@ import { FromSchema } from 'json-schema-to-ts'; -import { ApiTokenType } from '../../types/models/api-token'; + +const adminSchema = { + required: ['type'], + type: 'object', + properties: { + type: { + type: 'string', + pattern: '^[Aa][Dd][Mm][Ii][Nn]$', + description: `An admin token. Must be the string "admin" (not case sensitive).`, + example: 'admin', + }, + }, +} as const; + +const tokenNameSchema = { + type: 'object', + required: ['tokenName'], + properties: { + tokenName: { + type: 'string', + description: 'The name of the token.', + example: 'token-64522', + }, + }, +} as const; + +const usernameSchema = { + type: 'object', + required: ['username'], + properties: { + username: { + deprecated: true, + type: 'string', + description: + 'The name of the token. This property is deprecated. Use `tokenName` instead.', + example: 'token-64523', + }, + }, +} as const; + +const clientFrontendSchema = { + required: ['type'], + type: 'object', + properties: { + type: { + type: 'string', + pattern: + '^([Cc][Ll][Ii][Ee][Nn][Tt]|[Ff][Rr][Oo][Nn][Tt][Ee][Nn][Dd])$', + description: `A client or frontend token. Must be one of the strings "client" or "frontend" (not case sensitive).`, + example: 'frontend', + }, + environment: { + type: 'string', + description: + 'The environment that the token should be valid for. Defaults to "default"', + example: 'development', + }, + project: { + type: 'string', + description: + 'The project that the token should be valid for. Defaults to "*" meaning every project. This property is mutually incompatible with the `projects` property. If you specify one, you cannot specify the other.', + example: 'project-851', + }, + projects: { + type: 'array', + description: + 'A list of projects that the token should be valid for. This property is mutually incompatible with the `project` property. If you specify one, you cannot specify the other.', + example: ['project-851', 'project-852'], + items: { + type: 'string', + }, + }, + }, +} as const; // TODO: (openapi) this schema isn't entirely correct: `project` and `projects` // are mutually exclusive. @@ -7,62 +80,34 @@ import { ApiTokenType } from '../../types/models/api-token'; // That is, when creating a token, you can provide either `project` _or_ // `projects`, but *not* both. // -// We should be able to annotate this using `oneOf` and `allOf`, but making -// `oneOf` only valid for _either_ `project` _or_ `projects` is tricky. -// -// I've opened an issue to get some help (thought it was a bug initially). -// There's more info available at: -// -// https://github.com/ajv-validator/ajv/issues/2096 -// -// This also applies to apiTokenSchema and potentially other related schemas. - +// Because we allow additional properties, we cannot express the mutual +// exclusiveness in the schema (with OpenAPI 3.0). As such, it's mentioned in +// the description for now. export const createApiTokenSchema = { $id: '#/components/schemas/createApiTokenSchema', type: 'object', - required: ['type'], + description: + 'The data required to create an [Unleash API token](https://docs.getunleash.io/reference/api-tokens-and-client-keys).', properties: { - secret: { - type: 'string', - }, - type: { - type: 'string', - description: `One of ${Object.values(ApiTokenType).join(', ')}`, - }, - environment: { - type: 'string', - }, - project: { - type: 'string', - }, - projects: { - type: 'array', - items: { - type: 'string', - }, - }, expiresAt: { type: 'string', format: 'date-time', - nullable: true, + description: 'The time when this token should expire.', + example: '2023-07-04T11:26:24+02:00', }, }, - anyOf: [ + oneOf: [ { - properties: { - username: { - type: 'string', - }, - }, - required: ['username'], + allOf: [adminSchema, tokenNameSchema], }, { - properties: { - tokenName: { - type: 'string', - }, - }, - required: ['tokenName'], + allOf: [adminSchema, usernameSchema], + }, + { + allOf: [clientFrontendSchema, tokenNameSchema], + }, + { + allOf: [clientFrontendSchema, usernameSchema], }, ], components: {}, diff --git a/src/lib/openapi/spec/update-api-token-schema.ts b/src/lib/openapi/spec/update-api-token-schema.ts index 13a9d70ebe..c4e1160c4c 100644 --- a/src/lib/openapi/spec/update-api-token-schema.ts +++ b/src/lib/openapi/spec/update-api-token-schema.ts @@ -4,8 +4,11 @@ export const updateApiTokenSchema = { $id: '#/components/schemas/updateApiTokenSchema', type: 'object', required: ['expiresAt'], + description: 'An object with fields to updated for a given API token.', properties: { expiresAt: { + description: 'The new time when this token should expire.', + example: '2023-09-04T11:26:24+02:00', type: 'string', format: 'date-time', }, diff --git a/src/lib/openapi/util/standard-responses.ts b/src/lib/openapi/util/standard-responses.ts index dc8e8c4441..39a81e95d0 100644 --- a/src/lib/openapi/util/standard-responses.ts +++ b/src/lib/openapi/util/standard-responses.ts @@ -34,7 +34,7 @@ const unauthorizedResponse = { const forbiddenResponse = { description: - 'User credentials are valid but does not have enough privileges to execute this operation', + 'The provided user credentials are valid, but the user does not have the necessary permissions to perform this operation', content: { 'application/json': { schema: { diff --git a/src/lib/routes/admin-api/api-token.ts b/src/lib/routes/admin-api/api-token.ts index 8281cd2b23..fca1b3a753 100644 --- a/src/lib/routes/admin-api/api-token.ts +++ b/src/lib/routes/admin-api/api-token.ts @@ -37,7 +37,10 @@ import { ApiTokenSchema, } from '../../openapi/spec/api-token-schema'; import { UpdateApiTokenSchema } from '../../openapi/spec/update-api-token-schema'; -import { emptyResponse } from '../../openapi/util/standard-responses'; +import { + emptyResponse, + getStandardResponses, +} from '../../openapi/util/standard-responses'; import { ProxyService } from '../../services/proxy-service'; import { extractUsername } from '../../util'; import { OperationDeniedError } from '../../error'; @@ -154,8 +157,12 @@ export class ApiTokenController extends Controller { openApiService.validPath({ tags: ['API tokens'], operationId: 'getAllApiTokens', + summary: 'Get API tokens', + description: + 'Retrieves all API tokens that exist in the Unleash instance.', responses: { 200: createResponseSchema('apiTokensSchema'), + ...getStandardResponses(401, 403), }, }), ], @@ -175,8 +182,13 @@ export class ApiTokenController extends Controller { tags: ['API tokens'], operationId: 'createApiToken', requestBody: createRequestSchema('createApiTokenSchema'), + summary: 'Create API token', + description: `Create an API token of a specific type: one of ${Object.values( + ApiTokenType, + ).join(', ')}.`, responses: { 201: resourceCreatedResponseSchema('apiTokenSchema'), + ...getStandardResponses(401, 403, 415), }, }), ], @@ -195,9 +207,13 @@ export class ApiTokenController extends Controller { openApiService.validPath({ tags: ['API tokens'], operationId: 'updateApiToken', + summary: 'Update API token', + description: + "Updates an existing API token with a new expiry date. The `token` path parameter is the token's `secret`. If the token does not exist, this endpoint returns a 200 OK, but does nothing.", requestBody: createRequestSchema('updateApiTokenSchema'), responses: { 200: emptyResponse, + ...getStandardResponses(401, 403, 415), }, }), ], @@ -216,9 +232,13 @@ export class ApiTokenController extends Controller { middleware: [ openApiService.validPath({ tags: ['API tokens'], + summary: 'Delete API token', + description: + "Deletes an existing API token. The `token` path parameter is the token's `secret`. If the token does not exist, this endpoint returns a 200 OK, but does nothing.", operationId: 'deleteApiToken', responses: { 200: emptyResponse, + ...getStandardResponses(401, 403), }, }), ], diff --git a/src/test/e2e/api/admin/api-token.e2e.test.ts b/src/test/e2e/api/admin/api-token.e2e.test.ts index 0377a2997c..900b883495 100644 --- a/src/test/e2e/api/admin/api-token.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.e2e.test.ts @@ -1,4 +1,4 @@ -import { setupApp } from '../../helpers/test-helper'; +import { setupAppWithCustomConfig } from '../../helpers/test-helper'; import dbInit from '../../helpers/database-init'; import getLogger from '../../../fixtures/no-logger'; import { ALL, ApiTokenType } from '../../../../lib/types/models/api-token'; @@ -10,7 +10,13 @@ let app; beforeAll(async () => { db = await dbInit('token_api_serial', getLogger); - app = await setupApp(db.stores); + app = await setupAppWithCustomConfig(db.stores, { + experimental: { + flags: { + strictSchemaValidation: true, + }, + }, + }); }); afterAll(async () => { diff --git a/src/test/e2e/api/openapi/openapi.e2e.test.ts b/src/test/e2e/api/openapi/openapi.e2e.test.ts index 575db6c55c..035dd30a70 100644 --- a/src/test/e2e/api/openapi/openapi.e2e.test.ts +++ b/src/test/e2e/api/openapi/openapi.e2e.test.ts @@ -94,7 +94,15 @@ test('the generated OpenAPI spec is valid', async () => { console.error(enforcerError); } - expect(enforcerWarning ?? enforcerError).toBe(undefined); + const enforcerResults = { + warnings: enforcerWarning?.toString(), + errors: enforcerError?.toString(), + }; + + expect(enforcerResults).toMatchObject({ + warnings: undefined, + errors: undefined, + }); }); test('all root-level tags are "approved tags"', async () => {