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.

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

Make alerting properly space aware #42081

Merged
merged 15 commits into from
Aug 2, 2019
Merged

Make alerting properly space aware #42081

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f10f3ac
Make alerting properly space aware
mikecote Jul 26, 2019
a8af421
Fix broken jest tests
mikecote Jul 26, 2019
5f0f48a
Fix broken integration test
mikecote Jul 26, 2019
d3040ba
Cleanup pt1
mikecote Jul 29, 2019
e1bff5c
Add spaces integration tests
mikecote Jul 30, 2019
4ddfea8
Fix type check failure
mikecote Jul 30, 2019
b7c9d2a
Merge with master
mikecote Jul 31, 2019
278f57e
Apply PR feedback pt1
mikecote Jul 31, 2019
896b6a4
Use TS types from spaces plugin
mikecote Jul 31, 2019
1718df8
Merge branch 'master' of github.com:elastic/kibana into alerting/spac…
mikecote Jul 31, 2019
bca3f0e
Fix broken tests
mikecote Jul 31, 2019
76c0c99
Apply PR feedback pt1
mikecote Jul 31, 2019
06c7334
Fix getBasePath to return server.basePath when spaces is disabled
mikecote Jul 31, 2019
37bdaf1
Merge wtih master
mikecote Aug 1, 2019
9837c7c
Apply new API changes to tests
mikecote Aug 1, 2019
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
6 changes: 2 additions & 4 deletions x-pack/legacy/plugins/actions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,7 @@ The following table describes the properties of the `options` object.
|---|---|---|
|id|The id of the action you want to fire.|string|
|params|The `params` value to give the action type executor.|object|
|namespace|The saved object namespace the action exists within.|string|
|basePath|This is a temporary parameter, but we need to capture and track the value of `request.getBasePath()` until future changes are made.<br><br>In most cases this can be `undefined` unless you need cross spaces support.|string|
|spaceId|The space id the action is within.|string|

### Example

Expand All @@ -157,14 +156,13 @@ This example makes action `3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5` fire an email.
```
server.plugins.actions.fire({
id: '3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5',
spaceId: undefined, // The spaceId of the action
mikecote marked this conversation as resolved.
Show resolved Hide resolved
params: {
from: '[email protected]',
to: ['[email protected]'],
subject: 'My email subject',
body: 'My email body',
},
namespace: undefined, // The namespace the action exists within
basePath: undefined, // Usually `request.getBasePath();` or `undefined`
});
```

Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/actions/server/action_type_registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const actionTypeRegistryParams = {
getServices,
taskManager: mockTaskManager,
encryptedSavedObjectsPlugin: encryptedSavedObjectsMock.create(),
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
};

beforeEach(() => jest.resetAllMocks());
Expand Down
20 changes: 13 additions & 7 deletions x-pack/legacy/plugins/actions/server/action_type_registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,29 @@ interface ConstructorOptions {
taskManager: TaskManager;
getServices: GetServicesFunction;
encryptedSavedObjectsPlugin: EncryptedSavedObjectsPlugin;
spaceIdToNamespace: (spaceId: string) => string;
getBasePath: (spaceId: string) => string | undefined;
}

