Skip to content

Commit

Permalink
Fix detection of "system requests" in plugins (#57149) (#57639)
Browse files Browse the repository at this point in the history 
This aligns plugin usage with the new way that the Kibana Platform
handles checking for system requests.
  • Loading branch information
@jportner
jportner authored Feb 14, 2020
1 parent bc75eb2 commit 65f065e
Show file tree
Hide file tree
Showing 17 changed files with 72 additions and 128 deletions.
2 changes: 0 additions & 2 deletions src/legacy/core_plugins/kibana/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { migrations } from './migrations';
import { importApi } from './server/routes/api/import';
import { exportApi } from './server/routes/api/export';
import { managementApi } from './server/routes/api/management';
import * as systemApi from './server/lib/system_api';
import mappings from './mappings.json';
import { getUiSettingDefaults } from './ui_setting_defaults';
import { registerCspCollector } from './server/lib/csp_usage_collector';
Expand Down Expand Up @@ -323,7 +322,6 @@ export default function(kibana) {
exportApi(server);
managementApi(server);
registerCspCollector(usageCollection, server);
server.expose('systemApi', systemApi);
server.injectUiAppVars('kibana', () => injectVars(server));
},
});
Expand Down
41 changes: 0 additions & 41 deletions src/legacy/core_plugins/kibana/server/lib/__tests__/system_api.js
View file

This file was deleted.

31 changes: 0 additions & 31 deletions src/legacy/core_plugins/kibana/server/lib/system_api.js
View file

This file was deleted.

15 changes: 12 additions & 3 deletions src/legacy/ui/public/system_api/__tests__/system_api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,25 @@ describe('system_api', () => {
};
const newHeaders = addSystemApiHeader(headers);

expect(newHeaders).to.have.property('kbn-system-api');
expect(newHeaders['kbn-system-api']).to.be(true);
expect(newHeaders).to.have.property('kbn-system-request');
expect(newHeaders['kbn-system-request']).to.be(true);

expect(newHeaders).to.have.property('kbn-version');
expect(newHeaders['kbn-version']).to.be('4.6.0');
});
});

