From 51a784a5ff7bf3b2000b5b84f0cd4aef59d78354 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Mon, 16 Nov 2020 20:42:59 +0000 Subject: [PATCH 01/12] ECS audit events for alerts plugin --- docs/user/security/audit-logging.asciidoc | 68 +++ src/core/server/index.ts | 1 + src/core/server/saved_objects/index.ts | 1 + .../saved_objects/serialization/index.ts | 2 +- .../saved_objects/serialization/serializer.ts | 6 +- .../actions/server/actions_client.test.ts | 416 ++++++++++++++++-- .../plugins/actions/server/actions_client.ts | 226 ++++++++-- .../actions/server/lib/audit_events.test.ts | 93 ++++ .../actions/server/lib/audit_events.ts | 76 ++++ x-pack/plugins/actions/server/plugin.ts | 3 + .../actions/server/saved_objects/index.ts | 1 + .../server/alerts_client/alerts_client.ts | 390 +++++++++++++--- .../server/alerts_client/audit_events.test.ts | 93 ++++ .../server/alerts_client/audit_events.ts | 94 ++++ .../server/alerts_client/tests/create.test.ts | 88 +++- .../server/alerts_client/tests/delete.test.ts | 44 ++ .../alerts_client/tests/disable.test.ts | 47 +- .../server/alerts_client/tests/enable.test.ts | 47 +- .../server/alerts_client/tests/find.test.ts | 65 ++- .../server/alerts_client/tests/get.test.ts | 62 ++- .../alerts_client/tests/mute_all.test.ts | 85 ++++ .../alerts_client/tests/mute_instance.test.ts | 76 +++- .../alerts_client/tests/unmute_all.test.ts | 86 +++- .../tests/unmute_instance.test.ts | 76 +++- .../server/alerts_client/tests/update.test.ts | 91 +++- .../tests/update_api_key.test.ts | 47 +- .../alerts/server/alerts_client_factory.ts | 1 + .../alerts/server/saved_objects/index.ts | 1 + .../encrypted_saved_objects_client_wrapper.ts | 16 +- .../security/server/audit/audit_events.ts | 12 +- ...ecure_saved_objects_client_wrapper.test.ts | 4 +- .../secure_saved_objects_client_wrapper.ts | 34 +- 32 files changed, 2147 insertions(+), 205 deletions(-) create mode 100644 x-pack/plugins/actions/server/lib/audit_events.test.ts create mode 100644 x-pack/plugins/actions/server/lib/audit_events.ts create mode 100644 x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts create mode 100644 x-pack/plugins/alerts/server/alerts_client/audit_events.ts diff --git a/docs/user/security/audit-logging.asciidoc b/docs/user/security/audit-logging.asciidoc index bacd93f585adcf..45569ad9658c43 100644 --- a/docs/user/security/audit-logging.asciidoc +++ b/docs/user/security/audit-logging.asciidoc @@ -84,6 +84,14 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is creating a saved object. | `failure` | User is not authorized to create a saved object. +.2+| `connector_create` +| `unknown` | User is creating a connector. +| `failure` | User is not authorized to create a connector. + +.2+| `alert_rule_create` +| `unknown` | User is creating an alert rule. +| `failure` | User is not authorized to create an alert rule. + 3+a| ====== Type: change @@ -108,6 +116,42 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is removing references to a saved object. | `failure` | User is not authorized to remove references to a saved object. +.2+| `connector_update` +| `unknown` | User is updating a connector. +| `failure` | User is not authorized to update a connector. + +.2+| `alert_rule_update` +| `unknown` | User is updating an alert rule. +| `failure` | User is not authorized to update an alert rule. + +.2+| `alert_rule_update_api_key` +| `unknown` | User is updating the API key of an alert rule. +| `failure` | User is not authorized to update the API key of an alert rule. + +.2+| `alert_rule_enable` +| `unknown` | User is enabling an alert rule. +| `failure` | User is not authorized to enable an alert rule. + +.2+| `alert_rule_disable` +| `unknown` | User is disabling an alert rule. +| `failure` | User is not authorized to disable an alert rule. + +.2+| `alert_rule_mute` +| `unknown` | User is muting an alert rule. +| `failure` | User is not authorized to mute an alert rule. + +.2+| `alert_rule_unmute` +| `unknown` | User is unmuting an alert rule. +| `failure` | User is not authorized to unmute an alert rule. + +.2+| `alert_instance_mute` +| `unknown` | User is muting an alert instance. +| `failure` | User is not authorized to mute an alert instance. + +.2+| `alert_instance_unmute` +| `unknown` | User is unmuting an alert instance. +| `failure` | User is not authorized to unmute an alert instance. + 3+a| ====== Type: deletion @@ -120,6 +164,14 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is deleting a saved object. | `failure` | User is not authorized to delete a saved object. +.2+| `connector_delete` +| `unknown` | User is deleting a connector. +| `failure` | User is not authorized to delete a connector. + +.2+| `alert_rule_delete` +| `unknown` | User is deleting an alert rule. +| `failure` | User is not authorized to delete an alert rule. + 3+a| ====== Type: access @@ -135,6 +187,22 @@ Refer to the corresponding {es} logs for potential write errors. | `success` | User has accessed a saved object as part of a search operation. | `failure` | User is not authorized to search for saved objects. +.2+| `connector_get` +| `success` | User has accessed a connector. +| `failure` | User is not authorized to access a connector. + +.2+| `connector_find` +| `success` | User has accessed a connector as part of a search operation. +| `failure` | User is not authorized to search for connectors. + +.2+| `alert_rule_get` +| `success` | User has accessed an alert rule. +| `failure` | User is not authorized to access an alert rule. + +.2+| `alert_rule_find` +| `success` | User has accessed an alert rule as part of a search operation. +| `failure` | User is not authorized to search for alert rules. + 3+a| ===== Category: web diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 7b19c3a6867579..c01874c218f3d1 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -311,6 +311,7 @@ export { exportSavedObjectsToStream, importSavedObjectsFromStream, resolveSavedObjectsImportErrors, + generateSavedObjectId, } from './saved_objects'; export { diff --git a/src/core/server/saved_objects/index.ts b/src/core/server/saved_objects/index.ts index f2bae29c4743b5..8e9309ae77299b 100644 --- a/src/core/server/saved_objects/index.ts +++ b/src/core/server/saved_objects/index.ts @@ -32,6 +32,7 @@ export { SavedObjectsRawDoc, SavedObjectSanitizedDoc, SavedObjectUnsanitizedDoc, + generateSavedObjectId, } from './serialization'; export { SavedObjectsMigrationLogger } from './migrations/core/migration_logger'; diff --git a/src/core/server/saved_objects/serialization/index.ts b/src/core/server/saved_objects/serialization/index.ts index 812a0770ad988a..3d57bf2dd04722 100644 --- a/src/core/server/saved_objects/serialization/index.ts +++ b/src/core/server/saved_objects/serialization/index.ts @@ -28,4 +28,4 @@ export { SavedObjectsRawDoc, SavedObjectsRawDocSource, } from './types'; -export { SavedObjectsSerializer } from './serializer'; +export { SavedObjectsSerializer, generateSavedObjectId } from './serializer'; diff --git a/src/core/server/saved_objects/serialization/serializer.ts b/src/core/server/saved_objects/serialization/serializer.ts index 145dd286c1ca8d..092b62b080158d 100644 --- a/src/core/server/saved_objects/serialization/serializer.ts +++ b/src/core/server/saved_objects/serialization/serializer.ts @@ -130,7 +130,7 @@ export class SavedObjectsSerializer { public generateRawId(namespace: string | undefined, type: string, id?: string) { const namespacePrefix = namespace && this.registry.isSingleNamespace(type) ? `${namespace}:` : ''; - return `${namespacePrefix}${type}:${id || uuid.v1()}`; + return `${namespacePrefix}${type}:${id || generateSavedObjectId()}`; } private trimIdPrefix(namespace: string | undefined, type: string, id: string) { @@ -149,6 +149,10 @@ export class SavedObjectsSerializer { } } +export function generateSavedObjectId() { + return uuid.v1(); +} + function assertNonEmptyString(value: string, name: string) { if (!value || typeof value !== 'string') { throw new TypeError(`Expected "${value}" to be a ${name}`); diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 171f8d4b0b1d43..c4736dc4157e1e 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -15,6 +15,8 @@ import { actionsConfigMock } from './actions_config.mock'; import { getActionsConfigurationUtilities } from './actions_config'; import { licenseStateMock } from './lib/license_state.mock'; import { licensingMock } from '../../licensing/server/mocks'; +import { httpServerMock } from '../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../security/server/audit/index.mock'; import { elasticsearchServiceMock, @@ -22,17 +24,21 @@ import { } from '../../../../src/core/server/mocks'; import { actionExecutorMock } from './lib/action_executor.mock'; import uuid from 'uuid'; -import { KibanaRequest } from 'kibana/server'; import { ActionsAuthorization } from './authorization/actions_authorization'; import { actionsAuthorizationMock } from './authorization/actions_authorization.mock'; +jest.mock('../../../../src/core/server/saved_objects/serialization/serializer', () => ({ + generateSavedObjectId: () => 'GENERATED_ID', +})); + const defaultKibanaIndex = '.kibana'; const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); const scopedClusterClient = elasticsearchServiceMock.createLegacyScopedClusterClient(); const actionExecutor = actionExecutorMock.create(); const authorization = actionsAuthorizationMock.create(); const executionEnqueuer = jest.fn(); -const request = {} as KibanaRequest; +const request = httpServerMock.createKibanaRequest(); +const auditLogger = auditServiceMock.create().asScoped(request); const mockTaskManager = taskManagerMock.createSetup(); @@ -68,6 +74,7 @@ beforeEach(() => { executionEnqueuer, request, authorization: (authorization as unknown) as ActionsAuthorization, + auditLogger, }); }); @@ -142,6 +149,95 @@ describe('create()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when creating a connector', async () => { + const savedObjectCreateResult = { + id: '1', + type: 'action', + attributes: { + name: 'my name', + actionTypeId: 'my-action-type', + config: {}, + }, + references: [], + }; + actionTypeRegistry.register({ + id: savedObjectCreateResult.attributes.actionTypeId, + name: 'My action type', + minimumLicenseRequired: 'basic', + executor, + }); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce(savedObjectCreateResult); + + await actionsClient.create({ + action: { + ...savedObjectCreateResult.attributes, + secrets: {}, + }, + }); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_create', + outcome: 'unknown', + }), + kibana: { saved_object: { id: 'GENERATED_ID', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to create a connector', async () => { + const savedObjectCreateResult = { + id: '1', + type: 'action', + attributes: { + name: 'my name', + actionTypeId: 'my-action-type', + config: {}, + }, + references: [], + }; + actionTypeRegistry.register({ + id: savedObjectCreateResult.attributes.actionTypeId, + name: 'My action type', + minimumLicenseRequired: 'basic', + executor, + }); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce(savedObjectCreateResult); + + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect( + async () => + await actionsClient.create({ + action: { + ...savedObjectCreateResult.attributes, + secrets: {}, + }, + }) + ).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_create', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: 'GENERATED_ID', + type: 'action', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); + test('creates an action with all given properties', async () => { const savedObjectCreateResult = { id: '1', @@ -185,6 +281,9 @@ describe('create()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "id": "GENERATED_ID", + }, ] `); }); @@ -289,6 +388,9 @@ describe('create()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "id": "GENERATED_ID", + }, ] `); }); @@ -440,7 +542,7 @@ describe('get()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('get'); }); - test('throws when user is not authorised to create the type of action', async () => { + test('throws when user is not authorised to get the type of action', async () => { unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'type', @@ -463,7 +565,7 @@ describe('get()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('get'); }); - test('throws when user is not authorised to create preconfigured of action', async () => { + test('throws when user is not authorised to get preconfigured of action', async () => { actionsClient = new ActionsClient({ actionTypeRegistry, unsecuredSavedObjectsClient, @@ -501,6 +603,61 @@ describe('get()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when getting a connector', async () => { + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'type', + attributes: { + name: 'my name', + actionTypeId: 'my-action-type', + config: {}, + }, + references: [], + }); + + await actionsClient.get({ id: '1' }); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_get', + outcome: 'success', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to get a connector', async () => { + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'type', + attributes: { + name: 'my name', + actionTypeId: 'my-action-type', + config: {}, + }, + references: [], + }); + + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(actionsClient.get({ id: '1' })).rejects.toThrow(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_get', + outcome: 'failure', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + error: { code: 'Error', message: 'Unauthorized' }, + }) + ); + }); + }); + test('calls unsecuredSavedObjectsClient with id', async () => { unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', @@ -632,6 +789,64 @@ describe('getAll()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when searching connectors', async () => { + unsecuredSavedObjectsClient.find.mockResolvedValueOnce({ + total: 1, + per_page: 10, + page: 1, + saved_objects: [ + { + id: '1', + type: 'type', + attributes: { + name: 'test', + config: { + foo: 'bar', + }, + }, + score: 1, + references: [], + }, + ], + }); + scopedClusterClient.callAsInternalUser.mockResolvedValueOnce({ + aggregations: { + '1': { doc_count: 6 }, + testPreconfigured: { doc_count: 2 }, + }, + }); + + await actionsClient.getAll(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_find', + outcome: 'success', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to search connectors', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(actionsClient.getAll()).rejects.toThrow(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_find', + outcome: 'failure', + }), + error: { code: 'Error', message: 'Unauthorized' }, + }) + ); + }); + }); + test('calls unsecuredSavedObjectsClient with parameters', async () => { const expectedResult = { total: 1, @@ -773,6 +988,62 @@ describe('getBulk()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when bulk getting connectors', async () => { + unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({ + saved_objects: [ + { + id: '1', + type: 'action', + attributes: { + actionTypeId: 'test', + name: 'test', + config: { + foo: 'bar', + }, + }, + references: [], + }, + ], + }); + scopedClusterClient.callAsInternalUser.mockResolvedValueOnce({ + aggregations: { + '1': { doc_count: 6 }, + testPreconfigured: { doc_count: 2 }, + }, + }); + + await actionsClient.getBulk(['1']); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_get', + outcome: 'success', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to bulk get connectors', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(actionsClient.getBulk(['1'])).rejects.toThrow(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_get', + outcome: 'failure', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + error: { code: 'Error', message: 'Unauthorized' }, + }) + ); + }); + }); + test('calls getBulk unsecuredSavedObjectsClient with parameters', async () => { unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ @@ -864,6 +1135,39 @@ describe('delete()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when deleting a connector', async () => { + await actionsClient.delete({ id: '1' }); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_delete', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to delete a connector', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(actionsClient.delete({ id: '1' })).rejects.toThrow(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_delete', + outcome: 'failure', + }), + kibana: { saved_object: { id: '1', type: 'action' } }, + error: { code: 'Error', message: 'Unauthorized' }, + }) + ); + }); + }); + test('calls unsecuredSavedObjectsClient with id', async () => { const expectedResult = Symbol(); unsecuredSavedObjectsClient.delete.mockResolvedValueOnce(expectedResult); @@ -880,42 +1184,43 @@ describe('delete()', () => { }); describe('update()', () => { + function updateOperation(): ReturnType { + actionTypeRegistry.register({ + id: 'my-action-type', + name: 'My action type', + minimumLicenseRequired: 'basic', + executor, + }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'action', + attributes: { + actionTypeId: 'my-action-type', + }, + references: [], + }); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + id: 'my-action', + type: 'action', + attributes: { + actionTypeId: 'my-action-type', + name: 'my name', + config: {}, + secrets: {}, + }, + references: [], + }); + return actionsClient.update({ + id: 'my-action', + action: { + name: 'my name', + config: {}, + secrets: {}, + }, + }); + } + describe('authorization', () => { - function updateOperation(): ReturnType { - actionTypeRegistry.register({ - id: 'my-action-type', - name: 'My action type', - minimumLicenseRequired: 'basic', - executor, - }); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ - id: '1', - type: 'action', - attributes: { - actionTypeId: 'my-action-type', - }, - references: [], - }); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ - id: 'my-action', - type: 'action', - attributes: { - actionTypeId: 'my-action-type', - name: 'my name', - config: {}, - secrets: {}, - }, - references: [], - }); - return actionsClient.update({ - id: 'my-action', - action: { - name: 'my name', - config: {}, - secrets: {}, - }, - }); - } test('ensures user is authorised to update actions', async () => { await updateOperation(); expect(authorization.ensureAuthorized).toHaveBeenCalledWith('update'); @@ -934,6 +1239,39 @@ describe('update()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when updating a connector', async () => { + await updateOperation(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_update', + outcome: 'unknown', + }), + kibana: { saved_object: { id: 'my-action', type: 'action' } }, + }) + ); + }); + + test('logs audit event when not authorised to update a connector', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(updateOperation()).rejects.toThrow(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'connector_update', + outcome: 'failure', + }), + kibana: { saved_object: { id: 'my-action', type: 'action' } }, + error: { code: 'Error', message: 'Unauthorized' }, + }) + ); + }); + }); + test('updates an action with all given properties', async () => { actionTypeRegistry.register({ id: 'my-action-type', diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 0d41b520501add..4143c2b9223346 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -10,10 +10,13 @@ import { SavedObjectAttributes, SavedObject, KibanaRequest, -} from 'src/core/server'; + generateSavedObjectId, +} from '../../../../src/core/server'; import { i18n } from '@kbn/i18n'; import { omitBy, isUndefined } from 'lodash'; +import { AuditLogger, EventOutcome } from '../../security/server/audit'; +import { ActionType } from '../common'; import { ActionTypeRegistry } from './action_type_registry'; import { validateConfig, validateSecrets, ActionExecutorContract } from './lib'; import { @@ -30,11 +33,11 @@ import { ExecuteOptions as EnqueueExecutionOptions, } from './create_execute_function'; import { ActionsAuthorization } from './authorization/actions_authorization'; -import { ActionType } from '../common'; import { getAuthorizationModeBySource, AuthorizationMode, } from './authorization/get_authorization_mode_by_source'; +import { connectorEvent, ConnectorAction } from './lib/audit_events'; // We are assuming there won't be many actions. This is why we will load // all the actions in advance and assume the total count to not go over 10000. @@ -65,6 +68,7 @@ interface ConstructorOptions { executionEnqueuer: ExecutionEnqueuer; request: KibanaRequest; authorization: ActionsAuthorization; + auditLogger?: AuditLogger; } interface UpdateOptions { @@ -82,6 +86,7 @@ export class ActionsClient { private readonly request: KibanaRequest; private readonly authorization: ActionsAuthorization; private readonly executionEnqueuer: ExecutionEnqueuer; + private readonly auditLogger?: AuditLogger; constructor({ actionTypeRegistry, @@ -93,6 +98,7 @@ export class ActionsClient { executionEnqueuer, request, authorization, + auditLogger, }: ConstructorOptions) { this.actionTypeRegistry = actionTypeRegistry; this.unsecuredSavedObjectsClient = unsecuredSavedObjectsClient; @@ -103,6 +109,7 @@ export class ActionsClient { this.executionEnqueuer = executionEnqueuer; this.request = request; this.authorization = authorization; + this.auditLogger = auditLogger; } /** @@ -111,7 +118,20 @@ export class ActionsClient { public async create({ action: { actionTypeId, name, config, secrets }, }: CreateOptions): Promise { - await this.authorization.ensureAuthorized('create', actionTypeId); + const id = generateSavedObjectId(); + + try { + await this.authorization.ensureAuthorized('create', actionTypeId); + } catch (error) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.CREATE, + savedObject: { type: 'action', id }, + error, + }) + ); + throw error; + } const actionType = this.actionTypeRegistry.get(actionTypeId); const validatedActionTypeConfig = validateConfig(actionType, config); @@ -119,12 +139,24 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); - const result = await this.unsecuredSavedObjectsClient.create('action', { - actionTypeId, - name, - config: validatedActionTypeConfig as SavedObjectAttributes, - secrets: validatedActionTypeSecrets as SavedObjectAttributes, - }); + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.CREATE, + savedObject: { type: 'action', id }, + outcome: EventOutcome.UNKNOWN, + }) + ); + + const result = await this.unsecuredSavedObjectsClient.create( + 'action', + { + actionTypeId, + name, + config: validatedActionTypeConfig as SavedObjectAttributes, + secrets: validatedActionTypeSecrets as SavedObjectAttributes, + }, + { id } + ); return { id: result.id, @@ -139,27 +171,36 @@ export class ActionsClient { * Update action */ public async update({ id, action }: UpdateOptions): Promise { - await this.authorization.ensureAuthorized('update'); - - if ( - this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !== - undefined - ) { - throw new PreconfiguredActionDisabledModificationError( - i18n.translate('xpack.actions.serverSideErrors.predefinedActionUpdateDisabled', { - defaultMessage: 'Preconfigured action {id} is not allowed to update.', - values: { - id, - }, - }), - 'update' + try { + await this.authorization.ensureAuthorized('update'); + + if ( + this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !== + undefined + ) { + throw new PreconfiguredActionDisabledModificationError( + i18n.translate('xpack.actions.serverSideErrors.predefinedActionUpdateDisabled', { + defaultMessage: 'Preconfigured action {id} is not allowed to update.', + values: { + id, + }, + }), + 'update' + ); + } + } catch (error) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.UPDATE, + savedObject: { type: 'action', id }, + error, + }) ); + throw error; } - const { - attributes, - references, - version, - } = await this.unsecuredSavedObjectsClient.get('action', id); + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAction + >('action', id); const { actionTypeId } = attributes; const { name, config, secrets } = action; const actionType = this.actionTypeRegistry.get(actionTypeId); @@ -168,6 +209,14 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.UPDATE, + savedObject: { type: 'action', id }, + outcome: EventOutcome.UNKNOWN, + }) + ); + const result = await this.unsecuredSavedObjectsClient.create( 'action', { @@ -201,12 +250,30 @@ export class ActionsClient { * Get an action */ public async get({ id }: { id: string }): Promise { - await this.authorization.ensureAuthorized('get'); + try { + await this.authorization.ensureAuthorized('get'); + } catch (error) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.GET, + savedObject: { type: 'action', id }, + error, + }) + ); + throw error; + } const preconfiguredActionsList = this.preconfiguredActions.find( (preconfiguredAction) => preconfiguredAction.id === id ); if (preconfiguredActionsList !== undefined) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.GET, + savedObject: { type: 'action', id }, + }) + ); + return { id, actionTypeId: preconfiguredActionsList.actionTypeId, @@ -214,8 +281,16 @@ export class ActionsClient { isPreconfigured: true, }; } + const result = await this.unsecuredSavedObjectsClient.get('action', id); + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.GET, + savedObject: { type: 'action', id }, + }) + ); + return { id, actionTypeId: result.attributes.actionTypeId, @@ -229,7 +304,17 @@ export class ActionsClient { * Get all actions with preconfigured list */ public async getAll(): Promise { - await this.authorization.ensureAuthorized('get'); + try { + await this.authorization.ensureAuthorized('get'); + } catch (error) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.FIND, + error, + }) + ); + throw error; + } const savedObjectsActions = ( await this.unsecuredSavedObjectsClient.find({ @@ -238,6 +323,15 @@ export class ActionsClient { }) ).saved_objects.map(actionFromSavedObject); + savedObjectsActions.forEach(({ id }) => + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.FIND, + savedObject: { type: 'action', id }, + }) + ) + ); + const mergedResult = [ ...savedObjectsActions, ...this.preconfiguredActions.map((preconfiguredAction) => ({ @@ -258,7 +352,20 @@ export class ActionsClient { * Get bulk actions with preconfigured list */ public async getBulk(ids: string[]): Promise { - await this.authorization.ensureAuthorized('get'); + try { + await this.authorization.ensureAuthorized('get'); + } catch (error) { + ids.forEach((id) => + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.GET, + savedObject: { type: 'action', id }, + error, + }) + ) + ); + throw error; + } const actionResults = new Array(); for (const actionId of ids) { @@ -283,6 +390,15 @@ export class ActionsClient { const bulkGetOpts = actionSavedObjectsIds.map((id) => ({ id, type: 'action' })); const bulkGetResult = await this.unsecuredSavedObjectsClient.bulkGet(bulkGetOpts); + ids.forEach((id) => + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.GET, + savedObject: { type: 'action', id }, + }) + ) + ); + for (const action of bulkGetResult.saved_objects) { if (action.error) { throw Boom.badRequest( @@ -298,22 +414,42 @@ export class ActionsClient { * Delete action */ public async delete({ id }: { id: string }) { - await this.authorization.ensureAuthorized('delete'); - - if ( - this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !== - undefined - ) { - throw new PreconfiguredActionDisabledModificationError( - i18n.translate('xpack.actions.serverSideErrors.predefinedActionDeleteDisabled', { - defaultMessage: 'Preconfigured action {id} is not allowed to delete.', - values: { - id, - }, - }), - 'delete' + try { + await this.authorization.ensureAuthorized('delete'); + + if ( + this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !== + undefined + ) { + throw new PreconfiguredActionDisabledModificationError( + i18n.translate('xpack.actions.serverSideErrors.predefinedActionDeleteDisabled', { + defaultMessage: 'Preconfigured action {id} is not allowed to delete.', + values: { + id, + }, + }), + 'delete' + ); + } + } catch (error) { + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.DELETE, + savedObject: { type: 'action', id }, + error, + }) ); + throw error; } + + this.auditLogger?.log( + connectorEvent({ + action: ConnectorAction.DELETE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'action', id }, + }) + ); + return await this.unsecuredSavedObjectsClient.delete('action', id); } diff --git a/x-pack/plugins/actions/server/lib/audit_events.test.ts b/x-pack/plugins/actions/server/lib/audit_events.test.ts new file mode 100644 index 00000000000000..37c94a8c46a47b --- /dev/null +++ b/x-pack/plugins/actions/server/lib/audit_events.test.ts @@ -0,0 +1,93 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { EventOutcome } from '../../../security/server/audit'; +import { ConnectorAction, connectorEvent } from './audit_events'; + +describe('#connectorEvent', () => { + test('creates event with `unknown` outcome', () => { + expect( + connectorEvent({ + action: ConnectorAction.CREATE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'action', id: 'ACTION_ID' }, + }) + ).toMatchInlineSnapshot(` + Object { + "error": undefined, + "event": Object { + "action": "connector_create", + "category": "database", + "outcome": "unknown", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ACTION_ID", + "type": "action", + }, + }, + "message": "User is creating connector [id=ACTION_ID]", + } + `); + }); + + test('creates event with `success` outcome', () => { + expect( + connectorEvent({ + action: ConnectorAction.CREATE, + savedObject: { type: 'action', id: 'ACTION_ID' }, + }) + ).toMatchInlineSnapshot(` + Object { + "error": undefined, + "event": Object { + "action": "connector_create", + "category": "database", + "outcome": "success", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ACTION_ID", + "type": "action", + }, + }, + "message": "User has created connector [id=ACTION_ID]", + } + `); + }); + + test('creates event with `failure` outcome', () => { + expect( + connectorEvent({ + action: ConnectorAction.CREATE, + savedObject: { type: 'action', id: 'ACTION_ID' }, + error: new Error('ERROR_MESSAGE'), + }) + ).toMatchInlineSnapshot(` + Object { + "error": Object { + "code": "Error", + "message": "ERROR_MESSAGE", + }, + "event": Object { + "action": "connector_create", + "category": "database", + "outcome": "failure", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ACTION_ID", + "type": "action", + }, + }, + "message": "Failed attempt to create connector [id=ACTION_ID]", + } + `); + }); +}); diff --git a/x-pack/plugins/actions/server/lib/audit_events.ts b/x-pack/plugins/actions/server/lib/audit_events.ts new file mode 100644 index 00000000000000..aa4173769f4d2a --- /dev/null +++ b/x-pack/plugins/actions/server/lib/audit_events.ts @@ -0,0 +1,76 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server/audit'; + +export enum ConnectorAction { + CREATE = 'connector_create', + GET = 'connector_get', + UPDATE = 'connector_update', + DELETE = 'connector_delete', + FIND = 'connector_find', + EXECUTE = 'connector_execute', +} + +type VerbsTuple = [string, string, string]; + +const eventVerbs: Record = { + connector_create: ['create', 'creating', 'created'], + connector_get: ['access', 'accessing', 'accessed'], + connector_update: ['update', 'updating', 'updated'], + connector_delete: ['delete', 'deleting', 'deleted'], + connector_find: ['access', 'accessing', 'accessed'], + connector_execute: ['execute', 'executing', 'executed'], +}; + +const eventTypes: Record = { + connector_create: EventType.CREATION, + connector_get: EventType.ACCESS, + connector_update: EventType.CHANGE, + connector_delete: EventType.DELETION, + connector_find: EventType.ACCESS, + connector_execute: undefined, +}; + +export interface ConnectorEventParams { + action: ConnectorAction; + outcome?: EventOutcome; + savedObject?: Required['kibana']>['saved_object']; + error?: Error; +} + +export function connectorEvent({ + action, + savedObject, + outcome, + error, +}: ConnectorEventParams): AuditEvent { + const doc = savedObject ? `connector [id=${savedObject.id}]` : 'a connector'; + const [present, progressive, past] = eventVerbs[action]; + const message = error + ? `Failed attempt to ${present} ${doc}` + : outcome === 'unknown' + ? `User is ${progressive} ${doc}` + : `User has ${past} ${doc}`; + const type = eventTypes[action]; + + return { + message, + event: { + action, + category: EventCategory.DATABASE, + type, + outcome: outcome ?? (error ? EventOutcome.FAILURE : EventOutcome.SUCCESS), + }, + kibana: { + saved_object: savedObject, + }, + error: error && { + code: error.name, + message: error.message, + }, + }; +} diff --git a/x-pack/plugins/actions/server/plugin.ts b/x-pack/plugins/actions/server/plugin.ts index e61936321b8e0a..6e37d4bd7a92aa 100644 --- a/x-pack/plugins/actions/server/plugin.ts +++ b/x-pack/plugins/actions/server/plugin.ts @@ -314,6 +314,7 @@ export class ActionsPlugin implements Plugin, Plugi isESOUsingEphemeralEncryptionKey: isESOUsingEphemeralEncryptionKey!, preconfiguredActions, }), + auditLogger: this.security?.audit.asScoped(request), }); }; @@ -439,6 +440,7 @@ export class ActionsPlugin implements Plugin, Plugi preconfiguredActions, actionExecutor, instantiateAuthorization, + security, } = this; return async function actionsRouteHandlerContext(context, request) { @@ -468,6 +470,7 @@ export class ActionsPlugin implements Plugin, Plugi isESOUsingEphemeralEncryptionKey: isESOUsingEphemeralEncryptionKey!, preconfiguredActions, }), + auditLogger: security?.audit.asScoped(request), }); }, listTypes: actionTypeRegistry!.list.bind(actionTypeRegistry!), diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index db811e7cd3a4f4..fe722e89ded06f 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -33,6 +33,7 @@ export function setupSavedObjects( type: ACTION_SAVED_OBJECT_TYPE, attributesToEncrypt: new Set(['secrets']), attributesToExcludeFromAAD: new Set(['name']), + allowPredefinedID: true, }); savedObjects.registerType({ diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index c83e24c5a45f4f..24f58e77497bfe 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -13,7 +13,8 @@ import { SavedObjectReference, SavedObject, PluginInitializerContext, -} from 'src/core/server'; + generateSavedObjectId, +} from '../../../../../src/core/server'; import { esKuery } from '../../../../../src/plugins/data/server'; import { ActionsClient, ActionsAuthorization } from '../../../actions/server'; import { @@ -44,10 +45,12 @@ import { IEventLogClient } from '../../../../plugins/event_log/server'; import { parseIsoOrRelativeDate } from '../lib/iso_or_relative_date'; import { alertInstanceSummaryFromEventLog } from '../lib/alert_instance_summary_from_event_log'; import { IEvent } from '../../../event_log/server'; +import { AuditLogger, EventOutcome } from '../../../security/server/audit'; import { parseDuration } from '../../common/parse_duration'; import { retryIfConflicts } from '../lib/retry_if_conflicts'; import { partiallyUpdateAlert } from '../saved_objects'; import { markApiKeyForInvalidation } from '../invalidate_pending_api_keys/mark_api_key_for_invalidation'; +import { alertRuleEvent, AlertRuleAction } from './audit_events'; export interface RegistryAlertTypeWithAuth extends RegistryAlertType { authorizedConsumers: string[]; @@ -75,6 +78,7 @@ export interface ConstructorOptions { getActionsClient: () => Promise; getEventLogClient: () => Promise; kibanaVersion: PluginInitializerContext['env']['packageInfo']['version']; + auditLogger?: AuditLogger; } export interface MuteOptions extends IndexType { @@ -176,6 +180,7 @@ export class AlertsClient { private readonly getEventLogClient: () => Promise; private readonly encryptedSavedObjectsClient: EncryptedSavedObjectsClient; private readonly kibanaVersion!: PluginInitializerContext['env']['packageInfo']['version']; + private readonly auditLogger?: AuditLogger; constructor({ alertTypeRegistry, @@ -192,6 +197,7 @@ export class AlertsClient { actionsAuthorization, getEventLogClient, kibanaVersion, + auditLogger, }: ConstructorOptions) { this.logger = logger; this.getUserName = getUserName; @@ -207,14 +213,28 @@ export class AlertsClient { this.actionsAuthorization = actionsAuthorization; this.getEventLogClient = getEventLogClient; this.kibanaVersion = kibanaVersion; + this.auditLogger = auditLogger; } public async create({ data, options }: CreateOptions): Promise { - await this.authorization.ensureAuthorized( - data.alertTypeId, - data.consumer, - WriteOperations.Create - ); + const id = generateSavedObjectId(); + + try { + await this.authorization.ensureAuthorized( + data.alertTypeId, + data.consumer, + WriteOperations.Create + ); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.CREATE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } // Throws an error if alert type isn't registered const alertType = this.alertTypeRegistry.get(data.alertTypeId); @@ -248,6 +268,15 @@ export class AlertsClient { error: null, }, }; + + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.CREATE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) + ); + let createdAlert: SavedObject; try { createdAlert = await this.unsecuredSavedObjectsClient.create( @@ -256,6 +285,7 @@ export class AlertsClient { { ...options, references, + id, } ); } catch (e) { @@ -297,10 +327,27 @@ export class AlertsClient { public async get({ id }: { id: string }): Promise { const result = await this.unsecuredSavedObjectsClient.get('alert', id); - await this.authorization.ensureAuthorized( - result.attributes.alertTypeId, - result.attributes.consumer, - ReadOperations.Get + try { + await this.authorization.ensureAuthorized( + result.attributes.alertTypeId, + result.attributes.consumer, + ReadOperations.Get + ); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.GET, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.GET, + savedObject: { type: 'alert', id }, + }) ); return this.getAlertFromRaw(result.id, result.attributes, result.references); } @@ -370,11 +417,23 @@ export class AlertsClient { public async find({ options: { fields, ...options } = {}, }: { options?: FindOptions } = {}): Promise { + let authorizationTuple; + try { + authorizationTuple = await this.authorization.getFindAuthorizationFilter(); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.FIND, + error, + }) + ); + throw error; + } const { filter: authorizationFilter, ensureAlertTypeIsAuthorized, logSuccessfulAuthorization, - } = await this.authorization.getFindAuthorizationFilter(); + } = authorizationTuple; const { page, @@ -391,8 +450,20 @@ export class AlertsClient { type: 'alert', }); + // eslint-disable-next-line @typescript-eslint/naming-convention const authorizedData = data.map(({ id, attributes, references }) => { - ensureAlertTypeIsAuthorized(attributes.alertTypeId, attributes.consumer); + try { + ensureAlertTypeIsAuthorized(attributes.alertTypeId, attributes.consumer); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.FIND, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } return this.getAlertFromRaw( id, fields ? (pick(attributes, fields) as RawAlert) : attributes, @@ -400,6 +471,15 @@ export class AlertsClient { ); }); + authorizedData.forEach(({ id }) => + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.FIND, + savedObject: { type: 'alert', id }, + }) + ) + ); + logSuccessfulAuthorization(); return { @@ -473,10 +553,29 @@ export class AlertsClient { attributes = alert.attributes; } - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.Delete + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.Delete + ); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.DELETE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } + + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.DELETE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) ); const removeResult = await this.unsecuredSavedObjectsClient.delete('alert', id); @@ -520,10 +619,30 @@ export class AlertsClient { // Still attempt to load the object using SOC alertSavedObject = await this.unsecuredSavedObjectsClient.get('alert', id); } - await this.authorization.ensureAuthorized( - alertSavedObject.attributes.alertTypeId, - alertSavedObject.attributes.consumer, - WriteOperations.Update + + try { + await this.authorization.ensureAuthorized( + alertSavedObject.attributes.alertTypeId, + alertSavedObject.attributes.consumer, + WriteOperations.Update + ); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UPDATE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } + + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UPDATE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) ); const updateResult = await this.updateAlert({ id, data }, alertSavedObject); @@ -658,14 +777,28 @@ export class AlertsClient { attributes = alert.attributes; version = alert.version; } - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.UpdateApiKey - ); - if (attributes.actions.length && !this.authorization.shouldUseLegacyAuthorization(attributes)) { - await this.actionsAuthorization.ensureAuthorized('execute'); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.UpdateApiKey + ); + if ( + attributes.actions.length && + !this.authorization.shouldUseLegacyAuthorization(attributes) + ) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UPDATE_API_KEY, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; } const username = await this.getUserName(); @@ -678,6 +811,15 @@ export class AlertsClient { updatedAt: new Date().toISOString(), updatedBy: username, }); + + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UPDATE_API_KEY, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) + ); + try { await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version }); } catch (e) { @@ -732,16 +874,35 @@ export class AlertsClient { version = alert.version; } - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.Enable - ); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.Enable + ); - if (attributes.actions.length) { - await this.actionsAuthorization.ensureAuthorized('execute'); + if (attributes.actions.length) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.ENABLE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.ENABLE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) + ); + if (attributes.enabled === false) { const username = await this.getUserName(); const updateAttributes = this.updateMeta({ @@ -816,10 +977,29 @@ export class AlertsClient { version = alert.version; } - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.Disable + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.Disable + ); + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.DISABLE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; + } + + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.DISABLE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) ); if (attributes.enabled === true) { @@ -866,16 +1046,36 @@ export class AlertsClient { 'alert', id ); - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.MuteAll - ); - if (attributes.actions.length) { - await this.actionsAuthorization.ensureAuthorized('execute'); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.MuteAll + ); + + if (attributes.actions.length) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.MUTE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.MUTE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) + ); + const updateAttributes = this.updateMeta({ muteAll: true, mutedInstanceIds: [], @@ -905,16 +1105,36 @@ export class AlertsClient { 'alert', id ); - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.UnmuteAll - ); - if (attributes.actions.length) { - await this.actionsAuthorization.ensureAuthorized('execute'); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.UnmuteAll + ); + + if (attributes.actions.length) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UNMUTE, + savedObject: { type: 'alert', id }, + error, + }) + ); + throw error; } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UNMUTE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id }, + }) + ); + const updateAttributes = this.updateMeta({ muteAll: false, mutedInstanceIds: [], @@ -945,16 +1165,35 @@ export class AlertsClient { alertId ); - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.MuteInstance - ); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.MuteInstance + ); - if (attributes.actions.length) { - await this.actionsAuthorization.ensureAuthorized('execute'); + if (attributes.actions.length) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.MUTE_INSTANCE, + savedObject: { type: 'alert', id: alertId }, + error, + }) + ); + throw error; } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.MUTE_INSTANCE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id: alertId }, + }) + ); + const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && !mutedInstanceIds.includes(alertInstanceId)) { mutedInstanceIds.push(alertInstanceId); @@ -991,15 +1230,34 @@ export class AlertsClient { alertId ); - await this.authorization.ensureAuthorized( - attributes.alertTypeId, - attributes.consumer, - WriteOperations.UnmuteInstance - ); - if (attributes.actions.length) { - await this.actionsAuthorization.ensureAuthorized('execute'); + try { + await this.authorization.ensureAuthorized( + attributes.alertTypeId, + attributes.consumer, + WriteOperations.UnmuteInstance + ); + if (attributes.actions.length) { + await this.actionsAuthorization.ensureAuthorized('execute'); + } + } catch (error) { + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UNMUTE_INSTANCE, + savedObject: { type: 'alert', id: alertId }, + error, + }) + ); + throw error; } + this.auditLogger?.log( + alertRuleEvent({ + action: AlertRuleAction.UNMUTE_INSTANCE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id: alertId }, + }) + ); + const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && mutedInstanceIds.includes(alertInstanceId)) { await this.unsecuredSavedObjectsClient.update( diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts new file mode 100644 index 00000000000000..252bab6949b840 --- /dev/null +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts @@ -0,0 +1,93 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { EventOutcome } from '../../../security/server/audit'; +import { AlertRuleAction, alertRuleEvent } from './audit_events'; + +describe('#alertRuleEvent', () => { + test('creates event with `unknown` outcome', () => { + expect( + alertRuleEvent({ + action: AlertRuleAction.CREATE, + outcome: EventOutcome.UNKNOWN, + savedObject: { type: 'alert', id: 'ALERT_ID' }, + }) + ).toMatchInlineSnapshot(` + Object { + "error": undefined, + "event": Object { + "action": "alert_rule_create", + "category": "database", + "outcome": "unknown", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ALERT_ID", + "type": "alert", + }, + }, + "message": "User is creating alert rule [id=ALERT_ID]", + } + `); + }); + + test('creates event with `success` outcome', () => { + expect( + alertRuleEvent({ + action: AlertRuleAction.CREATE, + savedObject: { type: 'alert', id: 'ALERT_ID' }, + }) + ).toMatchInlineSnapshot(` + Object { + "error": undefined, + "event": Object { + "action": "alert_rule_create", + "category": "database", + "outcome": "success", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ALERT_ID", + "type": "alert", + }, + }, + "message": "User has created alert rule [id=ALERT_ID]", + } + `); + }); + + test('creates event with `failure` outcome', () => { + expect( + alertRuleEvent({ + action: AlertRuleAction.CREATE, + savedObject: { type: 'alert', id: 'ALERT_ID' }, + error: new Error('ERROR_MESSAGE'), + }) + ).toMatchInlineSnapshot(` + Object { + "error": Object { + "code": "Error", + "message": "ERROR_MESSAGE", + }, + "event": Object { + "action": "alert_rule_create", + "category": "database", + "outcome": "failure", + "type": "creation", + }, + "kibana": Object { + "saved_object": Object { + "id": "ALERT_ID", + "type": "alert", + }, + }, + "message": "Failed attempt to create alert rule [id=ALERT_ID]", + } + `); + }); +}); diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts new file mode 100644 index 00000000000000..0cce835d672c15 --- /dev/null +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts @@ -0,0 +1,94 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server/audit'; + +export enum AlertRuleAction { + CREATE = 'alert_rule_create', + GET = 'alert_rule_get', + UPDATE = 'alert_rule_update', + UPDATE_API_KEY = 'alert_rule_update_api_key', + ENABLE = 'alert_rule_enable', + DISABLE = 'alert_rule_disable', + DELETE = 'alert_rule_delete', + FIND = 'alert_rule_find', + MUTE = 'alert_rule_mute', + UNMUTE = 'alert_rule_unmute', + MUTE_INSTANCE = 'alert_instance_mute', + UNMUTE_INSTANCE = 'alert_instance_unmute', +} + +type VerbsTuple = [string, string, string]; + +const eventVerbs: Record = { + alert_rule_create: ['create', 'creating', 'created'], + alert_rule_get: ['access', 'accessing', 'accessed'], + alert_rule_update: ['update', 'updating', 'updated'], + alert_rule_update_api_key: ['update API key of', 'updating API key of', 'updated API key of'], + alert_rule_enable: ['enable', 'enabling', 'enabled'], + alert_rule_disable: ['disable', 'disabling', 'disabled'], + alert_rule_delete: ['delete', 'deleting', 'deleted'], + alert_rule_find: ['access', 'accessing', 'accessed'], + alert_rule_mute: ['mute', 'muting', 'muted'], + alert_rule_unmute: ['unmute', 'unmuting', 'unmuted'], + alert_instance_mute: ['mute instance of', 'muting instance of', 'muted instance of'], + alert_instance_unmute: ['unmute instance of', 'unmuting instance of', 'unmuted instance of'], +}; + +const eventTypes: Record = { + alert_rule_create: EventType.CREATION, + alert_rule_get: EventType.ACCESS, + alert_rule_update: EventType.CHANGE, + alert_rule_update_api_key: EventType.CHANGE, + alert_rule_enable: EventType.CHANGE, + alert_rule_disable: EventType.CHANGE, + alert_rule_delete: EventType.DELETION, + alert_rule_find: EventType.ACCESS, + alert_rule_mute: EventType.CHANGE, + alert_rule_unmute: EventType.CHANGE, + alert_instance_mute: EventType.CHANGE, + alert_instance_unmute: EventType.CHANGE, +}; + +export interface AlertRuleEventParams { + action: AlertRuleAction; + outcome?: EventOutcome; + savedObject?: Required['kibana']>['saved_object']; + error?: Error; +} + +export function alertRuleEvent({ + action, + savedObject, + outcome, + error, +}: AlertRuleEventParams): AuditEvent { + const doc = savedObject ? `alert rule [id=${savedObject.id}]` : 'an alert rule'; + const [present, progressive, past] = eventVerbs[action]; + const message = error + ? `Failed attempt to ${present} ${doc}` + : outcome === 'unknown' + ? `User is ${progressive} ${doc}` + : `User has ${past} ${doc}`; + const type = eventTypes[action]; + + return { + message, + event: { + action, + category: EventCategory.DATABASE, + type, + outcome: outcome ?? (error ? EventOutcome.FAILURE : EventOutcome.SUCCESS), + }, + kibana: { + saved_object: savedObject, + }, + error: error && { + code: error.name, + message: error.message, + }, + }; +} diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts index 171ed13763c46b..f2e571dc602fe8 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts @@ -14,14 +14,21 @@ import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization, ActionsClient } from '../../../../actions/server'; import { TaskStatus } from '../../../../task_manager/server'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; import { getBeforeSetup, setGlobalDate } from './lib'; +jest.mock('../../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ + generateSavedObjectId: () => 'GENERATED_ID', +})); + const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -39,10 +46,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -184,6 +193,62 @@ describe('create()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when creating an alert', async () => { + const data = getMockData({ + enabled: false, + actions: [], + }); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: data, + references: [], + }); + await alertsClient.create({ data }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_create', + outcome: 'unknown', + }), + kibana: { saved_object: { id: 'GENERATED_ID', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to create an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect( + alertsClient.create({ + data: getMockData({ + enabled: false, + actions: [], + }), + }) + ).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_create', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: 'GENERATED_ID', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); + test('creates an alert', async () => { const data = getMockData(); const createdAttributes = { @@ -336,16 +401,17 @@ describe('create()', () => { } `); expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - } - `); + Object { + "id": "GENERATED_ID", + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + } + `); expect(taskManager.schedule).toHaveBeenCalledTimes(1); expect(taskManager.schedule.mock.calls[0]).toMatchInlineSnapshot(` Array [ @@ -989,6 +1055,7 @@ describe('create()', () => { }, }, { + id: 'GENERATED_ID', references: [ { id: '1', @@ -1111,6 +1178,7 @@ describe('create()', () => { }, }, { + id: 'GENERATED_ID', references: [ { id: '1', diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts index e7b975aec8eb0b..0a791612d81d49 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts @@ -12,6 +12,8 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup } from './lib'; const taskManager = taskManagerMock.createStart(); @@ -20,6 +22,7 @@ const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -37,10 +40,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); describe('delete()', () => { @@ -239,4 +244,43 @@ describe('delete()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('myType', 'myApp', 'delete'); }); }); + + describe('auditLogger', () => { + test('logs audit event when deleting an alert', async () => { + await alertsClient.delete({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_delete', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to delete an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.delete({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_delete', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts index 8c9ab9494a50af..3edeffaa9ce6e4 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts @@ -12,16 +12,18 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; -import { getBeforeSetup, setGlobalDate } from './lib'; import { InvalidatePendingApiKey } from '../../types'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; +import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -39,10 +41,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -109,6 +113,45 @@ describe('disable()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when disabling an alert', async () => { + await alertsClient.disable({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_disable', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to disable an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.disable({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_disable', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); + test('disables an alert', async () => { unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts index feec1d1b9334a1..936b8d076b80b8 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts @@ -13,16 +13,18 @@ import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; import { TaskStatus } from '../../../../task_manager/server'; -import { getBeforeSetup, setGlobalDate } from './lib'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { InvalidatePendingApiKey } from '../../types'; +import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -40,10 +42,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -148,6 +152,45 @@ describe('enable()', () => { }); }); + describe('auditLogger', () => { + test('logs audit event when enabling an alert', async () => { + await alertsClient.enable({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_enable', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to enable an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.enable({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_enable', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); + test('enables an alert', async () => { const createdAt = new Date().toISOString(); unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts index 3d7473a7469869..23dc5b9a407719 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts @@ -14,15 +14,17 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -44,6 +46,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -248,4 +251,64 @@ describe('find()', () => { expect(logSuccessfulAuthorization).toHaveBeenCalled(); }); }); + + describe('auditLogger', () => { + test('logs audit event when searching alerts', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + await alertsClient.find(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_find', + outcome: 'success', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to search alerts', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + authorization.getFindAuthorizationFilter.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.find()).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_find', + outcome: 'failure', + }), + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + + test('logs audit event when not authorised to search alert type', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + authorization.getFindAuthorizationFilter.mockResolvedValue({ + ensureAlertTypeIsAuthorized: jest.fn(() => { + throw new Error('Unauthorized'); + }), + logSuccessfulAuthorization: jest.fn(), + }); + + await expect(async () => await alertsClient.find()).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_find', + outcome: 'failure', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts index 3f0c783f424d13..8a377856fd6a62 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts @@ -12,15 +12,17 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -42,6 +44,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -191,4 +194,61 @@ describe('get()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('myType', 'myApp', 'get'); }); }); + + describe('auditLogger', () => { + beforeEach(() => { + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + alertTypeId: '123', + schedule: { interval: '10s' }, + params: { + bar: true, + }, + actions: [], + }, + references: [], + }); + }); + + test('logs audit event when getting an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + await alertsClient.get({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_get', + outcome: 'success', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to get an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.get({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_get', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts index 14ebca21355879..396e933ec08baf 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts @@ -12,6 +12,8 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); @@ -20,6 +22,7 @@ const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -41,6 +44,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -137,4 +141,85 @@ describe('muteAll()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('myType', 'myApp', 'muteAll'); }); }); + + describe('auditLogger', () => { + test('logs audit event when muting an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: false, + }, + references: [], + version: '123', + }); + await alertsClient.muteAll({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_mute', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to mute an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: false, + }, + references: [], + version: '123', + }); + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.muteAll({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_mute', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/mute_instance.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/mute_instance.test.ts index c2188f128cb4d5..ec69dbdeac55f9 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/mute_instance.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/mute_instance.test.ts @@ -12,15 +12,17 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -42,6 +44,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -180,4 +183,75 @@ describe('muteInstance()', () => { ); }); }); + + describe('auditLogger', () => { + test('logs audit event when muting an alert instance', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', + mutedInstanceIds: [], + }, + version: '123', + references: [], + }); + await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_instance_mute', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to mute an alert instance', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', + mutedInstanceIds: [], + }, + version: '123', + references: [], + }); + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect( + alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }) + ).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_instance_mute', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts index d92304ab873be7..0315e7d42fbf71 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts @@ -12,15 +12,17 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -42,6 +44,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -138,4 +141,85 @@ describe('unmuteAll()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('myType', 'myApp', 'unmuteAll'); }); }); + + describe('auditLogger', () => { + test('logs audit event when unmuting an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: false, + }, + references: [], + version: '123', + }); + await alertsClient.unmuteAll({ id: '1' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_unmute', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to unmute an alert', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: false, + }, + references: [], + version: '123', + }); + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.unmuteAll({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_unmute', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_instance.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_instance.test.ts index 3486df98f2f057..c7d084a01a2a0e 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_instance.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_instance.test.ts @@ -12,15 +12,17 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -42,6 +44,7 @@ const alertsClientParams: jest.Mocked = { beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -178,4 +181,75 @@ describe('unmuteInstance()', () => { ); }); }); + + describe('auditLogger', () => { + test('logs audit event when unmuting an alert instance', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', + mutedInstanceIds: [], + }, + version: '123', + references: [], + }); + await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_instance_unmute', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to unmute an alert instance', async () => { + const alertsClient = new AlertsClient({ ...alertsClientParams, auditLogger }); + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', + mutedInstanceIds: [], + }, + version: '123', + references: [], + }); + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect( + alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }) + ).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_instance_unmute', + outcome: 'failure', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts index 046d7ec63c048c..6de3929f319d01 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts @@ -17,15 +17,17 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { resolvable } from '../../test_utils'; import { ActionsAuthorization, ActionsClient } from '../../../../actions/server'; import { TaskStatus } from '../../../../task_manager/server'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); - const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -43,10 +45,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -1298,4 +1302,89 @@ describe('update()', () => { expect(authorization.ensureAuthorized).toHaveBeenCalledWith('myType', 'myApp', 'update'); }); }); + + describe('auditLogger', () => { + beforeEach(() => { + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + enabled: true, + schedule: { interval: '10s' }, + params: { + bar: true, + }, + actions: [], + scheduledTaskId: 'task-123', + createdAt: new Date().toISOString(), + }, + updated_at: new Date().toISOString(), + references: [], + }); + }); + + test('logs audit event when updating an alert', async () => { + await alertsClient.update({ + id: '1', + data: { + schedule: { interval: '10s' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + throttle: null, + actions: [], + }, + }); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_update', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to update an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect( + alertsClient.update({ + id: '1', + data: { + schedule: { interval: '10s' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + throttle: null, + actions: [], + }, + }) + ).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + outcome: 'failure', + action: 'alert_rule_update', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts index ca5f44078f5138..f4cf8c36c627fe 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts @@ -12,8 +12,10 @@ import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/s import { actionsAuthorizationMock } from '../../../../actions/server/mocks'; import { AlertsAuthorization } from '../../authorization/alerts_authorization'; import { ActionsAuthorization } from '../../../../actions/server'; -import { getBeforeSetup, setGlobalDate } from './lib'; +import { httpServerMock } from '../../../../../../src/core/server/mocks'; +import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { InvalidatePendingApiKey } from '../../types'; +import { getBeforeSetup, setGlobalDate } from './lib'; const taskManager = taskManagerMock.createStart(); const alertTypeRegistry = alertTypeRegistryMock.create(); @@ -21,6 +23,7 @@ const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); const encryptedSavedObjects = encryptedSavedObjectsMock.createClient(); const authorization = alertsAuthorizationMock.create(); const actionsAuthorization = actionsAuthorizationMock.create(); +const auditLogger = auditServiceMock.create().asScoped(httpServerMock.createKibanaRequest()); const kibanaVersion = 'v7.10.0'; const alertsClientParams: jest.Mocked = { @@ -38,10 +41,12 @@ const alertsClientParams: jest.Mocked = { getActionsClient: jest.fn(), getEventLogClient: jest.fn(), kibanaVersion, + auditLogger, }; beforeEach(() => { getBeforeSetup(alertsClientParams, taskManager, alertTypeRegistry); + (auditLogger.log as jest.Mock).mockClear(); }); setGlobalDate(); @@ -269,4 +274,44 @@ describe('updateApiKey()', () => { ); }); }); + + describe('auditLogger', () => { + test('logs audit event when updating the API key of an alert', async () => { + await alertsClient.updateApiKey({ id: '1' }); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + action: 'alert_rule_update_api_key', + outcome: 'unknown', + }), + kibana: { saved_object: { id: '1', type: 'alert' } }, + }) + ); + }); + + test('logs audit event when not authorised to update the API key of an alert', async () => { + authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized')); + + await expect(alertsClient.updateApiKey({ id: '1' })).rejects.toThrow(); + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + outcome: 'failure', + action: 'alert_rule_update_api_key', + }), + kibana: { + saved_object: { + id: '1', + type: 'alert', + }, + }, + error: { + code: 'Error', + message: 'Unauthorized', + }, + }) + ); + }); + }); }); diff --git a/x-pack/plugins/alerts/server/alerts_client_factory.ts b/x-pack/plugins/alerts/server/alerts_client_factory.ts index 069703be72f8a5..9d71b5f817b2c0 100644 --- a/x-pack/plugins/alerts/server/alerts_client_factory.ts +++ b/x-pack/plugins/alerts/server/alerts_client_factory.ts @@ -100,6 +100,7 @@ export class AlertsClientFactory { actionsAuthorization: actions.getActionsAuthorizationWithRequest(request), namespace: this.spaceIdToNamespace(spaceId), encryptedSavedObjectsClient: this.encryptedSavedObjectsClient, + auditLogger: securityPluginSetup?.audit.asScoped(request), async getUserName() { if (!securityPluginSetup) { return null; diff --git a/x-pack/plugins/alerts/server/saved_objects/index.ts b/x-pack/plugins/alerts/server/saved_objects/index.ts index dfe122f56bc485..b2ef38e2fa8909 100644 --- a/x-pack/plugins/alerts/server/saved_objects/index.ts +++ b/x-pack/plugins/alerts/server/saved_objects/index.ts @@ -65,6 +65,7 @@ export function setupSavedObjects( type: 'alert', attributesToEncrypt: new Set(['apiKey']), attributesToExcludeFromAAD: new Set(AlertAttributesExcludedFromAAD), + allowPredefinedID: true, }); // Encrypted attributes diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index ddef9f477433ca..2002a779ba873b 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import uuid from 'uuid'; import { SavedObject, SavedObjectsBaseOptions, @@ -25,7 +24,8 @@ import { SavedObjectsRemoveReferencesToOptions, ISavedObjectTypeRegistry, SavedObjectsRemoveReferencesToResponse, -} from 'src/core/server'; + generateSavedObjectId, +} from '../../../../../src/core/server'; import { AuthenticatedUser } from '../../../security/common/model'; import { EncryptedSavedObjectsService } from '../crypto'; import { getDescriptorNamespace } from './get_descriptor_namespace'; @@ -37,14 +37,6 @@ interface EncryptedSavedObjectsClientOptions { getCurrentUser: () => AuthenticatedUser | undefined; } -/** - * Generates UUIDv4 ID for the any newly created saved object that is supposed to contain - * encrypted attributes. - */ -function generateID() { - return uuid.v4(); -} - export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientContract { constructor( private readonly options: EncryptedSavedObjectsClientOptions, @@ -79,7 +71,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); } - const id = options.id ?? generateID(); + const id = options.id ?? generateSavedObjectId(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -125,7 +117,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); } - const id = object.id ?? generateID(); + const id = object.id ?? generateSavedObjectId(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, diff --git a/x-pack/plugins/security/server/audit/audit_events.ts b/x-pack/plugins/security/server/audit/audit_events.ts index 6aba78c9360710..26b13222c6ff28 100644 --- a/x-pack/plugins/security/server/audit/audit_events.ts +++ b/x-pack/plugins/security/server/audit/audit_events.ts @@ -45,7 +45,7 @@ export interface AuditEvent { */ saved_object?: { type: string; - id?: string; + id: string; }; /** * Any additional event specific fields. @@ -178,7 +178,9 @@ export enum SavedObjectAction { REMOVE_REFERENCES = 'saved_object_remove_references', } -const eventVerbs = { +type VerbsTuple = [string, string, string]; + +const eventVerbs: Record = { saved_object_create: ['create', 'creating', 'created'], saved_object_get: ['access', 'accessing', 'accessed'], saved_object_update: ['update', 'updating', 'updated'], @@ -193,7 +195,7 @@ const eventVerbs = { ], }; -const eventTypes = { +const eventTypes: Record = { saved_object_create: EventType.CREATION, saved_object_get: EventType.ACCESS, saved_object_update: EventType.CHANGE, @@ -204,7 +206,7 @@ const eventTypes = { saved_object_remove_references: EventType.CHANGE, }; -export interface SavedObjectParams { +export interface SavedObjectEventParams { action: SavedObjectAction; outcome?: EventOutcome; savedObject?: Required['kibana']>['saved_object']; @@ -220,7 +222,7 @@ export function savedObjectEvent({ deleteFromSpaces, outcome, error, -}: SavedObjectParams): AuditEvent | undefined { +}: SavedObjectEventParams): AuditEvent | undefined { const doc = savedObject ? `${savedObject.type} [id=${savedObject.id}]` : 'saved objects'; const [present, progressive, past] = eventVerbs[action]; const message = error diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index c6f4ca6dd8afe6..40a0fb73e0c74f 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -724,14 +724,14 @@ describe('#create', () => { const options = { namespace }; await expectSuccess(client.create, { type, attributes, options }); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); - expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type }); + expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type, id: expect.any(String) }); }); test(`adds audit event when not successful`, async () => { clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error()); await expect(() => client.create(type, attributes, { namespace })).rejects.toThrow(); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); - expectAuditEvent('saved_object_create', EventOutcome.FAILURE, { type }); + expectAuditEvent('saved_object_create', EventOutcome.FAILURE, { type, id: expect.any(String) }); }); }); diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index e6e34de4ac9abf..c1aa3bd614c930 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -18,6 +18,7 @@ import { SavedObjectsRemoveReferencesToOptions, SavedObjectsUpdateOptions, SavedObjectsUtils, + generateSavedObjectId, } from '../../../../../src/core/server'; import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants'; import { @@ -96,15 +97,16 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra attributes: T = {} as T, options: SavedObjectsCreateOptions = {} ) { - const namespaces = [options.namespace, ...(options.initialNamespaces || [])]; + const optionsWithId = { ...options, id: options.id ?? generateSavedObjectId() }; + const namespaces = [optionsWithId.namespace, ...(optionsWithId.initialNamespaces || [])]; try { - const args = { type, attributes, options }; + const args = { type, attributes, options: optionsWithId }; await this.ensureAuthorized(type, 'create', namespaces, { args }); } catch (error) { this.auditLogger.log( savedObjectEvent({ action: SavedObjectAction.CREATE, - savedObject: { type, id: options.id }, + savedObject: { type, id: optionsWithId.id }, error, }) ); @@ -114,11 +116,11 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra savedObjectEvent({ action: SavedObjectAction.CREATE, outcome: EventOutcome.UNKNOWN, - savedObject: { type, id: options.id }, + savedObject: { type, id: optionsWithId.id }, }) ); - const savedObject = await this.baseClient.create(type, attributes, options); + const savedObject = await this.baseClient.create(type, attributes, optionsWithId); return await this.redactSavedObjectNamespaces(savedObject, namespaces); } @@ -141,17 +143,23 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra objects: Array>, options: SavedObjectsBaseOptions = {} ) { - const namespaces = objects.reduce( + const objectsWithId = objects.map((obj) => ({ ...obj, id: obj.id ?? generateSavedObjectId() })); + const namespaces = objectsWithId.reduce( (acc, { initialNamespaces = [] }) => acc.concat(initialNamespaces), [options.namespace] ); try { - const args = { objects, options }; - await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_create', namespaces, { - args, - }); + const args = { objects: objectsWithId, options }; + await this.ensureAuthorized( + this.getUniqueObjectTypes(objectsWithId), + 'bulk_create', + namespaces, + { + args, + } + ); } catch (error) { - objects.forEach(({ type, id }) => + objectsWithId.forEach(({ type, id }) => this.auditLogger.log( savedObjectEvent({ action: SavedObjectAction.CREATE, @@ -162,7 +170,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ); throw error; } - objects.forEach(({ type, id }) => + objectsWithId.forEach(({ type, id }) => this.auditLogger.log( savedObjectEvent({ action: SavedObjectAction.CREATE, @@ -172,7 +180,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ) ); - const response = await this.baseClient.bulkCreate(objects, options); + const response = await this.baseClient.bulkCreate(objectsWithId, options); return await this.redactSavedObjectsNamespaces(response, namespaces); } From 3652cf08ee30e4e575ebad57452f477526edae51 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Tue, 24 Nov 2020 09:19:28 +0000 Subject: [PATCH 02/12] added api changes --- ...-plugin-core-server.generatesavedobjectid.md | 17 +++++++++++++++++ .../core/server/kibana-plugin-core-server.md | 1 + .../saved_objects/serialization/serializer.ts | 5 +++++ src/core/server/server.api.md | 3 +++ 4 files changed, 26 insertions(+) create mode 100644 docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md diff --git a/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md b/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md new file mode 100644 index 00000000000000..6c43d934062fb4 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md @@ -0,0 +1,17 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [generateSavedObjectId](./kibana-plugin-core-server.generatesavedobjectid.md) + +## generateSavedObjectId() function + +Generates a random id for saved objects. + +Signature: + +```typescript +export declare function generateSavedObjectId(): string; +``` +Returns: + +`string` + diff --git a/docs/development/core/server/kibana-plugin-core-server.md b/docs/development/core/server/kibana-plugin-core-server.md index adbb2460dc80aa..5ecefcbbb8b989 100644 --- a/docs/development/core/server/kibana-plugin-core-server.md +++ b/docs/development/core/server/kibana-plugin-core-server.md @@ -43,6 +43,7 @@ The plugin integrates with the core system via lifecycle events: `setup` | Function | Description | | --- | --- | | [exportSavedObjectsToStream({ types, hasReference, objects, search, savedObjectsClient, exportSizeLimit, includeReferencesDeep, excludeExportDetails, namespace, })](./kibana-plugin-core-server.exportsavedobjectstostream.md) | Generates sorted saved object stream to be used for export. See the [options](./kibana-plugin-core-server.savedobjectsexportoptions.md) for more detailed information. | +| [generateSavedObjectId()](./kibana-plugin-core-server.generatesavedobjectid.md) | Generates a random id for saved objects. | | [importSavedObjectsFromStream({ readStream, objectLimit, overwrite, createNewCopies, savedObjectsClient, typeRegistry, namespace, })](./kibana-plugin-core-server.importsavedobjectsfromstream.md) | Import saved objects from given stream. See the [options](./kibana-plugin-core-server.savedobjectsimportoptions.md) for more detailed information. | | [resolveSavedObjectsImportErrors({ readStream, objectLimit, retries, savedObjectsClient, typeRegistry, namespace, createNewCopies, })](./kibana-plugin-core-server.resolvesavedobjectsimporterrors.md) | Resolve and return saved object import errors. See the [options](./kibana-plugin-core-server.savedobjectsresolveimporterrorsoptions.md) for more detailed informations. | diff --git a/src/core/server/saved_objects/serialization/serializer.ts b/src/core/server/saved_objects/serialization/serializer.ts index 092b62b080158d..c26ccc4fd0ec6d 100644 --- a/src/core/server/saved_objects/serialization/serializer.ts +++ b/src/core/server/saved_objects/serialization/serializer.ts @@ -149,6 +149,11 @@ export class SavedObjectsSerializer { } } +/** + * Generates a random id for saved objects. + * + * @public + */ export function generateSavedObjectId() { return uuid.v1(); } diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index a03e5ec9acd275..a63d0b5261c980 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -738,6 +738,9 @@ export interface FakeRequest { headers: Headers; } +// @public +export function generateSavedObjectId(): string; + // @public export type GetAuthHeaders = (request: KibanaRequest | LegacyRequest) => AuthHeaders | undefined; From ac215a06d32fe16910ccb458c2ccc35a76e805ed Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Tue, 24 Nov 2020 10:33:49 +0000 Subject: [PATCH 03/12] fixed linting and testing errors --- .../plugins/actions/server/actions_client.ts | 8 ++-- .../actions/server/lib/audit_events.ts | 2 +- .../server/alerts_client/alerts_client.ts | 42 +++++++------------ .../server/alerts_client/audit_events.ts | 2 +- x-pack/plugins/security/server/index.ts | 9 +++- ...ecure_saved_objects_client_wrapper.test.ts | 16 +++++-- 6 files changed, 42 insertions(+), 37 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 4143c2b9223346..790b92070b61d3 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -4,6 +4,9 @@ * you may not use this file except in compliance with the Elastic License. */ import Boom from '@hapi/boom'; + +import { i18n } from '@kbn/i18n'; +import { omitBy, isUndefined } from 'lodash'; import { ILegacyScopedClusterClient, SavedObjectsClientContract, @@ -12,10 +15,7 @@ import { KibanaRequest, generateSavedObjectId, } from '../../../../src/core/server'; - -import { i18n } from '@kbn/i18n'; -import { omitBy, isUndefined } from 'lodash'; -import { AuditLogger, EventOutcome } from '../../security/server/audit'; +import { AuditLogger, EventOutcome } from '../../security/server'; import { ActionType } from '../common'; import { ActionTypeRegistry } from './action_type_registry'; import { validateConfig, validateSecrets, ActionExecutorContract } from './lib'; diff --git a/x-pack/plugins/actions/server/lib/audit_events.ts b/x-pack/plugins/actions/server/lib/audit_events.ts index aa4173769f4d2a..481a0da090b491 100644 --- a/x-pack/plugins/actions/server/lib/audit_events.ts +++ b/x-pack/plugins/actions/server/lib/audit_events.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server/audit'; +import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server'; export enum ConnectorAction { CREATE = 'connector_create', diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index 24f58e77497bfe..17a94cca94b9e5 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -45,7 +45,7 @@ import { IEventLogClient } from '../../../../plugins/event_log/server'; import { parseIsoOrRelativeDate } from '../lib/iso_or_relative_date'; import { alertInstanceSummaryFromEventLog } from '../lib/alert_instance_summary_from_event_log'; import { IEvent } from '../../../event_log/server'; -import { AuditLogger, EventOutcome } from '../../../security/server/audit'; +import { AuditLogger, EventOutcome } from '../../../security/server'; import { parseDuration } from '../../common/parse_duration'; import { retryIfConflicts } from '../lib/retry_if_conflicts'; import { partiallyUpdateAlert } from '../saved_objects'; @@ -534,11 +534,9 @@ export class AlertsClient { let attributes: RawAlert; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( - 'alert', - id, - { namespace: this.namespace } - ); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; taskIdToRemove = decryptedAlert.attributes.scheduledTaskId; attributes = decryptedAlert.attributes; @@ -606,11 +604,9 @@ export class AlertsClient { let alertSavedObject: SavedObject; try { - alertSavedObject = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( - 'alert', - id, - { namespace: this.namespace } - ); + alertSavedObject = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -759,11 +755,9 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( - 'alert', - id, - { namespace: this.namespace } - ); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; @@ -855,11 +849,9 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( - 'alert', - id, - { namespace: this.namespace } - ); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; @@ -958,11 +950,9 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( - 'alert', - id, - { namespace: this.namespace } - ); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts index 0cce835d672c15..2aa25bc455a14e 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server/audit'; +import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server'; export enum AlertRuleAction { CREATE = 'alert_rule_create', diff --git a/x-pack/plugins/security/server/index.ts b/x-pack/plugins/security/server/index.ts index 04db65f88cda0a..d99fbc702a0782 100644 --- a/x-pack/plugins/security/server/index.ts +++ b/x-pack/plugins/security/server/index.ts @@ -27,7 +27,14 @@ export { SAMLLogin, OIDCLogin, } from './authentication'; -export { LegacyAuditLogger } from './audit'; +export { + LegacyAuditLogger, + AuditLogger, + AuditEvent, + EventCategory, + EventType, + EventOutcome, +} from './audit'; export { SecurityPluginSetup }; export { AuthenticatedUser } from '../common/model'; diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 40a0fb73e0c74f..bf8e925a2dc36c 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -12,6 +12,10 @@ import { SavedObjectsClientContract } from 'kibana/server'; import { SavedObjectActions } from '../authorization/actions/saved_object'; import { AuditEvent, EventOutcome } from '../audit'; +jest.mock('../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ + generateSavedObjectId: () => 'GENERATED_ID', +})); + let clientOpts: ReturnType; let client: SecureSavedObjectsClientWrapper; const USERNAME = Symbol(); @@ -686,7 +690,7 @@ describe('#create', () => { }); test(`throws decorated ForbiddenError when unauthorized`, async () => { - const options = { namespace }; + const options = { id: 'GENERATED_ID', namespace }; await expectForbiddenError(client.create, { type, attributes, options }); }); @@ -694,8 +698,12 @@ describe('#create', () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.create.mockResolvedValue(apiCallReturnValue as any); - const options = { namespace }; - const result = await expectSuccess(client.create, { type, attributes, options }); + const options = { id: 'GENERATED_ID', namespace }; + const result = await expectSuccess(client.create, { + type, + attributes, + options, + }); expect(result).toBe(apiCallReturnValue); }); @@ -721,7 +729,7 @@ describe('#create', () => { test(`adds audit event when successful`, async () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.create.mockResolvedValue(apiCallReturnValue as any); - const options = { namespace }; + const options = { id: 'GENERATED_ID', namespace }; await expectSuccess(client.create, { type, attributes, options }); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type, id: expect.any(String) }); From 6c3cb478b1753071cc298f318b660431c2199147 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Tue, 24 Nov 2020 11:22:47 +0000 Subject: [PATCH 04/12] fix test --- ...ypted_saved_objects_client_wrapper.test.ts | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 3c722ccfabae24..f773aef13d63b2 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -13,7 +13,9 @@ import { savedObjectsClientMock, savedObjectsTypeRegistryMock } from 'src/core/s import { mockAuthenticatedUser } from '../../../security/common/model/authenticated_user.mock'; import { encryptedSavedObjectsServiceMock } from '../crypto/index.mock'; -jest.mock('uuid', () => ({ v4: jest.fn().mockReturnValue('uuid-v4-id') })); +jest.mock('../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ + generateSavedObjectId: () => 'GENERATED_ID', +})); let wrapper: EncryptedSavedObjectsClientWrapper; let mockBaseClient: jest.Mocked; @@ -168,7 +170,7 @@ describe('#create', () => { }; const options = { overwrite: true }; const mockedResponse = { - id: 'uuid-v4-id', + id: 'GENERATED_ID', type: 'known-type', attributes: { attrOne: 'one', @@ -188,7 +190,7 @@ describe('#create', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'uuid-v4-id' }, + { type: 'known-type', id: 'GENERATED_ID' }, { attrOne: 'one', attrSecret: 'secret', @@ -207,7 +209,7 @@ describe('#create', () => { attrNotSoSecret: '*not-so-secret*', attrThree: 'three', }, - { id: 'uuid-v4-id', overwrite: true } + { id: 'GENERATED_ID', overwrite: true } ); }); @@ -216,7 +218,7 @@ describe('#create', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const options = { overwrite: true, namespace }; const mockedResponse = { - id: 'uuid-v4-id', + id: 'GENERATED_ID', type: 'known-type', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, references: [], @@ -233,7 +235,7 @@ describe('#create', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( { type: 'known-type', - id: 'uuid-v4-id', + id: 'GENERATED_ID', namespace: expectNamespaceInDescriptor ? namespace : undefined, }, { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, @@ -244,7 +246,7 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'uuid-v4-id', overwrite: true, namespace } + { id: 'GENERATED_ID', overwrite: true, namespace } ); }; @@ -270,7 +272,7 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'uuid-v4-id' } + { id: 'GENERATED_ID' } ); }); }); @@ -282,7 +284,7 @@ describe('#bulkCreate', () => { const mockedResponse = { saved_objects: [ { - id: 'uuid-v4-id', + id: 'GENERATED_ID', type: 'known-type', attributes, references: [], @@ -315,7 +317,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'uuid-v4-id', + id: 'GENERATED_ID', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, bulkCreateParams[1], @@ -456,7 +458,7 @@ describe('#bulkCreate', () => { const mockedResponse = { saved_objects: [ { - id: 'uuid-v4-id', + id: 'GENERATED_ID', type: 'known-type', attributes: { ...attributes, attrSecret: '*secret*', attrNotSoSecret: '*not-so-secret*' }, references: [], @@ -489,7 +491,7 @@ describe('#bulkCreate', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'uuid-v4-id' }, + { type: 'known-type', id: 'GENERATED_ID' }, { attrOne: 'one', attrSecret: 'secret', @@ -504,7 +506,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'uuid-v4-id', + id: 'GENERATED_ID', attributes: { attrOne: 'one', attrSecret: '*secret*', @@ -523,7 +525,7 @@ describe('#bulkCreate', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const options = { namespace }; const mockedResponse = { - saved_objects: [{ id: 'uuid-v4-id', type: 'known-type', attributes, references: [] }], + saved_objects: [{ id: 'GENERATED_ID', type: 'known-type', attributes, references: [] }], }; mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); @@ -542,7 +544,7 @@ describe('#bulkCreate', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( { type: 'known-type', - id: 'uuid-v4-id', + id: 'GENERATED_ID', namespace: expectNamespaceInDescriptor ? namespace : undefined, }, { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, @@ -554,7 +556,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'uuid-v4-id', + id: 'GENERATED_ID', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, ], @@ -590,7 +592,7 @@ describe('#bulkCreate', () => { [ { type: 'known-type', - id: 'uuid-v4-id', + id: 'GENERATED_ID', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, ], From 337a902f19d19d75983ded107ec8be647351da57 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Wed, 25 Nov 2020 10:37:35 +0000 Subject: [PATCH 05/12] Fixed linting errors after prettier update --- .../plugins/actions/server/actions_client.ts | 8 ++-- .../server/alerts_client/alerts_client.ts | 41 +++++++++++-------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 790b92070b61d3..ff5b74e1ab341b 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -198,9 +198,11 @@ export class ActionsClient { ); throw error; } - const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< - RawAction - >('action', id); + const { + attributes, + references, + version, + } = await this.unsecuredSavedObjectsClient.get('action', id); const { actionTypeId } = attributes; const { name, config, secrets } = action; const actionType = this.actionTypeRegistry.get(actionTypeId); diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index 17a94cca94b9e5..3dbae7a38842be 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -450,7 +450,6 @@ export class AlertsClient { type: 'alert', }); - // eslint-disable-next-line @typescript-eslint/naming-convention const authorizedData = data.map(({ id, attributes, references }) => { try { ensureAlertTypeIsAuthorized(attributes.alertTypeId, attributes.consumer); @@ -534,9 +533,11 @@ export class AlertsClient { let attributes: RawAlert; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + id, + { namespace: this.namespace } + ); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; taskIdToRemove = decryptedAlert.attributes.scheduledTaskId; attributes = decryptedAlert.attributes; @@ -604,9 +605,11 @@ export class AlertsClient { let alertSavedObject: SavedObject; try { - alertSavedObject = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); + alertSavedObject = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + id, + { namespace: this.namespace } + ); } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -755,9 +758,11 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + id, + { namespace: this.namespace } + ); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; @@ -849,9 +854,11 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + id, + { namespace: this.namespace } + ); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; @@ -950,9 +957,11 @@ export class AlertsClient { let version: string | undefined; try { - const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); + const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + id, + { namespace: this.namespace } + ); apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; From 5f598d1b1d5092f7f2748ae787722852831f8960 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Wed, 25 Nov 2020 19:05:06 +0000 Subject: [PATCH 06/12] Revert "Allow predefined ids for encrypted saved objects (#83482)" This reverts commit 7d929fe9030b67a6dd8017e8d61e8521f3fd74f8. --- ...ypted_saved_object_type_definition.test.ts | 15 ---- .../encrypted_saved_object_type_definition.ts | 2 - .../encrypted_saved_objects_service.mocks.ts | 7 -- .../encrypted_saved_objects_service.test.ts | 39 ----------- .../crypto/encrypted_saved_objects_service.ts | 20 ------ ...ypted_saved_objects_client_wrapper.test.ts | 69 ++----------------- .../encrypted_saved_objects_client_wrapper.ts | 24 +++---- 7 files changed, 16 insertions(+), 160 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts index f1e06a0cec03df..f528843cf9ea36 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.test.ts @@ -113,18 +113,3 @@ it('correctly determines attribute properties', () => { } } }); - -it('it correctly sets allowPredefinedID', () => { - const defaultTypeDefinition = new EncryptedSavedObjectAttributesDefinition({ - type: 'so-type', - attributesToEncrypt: new Set(['attr#1', 'attr#2']), - }); - expect(defaultTypeDefinition.allowPredefinedID).toBe(false); - - const typeDefinitionWithPredefinedIDAllowed = new EncryptedSavedObjectAttributesDefinition({ - type: 'so-type', - attributesToEncrypt: new Set(['attr#1', 'attr#2']), - allowPredefinedID: true, - }); - expect(typeDefinitionWithPredefinedIDAllowed.allowPredefinedID).toBe(true); -}); diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts index 398a64585411a7..849a2888b6e1a5 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts @@ -15,7 +15,6 @@ export class EncryptedSavedObjectAttributesDefinition { public readonly attributesToEncrypt: ReadonlySet; private readonly attributesToExcludeFromAAD: ReadonlySet | undefined; private readonly attributesToStrip: ReadonlySet; - public readonly allowPredefinedID: boolean; constructor(typeRegistration: EncryptedSavedObjectTypeRegistration) { const attributesToEncrypt = new Set(); @@ -35,7 +34,6 @@ export class EncryptedSavedObjectAttributesDefinition { this.attributesToEncrypt = attributesToEncrypt; this.attributesToStrip = attributesToStrip; this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD; - this.allowPredefinedID = !!typeRegistration.allowPredefinedID; } /** diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts index 0138e929ca1caf..c692d8698771fe 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts @@ -13,7 +13,6 @@ import { function createEncryptedSavedObjectsServiceMock() { return ({ isRegistered: jest.fn(), - canSpecifyID: jest.fn(), stripOrDecryptAttributes: jest.fn(), encryptAttributes: jest.fn(), decryptAttributes: jest.fn(), @@ -53,12 +52,6 @@ export const encryptedSavedObjectsServiceMock = { mock.isRegistered.mockImplementation( (type) => registrations.findIndex((r) => r.type === type) >= 0 ); - mock.canSpecifyID.mockImplementation((type, version, overwrite) => { - const registration = registrations.find((r) => r.type === type); - return ( - registration === undefined || registration.allowPredefinedID || !!(version && overwrite) - ); - }); mock.encryptAttributes.mockImplementation(async (descriptor, attrs) => processAttributes( descriptor, diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts index 6bc4a392064e43..88d57072697fe6 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts @@ -89,45 +89,6 @@ describe('#isRegistered', () => { }); }); -describe('#canSpecifyID', () => { - it('returns true for unknown types', () => { - expect(service.canSpecifyID('unknown-type')).toBe(true); - }); - - it('returns true for types registered setting allowPredefinedID to true', () => { - service.registerType({ - type: 'known-type-1', - attributesToEncrypt: new Set(['attr-1']), - allowPredefinedID: true, - }); - expect(service.canSpecifyID('known-type-1')).toBe(true); - }); - - it('returns true when overwriting a saved object with a version specified even when allowPredefinedID is not set', () => { - service.registerType({ - type: 'known-type-1', - attributesToEncrypt: new Set(['attr-1']), - }); - expect(service.canSpecifyID('known-type-1', '2', true)).toBe(true); - expect(service.canSpecifyID('known-type-1', '2', false)).toBe(false); - expect(service.canSpecifyID('known-type-1', undefined, true)).toBe(false); - }); - - it('returns false for types registered without setting allowPredefinedID', () => { - service.registerType({ type: 'known-type-1', attributesToEncrypt: new Set(['attr-1']) }); - expect(service.canSpecifyID('known-type-1')).toBe(false); - }); - - it('returns false for types registered setting allowPredefinedID to false', () => { - service.registerType({ - type: 'known-type-1', - attributesToEncrypt: new Set(['attr-1']), - allowPredefinedID: false, - }); - expect(service.canSpecifyID('known-type-1')).toBe(false); - }); -}); - describe('#stripOrDecryptAttributes', () => { it('does not strip attributes from unknown types', async () => { const attributes = { attrOne: 'one', attrTwo: 'two', attrThree: 'three' }; diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts index 8d2ebb575c35e0..1f1093a179538c 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts @@ -31,7 +31,6 @@ export interface EncryptedSavedObjectTypeRegistration { readonly type: string; readonly attributesToEncrypt: ReadonlySet; readonly attributesToExcludeFromAAD?: ReadonlySet; - readonly allowPredefinedID?: boolean; } /** @@ -145,25 +144,6 @@ export class EncryptedSavedObjectsService { return this.typeDefinitions.has(type); } - /** - * Checks whether ID can be specified for the provided saved object. - * - * If the type isn't registered as an encrypted saved object, or when overwriting an existing - * saved object with a version specified, this will return "true". - * - * @param type Saved object type. - * @param version Saved object version number which changes on each successful write operation. - * Can be used in conjunction with `overwrite` for implementing optimistic concurrency - * control. - * @param overwrite Overwrite existing documents. - */ - public canSpecifyID(type: string, version?: string, overwrite?: boolean) { - const typeDefinition = this.typeDefinitions.get(type); - return ( - typeDefinition === undefined || typeDefinition.allowPredefinedID || !!(version && overwrite) - ); - } - /** * Takes saved object attributes for the specified type and, depending on the type definition, * either decrypts or strips encrypted attributes (e.g. in case AAD or encryption key has changed diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index f773aef13d63b2..262ce45863fcd9 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -32,11 +32,6 @@ beforeEach(() => { { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, ]), }, - { - type: 'known-type-predefined-id', - attributesToEncrypt: new Set(['attrSecret']), - allowPredefinedID: true, - }, ]); wrapper = new EncryptedSavedObjectsClientWrapper({ @@ -79,36 +74,16 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith('unknown-type', attributes, options); }); - it('fails if type is registered without allowPredefinedID and ID is specified', async () => { + it('fails if type is registered and ID is specified', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; await expect(wrapper.create('known-type', attributes, { id: 'some-id' })).rejects.toThrowError( - 'Predefined IDs are not allowed for encrypted saved objects of type "known-type".' + 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); expect(mockBaseClient.create).not.toHaveBeenCalled(); }); - it('succeeds if type is registered with allowPredefinedID and ID is specified', async () => { - const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; - const mockedResponse = { - id: 'some-id', - type: 'known-type-predefined-id', - attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - references: [], - }; - - mockBaseClient.create.mockResolvedValue(mockedResponse); - await expect( - wrapper.create('known-type-predefined-id', attributes, { id: 'some-id' }) - ).resolves.toEqual({ - ...mockedResponse, - attributes: { attrOne: 'one', attrThree: 'three' }, - }); - - expect(mockBaseClient.create).toHaveBeenCalled(); - }); - it('allows a specified ID when overwriting an existing object', async () => { const attributes = { attrOne: 'one', @@ -326,7 +301,7 @@ describe('#bulkCreate', () => { ); }); - it('fails if ID is specified for registered type without allowPredefinedID', async () => { + it('fails if ID is specified for registered type', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const bulkCreateParams = [ @@ -335,48 +310,12 @@ describe('#bulkCreate', () => { ]; await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError( - 'Predefined IDs are not allowed for encrypted saved objects of type "known-type".' + 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled(); }); - it('succeeds if ID is specified for registered type with allowPredefinedID', async () => { - const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; - const options = { namespace: 'some-namespace' }; - const mockedResponse = { - saved_objects: [ - { - id: 'some-id', - type: 'known-type-predefined-id', - attributes, - references: [], - }, - { - id: 'some-id', - type: 'unknown-type', - attributes, - references: [], - }, - ], - }; - mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); - - const bulkCreateParams = [ - { id: 'some-id', type: 'known-type-predefined-id', attributes }, - { type: 'unknown-type', attributes }, - ]; - - await expect(wrapper.bulkCreate(bulkCreateParams, options)).resolves.toEqual({ - saved_objects: [ - { ...mockedResponse.saved_objects[0], attributes: { attrOne: 'one', attrThree: 'three' } }, - mockedResponse.saved_objects[1], - ], - }); - - expect(mockBaseClient.bulkCreate).toHaveBeenCalled(); - }); - it('allows a specified ID when overwriting an existing object', async () => { const attributes = { attrOne: 'one', diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 2002a779ba873b..e2984f7e7c34f0 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,14 +60,16 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon } // Saved objects with encrypted attributes should have IDs that are hard to guess especially - // since IDs are part of the AAD used during encryption. Types can opt-out of this restriction, - // when necessary, but it's much safer for this wrapper to generate them. - if ( - options.id && - !this.options.service.canSpecifyID(type, options.version, options.overwrite) - ) { + // since IDs are part of the AAD used during encryption, that's why we control them within this + // wrapper and don't allow consumers to specify their own IDs directly. + + // only allow a specified ID if we're overwriting an existing ESO with a Version + // this helps us ensure that the document really was previously created using ESO + // and not being used to get around the specified ID limitation + const canSpecifyID = options.overwrite && options.version; + if (options.id && !canSpecifyID) { throw new Error( - `Predefined IDs are not allowed for encrypted saved objects of type "${type}".` + 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } @@ -108,12 +110,10 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. - if ( - object.id && - !this.options.service.canSpecifyID(object.type, object.version, options?.overwrite) - ) { + const canSpecifyID = options?.overwrite && object.version; + if (object.id && !canSpecifyID) { throw new Error( - `Predefined IDs are not allowed for encrypted saved objects of type "${object.type}".` + 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } From a2dd90fc98ee1d0671936b1ae6f7ed30494f88f8 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Fri, 27 Nov 2020 00:58:07 +0000 Subject: [PATCH 07/12] Added suggestions from code review --- ...lugin-core-server.generatesavedobjectid.md | 17 --- .../core/server/kibana-plugin-core-server.md | 1 - ...er.savedobjectsserializer.generaterawid.md | 2 +- ...ore-server.savedobjectsutils.generateid.md | 17 +++ ...ore-server.savedobjectsutils.israndomid.md | 24 ++++ ...na-plugin-core-server.savedobjectsutils.md | 7 + src/core/server/index.ts | 1 - src/core/server/saved_objects/index.ts | 1 - .../saved_objects/serialization/index.ts | 2 +- .../serialization/serializer.test.ts | 133 +----------------- .../saved_objects/serialization/serializer.ts | 14 +- .../saved_objects/serialization/types.ts | 2 +- .../service/lib/repository.test.js | 11 +- .../saved_objects/service/lib/repository.ts | 7 +- .../saved_objects/service/lib/utils.test.ts | 29 +++- .../server/saved_objects/service/lib/utils.ts | 19 +++ src/core/server/server.api.md | 7 +- .../actions/server/actions_client.test.ts | 14 +- .../plugins/actions/server/actions_client.ts | 4 +- .../actions/server/saved_objects/index.ts | 1 - .../server/alerts_client/alerts_client.ts | 4 +- .../server/alerts_client/tests/create.test.ts | 16 ++- .../alerts/server/saved_objects/index.ts | 1 - .../encrypted_saved_objects_client_wrapper.ts | 16 ++- x-pack/plugins/lens/server/migrations.test.ts | 5 + .../secure_saved_objects_client_wrapper.ts | 8 +- .../policy/migrations/to_v7_11.0.test.ts | 4 + 27 files changed, 155 insertions(+), 212 deletions(-) delete mode 100644 docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.generateid.md create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.israndomid.md diff --git a/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md b/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md deleted file mode 100644 index 6c43d934062fb4..00000000000000 --- a/docs/development/core/server/kibana-plugin-core-server.generatesavedobjectid.md +++ /dev/null @@ -1,17 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [generateSavedObjectId](./kibana-plugin-core-server.generatesavedobjectid.md) - -## generateSavedObjectId() function - -Generates a random id for saved objects. - -Signature: - -```typescript -export declare function generateSavedObjectId(): string; -``` -Returns: - -`string` - diff --git a/docs/development/core/server/kibana-plugin-core-server.md b/docs/development/core/server/kibana-plugin-core-server.md index 5ecefcbbb8b989..adbb2460dc80aa 100644 --- a/docs/development/core/server/kibana-plugin-core-server.md +++ b/docs/development/core/server/kibana-plugin-core-server.md @@ -43,7 +43,6 @@ The plugin integrates with the core system via lifecycle events: `setup` | Function | Description | | --- | --- | | [exportSavedObjectsToStream({ types, hasReference, objects, search, savedObjectsClient, exportSizeLimit, includeReferencesDeep, excludeExportDetails, namespace, })](./kibana-plugin-core-server.exportsavedobjectstostream.md) | Generates sorted saved object stream to be used for export. See the [options](./kibana-plugin-core-server.savedobjectsexportoptions.md) for more detailed information. | -| [generateSavedObjectId()](./kibana-plugin-core-server.generatesavedobjectid.md) | Generates a random id for saved objects. | | [importSavedObjectsFromStream({ readStream, objectLimit, overwrite, createNewCopies, savedObjectsClient, typeRegistry, namespace, })](./kibana-plugin-core-server.importsavedobjectsfromstream.md) | Import saved objects from given stream. See the [options](./kibana-plugin-core-server.savedobjectsimportoptions.md) for more detailed information. | | [resolveSavedObjectsImportErrors({ readStream, objectLimit, retries, savedObjectsClient, typeRegistry, namespace, createNewCopies, })](./kibana-plugin-core-server.resolvesavedobjectsimporterrors.md) | Resolve and return saved object import errors. See the [options](./kibana-plugin-core-server.savedobjectsresolveimporterrorsoptions.md) for more detailed informations. | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsserializer.generaterawid.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsserializer.generaterawid.md index e86d7cbb36435a..a9dfd84cf0b427 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsserializer.generaterawid.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsserializer.generaterawid.md @@ -9,7 +9,7 @@ Given a saved object type and id, generates the compound id that is stored in th Signature: ```typescript -generateRawId(namespace: string | undefined, type: string, id?: string): string; +generateRawId(namespace: string | undefined, type: string, id: string): string; ``` ## Parameters diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.generateid.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.generateid.md new file mode 100644 index 00000000000000..f0951844849927 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.generateid.md @@ -0,0 +1,17 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsUtils](./kibana-plugin-core-server.savedobjectsutils.md) > [generateId](./kibana-plugin-core-server.savedobjectsutils.generateid.md) + +## SavedObjectsUtils.generateId() method + +Generates a random ID for a saved objects. + +Signature: + +```typescript +static generateId(): string; +``` +Returns: + +`string` + diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.israndomid.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.israndomid.md new file mode 100644 index 00000000000000..7bfb1bcbd8cd79 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.israndomid.md @@ -0,0 +1,24 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsUtils](./kibana-plugin-core-server.savedobjectsutils.md) > [isRandomId](./kibana-plugin-core-server.savedobjectsutils.israndomid.md) + +## SavedObjectsUtils.isRandomId() method + +Validates that a saved object ID matches UUID format. + +Signature: + +```typescript +static isRandomId(id: string | undefined): boolean; +``` + +## Parameters + +| Parameter | Type | Description | +| --- | --- | --- | +| id | string | undefined | | + +Returns: + +`boolean` + diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.md index 83831f65bd41a7..7b774e14b640f0 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsutils.md @@ -19,3 +19,10 @@ export declare class SavedObjectsUtils | [namespaceIdToString](./kibana-plugin-core-server.savedobjectsutils.namespaceidtostring.md) | static | (namespace?: string | undefined) => string | Converts a given saved object namespace ID to its string representation. All namespace IDs have an identical string representation, with the exception of the undefined namespace ID (which has a namespace string of 'default'). | | [namespaceStringToId](./kibana-plugin-core-server.savedobjectsutils.namespacestringtoid.md) | static | (namespace: string) => string | undefined | Converts a given saved object namespace string to its ID representation. All namespace strings have an identical ID representation, with the exception of the 'default' namespace string (which has a namespace ID of undefined). | +## Methods + +| Method | Modifiers | Description | +| --- | --- | --- | +| [generateId()](./kibana-plugin-core-server.savedobjectsutils.generateid.md) | static | Generates a random ID for a saved objects. | +| [isRandomId(id)](./kibana-plugin-core-server.savedobjectsutils.israndomid.md) | static | Validates that a saved object ID matches UUID format. | + diff --git a/src/core/server/index.ts b/src/core/server/index.ts index c01874c218f3d1..7b19c3a6867579 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -311,7 +311,6 @@ export { exportSavedObjectsToStream, importSavedObjectsFromStream, resolveSavedObjectsImportErrors, - generateSavedObjectId, } from './saved_objects'; export { diff --git a/src/core/server/saved_objects/index.ts b/src/core/server/saved_objects/index.ts index 8e9309ae77299b..f2bae29c4743b5 100644 --- a/src/core/server/saved_objects/index.ts +++ b/src/core/server/saved_objects/index.ts @@ -32,7 +32,6 @@ export { SavedObjectsRawDoc, SavedObjectSanitizedDoc, SavedObjectUnsanitizedDoc, - generateSavedObjectId, } from './serialization'; export { SavedObjectsMigrationLogger } from './migrations/core/migration_logger'; diff --git a/src/core/server/saved_objects/serialization/index.ts b/src/core/server/saved_objects/serialization/index.ts index 3d57bf2dd04722..812a0770ad988a 100644 --- a/src/core/server/saved_objects/serialization/index.ts +++ b/src/core/server/saved_objects/serialization/index.ts @@ -28,4 +28,4 @@ export { SavedObjectsRawDoc, SavedObjectsRawDocSource, } from './types'; -export { SavedObjectsSerializer, generateSavedObjectId } from './serializer'; +export { SavedObjectsSerializer } from './serializer'; diff --git a/src/core/server/saved_objects/serialization/serializer.test.ts b/src/core/server/saved_objects/serialization/serializer.test.ts index e5f0e8abd3b715..561f9bc001e30a 100644 --- a/src/core/server/saved_objects/serialization/serializer.test.ts +++ b/src/core/server/saved_objects/serialization/serializer.test.ts @@ -573,24 +573,10 @@ describe('#savedObjectToRaw', () => { }); describe('single-namespace type without a namespace', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = singleNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - attributes: { bar: true }, - } as any); - - const v2 = singleNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`doesn't specify _source.namespace`, () => { const actual = singleNamespaceSerializer.savedObjectToRaw({ type: '', + id: 'mock-saved-object-id', attributes: {}, } as any); @@ -599,23 +585,6 @@ describe('#savedObjectToRaw', () => { }); describe('single-namespace type with a namespace', () => { - test('generates an id prefixed with namespace and type, if no id is specified', () => { - const v1 = singleNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - const v2 = singleNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^bar\:foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`it copies namespace to _source.namespace`, () => { const actual = singleNamespaceSerializer.savedObjectToRaw({ type: 'foo', @@ -628,23 +597,6 @@ describe('#savedObjectToRaw', () => { }); describe('single-namespace type with namespaces', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - const v2 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`doesn't specify _source.namespaces`, () => { const actual = namespaceAgnosticSerializer.savedObjectToRaw({ type: 'foo', @@ -657,23 +609,6 @@ describe('#savedObjectToRaw', () => { }); describe('namespace-agnostic type with a namespace', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - const v2 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`doesn't specify _source.namespace`, () => { const actual = namespaceAgnosticSerializer.savedObjectToRaw({ type: 'foo', @@ -686,23 +621,6 @@ describe('#savedObjectToRaw', () => { }); describe('namespace-agnostic type with namespaces', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - const v2 = namespaceAgnosticSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`doesn't specify _source.namespaces`, () => { const actual = namespaceAgnosticSerializer.savedObjectToRaw({ type: 'foo', @@ -715,23 +633,6 @@ describe('#savedObjectToRaw', () => { }); describe('multi-namespace type with a namespace', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = multiNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - const v2 = multiNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespace: 'bar', - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`doesn't specify _source.namespace`, () => { const actual = multiNamespaceSerializer.savedObjectToRaw({ type: 'foo', @@ -744,23 +645,6 @@ describe('#savedObjectToRaw', () => { }); describe('multi-namespace type with namespaces', () => { - test('generates an id prefixed with type, if no id is specified', () => { - const v1 = multiNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - const v2 = multiNamespaceSerializer.savedObjectToRaw({ - type: 'foo', - namespaces: ['bar'], - attributes: { bar: true }, - } as any); - - expect(v1._id).toMatch(/^foo\:[\w-]+$/); - expect(v1._id).not.toEqual(v2._id); - }); - test(`it copies namespaces to _source.namespaces`, () => { const actual = multiNamespaceSerializer.savedObjectToRaw({ type: 'foo', @@ -1064,11 +948,6 @@ describe('#isRawSavedObject', () => { describe('#generateRawId', () => { describe('single-namespace type without a namespace', () => { - test('generates an id if none is specified', () => { - const id = singleNamespaceSerializer.generateRawId('', 'goodbye'); - expect(id).toMatch(/^goodbye\:[\w-]+$/); - }); - test('uses the id that is specified', () => { const id = singleNamespaceSerializer.generateRawId('', 'hello', 'world'); expect(id).toEqual('hello:world'); @@ -1076,11 +955,6 @@ describe('#generateRawId', () => { }); describe('single-namespace type with a namespace', () => { - test('generates an id if none is specified and prefixes namespace', () => { - const id = singleNamespaceSerializer.generateRawId('foo', 'goodbye'); - expect(id).toMatch(/^foo:goodbye\:[\w-]+$/); - }); - test('uses the id that is specified and prefixes the namespace', () => { const id = singleNamespaceSerializer.generateRawId('foo', 'hello', 'world'); expect(id).toEqual('foo:hello:world'); @@ -1088,11 +962,6 @@ describe('#generateRawId', () => { }); describe('namespace-agnostic type with a namespace', () => { - test(`generates an id if none is specified and doesn't prefix namespace`, () => { - const id = namespaceAgnosticSerializer.generateRawId('foo', 'goodbye'); - expect(id).toMatch(/^goodbye\:[\w-]+$/); - }); - test(`uses the id that is specified and doesn't prefix the namespace`, () => { const id = namespaceAgnosticSerializer.generateRawId('foo', 'hello', 'world'); expect(id).toEqual('hello:world'); diff --git a/src/core/server/saved_objects/serialization/serializer.ts b/src/core/server/saved_objects/serialization/serializer.ts index c26ccc4fd0ec6d..82999eeceb8875 100644 --- a/src/core/server/saved_objects/serialization/serializer.ts +++ b/src/core/server/saved_objects/serialization/serializer.ts @@ -17,7 +17,6 @@ * under the License. */ -import uuid from 'uuid'; import { decodeVersion, encodeVersion } from '../version'; import { ISavedObjectTypeRegistry } from '../saved_objects_type_registry'; import { SavedObjectsRawDoc, SavedObjectSanitizedDoc } from './types'; @@ -127,10 +126,10 @@ export class SavedObjectsSerializer { * @param {string} type - The saved object type * @param {string} id - The id of the saved object */ - public generateRawId(namespace: string | undefined, type: string, id?: string) { + public generateRawId(namespace: string | undefined, type: string, id: string) { const namespacePrefix = namespace && this.registry.isSingleNamespace(type) ? `${namespace}:` : ''; - return `${namespacePrefix}${type}:${id || generateSavedObjectId()}`; + return `${namespacePrefix}${type}:${id}`; } private trimIdPrefix(namespace: string | undefined, type: string, id: string) { @@ -149,15 +148,6 @@ export class SavedObjectsSerializer { } } -/** - * Generates a random id for saved objects. - * - * @public - */ -export function generateSavedObjectId() { - return uuid.v1(); -} - function assertNonEmptyString(value: string, name: string) { if (!value || typeof value !== 'string') { throw new TypeError(`Expected "${value}" to be a ${name}`); diff --git a/src/core/server/saved_objects/serialization/types.ts b/src/core/server/saved_objects/serialization/types.ts index 8b3eebceb2c5a8..e59b1a68e1ad1b 100644 --- a/src/core/server/saved_objects/serialization/types.ts +++ b/src/core/server/saved_objects/serialization/types.ts @@ -50,7 +50,7 @@ export interface SavedObjectsRawDocSource { */ interface SavedObjectDoc { attributes: T; - id?: string; // NOTE: SavedObjectDoc is used for uncreated objects where `id` is optional + id: string; type: string; namespace?: string; namespaces?: string[]; diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 6f885f17fd82b2..c74049c8734551 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -1831,21 +1831,16 @@ describe('SavedObjectsRepository', () => { }; describe('client calls', () => { - it(`should use the ES create action if ID is undefined and overwrite=true`, async () => { + it(`should use the ES index action if overwrite=true`, async () => { await createSuccess(type, attributes, { overwrite: true }); - expect(client.create).toHaveBeenCalled(); + expect(client.index).toHaveBeenCalled(); }); - it(`should use the ES create action if ID is undefined and overwrite=false`, async () => { + it(`should use the ES create action if overwrite=false`, async () => { await createSuccess(type, attributes); expect(client.create).toHaveBeenCalled(); }); - it(`should use the ES index action if ID is defined and overwrite=true`, async () => { - await createSuccess(type, attributes, { id, overwrite: true }); - expect(client.index).toHaveBeenCalled(); - }); - it(`should use the ES index with version if ID and version are defined and overwrite=true`, async () => { await createSuccess(type, attributes, { id, overwrite: true, version: mockVersion }); expect(client.index).toHaveBeenCalled(); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index d362c02de49153..9c61d6aa111ef3 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -18,7 +18,6 @@ */ import { omit } from 'lodash'; -import uuid from 'uuid'; import { ElasticsearchClient, DeleteDocumentResponse, @@ -226,7 +225,7 @@ export class SavedObjectsRepository { options: SavedObjectsCreateOptions = {} ): Promise> { const { - id, + id = SavedObjectsUtils.generateId(), migrationVersion, overwrite = false, references = [], @@ -347,7 +346,9 @@ export class SavedObjectsRepository { const method = object.id && overwrite ? 'index' : 'create'; const requiresNamespacesCheck = object.id && this._registry.isMultiNamespace(object.type); - if (object.id == null) object.id = uuid.v1(); + if (object.id == null) { + object.id = SavedObjectsUtils.generateId(); + } return { tag: 'Right' as 'Right', diff --git a/src/core/server/saved_objects/service/lib/utils.test.ts b/src/core/server/saved_objects/service/lib/utils.test.ts index ac06ca92757831..062a68e2dca28a 100644 --- a/src/core/server/saved_objects/service/lib/utils.test.ts +++ b/src/core/server/saved_objects/service/lib/utils.test.ts @@ -17,11 +17,22 @@ * under the License. */ +import uuid from 'uuid'; import { SavedObjectsFindOptions } from '../../types'; import { SavedObjectsUtils } from './utils'; +jest.mock('uuid', () => ({ + v1: jest.fn().mockReturnValue('mock-uuid'), +})); + describe('SavedObjectsUtils', () => { - const { namespaceIdToString, namespaceStringToId, createEmptyFindResponse } = SavedObjectsUtils; + const { + namespaceIdToString, + namespaceStringToId, + createEmptyFindResponse, + generateId, + isRandomId, + } = SavedObjectsUtils; describe('#namespaceIdToString', () => { it('converts `undefined` to default namespace string', () => { @@ -77,4 +88,20 @@ describe('SavedObjectsUtils', () => { expect(createEmptyFindResponse(options).per_page).toEqual(42); }); }); + + describe('#generateId', () => { + it('returns a valid uuid', () => { + expect(generateId()).toBe('mock-uuid'); + expect(uuid.v1).toHaveBeenCalled(); + }); + }); + + describe('#isRandomId', () => { + it('validates uuid correctly', () => { + expect(isRandomId('c4d82f66-3046-11eb-adc1-0242ac120002')).toBe(true); + expect(isRandomId('invalid')).toBe(false); + expect(isRandomId('')).toBe(false); + expect(isRandomId(undefined)).toBe(false); + }); + }); }); diff --git a/src/core/server/saved_objects/service/lib/utils.ts b/src/core/server/saved_objects/service/lib/utils.ts index 69abc370892184..e61f6d9224b92a 100644 --- a/src/core/server/saved_objects/service/lib/utils.ts +++ b/src/core/server/saved_objects/service/lib/utils.ts @@ -17,6 +17,7 @@ * under the License. */ +import uuid from 'uuid'; import { SavedObjectsFindOptions } from '../../types'; import { SavedObjectsFindResponse } from '..'; @@ -24,6 +25,7 @@ export const DEFAULT_NAMESPACE_STRING = 'default'; export const ALL_NAMESPACES_STRING = '*'; export const FIND_DEFAULT_PAGE = 1; export const FIND_DEFAULT_PER_PAGE = 20; +const UUID_REGEX = /^(?:[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}|00000000-0000-0000-0000-000000000000)$/i; /** * @public @@ -69,4 +71,21 @@ export class SavedObjectsUtils { total: 0, saved_objects: [], }); + + /** + * Generates a random ID for a saved objects. + */ + public static generateId() { + return uuid.v1(); + } + + /** + * Validates that a saved object ID matches UUID format. + * + * @param {string} id The ID of a saved object. + * @todo Use `uuid.validate` once upgraded to v5.3+ + */ + public static isRandomId(id: string | undefined) { + return typeof id === 'string' && UUID_REGEX.test(id); + } } diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index a63d0b5261c980..8bafd43ec21e5f 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -738,9 +738,6 @@ export interface FakeRequest { headers: Headers; } -// @public -export function generateSavedObjectId(): string; - // @public export type GetAuthHeaders = (request: KibanaRequest | LegacyRequest) => AuthHeaders | undefined; @@ -2473,7 +2470,7 @@ export interface SavedObjectsResolveImportErrorsOptions { export class SavedObjectsSerializer { // @internal constructor(registry: ISavedObjectTypeRegistry); - generateRawId(namespace: string | undefined, type: string, id?: string): string; + generateRawId(namespace: string | undefined, type: string, id: string): string; isRawSavedObject(rawDoc: SavedObjectsRawDoc): boolean; rawToSavedObject(doc: SavedObjectsRawDoc): SavedObjectSanitizedDoc; savedObjectToRaw(savedObj: SavedObjectSanitizedDoc): SavedObjectsRawDoc; @@ -2555,6 +2552,8 @@ export interface SavedObjectsUpdateResponse extends Omit({ page, perPage, }: SavedObjectsFindOptions) => SavedObjectsFindResponse; + static generateId(): string; + static isRandomId(id: string | undefined): boolean; static namespaceIdToString: (namespace?: string | undefined) => string; static namespaceStringToId: (namespace: string) => string | undefined; } diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index c4736dc4157e1e..8b6c25e1c3f241 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -27,8 +27,10 @@ import uuid from 'uuid'; import { ActionsAuthorization } from './authorization/actions_authorization'; import { actionsAuthorizationMock } from './authorization/actions_authorization.mock'; -jest.mock('../../../../src/core/server/saved_objects/serialization/serializer', () => ({ - generateSavedObjectId: () => 'GENERATED_ID', +jest.mock('../../../../src/core/server/saved_objects/service/lib/utils', () => ({ + SavedObjectsUtils: { + generateId: () => 'mock-saved-object-id', + }, })); const defaultKibanaIndex = '.kibana'; @@ -182,7 +184,7 @@ describe('create()', () => { action: 'connector_create', outcome: 'unknown', }), - kibana: { saved_object: { id: 'GENERATED_ID', type: 'action' } }, + kibana: { saved_object: { id: 'mock-saved-object-id', type: 'action' } }, }) ); }); @@ -225,7 +227,7 @@ describe('create()', () => { }), kibana: { saved_object: { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'action', }, }, @@ -282,7 +284,7 @@ describe('create()', () => { "secrets": Object {}, }, Object { - "id": "GENERATED_ID", + "id": "mock-saved-object-id", }, ] `); @@ -389,7 +391,7 @@ describe('create()', () => { "secrets": Object {}, }, Object { - "id": "GENERATED_ID", + "id": "mock-saved-object-id", }, ] `); diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index ff5b74e1ab341b..6917c41af84d89 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -13,7 +13,7 @@ import { SavedObjectAttributes, SavedObject, KibanaRequest, - generateSavedObjectId, + SavedObjectsUtils, } from '../../../../src/core/server'; import { AuditLogger, EventOutcome } from '../../security/server'; import { ActionType } from '../common'; @@ -118,7 +118,7 @@ export class ActionsClient { public async create({ action: { actionTypeId, name, config, secrets }, }: CreateOptions): Promise { - const id = generateSavedObjectId(); + const id = SavedObjectsUtils.generateId(); try { await this.authorization.ensureAuthorized('create', actionTypeId); diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index fe722e89ded06f..db811e7cd3a4f4 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -33,7 +33,6 @@ export function setupSavedObjects( type: ACTION_SAVED_OBJECT_TYPE, attributesToEncrypt: new Set(['secrets']), attributesToExcludeFromAAD: new Set(['name']), - allowPredefinedID: true, }); savedObjects.registerType({ diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index 3dbae7a38842be..f8beb09ce7ab5f 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -13,7 +13,7 @@ import { SavedObjectReference, SavedObject, PluginInitializerContext, - generateSavedObjectId, + SavedObjectsUtils, } from '../../../../../src/core/server'; import { esKuery } from '../../../../../src/plugins/data/server'; import { ActionsClient, ActionsAuthorization } from '../../../actions/server'; @@ -217,7 +217,7 @@ export class AlertsClient { } public async create({ data, options }: CreateOptions): Promise { - const id = generateSavedObjectId(); + const id = SavedObjectsUtils.generateId(); try { await this.authorization.ensureAuthorized( diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts index f2e571dc602fe8..523f0a3be2204b 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts @@ -18,8 +18,10 @@ import { auditServiceMock } from '../../../../security/server/audit/index.mock'; import { httpServerMock } from '../../../../../../src/core/server/mocks'; import { getBeforeSetup, setGlobalDate } from './lib'; -jest.mock('../../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ - generateSavedObjectId: () => 'GENERATED_ID', +jest.mock('../../../../../../src/core/server/saved_objects/service/lib/utils', () => ({ + SavedObjectsUtils: { + generateId: () => 'mock-saved-object-id', + }, })); const taskManager = taskManagerMock.createStart(); @@ -212,7 +214,7 @@ describe('create()', () => { action: 'alert_rule_create', outcome: 'unknown', }), - kibana: { saved_object: { id: 'GENERATED_ID', type: 'alert' } }, + kibana: { saved_object: { id: 'mock-saved-object-id', type: 'alert' } }, }) ); }); @@ -236,7 +238,7 @@ describe('create()', () => { }), kibana: { saved_object: { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'alert', }, }, @@ -402,7 +404,7 @@ describe('create()', () => { `); expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` Object { - "id": "GENERATED_ID", + "id": "mock-saved-object-id", "references": Array [ Object { "id": "1", @@ -1055,7 +1057,7 @@ describe('create()', () => { }, }, { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', references: [ { id: '1', @@ -1178,7 +1180,7 @@ describe('create()', () => { }, }, { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', references: [ { id: '1', diff --git a/x-pack/plugins/alerts/server/saved_objects/index.ts b/x-pack/plugins/alerts/server/saved_objects/index.ts index b2ef38e2fa8909..dfe122f56bc485 100644 --- a/x-pack/plugins/alerts/server/saved_objects/index.ts +++ b/x-pack/plugins/alerts/server/saved_objects/index.ts @@ -65,7 +65,6 @@ export function setupSavedObjects( type: 'alert', attributesToEncrypt: new Set(['apiKey']), attributesToExcludeFromAAD: new Set(AlertAttributesExcludedFromAAD), - allowPredefinedID: true, }); // Encrypted attributes diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index e2984f7e7c34f0..85dfded9cf8bfb 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -24,7 +24,7 @@ import { SavedObjectsRemoveReferencesToOptions, ISavedObjectTypeRegistry, SavedObjectsRemoveReferencesToResponse, - generateSavedObjectId, + SavedObjectsUtils, } from '../../../../../src/core/server'; import { AuthenticatedUser } from '../../../security/common/model'; import { EncryptedSavedObjectsService } from '../crypto'; @@ -66,14 +66,15 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // only allow a specified ID if we're overwriting an existing ESO with a Version // this helps us ensure that the document really was previously created using ESO // and not being used to get around the specified ID limitation - const canSpecifyID = options.overwrite && options.version; + const canSpecifyID = + (options.overwrite && options.version) || SavedObjectsUtils.isRandomId(options.id); if (options.id && !canSpecifyID) { throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' ); } - const id = options.id ?? generateSavedObjectId(); + const id = options.id ?? SavedObjectsUtils.generateId(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -110,14 +111,15 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. - const canSpecifyID = options?.overwrite && object.version; + const canSpecifyID = + (options?.overwrite && object.version) || SavedObjectsUtils.isRandomId(object.id); if (object.id && !canSpecifyID) { throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `.generateId`.' ); } - const id = object.id ?? generateSavedObjectId(); + const id = object.id ?? SavedObjectsUtils.generateId(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, diff --git a/x-pack/plugins/lens/server/migrations.test.ts b/x-pack/plugins/lens/server/migrations.test.ts index 957da5cbb37431..9764926fc03fc4 100644 --- a/x-pack/plugins/lens/server/migrations.test.ts +++ b/x-pack/plugins/lens/server/migrations.test.ts @@ -13,6 +13,7 @@ describe('Lens migrations', () => { const example = { type: 'lens', + id: 'mock-saved-object-id', attributes: { expression: 'kibana\n| kibana_context query="{\\"language\\":\\"kuery\\",\\"query\\":\\"\\"}" \n| lens_merge_tables layerIds="c61a8afb-a185-4fae-a064-fb3846f6c451" \n tables={esaggs index="logstash-*" metricsAtAllLevels=false partialRows=false includeFormatHints=true aggConfigs="[{\\"id\\":\\"2cd09808-3915-49f4-b3b0-82767eba23f7\\",\\"enabled\\":true,\\"type\\":\\"max\\",\\"schema\\":\\"metric\\",\\"params\\":{\\"field\\":\\"bytes\\"}}]" | lens_rename_columns idMap="{\\"col-0-2cd09808-3915-49f4-b3b0-82767eba23f7\\":\\"2cd09808-3915-49f4-b3b0-82767eba23f7\\"}"}\n| lens_metric_chart title="Maximum of bytes" accessor="2cd09808-3915-49f4-b3b0-82767eba23f7"', @@ -164,6 +165,7 @@ describe('Lens migrations', () => { const example = { type: 'lens', + id: 'mock-saved-object-id', attributes: { expression: `kibana | kibana_context query="{\\"query\\":\\"\\",\\"language\\":\\"kuery\\"}" filters="[]" @@ -265,6 +267,7 @@ describe('Lens migrations', () => { it('should handle pre-migrated expression', () => { const input = { type: 'lens', + id: 'mock-saved-object-id', attributes: { ...example.attributes, expression: `kibana @@ -283,6 +286,7 @@ describe('Lens migrations', () => { const context = {} as SavedObjectMigrationContext; const example = { + id: 'mock-saved-object-id', attributes: { description: '', expression: @@ -513,6 +517,7 @@ describe('Lens migrations', () => { const example = { type: 'lens', + id: 'mock-saved-object-id', attributes: { state: { datasourceStates: { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index c1aa3bd614c930..4b97dcfe313bdd 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -18,7 +18,6 @@ import { SavedObjectsRemoveReferencesToOptions, SavedObjectsUpdateOptions, SavedObjectsUtils, - generateSavedObjectId, } from '../../../../../src/core/server'; import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants'; import { @@ -97,7 +96,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra attributes: T = {} as T, options: SavedObjectsCreateOptions = {} ) { - const optionsWithId = { ...options, id: options.id ?? generateSavedObjectId() }; + const optionsWithId = { ...options, id: options.id ?? SavedObjectsUtils.generateId() }; const namespaces = [optionsWithId.namespace, ...(optionsWithId.initialNamespaces || [])]; try { const args = { type, attributes, options: optionsWithId }; @@ -143,7 +142,10 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra objects: Array>, options: SavedObjectsBaseOptions = {} ) { - const objectsWithId = objects.map((obj) => ({ ...obj, id: obj.id ?? generateSavedObjectId() })); + const objectsWithId = objects.map((obj) => ({ + ...obj, + id: obj.id ?? SavedObjectsUtils.generateId(), + })); const namespaces = objectsWithId.reduce( (acc, { initialNamespaces = [] }) => acc.concat(initialNamespaces), [options.namespace] diff --git a/x-pack/plugins/security_solution/common/endpoint/policy/migrations/to_v7_11.0.test.ts b/x-pack/plugins/security_solution/common/endpoint/policy/migrations/to_v7_11.0.test.ts index 6d3e320fba3af7..809e8b91e18f44 100644 --- a/x-pack/plugins/security_solution/common/endpoint/policy/migrations/to_v7_11.0.test.ts +++ b/x-pack/plugins/security_solution/common/endpoint/policy/migrations/to_v7_11.0.test.ts @@ -12,6 +12,7 @@ describe('7.11.0 Endpoint Package Policy migration', () => { const migration = migratePackagePolicyToV7110; it('adds malware notification checkbox and optional message', () => { const doc: SavedObjectUnsanitizedDoc = { + id: 'mock-saved-object-id', attributes: { name: 'Some Policy Name', package: { @@ -99,11 +100,13 @@ describe('7.11.0 Endpoint Package Policy migration', () => { ], }, type: ' nested', + id: 'mock-saved-object-id', }); }); it('does not modify non-endpoint package policies', () => { const doc: SavedObjectUnsanitizedDoc = { + id: 'mock-saved-object-id', attributes: { name: 'Some Policy Name', package: { @@ -163,6 +166,7 @@ describe('7.11.0 Endpoint Package Policy migration', () => { ], }, type: ' nested', + id: 'mock-saved-object-id', }); }); }); From d11ffc2bcb363ce9fcebff63f4b617c5a67a9edd Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Mon, 30 Nov 2020 12:12:20 +0000 Subject: [PATCH 08/12] Fixed unit tests --- ...ypted_saved_objects_client_wrapper.test.ts | 53 +++++++++++-------- .../encrypted_saved_objects_client_wrapper.ts | 2 +- ...ecure_saved_objects_client_wrapper.test.ts | 12 +++-- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 262ce45863fcd9..85ec08fb7388d0 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -13,9 +13,18 @@ import { savedObjectsClientMock, savedObjectsTypeRegistryMock } from 'src/core/s import { mockAuthenticatedUser } from '../../../security/common/model/authenticated_user.mock'; import { encryptedSavedObjectsServiceMock } from '../crypto/index.mock'; -jest.mock('../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ - generateSavedObjectId: () => 'GENERATED_ID', -})); +jest.mock('../../../../../src/core/server/saved_objects/service/lib/utils', () => { + const { SavedObjectsUtils } = jest.requireActual( + '../../../../../src/core/server/saved_objects/service/lib/utils' + ); + return { + SavedObjectsUtils: { + namespaceStringToId: SavedObjectsUtils.namespaceStringToId, + isRandomId: SavedObjectsUtils.isRandomId, + generateId: () => 'mock-saved-object-id', + }, + }; +}); let wrapper: EncryptedSavedObjectsClientWrapper; let mockBaseClient: jest.Mocked; @@ -78,7 +87,7 @@ describe('#create', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; await expect(wrapper.create('known-type', attributes, { id: 'some-id' })).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' ); expect(mockBaseClient.create).not.toHaveBeenCalled(); @@ -145,7 +154,7 @@ describe('#create', () => { }; const options = { overwrite: true }; const mockedResponse = { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'known-type', attributes: { attrOne: 'one', @@ -165,7 +174,7 @@ describe('#create', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'GENERATED_ID' }, + { type: 'known-type', id: 'mock-saved-object-id' }, { attrOne: 'one', attrSecret: 'secret', @@ -184,7 +193,7 @@ describe('#create', () => { attrNotSoSecret: '*not-so-secret*', attrThree: 'three', }, - { id: 'GENERATED_ID', overwrite: true } + { id: 'mock-saved-object-id', overwrite: true } ); }); @@ -193,7 +202,7 @@ describe('#create', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const options = { overwrite: true, namespace }; const mockedResponse = { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'known-type', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, references: [], @@ -210,7 +219,7 @@ describe('#create', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( { type: 'known-type', - id: 'GENERATED_ID', + id: 'mock-saved-object-id', namespace: expectNamespaceInDescriptor ? namespace : undefined, }, { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, @@ -221,7 +230,7 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'GENERATED_ID', overwrite: true, namespace } + { id: 'mock-saved-object-id', overwrite: true, namespace } ); }; @@ -247,7 +256,7 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'GENERATED_ID' } + { id: 'mock-saved-object-id' } ); }); }); @@ -259,7 +268,7 @@ describe('#bulkCreate', () => { const mockedResponse = { saved_objects: [ { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'known-type', attributes, references: [], @@ -292,7 +301,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'GENERATED_ID', + id: 'mock-saved-object-id', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, bulkCreateParams[1], @@ -310,7 +319,7 @@ describe('#bulkCreate', () => { ]; await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' ); expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled(); @@ -397,7 +406,7 @@ describe('#bulkCreate', () => { const mockedResponse = { saved_objects: [ { - id: 'GENERATED_ID', + id: 'mock-saved-object-id', type: 'known-type', attributes: { ...attributes, attrSecret: '*secret*', attrNotSoSecret: '*not-so-secret*' }, references: [], @@ -430,7 +439,7 @@ describe('#bulkCreate', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'GENERATED_ID' }, + { type: 'known-type', id: 'mock-saved-object-id' }, { attrOne: 'one', attrSecret: 'secret', @@ -445,7 +454,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'GENERATED_ID', + id: 'mock-saved-object-id', attributes: { attrOne: 'one', attrSecret: '*secret*', @@ -464,7 +473,9 @@ describe('#bulkCreate', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const options = { namespace }; const mockedResponse = { - saved_objects: [{ id: 'GENERATED_ID', type: 'known-type', attributes, references: [] }], + saved_objects: [ + { id: 'mock-saved-object-id', type: 'known-type', attributes, references: [] }, + ], }; mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); @@ -483,7 +494,7 @@ describe('#bulkCreate', () => { expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( { type: 'known-type', - id: 'GENERATED_ID', + id: 'mock-saved-object-id', namespace: expectNamespaceInDescriptor ? namespace : undefined, }, { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, @@ -495,7 +506,7 @@ describe('#bulkCreate', () => { [ { ...bulkCreateParams[0], - id: 'GENERATED_ID', + id: 'mock-saved-object-id', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, ], @@ -531,7 +542,7 @@ describe('#bulkCreate', () => { [ { type: 'known-type', - id: 'GENERATED_ID', + id: 'mock-saved-object-id', attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, }, ], diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 85dfded9cf8bfb..c186f3e26a82f7 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -115,7 +115,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon (options?.overwrite && object.version) || SavedObjectsUtils.isRandomId(object.id); if (object.id && !canSpecifyID) { throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `.generateId`.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' ); } diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index bf8e925a2dc36c..0fc7be4c7a5d8c 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -12,8 +12,10 @@ import { SavedObjectsClientContract } from 'kibana/server'; import { SavedObjectActions } from '../authorization/actions/saved_object'; import { AuditEvent, EventOutcome } from '../audit'; -jest.mock('../../../../../src/core/server/saved_objects/serialization/serializer', () => ({ - generateSavedObjectId: () => 'GENERATED_ID', +jest.mock('../../../../../src/core/server/saved_objects/service/lib/utils', () => ({ + SavedObjectsUtils: { + generateId: () => 'mock-saved-object-id', + }, })); let clientOpts: ReturnType; @@ -690,7 +692,7 @@ describe('#create', () => { }); test(`throws decorated ForbiddenError when unauthorized`, async () => { - const options = { id: 'GENERATED_ID', namespace }; + const options = { id: 'mock-saved-object-id', namespace }; await expectForbiddenError(client.create, { type, attributes, options }); }); @@ -698,7 +700,7 @@ describe('#create', () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.create.mockResolvedValue(apiCallReturnValue as any); - const options = { id: 'GENERATED_ID', namespace }; + const options = { id: 'mock-saved-object-id', namespace }; const result = await expectSuccess(client.create, { type, attributes, @@ -729,7 +731,7 @@ describe('#create', () => { test(`adds audit event when successful`, async () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.create.mockResolvedValue(apiCallReturnValue as any); - const options = { id: 'GENERATED_ID', namespace }; + const options = { id: 'mock-saved-object-id', namespace }; await expectSuccess(client.create, { type, attributes, options }); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type, id: expect.any(String) }); From 6e473a9960a497cfaf04bfc5ee20ff47ed23faec Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Mon, 30 Nov 2020 14:24:02 +0000 Subject: [PATCH 09/12] Added suggestions from code review --- x-pack/plugins/actions/server/lib/audit_events.ts | 4 ++-- x-pack/plugins/alerts/server/alerts_client/audit_events.ts | 4 ++-- x-pack/plugins/security/server/audit/audit_events.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/actions/server/lib/audit_events.ts b/x-pack/plugins/actions/server/lib/audit_events.ts index 481a0da090b491..5b9dc3449d2f73 100644 --- a/x-pack/plugins/actions/server/lib/audit_events.ts +++ b/x-pack/plugins/actions/server/lib/audit_events.ts @@ -38,7 +38,7 @@ const eventTypes: Record = { export interface ConnectorEventParams { action: ConnectorAction; outcome?: EventOutcome; - savedObject?: Required['kibana']>['saved_object']; + savedObject?: NonNullable['saved_object']; error?: Error; } @@ -52,7 +52,7 @@ export function connectorEvent({ const [present, progressive, past] = eventVerbs[action]; const message = error ? `Failed attempt to ${present} ${doc}` - : outcome === 'unknown' + : outcome === EventOutcome.UNKNOWN ? `User is ${progressive} ${doc}` : `User has ${past} ${doc}`; const type = eventTypes[action]; diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts index 2aa25bc455a14e..d24ce954d5a2ec 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts @@ -56,7 +56,7 @@ const eventTypes: Record = { export interface AlertRuleEventParams { action: AlertRuleAction; outcome?: EventOutcome; - savedObject?: Required['kibana']>['saved_object']; + savedObject?: NonNullable['saved_object']; error?: Error; } @@ -70,7 +70,7 @@ export function alertRuleEvent({ const [present, progressive, past] = eventVerbs[action]; const message = error ? `Failed attempt to ${present} ${doc}` - : outcome === 'unknown' + : outcome === EventOutcome.UNKNOWN ? `User is ${progressive} ${doc}` : `User has ${past} ${doc}`; const type = eventTypes[action]; diff --git a/x-pack/plugins/security/server/audit/audit_events.ts b/x-pack/plugins/security/server/audit/audit_events.ts index 26b13222c6ff28..2e003b1d55eac5 100644 --- a/x-pack/plugins/security/server/audit/audit_events.ts +++ b/x-pack/plugins/security/server/audit/audit_events.ts @@ -209,7 +209,7 @@ const eventTypes: Record = { export interface SavedObjectEventParams { action: SavedObjectAction; outcome?: EventOutcome; - savedObject?: Required['kibana']>['saved_object']; + savedObject?: NonNullable['saved_object']; addToSpaces?: readonly string[]; deleteFromSpaces?: readonly string[]; error?: Error; @@ -227,7 +227,7 @@ export function savedObjectEvent({ const [present, progressive, past] = eventVerbs[action]; const message = error ? `Failed attempt to ${present} ${doc}` - : outcome === 'unknown' + : outcome === EventOutcome.UNKNOWN ? `User is ${progressive} ${doc}` : `User has ${past} ${doc}`; const type = eventTypes[action]; From 9014744d8c7fedf6cab24dd8a3fac077441e1f18 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Tue, 1 Dec 2020 17:29:57 +0000 Subject: [PATCH 10/12] Changed names of alert events --- docs/user/security/audit-logging.asciidoc | 20 +++--- .../server/alerts_client/audit_events.test.ts | 12 ++-- .../server/alerts_client/audit_events.ts | 62 +++++++++---------- .../server/alerts_client/tests/create.test.ts | 4 +- .../server/alerts_client/tests/delete.test.ts | 4 +- .../alerts_client/tests/disable.test.ts | 4 +- .../server/alerts_client/tests/enable.test.ts | 4 +- .../server/alerts_client/tests/find.test.ts | 6 +- .../server/alerts_client/tests/get.test.ts | 4 +- .../alerts_client/tests/mute_all.test.ts | 4 +- .../alerts_client/tests/unmute_all.test.ts | 4 +- .../server/alerts_client/tests/update.test.ts | 4 +- .../tests/update_api_key.test.ts | 4 +- .../__snapshots__/migrations.test.ts.snap | 1 + ...ecure_saved_objects_client_wrapper.test.ts | 16 +++-- 15 files changed, 80 insertions(+), 73 deletions(-) diff --git a/docs/user/security/audit-logging.asciidoc b/docs/user/security/audit-logging.asciidoc index 45569ad9658c43..4b3512ae3056b5 100644 --- a/docs/user/security/audit-logging.asciidoc +++ b/docs/user/security/audit-logging.asciidoc @@ -88,7 +88,7 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is creating a connector. | `failure` | User is not authorized to create a connector. -.2+| `alert_rule_create` +.2+| `alert_create` | `unknown` | User is creating an alert rule. | `failure` | User is not authorized to create an alert rule. @@ -120,27 +120,27 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is updating a connector. | `failure` | User is not authorized to update a connector. -.2+| `alert_rule_update` +.2+| `alert_update` | `unknown` | User is updating an alert rule. | `failure` | User is not authorized to update an alert rule. -.2+| `alert_rule_update_api_key` +.2+| `alert_update_api_key` | `unknown` | User is updating the API key of an alert rule. | `failure` | User is not authorized to update the API key of an alert rule. -.2+| `alert_rule_enable` +.2+| `alert_enable` | `unknown` | User is enabling an alert rule. | `failure` | User is not authorized to enable an alert rule. -.2+| `alert_rule_disable` +.2+| `alert_disable` | `unknown` | User is disabling an alert rule. | `failure` | User is not authorized to disable an alert rule. -.2+| `alert_rule_mute` +.2+| `alert_mute` | `unknown` | User is muting an alert rule. | `failure` | User is not authorized to mute an alert rule. -.2+| `alert_rule_unmute` +.2+| `alert_unmute` | `unknown` | User is unmuting an alert rule. | `failure` | User is not authorized to unmute an alert rule. @@ -168,7 +168,7 @@ Refer to the corresponding {es} logs for potential write errors. | `unknown` | User is deleting a connector. | `failure` | User is not authorized to delete a connector. -.2+| `alert_rule_delete` +.2+| `alert_delete` | `unknown` | User is deleting an alert rule. | `failure` | User is not authorized to delete an alert rule. @@ -195,11 +195,11 @@ Refer to the corresponding {es} logs for potential write errors. | `success` | User has accessed a connector as part of a search operation. | `failure` | User is not authorized to search for connectors. -.2+| `alert_rule_get` +.2+| `alert_get` | `success` | User has accessed an alert rule. | `failure` | User is not authorized to access an alert rule. -.2+| `alert_rule_find` +.2+| `alert_find` | `success` | User has accessed an alert rule as part of a search operation. | `failure` | User is not authorized to search for alert rules. diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts index 252bab6949b840..d64a2c6bf6285a 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts @@ -19,7 +19,7 @@ describe('#alertRuleEvent', () => { Object { "error": undefined, "event": Object { - "action": "alert_rule_create", + "action": "alert_create", "category": "database", "outcome": "unknown", "type": "creation", @@ -30,7 +30,7 @@ describe('#alertRuleEvent', () => { "type": "alert", }, }, - "message": "User is creating alert rule [id=ALERT_ID]", + "message": "User is creating alert [id=ALERT_ID]", } `); }); @@ -45,7 +45,7 @@ describe('#alertRuleEvent', () => { Object { "error": undefined, "event": Object { - "action": "alert_rule_create", + "action": "alert_create", "category": "database", "outcome": "success", "type": "creation", @@ -56,7 +56,7 @@ describe('#alertRuleEvent', () => { "type": "alert", }, }, - "message": "User has created alert rule [id=ALERT_ID]", + "message": "User has created alert [id=ALERT_ID]", } `); }); @@ -75,7 +75,7 @@ describe('#alertRuleEvent', () => { "message": "ERROR_MESSAGE", }, "event": Object { - "action": "alert_rule_create", + "action": "alert_create", "category": "database", "outcome": "failure", "type": "creation", @@ -86,7 +86,7 @@ describe('#alertRuleEvent', () => { "type": "alert", }, }, - "message": "Failed attempt to create alert rule [id=ALERT_ID]", + "message": "Failed attempt to create alert [id=ALERT_ID]", } `); }); diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts index d24ce954d5a2ec..b380a74f06090f 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts @@ -7,16 +7,16 @@ import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server'; export enum AlertRuleAction { - CREATE = 'alert_rule_create', - GET = 'alert_rule_get', - UPDATE = 'alert_rule_update', - UPDATE_API_KEY = 'alert_rule_update_api_key', - ENABLE = 'alert_rule_enable', - DISABLE = 'alert_rule_disable', - DELETE = 'alert_rule_delete', - FIND = 'alert_rule_find', - MUTE = 'alert_rule_mute', - UNMUTE = 'alert_rule_unmute', + CREATE = 'alert_create', + GET = 'alert_get', + UPDATE = 'alert_update', + UPDATE_API_KEY = 'alert_update_api_key', + ENABLE = 'alert_enable', + DISABLE = 'alert_disable', + DELETE = 'alert_delete', + FIND = 'alert_find', + MUTE = 'alert_mute', + UNMUTE = 'alert_unmute', MUTE_INSTANCE = 'alert_instance_mute', UNMUTE_INSTANCE = 'alert_instance_unmute', } @@ -24,31 +24,31 @@ export enum AlertRuleAction { type VerbsTuple = [string, string, string]; const eventVerbs: Record = { - alert_rule_create: ['create', 'creating', 'created'], - alert_rule_get: ['access', 'accessing', 'accessed'], - alert_rule_update: ['update', 'updating', 'updated'], - alert_rule_update_api_key: ['update API key of', 'updating API key of', 'updated API key of'], - alert_rule_enable: ['enable', 'enabling', 'enabled'], - alert_rule_disable: ['disable', 'disabling', 'disabled'], - alert_rule_delete: ['delete', 'deleting', 'deleted'], - alert_rule_find: ['access', 'accessing', 'accessed'], - alert_rule_mute: ['mute', 'muting', 'muted'], - alert_rule_unmute: ['unmute', 'unmuting', 'unmuted'], + alert_create: ['create', 'creating', 'created'], + alert_get: ['access', 'accessing', 'accessed'], + alert_update: ['update', 'updating', 'updated'], + alert_update_api_key: ['update API key of', 'updating API key of', 'updated API key of'], + alert_enable: ['enable', 'enabling', 'enabled'], + alert_disable: ['disable', 'disabling', 'disabled'], + alert_delete: ['delete', 'deleting', 'deleted'], + alert_find: ['access', 'accessing', 'accessed'], + alert_mute: ['mute', 'muting', 'muted'], + alert_unmute: ['unmute', 'unmuting', 'unmuted'], alert_instance_mute: ['mute instance of', 'muting instance of', 'muted instance of'], alert_instance_unmute: ['unmute instance of', 'unmuting instance of', 'unmuted instance of'], }; const eventTypes: Record = { - alert_rule_create: EventType.CREATION, - alert_rule_get: EventType.ACCESS, - alert_rule_update: EventType.CHANGE, - alert_rule_update_api_key: EventType.CHANGE, - alert_rule_enable: EventType.CHANGE, - alert_rule_disable: EventType.CHANGE, - alert_rule_delete: EventType.DELETION, - alert_rule_find: EventType.ACCESS, - alert_rule_mute: EventType.CHANGE, - alert_rule_unmute: EventType.CHANGE, + alert_create: EventType.CREATION, + alert_get: EventType.ACCESS, + alert_update: EventType.CHANGE, + alert_update_api_key: EventType.CHANGE, + alert_enable: EventType.CHANGE, + alert_disable: EventType.CHANGE, + alert_delete: EventType.DELETION, + alert_find: EventType.ACCESS, + alert_mute: EventType.CHANGE, + alert_unmute: EventType.CHANGE, alert_instance_mute: EventType.CHANGE, alert_instance_unmute: EventType.CHANGE, }; @@ -66,7 +66,7 @@ export function alertRuleEvent({ outcome, error, }: AlertRuleEventParams): AuditEvent { - const doc = savedObject ? `alert rule [id=${savedObject.id}]` : 'an alert rule'; + const doc = savedObject ? `alert [id=${savedObject.id}]` : 'an alert'; const [present, progressive, past] = eventVerbs[action]; const message = error ? `Failed attempt to ${present} ${doc}` diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts index 523f0a3be2204b..f719282b427fe1 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts @@ -211,7 +211,7 @@ describe('create()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_create', + action: 'alert_create', outcome: 'unknown', }), kibana: { saved_object: { id: 'mock-saved-object-id', type: 'alert' } }, @@ -233,7 +233,7 @@ describe('create()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_create', + action: 'alert_create', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts index 0a791612d81d49..a7ef008eaa2ee7 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts @@ -251,7 +251,7 @@ describe('delete()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_delete', + action: 'alert_delete', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -266,7 +266,7 @@ describe('delete()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_delete', + action: 'alert_delete', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts index 3edeffaa9ce6e4..ce0688a5ab2ff4 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts @@ -119,7 +119,7 @@ describe('disable()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_disable', + action: 'alert_disable', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -134,7 +134,7 @@ describe('disable()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_disable', + action: 'alert_disable', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts index 936b8d076b80b8..daac6689a183b2 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/enable.test.ts @@ -158,7 +158,7 @@ describe('enable()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_enable', + action: 'alert_enable', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -173,7 +173,7 @@ describe('enable()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_enable', + action: 'alert_enable', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts index 23dc5b9a407719..410787d3c371fb 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts @@ -259,7 +259,7 @@ describe('find()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_find', + action: 'alert_find', outcome: 'success', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -275,7 +275,7 @@ describe('find()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_find', + action: 'alert_find', outcome: 'failure', }), error: { @@ -299,7 +299,7 @@ describe('find()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_find', + action: 'alert_find', outcome: 'failure', }), kibana: { saved_object: { id: '1', type: 'alert' } }, diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts index 8a377856fd6a62..32ac57459795eb 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts @@ -218,7 +218,7 @@ describe('get()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_get', + action: 'alert_get', outcome: 'success', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -234,7 +234,7 @@ describe('get()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_get', + action: 'alert_get', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts index 396e933ec08baf..b3c3e1bdd2ede4 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/mute_all.test.ts @@ -169,7 +169,7 @@ describe('muteAll()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_mute', + action: 'alert_mute', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -205,7 +205,7 @@ describe('muteAll()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_mute', + action: 'alert_mute', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts index 0315e7d42fbf71..fd0157091e3a53 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/unmute_all.test.ts @@ -169,7 +169,7 @@ describe('unmuteAll()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_unmute', + action: 'alert_unmute', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -205,7 +205,7 @@ describe('unmuteAll()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_unmute', + action: 'alert_unmute', outcome: 'failure', }), kibana: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts index 6de3929f319d01..5453ae5729ea13 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/update.test.ts @@ -1341,7 +1341,7 @@ describe('update()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_update', + action: 'alert_update', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -1371,7 +1371,7 @@ describe('update()', () => { expect.objectContaining({ event: expect.objectContaining({ outcome: 'failure', - action: 'alert_rule_update', + action: 'alert_update', }), kibana: { saved_object: { diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts index f4cf8c36c627fe..bf21256bb8413e 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/update_api_key.test.ts @@ -282,7 +282,7 @@ describe('updateApiKey()', () => { expect(auditLogger.log).toHaveBeenCalledWith( expect.objectContaining({ event: expect.objectContaining({ - action: 'alert_rule_update_api_key', + action: 'alert_update_api_key', outcome: 'unknown', }), kibana: { saved_object: { id: '1', type: 'alert' } }, @@ -298,7 +298,7 @@ describe('updateApiKey()', () => { expect.objectContaining({ event: expect.objectContaining({ outcome: 'failure', - action: 'alert_rule_update_api_key', + action: 'alert_update_api_key', }), kibana: { saved_object: { diff --git a/x-pack/plugins/lens/server/__snapshots__/migrations.test.ts.snap b/x-pack/plugins/lens/server/__snapshots__/migrations.test.ts.snap index 4979438dbd3d04..819fbd9c970ce6 100644 --- a/x-pack/plugins/lens/server/__snapshots__/migrations.test.ts.snap +++ b/x-pack/plugins/lens/server/__snapshots__/migrations.test.ts.snap @@ -156,6 +156,7 @@ Object { "title": "mylens", "visualizationType": "lnsXY", }, + "id": "mock-saved-object-id", "references": Array [ Object { "id": "ff959d40-b880-11e8-a6d9-e546fe2bba5f", diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 0fc7be4c7a5d8c..20ab98623d8738 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -12,11 +12,17 @@ import { SavedObjectsClientContract } from 'kibana/server'; import { SavedObjectActions } from '../authorization/actions/saved_object'; import { AuditEvent, EventOutcome } from '../audit'; -jest.mock('../../../../../src/core/server/saved_objects/service/lib/utils', () => ({ - SavedObjectsUtils: { - generateId: () => 'mock-saved-object-id', - }, -})); +jest.mock('../../../../../src/core/server/saved_objects/service/lib/utils', () => { + const { SavedObjectsUtils } = jest.requireActual( + '../../../../../src/core/server/saved_objects/service/lib/utils' + ); + return { + SavedObjectsUtils: { + createEmptyFindResponse: SavedObjectsUtils.createEmptyFindResponse, + generateId: () => 'mock-saved-object-id', + }, + }; +}); let clientOpts: ReturnType; let client: SecureSavedObjectsClientWrapper; From 2873fe7e1866ca6917868b9e965c9848936250cc Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Wed, 2 Dec 2020 11:24:53 +0000 Subject: [PATCH 11/12] Changed naming as suggested in code review --- .../plugins/actions/server/actions_client.ts | 54 +++++----- .../actions/server/lib/audit_events.test.ts | 16 +-- .../actions/server/lib/audit_events.ts | 14 +-- .../server/alerts_client/alerts_client.ts | 102 +++++++++--------- .../server/alerts_client/audit_events.test.ts | 16 +-- .../server/alerts_client/audit_events.ts | 14 +-- 6 files changed, 108 insertions(+), 108 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 6917c41af84d89..20fd50f92a5021 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -37,7 +37,7 @@ import { getAuthorizationModeBySource, AuthorizationMode, } from './authorization/get_authorization_mode_by_source'; -import { connectorEvent, ConnectorAction } from './lib/audit_events'; +import { connectorAuditEvent, ConnectorAuditAction } from './lib/audit_events'; // We are assuming there won't be many actions. This is why we will load // all the actions in advance and assume the total count to not go over 10000. @@ -124,8 +124,8 @@ export class ActionsClient { await this.authorization.ensureAuthorized('create', actionTypeId); } catch (error) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.CREATE, + connectorAuditEvent({ + action: ConnectorAuditAction.CREATE, savedObject: { type: 'action', id }, error, }) @@ -140,8 +140,8 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.CREATE, + connectorAuditEvent({ + action: ConnectorAuditAction.CREATE, savedObject: { type: 'action', id }, outcome: EventOutcome.UNKNOWN, }) @@ -190,8 +190,8 @@ export class ActionsClient { } } catch (error) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.UPDATE, + connectorAuditEvent({ + action: ConnectorAuditAction.UPDATE, savedObject: { type: 'action', id }, error, }) @@ -212,8 +212,8 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.UPDATE, + connectorAuditEvent({ + action: ConnectorAuditAction.UPDATE, savedObject: { type: 'action', id }, outcome: EventOutcome.UNKNOWN, }) @@ -256,8 +256,8 @@ export class ActionsClient { await this.authorization.ensureAuthorized('get'); } catch (error) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.GET, + connectorAuditEvent({ + action: ConnectorAuditAction.GET, savedObject: { type: 'action', id }, error, }) @@ -270,8 +270,8 @@ export class ActionsClient { ); if (preconfiguredActionsList !== undefined) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.GET, + connectorAuditEvent({ + action: ConnectorAuditAction.GET, savedObject: { type: 'action', id }, }) ); @@ -287,8 +287,8 @@ export class ActionsClient { const result = await this.unsecuredSavedObjectsClient.get('action', id); this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.GET, + connectorAuditEvent({ + action: ConnectorAuditAction.GET, savedObject: { type: 'action', id }, }) ); @@ -310,8 +310,8 @@ export class ActionsClient { await this.authorization.ensureAuthorized('get'); } catch (error) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.FIND, + connectorAuditEvent({ + action: ConnectorAuditAction.FIND, error, }) ); @@ -327,8 +327,8 @@ export class ActionsClient { savedObjectsActions.forEach(({ id }) => this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.FIND, + connectorAuditEvent({ + action: ConnectorAuditAction.FIND, savedObject: { type: 'action', id }, }) ) @@ -359,8 +359,8 @@ export class ActionsClient { } catch (error) { ids.forEach((id) => this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.GET, + connectorAuditEvent({ + action: ConnectorAuditAction.GET, savedObject: { type: 'action', id }, error, }) @@ -394,8 +394,8 @@ export class ActionsClient { ids.forEach((id) => this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.GET, + connectorAuditEvent({ + action: ConnectorAuditAction.GET, savedObject: { type: 'action', id }, }) ) @@ -435,8 +435,8 @@ export class ActionsClient { } } catch (error) { this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.DELETE, + connectorAuditEvent({ + action: ConnectorAuditAction.DELETE, savedObject: { type: 'action', id }, error, }) @@ -445,8 +445,8 @@ export class ActionsClient { } this.auditLogger?.log( - connectorEvent({ - action: ConnectorAction.DELETE, + connectorAuditEvent({ + action: ConnectorAuditAction.DELETE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'action', id }, }) diff --git a/x-pack/plugins/actions/server/lib/audit_events.test.ts b/x-pack/plugins/actions/server/lib/audit_events.test.ts index 37c94a8c46a47b..6c2fd99c2eafdb 100644 --- a/x-pack/plugins/actions/server/lib/audit_events.test.ts +++ b/x-pack/plugins/actions/server/lib/audit_events.test.ts @@ -5,13 +5,13 @@ */ import { EventOutcome } from '../../../security/server/audit'; -import { ConnectorAction, connectorEvent } from './audit_events'; +import { ConnectorAuditAction, connectorAuditEvent } from './audit_events'; -describe('#connectorEvent', () => { +describe('#connectorAuditEvent', () => { test('creates event with `unknown` outcome', () => { expect( - connectorEvent({ - action: ConnectorAction.CREATE, + connectorAuditEvent({ + action: ConnectorAuditAction.CREATE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'action', id: 'ACTION_ID' }, }) @@ -37,8 +37,8 @@ describe('#connectorEvent', () => { test('creates event with `success` outcome', () => { expect( - connectorEvent({ - action: ConnectorAction.CREATE, + connectorAuditEvent({ + action: ConnectorAuditAction.CREATE, savedObject: { type: 'action', id: 'ACTION_ID' }, }) ).toMatchInlineSnapshot(` @@ -63,8 +63,8 @@ describe('#connectorEvent', () => { test('creates event with `failure` outcome', () => { expect( - connectorEvent({ - action: ConnectorAction.CREATE, + connectorAuditEvent({ + action: ConnectorAuditAction.CREATE, savedObject: { type: 'action', id: 'ACTION_ID' }, error: new Error('ERROR_MESSAGE'), }) diff --git a/x-pack/plugins/actions/server/lib/audit_events.ts b/x-pack/plugins/actions/server/lib/audit_events.ts index 5b9dc3449d2f73..7d25b5c0cd479a 100644 --- a/x-pack/plugins/actions/server/lib/audit_events.ts +++ b/x-pack/plugins/actions/server/lib/audit_events.ts @@ -6,7 +6,7 @@ import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server'; -export enum ConnectorAction { +export enum ConnectorAuditAction { CREATE = 'connector_create', GET = 'connector_get', UPDATE = 'connector_update', @@ -17,7 +17,7 @@ export enum ConnectorAction { type VerbsTuple = [string, string, string]; -const eventVerbs: Record = { +const eventVerbs: Record = { connector_create: ['create', 'creating', 'created'], connector_get: ['access', 'accessing', 'accessed'], connector_update: ['update', 'updating', 'updated'], @@ -26,7 +26,7 @@ const eventVerbs: Record = { connector_execute: ['execute', 'executing', 'executed'], }; -const eventTypes: Record = { +const eventTypes: Record = { connector_create: EventType.CREATION, connector_get: EventType.ACCESS, connector_update: EventType.CHANGE, @@ -35,19 +35,19 @@ const eventTypes: Record = { connector_execute: undefined, }; -export interface ConnectorEventParams { - action: ConnectorAction; +export interface ConnectorAuditEventParams { + action: ConnectorAuditAction; outcome?: EventOutcome; savedObject?: NonNullable['saved_object']; error?: Error; } -export function connectorEvent({ +export function connectorAuditEvent({ action, savedObject, outcome, error, -}: ConnectorEventParams): AuditEvent { +}: ConnectorAuditEventParams): AuditEvent { const doc = savedObject ? `connector [id=${savedObject.id}]` : 'a connector'; const [present, progressive, past] = eventVerbs[action]; const message = error diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index f8beb09ce7ab5f..d697817be734b7 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -50,7 +50,7 @@ import { parseDuration } from '../../common/parse_duration'; import { retryIfConflicts } from '../lib/retry_if_conflicts'; import { partiallyUpdateAlert } from '../saved_objects'; import { markApiKeyForInvalidation } from '../invalidate_pending_api_keys/mark_api_key_for_invalidation'; -import { alertRuleEvent, AlertRuleAction } from './audit_events'; +import { alertAuditEvent, AlertAuditAction } from './audit_events'; export interface RegistryAlertTypeWithAuth extends RegistryAlertType { authorizedConsumers: string[]; @@ -227,8 +227,8 @@ export class AlertsClient { ); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.CREATE, + alertAuditEvent({ + action: AlertAuditAction.CREATE, savedObject: { type: 'alert', id }, error, }) @@ -270,8 +270,8 @@ export class AlertsClient { }; this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.CREATE, + alertAuditEvent({ + action: AlertAuditAction.CREATE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -335,8 +335,8 @@ export class AlertsClient { ); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.GET, + alertAuditEvent({ + action: AlertAuditAction.GET, savedObject: { type: 'alert', id }, error, }) @@ -344,8 +344,8 @@ export class AlertsClient { throw error; } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.GET, + alertAuditEvent({ + action: AlertAuditAction.GET, savedObject: { type: 'alert', id }, }) ); @@ -422,8 +422,8 @@ export class AlertsClient { authorizationTuple = await this.authorization.getFindAuthorizationFilter(); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.FIND, + alertAuditEvent({ + action: AlertAuditAction.FIND, error, }) ); @@ -455,8 +455,8 @@ export class AlertsClient { ensureAlertTypeIsAuthorized(attributes.alertTypeId, attributes.consumer); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.FIND, + alertAuditEvent({ + action: AlertAuditAction.FIND, savedObject: { type: 'alert', id }, error, }) @@ -472,8 +472,8 @@ export class AlertsClient { authorizedData.forEach(({ id }) => this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.FIND, + alertAuditEvent({ + action: AlertAuditAction.FIND, savedObject: { type: 'alert', id }, }) ) @@ -560,8 +560,8 @@ export class AlertsClient { ); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.DELETE, + alertAuditEvent({ + action: AlertAuditAction.DELETE, savedObject: { type: 'alert', id }, error, }) @@ -570,8 +570,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.DELETE, + alertAuditEvent({ + action: AlertAuditAction.DELETE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -627,8 +627,8 @@ export class AlertsClient { ); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UPDATE, + alertAuditEvent({ + action: AlertAuditAction.UPDATE, savedObject: { type: 'alert', id }, error, }) @@ -637,8 +637,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UPDATE, + alertAuditEvent({ + action: AlertAuditAction.UPDATE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -791,8 +791,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UPDATE_API_KEY, + alertAuditEvent({ + action: AlertAuditAction.UPDATE_API_KEY, savedObject: { type: 'alert', id }, error, }) @@ -812,8 +812,8 @@ export class AlertsClient { }); this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UPDATE_API_KEY, + alertAuditEvent({ + action: AlertAuditAction.UPDATE_API_KEY, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -885,8 +885,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.ENABLE, + alertAuditEvent({ + action: AlertAuditAction.ENABLE, savedObject: { type: 'alert', id }, error, }) @@ -895,8 +895,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.ENABLE, + alertAuditEvent({ + action: AlertAuditAction.ENABLE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -984,8 +984,8 @@ export class AlertsClient { ); } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.DISABLE, + alertAuditEvent({ + action: AlertAuditAction.DISABLE, savedObject: { type: 'alert', id }, error, }) @@ -994,8 +994,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.DISABLE, + alertAuditEvent({ + action: AlertAuditAction.DISABLE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -1058,8 +1058,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.MUTE, + alertAuditEvent({ + action: AlertAuditAction.MUTE, savedObject: { type: 'alert', id }, error, }) @@ -1068,8 +1068,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.MUTE, + alertAuditEvent({ + action: AlertAuditAction.MUTE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -1117,8 +1117,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UNMUTE, + alertAuditEvent({ + action: AlertAuditAction.UNMUTE, savedObject: { type: 'alert', id }, error, }) @@ -1127,8 +1127,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UNMUTE, + alertAuditEvent({ + action: AlertAuditAction.UNMUTE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id }, }) @@ -1176,8 +1176,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.MUTE_INSTANCE, + alertAuditEvent({ + action: AlertAuditAction.MUTE_INSTANCE, savedObject: { type: 'alert', id: alertId }, error, }) @@ -1186,8 +1186,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.MUTE_INSTANCE, + alertAuditEvent({ + action: AlertAuditAction.MUTE_INSTANCE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id: alertId }, }) @@ -1240,8 +1240,8 @@ export class AlertsClient { } } catch (error) { this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UNMUTE_INSTANCE, + alertAuditEvent({ + action: AlertAuditAction.UNMUTE_INSTANCE, savedObject: { type: 'alert', id: alertId }, error, }) @@ -1250,8 +1250,8 @@ export class AlertsClient { } this.auditLogger?.log( - alertRuleEvent({ - action: AlertRuleAction.UNMUTE_INSTANCE, + alertAuditEvent({ + action: AlertAuditAction.UNMUTE_INSTANCE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id: alertId }, }) diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts index d64a2c6bf6285a..9cd48248320c08 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.test.ts @@ -5,13 +5,13 @@ */ import { EventOutcome } from '../../../security/server/audit'; -import { AlertRuleAction, alertRuleEvent } from './audit_events'; +import { AlertAuditAction, alertAuditEvent } from './audit_events'; -describe('#alertRuleEvent', () => { +describe('#alertAuditEvent', () => { test('creates event with `unknown` outcome', () => { expect( - alertRuleEvent({ - action: AlertRuleAction.CREATE, + alertAuditEvent({ + action: AlertAuditAction.CREATE, outcome: EventOutcome.UNKNOWN, savedObject: { type: 'alert', id: 'ALERT_ID' }, }) @@ -37,8 +37,8 @@ describe('#alertRuleEvent', () => { test('creates event with `success` outcome', () => { expect( - alertRuleEvent({ - action: AlertRuleAction.CREATE, + alertAuditEvent({ + action: AlertAuditAction.CREATE, savedObject: { type: 'alert', id: 'ALERT_ID' }, }) ).toMatchInlineSnapshot(` @@ -63,8 +63,8 @@ describe('#alertRuleEvent', () => { test('creates event with `failure` outcome', () => { expect( - alertRuleEvent({ - action: AlertRuleAction.CREATE, + alertAuditEvent({ + action: AlertAuditAction.CREATE, savedObject: { type: 'alert', id: 'ALERT_ID' }, error: new Error('ERROR_MESSAGE'), }) diff --git a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts index b380a74f06090f..f3e39598240847 100644 --- a/x-pack/plugins/alerts/server/alerts_client/audit_events.ts +++ b/x-pack/plugins/alerts/server/alerts_client/audit_events.ts @@ -6,7 +6,7 @@ import { AuditEvent, EventOutcome, EventCategory, EventType } from '../../../security/server'; -export enum AlertRuleAction { +export enum AlertAuditAction { CREATE = 'alert_create', GET = 'alert_get', UPDATE = 'alert_update', @@ -23,7 +23,7 @@ export enum AlertRuleAction { type VerbsTuple = [string, string, string]; -const eventVerbs: Record = { +const eventVerbs: Record = { alert_create: ['create', 'creating', 'created'], alert_get: ['access', 'accessing', 'accessed'], alert_update: ['update', 'updating', 'updated'], @@ -38,7 +38,7 @@ const eventVerbs: Record = { alert_instance_unmute: ['unmute instance of', 'unmuting instance of', 'unmuted instance of'], }; -const eventTypes: Record = { +const eventTypes: Record = { alert_create: EventType.CREATION, alert_get: EventType.ACCESS, alert_update: EventType.CHANGE, @@ -53,19 +53,19 @@ const eventTypes: Record = { alert_instance_unmute: EventType.CHANGE, }; -export interface AlertRuleEventParams { - action: AlertRuleAction; +export interface AlertAuditEventParams { + action: AlertAuditAction; outcome?: EventOutcome; savedObject?: NonNullable['saved_object']; error?: Error; } -export function alertRuleEvent({ +export function alertAuditEvent({ action, savedObject, outcome, error, -}: AlertRuleEventParams): AuditEvent { +}: AlertAuditEventParams): AuditEvent { const doc = savedObject ? `alert [id=${savedObject.id}]` : 'an alert'; const [present, progressive, past] = eventVerbs[action]; const message = error From 459e0c4f25a7184c5c35da246369f62f8691eea7 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Fri, 4 Dec 2020 13:27:18 +0000 Subject: [PATCH 12/12] Added suggestions from PR --- .../server/saved_objects/service/lib/utils.ts | 2 +- .../plugins/actions/server/actions_client.ts | 18 ++++--- .../encrypted_saved_objects_client_wrapper.ts | 53 +++++++++---------- ...ecure_saved_objects_client_wrapper.test.ts | 2 +- .../secure_saved_objects_client_wrapper.ts | 18 ++++--- 5 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/utils.ts b/src/core/server/saved_objects/service/lib/utils.ts index e61f6d9224b92a..b59829cb4978aa 100644 --- a/src/core/server/saved_objects/service/lib/utils.ts +++ b/src/core/server/saved_objects/service/lib/utils.ts @@ -80,7 +80,7 @@ export class SavedObjectsUtils { } /** - * Validates that a saved object ID matches UUID format. + * Validates that a saved object ID has been randomly generated. * * @param {string} id The ID of a saved object. * @todo Use `uuid.validate` once upgraded to v5.3+ diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 20fd50f92a5021..ab693dc340c927 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -392,14 +392,16 @@ export class ActionsClient { const bulkGetOpts = actionSavedObjectsIds.map((id) => ({ id, type: 'action' })); const bulkGetResult = await this.unsecuredSavedObjectsClient.bulkGet(bulkGetOpts); - ids.forEach((id) => - this.auditLogger?.log( - connectorAuditEvent({ - action: ConnectorAuditAction.GET, - savedObject: { type: 'action', id }, - }) - ) - ); + bulkGetResult.saved_objects.forEach(({ id, error }) => { + if (!error && this.auditLogger) { + this.auditLogger.log( + connectorAuditEvent({ + action: ConnectorAuditAction.GET, + savedObject: { type: 'action', id }, + }) + ); + } + }); for (const action of bulkGetResult.saved_objects) { if (action.error) { diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index c186f3e26a82f7..313e7c7da9eba2 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -59,22 +59,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return await this.options.baseClient.create(type, attributes, options); } - // Saved objects with encrypted attributes should have IDs that are hard to guess especially - // since IDs are part of the AAD used during encryption, that's why we control them within this - // wrapper and don't allow consumers to specify their own IDs directly. - - // only allow a specified ID if we're overwriting an existing ESO with a Version - // this helps us ensure that the document really was previously created using ESO - // and not being used to get around the specified ID limitation - const canSpecifyID = - (options.overwrite && options.version) || SavedObjectsUtils.isRandomId(options.id); - if (options.id && !canSpecifyID) { - throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' - ); - } - - const id = options.id ?? SavedObjectsUtils.generateId(); + const id = getValidId(options.id, options.version, options.overwrite); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -108,18 +93,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return object; } - // Saved objects with encrypted attributes should have IDs that are hard to guess especially - // since IDs are part of the AAD used during encryption, that's why we control them within this - // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. - const canSpecifyID = - (options?.overwrite && object.version) || SavedObjectsUtils.isRandomId(object.id); - if (object.id && !canSpecifyID) { - throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' - ); - } - - const id = object.id ?? SavedObjectsUtils.generateId(); + const id = getValidId(object.id, object.version, options?.overwrite); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, @@ -321,3 +295,26 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return response; } } + +// Saved objects with encrypted attributes should have IDs that are hard to guess especially +// since IDs are part of the AAD used during encryption, that's why we control them within this +// wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. +function getValidId( + id: string | undefined, + version: string | undefined, + overwrite: boolean | undefined +) { + if (id) { + // only allow a specified ID if we're overwriting an existing ESO with a Version + // this helps us ensure that the document really was previously created using ESO + // and not being used to get around the specified ID limitation + const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id); + if (!canSpecifyID) { + throw new Error( + 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' + ); + } + return id; + } + return SavedObjectsUtils.generateId(); +} diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 20ab98623d8738..15ca8bac89bd61 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -563,7 +563,7 @@ describe('#bulkGet', () => { }); test(`adds audit event when successful`, async () => { - const apiCallReturnValue = { saved_objects: [], foo: 'bar' }; + const apiCallReturnValue = { saved_objects: [obj1, obj2], foo: 'bar' }; clientOpts.baseClient.bulkGet.mockReturnValue(apiCallReturnValue as any); const objects = [obj1, obj2]; const options = { namespace }; diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index 4b97dcfe313bdd..765274a839efab 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -294,14 +294,16 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra const response = await this.baseClient.bulkGet(objects, options); - objects.forEach(({ type, id }) => - this.auditLogger.log( - savedObjectEvent({ - action: SavedObjectAction.GET, - savedObject: { type, id }, - }) - ) - ); + response.saved_objects.forEach(({ error, type, id }) => { + if (!error) { + this.auditLogger.log( + savedObjectEvent({ + action: SavedObjectAction.GET, + savedObject: { type, id }, + }) + ); + } + }); return await this.redactSavedObjectsNamespaces(response, [options.namespace]); }