Reduce saved objects authorization checks

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts

@@ -31,7 +31,9 @@ const createSecureSavedObjectsClientWrapperOptions = () => {
     createBadRequestError: jest.fn().mockImplementation((message) => new Error(message)),
     isNotFoundError: jest.fn().mockReturnValue(false),
   } as unknown) as jest.Mocked<SavedObjectsClientContract['errors']>;
-  const getSpacesService = jest.fn().mockReturnValue(true);
+  const getSpacesService = jest.fn().mockReturnValue({
+    namespaceToSpaceId: (namespace?: string) => (namespace ? namespace : 'default'),
+  });
 
   return {
     actions,
@@ -174,7 +176,9 @@ const expectObjectNamespaceFiltering = async (
   );
   expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
     'login:',
-    namespaces.filter((x) => x !== '*') // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
+    ['some-other-namespace']
+    // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
+    // we don't check privileges for authorizedNamespace either, as that was already checked earlier in the operation
   );
 };
 
@@ -206,12 +210,14 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
     getMockCheckPrivilegesFailure // privilege check for namespace filtering
   );
 
-  const authorizedNamespace = args.options.namespace || 'default';
+  // the 'find' operation has options.namespaces, the others have options.namespace
+  const authorizedNamespaces =
+    args.options.namespaces ?? (args.options.namespace ? [args.options.namespace] : ['default']);
   const returnValue = {
     saved_objects: [
-      { namespaces: ['foo'] },
-      { namespaces: [authorizedNamespace] },
-      { namespaces: ['foo', authorizedNamespace] },
+      { namespaces: ['*'] },
+      { namespaces: authorizedNamespaces },
+      { namespaces: ['some-other-namespace', ...authorizedNamespaces] },
     ],
   };
 
@@ -224,17 +230,19 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
   const result = await fn.bind(client)(...Object.values(args));
   expect(result).toEqual({
     saved_objects: [
-      { namespaces: ['?'] },
-      { namespaces: [authorizedNamespace] },
-      { namespaces: [authorizedNamespace, '?'] },
+      { namespaces: ['*'] },
+      { namespaces: authorizedNamespaces },
+      { namespaces: [...authorizedNamespaces, '?'] },
     ],
   });
 
   expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(2);
-  expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith('login:', [
-    'foo',
-    authorizedNamespace,
-  ]);
+  expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
+    'login:',
+    ['some-other-namespace']
+    // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
+    // we don't check privileges for authorizedNamespaces either, as that was already checked earlier in the operation
+  );
 };
 
 function getMockCheckPrivilegesSuccess(actions: string | string[], namespaces?: string | string[]) {
@@ -964,8 +972,8 @@ describe('#get', () => {
 describe('#deleteFromNamespaces', () => {
   const type = 'foo';
   const id = `${type}-id`;
-  const namespace1 = 'foo-namespace';
-  const namespace2 = 'bar-namespace';
+  const namespace1 = 'default';
+  const namespace2 = 'another-namespace';
   const namespaces = [namespace1, namespace2];
   const privilege = `mock-saved_object:${type}/share_to_space`;
 
@@ -1153,4 +1161,41 @@ describe('other', () => {
   test(`assigns errors from constructor to .errors`, () => {
     expect(client.errors).toBe(clientOpts.errors);
   });
+
+  test(`namespace redaction fails safe`, async () => {
+    const type = 'foo';
+    const id = `${type}-id`;
+    const namespace = 'some-ns';
+    const namespaces = ['some-other-namespace', '*', namespace];
+    const returnValue = { namespaces, foo: 'bar' };
+    clientOpts.baseClient.get.mockReturnValue(returnValue as any);
+
+    clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementationOnce(
+      getMockCheckPrivilegesSuccess // privilege check for authorization
+    );
+    clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementation(
+      // privilege check for namespace filtering
+      (_actions: string | string[], _namespaces?: string | string[]) => ({
+        hasAllRequested: false,
+        username: USERNAME,
+        privileges: {
+          kibana: [
+            // this is a contrived scenario as we *shouldn't* get both an unauthorized and authorized result for a given resource...
+            // however, in case we do, we should fail-safe (authorized + unauthorized = unauthorized)
+            { resource: 'some-other-namespace', privilege: 'login:', authorized: false },
+            { resource: 'some-other-namespace', privilege: 'login:', authorized: true },
+          ],
+        },
+      })
+    );
+
+    const result = await client.get(type, id, { namespace });
+    // we will never redact the "All Spaces" ID
+    expect(result).toEqual(expect.objectContaining({ namespaces: ['*', namespace, '?'] }));
+
+    expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(2);
+    expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith('login:', [
+      'some-other-namespace',
+    ]);
+  });
 });

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

@@ -96,9 +96,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     attributes: T = {} as T,
     options: SavedObjectsCreateOptions = {}
   ) {
+    const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
     try {
       const args = { type, attributes, options };
-      const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
       await this.ensureAuthorized(type, 'create', namespaces, { args });
     } catch (error) {
       this.auditLogger.log(
@@ -119,7 +119,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const savedObject = await this.baseClient.create(type, attributes, options);
-    return await this.redactSavedObjectNamespaces(savedObject);
+    return await this.redactSavedObjectNamespaces(savedObject, namespaces);
   }
 
   public async checkConflicts(
@@ -141,15 +141,12 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     objects: Array<SavedObjectsBulkCreateObject<T>>,
     options: SavedObjectsBaseOptions = {}
   ) {
+    const namespaces = objects.reduce(
+      (acc, { initialNamespaces = [] }) => acc.concat(initialNamespaces),
+      [options.namespace]
+    );
     try {
       const args = { objects, options };
-      const namespaces = objects.reduce(
-        (acc, { initialNamespaces = [] }) => {
-          return acc.concat(initialNamespaces);
-        },
-        [options.namespace]
-      );
-
       await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_create', namespaces, {
         args,
       });
@@ -176,7 +173,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const response = await this.baseClient.bulkCreate(objects, options);
-    return await this.redactSavedObjectsNamespaces(response);
+    return await this.redactSavedObjectsNamespaces(response, namespaces);
   }
 
   public async delete(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
@@ -255,7 +252,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       )
     );
 
-    return await this.redactSavedObjectsNamespaces(response);
+    return await this.redactSavedObjectsNamespaces(response, options.namespaces ?? [undefined]);
   }
 
   public async bulkGet<T = unknown>(
@@ -296,7 +293,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       )
     );
 
-    return await this.redactSavedObjectsNamespaces(response);
+    return await this.redactSavedObjectsNamespaces(response, [options.namespace]);
   }
 
   public async get<T = unknown>(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
@@ -323,7 +320,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       })
     );
 
