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.

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] OAS Descriptions for Route Authz #197001

Merged
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
959fa99
bring in POC code
SiddharthMantri Oct 21, 2024
b735fbe
Add feature extraction logic
SiddharthMantri Oct 21, 2024
1e19b12
reuse service function
SiddharthMantri Oct 22, 2024
4c45a8f
use subFeaturePrivilegeIterator
SiddharthMantri Oct 22, 2024
502e6b7
update typescript type checks for authz in union types
SiddharthMantri Oct 22, 2024
ddba3c1
improve typecheck
SiddharthMantri Oct 22, 2024
574cfee
update test:
SiddharthMantri Oct 22, 2024
625c4bc
update process router
SiddharthMantri Oct 22, 2024
99e15f2
remove console log
SiddharthMantri Oct 22, 2024
bae9f19
Merge branch 'main' into oas-route-authzn-description
SiddharthMantri Oct 22, 2024
cf5e935
fix merge and lint
SiddharthMantri Oct 22, 2024
1dc8f93
[CI] Auto-commit changed files from 'node scripts/capture_oas_snapsho…
kibanamachine Oct 22, 2024
4305827
[CI] Auto-commit changed files from 'make api-docs && make api-docs-s…
kibanamachine Oct 22, 2024
3ab848c
update tests, fix spelling
SiddharthMantri Oct 23, 2024
da95c57
remove description if empty
SiddharthMantri Oct 23, 2024
ef225c3
[CI] Auto-commit changed files from 'node scripts/capture_oas_snapsho…
kibanamachine Oct 23, 2024
3e70aa5
[CI] Auto-commit changed files from 'make api-docs && make api-docs-s…
kibanamachine Oct 23, 2024
92a32a7
address PR comments, separate extraction logic
SiddharthMantri Oct 23, 2024
0bf118c
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Oct 23, 2024
e52c25e
update tests
SiddharthMantri Oct 23, 2024
dc2618b
remove unused buildFlavor option
SiddharthMantri Oct 23, 2024
2e2d2ed
Merge branch 'main' into oas-route-authzn-description
elasticmachine Oct 23, 2024
0437c30
update versioned router OAS generation and update tests
SiddharthMantri Oct 23, 2024
73e4ae8
fix types
SiddharthMantri Oct 23, 2024
42a297f
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Oct 23, 2024
c10ed38
remove empty lines
SiddharthMantri Oct 23, 2024
91997ae
fix test scenario
SiddharthMantri Oct 23, 2024
9a9872b
address PR comments: update if condition, add test scenario
SiddharthMantri Oct 24, 2024
162acae
update test
SiddharthMantri Oct 24, 2024
42bf34d
remove unused extract function
SiddharthMantri Oct 28, 2024
5ad07de
remove unused import
SiddharthMantri Oct 28, 2024
381c9a0
Merge branch 'main' into oas-route-authzn-description
elasticmachine Oct 28, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { schema } from '@kbn/config-schema';
import { extractAuthzDescription } from './extract_authz_description';
import { InternalRouterRoute } from './type';

