From e90b9005de3b3d49f4a16c6d529086e724423d02 Mon Sep 17 00:00:00 2001
From: Joe Portner <5295965+jportner@users.noreply.github.com>
Date: Wed, 25 Aug 2021 11:09:28 -0400
Subject: [PATCH] Update authZ checks for SpacesSavedObjectsClient

1. Removed previous changes to pass unit tests, changed the tests
   instead. The tests were meant to be asserting for the "403 Forbidden"
   error.
2. Updated bulkGet to replace `'*'` with an empty namespaces array when
   a user is not authorized to access any spaces. This is primarily so
   that the API is consistent (the Security plugin will still check
   that they are authorized to access the type in the active space and
   return a more specific 403 error if not).
---
 .../spaces_saved_objects_client.test.ts       | 26 +++++++++++++++++--
 .../spaces_saved_objects_client.ts            | 14 +++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts
index c1c5e63d5ae6e..980682fcfce3c 100644
--- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts
+++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts
@@ -189,6 +189,28 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
         );
         expect(spacesClient.getAll).toHaveBeenCalledTimes(1);
       });
+
+      test(`replaces object namespaces '*' with an empty array when the user doesn't have access to any spaces`, async () => {
+        const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
+        spacesClient.getAll.mockRejectedValue(Boom.forbidden());
+
+        const objects = [
+          { type: 'foo', id: '1', namespaces: ['*'] },
+          { type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] },
+          { type: 'baz', id: '3', namespaces: ['another-space'] },
+        ];
+        await client.bulkGet(objects);
+
+        expect(baseClient.bulkGet).toHaveBeenCalledWith(
+          [
+            { type: 'foo', id: '1', namespaces: [] },
+            { type: 'bar', id: '2', namespaces: [] },
+            { type: 'baz', id: '3', namespaces: ['another-space'] }, // even if another space doesn't exist, it can be specified explicitly
+          ],
+          { namespace: currentSpace.expectedNamespace }
+        );
+        expect(spacesClient.getAll).toHaveBeenCalledTimes(1);
+      });
     });
 
     describe('#find', () => {
@@ -207,7 +229,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
 
       test(`returns empty result if user is unauthorized in any space`, async () => {
         const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
-        spacesClient.getAll.mockRejectedValue(Boom.unauthorized());
+        spacesClient.getAll.mockRejectedValue(Boom.forbidden());
 
         const options = Object.freeze({ type: 'foo', namespaces: ['some-ns'] });
         const actualReturnValue = await client.find(options);
@@ -560,7 +582,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
 
       test(`throws error if if user is unauthorized in any space`, async () => {
         const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
-        spacesClient.getAll.mockRejectedValue(Boom.unauthorized());
+        spacesClient.getAll.mockRejectedValue(Boom.forbidden());
 
         await expect(
           client.openPointInTimeForType('foo', { namespaces: ['bar'] })
diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
index 868c70b30306d..c3b208029c8ac 100644
--- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
+++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
@@ -179,7 +179,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
     try {
       namespaces = await this.getSearchableSpaces(options.namespaces);
     } catch (err) {
-      if (Boom.isBoom(err) && err.output.payload.statusCode === 401) {
+      if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
         // return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations
         return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
       }
@@ -222,7 +222,15 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
     let availableSpaces: string[] | undefined;
     const getAvailableSpaces = async () => {
       if (!availableSpaces) {
-        availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]);
+        try {
+          availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]);
+        } catch (err) {
+          if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
+            availableSpaces = []; // the user doesn't have access to any spaces
+          } else {
+            throw err;
+          }
+        }
       }
       return availableSpaces;
     };
@@ -405,7 +413,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
     try {
       namespaces = await this.getSearchableSpaces(options.namespaces);
     } catch (err) {
-      if (Boom.isBoom(err) && err.output.payload.statusCode === 401) {
+      if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
         // throw bad request since the user is unauthorized in any space
         throw SavedObjectsErrorHelpers.createBadRequestError();
       }