Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Authz] Fix description generation for Open API spec for an API #198054

Merged
merged 16 commits into from
Nov 4, 2024
Merged

[Authz] Fix description generation for Open API spec for an API #198054

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e1c54a3
[Authz] Fix how description was added to OAS spec
SiddharthMantri Oct 28, 2024
8bf8290
add test to ensure correct description
SiddharthMantri Oct 28, 2024
4adfa2b
Merge branch 'main' into fix-process-router-route-description
elasticmachine Oct 28, 2024
aa1fcf4
[CI] Auto-commit changed files from 'node scripts/capture_oas_snapsho…
kibanamachine Oct 28, 2024
c6cfa1d
fix title and add linebreak in description generation
SiddharthMantri Oct 28, 2024
cad2242
[CI] Auto-commit changed files from 'make api-docs'
kibanamachine Oct 28, 2024
6958eb7
fix description
SiddharthMantri Oct 28, 2024
ae6cd6b
[CI] Auto-commit changed files from 'node scripts/capture_oas_snapsho…
kibanamachine Oct 28, 2024
422e823
[CI] Auto-commit changed files from 'make api-docs'
kibanamachine Oct 28, 2024
78d8449
fix description
SiddharthMantri Oct 29, 2024
9dbfb2c
fix versioned router tests
SiddharthMantri Oct 29, 2024
8d20aa7
fix unit test
SiddharthMantri Oct 29, 2024
5f674ec
address PR comments: add test
SiddharthMantri Oct 30, 2024
ed079d0
Merge branch 'main' into fix-process-router-route-description
elasticmachine Nov 4, 2024
66817c9
[CI] Auto-commit changed files from 'node scripts/capture_oas_snapsho…
kibanamachine Nov 4, 2024
7203e2a
[CI] Auto-commit changed files from 'make api-docs'
kibanamachine Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/kbn-router-to-openapispec/src/extract_authz_description.test.ts
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion packages/kbn-router-to-openapispec/src/extract_authz_description.ts
View file
Open in desktop
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()}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I wonder if "Authorization" is enough, given that we always follow it with "Route required privileges:"

Suggested change
return `[Required authorization] ${getDescriptionForRoute()}`;
return `[Authorization] ${getDescriptionForRoute()}`;

};
28 changes: 26 additions & 2 deletions packages/kbn-router-to-openapispec/src/process_router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,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 @@ -129,7 +149,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(), createOperationIdCounter(), {
version: '2024-10-31',
Expand All @@ -145,7 +165,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? `<br/><br/>` : ''}${
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Searching for newline implementation for OAS yaml spec, i came across both <br/> and \n. However, I used the former because the new line wouldn't get rendered correctly with \n - for example:
image

authzDescription ?? ''
}`;
}

const hasDeprecations = !!route.options.deprecated;
Expand All @@ -77,7 +79,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
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts
View file
Open in desktop
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].'
'[Required authorization] Route required privileges: ALL of [manage_spaces].'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would it make sense to have the same set of test cases as process_router.test - covering when there is and is not a base description?

);
});
});
Expand Down
8 changes: 4 additions & 4 deletions packages/kbn-router-to-openapispec/src/process_versioned_router.ts
View file
Open in desktop
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