describe('extractAuthzDescription', () => {
it('should return empty if route does not require privileges', () => {
const route: InternalRouterRoute = {
path: '/foo',
options: { access: 'internal' },
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
method: 'get',
isVersioned: false,
};
const description = extractAuthzDescription(route.security);
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
expect(description).toBe('');
});

it('should return route authz description for simple privileges', () => {
const route: InternalRouterRoute = {
path: '/foo',
options: { access: 'public' },
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
method: 'get',
isVersioned: false,
security: {
authz: {
requiredPrivileges: ['manage_spaces'],
},
},
};
const description = extractAuthzDescription(route.security);
expect(description).toBe('[Authz] Route required privileges: ALL of [manage_spaces].');
});

it('should return route authz description for privilege groups', () => {
{
const route: InternalRouterRoute = {
path: '/foo',
options: { access: 'public' },
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
method: 'get',
isVersioned: false,
security: {
authz: {
requiredPrivileges: [{ allRequired: ['console'], anyRequired: ['manage_spaces'] }],
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
},
},
};
const description = extractAuthzDescription(route.security);
expect(description).toBe(
'[Authz] Route required privileges: ALL of [console] AND ANY of [manage_spaces].'
);
}
{
const route: InternalRouterRoute = {
path: '/foo',
options: { access: 'public' },
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
method: 'get',
isVersioned: false,
security: {
authz: {
requiredPrivileges: [
{
allRequired: ['console', 'filesManagement'],
anyRequired: ['manage_spaces', 'taskmanager'],
},
],
},
},
};
const description = extractAuthzDescription(route.security);
expect(description).toBe(
'[Authz] Route required privileges: ALL of [console, filesManagement] AND ANY of [manage_spaces OR taskmanager].'
);
}
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import type { AuthzEnabled, AuthzDisabled, InternalRouteSecurity } from '@kbn/core-http-server';

interface PrivilegeGroupValue {
allRequired: string[];
anyRequired: string[];
}

export const extractAuthzDescription = (routeSecurity: InternalRouteSecurity | undefined) => {
if (!routeSecurity) {
return '';
}
if (!('authz' in routeSecurity) || (routeSecurity.authz as AuthzDisabled).enabled === false) {
return '';
}
if ('authz' in routeSecurity) {
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
const privileges = (routeSecurity.authz as AuthzEnabled).requiredPrivileges;

const groupedPrivileges = privileges.reduce<PrivilegeGroupValue>(
(groups, privilege) => {
if (typeof privilege === 'string') {
groups.allRequired.push(privilege);

return groups;
}
groups.allRequired.push(...(privilege.allRequired ?? []));
groups.anyRequired.push(...(privilege.anyRequired ?? []));

return groups;
},
{
anyRequired: [],
allRequired: [],
}
);

const getPrivilegesDescription = (allRequired: string[], anyRequired: string[]) => {
const allDescription = allRequired.length ? `ALL of [${allRequired.join(', ')}]` : '';
const anyDescription = anyRequired.length ? `ANY of [${anyRequired.join(' OR ')}]` : '';

return `${allDescription}${allDescription && anyDescription ? ' AND ' : ''}${anyDescription}`;
};

const getDescriptionForRoute = () => {
const allRequired = [...groupedPrivileges.allRequired];
const anyRequired = [...groupedPrivileges.anyRequired];

return `Route required privileges: ${getPrivilegesDescription(allRequired, anyRequired)}.`;
};

return `[Authz] ${getDescriptionForRoute()}`;
}
};
35 changes: 33 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 @@ -11,7 +11,8 @@ import { schema } from '@kbn/config-schema';
import { Router } from '@kbn/core-http-router-server-internal';
import { OasConverter } from './oas_converter';
import { createOperationIdCounter } from './operation_id_counter';
import { extractResponses, processRouter, type InternalRouterRoute } from './process_router';
import { extractResponses, processRouter } from './process_router';
import { type InternalRouterRoute } from './type';

describe('extractResponses', () => {
let oasConverter: OasConverter;
Expand Down Expand Up @@ -102,6 +103,24 @@ describe('processRouter', () => {
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
},
{
path: '/qux',
method: 'post',
options: {},
handler: jest.fn(),
validationSchemas: { request: { body: schema.object({}) } },
security: {
authz: {
requiredPrivileges: [
'manage_spaces',
{
allRequired: ['taskmanager'],
anyRequired: ['console'],
},
],
},
},
},
],
} as unknown as Router;

Expand All @@ -110,11 +129,23 @@ describe('processRouter', () => {
version: '2023-10-31',
});

expect(Object.keys(result1.paths!)).toHaveLength(3);
expect(Object.keys(result1.paths!)).toHaveLength(4);

const result2 = processRouter(testRouter, new OasConverter(), createOperationIdCounter(), {
version: '2024-10-31',
});
expect(Object.keys(result2.paths!)).toHaveLength(0);
});

it('updates description with privileges required', () => {
const result = processRouter(testRouter, new OasConverter(), createOperationIdCounter(), {
version: '2023-10-31',
});

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].'
);
});
});
13 changes: 11 additions & 2 deletions packages/kbn-router-to-openapispec/src/process_router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import {
} from './util';
import type { OperationIdCounter } from './operation_id_counter';
import type { GenerateOpenApiDocumentOptionsFilters } from './generate_oas';
import type { CustomOperationObject } from './type';
import type { CustomOperationObject, InternalRouterRoute } from './type';
import { extractAuthzDescription } from './extract_authz_description';

export const processRouter = (
appRouter: Router,
Expand Down Expand Up @@ -63,10 +64,19 @@ export const processRouter = (
parameters.push(...pathObjects, ...queryObjects);
}

let description = '';
if (route.security) {
const authzDescription = extractAuthzDescription(route.security);

description = `${route.options.description ?? ''}${authzDescription ?? ''}`;
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, we're appending the authz description with 0 whitespace after the route-provided description. Can we do better than that? Does the description support line breaks/new lines? Having this render on its own line would make it easier to visually parse. If we can't do it on its own line, can we at least consider adding a space between the original description and our authz addition?

image

}

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 } : {}),
Expand Down Expand Up @@ -99,7 +109,6 @@ export const processRouter = (
return { paths };
};

