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

Removing circular dependency between spaces and security #81891

Merged
merged 15 commits into from
Nov 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions x-pack/plugins/actions/server/lib/task_runner_factory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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),
};

Expand Down
18 changes: 11 additions & 7 deletions x-pack/plugins/actions/server/lib/task_runner_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@
*/

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 {
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';
Expand All @@ -21,7 +23,6 @@ import { ActionTypeDisabledError } from './errors';
import {
ActionTaskParams,
ActionTypeRegistryContract,
GetBasePathFunction,
SpaceIdToNamespaceFunction,
ActionTypeExecutorResult,
} from '../types';
Expand All @@ -33,7 +34,7 @@ export interface TaskRunnerContext {
actionTypeRegistry: ActionTypeRegistryContract;
encryptedSavedObjectsClient: EncryptedSavedObjectsClient;
spaceIdToNamespace: SpaceIdToNamespaceFunction;
getBasePath: GetBasePathFunction;
basePathService: IBasePath;
getUnsecuredSavedObjectsClient: (request: KibanaRequest) => SavedObjectsClientContract;
}

Expand Down Expand Up @@ -64,7 +65,7 @@ export class TaskRunnerFactory {
logger,
encryptedSavedObjectsClient,
spaceIdToNamespace,
getBasePath,
basePathService,
getUnsecuredSavedObjectsClient,
} = this.taskRunnerContext!;

Expand All @@ -87,11 +88,12 @@ export class TaskRunnerFactory {
requestHeaders.authorization = `ApiKey ${apiKey}`;
}

const path = spaceId ? `/s/${spaceId}` : '/';
Copy link
Member

Choose a reason for hiding this comment

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

question: any reason why we cannot use already exported addSpaceIdToPath here (to leave /s/ logic to Spaces plugin)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I don't see why we can't use that


// 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: {
Expand All @@ -102,7 +104,9 @@ export class TaskRunnerFactory {
url: '/',
},
},
} as unknown) as KibanaRequest;
} as unknown) as Request);

basePathService.set(fakeRequest, path);

let executorResult: ActionTypeExecutorResult<unknown>;
try {
Expand Down
10 changes: 1 addition & 9 deletions x-pack/plugins/actions/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ 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;
Expand Down Expand Up @@ -210,7 +209,6 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
});
this.taskRunnerFactory = taskRunnerFactory;
this.actionTypeRegistry = actionTypeRegistry;
this.serverBasePath = core.http.basePath.serverBasePath;
this.actionExecutor = actionExecutor;
this.security = plugins.security;

Expand Down Expand Up @@ -357,12 +355,6 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
: undefined,
});

const getBasePath = (spaceId?: string): string => {
return plugins.spaces && spaceId
? plugins.spaces.spacesService.getBasePath(spaceId)
: this.serverBasePath!;
};

