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

Make alerting properly space aware #42081

Merged
merged 15 commits into from
Aug 2, 2019
6 changes: 2 additions & 4 deletions x-pack/legacy/plugins/actions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,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 @@ -155,14 +154,13 @@ This example makes action `3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5` fire an email.
```
server.plugins.actions.fire({
id: '3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5',
spaceId: 'default', // The spaceId of the action
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
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const actionTypeRegistryParams = {
getServices,
taskManager: mockTaskManager,
encryptedSavedObjectsPlugin: encryptedSavedObjectsMock.create(),
spaceIdToNamespace: jest.fn().mockReturnValue(undefined),
getBasePath: jest.fn().mockReturnValue(undefined),
};

beforeEach(() => jest.resetAllMocks());
Expand Down
21 changes: 14 additions & 7 deletions x-pack/legacy/plugins/actions/server/action_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,35 @@ import { ActionType, GetServicesFunction } from './types';
import { TaskManager, TaskRunCreatorFunction } from '../../task_manager';
import { getCreateTaskRunnerFunction, ExecutorError } from './lib';
import { EncryptedSavedObjectsPlugin } from '../../encrypted_saved_objects';
import { SpacesPlugin } from '../../spaces';
mikecote marked this conversation as resolved.
Show resolved Hide resolved

interface ConstructorOptions {
taskManager: TaskManager;
getServices: GetServicesFunction;
encryptedSavedObjectsPlugin: EncryptedSavedObjectsPlugin;
spaceIdToNamespace: SpacesPlugin['spaceIdToNamespace'];
mikecote marked this conversation as resolved.
Show resolved Hide resolved
getBasePath: SpacesPlugin['getBasePath'];
}

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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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
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 {
"basePath": "/s/default",
"id": "123",
"namespace": "abc",
"params": Object {
"baz": false,
},
},
"scope": Array [
"actions",
],
"state": Object {},
"taskType": "actions:mock-action",
},
]
`);
Array [
Object {
"params": Object {
"id": "123",
"params": Object {
"baz": false,
},
"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');
});
});
14 changes: 8 additions & 6 deletions x-pack/legacy/plugins/actions/server/create_fire_function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,31 +6,33 @@

import { SavedObjectsClientContract } from 'src/core/server';
import { TaskManager } from '../../task_manager';
import { SpacesPlugin } from '../../spaces';

interface CreateFireFunctionOptions {
taskManager: TaskManager;
internalSavedObjectsRepository: SavedObjectsClientContract;
spaceIdToNamespace: SpacesPlugin['spaceIdToNamespace'];
}

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,
params,
},
state: {},
Expand Down
22 changes: 21 additions & 1 deletion x-pack/legacy/plugins/actions/server/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,19 @@ import {
listActionTypesRoute,
fireRoute,
} from './routes';

import { registerBuiltInActionTypes } from './builtin_action_types';
import { SpacesPlugin } from '../../spaces';
import { createOptionalPlugin } from '../../../server/lib/optional_plugin';

export function init(server: Legacy.Server) {
const config = server.config();
const spaces = createOptionalPlugin<SpacesPlugin>(
config,
'xpack.spaces',
server.plugins,
'spaces'
);

const { callWithInternalUser } = server.plugins.elasticsearch.getCluster('admin');
const savedObjectsRepositoryWithInternalUser = server.savedObjects.getSavedObjectsRepository(
callWithInternalUser
Expand Down Expand Up @@ -58,6 +67,14 @@ export function init(server: Legacy.Server) {
getServices,
taskManager: taskManager!,
encryptedSavedObjectsPlugin: server.plugins.encrypted_saved_objects!,
getBasePath(...args) {
return spaces.isEnabled
? spaces.getBasePath(...args)
: server.config().get('server.basePath');
},
spaceIdToNamespace(...args) {
return spaces.isEnabled ? spaces.spaceIdToNamespace(...args) : undefined;
},
});

registerBuiltInActionTypes(actionTypeRegistry);
Expand All @@ -78,6 +95,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(...args) {
return spaces.isEnabled ? spaces.spaceIdToNamespace(...args) : undefined;
},
});

// Expose functions to server
Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/actions/server/lib/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { EncryptedSavedObjectsPlugin } from '../../../encrypted_saved_objects';

interface ExecuteOptions {
actionId: string;
namespace: string;
namespace?: string;
services: Services;
params: Record<string, any>;
encryptedSavedObjectsPlugin: EncryptedSavedObjectsPlugin;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { SavedObjectsClientMock } from '../../../../../../src/core/server/mocks'
import { actionTypeRegistryMock } from '../action_type_registry.mock';
import { ExecutorError } from './executor_error';

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',
params: { baz: true },
namespace: 'test',
spaceId: 'test',
},
taskType: 'actions:1',
};
Expand All @@ -56,12 +59,14 @@ test('executes the task by calling the executor with proper parameters', async (
const runner = createTaskRunner({ taskInstance: taskInstanceMock });

mockExecute.mockResolvedValueOnce({ status: 'ok' });
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
Loading