bulkGet saved objects across spaces by jportner · Pull Request #109967 · elastic/kibana

jportner · 2021-08-24T21:27:18Z

Summary

These unit tests were not using mocks correctly, so they passed when they should not have: * '#find returns empty result if user is unauthorized in any space' * '#openPointInTimeForType throws error if if user is unauthorized in any space' The previous refactor fixed the mocks, which caused these two tests to start failing. In this commit, I have fixed the code so the tests pass as written.

pgayvallet · 2021-08-25T12:35:17Z

Did not look at implementation in depth for now, but API looks good

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).

At the repository level, if a user tries to bulkGet an isolated object in multiple spaces (explicitly defined, or '*') they will get a 400 Bad Request error. However, in the Spaces SOC wrapper we are deconstructing the '*' identifier and replacing it with all available spaces. This can cause a problem when the user has access to only one space, the API response would be inconsistent (the repository would treat this as a perfectly valid request and attempt to fetch the object.) So, now in the Spaces SOC wrapper we modify the results based on what the request, leaving any 403 errors from the Security SOC wrapper intact.

jportner

Author's notes for reviewers.

Note: I looked at the EncryptedSavedObjects SOC wrapper and it doesn't need any changes, since it just takes the returned objects and decrypts the attributes accordingly.

jportner · 2021-08-25T20:06:55Z

x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_create.ts

  ];
  const hiddenType = [{ ...CASES.HIDDEN, ...fail400() }];
-  const allTypes = normalTypes.concat(hiddenType);
+  const allTypes = [...normalTypes, ...crossNamespace, ...hiddenType];


This change is unrelated to the rest of this PR, but as I was looking at these test cases I noticed that the crossNamespace test cases were not included in the allTypes superset. That meant they were getting skipped for the superuser.

jportner · 2021-08-25T20:09:32Z

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

+      return availableSpacesPromise;
+    };
+
+    const expectedResults = await Promise.all(


This is ugly but necessary to keep the API responses consistent for various user- and space-combinations. See the individual commit messages (5e5e00c, f224966) and integration tests for more info.

jportner · 2021-08-25T20:11:07Z

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

-    const namespaces = await this.getSearchableSpaces(options.namespaces);
+    let namespaces: string[];
+    try {
+      namespaces = await this.getSearchableSpaces(options.namespaces);
+    } catch (err) {
+      if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
+        // throw bad request since the user is unauthorized in any space
+        throw SavedObjectsErrorHelpers.createBadRequestError();
+      }
+      throw err;
+    }


The unit test for this (and for find) were not working correctly (see e6588b7). I fixed the tests and added this bit to match the behavior to the unit tests.

jportner · 2021-08-25T23:33:58Z

@elasticmachine merge upstream

jportner · 2021-08-26T04:16:34Z

@elasticmachine merge upstream

pgayvallet

Overall LGTM. A few remarks and questions

pgayvallet · 2021-08-26T06:56:06Z

x-pack/test/saved_object_api_integration/security_only/apis/bulk_get.ts

+    { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, namespaces: [SPACE_1_ID] }, // second try searches for it in a single other space, which is valid
+    { ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, namespaces: [SPACE_2_ID], ...fail404() },


I was just wondering: our bulkGet security suite is only performing tests where it bulkGets a single object, right 😅 ?

All of the bulk* SO test suites behave as follows:

For non-forbidden cases (successes and object-specific errors), batch them into a single request

For forbidden cases (where the entire request fails with a 403), test each case individually

Note this is not automated in any way but it is controlled by use of the singleRequest option in each test definition.

x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts

src/core/server/saved_objects/service/lib/internal_utils.ts

pgayvallet · 2021-08-26T07:10:30Z

src/core/server/saved_objects/service/lib/internal_utils.ts

+    return true;
+  }
+
+  const namespacesToCheck = new Set(namespaces);


NIT: I don't think there's any need to convert to a set here? Can't we just work with the initial array?

Yeah, had the same thought. The only corner case I can imagine where it'd be beneficial to have a Set is if we have large existingNamespaces and namespaces lists that have intersection only at the end of these lists (and bulkGet is large enough to have a significant compound effect), but it probably doesn't make sense to cater for that.

Having a set here will not completely protect us from a malicious actor anyway, they can just do something like this...., for every ID:

const namespaces = [...Array.from({ length: 1000000 }).map(() => Math.random().toString()), 'valid-ns']

The purpose of the Set here is to reduce the time complexity, this takes us down from O(n^2) to O(n). This is because line 179 below loops through existingNamespaces and checks to see if any existing space is present in namespacesToCheck.

pgayvallet · 2021-08-26T07:21:33Z

src/core/server/saved_objects/service/lib/internal_utils.test.ts

