Sharing saved-objects phase 1 by jportner · Pull Request #54605 · elastic/kibana

jportner · 2020-01-13T17:19:17Z

Phase 1 from #27004.

Overview

Currently, saved objects can be namespace-agnostic (which resides outside of spaces), or single-namespace (which is isolated to a single space).

This PR adds a third type of saved object: multi-namespace (which resides in one or more namespaces). It also includes changes to the saved object repository, client, and client wrappers to include new behavior for this type of saved object.

Click for a list of changes...

Primary changes in core:

I updated SavedObjectsType (along with legacy SavedObjectsSchemaTypeDefinition) to include a new multiNamespace boolean attribute. Accordingly, I updated the SavedObjectTypeRegistry and legacy SavedObjectsSchema to add two new methods: isSingleNamespace and isMultiNamespace. These, along with the existing isNamespaceAgnostic method, all return boolean values and are all mutually exclusive.
I updated the SavedObject interface to include a new optional namespaces field. This is only used if the saved object type is multi-namespace.
I updated the SavedObjectsRepository to account for new behavior.
- Existing CRUD operations behave differently if the type is multi-namespace. For instance, deleting a multi-namespace saved object removes it from the current namespace, instead of completely deleting it. Also, creating a new multi-namespace saved object in a space will error with a conflict if an object with that type/id exists in a different space
- I added two new methods for addNamespaces and removeNamespaces, which can only be used for multi-namespace saved objects. Also updated the SavedObjectsClient to include these methods.

Primary changes in x-pack:

I updated EncryptedSavedObjectsClientWrapper to treat multi-namespace saved objects differently. These do not use the current space for the encryption's AAD.
I updated the SecureSavedObjectsClientWrapper to add the appropriate authorization checks for multi-namespace saved objects. To add a new space to an existing multi-namespace saved object, you must have "create" permissions in the source and destination spaces. For all other operations, you must have the appropriate permission in any of the saved object's spaces.
I overhauled/consolidated functionality for checking privileges and checking saved objects privileges. For a given resource, we need to be able to fetch all privileges for all spaces.
I added two new API routes, /api/spaces/_share_saved_object_add and /api/spaces/_share_saved_object_remove, and added corresponding methods in the SpacesSavedObjectClient.
I modified the SecurityAuditLogger to include more information about missing privileges -- it now the space for each privilege check.

Test Changes

I had to ensure we are exercising new test cases in unit tests and integration tests. When I started looking through these, some of the existing tests were stale or extremely verbose / hard to validate. I opted to completely rewrite some of these test suites.

Click for a list of changes...

Rewritten test suites:

src/core/server/saved_objects/service/lib/repository.test.js
src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts
x-pack/plugins/security/server/authorization/check_saved_objects_privileges.test.ts
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
x-pack/test/saved_object_api_integration/

jportner · 2020-01-13T17:20:10Z

Note: includes POC from #50404

Used array filtering to make the code more readable.

These are now called `addToNamespaces` and `deleteFromNamespaces` to better describe what they do. Also changed the names of the related Spaces integration test suites to reflect their routes -- these test suites are now called `share_add` and `share_remove`.

jportner · 2020-04-06T17:18:23Z

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


    let bulkRequestIndexCounter = 0;
    const bulkCreateParams: object[] = [];
