Skip to content

Commit

Permalink
[8.x] [Authz] Fix description generation for Open API spec for an API (
Browse files Browse the repository at this point in the history
…#198054) (#198814)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Authz] Fix description generation for Open API spec for an API
(#198054)](#198054)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Sid","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-04T15:57:45Z","message":"[Authz]
Fix description generation for Open API spec for an API
(#198054)\n\nCloses #198058.
\r\n\r\nAdds a fix for
https://github.com/elastic/kibana/pull/197001\r\n\r\n## Summary\r\nThere
was an error in how descriptions were added to the Open API spec\r\nfor
a given route - for the specific case when both a route
description\r\nand security authz required privileges were present. The
code with the\r\nerror
is:\r\nhttps://github.com//pull/197001/files#diff-5942307fac5a7b321e7f317bacd2837a7f766f3e79d5aad285513b1f82951b46R79-R80\r\n\r\nThis
PR fixes that error. \r\n\r\n\r\nAlso updated: Description field for
required privileges now includes a\r\nmore intuitive descriptor:
`Required authorization` as well as a line\r\nbreak.\r\n\r\n<img
width=\"838\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/e6af0459-28e8-40e5-873d-924d1a49b01b\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b12e7d0e79af8150ea9f2b5940a6ad1d428cff72","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:Security","release_note:skip","Feature:Security/Authorization","v9.0.0","backport:prev-major","v8.16.0","v8.17.0"],"title":"[Authz]
Fix description generation for Open API spec for an
API","number":198054,"url":"https://github.com/elastic/kibana/pull/198054","mergeCommit":{"message":"[Authz]
Fix description generation for Open API spec for an API
(#198054)\n\nCloses #198058.
\r\n\r\nAdds a fix for
https://github.com/elastic/kibana/pull/197001\r\n\r\n## Summary\r\nThere
was an error in how descriptions were added to the Open API spec\r\nfor
a given route - for the specific case when both a route
description\r\nand security authz required privileges were present. The
code with the\r\nerror
is:\r\nhttps://github.com//pull/197001/files#diff-5942307fac5a7b321e7f317bacd2837a7f766f3e79d5aad285513b1f82951b46R79-R80\r\n\r\nThis
PR fixes that error. \r\n\r\n\r\nAlso updated: Description field for
required privileges now includes a\r\nmore intuitive descriptor:
`Required authorization` as well as a line\r\nbreak.\r\n\r\n<img
width=\"838\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/e6af0459-28e8-40e5-873d-924d1a49b01b\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b12e7d0e79af8150ea9f2b5940a6ad1d428cff72"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198054","number":198054,"mergeCommit":{"message":"[Authz]
Fix description generation for Open API spec for an API
(#198054)\n\nCloses #198058.
\r\n\r\nAdds a fix for
https://github.com/elastic/kibana/pull/197001\r\n\r\n## Summary\r\nThere
was an error in how descriptions were added to the Open API spec\r\nfor
a given route - for the specific case when both a route
description\r\nand security authz required privileges were present. The
code with the\r\nerror
is:\r\nhttps://github.com//pull/197001/files#diff-5942307fac5a7b321e7f317bacd2837a7f766f3e79d5aad285513b1f82951b46R79-R80\r\n\r\nThis
PR fixes that error. \r\n\r\n\r\nAlso updated: Description field for
required privileges now includes a\r\nmore intuitive descriptor:
`Required authorization` as well as a line\r\nbreak.\r\n\r\n<img
width=\"838\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/e6af0459-28e8-40e5-873d-924d1a49b01b\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b12e7d0e79af8150ea9f2b5940a6ad1d428cff72"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Sid <[email protected]>
  • Loading branch information
kibanamachine and SiddharthMantri authored Nov 4, 2024
1 parent 8fdcef5 commit 312f642
Show file tree
Hide file tree
Showing 8 changed files with 52 additions and 19 deletions.
4 changes: 2 additions & 2 deletions oas_docs/bundle.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.<br/><br/>[Required authorization] Route required privileges: ALL of [copySavedObjectsToSpaces].",
"operationId": "post-spaces-copy-saved-objects",
"parameters": [
{
Expand Down Expand Up @@ -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.<br/><br/>[Required authorization] Route required privileges: ALL of [copySavedObjectsToSpaces].",
"operationId": "post-spaces-resolve-copy-saved-objects-errors",
"parameters": [
{
Expand Down
7 changes: 5 additions & 2 deletions oas_docs/output/kibana.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.<br/><br/>[Required authorization] Route required privileges: ALL
of [copySavedObjectsToSpaces].
operationId: post-spaces-copy-saved-objects
parameters:
- description: The version of the API to use
Expand Down Expand Up @@ -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.<br/><br/>[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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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 = {
Expand All @@ -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].'
);
}
{
Expand All @@ -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].'
);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,5 @@ export const extractAuthzDescription = (routeSecurity: InternalRouteSecurity | u
return `Route required privileges: ${getPrivilegesDescription(allRequired, anyRequired)}.`;
};

return `[Authz] ${getDescriptionForRoute()}`;
return `[Required authorization] ${getDescriptionForRoute()}`;
};
28 changes: 26 additions & 2 deletions packages/kbn-router-to-openapispec/src/process_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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',
Expand All @@ -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.<br/><br/>[Required authorization] Route required privileges: ALL of [manage_spaces, taskmanager] AND ANY of [console].'
);
});
});
7 changes: 4 additions & 3 deletions packages/kbn-router-to-openapispec/src/process_router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,20 @@ 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 ? `<br/><br/>` : ''}${
authzDescription ?? ''
}`;
}

const hasDeprecations = !!route.options.deprecated;
const operation: CustomOperationObject = {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.<br/><br/>[Required authorization] Route required privileges: ALL of [manage_spaces].'
);
});
});
Expand All @@ -176,6 +176,7 @@ const createTestRoute: () => VersionedRouterRoute = () => ({
requiredPrivileges: ['manage_spaces'],
},
},
description: 'This is a test route description.',
},
handlers: [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? '<br/><br/>' : ''}${
authzDescription ?? ''
}`;
}

const hasBody = Boolean(extractValidationSchemaFromVersionedHandler(handler)?.request?.body);
Expand All @@ -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
Expand Down

0 comments on commit 312f642

Please sign in to comment.