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

[HTTP] First pass of making Kibana work with internal restrictions enforced #162258

Merged
merged 17 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -42,6 +42,7 @@ describe('registerRouteForBundle', () => {
{
path: '/route-path/{path*}',
options: {
access: 'public',
authRequired: false,
},
validate: expect.any(Object),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export function registerRouteForBundle(
path: `${routePath}{path*}`,
options: {
authRequired: false,
access: 'public',
},
validate: {
params: schema.object({
Expand Down
27 changes: 15 additions & 12 deletions packages/core/apps/core-apps-server-internal/src/core_app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,21 @@ export class CoreAppsService {
const router = httpSetup.createRouter<InternalCoreAppsServiceRequestHandlerContext>('');
const resources = coreSetup.httpResources.createRegistrar(router);

router.get({ path: '/', validate: false }, async (context, req, res) => {
const { uiSettings } = await context.core;
const defaultRoute = await uiSettings.client.get<string>('defaultRoute');
const basePath = httpSetup.basePath.get(req);
const url = `${basePath}${defaultRoute}`;

return res.redirected({
headers: {
location: url,
},
});
});
router.get(
{ path: '/', validate: false, options: { access: 'public' } },
async (context, req, res) => {
const { uiSettings } = await context.core;
const defaultRoute = await uiSettings.client.get<string>('defaultRoute');
const basePath = httpSetup.basePath.get(req);
const url = `${basePath}${defaultRoute}`;

return res.redirected({
headers: {
location: url,
},
});
}
);

this.registerCommonDefaultRoutes({
basePath: coreSetup.http.basePath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,20 @@ describe('HttpResources service', () => {
register = await initializer();
});

it('registration defaults to "public" access', () => {
register(routeConfig, async (ctx, req, res) => res.ok());
const [[registeredRouteConfig]] = router.get.mock.calls;
expect(registeredRouteConfig.options?.access).toBe('public');
});

it('registration can set access to "internal"', () => {
register({ ...routeConfig, options: { access: 'internal' } }, async (ctx, req, res) =>
res.ok()
);
const [[registeredRouteConfig]] = router.get.mock.calls;
expect(registeredRouteConfig.options?.access).toBe('internal');
});

describe('renderCoreApp', () => {
it('formats successful response', async () => {
register(routeConfig, async (ctx, req, res) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,21 @@ export class HttpResourcesService implements CoreService<InternalHttpResourcesSe
route: RouteConfig<P, Q, B, 'get'>,
handler: HttpResourcesRequestHandler<P, Q, B, Context>
) => {
return router.get<P, Q, B>(route, (context, request, response) => {
return handler(context as Context, request, {
...response,
...this.createResponseToolkit(deps, context, request, response),
});
});
return router.get<P, Q, B>(
{
...route,
options: {
access: 'public',
...route.options,
},
},
(context, request, response) => {
return handler(context as Context, request, {
...response,
...this.createResponseToolkit(deps, context, request, response),
});
}
);
},
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@
* Side Public License, v 1.
*/

jest.mock('@kbn/server-http-tools', () => {
const module = jest.requireActual('@kbn/server-http-tools');
return {
...module,
createServer: jest.fn(module.createServer),
};
});

import { Server } from 'http';
import { rm, mkdtemp, readFile, writeFile } from 'fs/promises';
import supertest from 'supertest';
Expand All @@ -23,6 +31,7 @@ import type {
RequestHandlerContextBase,
} from '@kbn/core-http-server';
import { Router, type RouterOptions } from '@kbn/core-http-router-server-internal';
import { createServer } from '@kbn/server-http-tools';
import { HttpConfig } from './http_config';
import { HttpServer } from './http_server';
import { Readable } from 'stream';
Expand Down Expand Up @@ -1506,6 +1515,29 @@ describe('setup contract', () => {
}
});

test('registers routes with expected options', async () => {
const { registerStaticDir } = await server.setup(config);
expect(createServer).toHaveBeenCalledTimes(1);
const [{ value: myServer }] = (createServer as jest.Mock).mock.results;
jest.spyOn(myServer, 'route');
expect(myServer.route).toHaveBeenCalledTimes(0);
registerStaticDir('/static/{path*}', assetFolder);
expect(myServer.route).toHaveBeenCalledTimes(1);
expect(myServer.route).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
options: {
app: { access: 'public' },
auth: false,
cache: {
privacy: 'public',
otherwise: 'must-revalidate',
},
},
})
);
});

test('does not throw if called after stop', async () => {
const { registerStaticDir } = await server.setup(config);
await server.stop();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ export class HttpServer {
},
},
options: {
app: { access: 'public' },
auth: false,
cache: {
privacy: 'public',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* 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 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 or the Server
* Side Public License, v 1.
*/

import { mockRouter } from '@kbn/core-http-router-server-mocks';
import { registerTranslationsRoute } from './translations';

describe('registerTranslationsRoute', () => {
test('registers route with expected options', () => {
const router = mockRouter.create();
registerTranslationsRoute(router, 'en');
expect(router.get).toHaveBeenCalledTimes(1);
expect(router.get).toHaveBeenNthCalledWith(
1,
expect.objectContaining({ options: { access: 'public', authRequired: false } }),
expect.any(Function)
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const registerTranslationsRoute = (router: IRouter, locale: string) => {
}),
},
options: {
access: 'public',
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of Core routes that were (rightfully) switched to public aren't listed in the issue's description (e.g this one). Future us would probably be very grateful if we had an exhaustive list somewhere for when we may want to switch some of them back to internal (e.g when updating the internal consumers to provide the header)

authRequired: false,
},
},
Expand Down
1 change: 1 addition & 0 deletions packages/core/i18n/core-i18n-server-internal/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@kbn/i18n",
"@kbn/std",
"@kbn/repo-packages",
"@kbn/core-http-router-server-mocks",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* 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 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 or the Server
* Side Public License, v 1.
*/

import { registerBootstrapRoute } from './register_bootstrap_route';
import { mockRouter } from '@kbn/core-http-router-server-mocks';

describe('registerBootstrapRoute', () => {
test('register with expected options', () => {
const router = mockRouter.create();
const renderer = jest.fn();
registerBootstrapRoute({ router, renderer });
expect(router.get).toHaveBeenCalledTimes(2);
expect(router.get).toHaveBeenNthCalledWith(
1,
expect.objectContaining({ options: { access: 'public', tags: ['api'] } }),
expect.any(Function)
);
expect(router.get).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
options: { access: 'public', tags: ['api'], authRequired: 'optional' },
}),
expect.any(Function)
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const registerBootstrapRoute = ({
path: '/bootstrap.js',
options: {
tags: ['api'],
access: 'public',
},
validate: false,
},
Expand All @@ -44,6 +45,7 @@ export const registerBootstrapRoute = ({
options: {
authRequired: 'optional',
tags: ['api'],
access: 'public',
},
validate: false,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export const registerStatusRoute = ({
options: {
authRequired: 'optional',
tags: ['api'], // ensures that unauthenticated calls receive a 401 rather than a 302 redirect to login page
access: 'public', // needs to be public to allow access from "system" users like k8s readiness probes.
},
validate: {
query: schema.object(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export const registerPrebootStatusRoute = ({ router }: { router: IRouter }) => {
options: {
authRequired: false,
tags: ['api'],
access: 'public', // needs to be public to allow access from "system" users like k8s readiness probes.
},
validate: false,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe('StatusService', () => {
expect(prebootRouterMock.get).toHaveBeenCalledWith(
{
path: '/api/status',
options: { authRequired: false, tags: ['api'] },
options: { authRequired: false, tags: ['api'], access: 'public' },
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
validate: false,
},
expect.any(Function)
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/home/public/application/load_tutorials.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
import _ from 'lodash';
import { getServices } from './kibana_services';
import { i18n } from '@kbn/i18n';
import { X_ELASTIC_INTERNAL_ORIGIN_REQUEST } from '@kbn/core-http-common';

const baseUrl = getServices().addBasePath('/api/kibana/home/tutorials');
const headers = new Headers();
headers.append('Accept', 'application/json');
headers.append('Content-Type', 'application/json');
headers.append('kbn-xsrf', 'kibana');
headers.append(X_ELASTIC_INTERNAL_ORIGIN_REQUEST, 'kibana');
Comment on lines 18 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 I don't even want to know why the home plugin isn't using core's fetch service here...


let tutorials = [];
let tutorialsLoaded = false;
Expand Down
1 change: 1 addition & 0 deletions src/plugins/home/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@kbn/storybook",
"@kbn/cloud-chat-provider-plugin",
"@kbn/shared-ux-router",
"@kbn/core-http-common",
],
"exclude": [
"target/**/*",
Expand Down
1 change: 1 addition & 0 deletions src/plugins/usage_collection/server/routes/stats/stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export function registerStatsRoute({
options: {
authRequired: !config.allowAnonymous,
tags: ['api'], // ensures that unauthenticated calls receive a 401 rather than a 302 redirect to login page
access: 'public', // needs to be public to allow access from "system" users like metricbeat.
},
validate: {
query: schema.object({
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/routes/agent_policy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export const registerRoutes = (router: FleetAuthzRouter) => {
{
path: AGENT_POLICY_API_ROUTES.LIST_PATTERN,
validate: GetAgentPoliciesRequestSchema,
options: { access: 'public' },
juliaElastic marked this conversation as resolved.
Show resolved Hide resolved
fleetAuthz: {
fleet: { readAgentPolicies: true },
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const getClusterHealthRoute = (router: SecuritySolutionPluginRouter) => {
validate: {},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand All @@ -61,6 +62,7 @@ export const getClusterHealthRoute = (router: SecuritySolutionPluginRouter) => {
},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const getRuleHealthRoute = (router: SecuritySolutionPluginRouter) => {
},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const getSpaceHealthRoute = (router: SecuritySolutionPluginRouter) => {
validate: {},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand All @@ -61,6 +62,7 @@ export const getSpaceHealthRoute = (router: SecuritySolutionPluginRouter) => {
},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const setupHealthRoute = (router: SecuritySolutionPluginRouter) => {
validate: {},
options: {
tags: ['access:securitySolution'],
access: 'public', // must be public to enable "system" users to collect data
},
},
async (context, request, response) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export function backgroundTaskUtilizationRoute(
// options: { tags: ['access:taskManager'] },
validate: false,
options: {
access: 'public', // access must be public to allow "system" users, like metrics collectors, to access these routes
authRequired: routeOption.isAuthenticated ?? true,
},
},
Expand Down