Skip to content

Commit 21016a8

Browse files
talborentalboren
authored
[One Workflow] Tech Debt: use ActionsClient instead of UnsecuredActionsClient when possible (#240726)
_LLM generated description_ ⚠️ This PR should be merged only after #240725 is merged ### Summary This PR enhances the workflow execution engine to preserve user context when executing connectors by utilizing the scoped ActionsClient when a request context is available, while gracefully falling back to the IUnsecuredActionsClient when no context exists. ### Changes Made 1. Enhanced `ConnectorExecutor` to Support Both Client Types - Updated constructor to accept either `ActionsClient` (scoped) or `IUnsecuredActionsClient` (unsecured) - Added isScoped flag to track which client type is being used - Refactored connector execution logic to handle both client types: - Scoped client: Uses actionId parameter and automatically inherits user permissions and space context - Unsecured client: Uses id parameter with explicit spaceId and requesterId - Extracted resolveConnectorId() method to handle connector name-to-ID resolution for both client types 2. Updated setup_dependencies.ts to Use Context-Aware Client Selection - When fakeRequest is available: Calls actionsPlugin.getActionsClientWithRequest(fakeRequest) to create a scoped actions client that preserves the original user's context, permissions, and space - When fakeRequest is not available: Falls back to actionsPlugin.getUnsecuredActionsClient() - **Added TODO comment about potentially disabling connectors completely when no fakeRequest is available for security considerations** ### Benefits - Preserves User Context: Connectors executed from workflows now run with the original user's permissions, improving security and audit trails - Better Security: Actions are properly scoped to the user and space, respecting Kibana's security model - Backward Compatible: Gracefully falls back to unsecured client when no request context is available - Efficient Design: Client selection happens once during setup, not on every connector execution - Maintains Clean Architecture: Implementation follows clean code principles with small, focused functions
1 parent 683f0d8 commit 21016a8

File tree

5 files changed

+222
-73
lines changed

5 files changed

+222
-73
lines changed

‎src/platform/plugins/shared/workflows_execution_engine/integration_tests/mocks/actions_plugin.mock.ts‎

Lines changed: 80 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77
* License v3.0 only", or the "Server Side Public License, v 1".
88
*/
99

10+
/* eslint-disable max-classes-per-file */
11+
1012
import type { ActionTypeExecutorResult } from '@kbn/actions-plugin/common';
11-
import type { IUnsecuredActionsClient } from '@kbn/actions-plugin/server';
13+
import type { ActionsClient, IUnsecuredActionsClient } from '@kbn/actions-plugin/server';
1214
import type { ConnectorWithExtraFindData } from '@kbn/actions-plugin/server/application/connector/types';
1315

1416
export const FakeConnectors = {
@@ -50,73 +52,98 @@ export const FakeConnectors = {
5052
},
5153
};
5254

55+
async function getMockedConnectorResult(
56+
id: string,
57+
params: Record<string, any>
58+
): Promise<ActionTypeExecutorResult<unknown>> {
59+
const fakeConnector = Object.values(FakeConnectors).find((c) => c.id === id);
60+
61+
switch (fakeConnector?.name) {
62+
case FakeConnectors.slack1.name:
63+
case FakeConnectors.slack2.name: {
64+
return {
65+
status: 'ok',
66+
actionId: id,
67+
data: { text: 'ok' },
68+
};
69+
}
70+
case FakeConnectors.echo_inference.name: {
71+
return {
72+
status: 'ok',
73+
actionId: id,
74+
data: [
75+
{
76+
result: params?.text,
77+
},
78+
],
79+
};
80+
}
81+
case FakeConnectors.constantlyFailing.name: {
82+
throw new Error('Error: Constantly failing connector');
83+
}
84+
case FakeConnectors.slow_1sec_inference.name: {
85+
await new Promise<void>((resolve) => setTimeout(() => resolve(), 1000));
86+
return {
87+
status: 'ok',
88+
actionId: id,
89+
data: [
90+
{
91+
result: 'Hello! How can I help you?',
92+
},
93+
],
94+
};
95+
}
96+
case FakeConnectors.slow_3sec_inference.name: {
97+
await new Promise<void>((resolve) => setTimeout(() => resolve(), 3000));
98+
return {
99+
status: 'ok',
100+
actionId: id,
101+
data: [
102+
{
103+
result: 'Hello! How can I help you?',
104+
},
105+
],
106+
};
107+
}
108+
}
109+
110+
throw new Error(`Connector with id ${id} not found in mock`);
111+
}
112+
53113
export class UnsecuredActionsClientMock implements IUnsecuredActionsClient {
54114
getAll = jest
55115
.fn()
56116
.mockResolvedValue(Object.values(FakeConnectors) as ConnectorWithExtraFindData[]);
57117
execute = jest.fn().mockImplementation((options) => this.returnMockedConnectorResult(options));
58118
bulkEnqueueExecution = jest.fn().mockResolvedValue(undefined);
59119

60-
private async returnMockedConnectorResult({
120+
public async returnMockedConnectorResult({
61121
id,
62122
params,
63123
}: {
64124
id: string;
65125
params: Record<string, any>;
66126
spaceId: string;
67-
requesterId: string; // This is a custom ID for testing purposes
127+
requesterId: string;
68128
}): Promise<ActionTypeExecutorResult<unknown>> {
69-
const fakeConnector = Object.values(FakeConnectors).find((c) => c.id === id);
129+
return getMockedConnectorResult(id, params);
130+
}
131+
}
70132

71-
switch (fakeConnector?.name) {
72-
case FakeConnectors.slack1.name:
73-
case FakeConnectors.slack2.name: {
74-
return {
75-
status: 'ok',
76-
actionId: id,
77-
data: { text: 'ok' },
78-
};
79-
}
80-
case FakeConnectors.echo_inference.name: {
81-
return {
82-
status: 'ok',
83-
actionId: id,
84-
data: [
85-
{
86-
result: params?.text,
87-
},
88-
],
89-
};
90-
}
91-
case FakeConnectors.constantlyFailing.name: {
92-
throw new Error('Error: Constantly failing connector');
93-
}
94-
case FakeConnectors.slow_1sec_inference.name: {
95-
await new Promise<void>((resolve) => setTimeout(() => resolve(), 1000));
96-
return {
97-
status: 'ok',
98-
actionId: id,
99-
data: [
100-
{
101-
result: 'Hello! How can I help you?',
102-
},
103-
],
104-
};
105-
}
106-
case FakeConnectors.slow_3sec_inference.name: {
107-
await new Promise<void>((resolve) => setTimeout(() => resolve(), 3000));
108-
return {
109-
status: 'ok',
110-
actionId: id,
111-
data: [
112-
{
113-
result: 'Hello! How can I help you?',
114-
},
115-
],
116-
};
117-
}
118-
}
133+
export class ScopedActionsClientMock implements Partial<ActionsClient> {
134+
getAll = jest
135+
.fn()
136+
.mockResolvedValue(Object.values(FakeConnectors) as ConnectorWithExtraFindData[]);
137+
execute = jest.fn().mockImplementation((options) => this.returnMockedConnectorResult(options));
138+
bulkEnqueueExecution = jest.fn().mockResolvedValue(undefined);
119139

120-
throw new Error(`Connector with id ${id} not found in mock`);
140+
public async returnMockedConnectorResult({
141+
actionId,
142+
params,
143+
}: {
144+
actionId: string;
145+
params: Record<string, any>;
146+
}): Promise<ActionTypeExecutorResult<unknown>> {
147+
return getMockedConnectorResult(actionId, params);
121148
}
122149
}

‎src/platform/plugins/shared/workflows_execution_engine/integration_tests/workflow_run_fixture.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import {
2828
StepExecutionRepositoryMock,
2929
WorkflowExecutionRepositoryMock,
3030
} from './mocks';
31-
import { UnsecuredActionsClientMock } from './mocks/actions_plugin.mock';
31+
import { ScopedActionsClientMock, UnsecuredActionsClientMock } from './mocks/actions_plugin.mock';
3232
import { TaskManagerMock } from './mocks/task_manager.mock';
3333
import type { WorkflowsExecutionEngineConfig } from '../server/config';
3434
import { resumeWorkflow } from '../server/execution_functions';
@@ -57,10 +57,29 @@ export class WorkflowRunFixture {
5757
debug: jest.fn(),
5858
trace: jest.fn(),
5959
} as unknown as Logger;
60+
// Create shared execute mock so tests can verify calls regardless of which client is used
61+
private readonly sharedExecuteMock = jest.fn();
6062
public readonly unsecuredActionsClientMock = new UnsecuredActionsClientMock();
63+
public readonly scopedActionsClientMock = new ScopedActionsClientMock();
6164
public readonly actionsClientMock = {
6265
getUnsecuredActionsClient: jest.fn().mockReturnValue(this.unsecuredActionsClientMock),
66+
getActionsClientWithRequest: jest.fn().mockResolvedValue(this.scopedActionsClientMock),
6367
} as unknown as ActionsPluginStartContract;
68+
69+
constructor() {
70+
// Wire both clients to use the same shared execute mock with normalized parameters
71+
this.unsecuredActionsClientMock.execute = this.sharedExecuteMock.mockImplementation((options) =>
72+
this.unsecuredActionsClientMock.returnMockedConnectorResult(options)
73+
);
74+
this.scopedActionsClientMock.execute = this.sharedExecuteMock.mockImplementation((options) => {
75+
// Normalize scoped client parameters to match unsecured client for test assertions
76+
// Convert actionId -> id so tests can check using 'id' parameter
77+
const normalizedOptions = { ...options, id: options.actionId };
78+
this.sharedExecuteMock.mock.calls[this.sharedExecuteMock.mock.calls.length - 1][0] =
79+
normalizedOptions;
80+
return this.scopedActionsClientMock.returnMockedConnectorResult(options);
81+
});
82+
}
6483
public readonly configMock = {
6584
logging: {
6685
console: true,

‎src/platform/plugins/shared/workflows_execution_engine/server/connector_executor.ts‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,14 @@
1212

1313
import { validate as validateUuid } from 'uuid';
1414
import type { ActionTypeExecutorResult } from '@kbn/actions-plugin/common';
15-
import type { IUnsecuredActionsClient } from '@kbn/actions-plugin/server';
15+
import type { ActionsClient, IUnsecuredActionsClient } from '@kbn/actions-plugin/server';
16+
import type { ConnectorWithExtraFindData } from '@kbn/actions-plugin/server/application/connector/types';
1617

1718
export class ConnectorExecutor {
18-
constructor(private actionsClient: IUnsecuredActionsClient) {}
19+
constructor(
20+
private actionsClient: IUnsecuredActionsClient | ActionsClient,
21+
private isScoped: boolean = false
22+
) {}
1923

2024
public async execute(
2125
connectorType: string,
@@ -49,26 +53,40 @@ export class ConnectorExecutor {
4953
connectorParams: Record<string, any>,
5054
spaceId: string
5155
): Promise<ActionTypeExecutorResult<unknown>> {
52-
let connectorId: string;
56+
const connectorId = await this.resolveConnectorId(connectorName, spaceId);
5357

54-
if (validateUuid(connectorName)) {
55-
connectorId = connectorName;
56-
} else {
57-
const allConnectors = await this.actionsClient.getAll('default');
58-
const connector = allConnectors.find((c) => c.name === connectorName);
59-
60-
if (!connector) {
61-
throw new Error(`Connector with name ${connectorName} not found`);
62-
}
63-
64-
connectorId = connector?.id;
58+
if (this.isScoped) {
59+
return (this.actionsClient as ActionsClient).execute({
60+
actionId: connectorId,
61+
params: connectorParams,
62+
});
6563
}
6664

67-
return this.actionsClient.execute({
65+
return (this.actionsClient as IUnsecuredActionsClient).execute({
6866
id: connectorId,
6967
params: connectorParams,
7068
spaceId,
71-
requesterId: 'background_task', // This is a custom ID for testing purposes
69+
requesterId: 'background_task',
7270
});
7371
}
72+
73+
private async resolveConnectorId(connectorName: string, spaceId: string): Promise<string> {
74+
if (validateUuid(connectorName)) {
75+
return connectorName;
76+
}
77+
78+
const allConnectors = this.isScoped
79+
? await (this.actionsClient as ActionsClient).getAll()
80+
: await (this.actionsClient as IUnsecuredActionsClient).getAll(spaceId);
81+
82+
const connector = allConnectors.find(
83+
(c: ConnectorWithExtraFindData) => c.name === connectorName
84+
);
85+
86+
if (!connector) {
87+
throw new Error(`Connector with name ${connectorName} not found`);
88+
}
89+
90+
return connector.id;
91+
}
7492
}

‎src/platform/plugins/shared/workflows_execution_engine/server/execution_functions/setup_dependencies.test.ts‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,19 @@ describe('setupDependencies', () => {
5151
startedAt: Date.now(),
5252
};
5353

54+
const mockUnsecuredActionsClient = {
55+
getAll: jest.fn(),
56+
execute: jest.fn(),
57+
};
58+
59+
const mockScopedActionsClient = {
60+
getAll: jest.fn(),
61+
execute: jest.fn(),
62+
};
63+
5464
const mockActionsPlugin = {
55-
getUnsecuredActionsClient: jest.fn().mockResolvedValue({}),
65+
getUnsecuredActionsClient: jest.fn().mockResolvedValue(mockUnsecuredActionsClient),
66+
getActionsClientWithRequest: jest.fn().mockResolvedValue(mockScopedActionsClient),
5667
} as unknown as ActionsPluginStartContract;
5768

5869
const mockTaskManager = {} as TaskManagerStartContract;
@@ -170,4 +181,69 @@ describe('setupDependencies', () => {
170181
expect(result.clientToUse).toBe(mockAsCurrentUser);
171182
expect(result.clientToUse).not.toBe(mockEsClient);
172183
});
184+
185+
it('should use unsecured actions client when fakeRequest is not provided', async () => {
186+
await setupDependencies(
187+
workflowRunId,
188+
spaceId,
189+
mockActionsPlugin,
190+
mockTaskManager,
191+
mockEsClient,
192+
mockLogger,
193+
mockConfig,
194+
mockWorkflowExecutionRepository,
195+
mockStepExecutionRepository,
196+
mockLogsRepository,
197+
{} as CoreStart,
198+
mockDependencies
199+
);
200+
201+
expect(mockActionsPlugin.getUnsecuredActionsClient).toHaveBeenCalled();
202+
expect(mockActionsPlugin.getActionsClientWithRequest).not.toHaveBeenCalled();
203+
expect(ConnectorExecutor).toHaveBeenCalledWith(mockUnsecuredActionsClient, false);
204+
});
205+
206+
it('should use scoped actions client when fakeRequest is provided', async () => {
207+
const mockScopedClient = {
208+
search: jest.fn(),
209+
index: jest.fn(),
210+
} as unknown as ElasticsearchClient;
211+
212+
const mockAsCurrentUser = mockScopedClient;
213+
const mockAsScoped = jest.fn().mockReturnValue({
214+
asCurrentUser: mockAsCurrentUser,
215+
});
216+
217+
const mockCoreStart = {
218+
elasticsearch: {
219+
client: {
220+
asScoped: mockAsScoped,
221+
},
222+
},
223+
} as unknown as CoreStart;
224+
225+
const mockFakeRequest = {
226+
headers: {},
227+
} as KibanaRequest;
228+
229+
await setupDependencies(
230+
workflowRunId,
231+
spaceId,
232+
mockActionsPlugin,
233+
mockTaskManager,
234+
mockEsClient,
235+
mockLogger,
236+
mockConfig,
237+
mockWorkflowExecutionRepository,
238+
mockStepExecutionRepository,
239+
mockLogsRepository,
240+
mockCoreStart,
241+
mockDependencies,
242+
mockFakeRequest
243+
);
244+
245+
expect(mockActionsPlugin.getActionsClientWithRequest).toHaveBeenCalledWith(mockFakeRequest);
246+
expect(mockActionsPlugin.getUnsecuredActionsClient).not.toHaveBeenCalled();
247+
expect(ConnectorExecutor).toHaveBeenCalledWith(mockScopedActionsClient, true);
248+
});
173249
});

‎src/platform/plugins/shared/workflows_execution_engine/server/execution_functions/setup_dependencies.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,17 @@ export async function setupDependencies(
6262
workflowExecutionGraph = workflowExecutionGraph.getStepGraph(workflowExecution.stepId);
6363
}
6464

65-
const unsecuredActionsClient = await actionsPlugin.getUnsecuredActionsClient();
66-
const connectorExecutor = new ConnectorExecutor(unsecuredActionsClient);
65+
// Use scoped actions client when fakeRequest is available to preserve user context
66+
// Otherwise fallback to unsecured actions client
67+
// TODO(tb): Consider completely disabling connectors when no fakeRequest is available
68+
let connectorExecutor: ConnectorExecutor;
69+
if (fakeRequest) {
70+
const scopedActionsClient = await actionsPlugin.getActionsClientWithRequest(fakeRequest);
71+
connectorExecutor = new ConnectorExecutor(scopedActionsClient, true);
72+
} else {
73+
const unsecuredActionsClient = await actionsPlugin.getUnsecuredActionsClient();
74+
connectorExecutor = new ConnectorExecutor(unsecuredActionsClient, false);
75+
}
6776

6877
const workflowLogger = new WorkflowEventLogger(
6978
logsRepository,

0 commit comments

Comments
 (0)