Skip to content

Commit

Permalink
[Authz] Removed security property from route.options types (#201352)
Browse files Browse the repository at this point in the history
## Summary

Removed `security` property from `route.options` types, `security`
should exist only as a top level property.
Fixed routes with incorrect config accordingly.

## Routes Impacted
Routes with disabled authorization (impact can be considered negligible)
```
/internal/entities/managed/enablement
/internal/entities/managed/enablement
/internal/entities/managed/enablement
/internal/entities/definition
/internal/entities/definition/{id}
/internal/entities/definition/{id?}
/internal/entities/definition/{id}/_reset
/internal/entities/definition/{id}
/api/streams/_enable
/api/streams/_resync
/api/streams/{id}/_fork
/api/streams/{id}
/api/streams/{id}
/api/streams/{id}
/api/streams
```

Routes with authorization (will be backported to `8.17.0`)

```
/internal/product_doc_base/status
/internal/product_doc_base/install
/internal/product_doc_base/uninstall
```

__Fixes: https://github.com/elastic/kibana/issues/201347__

(cherry picked from commit ec7e1a8)
  • Loading branch information
elena-shostak committed Nov 22, 2024
1 parent 687013c commit dafc885
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 dafc885

Please sign in to comment.