-    return await this.redactSavedObjectNamespaces(savedObject);
+    return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
   }
 
   public async update<T = unknown>(
@@ -354,7 +351,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const savedObject = await this.baseClient.update(type, id, attributes, options);
-    return await this.redactSavedObjectNamespaces(savedObject);
+    return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
   }
 
   public async addToNamespaces(
@@ -363,9 +360,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     namespaces: string[],
     options: SavedObjectsAddToNamespacesOptions = {}
   ) {
+    const { namespace } = options;
     try {
       const args = { type, id, namespaces, options };
-      const { namespace } = options;
       // To share an object, the user must have the "share_to_space" permission in each of the destination namespaces.
       await this.ensureAuthorized(type, 'share_to_space', namespaces, {
         args,
@@ -401,7 +398,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const response = await this.baseClient.addToNamespaces(type, id, namespaces, options);
-    return await this.redactSavedObjectNamespaces(response);
+    return await this.redactSavedObjectNamespaces(response, [namespace, ...namespaces]);
   }
 
   public async deleteFromNamespaces(
@@ -438,20 +435,20 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const response = await this.baseClient.deleteFromNamespaces(type, id, namespaces, options);
-    return await this.redactSavedObjectNamespaces(response);
+    return await this.redactSavedObjectNamespaces(response, namespaces);
   }
 
   public async bulkUpdate<T = unknown>(
     objects: Array<SavedObjectsBulkUpdateObject<T>> = [],
     options: SavedObjectsBaseOptions = {}
   ) {
+    const objectNamespaces = objects
+      // The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
+      // in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
+      .filter(({ namespace }) => namespace !== undefined)
+      .map(({ namespace }) => namespace!);
+    const namespaces = [options?.namespace, ...objectNamespaces];
     try {
-      const objectNamespaces = objects
-        // The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
-        // in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
-        .filter(({ namespace }) => namespace !== undefined)
-        .map(({ namespace }) => namespace!);
-      const namespaces = [options?.namespace, ...objectNamespaces];
       const args = { objects, options };
       await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_update', namespaces, {
         args,
@@ -479,7 +476,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     );
 
     const response = await this.baseClient.bulkUpdate<T>(objects, options);
-    return await this.redactSavedObjectsNamespaces(response);
+    return await this.redactSavedObjectsNamespaces(response, namespaces);
   }
 
   public async removeReferencesTo(
@@ -617,32 +614,43 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
     return uniq(objects.map((o) => o.type));
   }
 
-  private async getNamespacesPrivilegeMap(namespaces: string[]) {
+  private async getNamespacesPrivilegeMap(
+    namespaces: string[],
+    previouslyAuthorizedSpaceIds: string[]
+  ) {
+    const namespacesToCheck = namespaces.filter(
+      (namespace) => !previouslyAuthorizedSpaceIds.includes(namespace)
+    );
+    const initialPrivilegeMap = previouslyAuthorizedSpaceIds.reduce(
+      (acc, spaceId) => acc.set(spaceId, true),
+      new Map<string, boolean>()
+    );
+    if (namespacesToCheck.length === 0) {
+      return initialPrivilegeMap;
+    }
     const action = this.actions.login;
-    const checkPrivilegesResult = await this.checkPrivileges(action, namespaces);
+    const checkPrivilegesResult = await this.checkPrivileges(action, namespacesToCheck);
     // check if the user can log into each namespace
-    const map = checkPrivilegesResult.privileges.kibana.reduce(
-      (acc: Record<string, boolean>, { resource, authorized }) => {
-        // there should never be a case where more than one privilege is returned for a given space
-        // if there is, fail-safe (authorized + unauthorized = unauthorized)
-        if (resource && (!authorized || !acc.hasOwnProperty(resource))) {
-          acc[resource] = authorized;
-        }
-        return acc;
-      },
-      {}
-    );
+    const map = checkPrivilegesResult.privileges.kibana.reduce((acc, { resource, authorized }) => {
+      // there should never be a case where more than one privilege is returned for a given space
+      // if there is, fail-safe (authorized + unauthorized = unauthorized)
+      if (resource && (!authorized || !acc.has(resource))) {
+        acc.set(resource, authorized);
+      }
+      return acc;
+    }, initialPrivilegeMap);
     return map;
   }
 
-  private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Record<string, boolean>) {
+  private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Map<string, boolean>) {
     return spaceIds
-      .map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : UNKNOWN_SPACE))
+      .map((x) => (x === ALL_SPACES_ID || privilegeMap.get(x) ? x : UNKNOWN_SPACE))
       .sort(namespaceComparator);
   }
 
   private async redactSavedObjectNamespaces<T extends SavedObjectNamespaces>(
-    savedObject: T
+    savedObject: T,
+    previouslyAuthorizedNamespaces: Array<string | undefined>
   ): Promise<T> {
     if (
       this.getSpacesService() === undefined ||
@@ -652,12 +660,18 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       return savedObject;
     }
 
-    const namespaces = savedObject.namespaces.filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
-    if (namespaces.length === 0) {
-      return savedObject;
-    }
+    const previouslyAuthorizedSpaceIds = previouslyAuthorizedNamespaces.map((x) =>
+      this.getSpacesService()!.namespaceToSpaceId(x)
+    );
+    // all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
+    const namespaces = savedObject.namespaces.filter(
+      (x) => x !== ALL_SPACES_ID && !previouslyAuthorizedSpaceIds.includes(x)
+    );
 
-    const privilegeMap = await this.getNamespacesPrivilegeMap(namespaces);
+    const privilegeMap = await this.getNamespacesPrivilegeMap(
+      namespaces,
+      previouslyAuthorizedSpaceIds
+    );
 
     return {
       ...savedObject,
@@ -666,20 +680,26 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
   }
 
   private async redactSavedObjectsNamespaces<T extends SavedObjectsNamespaces>(
-    response: T
+    response: T,
+    previouslyAuthorizedNamespaces: Array<string | undefined>
   ): Promise<T> {
     if (this.getSpacesService() === undefined) {
       return response;
     }
+
+    const previouslyAuthorizedSpaceIds = previouslyAuthorizedNamespaces.map((x) =>
+      this.getSpacesService()!.namespaceToSpaceId(x)
+    );
     const { saved_objects: savedObjects } = response;
+    // all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
     const namespaces = uniq(
       savedObjects.flatMap((savedObject) => savedObject.namespaces || [])
-    ).filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
-    if (namespaces.length === 0) {
-      return response;
-    }
+    ).filter((x) => x !== ALL_SPACES_ID && !previouslyAuthorizedSpaceIds.includes(x));
 
-    const privilegeMap = await this.getNamespacesPrivilegeMap(namespaces);
+    const privilegeMap = await this.getNamespacesPrivilegeMap(
+      namespaces,
+      previouslyAuthorizedSpaceIds
+    );
 
     return {
       ...response,