-    const expectedBulkResults: Array<Either<any, any>> = expectedResults.map(


Note: the diff below this is messy, but code didn't change, just indentation.

pgayvallet

Second pass review of the platform owned files.

This was a beast. Some nits and questions, but overall I think it's LGTM.

Let see if @rudolf find anything I might have missed 😄

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

pgayvallet · 2020-04-07T07:50:39Z

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

+function getNamespaceString(namespace?: string) {
+  return namespace ?? 'default';
+}


NIT: getNamespaceOrDefault maybe?

Also, if default is used multiple times for some checks (did not look) I would create a const for it.

The string representation of the default (undefined) namespace is 'default'. I'll update the jsdoc to better describe this.

Edit: added in ec7fc3e.

pgayvallet · 2020-04-07T07:52:07Z

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

+  document?: SavedObjectsRawDoc
+): string[] | undefined {
+  if (document) {
+    return document._source.namespaces;


I saw document._source?.namespaces ?? [] in some places (I think?). We don't want it there?

We don't want to coalesce to an array here if the document has no namespaces, because of the ways that consumers use this function. However, I suppose it wouldn't hurt to include the optional chaining!

Edit: added in ec7fc3e.

legrego

Functional test coverage looks really good, nice job with this! I struggled to follow some of the logic for constructing the various permutations, but the tradeoff here is that the actual test definitions are more straightforward than before

x-pack/test/saved_object_api_integration/common/lib/saved_object_test_utils.ts

legrego · 2020-04-06T19:57:10Z

x-pack/test/saved_object_api_integration/common/lib/saved_object_test_utils.ts

+  interface Modifier {
+    modifier?: T;
+  }
+  interface Security extends Modifier {


nit: The naming here is confusing to me. An interface named Security extends another named Modifier -- this leads me to expect that this is some sort of "security modifier", but I'm not sure what it's modifying, or what security means in this context.

I don't have a concrete suggestion at the moment, so don't get too hung up on this

I had a few different iterations of this getTestScenarios method, and at one point I needed to define the interfaces this way to avoid duplicating a bunch of code. However, looking at it now I think I can get rid of the Modifier interface entirely and define these interfaces like so:

interface Security { modifier?: T; users: Record< | keyof typeof commonUsers | 'allAtDefaultSpace' | 'readAtDefaultSpace' | 'allAtSpace1' | 'readAtSpace1', TestUser >; } interface SecurityAndSpaces { modifier?: T; users: Record< keyof typeof commonUsers | 'allAtSpace' | 'readAtSpace' | 'allAtOtherSpace', TestUser >; spaceId: string; } interface Spaces { modifier?: T; spaceId: string; }

x-pack/test/saved_object_api_integration/common/lib/saved_object_test_utils.ts

legrego · 2020-04-07T11:58:20Z

x-pack/test/saved_object_api_integration/common/lib/types.ts

@@ -6,7 +6,28 @@

 export type DescribeFn = (text: string, fn: () => void) => void;


Why do we have our own definition for a Mocha test suite? Can we use Mocha.SuiteFunction instead?

😬

I went ahead and updated it, it passes the type checks so I think that's OK.

legrego · 2020-04-07T12:10:29Z

x-pack/test/saved_object_api_integration/common/lib/saved_object_test_utils.ts

+      message: `Unable to ${action} ${uniqueSorted.join()}`,
+    });
+  },
+  permitted: async (object: Record<string, any>, testCase: TestCase, expectSuccess?: any) => {


nit: expectSuccess reads like an optional boolean value, but it appears that's not the case. I'm having a hard time following, so I'm sure there's a reason, but why isn't the success condition part of the testCase, like the failure condition is?

This is an optional override to the default behavior of testing the success outcome. I went through several iterations of rewriting these test suites. Looking at them now it appears that expectSuccess is never used, so I can remove it.

I'll be honest, I am not 100% sure at this point why I had it defined this way, I believe at one point I had a need to modify the assertions for the success outcome within the common test suite files (as opposed to when the test case was defined), and it seemed better to do it this way.

Edit: this was actually being used in the delete test suite, because the success response for the delete API is an empty object. However, I still removed it from the test utils here and just added a conditional in the common delete test suite instead.

legrego · 2020-04-08T17:02:48Z

@elasticmachine merge upstream

legrego

LGTM once Rudolf approves. This was a monster of a PR, nice work! 👏 🚢

rudolf

🎉 Good job!

jportner · 2020-04-10T01:14:43Z

@elasticmachine merge upstream

kibanamachine · 2020-04-10T02:51:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c4c35c1

History

💚 Build #39666 succeeded c26a49d
💔 Build #39336 failed e3a5416
💚 Build #39249 succeeded ec7fc3e
💚 Build #39017 succeeded c0dd3b0
💔 Build #38972 failed e2f5d65

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

Remove `namespaceAgnostic` field that was deprecated in elastic#54605, use `namespaceType` instead.

Co-authored-by: kobelb <brandon.kobel@elastic.co>

Remove `namespaceAgnostic` field that was deprecated in #54605, use `namespaceType` instead.

jportner force-pushed the issue-54043-sharing-saved-objects-phase-1 branch 2 times, most recently from 4efc8d2 to 254f55c Compare February 5, 2020 19:24

streamich added the Feature:Saved Objects label Feb 7, 2020

jportner force-pushed the issue-54043-sharing-saved-objects-phase-1 branch 8 times, most recently from f37b241 to 9caba22 Compare February 14, 2020 18:58

jportner force-pushed the issue-54043-sharing-saved-objects-phase-1 branch 6 times, most recently from f467c4f to 947798f Compare February 26, 2020 18:16

jportner force-pushed the issue-54043-sharing-saved-objects-phase-1 branch from 947798f to 17e9c68 Compare February 27, 2020 14:03

jportner force-pushed the issue-54043-sharing-saved-objects-phase-1 branch 11 times, most recently from 63bb5b6 to 8544545 Compare March 10, 2020 18:21

jportner added 3 commits April 6, 2020 13:12

Clean up preflight checks for bulk operations

94d31ec

Used array filtering to make the code more readable.

Address remaining items from fifth round of PR review feedback

e2f5d65

jportner commented Apr 6, 2020

View reviewed changes

Fix integration test file names in test suites

c0dd3b0

pgayvallet approved these changes Apr 7, 2020

View reviewed changes

jportner added 2 commits April 7, 2020 09:52

Merge branch 'master' into pr/jportner/54605

65fac79

Address sixth round of PR review feedback

ec7fc3e

legrego reviewed Apr 7, 2020

View reviewed changes

jportner added 4 commits April 7, 2020 12:00

WIP

dbce4e8

Address nits on integration tests

e3a5416

Merge branch 'master' into pr/jportner/54605

7a8c11b

Fix more nits

223ecef

Merge branch 'master' into issue-54043-sharing-saved-objects-phase-1

c26a49d

legrego approved these changes Apr 8, 2020

View reviewed changes

rudolf approved these changes Apr 9, 2020

View reviewed changes

Merge branch 'master' into pr/jportner/54605

d24fa35

Merge branch 'master' into issue-54043-sharing-saved-objects-phase-1

c4c35c1

jportner merged commit 97d1685 into elastic:master Apr 10, 2020

jportner deleted the issue-54043-sharing-saved-objects-phase-1 branch April 10, 2020 03:18

jportner mentioned this pull request Apr 10, 2020

[7.x] Sharing saved-objects phase 1 (#54605) #63215

Merged

jportner added a commit to jportner/kibana that referenced this pull request Apr 10, 2020

Refactor saved object types to use namespaceType

9d1e8b1

Remove `namespaceAgnostic` field that was deprecated in elastic#54605, use `namespaceType` instead.

jportner mentioned this pull request Apr 10, 2020

Refactor saved object types to use namespaceType #63217

Merged

jportner added a commit that referenced this pull request Apr 10, 2020

[7.x] Sharing saved-objects phase 1 (#54605) (#63215)

a0bde86

Co-authored-by: kobelb <brandon.kobel@elastic.co>

jportner added the backported label Apr 10, 2020

jportner added a commit that referenced this pull request Apr 16, 2020

Refactor saved object types to use namespaceType (#63217)

d72de0e

Remove `namespaceAgnostic` field that was deprecated in #54605, use `namespaceType` instead.

jportner added a commit that referenced this pull request Apr 16, 2020

[7.x] Refactor SO types to use namespaceType (#63217) (#63745)

f6cb14a

Remove `namespaceAgnostic` field that was deprecated in #54605, use `namespaceType` instead.

		@@ -6,7 +6,28 @@

		export type DescribeFn = (text: string, fn: () => void) => void;

Conversation

jportner commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Primary changes in core:

Primary changes in x-pack:

Test Changes

Uh oh!

jportner commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Apr 7, 2020 • 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!

jportner Apr 7, 2020 • 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!

legrego left a comment

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!

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!

jportner Apr 7, 2020 • 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!

legrego commented Apr 8, 2020

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

jportner commented Apr 10, 2020

Uh oh!

kibanamachine commented Apr 10, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jportner commented Jan 13, 2020 •

edited

Loading

jportner Apr 7, 2020 •

edited

Loading

jportner Apr 7, 2020 •

edited

Loading

jportner Apr 7, 2020 •

edited

Loading