From 9548cae2e339a3910149fe97b70a2fff21881e7c Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 13 Jul 2023 12:33:54 +0300 Subject: [PATCH 1/8] Disallow getting system actions --- .../actions/server/actions_client.test.ts | 124 ++++++++++++------ .../plugins/actions/server/actions_client.ts | 24 +++- .../group2/tests/actions/get.ts | 13 +- .../group2/tests/actions/get_all.ts | 27 ---- 4 files changed, 111 insertions(+), 77 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index a18a8adada424..0e9f2dbd32777 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -917,7 +917,7 @@ describe('get()', () => { getEventLogClient, }); - await actionsClient.get({ id: 'system-connector-.cases' }); + await expect(actionsClient.get({ id: 'system-connector-.cases' })).rejects.toThrow(); expect(authorization.ensureAuthorized).toHaveBeenCalledWith('get'); }); @@ -1158,7 +1158,7 @@ describe('get()', () => { expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); }); - it('return system action with id', async () => { + it('throws when getting a system action', async () => { actionsClient = new ActionsClient({ logger, actionTypeRegistry, @@ -1188,18 +1188,9 @@ describe('get()', () => { getEventLogClient, }); - const result = await actionsClient.get({ id: 'system-connector-.cases' }); - - expect(result).toEqual({ - id: 'system-connector-.cases', - actionTypeId: '.cases', - isPreconfigured: false, - isDeprecated: false, - isSystemAction: true, - name: 'System action: .cases', - }); - - expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); + await expect( + actionsClient.get({ id: 'system-connector-.cases' }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"Connector system-connector-.cases not found"`); }); }); @@ -1406,6 +1397,10 @@ describe('getAll()', () => { foo: 'bar', }, }, + /** + * System actions should not + * be returned from getAll + */ { id: 'system-connector-.cases', actionTypeId: '.cases', @@ -1425,15 +1420,6 @@ describe('getAll()', () => { const result = await actionsClient.getAll(); expect(result).toEqual([ - { - id: 'system-connector-.cases', - actionTypeId: '.cases', - name: 'System action: .cases', - isPreconfigured: false, - isSystemAction: true, - isDeprecated: false, - referencedByCount: 2, - }, { id: '1', name: 'test', @@ -1667,11 +1653,7 @@ describe('getBulk()', () => { getEventLogClient, }); - const result = await actionsClient.getBulk([ - '1', - 'testPreconfigured', - 'system-connector-.cases', - ]); + const result = await actionsClient.getBulk(['1', 'testPreconfigured']); expect(result).toEqual([ { @@ -1684,17 +1666,6 @@ describe('getBulk()', () => { name: 'test', config: { foo: 'bar' }, }, - { - id: 'system-connector-.cases', - actionTypeId: '.cases', - name: 'System action: .cases', - config: {}, - secrets: {}, - isDeprecated: false, - isMissingSecrets: false, - isPreconfigured: false, - isSystemAction: true, - }, { id: '1', actionTypeId: 'test', @@ -1707,6 +1678,81 @@ describe('getBulk()', () => { }, ]); }); + + test('should throw an error if a system action is requested', async () => { + unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({ + saved_objects: [ + { + id: '1', + type: 'action', + attributes: { + actionTypeId: 'test', + name: 'test', + config: { + foo: 'bar', + }, + isMissingSecrets: false, + }, + references: [], + }, + ], + }); + scopedClusterClient.asInternalUser.search.mockResponse( + // @ts-expect-error not full search response + { + aggregations: { + '1': { doc_count: 6 }, + testPreconfigured: { doc_count: 2 }, + 'system-connector-.cases': { doc_count: 2 }, + }, + } + ); + + actionsClient = new ActionsClient({ + logger, + actionTypeRegistry, + unsecuredSavedObjectsClient, + scopedClusterClient, + kibanaIndices, + actionExecutor, + executionEnqueuer, + ephemeralExecutionEnqueuer, + bulkExecutionEnqueuer, + request, + authorization: authorization as unknown as ActionsAuthorization, + inMemoryConnectors: [ + { + id: 'testPreconfigured', + actionTypeId: '.slack', + secrets: {}, + isPreconfigured: true, + isDeprecated: false, + isSystemAction: false, + name: 'test', + config: { + foo: 'bar', + }, + }, + { + id: 'system-connector-.cases', + actionTypeId: '.cases', + name: 'System action: .cases', + config: {}, + secrets: {}, + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: true, + }, + ], + connectorTokenClient: connectorTokenClientMock.create(), + getEventLogClient, + }); + + await expect( + actionsClient.getBulk(['1', 'testPreconfigured', 'system-connector-.cases']) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"Connector system-connector-.cases not found"`); + }); }); describe('getOAuthAccessToken()', () => { diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index db25e302a4ac0..63d011f69773e 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -410,7 +410,14 @@ export class ActionsClient { const foundInMemoryConnector = this.inMemoryConnectors.find((connector) => connector.id === id); - if (foundInMemoryConnector !== undefined) { + /** + * Getting system connector is not allowed + */ + if (foundInMemoryConnector !== undefined && foundInMemoryConnector.isSystemAction) { + throw Boom.notFound(`Connector ${id} not found`); + } + + if (foundInMemoryConnector !== undefined && foundInMemoryConnector.isPreconfigured) { this.auditLogger?.log( connectorAuditEvent({ action: ConnectorAuditAction.GET, @@ -483,9 +490,13 @@ export class ActionsClient { ) ); + const inMemoryConnectorsWithoutSystemActions = this.inMemoryConnectors.filter( + (connector) => !connector.isSystemAction + ); + const mergedResult = [ ...savedObjectsActions, - ...this.inMemoryConnectors.map((inMemoryConnector) => ({ + ...inMemoryConnectorsWithoutSystemActions.map((inMemoryConnector) => ({ id: inMemoryConnector.id, actionTypeId: inMemoryConnector.actionTypeId, name: inMemoryConnector.name, @@ -523,7 +534,14 @@ export class ActionsClient { (inMemoryConnector) => inMemoryConnector.id === actionId ); - if (action !== undefined) { + /** + * Getting system connector is not allowed + */ + if (action !== undefined && action.isSystemAction) { + throw Boom.notFound(`Connector ${action.id} not found`); + } + + if (action !== undefined && action.isPreconfigured) { actionResults.push(action); } } diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get.ts b/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get.ts index 42cfce82cb2e1..95e11720a0093 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get.ts @@ -161,7 +161,7 @@ export default function getActionTests({ getService }: FtrProviderContext) { } }); - it('should handle get system action request appropriately', async () => { + it('should throw when requesting a system action', async () => { const response = await supertestWithoutAuth .get( `${getUrlPrefix(space.id)}/api/actions/connector/system-connector-test.system-action` @@ -183,14 +183,11 @@ export default function getActionTests({ getService }: FtrProviderContext) { case 'superuser at space1': case 'space_1_all at space1': case 'space_1_all_with_restricted_fixture at space1': - expect(response.statusCode).to.eql(200); + expect(response.statusCode).to.eql(404); expect(response.body).to.eql({ - id: 'system-connector-test.system-action', - connector_type_id: 'test.system-action', - name: 'System action: test.system-action', - is_preconfigured: false, - is_system_action: true, - is_deprecated: false, + statusCode: 404, + error: 'Not Found', + message: 'Connector system-connector-test.system-action not found', }); break; default: diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get_all.ts b/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get_all.ts index bc3444b5a32b3..37475d7f191ca 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get_all.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/group2/tests/actions/get_all.ts @@ -125,15 +125,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referenced_by_count: 0, }, - { - connector_type_id: 'test.system-action', - id: 'system-connector-test.system-action', - is_deprecated: false, - is_preconfigured: false, - is_system_action: true, - name: 'System action: test.system-action', - referenced_by_count: 0, - }, { id: 'custom-system-abc-connector', is_preconfigured: true, @@ -294,15 +285,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referenced_by_count: 0, }, - { - connector_type_id: 'test.system-action', - id: 'system-connector-test.system-action', - is_deprecated: false, - is_preconfigured: false, - is_system_action: true, - name: 'System action: test.system-action', - referenced_by_count: 0, - }, { id: 'custom-system-abc-connector', is_preconfigured: true, @@ -426,15 +408,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referenced_by_count: 0, }, - { - connector_type_id: 'test.system-action', - id: 'system-connector-test.system-action', - is_deprecated: false, - is_preconfigured: false, - is_system_action: true, - name: 'System action: test.system-action', - referenced_by_count: 0, - }, { id: 'custom-system-abc-connector', is_preconfigured: true, From dd08b3d91eb631d54fddfc385818f76fe1471d18 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 13 Jul 2023 13:17:57 +0300 Subject: [PATCH 2/8] Do not return system action types --- .../actions/server/actions_client.test.ts | 46 +++++++++++++++++++ .../plugins/actions/server/actions_client.ts | 8 +++- .../server/routes/legacy/list_action_types.ts | 4 ++ .../tests/actions/connector_types.ts | 20 ++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 0e9f2dbd32777..f6e647ed40bfe 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -3116,6 +3116,52 @@ describe('bulkEnqueueExecution()', () => { }); }); +describe('listType()', () => { + it('filters out system action types', async () => { + mockedLicenseState.isLicenseValidForActionType.mockReturnValue({ isValid: true }); + + actionTypeRegistry.register({ + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['alerting'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + executor, + }); + + actionTypeRegistry.register({ + id: '.cases', + name: 'Cases', + minimumLicenseRequired: 'platinum', + supportedFeatureIds: ['alerting'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + isSystemActionType: true, + executor, + }); + + expect(await actionsClient.listTypes()).toEqual([ + { + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + enabled: true, + enabledInConfig: true, + enabledInLicense: true, + supportedFeatureIds: ['alerting'], + isSystemActionType: false, + }, + ]); + }); +}); + describe('isActionTypeEnabled()', () => { const fooActionType: ActionType = { id: 'foo', diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 63d011f69773e..707b1e28911c9 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -813,8 +813,14 @@ export class ActionsClient { return this.ephemeralExecutionEnqueuer(this.unsecuredSavedObjectsClient, options); } + /** + * Return all available action types + * expect system action types + */ public async listTypes(featureId?: string): Promise { - return this.actionTypeRegistry.list(featureId); + return this.actionTypeRegistry + .list(featureId) + .filter((actionType) => !Boolean(actionType.isSystemActionType)); } public isActionTypeEnabled( diff --git a/x-pack/plugins/actions/server/routes/legacy/list_action_types.ts b/x-pack/plugins/actions/server/routes/legacy/list_action_types.ts index 92213c0b9be61..1316d10f434b0 100644 --- a/x-pack/plugins/actions/server/routes/legacy/list_action_types.ts +++ b/x-pack/plugins/actions/server/routes/legacy/list_action_types.ts @@ -12,6 +12,10 @@ import { BASE_ACTION_API_PATH } from '../../../common'; import { ActionsRequestHandlerContext } from '../../types'; import { trackLegacyRouteUsage } from '../../lib/track_legacy_route_usage'; +/** + * Return all available action types + * expect system action types + */ export const listActionTypesRoute = ( router: IRouter, licenseState: ILicenseState, diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/connector_types.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/connector_types.ts index bb7fbbf0802d2..4ab680c3b5518 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/connector_types.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/connector_types.ts @@ -34,6 +34,16 @@ export default function listActionTypesTests({ getService }: FtrProviderContext) ).to.be(true); }); + it('should filter out system action types', async () => { + const response = await supertest.get( + `${getUrlPrefix(Spaces.space1.id)}/api/actions/connector_types` + ); + + const actionTypes = response.body as Array<{ is_system_action_type: boolean }>; + + expect(actionTypes.every((actionType) => !actionType.is_system_action_type)).to.be(true); + }); + describe('legacy', () => { it('should return 200 with list of action types containing defaults', async () => { const response = await supertest.get( @@ -53,6 +63,16 @@ export default function listActionTypesTests({ getService }: FtrProviderContext) response.body.some(createActionTypeMatcher('test.index-record', 'Test: Index Record')) ).to.be(true); }); + + it('should filter out system action types', async () => { + const response = await supertest.get( + `${getUrlPrefix(Spaces.space1.id)}/api/actions/list_action_types` + ); + + const actionTypes = response.body as Array<{ is_system_action_type: boolean }>; + + expect(actionTypes.every((actionType) => !actionType.is_system_action_type)).to.be(true); + }); }); }); } From db5a8fba4893244d7d87309d6a0cf0a62ddbe5d4 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 18 Jul 2023 10:44:01 +0300 Subject: [PATCH 3/8] Add flags to return system actions --- .../actions/server/actions_client.test.ts | 233 +++++++++++++++++- .../plugins/actions/server/actions_client.ts | 36 ++- .../actions/server/routes/connector_types.ts | 2 +- 3 files changed, 259 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index f6e647ed40bfe..39dd7289c6c84 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -1398,8 +1398,9 @@ describe('getAll()', () => { }, }, /** - * System actions should not + * System actions will not * be returned from getAll + * if no options are provided */ { id: 'system-connector-.cases', @@ -1441,6 +1442,114 @@ describe('getAll()', () => { }, ]); }); + + test('get system actions correctly', async () => { + const expectedResult = { + total: 1, + per_page: 10, + page: 1, + saved_objects: [ + { + id: '1', + type: 'type', + attributes: { + name: 'test', + isMissingSecrets: false, + config: { + foo: 'bar', + }, + }, + score: 1, + references: [], + }, + ], + }; + unsecuredSavedObjectsClient.find.mockResolvedValueOnce(expectedResult); + scopedClusterClient.asInternalUser.search.mockResponse( + // @ts-expect-error not full search response + { + aggregations: { + '1': { doc_count: 6 }, + testPreconfigured: { doc_count: 2 }, + 'system-connector-.cases': { doc_count: 2 }, + }, + } + ); + + actionsClient = new ActionsClient({ + logger, + actionTypeRegistry, + unsecuredSavedObjectsClient, + scopedClusterClient, + kibanaIndices, + actionExecutor, + executionEnqueuer, + ephemeralExecutionEnqueuer, + bulkExecutionEnqueuer, + request, + authorization: authorization as unknown as ActionsAuthorization, + inMemoryConnectors: [ + { + id: 'testPreconfigured', + actionTypeId: '.slack', + secrets: {}, + isPreconfigured: true, + isDeprecated: false, + isSystemAction: false, + name: 'test', + config: { + foo: 'bar', + }, + }, + { + id: 'system-connector-.cases', + actionTypeId: '.cases', + name: 'System action: .cases', + config: {}, + secrets: {}, + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: true, + }, + ], + connectorTokenClient: connectorTokenClientMock.create(), + getEventLogClient, + }); + + const result = await actionsClient.getAll({ includeSystemActions: true }); + + expect(result).toEqual([ + { + actionTypeId: '.cases', + id: 'system-connector-.cases', + isDeprecated: false, + isPreconfigured: false, + isSystemAction: true, + name: 'System action: .cases', + referencedByCount: 2, + }, + { + id: '1', + name: 'test', + isMissingSecrets: false, + config: { foo: 'bar' }, + isPreconfigured: false, + isDeprecated: false, + isSystemAction: false, + referencedByCount: 6, + }, + { + id: 'testPreconfigured', + actionTypeId: '.slack', + name: 'test', + isPreconfigured: true, + isSystemAction: false, + isDeprecated: false, + referencedByCount: 2, + }, + ]); + }); }); describe('getBulk()', () => { @@ -3117,7 +3226,7 @@ describe('bulkEnqueueExecution()', () => { }); describe('listType()', () => { - it('filters out system action types', async () => { + it('filters action types by feature ID', async () => { mockedLicenseState.isLicenseValidForActionType.mockReturnValue({ isValid: true }); actionTypeRegistry.register({ @@ -3133,6 +3242,62 @@ describe('listType()', () => { executor, }); + actionTypeRegistry.register({ + id: 'my-action-type-2', + name: 'My action type 2', + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['cases'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + executor, + }); + + expect(await actionsClient.listTypes({ featureId: 'alerting' })).toEqual([ + { + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + enabled: true, + enabledInConfig: true, + enabledInLicense: true, + supportedFeatureIds: ['alerting'], + isSystemActionType: false, + }, + ]); + }); + + it('filters out system action types when not defining options', async () => { + mockedLicenseState.isLicenseValidForActionType.mockReturnValue({ isValid: true }); + + actionTypeRegistry.register({ + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['alerting'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + executor, + }); + + actionTypeRegistry.register({ + id: 'my-action-type-2', + name: 'My action type 2', + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['cases'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + executor, + }); + actionTypeRegistry.register({ id: '.cases', name: 'Cases', @@ -3158,6 +3323,70 @@ describe('listType()', () => { supportedFeatureIds: ['alerting'], isSystemActionType: false, }, + { + id: 'my-action-type-2', + name: 'My action type 2', + isSystemActionType: false, + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['cases'], + enabled: true, + enabledInConfig: true, + enabledInLicense: true, + }, + ]); + }); + + it('return system action types when defining options', async () => { + mockedLicenseState.isLicenseValidForActionType.mockReturnValue({ isValid: true }); + + actionTypeRegistry.register({ + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + supportedFeatureIds: ['alerting'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + executor, + }); + + actionTypeRegistry.register({ + id: '.cases', + name: 'Cases', + minimumLicenseRequired: 'platinum', + supportedFeatureIds: ['alerting'], + validate: { + config: { schema: schema.object({}) }, + secrets: { schema: schema.object({}) }, + params: { schema: schema.object({}) }, + }, + isSystemActionType: true, + executor, + }); + + expect(await actionsClient.listTypes({ includeSystemActionTypes: true })).toEqual([ + { + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + enabled: true, + enabledInConfig: true, + enabledInLicense: true, + supportedFeatureIds: ['alerting'], + isSystemActionType: false, + }, + { + id: '.cases', + name: 'Cases', + isSystemActionType: true, + minimumLicenseRequired: 'platinum', + supportedFeatureIds: ['alerting'], + enabled: true, + enabledInConfig: true, + enabledInLicense: true, + }, ]); }); }); diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 707b1e28911c9..65724200030ab 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -133,6 +133,15 @@ export interface UpdateOptions { action: ActionUpdate; } +interface GetAllOptions { + includeSystemActions?: boolean; +} + +interface ListTypesOptions { + featureId?: string; + includeSystemActionTypes?: boolean; +} + export class ActionsClient { private readonly logger: Logger; private readonly kibanaIndices: string[]; @@ -459,7 +468,9 @@ export class ActionsClient { /** * Get all actions with in-memory connectors */ - public async getAll(): Promise { + public async getAll({ includeSystemActions = false }: GetAllOptions = {}): Promise< + FindActionResult[] + > { try { await this.authorization.ensureAuthorized('get'); } catch (error) { @@ -490,13 +501,13 @@ export class ActionsClient { ) ); - const inMemoryConnectorsWithoutSystemActions = this.inMemoryConnectors.filter( - (connector) => !connector.isSystemAction - ); + const inMemoryConnectorsFiltered = includeSystemActions + ? this.inMemoryConnectors + : this.inMemoryConnectors.filter((connector) => !connector.isSystemAction); const mergedResult = [ ...savedObjectsActions, - ...inMemoryConnectorsWithoutSystemActions.map((inMemoryConnector) => ({ + ...inMemoryConnectorsFiltered.map((inMemoryConnector) => ({ id: inMemoryConnector.id, actionTypeId: inMemoryConnector.actionTypeId, name: inMemoryConnector.name, @@ -817,10 +828,17 @@ export class ActionsClient { * Return all available action types * expect system action types */ - public async listTypes(featureId?: string): Promise { - return this.actionTypeRegistry - .list(featureId) - .filter((actionType) => !Boolean(actionType.isSystemActionType)); + public async listTypes({ + featureId, + includeSystemActionTypes = false, + }: ListTypesOptions = {}): Promise { + const actionTypes = this.actionTypeRegistry.list(featureId); + + const filteredActionTypes = includeSystemActionTypes + ? actionTypes + : actionTypes.filter((actionType) => !Boolean(actionType.isSystemActionType)); + + return filteredActionTypes; } public isActionTypeEnabled( diff --git a/x-pack/plugins/actions/server/routes/connector_types.ts b/x-pack/plugins/actions/server/routes/connector_types.ts index 39c24b6c178c8..d54b35a7a99df 100644 --- a/x-pack/plugins/actions/server/routes/connector_types.ts +++ b/x-pack/plugins/actions/server/routes/connector_types.ts @@ -51,7 +51,7 @@ export const connectorTypesRoute = ( verifyAccessAndContext(licenseState, async function (context, req, res) { const actionsClient = (await context.actions).getActionsClient(); return res.ok({ - body: rewriteBodyRes(await actionsClient.listTypes(req.query?.feature_id)), + body: rewriteBodyRes(await actionsClient.listTypes({ featureId: req.query?.feature_id })), }); }) ) From 912188769bc529afa1dc8b4504199ab6bc9eaf06 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 18 Jul 2023 13:05:14 +0300 Subject: [PATCH 4/8] Fix tests --- .../spaces_only/tests/actions/get_all.ts | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts index 2366c392c4e4d..d3cc7cccd47b8 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts @@ -114,15 +114,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referenced_by_count: 0, }, - { - connector_type_id: 'test.system-action', - id: 'system-connector-test.system-action', - is_deprecated: false, - is_preconfigured: false, - is_system_action: true, - name: 'System action: test.system-action', - referenced_by_count: 0, - }, { id: 'custom-system-abc-connector', is_preconfigured: true, @@ -235,15 +226,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referenced_by_count: 0, }, - { - connector_type_id: 'test.system-action', - id: 'system-connector-test.system-action', - is_deprecated: false, - is_preconfigured: false, - is_system_action: true, - name: 'System action: test.system-action', - referenced_by_count: 0, - }, { id: 'custom-system-abc-connector', is_preconfigured: true, From bb87ad3e3b2c8ab581a2f189a7d1ddc8046c579d Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 18 Jul 2023 14:01:20 +0300 Subject: [PATCH 5/8] Fix tests --- .../actions/server/routes/connector_types.test.ts | 4 +++- .../spaces_only/tests/actions/get_all.ts | 9 --------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/actions/server/routes/connector_types.test.ts b/x-pack/plugins/actions/server/routes/connector_types.test.ts index 51fa8c1630d5f..4c5a7089b75c4 100644 --- a/x-pack/plugins/actions/server/routes/connector_types.test.ts +++ b/x-pack/plugins/actions/server/routes/connector_types.test.ts @@ -142,7 +142,9 @@ describe('connectorTypesRoute', () => { expect(actionsClient.listTypes).toHaveBeenCalledTimes(1); expect(actionsClient.listTypes.mock.calls[0]).toMatchInlineSnapshot(` Array [ - "alerting", + Object { + "featureId": "alerting", + }, ] `); diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts index d3cc7cccd47b8..d4a43da31894e 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get_all.ts @@ -352,15 +352,6 @@ export default function getAllActionTests({ getService }: FtrProviderContext) { name: 'Slack#xyz', referencedByCount: 0, }, - { - actionTypeId: 'test.system-action', - id: 'system-connector-test.system-action', - isDeprecated: false, - isPreconfigured: false, - isSystemAction: true, - name: 'System action: test.system-action', - referencedByCount: 0, - }, { id: 'custom-system-abc-connector', isPreconfigured: true, From 5e94674b43e9a7fdfbd6efa5dd0e36ec692f132d Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 19 Jul 2023 13:09:20 +0300 Subject: [PATCH 6/8] Fix integration test --- .../spaces_only/tests/actions/get.ts | 22 ++++--------------- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get.ts index dc99597a901e1..a6957b399b1ea 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/get.ts @@ -90,21 +90,14 @@ export default function getActionTests({ getService }: FtrProviderContext) { }); }); - it('should handle get a system connector', async () => { + it('should return 404 when trying to get a system connector', async () => { await supertest .get( `${getUrlPrefix( Spaces.space1.id )}/api/actions/connector/system-connector-test.system-action` ) - .expect(200, { - id: 'system-connector-test.system-action', - connector_type_id: 'test.system-action', - name: 'System action: test.system-action', - is_preconfigured: false, - is_system_action: true, - is_deprecated: false, - }); + .expect(404); }); it('should handle get a deprecated connector', async () => { @@ -206,21 +199,14 @@ export default function getActionTests({ getService }: FtrProviderContext) { }); }); - it('should handle get a system connector', async () => { + it('should return 404 when trying to get a system connector', async () => { await supertest .get( `${getUrlPrefix( Spaces.space1.id )}/api/actions/action/system-connector-test.system-action` ) - .expect(200, { - id: 'system-connector-test.system-action', - actionTypeId: 'test.system-action', - name: 'System action: test.system-action', - isPreconfigured: false, - isSystemAction: true, - isDeprecated: false, - }); + .expect(404); }); }); }); From 8b24cc653875947e8bf9b6e86476b0b771471505 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 20 Jul 2023 17:49:08 +0300 Subject: [PATCH 7/8] Allow getting system actions with a flag --- .../actions/server/actions_client.test.ts | 153 +++++++++++++++++- .../plugins/actions/server/actions_client.ts | 29 +++- 2 files changed, 174 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 291c59a65e73d..548f91be0f2a4 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -1164,7 +1164,7 @@ describe('get()', () => { expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); }); - it('throws when getting a system action', async () => { + it('throws when getting a system action by default', async () => { actionsClient = new ActionsClient({ logger, actionTypeRegistry, @@ -1198,6 +1198,48 @@ describe('get()', () => { actionsClient.get({ id: 'system-connector-.cases' }) ).rejects.toThrowErrorMatchingInlineSnapshot(`"Connector system-connector-.cases not found"`); }); + + it('does not throw when getting a system action if throwIfSystemAction=false', async () => { + actionsClient = new ActionsClient({ + logger, + actionTypeRegistry, + unsecuredSavedObjectsClient, + scopedClusterClient, + kibanaIndices, + actionExecutor, + executionEnqueuer, + ephemeralExecutionEnqueuer, + bulkExecutionEnqueuer, + request, + authorization: authorization as unknown as ActionsAuthorization, + inMemoryConnectors: [ + { + id: 'system-connector-.cases', + actionTypeId: '.cases', + name: 'System action: .cases', + config: {}, + secrets: {}, + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: true, + }, + ], + connectorTokenClient: connectorTokenClientMock.create(), + getEventLogClient, + }); + + expect( + await actionsClient.get({ id: 'system-connector-.cases', throwIfSystemAction: false }) + ).toEqual({ + actionTypeId: '.cases', + id: 'system-connector-.cases', + isDeprecated: false, + isPreconfigured: false, + isSystemAction: true, + name: 'System action: .cases', + }); + }); }); describe('getAll()', () => { @@ -1794,7 +1836,7 @@ describe('getBulk()', () => { ]); }); - test('should throw an error if a system action is requested', async () => { + test('should throw an error if a system action is requested by default', async () => { unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -1868,6 +1910,113 @@ describe('getBulk()', () => { actionsClient.getBulk(['1', 'testPreconfigured', 'system-connector-.cases']) ).rejects.toThrowErrorMatchingInlineSnapshot(`"Connector system-connector-.cases not found"`); }); + + test('should throw an error if a system action is requested', async () => { + unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({ + saved_objects: [ + { + id: '1', + type: 'action', + attributes: { + actionTypeId: 'test', + name: 'test', + config: { + foo: 'bar', + }, + isMissingSecrets: false, + }, + references: [], + }, + ], + }); + scopedClusterClient.asInternalUser.search.mockResponse( + // @ts-expect-error not full search response + { + aggregations: { + '1': { doc_count: 6 }, + testPreconfigured: { doc_count: 2 }, + 'system-connector-.cases': { doc_count: 2 }, + }, + } + ); + + actionsClient = new ActionsClient({ + logger, + actionTypeRegistry, + unsecuredSavedObjectsClient, + scopedClusterClient, + kibanaIndices, + actionExecutor, + executionEnqueuer, + ephemeralExecutionEnqueuer, + bulkExecutionEnqueuer, + request, + authorization: authorization as unknown as ActionsAuthorization, + inMemoryConnectors: [ + { + id: 'testPreconfigured', + actionTypeId: '.slack', + secrets: {}, + isPreconfigured: true, + isDeprecated: false, + isSystemAction: false, + name: 'test', + config: { + foo: 'bar', + }, + }, + { + id: 'system-connector-.cases', + actionTypeId: '.cases', + name: 'System action: .cases', + config: {}, + secrets: {}, + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: true, + }, + ], + connectorTokenClient: connectorTokenClientMock.create(), + getEventLogClient, + }); + + expect( + await actionsClient.getBulk(['1', 'testPreconfigured', 'system-connector-.cases'], false) + ).toEqual([ + { + actionTypeId: '.slack', + config: { foo: 'bar' }, + id: 'testPreconfigured', + isDeprecated: false, + isPreconfigured: true, + isSystemAction: false, + name: 'test', + secrets: {}, + }, + { + actionTypeId: '.cases', + config: {}, + id: 'system-connector-.cases', + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: true, + name: 'System action: .cases', + secrets: {}, + }, + { + actionTypeId: 'test', + config: { foo: 'bar' }, + id: '1', + isDeprecated: false, + isMissingSecrets: false, + isPreconfigured: false, + isSystemAction: false, + name: 'test', + }, + ]); + }); }); describe('getOAuthAccessToken()', () => { diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 63222ab9db0f3..433093f1e21f3 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -403,7 +403,13 @@ export class ActionsClient { /** * Get an action */ - public async get({ id }: { id: string }): Promise { + public async get({ + id, + throwIfSystemAction = true, + }: { + id: string; + throwIfSystemAction?: boolean; + }): Promise { try { await this.authorization.ensureAuthorized({ operation: 'get' }); } catch (error) { @@ -421,12 +427,18 @@ export class ActionsClient { /** * Getting system connector is not allowed + * if throwIfSystemAction is set to true. + * Default behavior is to throw */ - if (foundInMemoryConnector !== undefined && foundInMemoryConnector.isSystemAction) { + if ( + foundInMemoryConnector !== undefined && + foundInMemoryConnector.isSystemAction && + throwIfSystemAction + ) { throw Boom.notFound(`Connector ${id} not found`); } - if (foundInMemoryConnector !== undefined && foundInMemoryConnector.isPreconfigured) { + if (foundInMemoryConnector !== undefined) { this.auditLogger?.log( connectorAuditEvent({ action: ConnectorAuditAction.GET, @@ -522,7 +534,10 @@ export class ActionsClient { /** * Get bulk actions with in-memory list */ - public async getBulk(ids: string[]): Promise { + public async getBulk( + ids: string[], + throwIfSystemAction: boolean = true + ): Promise { try { await this.authorization.ensureAuthorized({ operation: 'get' }); } catch (error) { @@ -547,12 +562,14 @@ export class ActionsClient { /** * Getting system connector is not allowed + * if throwIfSystemAction is set to true. + * Default behavior is to throw */ - if (action !== undefined && action.isSystemAction) { + if (action !== undefined && action.isSystemAction && throwIfSystemAction) { throw Boom.notFound(`Connector ${action.id} not found`); } - if (action !== undefined && action.isPreconfigured) { + if (action !== undefined) { actionResults.push(action); } } From 1613160d54ad489324b5196eb0897b7f76c7d26c Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 20 Jul 2023 17:56:35 +0300 Subject: [PATCH 8/8] Allow the event log to get system actions --- x-pack/plugins/actions/server/plugin.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/actions/server/plugin.ts b/x-pack/plugins/actions/server/plugin.ts index 85233356ffda8..bb8fcc3bbea5d 100644 --- a/x-pack/plugins/actions/server/plugin.ts +++ b/x-pack/plugins/actions/server/plugin.ts @@ -491,7 +491,13 @@ export class ActionsPlugin implements Plugin objects ? Promise.all( - objects.map(async (objectItem) => await (await client).get({ id: objectItem.id })) + objects.map( + async (objectItem) => + /** + * TODO: Change with getBulk + */ + await (await client).get({ id: objectItem.id, throwIfSystemAction: false }) + ) ) : Promise.resolve([]); });