Skip to content

Fix detection of "system requests" in plugins #57149

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.

Sign up for GitHub

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

Merged
jportner merged 8 commits into from
Feb 13, 2020
Merged

Fix detection of "system requests" in plugins #57149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
898e2d5
Fix detection of "system requests"
jportner Feb 7, 2020
34987b1
Update tests
jportner Feb 7, 2020
04a78cd
Update tests again
jportner Feb 7, 2020
611607d
Merge branch 'master' into issue-57119-fix-system-request-header
jportner Feb 12, 2020
e158fc4
PR review feedback
jportner Feb 12, 2020
f0af708
Merge branch 'master' into issue-57119-fix-system-request-header
jportner Feb 13, 2020
1e05091
Remove unused internal system API
jportner Feb 13, 2020
3fcab8c
Remove code that exposes internal system API
jportner Feb 13, 2020
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
2 changes: 0 additions & 2 deletions src/legacy/core_plugins/kibana/index.js
View file
Open in desktop
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
Open in desktop

This file was deleted.

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

This file was deleted.

15 changes: 12 additions & 3 deletions src/legacy/ui/public/system_api/__tests__/system_api.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,6 @@ export const security = kibana =>
const xpackInfo = server.plugins.xpack_main.info;
securityPlugin.__legacyCompat.registerLegacyAPI({
auditLogger: new AuditLogger(server, 'security', config, xpackInfo),
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
Open in desktop
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(),
config: {
session: { idleTimeout: null, lifespan: null },
authc: { providers: [], oidc: {}, saml: {} },
Expand Down Expand Up @@ -286,10 +285,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 @@ -307,10 +307,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 @@ -328,9 +329,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 @@ -346,9 +348,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 @@ -510,9 +513,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 @@ -526,9 +530,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 @@ -544,9 +549,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 @@ -567,9 +573,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 @@ -588,9 +595,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 @@ -604,9 +612,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 @@ -635,9 +644,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 @@ -651,9 +661,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 @@ -667,9 +678,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 @@ -683,9 +695,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
Open in desktop
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;
}

// Mapping between provider key defined in the config and authentication
Expand Down Expand Up @@ -310,7 +309,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 @@ -434,12 +433,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 @@ -451,7 +450,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
Loading