mirror of
https://github.com/elastic/kibana.git
synced 2025-04-24 09:48:58 -04:00
Remove RBAC from action task execution (#151859)
Resolves https://github.com/elastic/kibana/issues/150496 In this PR, I'm removing the redundant RBAC check happening during the task execution. Since the RBAC check already happens prior to enqueueing the task in the actions client (before enqueue or execute), there is no need to check again before the task runs. ## To verify Follow scenario outlined in the bug report (https://github.com/elastic/kibana/issues/150496) with a connector that isn't pre-configured. You will notice the errors happening when you test against main and the errors go away (and go through successfully) when testing against this branch.
This commit is contained in:
Mike Côté
GitHub
parent
5158eda46e
commit
fa6c0d1c31
3 changed files with 19 additions and 121 deletions
|
|
@ -14,13 +14,11 @@ import { loggingSystemMock } from '@kbn/core/server/mocks';
|
|||
import { eventLoggerMock } from '@kbn/event-log-plugin/server/mocks';
|
||||
import { spacesServiceMock } from '@kbn/spaces-plugin/server/spaces_service/spaces_service.mock';
|
||||
import { ActionType } from '../types';
|
||||
import { actionsMock, actionsClientMock } from '../mocks';
|
||||
import { pick } from 'lodash';
|
||||
import { actionsMock } from '../mocks';
|
||||
|
||||
const actionExecutor = new ActionExecutor({ isESOCanEncrypt: true });
|
||||
const services = actionsMock.createServices();
|
||||
|
||||
const actionsClient = actionsClientMock.create();
|
||||
const encryptedSavedObjectsClient = encryptedSavedObjectsMock.createClient();
|
||||
const actionTypeRegistry = actionTypeRegistryMock.create();
|
||||
const eventLogger = eventLoggerMock.create();
|
||||
|
|
@ -39,12 +37,10 @@ const spacesMock = spacesServiceMock.createStartContract();
|
|||
const loggerMock: ReturnType<typeof loggingSystemMock.createLogger> =
|
||||
loggingSystemMock.createLogger();
|
||||
|
||||
const getActionsClientWithRequest = jest.fn();
|
||||
actionExecutor.initialize({
|
||||
logger: loggerMock,
|
||||
spaces: spacesMock,
|
||||
getServices: () => services,
|
||||
getActionsClientWithRequest,
|
||||
actionTypeRegistry,
|
||||
encryptedSavedObjectsClient,
|
||||
eventLogger,
|
||||
|
|
@ -68,7 +64,6 @@ actionExecutor.initialize({
|
|||
beforeEach(() => {
|
||||
jest.resetAllMocks();
|
||||
spacesMock.getSpaceId.mockReturnValue('some-namespace');
|
||||
getActionsClientWithRequest.mockResolvedValue(actionsClient);
|
||||
loggerMock.get.mockImplementation(() => loggerMock);
|
||||
});
|
||||
|
||||
|
|
@ -84,6 +79,7 @@ test('successfully executes', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
config: {
|
||||
bar: true,
|
||||
|
|
@ -94,14 +90,6 @@ test('successfully executes', async () => {
|
|||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
...pick(actionSavedObject.attributes, 'actionTypeId', 'config'),
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
await actionExecutor.execute(executeParams);
|
||||
|
|
@ -224,7 +212,6 @@ test('successfully executes with preconfigured connector', async () => {
|
|||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
await actionExecutor.execute({ ...executeParams, actionId: 'preconfigured' });
|
||||
|
||||
expect(actionsClient.get).not.toHaveBeenCalled();
|
||||
expect(encryptedSavedObjectsClient.getDecryptedAsInternalUser).not.toHaveBeenCalled();
|
||||
|
||||
expect(actionTypeRegistry.get).toHaveBeenCalledWith('test');
|
||||
|
|
@ -341,6 +328,7 @@ test('successfully executes as a task', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
config: {
|
||||
bar: true,
|
||||
|
|
@ -351,14 +339,6 @@ test('successfully executes as a task', async () => {
|
|||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
...pick(actionSavedObject.attributes, 'actionTypeId', 'config'),
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
|
||||
|
|
@ -392,18 +372,11 @@ test('provides empty config when config and / or secrets is empty', async () =>
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
actionTypeId: actionSavedObject.attributes.actionTypeId,
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
await actionExecutor.execute(executeParams);
|
||||
|
|
@ -432,18 +405,11 @@ test('throws an error when config is invalid', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
actionTypeId: actionSavedObject.attributes.actionTypeId,
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
|
||||
|
|
@ -473,18 +439,11 @@ test('throws an error when connector is invalid', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
actionTypeId: actionSavedObject.attributes.actionTypeId,
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
|
||||
|
|
@ -516,18 +475,11 @@ test('throws an error when params is invalid', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
actionTypeId: actionSavedObject.attributes.actionTypeId,
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
|
||||
|
|
@ -541,7 +493,9 @@ test('throws an error when params is invalid', async () => {
|
|||
});
|
||||
|
||||
test('throws an error when failing to load action through savedObjectsClient', async () => {
|
||||
actionsClient.get.mockRejectedValueOnce(new Error('No access'));
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockRejectedValueOnce(
|
||||
new Error('No access')
|
||||
);
|
||||
await expect(actionExecutor.execute(executeParams)).rejects.toThrowErrorMatchingInlineSnapshot(
|
||||
`"No access"`
|
||||
);
|
||||
|
|
@ -559,18 +513,11 @@ test('throws an error if actionType is not enabled', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
actionTypeId: actionSavedObject.attributes.actionTypeId,
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
actionTypeRegistry.ensureActionTypeEnabled.mockImplementationOnce(() => {
|
||||
|
|
@ -595,6 +542,7 @@ test('should not throws an error if actionType is preconfigured', async () => {
|
|||
id: '1',
|
||||
type: 'action',
|
||||
attributes: {
|
||||
name: '1',
|
||||
actionTypeId: 'test',
|
||||
config: {
|
||||
bar: true,
|
||||
|
|
@ -605,14 +553,6 @@ test('should not throws an error if actionType is preconfigured', async () => {
|
|||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.id,
|
||||
...pick(actionSavedObject.attributes, 'actionTypeId', 'config', 'secrets'),
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
actionTypeRegistry.ensureActionTypeEnabled.mockImplementationOnce(() => {
|
||||
|
|
@ -641,7 +581,6 @@ test('throws an error when passing isESOCanEncrypt with value of false', async (
|
|||
customActionExecutor.initialize({
|
||||
logger: loggingSystemMock.create().get(),
|
||||
spaces: spacesMock,
|
||||
getActionsClientWithRequest,
|
||||
getServices: () => services,
|
||||
actionTypeRegistry,
|
||||
encryptedSavedObjectsClient,
|
||||
|
|
@ -660,7 +599,6 @@ test('should not throw error if action is preconfigured and isESOCanEncrypt is f
|
|||
customActionExecutor.initialize({
|
||||
logger: loggingSystemMock.create().get(),
|
||||
spaces: spacesMock,
|
||||
getActionsClientWithRequest,
|
||||
getServices: () => services,
|
||||
actionTypeRegistry,
|
||||
encryptedSavedObjectsClient,
|
||||
|
|
@ -692,7 +630,6 @@ test('should not throw error if action is preconfigured and isESOCanEncrypt is f
|
|||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
await actionExecutor.execute({ ...executeParams, actionId: 'preconfigured' });
|
||||
|
||||
expect(actionsClient.get).not.toHaveBeenCalled();
|
||||
expect(encryptedSavedObjectsClient.getDecryptedAsInternalUser).not.toHaveBeenCalled();
|
||||
|
||||
expect(actionTypeRegistry.get).toHaveBeenCalledWith('test');
|
||||
|
|
@ -1018,8 +955,8 @@ function setupActionExecutorMock() {
|
|||
const actionSavedObject = {
|
||||
id: '1',
|
||||
type: 'action',
|
||||
name: 'action-1',
|
||||
attributes: {
|
||||
name: 'action-1',
|
||||
actionTypeId: 'test',
|
||||
config: {
|
||||
bar: true,
|
||||
|
|
@ -1030,14 +967,6 @@ function setupActionExecutorMock() {
|
|||
},
|
||||
references: [],
|
||||
};
|
||||
const actionResult = {
|
||||
id: actionSavedObject.id,
|
||||
name: actionSavedObject.name,
|
||||
...pick(actionSavedObject.attributes, 'actionTypeId', 'config'),
|
||||
isPreconfigured: false,
|
||||
isDeprecated: false,
|
||||
};
|
||||
actionsClient.get.mockResolvedValueOnce(actionResult);
|
||||
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce(actionSavedObject);
|
||||
actionTypeRegistry.get.mockReturnValueOnce(actionType);
|
||||
return actionType.executor;
|
||||
|
|
|
|||
|
|
@ -29,7 +29,6 @@ import {
|
|||
ValidatorServices,
|
||||
} from '../types';
|
||||
import { EVENT_LOG_ACTIONS } from '../constants/event_log';
|
||||
import { ActionsClient } from '../actions_client';
|
||||
import { ActionExecutionSource } from './action_execution_source';
|
||||
import { RelatedSavedObjects } from './related_saved_objects';
|
||||
import { createActionEventLogRecordObject } from './create_action_event_log_record_object';
|
||||
|
|
@ -42,10 +41,6 @@ export interface ActionExecutorContext {
|
|||
logger: Logger;
|
||||
spaces?: SpacesServiceStart;
|
||||
getServices: GetServicesFunction;
|
||||
getActionsClientWithRequest: (
|
||||
request: KibanaRequest,
|
||||
authorizationContext?: ActionExecutionSource<unknown>
|
||||
) => Promise<PublicMethodsOf<ActionsClient>>;
|
||||
encryptedSavedObjectsClient: EncryptedSavedObjectsClient;
|
||||
actionTypeRegistry: ActionTypeRegistryContract;
|
||||
eventLogger: IEventLogger;
|
||||
|
|
@ -95,7 +90,6 @@ export class ActionExecutor {
|
|||
actionId,
|
||||
params,
|
||||
request,
|
||||
source,
|
||||
isEphemeral,
|
||||
taskInfo,
|
||||
executionId,
|
||||
|
|
@ -123,7 +117,6 @@ export class ActionExecutor {
|
|||
actionTypeRegistry,
|
||||
eventLogger,
|
||||
preconfiguredActions,
|
||||
getActionsClientWithRequest,
|
||||
} = this.actionExecutorContext!;
|
||||
|
||||
const services = getServices(request);
|
||||
|
|
@ -131,14 +124,11 @@ export class ActionExecutor {
|
|||
const namespace = spaceId && spaceId !== 'default' ? { namespace: spaceId } : {};
|
||||
|
||||
const actionInfo = await getActionInfoInternal(
|
||||
getActionsClientWithRequest,
|
||||
request,
|
||||
this.isESOCanEncrypt,
|
||||
encryptedSavedObjectsClient,
|
||||
preconfiguredActions,
|
||||
actionId,
|
||||
namespace.namespace,
|
||||
source
|
||||
namespace.namespace
|
||||
);
|
||||
|
||||
const { actionTypeId, name, config, secrets } = actionInfo;
|
||||
|
|
@ -313,26 +303,18 @@ export class ActionExecutor {
|
|||
source?: ActionExecutionSource<Source>;
|
||||
consumer?: string;
|
||||
}) {
|
||||
const {
|
||||
spaces,
|
||||
encryptedSavedObjectsClient,
|
||||
preconfiguredActions,
|
||||
eventLogger,
|
||||
getActionsClientWithRequest,
|
||||
} = this.actionExecutorContext!;
|
||||
const { spaces, encryptedSavedObjectsClient, preconfiguredActions, eventLogger } =
|
||||
this.actionExecutorContext!;
|
||||
|
||||
const spaceId = spaces && spaces.getSpaceId(request);
|
||||
const namespace = spaceId && spaceId !== 'default' ? { namespace: spaceId } : {};
|
||||
if (!this.actionInfo || this.actionInfo.actionId !== actionId) {
|
||||
this.actionInfo = await getActionInfoInternal(
|
||||
getActionsClientWithRequest,
|
||||
request,
|
||||
this.isESOCanEncrypt,
|
||||
encryptedSavedObjectsClient,
|
||||
preconfiguredActions,
|
||||
actionId,
|
||||
namespace.namespace,
|
||||
source
|
||||
namespace.namespace
|
||||
);
|
||||
}
|
||||
const task = taskInfo
|
||||
|
|
@ -382,17 +364,11 @@ interface ActionInfo {
|
|||
}
|
||||
|
||||
async function getActionInfoInternal<Source = unknown>(
|
||||
getActionsClientWithRequest: (
|
||||
request: KibanaRequest,
|
||||
authorizationContext?: ActionExecutionSource<unknown>
|
||||
) => Promise<PublicMethodsOf<ActionsClient>>,
|
||||
request: KibanaRequest,
|
||||
isESOCanEncrypt: boolean,
|
||||
encryptedSavedObjectsClient: EncryptedSavedObjectsClient,
|
||||
preconfiguredActions: PreConfiguredAction[],
|
||||
actionId: string,
|
||||
namespace: string | undefined,
|
||||
source?: ActionExecutionSource<Source>
|
||||
namespace: string | undefined
|
||||
): Promise<ActionInfo> {
|
||||
// check to see if it's a pre-configured action first
|
||||
const pcAction = preconfiguredActions.find(
|
||||
|
|
@ -415,14 +391,8 @@ async function getActionInfoInternal<Source = unknown>(
|
|||
);
|
||||
}
|
||||
|
||||
const actionsClient = await getActionsClientWithRequest(request, source);
|
||||
|
||||
// if not pre-configured action, should be a saved object
|
||||
// ensure user can read the action before processing
|
||||
const { actionTypeId, config, name } = await actionsClient.get({ id: actionId });
|
||||
|
||||
const {
|
||||
attributes: { secrets },
|
||||
attributes: { secrets, actionTypeId, config, name },
|
||||
} = await encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawAction>('action', actionId, {
|
||||
namespace: namespace === 'default' ? undefined : namespace,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -507,7 +507,6 @@ export class ActionsPlugin implements Plugin<PluginSetupContract, PluginStartCon
|
|||
logger,
|
||||
eventLogger: this.eventLogger!,
|
||||
spaces: plugins.spaces?.spacesService,
|
||||
getActionsClientWithRequest,
|
||||
getServices: this.getServicesFactory(
|
||||
getScopedSavedObjectsClientWithoutAccessToActions,
|
||||
core.elasticsearch,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue