Skip to content

Commit

Permalink
[8.17] [Authz] Removed security property from route.options types (#2…
Browse files Browse the repository at this point in the history
…01352) (#201428)

# Backport

This will backport the following commits from `main` to `8.17`:
- [[Authz] Removed security property from route.options types
(#201352)](#201352)

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

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

<!--BACKPORT [{"author":{"name":"Elena
Shostak","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-22T16:13:08Z","message":"[Authz]
Removed security property from route.options types (#201352)\n\n##
Summary\r\n\r\nRemoved `security` property from `route.options` types,
`security`\r\nshould exist only as a top level property.\r\nFixed routes
with incorrect config accordingly.\r\n\r\n\r\n## Routes
Impacted\r\nRoutes with disabled authorization (impact can be considered
negligible)\r\n```\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/definition\r\n/internal/entities/definition/{id}\r\n/internal/entities/definition/{id?}\r\n/internal/entities/definition/{id}/_reset\r\n/internal/entities/definition/{id}\r\n/api/streams/_enable\r\n/api/streams/_resync\r\n/api/streams/{id}/_fork\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams
\r\n```\r\n\r\nRoutes with authorization (will be backported to
`8.17.0`)\r\n\r\n```\r\n/internal/product_doc_base/status\r\n/internal/product_doc_base/install\r\n/internal/product_doc_base/uninstall\r\n```\r\n\r\n\r\n\r\n__Fixes:
https://github.com/elastic/kibana/issues/201347__","sha":"ec7e1a808f9dc8012fb53a6a7bad0391afe265f2","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Security","release_note:skip","Feature:Security/Authorization","v9.0.0","backport:prev-minor","backport:version","v8.17.0"],"title":"[Authz]
Removed security property from route.options
types","number":201352,"url":"https://github.com/elastic/kibana/pull/201352","mergeCommit":{"message":"[Authz]
Removed security property from route.options types (#201352)\n\n##
Summary\r\n\r\nRemoved `security` property from `route.options` types,
`security`\r\nshould exist only as a top level property.\r\nFixed routes
with incorrect config accordingly.\r\n\r\n\r\n## Routes
Impacted\r\nRoutes with disabled authorization (impact can be considered
negligible)\r\n```\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/definition\r\n/internal/entities/definition/{id}\r\n/internal/entities/definition/{id?}\r\n/internal/entities/definition/{id}/_reset\r\n/internal/entities/definition/{id}\r\n/api/streams/_enable\r\n/api/streams/_resync\r\n/api/streams/{id}/_fork\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams
\r\n```\r\n\r\nRoutes with authorization (will be backported to
`8.17.0`)\r\n\r\n```\r\n/internal/product_doc_base/status\r\n/internal/product_doc_base/install\r\n/internal/product_doc_base/uninstall\r\n```\r\n\r\n\r\n\r\n__Fixes:
https://github.com/elastic/kibana/issues/201347__","sha":"ec7e1a808f9dc8012fb53a6a7bad0391afe265f2"}},"sourceBranch":"main","suggestedTargetBranches":["8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201352","number":201352,"mergeCommit":{"message":"[Authz]
Removed security property from route.options types (#201352)\n\n##
Summary\r\n\r\nRemoved `security` property from `route.options` types,
`security`\r\nshould exist only as a top level property.\r\nFixed routes
with incorrect config accordingly.\r\n\r\n\r\n## Routes
Impacted\r\nRoutes with disabled authorization (impact can be considered
negligible)\r\n```\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/managed/enablement\r\n/internal/entities/definition\r\n/internal/entities/definition/{id}\r\n/internal/entities/definition/{id?}\r\n/internal/entities/definition/{id}/_reset\r\n/internal/entities/definition/{id}\r\n/api/streams/_enable\r\n/api/streams/_resync\r\n/api/streams/{id}/_fork\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams/{id}\r\n/api/streams
\r\n```\r\n\r\nRoutes with authorization (will be backported to
`8.17.0`)\r\n\r\n```\r\n/internal/product_doc_base/status\r\n/internal/product_doc_base/install\r\n/internal/product_doc_base/uninstall\r\n```\r\n\r\n\r\n\r\n__Fixes:
https://github.com/elastic/kibana/issues/201347__","sha":"ec7e1a808f9dc8012fb53a6a7bad0391afe265f2"}},{"branch":"8.17","label":"v8.17.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Elena Shostak <[email protected]>
  • Loading branch information
kibanamachine and elena-shostak authored Nov 22, 2024
1 parent 25884bd commit 042993d
Show file tree
Hide file tree
Showing 8 changed files with 51 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,23 @@ describe('Router', () => {
);
});

it('throws if route has security declared wrong', () => {
const router = new Router('', logger, enhanceWithContext, routerOptions);
expect(() =>
router.get(
// we use 'any' because validate requires valid Type or function usage
{
path: '/',
validate: false,
options: { security: { authz: { requiredPrivileges: [] } } } as any,
},
(context, req, res) => res.ok({})
)
).toThrowErrorMatchingInlineSnapshot(
`"\`options.security\` is not allowed in route config. Use \`security\` instead."`
);
});

it('throws if options.body.output is not a valid value', () => {
const router = new Router('', logger, enhanceWithContext, routerOptions);
expect(() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@ function validOptions(
);
}

// @ts-expect-error to eliminate problems with `security` in the options for route factories abstractions
if (options.security) {
throw new Error('`options.security` is not allowed in route config. Use `security` instead.');
}

const body = shouldNotHavePayload
? undefined
: {
Expand Down
6 changes: 4 additions & 2 deletions packages/core/http/core-http-server/src/router/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@ export interface KibanaRequestState extends RequestApplicationState {
* Route options: If 'GET' or 'OPTIONS' method, body options won't be returned.
* @public
*/
export type KibanaRequestRouteOptions<Method extends RouteMethod> = Method extends 'get' | 'options'
export type KibanaRequestRouteOptions<Method extends RouteMethod> = (Method extends
| 'get'
| 'options'
? Required<Omit<RouteConfigOptions<Method>, 'body'>>
: Required<RouteConfigOptions<Method>>;
: Required<RouteConfigOptions<Method>>) & { security?: RouteSecurity };

/**
* Request specific route information exposed to a handler.
Expand Down
7 changes: 0 additions & 7 deletions packages/core/http/core-http-server/src/router/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,13 +398,6 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
*/
discontinued?: string;

/**
* Defines the security requirements for a route, including authorization and authentication.
*
* @remarks This will be surfaced in OAS documentation.
*/
security?: RouteSecurity;

/**
* Whether this endpoint is being used to serve generated or static HTTP resources
* like JS, CSS or HTML. _Do not set to `true` for HTTP APIs._
Expand Down
7 changes: 4 additions & 3 deletions packages/core/http/core-http-server/src/versioning/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import type {
RequestHandlerContextBase,
RouteValidationFunction,
LazyValidator,
RouteSecurity,
} from '../..';
import type { RouteDeprecationInfo } from '../router/route';
type RqCtx = RequestHandlerContextBase;
Expand All @@ -35,12 +36,12 @@ export type VersionedRouteConfig<Method extends RouteMethod> = Omit<
> & {
options?: Omit<
RouteConfigOptions<Method>,
'access' | 'description' | 'summary' | 'deprecated' | 'discontinued' | 'security'
'access' | 'description' | 'summary' | 'deprecated' | 'discontinued'
>;
/** See {@link RouteConfigOptions<RouteMethod>['access']} */
access: Exclude<RouteConfigOptions<Method>['access'], undefined>;
/** See {@link RouteConfigOptions<RouteMethod>['security']} */
security?: Exclude<RouteConfigOptions<Method>['security'], undefined>;
security?: RouteSecurity;
/**
* When enabled, the router will also check for the presence of an `apiVersion`
* query parameter to determine the route version to resolve to:
Expand Down Expand Up @@ -337,7 +338,7 @@ export interface AddVersionOpts<P, Q, B> {
*/
validate: false | VersionedRouteValidation<P, Q, B> | (() => VersionedRouteValidation<P, Q, B>); // Provide a way to lazily load validation schemas

security?: Exclude<RouteConfigOptions<RouteMethod>['security'], undefined>;
security?: RouteSecurity;

options?: {
deprecated?: RouteDeprecationInfo;
Expand Down
3 changes: 2 additions & 1 deletion packages/kbn-server-route-repository-utils/src/typings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
Logger,
RequestHandlerContext,
RouteConfigOptions,
RouteSecurity,
RouteMethod,
} from '@kbn/core/server';
import type { ServerSentEvent } from '@kbn/sse-utils';
Expand Down Expand Up @@ -277,5 +278,5 @@ export interface DefaultRouteHandlerResources extends CoreRouteHandlerResources
}

export interface DefaultRouteCreateOptions {
options?: RouteConfigOptions<RouteMethod>;
options?: RouteConfigOptions<RouteMethod> & { security?: RouteSecurity };
}
8 changes: 6 additions & 2 deletions packages/kbn-server-route-repository/src/register_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,14 @@ export function registerRoutes<TDependencies extends Record<string, any>>({
validationObject = passThroughValidationObject;
}

const { security, ...restOptions } = options ?? {};

if (!version) {
router[method](
{
path: pathname,
options,
security,
options: restOptions,
validate: validationObject,
},
wrappedHandler
Expand All @@ -150,7 +153,8 @@ export function registerRoutes<TDependencies extends Record<string, any>>({
router.versioned[method]({
path: pathname,
access: pathname.startsWith('/internal/') ? 'internal' : 'public',
options,
options: restOptions,
security,
}).addVersion(
{
version,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ export const registerInstallationRoutes = ({
validate: false,
options: {
access: 'internal',
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
},
Expand All @@ -56,13 +56,13 @@ export const registerInstallationRoutes = ({
validate: false,
options: {
access: 'internal',
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
timeout: { idleSocket: 20 * 60 * 1000 }, // install can take time.
},
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
},
async (ctx, req, res) => {
const { documentationManager } = getServices();
Expand Down Expand Up @@ -90,10 +90,10 @@ export const registerInstallationRoutes = ({
validate: false,
options: {
access: 'internal',
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
security: {
authz: {
requiredPrivileges: ['manage_llm_product_doc'],
},
},
},
Expand Down

0 comments on commit 042993d

Please sign in to comment.