+      // documents with the correct namespace prefix. We may revisit this in the future.
+      const doc1 = createRawDoc(SINGLE_NAMESPACE_TYPE, { namespace: 'some-space' }); // the namespace field is ignored
+      const doc2 = createRawDoc(SINGLE_NAMESPACE_TYPE, { namespaces: ['some-space'] }); // the namespaces field is ignored
+      expect(rawDocExistsInNamespaces(registry, doc1, [])).toBe(true);


Just a remark: We already talked about it in a similar function from a previous PR, but I still don't like the way single-NS docs are short-circuiting these functions, as it could lead to errors if a developer decides to use it elsewhere than in a part of the code where we're already assured that the fetched single-NS objects are of the correct requested space.

I'll one-up you, I don't like single-NS docs at all 😄

pgayvallet · 2021-08-26T07:35:39Z

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

      await this.legacyEnsureAuthorized(
        this.getUniqueObjectTypes(objects),
        'bulk_get',
-        options.namespace,
+        namespaces,
        {
          args,
        }


TIL: I thought we were doing per-object security checks and returning per-object unauthorized errors (as we do for 404)

Nope! If the user is unauthorized for part of the request, the entire request fails 😄

pgayvallet · 2021-08-26T07:48:36Z

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

+    let availableSpacesPromise: Promise<string[]> | undefined;
+    const getAvailableSpaces = async () => {
+      if (!availableSpacesPromise) {
+        availableSpacesPromise = this.getSearchableSpaces([ALL_SPACES_ID]).catch((err) => {
+          if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {


Can't we just

const getAvailableSpaces = async () => { ... } const availableSpaces = await getAvailableSpaces();

instead of using Promise.all around the expectedResults ? current implementation seems more complex than necessary.

Or something like this to block on this only when necessary:

const availableSpaces = objects.some((object) => object.namespaces?.includes(ALL_SPACES_ID)) ? await this.getSearchableSpaces([ALL_SPACES_ID]).catch((err) => { // the user doesn't have access to any spaces if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { return []; } throw err; }) : [];

Yeah the idea here was not to fetch all available spaces if we didn't need to, since that is a potentially expensive operation.

We could go through @azasypkin's suggestion but it effectively means we loop through all objects' namespaces fields twice in the worst case scenario. I'm on the fence about changing it.

We could go through @azasypkin's suggestion but it effectively means we loop through all objects' namespaces fields twice in the worst case scenario.

My assumption was that the time it takes to iterate through objects in request is negligible comparing to the request execution time, but it's just not-validated assumption, feel free to pick whatever approach you feel is better!

pgayvallet · 2021-08-26T07:51:56Z

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

+        if (isLeft(expectedResult) && actualResult?.error?.statusCode !== 403) {
+          const { type, id } = expectedResult.value;
+          return ({
+            type,
+            id,
+            error: SavedObjectsErrorHelpers.createBadRequestError(
+              '"namespaces" can only specify a single space when used with space-isolated types'
+            ).output.payload,
+          } as unknown) as SavedObject<T>;
+        }


Thinking out loud here, but I wonder if we would be able to perform this check before the underlying call to client.bulkGet?. I think we have all the necessary informations without performing the call?

In that case, we could potentially only bulkGet the objects that are not in that situation, and aggregate with the ones that are matching this case? May be premature optimization though.

The original implementation that I wrote used that approach -- any object that we predicted should result in a bad request, it skipped for the base client bulkGet.

The problem is that we really need to factor in behavior of both the Secure SOC wrapper and the SOR. If the user is unauthorized to get a specific type, then the request would never reach the SOR level validation. So that original implementation caused even more irregular API responses (and made the integration tests fail spectacularly).

However: I realized as I wrote this comment that the && actualResult?.error?.statusCode !== 403 bit is not necessary; if the user is unauthorized to fetch one specific object, the whole operation fails, so that code would never be reached anyway. So I'll take that bit out 😄

azasypkin

Code LGTM 👍

azasypkin · 2021-08-26T09:34:58Z

src/core/server/saved_objects/service/lib/repository.ts

+    const getNamespaceId = (namespaces?: string[]) =>
+      namespaces !== undefined ? SavedObjectsUtils.namespaceStringToId(namespaces[0]) : namespace;


note: no need to change anything, just wanted to note that if the reader (okay, that was me) doesn't know the internal implementation of generateRawId by heart they may be confused by the SavedObjectsUtils.namespaceStringToId(namespaces[0]) for shareable objects as it gives impression that we search only in the very first namespace 🙂

Yea, I had to check the implementation too to be sure 😄

I added a comment here to clarify.

azasypkin · 2021-08-26T10:09:47Z

src/core/server/saved_objects/service/lib/internal_utils.ts

+    return true;
+  }
+
+  const namespacesToCheck = new Set(namespaces);


Yeah, had the same thought. The only corner case I can imagine where it'd be beneficial to have a Set is if we have large existingNamespaces and namespaces lists that have intersection only at the end of these lists (and bulkGet is large enough to have a significant compound effect), but it probably doesn't make sense to cater for that.

Having a set here will not completely protect us from a malicious actor anyway, they can just do something like this...., for every ID:

const namespaces = [...Array.from({ length: 1000000 }).map(() => Math.random().toString()), 'valid-ns']

src/core/server/saved_objects/service/lib/internal_utils.test.ts

azasypkin · 2021-08-26T10:48:58Z

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

+    let availableSpacesPromise: Promise<string[]> | undefined;
+    const getAvailableSpaces = async () => {
+      if (!availableSpacesPromise) {
+        availableSpacesPromise = this.getSearchableSpaces([ALL_SPACES_ID]).catch((err) => {
+          if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {


Or something like this to block on this only when necessary:

const availableSpaces = objects.some((object) => object.namespaces?.includes(ALL_SPACES_ID)) ? await this.getSearchableSpaces([ALL_SPACES_ID]).catch((err) => { // the user doesn't have access to any spaces if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { return []; } throw err; }) : [];

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

kibanamachine · 2021-08-26T15:26:30Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2246	2247	+1

History

💚 Build #148803 succeeded c738184
💔 Build #148776 failed 8d0bfa4
💔 Build #148719 failed 1215e2c
💔 Build #148381 failed 9f6efb6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-08-26T15:38:06Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner added 3 commits August 24, 2021 15:53

Refactor SpacesSavedObjectsClient unit tests

0b30d72

Add per-object namespaces field for bulkGet

9f6efb6

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Aug 24, 2021

jportner changed the title ~~bulkGet across spaces~~ bulkGet saved objects across spaces Aug 24, 2021

jportner requested a review from pgayvallet August 24, 2021 21:27

jportner added 5 commits August 25, 2021 10:23

Add missing bulkCreate test cases

beedf6f

Add API integration test cases

f224966

Merge branch 'master' into issue-109197-bulkget-across-spaces

1215e2c

jportner commented Aug 25, 2021

View reviewed changes

jportner marked this pull request as ready for review August 25, 2021 20:13

jportner requested review from a team as code owners August 25, 2021 20:13

Merge branch 'master' into issue-109197-bulkget-across-spaces

8d0bfa4

Merge branch 'master' into issue-109197-bulkget-across-spaces

c738184

pgayvallet approved these changes Aug 26, 2021

View reviewed changes

azasypkin approved these changes Aug 26, 2021

View reviewed changes

jportner added 2 commits August 26, 2021 08:28

Merge branch 'master' into issue-109197-bulkget-across-spaces

dcec828

PR review feedback

fab5c21

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 26, 2021

jportner enabled auto-merge (squash) August 26, 2021 14:11

jportner merged commit 695280b into elastic:master Aug 26, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 26, 2021

bulkGet saved objects across spaces (elastic#109967)

1f1d829

kibanamachine mentioned this pull request Aug 26, 2021

[7.x] bulkGet saved objects across spaces (#109967) #110270

Merged

kibanamachine added a commit that referenced this pull request Aug 26, 2021

bulkGet saved objects across spaces (#109967) (#110270)

247f610

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

This was referenced Aug 30, 2021

[Discuss] - Move Spaces functionality into core #110492

Open

[SOR] use initialNamespaces when checking for conflict for create and bulkCreate #111023

Merged

pgayvallet mentioned this pull request Sep 3, 2021

[7.15] normalize initialNamespaces (#110936) #111032

Closed

joshdover mentioned this pull request Sep 3, 2021

Handle bulkGet errors on package retrieval from ES storage #111114

Merged

jportner deleted the issue-109197-bulkget-across-spaces branch January 19, 2022 20:46

jportner mentioned this pull request Apr 29, 2022

Allow the SavedObjectsClient to work outside of a space #131254

Open

		{ ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, namespaces: [SPACE_1_ID] }, // second try searches for it in a single other space, which is valid
		{ ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, namespaces: [SPACE_2_ID], ...fail404() },

		const getNamespaceId = (namespaces?: string[]) =>
		namespaces !== undefined ? SavedObjectsUtils.namespaceStringToId(namespaces[0]) : namespace;

Conversation

jportner commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

pgayvallet commented Aug 25, 2021

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner commented Aug 25, 2021

Uh oh!

jportner commented Aug 26, 2021

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Aug 26, 2021

💚 Build Succeeded

Metrics [docs]

API count

History

Uh oh!

kibanamachine commented Aug 26, 2021

jportner commented Aug 24, 2021 •

edited

Loading

jportner Aug 26, 2021 •

edited

Loading

azasypkin left a comment •

edited

Loading