From f3ab3a16d031312c904ceb1db474ef4e0f9b5199 Mon Sep 17 00:00:00 2001 From: Sid Date: Mon, 4 Nov 2024 16:57:45 +0100 Subject: [PATCH] [Authz] Fix description generation for Open API spec for an API (#198054) Closes https://github.com/elastic/kibana/issues/198058. Adds a fix for https://github.com/elastic/kibana/pull/197001 ## Summary There was an error in how descriptions were added to the Open API spec for a given route - for the specific case when both a route description and security authz required privileges were present. The code with the error is: https://github.com/elastic/kibana/pull/197001/files#diff-5942307fac5a7b321e7f317bacd2837a7f766f3e79d5aad285513b1f82951b46R79-R80 This PR fixes that error. Also updated: Description field for required privileges now includes a more intuitive descriptor: `Required authorization` as well as a line break. image --------- Co-authored-by: Elastic Machine Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit b12e7d0e79af8150ea9f2b5940a6ad1d428cff72) --- oas_docs/bundle.json | 4 +-- oas_docs/output/kibana.yaml | 7 +++-- .../src/extract_authz_description.test.ts | 12 +++++--- .../src/extract_authz_description.ts | 2 +- .../src/process_router.test.ts | 28 +++++++++++++++++-- .../src/process_router.ts | 7 +++-- .../src/process_versioned_router.test.ts | 3 +- .../src/process_versioned_router.ts | 8 +++--- 8 files changed, 52 insertions(+), 19 deletions(-) diff --git a/oas_docs/bundle.json b/oas_docs/bundle.json index 18f21be9432a6..95965f1f96804 100644 --- a/oas_docs/bundle.json +++ b/oas_docs/bundle.json @@ -6934,7 +6934,7 @@ }, "/api/spaces/_copy_saved_objects": { "post": { - "description": "It also allows you to automatically copy related objects, so when you copy a dashboard, this can automatically copy over the associated visualizations, data views, and saved searches, as required. You can request to overwrite any objects that already exist in the target space if they share an identifier or you can use the resolve copy saved objects conflicts API to do this on a per-object basis.", + "description": "It also allows you to automatically copy related objects, so when you copy a dashboard, this can automatically copy over the associated visualizations, data views, and saved searches, as required. You can request to overwrite any objects that already exist in the target space if they share an identifier or you can use the resolve copy saved objects conflicts API to do this on a per-object basis.

[Required authorization] Route required privileges: ALL of [copySavedObjectsToSpaces].", "operationId": "post-spaces-copy-saved-objects", "parameters": [ { @@ -7177,7 +7177,7 @@ }, "/api/spaces/_resolve_copy_saved_objects_errors": { "post": { - "description": "Overwrite saved objects that are returned as errors from the copy saved objects to space API.", + "description": "Overwrite saved objects that are returned as errors from the copy saved objects to space API.

[Required authorization] Route required privileges: ALL of [copySavedObjectsToSpaces].", "operationId": "post-spaces-resolve-copy-saved-objects-errors", "parameters": [ { diff --git a/oas_docs/output/kibana.yaml b/oas_docs/output/kibana.yaml index 4069cd9f30c9a..38b99fdf1018b 100644 --- a/oas_docs/output/kibana.yaml +++ b/oas_docs/output/kibana.yaml @@ -21350,7 +21350,9 @@ paths: visualizations, data views, and saved searches, as required. You can request to overwrite any objects that already exist in the target space if they share an identifier or you can use the resolve copy saved - objects conflicts API to do this on a per-object basis. + objects conflicts API to do this on a per-object + basis.

[Required authorization] Route required privileges: ALL + of [copySavedObjectsToSpaces]. operationId: post-spaces-copy-saved-objects parameters: - description: The version of the API to use @@ -21539,7 +21541,8 @@ paths: post: description: >- Overwrite saved objects that are returned as errors from the copy saved - objects to space API. + objects to space API.

[Required authorization] Route required + privileges: ALL of [copySavedObjectsToSpaces]. operationId: post-spaces-resolve-copy-saved-objects-errors parameters: - description: The version of the API to use diff --git a/packages/kbn-router-to-openapispec/src/extract_authz_description.test.ts b/packages/kbn-router-to-openapispec/src/extract_authz_description.test.ts index 8da2324e68f02..308f0a7686597 100644 --- a/packages/kbn-router-to-openapispec/src/extract_authz_description.test.ts +++ b/packages/kbn-router-to-openapispec/src/extract_authz_description.test.ts @@ -33,7 +33,9 @@ describe('extractAuthzDescription', () => { }, }; const description = extractAuthzDescription(routeSecurity); - expect(description).toBe('[Authz] Route required privileges: ALL of [manage_spaces].'); + expect(description).toBe( + '[Required authorization] Route required privileges: ALL of [manage_spaces].' + ); }); it('should return route authz description for privilege groups', () => { @@ -44,7 +46,9 @@ describe('extractAuthzDescription', () => { }, }; const description = extractAuthzDescription(routeSecurity); - expect(description).toBe('[Authz] Route required privileges: ALL of [console].'); + expect(description).toBe( + '[Required authorization] Route required privileges: ALL of [console].' + ); } { const routeSecurity: RouteSecurity = { @@ -58,7 +62,7 @@ describe('extractAuthzDescription', () => { }; const description = extractAuthzDescription(routeSecurity); expect(description).toBe( - '[Authz] Route required privileges: ANY of [manage_spaces OR taskmanager].' + '[Required authorization] Route required privileges: ANY of [manage_spaces OR taskmanager].' ); } { @@ -74,7 +78,7 @@ describe('extractAuthzDescription', () => { }; const description = extractAuthzDescription(routeSecurity); expect(description).toBe( - '[Authz] Route required privileges: ALL of [console, filesManagement] AND ANY of [manage_spaces OR taskmanager].' + '[Required authorization] Route required privileges: ALL of [console, filesManagement] AND ANY of [manage_spaces OR taskmanager].' ); } }); diff --git a/packages/kbn-router-to-openapispec/src/extract_authz_description.ts b/packages/kbn-router-to-openapispec/src/extract_authz_description.ts index 4cd6875913780..7979188f2641e 100644 --- a/packages/kbn-router-to-openapispec/src/extract_authz_description.ts +++ b/packages/kbn-router-to-openapispec/src/extract_authz_description.ts @@ -56,5 +56,5 @@ export const extractAuthzDescription = (routeSecurity: InternalRouteSecurity | u return `Route required privileges: ${getPrivilegesDescription(allRequired, anyRequired)}.`; }; - return `[Authz] ${getDescriptionForRoute()}`; + return `[Required authorization] ${getDescriptionForRoute()}`; }; diff --git a/packages/kbn-router-to-openapispec/src/process_router.test.ts b/packages/kbn-router-to-openapispec/src/process_router.test.ts index 17191e7ab1b1c..2ce135a378789 100644 --- a/packages/kbn-router-to-openapispec/src/process_router.test.ts +++ b/packages/kbn-router-to-openapispec/src/process_router.test.ts @@ -124,6 +124,26 @@ describe('processRouter', () => { }, }, }, + { + path: '/quux', + method: 'post', + options: { + description: 'This a test route description.', + }, + handler: jest.fn(), + validationSchemas: { request: { body: schema.object({}) } }, + security: { + authz: { + requiredPrivileges: [ + 'manage_spaces', + { + allRequired: ['taskmanager'], + anyRequired: ['console'], + }, + ], + }, + }, + }, ], } as unknown as Router; @@ -132,7 +152,7 @@ describe('processRouter', () => { version: '2023-10-31', }); - expect(Object.keys(result1.paths!)).toHaveLength(4); + expect(Object.keys(result1.paths!)).toHaveLength(5); const result2 = processRouter(testRouter, new OasConverter(), createOpIdGenerator(), { version: '2024-10-31', @@ -148,7 +168,11 @@ describe('processRouter', () => { expect(result.paths['/qux']?.post).toBeDefined(); expect(result.paths['/qux']?.post?.description).toEqual( - '[Authz] Route required privileges: ALL of [manage_spaces, taskmanager] AND ANY of [console].' + '[Required authorization] Route required privileges: ALL of [manage_spaces, taskmanager] AND ANY of [console].' + ); + + expect(result.paths['/quux']?.post?.description).toEqual( + 'This a test route description.

[Required authorization] Route required privileges: ALL of [manage_spaces, taskmanager] AND ANY of [console].' ); }); }); diff --git a/packages/kbn-router-to-openapispec/src/process_router.ts b/packages/kbn-router-to-openapispec/src/process_router.ts index acf6ff443b8f5..35e8f125914a6 100644 --- a/packages/kbn-router-to-openapispec/src/process_router.ts +++ b/packages/kbn-router-to-openapispec/src/process_router.ts @@ -64,11 +64,13 @@ export const processRouter = ( parameters.push(...pathObjects, ...queryObjects); } - let description = ''; + let description = `${route.options.description ?? ''}`; if (route.security) { const authzDescription = extractAuthzDescription(route.security); - description = `${route.options.description ?? ''}${authzDescription ?? ''}`; + description += `${route.options.description && authzDescription ? `

` : ''}${ + authzDescription ?? '' + }`; } const hasDeprecations = !!route.options.deprecated; @@ -76,7 +78,6 @@ export const processRouter = ( summary: route.options.summary ?? '', tags: route.options.tags ? extractTags(route.options.tags) : [], ...(description ? { description } : {}), - ...(route.options.description ? { description: route.options.description } : {}), ...(hasDeprecations ? { deprecated: true } : {}), ...(route.options.discontinued ? { 'x-discontinued': route.options.discontinued } : {}), requestBody: !!validationSchemas?.body diff --git a/packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts b/packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts index 839ba5f298134..b7a4827e4f365 100644 --- a/packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts +++ b/packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts @@ -157,7 +157,7 @@ describe('processVersionedRouter', () => { expect(results.paths['/foo']!.get).toBeDefined(); expect(results.paths['/foo']!.get!.description).toBe( - '[Authz] Route required privileges: ALL of [manage_spaces].' + 'This is a test route description.

[Required authorization] Route required privileges: ALL of [manage_spaces].' ); }); }); @@ -176,6 +176,7 @@ const createTestRoute: () => VersionedRouterRoute = () => ({ requiredPrivileges: ['manage_spaces'], }, }, + description: 'This is a test route description.', }, handlers: [ { diff --git a/packages/kbn-router-to-openapispec/src/process_versioned_router.ts b/packages/kbn-router-to-openapispec/src/process_versioned_router.ts index 380bbd2e86c26..eab2dfef78a21 100644 --- a/packages/kbn-router-to-openapispec/src/process_versioned_router.ts +++ b/packages/kbn-router-to-openapispec/src/process_versioned_router.ts @@ -90,12 +90,13 @@ export const processVersionedRouter = ( ...queryObjects, ]; } - - let description = ''; + let description = `${route.options.description ?? ''}`; if (route.options.security) { const authzDescription = extractAuthzDescription(route.options.security); - description = `${route.options.description ?? ''}${authzDescription ?? ''}`; + description += `${route.options.description && authzDescription ? '

' : ''}${ + authzDescription ?? '' + }`; } const hasBody = Boolean(extractValidationSchemaFromVersionedHandler(handler)?.request?.body); @@ -107,7 +108,6 @@ export const processVersionedRouter = ( summary: route.options.summary ?? '', tags: route.options.options?.tags ? extractTags(route.options.options.tags) : [], ...(description ? { description } : {}), - ...(route.options.description ? { description: route.options.description } : {}), ...(hasDeprecations ? { deprecated: true } : {}), ...(route.options.discontinued ? { 'x-discontinued': route.options.discontinued } : {}), requestBody: hasBody