Revert "Add audit logging to space deletion (#123378)"

This reverts commit 5819cfb.
elastic · Jan 25, 2022 · cd06e5f · cd06e5f
1 parent 831a40f
commit cd06e5f
Show file tree

Hide file tree

Showing 19 changed files with 29 additions and 158 deletions.
diff --git a/x-pack/plugins/cases/server/authorization/audit_logger.test.ts b/x-pack/plugins/cases/server/authorization/audit_logger.test.ts
@@ -32,7 +32,6 @@ describe('audit_logger', () => {
   describe('log function', () => {
     const mockLogger: jest.Mocked<AuditLogger> = {
       log: jest.fn(),
-      enabled: true,
     };
 
     let logger: AuthorizationAuditLogger;

diff --git a/x-pack/plugins/cases/server/authorization/authorization.test.ts b/x-pack/plugins/cases/server/authorization/authorization.test.ts
@@ -24,7 +24,6 @@ describe('authorization', () => {
     request = httpServerMock.createKibanaRequest();
     mockLogger = {
       log: jest.fn(),
-      enabled: true,
     };
   });
 

diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client_factory.test.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client_factory.test.ts
@@ -46,7 +46,6 @@ const fakeRequest = {
 
 const auditLogger = {
   log: jest.fn(),
-  enabled: true,
 } as jest.Mocked<AuditLogger>;
 
 describe('AlertsClientFactory', () => {

diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/tests/bulk_update.test.ts b/x-pack/plugins/rule_registry/server/alert_data_client/tests/bulk_update.test.ts
@@ -25,7 +25,6 @@ const alertingAuthMock = alertingAuthorizationMock.create();
 const esClientMock = elasticsearchClientMock.createElasticsearchClient();
 const auditLogger = {
   log: jest.fn(),
-  enabled: true,
 } as jest.Mocked<AuditLogger>;
 
 const alertsClientParams: jest.Mocked<ConstructorOptions> = {

diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/tests/find_alerts.test.ts b/x-pack/plugins/rule_registry/server/alert_data_client/tests/find_alerts.test.ts
@@ -24,7 +24,6 @@ const alertingAuthMock = alertingAuthorizationMock.create();
 const esClientMock = elasticsearchClientMock.createElasticsearchClient();
 const auditLogger = {
   log: jest.fn(),
-  enabled: true,
 } as jest.Mocked<AuditLogger>;
 
 const alertsClientParams: jest.Mocked<ConstructorOptions> = {

diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/tests/get.test.ts b/x-pack/plugins/rule_registry/server/alert_data_client/tests/get.test.ts
@@ -25,7 +25,6 @@ const alertingAuthMock = alertingAuthorizationMock.create();
 const esClientMock = elasticsearchClientMock.createElasticsearchClient();
 const auditLogger = {
   log: jest.fn(),
-  enabled: true,
 } as jest.Mocked<AuditLogger>;
 
 const alertsClientParams: jest.Mocked<ConstructorOptions> = {

diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/tests/update.test.ts b/x-pack/plugins/rule_registry/server/alert_data_client/tests/update.test.ts
@@ -24,7 +24,6 @@ const alertingAuthMock = alertingAuthorizationMock.create();
 const esClientMock = elasticsearchClientMock.createElasticsearchClient();
 const auditLogger = {
   log: jest.fn(),
-  enabled: true,
 } as jest.Mocked<AuditLogger>;
 
 const alertsClientParams: jest.Mocked<ConstructorOptions> = {

diff --git a/x-pack/plugins/security/server/audit/audit_events.ts b/x-pack/plugins/security/server/audit/audit_events.ts
@@ -218,6 +218,8 @@ export enum SavedObjectAction {
   UPDATE = 'saved_object_update',
   DELETE = 'saved_object_delete',
   FIND = 'saved_object_find',
+  ADD_TO_SPACES = 'saved_object_add_to_spaces',
+  DELETE_FROM_SPACES = 'saved_object_delete_from_spaces',
   REMOVE_REFERENCES = 'saved_object_remove_references',
   OPEN_POINT_IN_TIME = 'saved_object_open_point_in_time',
   CLOSE_POINT_IN_TIME = 'saved_object_close_point_in_time',
@@ -234,6 +236,8 @@ const savedObjectAuditVerbs: Record<SavedObjectAction, VerbsTuple> = {
   saved_object_update: ['update', 'updating', 'updated'],
   saved_object_delete: ['delete', 'deleting', 'deleted'],
   saved_object_find: ['access', 'accessing', 'accessed'],
+  saved_object_add_to_spaces: ['update', 'updating', 'updated'],
+  saved_object_delete_from_spaces: ['update', 'updating', 'updated'],
   saved_object_open_point_in_time: [
     'open point-in-time',
     'opening point-in-time',
@@ -268,6 +272,8 @@ const savedObjectAuditTypes: Record<SavedObjectAction, EcsEventType> = {
   saved_object_update: 'change',
   saved_object_delete: 'deletion',
   saved_object_find: 'access',
+  saved_object_add_to_spaces: 'change',
+  saved_object_delete_from_spaces: 'change',
   saved_object_open_point_in_time: 'creation',
   saved_object_close_point_in_time: 'deletion',
   saved_object_remove_references: 'change',

diff --git a/x-pack/plugins/security/server/audit/audit_service.test.ts b/x-pack/plugins/security/server/audit/audit_service.test.ts
@@ -68,7 +68,6 @@ describe('#setup', () => {
       Object {
         "asScoped": [Function],
         "withoutRequest": Object {
-          "enabled": true,
           "log": [Function],
         },
       }

diff --git a/x-pack/plugins/security/server/audit/audit_service.ts b/x-pack/plugins/security/server/audit/audit_service.ts
@@ -49,14 +49,6 @@ export interface AuditLogger {
    * ```
    */
   log: (event: AuditEvent | undefined) => void;
-
-  /**
-   * Indicates whether audit logging is enabled or not.
-   *
-   * Useful for skipping resource-intense operations that don't need to be performed when audit
-   * logging is disabled.
-   */
-  readonly enabled: boolean;
 }
 
 export interface AuditServiceSetup {
@@ -130,8 +122,7 @@ export class AuditService {
     );
 
     // Record feature usage at a regular interval if enabled and license allows
-    const enabled = !!(config.enabled && config.appender);
-    if (enabled) {
+    if (config.enabled && config.appender) {
       license.features$.subscribe((features) => {
         clearInterval(this.usageIntervalId!);
         if (features.allowAuditLogging) {
@@ -178,7 +169,6 @@ export class AuditService {
           trace: { id: request.id },
         });
       },
-      enabled,
     });
 
     http.registerOnPostAuth((request, response, t) => {
@@ -190,7 +180,7 @@ export class AuditService {
 
     return {
       asScoped,
-      withoutRequest: { log, enabled },
+      withoutRequest: { log },
     };
   }
 

diff --git a/x-pack/plugins/security/server/audit/index.mock.ts b/x-pack/plugins/security/server/audit/index.mock.ts
@@ -13,11 +13,9 @@ export const auditServiceMock = {
       getLogger: jest.fn(),
       asScoped: jest.fn().mockReturnValue({
         log: jest.fn(),
-        enabled: true,
       }),
       withoutRequest: {
         log: jest.fn(),
-        enabled: true,
       },
     } as jest.Mocked<ReturnType<AuditService['setup']>>;
   },

diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts
@@ -264,7 +264,6 @@ describe('Authenticator', () => {
     let mockSessVal: SessionValue;
     const auditLogger = {
       log: jest.fn(),
-      enabled: true,
     };
 
     beforeEach(() => {
@@ -1095,7 +1094,6 @@ describe('Authenticator', () => {
     let mockSessVal: SessionValue;
     const auditLogger = {
       log: jest.fn(),
-      enabled: true,
     };
 
     beforeEach(() => {
@@ -2011,7 +2009,6 @@ describe('Authenticator', () => {
     let mockSessVal: SessionValue;
     const auditLogger = {
       log: jest.fn(),
-      enabled: true,
     };
 
     beforeEach(() => {
@@ -2148,7 +2145,6 @@ describe('Authenticator', () => {
     let mockSessionValue: SessionValue;
     const auditLogger = {
       log: jest.fn(),
-      enabled: true,
     };
 
     beforeEach(() => {

diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts
@@ -68,7 +68,6 @@ describe('Security Plugin', () => {
           "audit": Object {
             "asScoped": [Function],
             "withoutRequest": Object {
-              "enabled": false,
               "log": [Function],
             },
           },

diff --git a/x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.test.ts b/x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.test.ts
@@ -8,11 +8,7 @@
 import { mockEnsureAuthorized } from './secure_spaces_client_wrapper.test.mocks';
 
 import { deepFreeze } from '@kbn/std';
-import type {
-  EcsEventOutcome,
-  SavedObjectsClientContract,
-  SavedObjectsFindResponse,
-} from 'src/core/server';
+import type { EcsEventOutcome, SavedObjectsClientContract } from 'src/core/server';
 import { SavedObjectsErrorHelpers } from 'src/core/server';
 import { httpServerMock } from 'src/core/server/mocks';
 
@@ -67,31 +63,6 @@ const setup = ({ securityEnabled = false }: Opts = {}) => {
     return space;
   });
 
-  baseClient.createSavedObjectFinder.mockImplementation(() => ({
-    async *find() {
-      yield {
-        saved_objects: [
-          {
-            namespaces: ['*'],
-            type: 'dashboard',
-            id: '1',
-          },
-          {
-            namespaces: ['existing_space'],
-            type: 'dashboard',
-            id: '2',
-          },
-          {
-            namespaces: ['default', 'existing_space'],
-            type: 'dashboard',
-            id: '3',
-          },
-        ],
-      } as SavedObjectsFindResponse<unknown, unknown>;
-    },
-    async close() {},
-  }));
-
   const authorization = authorizationMock.create({
     version: 'unit-test',
     applicationName: 'kibana',
@@ -631,7 +602,7 @@ describe('SecureSpacesClientWrapper', () => {
       });
     });
 
-    it(`throws a forbidden error when unauthorized`, async () => {
+    test(`throws a forbidden error when unauthorized`, async () => {
       const username = 'some_user';
 
       const { wrapper, baseClient, authorization, auditLogger, request } = setup({
@@ -666,7 +637,7 @@ describe('SecureSpacesClientWrapper', () => {
       });
     });
 
-    it('deletes the space with all saved objects when authorized', async () => {
+    it('deletes the space when authorized', async () => {
       const username = 'some_user';
 
       const { wrapper, baseClient, authorization, auditLogger, request } = setup({
@@ -698,14 +669,6 @@ describe('SecureSpacesClientWrapper', () => {
         type: 'space',
         id: space.id,
       });
-      expectAuditEvent(auditLogger, SavedObjectAction.DELETE, 'unknown', {
-        type: 'dashboard',
-        id: '2',
-      });
-      expectAuditEvent(auditLogger, SavedObjectAction.UPDATE_OBJECTS_SPACES, 'unknown', {
-        type: 'dashboard',
-        id: '3',
-      });
     });
   });
 

diff --git a/x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.ts b/x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.ts
@@ -17,7 +17,6 @@ import type {
   LegacyUrlAliasTarget,
   Space,
 } from '../../../spaces/server';
-import { ALL_SPACES_ID } from '../../common/constants';
 import type { AuditLogger } from '../audit';
 import { SavedObjectAction, savedObjectEvent, SpaceAuditAction, spaceAuditEvent } from '../audit';
 import type { AuthorizationServiceSetup } from '../authorization';
@@ -247,10 +246,6 @@ export class SecureSpacesClientWrapper implements ISpacesClient {
     return this.spacesClient.update(id, space);
   }
 
-  public createSavedObjectFinder(id: string) {
-    return this.spacesClient.createSavedObjectFinder(id);
-  }
-
   public async delete(id: string) {
     if (this.useRbac) {
       try {
@@ -270,35 +265,6 @@ export class SecureSpacesClientWrapper implements ISpacesClient {
       }
     }
 
-    // Fetch saved objects to be removed for audit logging
-    if (this.auditLogger.enabled) {
-      const finder = this.spacesClient.createSavedObjectFinder(id);
-      try {
-        for await (const response of finder.find()) {
-          response.saved_objects.forEach((savedObject) => {
-            const { namespaces = [] } = savedObject;
-            const isOnlySpace = namespaces.length === 1; // We can always rely on the `namespaces` field having >=1 element
-            if (namespaces.includes(ALL_SPACES_ID) && !namespaces.includes(id)) {
-              // This object exists in All Spaces and its `namespaces` field isn't going to change; there's nothing to audit
-              return;
-            }
-            this.auditLogger.log(
-              savedObjectEvent({
-                action: isOnlySpace
-                  ? SavedObjectAction.DELETE
-                  : SavedObjectAction.UPDATE_OBJECTS_SPACES,
-                outcome: 'unknown',
-                savedObject: { type: savedObject.type, id: savedObject.id },
-                deleteFromSpaces: [id],
-              })
-            );
-          });
-        }
-      } finally {
-        await finder.close();
-      }
-    }
-
     this.auditLogger.log(
       spaceAuditEvent({
         action: SpaceAuditAction.DELETE,

diff --git a/x-pack/plugins/spaces/server/spaces_client/spaces_client.mock.ts b/x-pack/plugins/spaces/server/spaces_client/spaces_client.mock.ts
@@ -5,15 +5,12 @@
  * 2.0.
  */
 
-import { savedObjectsRepositoryMock } from 'src/core/server/mocks';
-
 import type { Space } from '../../common';
 import { DEFAULT_SPACE_ID } from '../../common/constants';
 import type { SpacesClient } from './spaces_client';
 
-const createSpacesClientMock = () => {
-  const repositoryMock = savedObjectsRepositoryMock.create();
-  return {
+const createSpacesClientMock = () =>
+  ({
     getAll: jest.fn().mockResolvedValue([
       {
         id: DEFAULT_SPACE_ID,
@@ -31,11 +28,10 @@ const createSpacesClientMock = () => {
     }),
     create: jest.fn().mockImplementation((space: Space) => Promise.resolve(space)),
     update: jest.fn().mockImplementation((space: Space) => Promise.resolve(space)),
-    createSavedObjectFinder: repositoryMock.createPointInTimeFinder,
     delete: jest.fn(),
     disableLegacyUrlAliases: jest.fn(),
-  } as unknown as jest.Mocked<SpacesClient>;
-};
+  } as unknown as jest.Mocked<SpacesClient>);
+
 export const spacesClientMock = {
   create: createSpacesClientMock,
 };