[Cases] Refactoring authorization (#97483)

* Refactoring authorization * Wrapping auth calls in helper for try catch * Reverting name change * Hardcoding the saved object types * Switching ensure to owner array
elastic · Apr 20, 2021 · 676173e · 676173e
1 parent 613e859
commit 676173e
Show file tree

Hide file tree

Showing 9 changed files with 211 additions and 135 deletions.
diff --git a/x-pack/plugins/cases/common/constants.ts b/x-pack/plugins/cases/common/constants.ts
@@ -14,6 +14,9 @@ export const CASE_USER_ACTION_SAVED_OBJECT = 'cases-user-actions';
 export const CASE_COMMENT_SAVED_OBJECT = 'cases-comments';
 export const CASE_CONFIGURE_SAVED_OBJECT = 'cases-configure';
 
+/**
+ * If more values are added here please also add them here: x-pack/test/case_api_integration/common/fixtures/plugins
+ */
 export const SAVED_OBJECT_TYPES = [
   CASE_SAVED_OBJECT,
   CASE_CONNECTOR_MAPPINGS_SAVED_OBJECT,

diff --git a/x-pack/plugins/cases/server/authorization/audit_logger.ts b/x-pack/plugins/cases/server/authorization/audit_logger.ts
@@ -20,19 +20,19 @@ export class AuthorizationAuditLogger {
     this.auditLogger = logger;
   }
 
-  private createMessage({
+  private static createMessage({
     result,
-    owner,
+    owners,
     operation,
   }: {
     result: AuthorizationResult;
-    owner?: string;
+    owners?: string[];
     operation: OperationDetails;
   }): string {
-    const ownerMsg = owner == null ? 'of any owner' : `with "${owner}" as the owner`;
+    const ownerMsg = owners == null ? 'of any owner' : `with owners: "${owners.join(', ')}"`;
     /**
      * This will take the form:
-     * `Unauthorized to create case with "securitySolution" as the owner`
+     * `Unauthorized to create case with owners: "securitySolution, observability"`
      * `Unauthorized to find cases of any owner`.
      */
     return `${result} to ${operation.verbs.present} ${operation.docType} ${ownerMsg}`;
@@ -65,16 +65,16 @@ export class AuthorizationAuditLogger {
 
   public failure({
     username,
-    owner,
+    owners,
     operation,
   }: {
     username?: string;
-    owner?: string;
+    owners?: string[];
     operation: OperationDetails;
   }): string {
-    const message = this.createMessage({
+    const message = AuthorizationAuditLogger.createMessage({
       result: AuthorizationResult.Unauthorized,
-      owner,
+      owners,
       operation,
     });
     this.auditLogger?.log({
@@ -96,24 +96,6 @@ export class AuthorizationAuditLogger {
   }
 
   public success({
-    username,
-    operation,
-    owner,
-  }: {
-    username: string;
-    owner: string;
-    operation: OperationDetails;
-  }): string {
-    const message = this.createMessage({
-      result: AuthorizationResult.Authorized,
-      owner,
-      operation,
-    });
-    this.logSuccessEvent({ message, operation, username });
-    return message;
-  }
-
-  public bulkSuccess({
     username,
     operation,
     owners,
@@ -122,9 +104,11 @@ export class AuthorizationAuditLogger {
     owners: string[];
     operation: OperationDetails;
   }): string {
-    const message = `${AuthorizationResult.Authorized} to ${operation.verbs.present} ${
-      operation.docType
-    } of owner: ${owners.join(', ')}`;
+    const message = AuthorizationAuditLogger.createMessage({
+      result: AuthorizationResult.Authorized,
+      owners,
+      operation,
+    });
     this.logSuccessEvent({ message, operation, username });
     return message;
   }

diff --git a/x-pack/plugins/cases/server/authorization/authorization.ts b/x-pack/plugins/cases/server/authorization/authorization.ts
@@ -11,7 +11,7 @@ import { SecurityPluginStart } from '../../../security/server';
 import { PluginStartContract as FeaturesPluginStart } from '../../../features/server';
 import { AuthorizationFilter, GetSpaceFn } from './types';
 import { getOwnersFilter } from './utils';
-import { AuthorizationAuditLogger, OperationDetails, Operations } from '.';
+import { AuthorizationAuditLogger, OperationDetails } from '.';
 
 /**
  * This class handles ensuring that the user making a request has the correct permissions
@@ -79,44 +79,47 @@ export class Authorization {
     return this.securityAuth?.mode?.useRbacForRequest(this.request) ?? false;
   }
 
-  public async ensureAuthorized(owner: string, operation: OperationDetails) {
+  public async ensureAuthorized(owners: string[], operation: OperationDetails) {
     const { securityAuth } = this;
-    const isOwnerAvailable = this.featureCaseOwners.has(owner);
+    const areAllOwnersAvailable = owners.every((owner) => this.featureCaseOwners.has(owner));
 
     if (securityAuth && this.shouldCheckAuthorization()) {
-      const requiredPrivileges: string[] = [securityAuth.actions.cases.get(owner, operation.name)];
+      const requiredPrivileges: string[] = owners.map((owner) =>
+        securityAuth.actions.cases.get(owner, operation.name)
+      );
 
       const checkPrivileges = securityAuth.checkPrivilegesDynamicallyWithRequest(this.request);
       const { hasAllRequested, username } = await checkPrivileges({
         kibana: requiredPrivileges,
       });
 
-      if (!isOwnerAvailable) {
+      if (!areAllOwnersAvailable) {
         /**
          * Under most circumstances this would have been caught by `checkPrivileges` as
          * a user can't have Privileges to an unknown owner, but super users
          * don't actually get "privilege checked" so the made up owner *will* return
          * as Privileged.
          * This check will ensure we don't accidentally let these through
          */
-        throw Boom.forbidden(this.auditLogger.failure({ username, owner, operation }));
+        throw Boom.forbidden(this.auditLogger.failure({ username, owners, operation }));
       }
 
       if (hasAllRequested) {
-        this.auditLogger.success({ username, operation, owner });
+        this.auditLogger.success({ username, operation, owners });
       } else {
-        throw Boom.forbidden(this.auditLogger.failure({ owner, operation, username }));
+        throw Boom.forbidden(this.auditLogger.failure({ owners, operation, username }));
       }
-    } else if (!isOwnerAvailable) {
-      throw Boom.forbidden(this.auditLogger.failure({ owner, operation }));
+    } else if (!areAllOwnersAvailable) {
+      throw Boom.forbidden(this.auditLogger.failure({ owners, operation }));
     }
 
     // else security is disabled so let the operation proceed
   }
 
-  public async getFindAuthorizationFilter(savedObjectType: string): Promise<AuthorizationFilter> {
+  public async getFindAuthorizationFilter(
+    operation: OperationDetails
+  ): Promise<AuthorizationFilter> {
     const { securityAuth } = this;
-    const operation = Operations.findCases;
     if (securityAuth && this.shouldCheckAuthorization()) {
       const { username, authorizedOwners } = await this.getAuthorizedOwners([operation]);
 
@@ -125,15 +128,17 @@ export class Authorization {
       }
 
       return {
-        filter: getOwnersFilter(savedObjectType, authorizedOwners),
+        filter: getOwnersFilter(operation.savedObjectType, authorizedOwners),
         ensureSavedObjectIsAuthorized: (owner: string) => {
           if (!authorizedOwners.includes(owner)) {
-            throw Boom.forbidden(this.auditLogger.failure({ username, operation, owner }));
+            throw Boom.forbidden(
+              this.auditLogger.failure({ username, operation, owners: [owner] })
+            );
           }
         },
         logSuccessfulAuthorization: () => {
           if (authorizedOwners.length) {
-            this.auditLogger.bulkSuccess({ username, owners: authorizedOwners, operation });
+            this.auditLogger.success({ username, owners: authorizedOwners, operation });
           }
         },
       };
@@ -155,11 +160,11 @@ export class Authorization {
     const { securityAuth, featureCaseOwners } = this;
     if (securityAuth && this.shouldCheckAuthorization()) {
       const checkPrivileges = securityAuth.checkPrivilegesDynamicallyWithRequest(this.request);
-      const requiredPrivileges = new Map<string, [string]>();
+      const requiredPrivileges = new Map<string, string>();
 
       for (const owner of featureCaseOwners) {
         for (const operation of operations) {
-          requiredPrivileges.set(securityAuth.actions.cases.get(owner, operation.name), [owner]);
+          requiredPrivileges.set(securityAuth.actions.cases.get(owner, operation.name), owner);
         }
       }
 
@@ -174,7 +179,7 @@ export class Authorization {
           ? Array.from(featureCaseOwners)
           : privileges.kibana.reduce<string[]>((authorizedOwners, { authorized, privilege }) => {
               if (authorized && requiredPrivileges.has(privilege)) {
-                const [owner] = requiredPrivileges.get(privilege)!;
+                const owner = requiredPrivileges.get(privilege)!;
                 authorizedOwners.push(owner);
               }
 

diff --git a/x-pack/plugins/cases/server/client/cases/create.ts b/x-pack/plugins/cases/server/client/cases/create.ts
@@ -28,7 +28,7 @@ import {
   User,
 } from '../../../common/api';
 import { buildCaseUserActionItem } from '../../services/user_actions/helpers';
-import { getConnectorFromConfiguration } from '../utils';
+import { createAuditMsg, ensureAuthorized, getConnectorFromConfiguration } from '../utils';
 
 import { CaseConfigureService, CaseService, CaseUserActionService } from '../../services';
 import { createCaseError } from '../../common/error';
@@ -37,7 +37,6 @@ import { Operations } from '../../authorization';
 import { AuditLogger, EventOutcome } from '../../../../security/server';
 import { ENABLE_CASE_CONNECTOR } from '../../../common/constants';
 import {
-  createAuditMsg,
   flattenCaseSavedObject,
   transformCaseConnectorToEsConnector,
   transformNewCase,
@@ -89,12 +88,14 @@ export const create = async ({
 
   try {
     const savedObjectID = SavedObjectsUtils.generateId();
-    try {
-      await auth.ensureAuthorized(query.owner, Operations.createCase);
-    } catch (error) {
-      auditLogger?.log(createAuditMsg({ operation: Operations.createCase, error, savedObjectID }));
-      throw error;
-    }
+
+    await ensureAuthorized({
+      operation: Operations.createCase,
+      owners: [query.owner],
+      authorization: auth,
+      auditLogger,
+      savedObjectIDs: [savedObjectID],
+    });
 
     // log that we're attempting to create a case
     auditLogger?.log(

diff --git a/x-pack/plugins/cases/server/client/cases/find.ts b/x-pack/plugins/cases/server/client/cases/find.ts
@@ -22,15 +22,14 @@ import {
   excess,
 } from '../../../common/api';
 
-import { CASE_SAVED_OBJECT } from '../../../common/constants';
 import { CaseService } from '../../services';
 import { createCaseError } from '../../common/error';
-import { constructQueryOptions } from '../utils';
+import { constructQueryOptions, getAuthorizationFilter } from '../utils';
 import { Authorization } from '../../authorization/authorization';
 import { includeFieldsRequiredForAuthentication } from '../../authorization/utils';
-import { AuthorizationFilter, Operations } from '../../authorization';
+import { Operations } from '../../authorization';
 import { AuditLogger } from '../../../../security/server';
-import { createAuditMsg, transformCases } from '../../common';
+import { transformCases } from '../../common';
 
 interface FindParams {
   savedObjectsClient: SavedObjectsClientContract;
@@ -49,28 +48,24 @@ export const find = async ({
   caseService,
   logger,
   auth,
-  auditLogger,
   options,
+  auditLogger,
 }: FindParams): Promise<CasesFindResponse> => {
   try {
     const queryParams = pipe(
       excess(CasesFindRequestRt).decode(options),
       fold(throwErrors(Boom.badRequest), identity)
     );
 
-    let authFindHelpers: AuthorizationFilter;
-    try {
-      authFindHelpers = await auth.getFindAuthorizationFilter(CASE_SAVED_OBJECT);
-    } catch (error) {
-      auditLogger?.log(createAuditMsg({ operation: Operations.findCases, error }));
-      throw error;
-    }
-
     const {
       filter: authorizationFilter,
-      ensureSavedObjectIsAuthorized,
+      ensureSavedObjectsAreAuthorized,
       logSuccessfulAuthorization,
-    } = authFindHelpers;
+    } = await getAuthorizationFilter({
+      authorization: auth,
+      operation: Operations.findCases,
+      auditLogger,
+    });
 
     const queryArgs = {
       tags: queryParams.tags,
@@ -100,20 +95,7 @@ export const find = async ({
       subCaseOptions: caseQueries.subCase,
     });
 
-    for (const theCase of cases.casesMap.values()) {
-      try {
-        ensureSavedObjectIsAuthorized(theCase.owner);
-        // log each of the found cases
-        auditLogger?.log(
-          createAuditMsg({ operation: Operations.findCases, savedObjectID: theCase.id })
-        );
-      } catch (error) {
-        auditLogger?.log(
-          createAuditMsg({ operation: Operations.findCases, error, savedObjectID: theCase.id })
-        );
-        throw error;
-      }
-    }
+    ensureSavedObjectsAreAuthorized([...cases.casesMap.values()]);
 
     // TODO: Make sure we do not leak information when authorization is on
     const [openCases, inProgressCases, closedCases] = await Promise.all([