Fix detection of "system requests" in plugins (#57149)

This aligns plugin usage with the new way that the Kibana Platform handles checking for system requests.
elastic · Feb 13, 2020 · 5b5421c · 5b5421c
1 parent fdef026
commit 5b5421c
Show file tree

Hide file tree

Showing 17 changed files with 70 additions and 133 deletions.
diff --git a/src/legacy/core_plugins/kibana/index.js b/src/legacy/core_plugins/kibana/index.js
@@ -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';
@@ -323,7 +322,6 @@ export default function(kibana) {
       exportApi(server);
       managementApi(server);
       registerCspCollector(usageCollection, server);
-      server.expose('systemApi', systemApi);
       server.injectUiAppVars('kibana', () => injectVars(server));
     },
   });

diff --git a/src/legacy/core_plugins/kibana/server/lib/__tests__/system_api.js b/src/legacy/core_plugins/kibana/server/lib/__tests__/system_api.js
diff --git a/src/legacy/core_plugins/kibana/server/lib/system_api.js b/src/legacy/core_plugins/kibana/server/lib/system_api.js
diff --git a/src/legacy/ui/public/system_api/__tests__/system_api.js b/src/legacy/ui/public/system_api/__tests__/system_api.js
@@ -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,

diff --git a/src/plugins/kibana_legacy/public/utils/system_api.ts b/src/plugins/kibana_legacy/public/utils/system_api.ts
@@ -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
@@ -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,
@@ -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])
+  );
 }
diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js
@@ -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`

diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts
@@ -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: {} },
@@ -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 } })
       );
@@ -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 } })
       );
@@ -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)
       );
@@ -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)
       );
@@ -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'))
       );
@@ -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'))
       );
@@ -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 })
       );
@@ -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 })
       );
@@ -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())
       );
@@ -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())
       );
@@ -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()
       );
@@ -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()
       );
@@ -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()
       );
@@ -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()
       );

diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts
@@ -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
@@ -310,7 +309,7 @@ export class Authenticator {
 
       this.updateSessionValue(sessionStorage, {
         providerType,
-        isSystemAPIRequest: this.options.isSystemAPIRequest(request),
+        isSystemRequest: request.isSystemRequest,
         authenticationResult,
         existingSession: ownsSession ? existingSession : null,
       });
@@ -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()) {
@@ -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