const spaceIdToNamespace = (spaceId?: string): string | undefined => {
return plugins.spaces && spaceId
? plugins.spaces.spacesService.spaceIdToNamespace(spaceId)
Expand All @@ -373,7 +365,7 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
logger,
actionTypeRegistry: actionTypeRegistry!,
encryptedSavedObjectsClient,
getBasePath,
basePathService: core.http.basePath,
spaceIdToNamespace,
getUnsecuredSavedObjectsClient: (request: KibanaRequest) =>
this.getUnsecuredSavedObjectsClient(core.savedObjects, request),
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/actions/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
11 changes: 1 addition & 10 deletions x-pack/plugins/alerts/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ export class AlertingPlugin {
private readonly logger: Logger;
private alertTypeRegistry?: AlertTypeRegistry;
private readonly taskRunnerFactory: TaskRunnerFactory;
private serverBasePath?: string;
private licenseState: LicenseState | null = null;
private isESOUsingEphemeralEncryptionKey?: boolean;
private security?: SecurityPluginSetup;
Expand Down Expand Up @@ -182,8 +181,6 @@ export class AlertingPlugin {
});
this.alertTypeRegistry = alertTypeRegistry;

this.serverBasePath = core.http.basePath.serverBasePath;

const usageCollection = plugins.usageCollection;
if (usageCollection) {
initializeAlertingTelemetry(
Expand Down Expand Up @@ -259,12 +256,6 @@ export class AlertingPlugin {
includedHiddenTypes: ['alert'],
});

const getBasePath = (spaceId?: string): string => {
return plugins.spaces && spaceId
? plugins.spaces.spacesService.getBasePath(spaceId)
: this.serverBasePath!;
};

const spaceIdToNamespace = (spaceId?: string): string | undefined => {
return plugins.spaces && spaceId
? plugins.spaces.spacesService.spaceIdToNamespace(spaceId)
Expand Down Expand Up @@ -306,7 +297,7 @@ export class AlertingPlugin {
spaceIdToNamespace,
actionsPlugin: plugins.actions,
encryptedSavedObjectsClient,
getBasePath,
basePathService: core.http.basePath,
eventLogger: this.eventLogger!,
internalSavedObjectsRepository: core.savedObjects.createInternalRepository(['alert']),
});
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/alerts/server/task_runner/task_runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { encryptedSavedObjectsMock } from '../../../encrypted_saved_objects/serv
import {
loggingSystemMock,
savedObjectsRepositoryMock,
httpServiceMock,
} from '../../../../../src/core/server/mocks';
import { PluginStartContract as ActionsPluginStart } from '../../../actions/server';
import { actionsMock, actionsClientMock } from '../../../actions/server/mocks';
Expand Down Expand Up @@ -78,7 +79,7 @@ describe('Task Runner', () => {
encryptedSavedObjectsClient,
logger: loggingSystemMock.create().get(),
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
basePathService: httpServiceMock.createBasePath(),
eventLogger: eventLoggerMock.create(),
internalSavedObjectsRepository: savedObjectsRepositoryMock.create(),
};
Expand Down
12 changes: 9 additions & 3 deletions x-pack/plugins/alerts/server/task_runner/task_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
import type { PublicMethodsOf } from '@kbn/utility-types';
import { Dictionary, pickBy, mapValues, without, cloneDeep } from 'lodash';
import type { Request } from '@hapi/hapi';
import { Logger, KibanaRequest } from '../../../../../src/core/server';
import { TaskRunnerContext } from './task_runner_factory';
import { ConcreteTaskInstance, throwUnrecoverableError } from '../../../task_manager/server';
Expand Down Expand Up @@ -91,9 +92,10 @@ export class TaskRunner {
requestHeaders.authorization = `ApiKey ${apiKey}`;
}

return ({
const path = spaceId ? `/s/${spaceId}` : '/';
Copy link
Member

Choose a reason for hiding this comment

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

question: same question about addSpaceIdToPath here.


const fakeRequest = KibanaRequest.from(({
headers: requestHeaders,
getBasePath: () => this.context.getBasePath(spaceId),
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks wrong to me... but I'm not actually sure.
I thought context.getBasePath was implemented by the Spaces plugin and so line 95 feels like Spaces domain... 🤔
In plugin.ts we do this:

  private getBasePath = (spaceId?: string): string => {
    return this.spaces && spaceId ? this.spaces.getBasePath(spaceId) : this.serverBasePath!;
  };

Perhaps @mikecote can help here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought context.getBasePath was implemented by the Spaces plugin and so line 95 feels like Spaces domain...

Yeah we were using getBasePath from the Spaces plugin in order to simulate a request coming from the Legacy platform. This PR removed support for the LP requests, and instead forces consumers to construct KibanaRequest instances. Alerting & Actions were creating a LP fake request, but casting it as a KibanaRequest.

One of the reasons to fake a request was so that the Spaces plugin could properly scope instances of the Saved Objects client. Since we aren't honoring LP requests anymore, there isn't a need to provide the getBasePath function -- rather, consumers need to inform core about the request's base path by calling core.http.basePath.set(fakeRequest, spaceAwarePath) instead.

^^^^^
At least, this is the intent behind my most recent set of changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikecote what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

consumers need to inform core about the request's base path by calling core.http.basePath.set(fakeRequest, spaceAwarePath) instead.

If that is their recommendation for now, I'm ok with this change. Alerting is hacking fake requests in Kibana until scope-able elasticsearch clients is around so we'll have some odd code to deal with until then either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case my review still stands, but on this specific question I'm relying on your understanding of this 😆

path: '/',
route: { settings: {} },
url: {
Expand All @@ -104,7 +106,11 @@ export class TaskRunner {
url: '/',
},
},
} as unknown) as KibanaRequest;
} as unknown) as Request);

this.context.basePathService.set(fakeRequest, path);

return fakeRequest;
}

private getServicesWithSpaceLevelPermissions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { encryptedSavedObjectsMock } from '../../../encrypted_saved_objects/serv
import {
loggingSystemMock,
savedObjectsRepositoryMock,
httpServiceMock,
} from '../../../../../src/core/server/mocks';
import { actionsMock } from '../../../actions/server/mocks';
import { alertsMock, alertsClientMock } from '../mocks';
Expand Down Expand Up @@ -64,7 +65,7 @@ describe('Task Runner Factory', () => {
encryptedSavedObjectsClient: encryptedSavedObjectsPlugin.getClient(),
logger: loggingSystemMock.create().get(),
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
basePathService: httpServiceMock.createBasePath(),
eventLogger: eventLoggerMock.create(),
internalSavedObjectsRepository: savedObjectsRepositoryMock.create(),
};
Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugins/alerts/server/task_runner/task_runner_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@
* you may not use this file except in compliance with the Elastic License.
*/
import type { PublicMethodsOf } from '@kbn/utility-types';
import { Logger, KibanaRequest, ISavedObjectsRepository } from '../../../../../src/core/server';
import {
Logger,
KibanaRequest,
ISavedObjectsRepository,
IBasePath,
} from '../../../../../src/core/server';
import { RunContext } from '../../../task_manager/server';
import { EncryptedSavedObjectsClient } from '../../../encrypted_saved_objects/server';
import { PluginStartContract as ActionsPluginStartContract } from '../../../actions/server';
import {
AlertType,
GetBasePathFunction,
GetServicesFunction,
SpaceIdToNamespaceFunction,
} from '../types';
import { AlertType, GetServicesFunction, SpaceIdToNamespaceFunction } from '../types';
import { TaskRunner } from './task_runner';
import { IEventLogger } from '../../../event_log/server';
import { AlertsClient } from '../alerts_client';
Expand All @@ -26,7 +26,7 @@ export interface TaskRunnerContext {
eventLogger: IEventLogger;
encryptedSavedObjectsClient: EncryptedSavedObjectsClient;
spaceIdToNamespace: SpaceIdToNamespaceFunction;
getBasePath: GetBasePathFunction;
basePathService: IBasePath;
internalSavedObjectsRepository: ISavedObjectsRepository;
}

Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/alerts/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {

export type WithoutQueryAndParams<T> = Pick<T, Exclude<keyof T, 'query' | 'params'>>;
export type GetServicesFunction = (request: KibanaRequest) => Services;
export type GetBasePathFunction = (spaceId?: string) => string;
export type SpaceIdToNamespaceFunction = (spaceId?: string) => string | undefined;

declare module 'src/core/server' {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const createStartContractMock = (spaceId = DEFAULT_SPACE_ID) => {
createSpacesClient: jest.fn().mockReturnValue(spacesClientMock.create()),
getSpaceId: jest.fn().mockReturnValue(spaceId),
isInDefaultSpace: jest.fn().mockReturnValue(spaceId === DEFAULT_SPACE_ID),
getBasePath: jest.fn().mockReturnValue(''),
getActiveSpace: jest.fn(),
};
return startContract;
Expand Down
15 changes: 2 additions & 13 deletions x-pack/plugins/spaces/server/spaces_service/spaces_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
SpacesClientServiceStart,
} from '../lib/spaces_client';
import { ConfigType } from '../config';
import { getSpaceIdFromPath, addSpaceIdToPath } from '../../common/lib/spaces_url_parser';
import { getSpaceIdFromPath } from '../../common/lib/spaces_url_parser';
import { DEFAULT_SPACE_ID } from '../../common/constants';
import { spaceIdToNamespace, namespaceToSpaceId } from '../lib/utils/namespace';
import { Space } from '..';
Expand Down Expand Up @@ -74,12 +74,6 @@ export interface SpacesServiceStart {
*/
getActiveSpace(request: KibanaRequest): Promise<Space>;

/**
* Gets the space-aware base path for the specified space id.
* @param spaceId
*/
getBasePath(spaceId: string): string;

/**
* Converts the provided space id into the corresponding Saved Objects `namespace` id.
* @param spaceId
Expand Down Expand Up @@ -141,12 +135,7 @@ export class SpacesService {
const spaceId = this.getSpaceId(request, coreStart.http.basePath);
return getScopedClient(request).get(spaceId);
},
getBasePath: (spaceId: string) => {
if (!spaceId) {
throw new TypeError(`spaceId is required to retrieve base path`);
}
return addSpaceIdToPath(coreStart.http.basePath.serverBasePath, spaceId);
},

isInDefaultSpace: (request: KibanaRequest) => {
const spaceId = this.getSpaceId(request, coreStart.http.basePath);

Expand Down