Skip to content

[ResponseOps] Cleanup alerting RBAC exception code #197719

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

Merged
doakalexi merged 7 commits into from
Oct 30, 2024
Merged

[ResponseOps] Cleanup alerting RBAC exception code #197719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7b1e89
Removing legacy rbac code
doakalexi Oct 24, 2024
190b937
Removing the legacy functional tests
doakalexi Oct 24, 2024
c41bb9b
Merge branch 'main' into remove-legacy-rbac
doakalexi Oct 24, 2024
c4b9961
Merge branch 'main' into remove-legacy-rbac
doakalexi Oct 28, 2024
83caa27
Adding back tests
doakalexi Oct 30, 2024
81c10a3
Merge branch 'remove-legacy-rbac' of github.com:doakalexi/kibana into…
doakalexi Oct 30, 2024
d8945b4
Merge branch 'main' into remove-legacy-rbac
doakalexi Oct 30, 2024
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
114 changes: 0 additions & 114 deletions x-pack/plugins/actions/server/actions_client/actions_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,7 @@ import { usageCountersServiceMock } from '@kbn/usage-collection-plugin/server/us
import { actionExecutorMock } from '../lib/action_executor.mock';
import { v4 as uuidv4 } from 'uuid';
import { ActionsAuthorization } from '../authorization/actions_authorization';
import {
getAuthorizationModeBySource,
AuthorizationMode,
bulkGetAuthorizationModeBySource,
} from '../authorization/get_authorization_mode_by_source';
import { actionsAuthorizationMock } from '../authorization/actions_authorization.mock';
import { trackLegacyRBACExemption } from '../lib/track_legacy_rbac_exemption';
import { ConnectorTokenClient } from '../lib/connector_token_client';
import { encryptedSavedObjectsMock } from '@kbn/encrypted-saved-objects-plugin/server/mocks';
import { SavedObject } from '@kbn/core/server';
Expand All @@ -68,25 +62,6 @@ jest.mock('@kbn/core-saved-objects-utils-server', () => {
};
});

jest.mock('../lib/track_legacy_rbac_exemption', () => ({
trackLegacyRBACExemption: jest.fn(),
}));

jest.mock('../authorization/get_authorization_mode_by_source', () => {
return {
getAuthorizationModeBySource: jest.fn(() => {
return 1;
}),
bulkGetAuthorizationModeBySource: jest.fn(() => {
return 1;
}),
AuthorizationMode: {
Legacy: 0,
RBAC: 1,
},
};
});