export type InternalRouterRoute = ReturnType<Router['getRoutes']>[0];
export const extractResponses = (route: InternalRouterRoute, converter: OasConverter) => {
const responses: OpenAPIV3.ResponsesObject = {};
if (!route.validationSchemas) return responses;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,22 @@ describe('processVersionedRouter', () => {
'application/test+json; Elastic-Api-Version=2023-10-31',
]);
});

it('correctly updates the authz description for routes that require privileges', () => {
const results = processVersionedRouter(
{ getRoutes: () => [createTestRoute()] } as unknown as CoreVersionedRouter,
new OasConverter(),
createOperationIdCounter(),
{}
);
expect(results.paths['/foo']).toBeDefined();

expect(results.paths['/foo']!.get).toBeDefined();

expect(results.paths['/foo']!.get!.description).toBe(
'[Authz] Route required privileges: ALL of [manage_spaces].'
);
});
});

const createTestRoute: () => VersionedRouterRoute = () => ({
Expand All @@ -155,6 +171,11 @@ const createTestRoute: () => VersionedRouterRoute = () => ({
deprecated: true,
discontinued: 'discontinued versioned router',
options: { body: { access: ['application/test+json'] } as any },
security: {
authz: {
requiredPrivileges: ['manage_spaces'],
},
},
},
handlers: [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
} from '@kbn/core-http-router-server-internal';
import type { RouteMethod, VersionedRouterRoute } from '@kbn/core-http-server';
import type { OpenAPIV3 } from 'openapi-types';
import { extractAuthzDescription } from './extract_authz_description';
import type { GenerateOpenApiDocumentOptionsFilters } from './generate_oas';
import type { OasConverter } from './oas_converter';
import { isReferenceObject } from './oas_converter/common';
Expand Down Expand Up @@ -90,6 +91,13 @@ export const processVersionedRouter = (
];
}

let description = '';
if (route.options.security) {
const authzDescription = extractAuthzDescription(route.options.security);

description = `${route.options.description ?? ''}${authzDescription ?? ''}`;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here w/r/t whitespace.

}

const hasBody = Boolean(extractValidationSchemaFromVersionedHandler(handler)?.request?.body);
const contentType = extractContentType(route.options.options?.body);
const hasVersionFilter = Boolean(filters?.version);
Expand All @@ -98,6 +106,7 @@ export const processVersionedRouter = (
const operation: OpenAPIV3.OperationObject = {
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 } : {}),
Expand Down
3 changes: 3 additions & 0 deletions packages/kbn-router-to-openapispec/src/type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import type { Router } from '@kbn/core-http-router-server-internal';
import type { OpenAPIV3 } from '../openapi-types';
export type { OpenAPIV3 } from '../openapi-types';
export interface KnownParameters {
Expand Down Expand Up @@ -39,3 +40,5 @@ export type CustomOperationObject = OpenAPIV3.OperationObject<{
// Custom OpenAPI from ES API spec based on @availability
'x-state'?: 'Technical Preview' | 'Beta';
}>;

export type InternalRouterRoute = ReturnType<Router['getRoutes']>[0];
2 changes: 1 addition & 1 deletion packages/kbn-router-to-openapispec/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@
"@kbn/core-http-router-server-internal",
"@kbn/core-http-server",
"@kbn/config-schema",
"@kbn/zod"
"@kbn/zod",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import type {
FeatureKibanaPrivileges,
FeatureKibanaPrivilegesReference,
} from '@kbn/features-plugin/common';
import type { FeaturesPluginSetup, KibanaFeature } from '@kbn/features-plugin/server';
import type {
FeaturesPluginSetup,
KibanaFeature,
SubFeaturePrivilegeIterator,
} from '@kbn/features-plugin/server';
import {
getMinimalPrivilegeId,
isMinimalPrivilegeId,
Expand Down Expand Up @@ -247,6 +251,31 @@ export function privilegesFactory(
}, {}),
};
},
getFeatureForPrivilege(
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: where is this being used? My IDE keeps saying its unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Removed in 42bf34d

searchTerm: string,
subFeaturePrivilegeIterator: SubFeaturePrivilegeIterator
) {
const features = featuresService.getKibanaFeatures();
const result: string[] = [];

features.forEach((feature) => {
if (feature.privileges?.all.api?.includes(searchTerm)) {
result.push(`${feature.id}.all`);
}

if (feature.privileges?.read && feature.privileges.read.api?.includes(searchTerm)) {
result.push(`${feature.id}.read`);
}

for (const subFeaturePrivilege of subFeaturePrivilegeIterator(feature, () => true)) {
if (subFeaturePrivilege.api?.includes(searchTerm)) {
result.push(`${feature.id}.${subFeaturePrivilege.id}`);
}
}
});

return result;
},
};
}

Expand Down