[7.x] Removing circular dependency between spaces and security (#81891) (#83841)

* Removing circular dependency between spaces and security

* Apply suggestions from code review

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Tests refactor

- Reorganize top level describes into 3 space-based blocks into based on spaces:
  - space disabled
  - spaces plugin unavailable
  - space enabled (most previous tests go under this new block) with new beforeEach

- wrote new tests for uncovered lines 58, 66-69

* Review1: address PR feedback

* changing fake requests for alerts/actions

* Fixing tests

* fixing more tests

* Additional testing and refactoring

* Apply suggestions from code review

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Review 2: Address feedback

* Make ESLint happy again

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

Author: 4 people

x-pack/plugins/actions/server/lib/action_executor.test.ts

@@ -31,7 +31,7 @@ const executeParams = {
   request: {} as KibanaRequest,
 };
 
-const spacesMock = spacesServiceMock.createSetupContract();
+const spacesMock = spacesServiceMock.createStartContract();
 const loggerMock = loggingSystemMock.create().get();
 const getActionsClientWithRequest = jest.fn();
 actionExecutor.initialize({

x-pack/plugins/actions/server/lib/action_executor.ts

@@ -15,15 +15,15 @@ import {
   ProxySettings,
 } from '../types';
 import { EncryptedSavedObjectsClient } from '../../../encrypted_saved_objects/server';
-import { SpacesServiceSetup } from '../../../spaces/server';
+import { SpacesServiceStart } from '../../../spaces/server';
 import { EVENT_LOG_ACTIONS } from '../plugin';
 import { IEvent, IEventLogger, SAVED_OBJECT_REL_PRIMARY } from '../../../event_log/server';
 import { ActionsClient } from '../actions_client';
 import { ActionExecutionSource } from './action_execution_source';
 
 export interface ActionExecutorContext {
   logger: Logger;
-  spaces?: SpacesServiceSetup;
+  spaces?: SpacesServiceStart;
   getServices: GetServicesFunction;
   getActionsClientWithRequest: (
     request: KibanaRequest,

x-pack/plugins/actions/server/lib/task_runner_factory.test.ts

@@ -12,7 +12,7 @@ import { TaskRunnerFactory } from './task_runner_factory';
 import { actionTypeRegistryMock } from '../action_type_registry.mock';
 import { actionExecutorMock } from './action_executor.mock';
 import { encryptedSavedObjectsMock } from '../../../encrypted_saved_objects/server/mocks';
-import { savedObjectsClientMock, loggingSystemMock } from 'src/core/server/mocks';
+import { savedObjectsClientMock, loggingSystemMock, httpServiceMock } from 'src/core/server/mocks';
 import { eventLoggerMock } from '../../../event_log/server/mocks';
 import { ActionTypeDisabledError } from './errors';
 import { actionsClientMock } from '../mocks';
@@ -70,7 +70,7 @@ const taskRunnerFactoryInitializerParams = {
   actionTypeRegistry,
   logger: loggingSystemMock.create().get(),
   encryptedSavedObjectsClient: mockedEncryptedSavedObjectsClient,
-  getBasePath: jest.fn().mockReturnValue(undefined),
+  basePathService: httpServiceMock.createBasePath(),
   getUnsecuredSavedObjectsClient: jest.fn().mockReturnValue(services.savedObjectsClient),
 };
 
@@ -126,27 +126,23 @@ test('executes the task by calling the executor with proper parameters', async (
   expect(
     mockedEncryptedSavedObjectsClient.getDecryptedAsInternalUser
   ).toHaveBeenCalledWith('action_task_params', '3', { namespace: 'namespace-test' });
+
   expect(mockedActionExecutor.execute).toHaveBeenCalledWith({
     actionId: '2',
     params: { baz: true },
-    request: {
-      getBasePath: expect.any(Function),
+    request: expect.objectContaining({
       headers: {
         // base64 encoded "123:abc"
         authorization: 'ApiKey MTIzOmFiYw==',
       },
-      path: '/',
-      route: { settings: {} },
-      url: {
-        href: '/',
-      },
-      raw: {
-        req: {
-          url: '/',
-        },
-      },
-    },
+    }),
   });
+
+  const [executeParams] = mockedActionExecutor.execute.mock.calls[0];
+  expect(taskRunnerFactoryInitializerParams.basePathService.set).toHaveBeenCalledWith(
+    executeParams.request,
+    '/s/test'
+  );
 });
 
 test('cleans up action_task_params object', async () => {
@@ -255,24 +251,19 @@ test('uses API key when provided', async () => {
   expect(mockedActionExecutor.execute).toHaveBeenCalledWith({
     actionId: '2',
     params: { baz: true },
-    request: {
-      getBasePath: expect.anything(),
+    request: expect.objectContaining({
       headers: {
         // base64 encoded "123:abc"
         authorization: 'ApiKey MTIzOmFiYw==',
       },
-      path: '/',
-      route: { settings: {} },
-      url: {
-        href: '/',
-      },
-      raw: {
-        req: {
-          url: '/',
-        },
-      },
-    },
+    }),
   });
+
+  const [executeParams] = mockedActionExecutor.execute.mock.calls[0];
+  expect(taskRunnerFactoryInitializerParams.basePathService.set).toHaveBeenCalledWith(
+    executeParams.request,
+    '/s/test'
+  );
 });
 
 test(`doesn't use API key when not provided`, async () => {
@@ -297,21 +288,16 @@ test(`doesn't use API key when not provided`, async () => {
   expect(mockedActionExecutor.execute).toHaveBeenCalledWith({
     actionId: '2',
     params: { baz: true },
-    request: {
-      getBasePath: expect.anything(),
+    request: expect.objectContaining({
       headers: {},
-      path: '/',
-      route: { settings: {} },
-      url: {
-        href: '/',
-      },
-      raw: {
-        req: {
-          url: '/',
-        },
-      },
-    },
+    }),
   });
+
+  const [executeParams] = mockedActionExecutor.execute.mock.calls[0];
+  expect(taskRunnerFactoryInitializerParams.basePathService.set).toHaveBeenCalledWith(
+    executeParams.request,
+    '/s/test'
+  );
 });
 
 test(`throws an error when license doesn't support the action type`, async () => {

x-pack/plugins/actions/server/lib/task_runner_factory.ts

@@ -5,14 +5,17 @@
  */
 
 import { pick } from 'lodash';
+import type { Request } from '@hapi/hapi';
 import { pipe } from 'fp-ts/lib/pipeable';
 import { map, fromNullable, getOrElse } from 'fp-ts/lib/Option';
+import { addSpaceIdToPath } from '../../../spaces/server';
 import {
   Logger,
   SavedObjectsClientContract,
   KibanaRequest,
   SavedObjectReference,
-} from 'src/core/server';
+  IBasePath,
+} from '../../../../../src/core/server';
 import { ActionExecutorContract } from './action_executor';
 import { ExecutorError } from './executor_error';
 import { RunContext } from '../../../task_manager/server';
@@ -21,7 +24,6 @@ import { ActionTypeDisabledError } from './errors';
 import {
   ActionTaskParams,
   ActionTypeRegistryContract,
-  GetBasePathFunction,
   SpaceIdToNamespaceFunction,
   ActionTypeExecutorResult,
 } from '../types';
@@ -33,7 +35,7 @@ export interface TaskRunnerContext {
   actionTypeRegistry: ActionTypeRegistryContract;
   encryptedSavedObjectsClient: EncryptedSavedObjectsClient;
   spaceIdToNamespace: SpaceIdToNamespaceFunction;
-  getBasePath: GetBasePathFunction;
+  basePathService: IBasePath;
   getUnsecuredSavedObjectsClient: (request: KibanaRequest) => SavedObjectsClientContract;
 }
 
@@ -64,7 +66,7 @@ export class TaskRunnerFactory {
       logger,
       encryptedSavedObjectsClient,
       spaceIdToNamespace,
-      getBasePath,
+      basePathService,
       getUnsecuredSavedObjectsClient,
     } = this.taskRunnerContext!;
 
@@ -87,11 +89,12 @@ export class TaskRunnerFactory {
           requestHeaders.authorization = `ApiKey ${apiKey}`;
         }
 
+        const path = addSpaceIdToPath('/', spaceId);
+
         // Since we're using API keys and accessing elasticsearch can only be done
         // via a request, we're faking one with the proper authorization headers.
-        const fakeRequest = ({
+        const fakeRequest = KibanaRequest.from(({
           headers: requestHeaders,
-          getBasePath: () => getBasePath(spaceId),
           path: '/',
           route: { settings: {} },
           url: {
@@ -102,7 +105,9 @@ export class TaskRunnerFactory {
               url: '/',
             },
           },
-        } as unknown) as KibanaRequest;
+        } as unknown) as Request);
+
+        basePathService.set(fakeRequest, path);
 
         let executorResult: ActionTypeExecutorResult<unknown>;
         try {

x-pack/plugins/actions/server/plugin.ts

@@ -27,7 +27,7 @@ import {
 } from '../../encrypted_saved_objects/server';
 import { TaskManagerSetupContract, TaskManagerStartContract } from '../../task_manager/server';
 import { LicensingPluginSetup, LicensingPluginStart } from '../../licensing/server';
-import { SpacesPluginSetup, SpacesServiceSetup } from '../../spaces/server';
+import { SpacesPluginStart } from '../../spaces/server';
 import { PluginSetupContract as FeaturesPluginSetup } from '../../features/server';
 import { SecurityPluginSetup } from '../../security/server';
 
@@ -109,7 +109,6 @@ export interface ActionsPluginsSetup {
   taskManager: TaskManagerSetupContract;
   encryptedSavedObjects: EncryptedSavedObjectsPluginSetup;
   licensing: LicensingPluginSetup;
-  spaces?: SpacesPluginSetup;
   eventLog: IEventLogService;
   usageCollection?: UsageCollectionSetup;
   security?: SecurityPluginSetup;
@@ -119,6 +118,7 @@ export interface ActionsPluginsStart {
   encryptedSavedObjects: EncryptedSavedObjectsPluginStart;
   taskManager: TaskManagerStartContract;
   licensing: LicensingPluginStart;
+  spaces?: SpacesPluginStart;
 }
 
 const includedHiddenTypes = [
@@ -133,12 +133,10 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
 
   private readonly logger: Logger;
   private actionsConfig?: ActionsConfig;
-  private serverBasePath?: string;
   private taskRunnerFactory?: TaskRunnerFactory;
   private actionTypeRegistry?: ActionTypeRegistry;
   private actionExecutor?: ActionExecutor;
   private licenseState: ILicenseState | null = null;
-  private spaces?: SpacesServiceSetup;
   private security?: SecurityPluginSetup;
   private eventLogService?: IEventLogService;
   private eventLogger?: IEventLogger;
@@ -211,9 +209,7 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
     });
     this.taskRunnerFactory = taskRunnerFactory;
     this.actionTypeRegistry = actionTypeRegistry;
-    this.serverBasePath = core.http.basePath.serverBasePath;
     this.actionExecutor = actionExecutor;
-    this.spaces = plugins.spaces?.spacesService;
     this.security = plugins.security;
 
     registerBuiltInActionTypes({
@@ -339,7 +335,7 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
     actionExecutor!.initialize({
       logger,
       eventLogger: this.eventLogger!,
-      spaces: this.spaces,
+      spaces: plugins.spaces?.spacesService,
       getActionsClientWithRequest,
       getServices: this.getServicesFactory(
         getScopedSavedObjectsClientWithoutAccessToActions,
@@ -359,12 +355,18 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
           : undefined,
     });
 
+    const spaceIdToNamespace = (spaceId?: string) => {
+      return plugins.spaces && spaceId
+        ? plugins.spaces.spacesService.spaceIdToNamespace(spaceId)
+        : undefined;
+    };
+
     taskRunnerFactory!.initialize({
       logger,
       actionTypeRegistry: actionTypeRegistry!,
       encryptedSavedObjectsClient,
-      getBasePath: this.getBasePath,
-      spaceIdToNamespace: this.spaceIdToNamespace,
+      basePathService: core.http.basePath,
+      spaceIdToNamespace,
       getUnsecuredSavedObjectsClient: (request: KibanaRequest) =>
         this.getUnsecuredSavedObjectsClient(core.savedObjects, request),
     });
@@ -474,14 +476,6 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
     };
   };
 
-  private spaceIdToNamespace = (spaceId?: string): string | undefined => {
-    return this.spaces && spaceId ? this.spaces.spaceIdToNamespace(spaceId) : undefined;
-  };
-
-  private getBasePath = (spaceId?: string): string => {
-    return this.spaces && spaceId ? this.spaces.getBasePath(spaceId) : this.serverBasePath!;
-  };
-
   public stop() {
     if (this.licenseState) {
       this.licenseState.clean();

x-pack/plugins/actions/server/types.ts

@@ -22,7 +22,6 @@ export { ActionTypeExecutorResult } from '../common';
 export type WithoutQueryAndParams<T> = Pick<T, Exclude<keyof T, 'query' | 'params'>>;
 export type GetServicesFunction = (request: KibanaRequest) => Services;
 export type ActionTypeRegistryContract = PublicMethodsOf<ActionTypeRegistry>;
-export type GetBasePathFunction = (spaceId?: string) => string;
 export type SpaceIdToNamespaceFunction = (spaceId?: string) => string | undefined;
 export type ActionTypeConfig = Record<string, unknown>;
 export type ActionTypeSecrets = Record<string, unknown>;

x-pack/plugins/alerts/server/plugin.test.ts

@@ -158,7 +158,6 @@ describe('Alerting Plugin', () => {
               getActionsClientWithRequest: jest.fn(),
               getActionsAuthorizationWithRequest: jest.fn(),
             },
-            spaces: () => null,
             encryptedSavedObjects: encryptedSavedObjectsMock.createStart(),
             features: mockFeatures(),
           } as unknown) as AlertingPluginsStart