jest.mock('../lib/get_oauth_jwt_access_token', () => ({
getOAuthJwtAccessToken: jest.fn(),
}));
Expand Down Expand Up @@ -2745,9 +2720,6 @@ describe('update()', () => {
describe('execute()', () => {
describe('authorization', () => {
Copy link
Contributor

@ymao1 ymao1 Oct 28, 2024

Choose a reason for hiding this comment

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

we should keep the RBAC authorization tests right?

Copy link
Contributor Author

@doakalexi doakalexi Oct 28, 2024

Choose a reason for hiding this comment

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

Yes you're right! I prob shouldn't have removed all of them

Copy link
Contributor Author

@doakalexi doakalexi Oct 30, 2024

Choose a reason for hiding this comment

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

Resolved in this commit 83caa27

test('ensures user is authorised to excecute actions', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
unsecuredSavedObjectsClient.get.mockResolvedValueOnce(actionTypeIdFromSavedObjectMock());
await actionsClient.execute({
actionId: 'action-id',
Expand All @@ -2764,9 +2736,6 @@ describe('execute()', () => {
});

test('throws when user is not authorised to create the type of action', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
authorization.ensureAuthorized.mockRejectedValue(
new Error(`Unauthorized to execute all actions`)
);
Expand All @@ -2790,28 +2759,7 @@ describe('execute()', () => {
});
});

test('tracks legacy RBAC', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.Legacy;
});

await actionsClient.execute({
actionId: 'action-id',
params: {
name: 'my name',
},
source: asHttpRequestExecutionSource(request),
});

expect(trackLegacyRBACExemption as jest.Mock).toBeCalledWith('execute', mockUsageCounter);
expect(authorization.ensureAuthorized).not.toHaveBeenCalled();
});

test('ensures that system actions privileges are being authorized correctly', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});

actionsClient = new ActionsClient({
inMemoryConnectors: [
{
Expand Down Expand Up @@ -2872,10 +2820,6 @@ describe('execute()', () => {
});

test('does not authorize kibana privileges for non system actions', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});

actionsClient = new ActionsClient({
inMemoryConnectors: [
{
Expand Down Expand Up @@ -2939,10 +2883,6 @@ describe('execute()', () => {
});

test('pass the params to the actionTypeRegistry when authorizing system actions', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});

const getKibanaPrivileges = jest.fn().mockReturnValue(['test/create']);

actionsClient = new ActionsClient({
Expand Down Expand Up @@ -3106,9 +3046,6 @@ describe('execute()', () => {
describe('bulkEnqueueExecution()', () => {
describe('authorization', () => {
test('ensures user is authorised to execute actions', async () => {
(bulkGetAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return { [AuthorizationMode.RBAC]: 1, [AuthorizationMode.Legacy]: 0 };
});
await actionsClient.bulkEnqueueExecution([
{
id: uuidv4(),
Expand Down Expand Up @@ -3136,9 +3073,6 @@ describe('bulkEnqueueExecution()', () => {
});

test('throws when user is not authorised to create the type of action', async () => {
(bulkGetAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return { [AuthorizationMode.RBAC]: 1, [AuthorizationMode.Legacy]: 0 };
});
authorization.ensureAuthorized.mockRejectedValue(
new Error(`Unauthorized to execute all actions`)
);
Expand Down Expand Up @@ -3170,45 +3104,9 @@ describe('bulkEnqueueExecution()', () => {
operation: 'execute',
});
});

test('tracks legacy RBAC', async () => {
(bulkGetAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return { [AuthorizationMode.RBAC]: 0, [AuthorizationMode.Legacy]: 2 };
});

await actionsClient.bulkEnqueueExecution([
{
id: uuidv4(),
params: {},
spaceId: 'default',
executionId: '123abc',
apiKey: null,
source: asHttpRequestExecutionSource(request),
actionTypeId: 'my-action-type',
},
{
id: uuidv4(),
params: {},
spaceId: 'default',
executionId: '456def',
apiKey: null,
source: asHttpRequestExecutionSource(request),
actionTypeId: 'my-action-type',
},
]);

expect(trackLegacyRBACExemption as jest.Mock).toBeCalledWith(
'bulkEnqueueExecution',
mockUsageCounter,
2
);
});
});

test('calls the bulkExecutionEnqueuer with the appropriate parameters', async () => {
(bulkGetAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return { [AuthorizationMode.RBAC]: 0, [AuthorizationMode.Legacy]: 0 };
});
const opts = [
{
id: uuidv4(),
Expand Down Expand Up @@ -3504,17 +3402,11 @@ describe('getGlobalExecutionLogWithAuth()', () => {
test('ensures user is authorised to access logs', async () => {
eventLogClient.aggregateEventsWithAuthFilter.mockResolvedValue(results);

(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
await actionsClient.getGlobalExecutionLogWithAuth(opts);
expect(authorization.ensureAuthorized).toHaveBeenCalledWith({ operation: 'get' });
});

test('throws when user is not authorised to access logs', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
authorization.ensureAuthorized.mockRejectedValue(new Error(`Unauthorized to access logs`));

await expect(actionsClient.getGlobalExecutionLogWithAuth(opts)).rejects.toMatchInlineSnapshot(
Expand Down Expand Up @@ -3563,17 +3455,11 @@ describe('getGlobalExecutionKpiWithAuth()', () => {
test('ensures user is authorised to access kpi', async () => {
eventLogClient.aggregateEventsWithAuthFilter.mockResolvedValue(results);

(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
await actionsClient.getGlobalExecutionKpiWithAuth(opts);
expect(authorization.ensureAuthorized).toHaveBeenCalledWith({ operation: 'get' });
});

test('throws when user is not authorised to access kpi', async () => {
(getAuthorizationModeBySource as jest.Mock).mockImplementationOnce(() => {
return AuthorizationMode.RBAC;
});
authorization.ensureAuthorized.mockRejectedValue(new Error(`Unauthorized to access kpi`));

await expect(actionsClient.getGlobalExecutionKpiWithAuth(opts)).rejects.toMatchInlineSnapshot(
Expand Down
65 changes: 17 additions & 48 deletions x-pack/plugins/actions/server/actions_client/actions_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import url from 'url';
import { UsageCounter } from '@kbn/usage-collection-plugin/server';

import { i18n } from '@kbn/i18n';
import { compact, uniq } from 'lodash';
import { uniq } from 'lodash';
import {
IScopedClusterClient,
SavedObjectsClientContract,
Expand All @@ -35,7 +35,7 @@ import {
IExecutionLogResult,
} from '../../common';
import { ActionTypeRegistry } from '../action_type_registry';
import { ActionExecutorContract, ActionExecutionSource, parseDate } from '../lib';
import { ActionExecutorContract, parseDate } from '../lib';
import {
ActionResult,
RawAction,
Expand All @@ -52,13 +52,7 @@ import {
ExecutionResponse,
} from '../create_execute_function';
import { ActionsAuthorization } from '../authorization/actions_authorization';
import {
getAuthorizationModeBySource,
bulkGetAuthorizationModeBySource,
AuthorizationMode,
} from '../authorization/get_authorization_mode_by_source';
import { connectorAuditEvent, ConnectorAuditAction } from '../lib/audit_events';
import { trackLegacyRBACExemption } from '../lib/track_legacy_rbac_exemption';
import { ActionsConfigurationUtilities } from '../actions_config';
import {
OAuthClientCredentialsParams,
Expand Down Expand Up @@ -496,51 +490,26 @@ export class ActionsClient {
public async bulkEnqueueExecution(
options: EnqueueExecutionOptions[]
): Promise<ExecutionResponse> {
const sources: Array<ActionExecutionSource<unknown>> = compact(
(options ?? []).map((option) => option.source)
);

const authModes = await bulkGetAuthorizationModeBySource(
this.context.logger,
this.context.unsecuredSavedObjectsClient,
sources
/**
* For scheduled executions the additional authorization check
* for system actions (kibana privileges) will be performed
* inside the ActionExecutor at execution time
*/
await this.context.authorization.ensureAuthorized({ operation: 'execute' });
await Promise.all(
uniq(options.map((o) => o.actionTypeId)).map((actionTypeId) =>
this.context.authorization.ensureAuthorized({ operation: 'execute', actionTypeId })
)
);
if (authModes[AuthorizationMode.RBAC] > 0) {
/**
* For scheduled executions the additional authorization check
* for system actions (kibana privileges) will be performed
* inside the ActionExecutor at execution time
*/
await this.context.authorization.ensureAuthorized({ operation: 'execute' });
await Promise.all(
uniq(options.map((o) => o.actionTypeId)).map((actionTypeId) =>
this.context.authorization.ensureAuthorized({ operation: 'execute', actionTypeId })
)
);
}
if (authModes[AuthorizationMode.Legacy] > 0) {
trackLegacyRBACExemption(
'bulkEnqueueExecution',
this.context.usageCounter,
authModes[AuthorizationMode.Legacy]
);
}
return this.context.bulkExecutionEnqueuer(this.context.unsecuredSavedObjectsClient, options);
}

public async ephemeralEnqueuedExecution(options: EnqueueExecutionOptions): Promise<RunNowResult> {
const { source } = options;
if (
(await getAuthorizationModeBySource(this.context.unsecuredSavedObjectsClient, source)) ===
AuthorizationMode.RBAC
) {
await this.context.authorization.ensureAuthorized({
operation: 'execute',
actionTypeId: options.actionTypeId,
});
} else {
trackLegacyRBACExemption('ephemeralEnqueuedExecution', this.context.usageCounter);
}
await this.context.authorization.ensureAuthorized({
operation: 'execute',
actionTypeId: options.actionTypeId,
});

return this.context.ephemeralExecutionEnqueuer(
this.context.unsecuredSavedObjectsClient,
options
Expand Down
64 changes: 25 additions & 39 deletions x-pack/plugins/actions/server/application/connector/methods/execute/execute.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ import { RawAction, ActionTypeExecutorResult } from '../../../../types';
import { getSystemActionKibanaPrivileges } from '../../../../lib/get_system_action_kibana_privileges';
import { isPreconfigured } from '../../../../lib/is_preconfigured';
import { isSystemAction } from '../../../../lib/is_system_action';
import {
getAuthorizationModeBySource,
AuthorizationMode,
} from '../../../../authorization/get_authorization_mode_by_source';
import { trackLegacyRBACExemption } from '../../../../lib/track_legacy_rbac_exemption';
import { ConnectorExecuteParams } from './types';
import { ACTION_SAVED_OBJECT_TYPE } from '../../../../constants/saved_objects';
import { ActionsClientContext } from '../../../../actions_client';
Expand All @@ -25,43 +20,34 @@ export async function execute(
): Promise<ActionTypeExecutorResult<unknown>> {
const log = context.logger;
const { actionId, params, source, relatedSavedObjects } = connectorExecuteParams;

if (
(await getAuthorizationModeBySource(context.unsecuredSavedObjectsClient, source)) ===
AuthorizationMode.RBAC
) {
const additionalPrivileges = getSystemActionKibanaPrivileges(context, actionId, params);
let actionTypeId: string | undefined;

try {
if (isPreconfigured(context, actionId) || isSystemAction(context, actionId)) {
const connector = context.inMemoryConnectors.find(
(inMemoryConnector) => inMemoryConnector.id === actionId
);

actionTypeId = connector?.actionTypeId;
} else {
// TODO: Optimize so we don't do another get on top of getAuthorizationModeBySource and within the actionExecutor.execute
const { attributes } = await context.unsecuredSavedObjectsClient.get<RawAction>(
ACTION_SAVED_OBJECT_TYPE,
actionId
);

actionTypeId = attributes.actionTypeId;
}
} catch (err) {
log.debug(`Failed to retrieve actionTypeId for action [${actionId}]`, err);
const additionalPrivileges = getSystemActionKibanaPrivileges(context, actionId, params);
let actionTypeId: string | undefined;

try {
if (isPreconfigured(context, actionId) || isSystemAction(context, actionId)) {
const connector = context.inMemoryConnectors.find(
(inMemoryConnector) => inMemoryConnector.id === actionId
);

actionTypeId = connector?.actionTypeId;
} else {
const { attributes } = await context.unsecuredSavedObjectsClient.get<RawAction>(
ACTION_SAVED_OBJECT_TYPE,
actionId
);

actionTypeId = attributes.actionTypeId;
}

await context.authorization.ensureAuthorized({
operation: 'execute',
additionalPrivileges,
actionTypeId,
});
} else {
trackLegacyRBACExemption('execute', context.usageCounter);
} catch (err) {
log.debug(`Failed to retrieve actionTypeId for action [${actionId}]`, err);
}

await context.authorization.ensureAuthorized({
operation: 'execute',
additionalPrivileges,
actionTypeId,
});

return context.actionExecutor.execute({
actionId,
params,
Expand Down
Loading