export class ActionTypeRegistry {
private readonly taskRunCreatorFunction: TaskRunCreatorFunction;
private readonly getServices: GetServicesFunction;
private readonly taskManager: TaskManager;
private readonly actionTypes: Map<string, ActionType> = new Map();
private readonly encryptedSavedObjectsPlugin: EncryptedSavedObjectsPlugin;

constructor({ getServices, taskManager, encryptedSavedObjectsPlugin }: ConstructorOptions) {
this.getServices = getServices;
constructor({
getServices,
taskManager,
encryptedSavedObjectsPlugin,
spaceIdToNamespace,
getBasePath,
}: ConstructorOptions) {
this.taskManager = taskManager;
this.encryptedSavedObjectsPlugin = encryptedSavedObjectsPlugin;
this.taskRunCreatorFunction = getCreateTaskRunnerFunction({
getServices,
actionTypeRegistry: this,
getServices: this.getServices,
encryptedSavedObjectsPlugin: this.encryptedSavedObjectsPlugin,
encryptedSavedObjectsPlugin,
spaceIdToNamespace,
getBasePath,
});
}

Expand Down
8 changes: 4 additions & 4 deletions x-pack/legacy/plugins/actions/server/actions_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@ import { ActionTypeRegistry } from './action_type_registry';
import { ActionsClient } from './actions_client';
import { ExecutorType } from './types';
import { taskManagerMock } from '../../task_manager/task_manager.mock';
import { EncryptedSavedObjectsPlugin } from '../../encrypted_saved_objects';
import { SavedObjectsClientMock } from '../../../../../src/core/server/mocks';
import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/plugin.mock';

const savedObjectsClient = SavedObjectsClientMock.create();

const mockTaskManager = taskManagerMock.create();

const mockEncryptedSavedObjectsPlugin = {
getDecryptedAsInternalUser: jest.fn() as EncryptedSavedObjectsPlugin['getDecryptedAsInternalUser'],
} as EncryptedSavedObjectsPlugin;
const mockEncryptedSavedObjectsPlugin = encryptedSavedObjectsMock.create();

function getServices() {
return {
Expand All @@ -33,6 +31,8 @@ const actionTypeRegistryParams = {
getServices,
taskManager: mockTaskManager,
encryptedSavedObjectsPlugin: mockEncryptedSavedObjectsPlugin,
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
};

const executor: ExecutorType = async options => {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ beforeAll(() => {
getServices,
taskManager: taskManagerMock.create(),
encryptedSavedObjectsPlugin: mockEncryptedSavedObjectsPlugin,
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
mikecote marked this conversation as resolved.
Show resolved Hide resolved
getBasePath: jest.fn().mockReturnValue(undefined),
});

registerBuiltInActionTypes(actionTypeRegistry);
Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/actions/server/builtin_action_types/es_index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ beforeAll(() => {
getServices,
taskManager: taskManagerMock.create(),
encryptedSavedObjectsPlugin: mockEncryptedSavedObjectsPlugin,
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
});

registerBuiltInActionTypes(actionTypeRegistry);
Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ beforeAll(() => {
getServices,
taskManager: taskManagerMock.create(),
encryptedSavedObjectsPlugin: mockEncryptedSavedObjectsPlugin,
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
});
registerBuiltInActionTypes(actionTypeRegistry);
});
Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/actions/server/builtin_action_types/slack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ beforeAll(() => {
getServices,
taskManager: taskManagerMock.create(),
encryptedSavedObjectsPlugin: mockEncryptedSavedObjectsPlugin,
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
});
actionTypeRegistry.register(getActionType({ executor: mockSlackExecutor }));
actionType = actionTypeRegistry.get(ACTION_TYPE_ID);
Expand Down
58 changes: 30 additions & 28 deletions x-pack/legacy/plugins/actions/server/create_fire_function.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SavedObjectsClientMock } from '../../../../../src/core/server/mocks';

const mockTaskManager = taskManagerMock.create();
const savedObjectsClient = SavedObjectsClientMock.create();
const spaceIdToNamespace = jest.fn();

beforeEach(() => jest.resetAllMocks());

Expand All @@ -18,6 +19,7 @@ describe('fire()', () => {
const fireFn = createFireFunction({
taskManager: mockTaskManager,
internalSavedObjectsRepository: savedObjectsClient,
spaceIdToNamespace,
});
savedObjectsClient.get.mockResolvedValueOnce({
id: '123',
Expand All @@ -27,41 +29,41 @@ describe('fire()', () => {
},
references: [],
});
spaceIdToNamespace.mockReturnValueOnce('namespace1');
await fireFn({
id: '123',
params: { baz: false },
namespace: 'abc',
basePath: '/s/default',
spaceId: 'default',
});
expect(mockTaskManager.schedule).toHaveBeenCalledTimes(1);
expect(mockTaskManager.schedule.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"params": Object {
"actionTypeParams": Object {
"baz": false,
},
"basePath": "/s/default",
"id": "123",
"namespace": "abc",
},
"scope": Array [
"actions",
],
"state": Object {},
"taskType": "actions:mock-action",
},
]
`);
Array [
Object {
"params": Object {
"actionTypeParams": Object {
"baz": false,
},
"id": "123",
"spaceId": "default",
},
"scope": Array [
"actions",
],
"state": Object {},
"taskType": "actions:mock-action",
},
]
`);
expect(savedObjectsClient.get).toHaveBeenCalledTimes(1);
expect(savedObjectsClient.get.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"action",
"123",
Object {
"namespace": "abc",
},
]
`);
Array [
"action",
"123",
Object {
"namespace": "namespace1",
},
]
`);
expect(spaceIdToNamespace).toHaveBeenCalledWith('default');
});
});
13 changes: 7 additions & 6 deletions x-pack/legacy/plugins/actions/server/create_fire_function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,28 @@ import { TaskManager } from '../../task_manager';
interface CreateFireFunctionOptions {
taskManager: TaskManager;
internalSavedObjectsRepository: SavedObjectsClientContract;
spaceIdToNamespace: (spaceId: string) => string | undefined;
}