describe('#isSystemApiRequest', () => {
it('returns true for a system API HTTP request', () => {
it('returns true for a system HTTP request', () => {
const mockRequest = {
headers: {
'kbn-system-request': true,
},
};
expect(isSystemApiRequest(mockRequest)).to.be(true);
});

it('returns true for a legacy system API HTTP request', () => {
const mockRequest = {
headers: {
'kbn-system-api': true,
Expand Down
9 changes: 6 additions & 3 deletions src/plugins/kibana_legacy/public/utils/system_api.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

import { IRequestConfig } from 'angular';

const SYSTEM_API_HEADER_NAME = 'kbn-system-api';
const SYSTEM_REQUEST_HEADER_NAME = 'kbn-system-request';
const LEGACY_SYSTEM_API_HEADER_NAME = 'kbn-system-api';

/**
* Adds a custom header designating request as system API
Expand All @@ -28,7 +29,7 @@ const SYSTEM_API_HEADER_NAME = 'kbn-system-api';
*/
export function addSystemApiHeader(originalHeaders: Record<string, string>) {
const systemApiHeaders = {
[SYSTEM_API_HEADER_NAME]: true,
[SYSTEM_REQUEST_HEADER_NAME]: true,
};
return {
...originalHeaders,
Expand All @@ -44,5 +45,7 @@ export function addSystemApiHeader(originalHeaders: Record<string, string>) {
*/
export function isSystemApiRequest(request: IRequestConfig) {
const { headers } = request;
return headers && !!headers[SYSTEM_API_HEADER_NAME];
return (
headers && (!!headers[SYSTEM_REQUEST_HEADER_NAME] || !!headers[LEGACY_SYSTEM_API_HEADER_NAME])
);
}
3 changes: 0 additions & 3 deletions x-pack/legacy/plugins/security/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,6 @@ export const security = kibana =>
hostname: config.get('server.host'),
port: config.get('server.port'),
},
isSystemAPIRequest: server.plugins.kibana.systemApi.isSystemApiRequest.bind(
server.plugins.kibana.systemApi
),
});

// Legacy xPack Info endpoint returns whatever we return in a callback for `registerLicenseCheckResultsGenerator`
Expand Down
71 changes: 42 additions & 29 deletions x-pack/plugins/security/server/authentication/authenticator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ function getMockOptions(config: Partial<AuthenticatorOptions['config']> = {}) {
clusterClient: elasticsearchServiceMock.createClusterClient(),
basePath: httpServiceMock.createSetupContract().basePath,
loggers: loggingServiceMock.create(),
isSystemAPIRequest: jest.fn(),
getServerBaseURL: jest.fn(),
config: {
session: { idleTimeout: null, lifespan: null },
Expand Down Expand Up @@ -287,10 +286,11 @@ describe('Authenticator', () => {

it('creates session whenever authentication provider returns state for system API requests', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});
const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`;

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: { authorization } })
);
Expand All @@ -308,10 +308,11 @@ describe('Authenticator', () => {

it('creates session whenever authentication provider returns state for non-system API requests', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});
const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`;

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: { authorization } })
);
Expand All @@ -329,9 +330,10 @@ describe('Authenticator', () => {

it('does not extend session for system API calls.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user)
);
Expand All @@ -347,9 +349,10 @@ describe('Authenticator', () => {

it('extends session for non-system API calls.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user)
);
Expand Down Expand Up @@ -511,9 +514,10 @@ describe('Authenticator', () => {
});

it('does not touch session for system API calls if authentication fails with non-401 reason.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.failed(new Error('some error'))
);
Expand All @@ -527,9 +531,10 @@ describe('Authenticator', () => {
});

it('does not touch session for non-system API calls if authentication fails with non-401 reason.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.failed(new Error('some error'))
);
Expand All @@ -545,9 +550,10 @@ describe('Authenticator', () => {
it('replaces existing session with the one returned by authentication provider for system API requests', async () => {
const user = mockAuthenticatedUser();
const newState = { authorization: 'Basic yyy' };
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: newState })
);
Expand All @@ -568,9 +574,10 @@ describe('Authenticator', () => {
it('replaces existing session with the one returned by authentication provider for non-system API requests', async () => {
const user = mockAuthenticatedUser();
const newState = { authorization: 'Basic yyy' };
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: newState })
);
Expand All @@ -589,9 +596,10 @@ describe('Authenticator', () => {
});

it('clears session if provider failed to authenticate system API request with 401 with active session.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.failed(Boom.unauthorized())
);
Expand All @@ -605,9 +613,10 @@ describe('Authenticator', () => {
});

it('clears session if provider failed to authenticate non-system API request with 401 with active session.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.failed(Boom.unauthorized())
);
Expand Down Expand Up @@ -636,9 +645,10 @@ describe('Authenticator', () => {
});

it('does not clear session if provider can not handle system API request authentication with active session.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
Expand All @@ -652,9 +662,10 @@ describe('Authenticator', () => {
});

it('does not clear session if provider can not handle non-system API request authentication with active session.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
Expand All @@ -668,9 +679,10 @@ describe('Authenticator', () => {
});

it('clears session for system API request if it belongs to not configured provider.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'true' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(true);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
Expand All @@ -684,9 +696,10 @@ describe('Authenticator', () => {
});

it('clears session for non-system API request if it belongs to not configured provider.', async () => {
const request = httpServerMock.createKibanaRequest();
const request = httpServerMock.createKibanaRequest({
headers: { 'kbn-system-request': 'false' },
});

mockOptions.isSystemAPIRequest.mockReturnValue(false);
mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
AuthenticationResult.notHandled()
);
Expand Down
9 changes: 4 additions & 5 deletions x-pack/plugins/security/server/authentication/authenticator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ export interface AuthenticatorOptions {
loggers: LoggerFactory;
clusterClient: IClusterClient;
sessionStorageFactory: SessionStorageFactory<ProviderSession>;
isSystemAPIRequest: (request: KibanaRequest) => boolean;
getServerBaseURL: () => string;
}

Expand Down Expand Up @@ -312,7 +311,7 @@ export class Authenticator {

this.updateSessionValue(sessionStorage, {
providerType,
isSystemAPIRequest: this.options.isSystemAPIRequest(request),
isSystemRequest: request.isSystemRequest,
authenticationResult,
existingSession: ownsSession ? existingSession : null,
});
Expand Down Expand Up @@ -436,12 +435,12 @@ export class Authenticator {
providerType,
authenticationResult,
existingSession,
isSystemAPIRequest,
isSystemRequest,
}: {
providerType: string;
authenticationResult: AuthenticationResult;
existingSession: ProviderSession | null;
isSystemAPIRequest: boolean;
isSystemRequest: boolean;
}
) {
if (!existingSession && !authenticationResult.shouldUpdateState()) {
Expand All @@ -453,7 +452,7 @@ export class Authenticator {
// state we should store it in the session regardless of whether it's a system API request or not.
const sessionCanBeUpdated =
(authenticationResult.succeeded() || authenticationResult.redirected()) &&
(authenticationResult.shouldUpdateState() || !isSystemAPIRequest);
(authenticationResult.shouldUpdateState() || !isSystemRequest);

// If provider owned the session, but failed to authenticate anyway, that likely means that
// session is not valid and we should clear it. Also provider can specifically ask to clear
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/authentication/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe('setupAuthentication()', () => {
let mockSetupAuthenticationParams: {
config: ConfigType;
loggers: LoggerFactory;
getLegacyAPI(): Pick<LegacyAPI, 'serverConfig' | 'isSystemAPIRequest'>;
getLegacyAPI(): Pick<LegacyAPI, 'serverConfig'>;
http: jest.Mocked<CoreSetup['http']>;
clusterClient: jest.Mocked<IClusterClient>;
license: jest.Mocked<SecurityLicense>;
Expand Down
Loading

0 comments on commit 65f065e

Please sign in to comment.