interface FireOptions {
export interface FireOptions {
id: string;
params: Record<string, any>;
namespace?: string;
basePath: string;
spaceId: string;
}

export function createFireFunction({
taskManager,
internalSavedObjectsRepository,
spaceIdToNamespace,
}: CreateFireFunctionOptions) {
return async function fire({ id, params, namespace, basePath }: FireOptions) {
return async function fire({ id, params, spaceId }: FireOptions) {
const namespace = spaceIdToNamespace(spaceId);
const actionSavedObject = await internalSavedObjectsRepository.get('action', id, { namespace });
await taskManager.schedule({
taskType: `actions:${actionSavedObject.attributes.actionTypeId}`,
params: {
id,
basePath,
namespace,
spaceId,
actionTypeParams: params,
},
state: {},
Expand Down
9 changes: 9 additions & 0 deletions x-pack/legacy/plugins/actions/server/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ export function init(server: Legacy.Server) {
getServices,
taskManager: taskManager!,
encryptedSavedObjectsPlugin: server.plugins.encrypted_saved_objects!,
getBasePath(spaceId: string) {
return server.plugins.spaces ? server.plugins.spaces.getBasePath(spaceId) : undefined;
},
spaceIdToNamespace(spaceId: string) {
return server.plugins.spaces ? server.plugins.spaces.spaceIdToNamespace(spaceId) : undefined;
},
});

registerBuiltInActionTypes(actionTypeRegistry);
Expand All @@ -75,6 +81,9 @@ export function init(server: Legacy.Server) {
const fireFn = createFireFunction({
Copy link
Member

Choose a reason for hiding this comment

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

fireFn, which we expose via server.expose, allows consumers to specify their own space id. Do we know of any scenarios where consumers will need this capability? If not, we could potentially create a "scoped" fire function, where the space id is derived from the current request, or similar.

If we do need to allow consumers to specify their own space id, then you'll likely want to update this so that it can handle spaces being disabled, and handle cases where a space id is required, but missing. Currently, spaceIdToNamespace will throw an error if a space id is not provided.

Also, the underlying fireFn uses callWithInternalUser in an unguarded fashion, so there isn't any authorization being applied there. This would be less of a concern if it wasn't exposed to consumers, but since this is a consumer-facing function, we should have protections in place there. (perhaps out of scope for this PR?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, consumers will need to specify their own space id. The example I can think of is task manager wants to fire action a in space b and uses this function to indicate that. We should be ok when spaces is disabled due to spaceIdToNamespace being passed as https://github.com/elastic/kibana/pull/42081/files#diff-c2af122155b3536ee79e8b5384fdaa6eR94.

In regards to callWithInternalUser being unguarded, we'll have this fixed when API keys are implemented. We will do the callWithInternalUser first, setup a connection with the keys and re-load the same object via the saved objects client.

taskManager: taskManager!,
internalSavedObjectsRepository: savedObjectsRepositoryWithInternalUser,
spaceIdToNamespace(spaceId: string) {
return server.plugins.spaces ? server.plugins.spaces.spaceIdToNamespace(spaceId) : undefined;
},
});

// Expose functions to server
Expand Down
11 changes: 9 additions & 2 deletions x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { getCreateTaskRunnerFunction } from './get_create_task_runner_function';
import { SavedObjectsClientMock } from '../../../../../../src/core/server/mocks';
import { actionTypeRegistryMock } from '../action_type_registry.mock';

const spaceIdToNamespace = jest.fn();
const actionTypeRegistry = actionTypeRegistryMock.create();
const mockedEncryptedSavedObjectsPlugin = encryptedSavedObjectsMock.create();

Expand All @@ -34,7 +35,9 @@ const getCreateTaskRunnerFunctionParams = {
};
},
actionTypeRegistry,
spaceIdToNamespace,
encryptedSavedObjectsPlugin: mockedEncryptedSavedObjectsPlugin,
getBasePath: jest.fn().mockReturnValue(undefined),
};

const taskInstanceMock = {
Expand All @@ -43,7 +46,7 @@ const taskInstanceMock = {
params: {
id: '2',
actionTypeParams: { baz: true },
namespace: 'test',
spaceId: 'test',
},
taskType: 'actions:1',
};
Expand All @@ -54,11 +57,15 @@ test('executes the task by calling the executor with proper parameters', async (
const { execute: mockExecute } = jest.requireMock('./execute');
const createTaskRunner = getCreateTaskRunnerFunction(getCreateTaskRunnerFunctionParams);
const runner = createTaskRunner({ taskInstance: taskInstanceMock });

spaceIdToNamespace.mockReturnValueOnce('namespace-test');

const runnerResult = await runner.run();

expect(runnerResult).toBeUndefined();
expect(spaceIdToNamespace).toHaveBeenCalledWith('test');
expect(mockExecute).toHaveBeenCalledWith({
namespace: 'test',
namespace: 'namespace-test',
actionId: '2',
actionTypeRegistry,
encryptedSavedObjectsPlugin: mockedEncryptedSavedObjectsPlugin,
Expand Down
7 changes: 6 additions & 1 deletion x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ interface CreateTaskRunnerFunctionOptions {
getServices: GetServicesFunction;
actionTypeRegistry: ActionTypeRegistryContract;
encryptedSavedObjectsPlugin: EncryptedSavedObjectsPlugin;
spaceIdToNamespace: (spaceId: string) => string;
getBasePath: (spaceId: string) => string | undefined;
}

interface TaskRunnerOptions {
Expand All @@ -23,11 +25,14 @@ export function getCreateTaskRunnerFunction({
getServices,
actionTypeRegistry,
encryptedSavedObjectsPlugin,
spaceIdToNamespace,
getBasePath,
mikecote marked this conversation as resolved.
Show resolved Hide resolved
}: CreateTaskRunnerFunctionOptions) {
return ({ taskInstance }: TaskRunnerOptions) => {
return {
run: async () => {
const { namespace, id, actionTypeParams } = taskInstance.params;
const { spaceId, id, actionTypeParams } = taskInstance.params;
const namespace = spaceIdToNamespace(spaceId);
await execute({
namespace,
actionTypeRegistry,
Expand Down
3 changes: 2 additions & 1 deletion x-pack/legacy/plugins/actions/server/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { SavedObjectsClientContract } from 'src/core/server';
import { ActionTypeRegistry } from './action_type_registry';
import { FireOptions } from './create_fire_function';

export type WithoutQueryAndParams<T> = Pick<T, Exclude<keyof T, 'query' | 'params'>>;
export type GetServicesFunction = (basePath: string, overwrites?: Partial<Services>) => Services;
Expand All @@ -26,7 +27,7 @@ export interface Services {
export interface ActionsPlugin {
registerType: ActionTypeRegistry['register'];
listTypes: ActionTypeRegistry['list'];
fire(options: { id: string; params: Record<string, any>; basePath: string }): Promise<void>;
fire(options: FireOptions): Promise<void>;
}

// the parameters passed to an action type executor function
Expand Down
6 changes: 5 additions & 1 deletion x-pack/legacy/plugins/alerting/server/alert_type_registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ const alertTypeRegistryParams = {
taskManager,
fireAction: jest.fn(),
internalSavedObjectsRepository: SavedObjectsClientMock.create(),
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
};

beforeEach(() => jest.resetAllMocks());
Expand All @@ -46,7 +48,7 @@ describe('has()', () => {
});
});

describe('registry()', () => {
describe('register()', () => {
test('registers the executor with the task manager', () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { getCreateTaskRunnerFunction } = require('./lib/get_create_task_runner_function');
Expand Down Expand Up @@ -79,6 +81,8 @@ Object {
}
`);
expect(firstCall.internalSavedObjectsRepository).toBeTruthy();
expect(firstCall.getBasePath).toBeTruthy();
expect(firstCall.spaceIdToNamespace).toBeTruthy();
expect(firstCall.fireAction).toMatchInlineSnapshot(`[MockFunction]`);
});

Expand Down
Loading