Add multiple namespaces support to PIT search and finder #109062

pgayvallet · 2021-08-18T08:59:06Z

Summary

Fix #99044

Add multi-namespaces support to openPointInTimeForType and createPointInTimeFinder

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pgayvallet

This is a rough draft of what I think the implementation should look like given the info provided in #99044.

A question I have regarding the issue's description is regarding this point:

soClient.createPointInTimeFinder will need to ensure the typeToNamespacesMap is handed off when calling openPointInTimeForType

from my current understanding, createPointInTimeFinder would remain unchanged, as the logic change would be handled in the internal calls of PointInTimeFinder to client.openPointInTimeForType which would leverage the security wrapper's modifications? Could you confirm this?

@jportner @lukeelmers could you take a look and tell me if this seems like the correct approach?

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

pgayvallet · 2021-08-18T09:05:37Z

src/core/server/saved_objects/service/saved_objects_client.ts

 * @public
 */
-export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOptions {
+export interface SavedObjectsOpenPointInTimeOptions {


We no longer extend SavedObjectsBaseOptions to remove the namespace option, as this new namespaces option was added.

See my comment in #109062 (review), I think we should leave the SavedObjectsOpenPointInTimeOptions options as-is.

I agree with your approach, so typeToNamespacesMap is no longer necessary here and I will remove it. However SavedObjectsOpenPointInTimeOptions had a namespace?: string option due to this extension, that needs to be changed to a namespaces?: string[], for the security wrapper to use it.

Oh, I was going too fast, yep agreed

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

pgayvallet · 2021-08-18T09:09:31Z

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

-      throw error;
+      throw SavedObjectsErrorHelpers.decorateForbiddenError(new Error(status));


In the same scenario, the find wrapper returns SavedObjectsUtils.createEmptyFindResponse, however this is not possible for openPointInTimeForType, as the expected return type is a SavedObjectsOpenPointInTimeResponse. I'm throwing a forbidden error in that case, but really not sure this is the correct error type?

FWIW this makes sense to me: If you are opening a PIT against a type & you don't have permissions to access that type's backing index, I'd expect a forbidden error. But let's see what @jportner thinks.

Yeah, the forbidden error seems like the best approach here. Note that if you are only authorized to access a type in space A, and you attempt to open a PIT for that type only in space B, you'll run into a forbidden error. But that seems acceptable.

if you are only authorized to access a type in space A, and you attempt to open a PIT for that type only in space B, you'll run into a forbidden error. But that seems acceptable.

I would even think that this is expected and not just acceptable? OTOH that's right that it differs from the find behavior that just returns an empty find response without throwing in the same situation.

The alternative would be to call baseClient.openPointInTimeForType with an empty list of types, and have it return a stubbed PIT finder impl that returns no results in that case, but that feels a little over-complicated to me?

Hum, actually we can't do that, as it would need to be done in createPointInTimeFinder, so it doesn't solve the issue when calling openPointInTimeForType directly. So throwing here seems the only viable option anyway

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

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

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

lukeelmers

Overall this makes sense to me, but will defer to @jportner to make sure the behavior is what we'd expect here.

from my current understanding, createPointInTimeFinder would remain unchanged, as the logic change would be handled in the internal calls of PointInTimeFinder to client.openPointInTimeForType which would leverage the security wrapper's modifications? Could you confirm this?

Yes, this sounds correct to me as the typeToNamespacesMap would be handled by the wrapper. That issue description was written without doing an actual POC, so most likely I had overlooked this consideration.

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

lukeelmers · 2021-08-18T15:07:54Z

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

-      throw error;
+      throw SavedObjectsErrorHelpers.decorateForbiddenError(new Error(status));


FWIW this makes sense to me: If you are opening a PIT against a type & you don't have permissions to access that type's backing index, I'd expect a forbidden error. But let's see what @jportner thinks.

jportner

I agree with most of the approach, with one exception:

In the SSOCW, in the openPointInTimeForType function, I don't think we need to pass typeToNamespacesMap down to the base client at all. When the user is partially or fully authorized, we should just pass down the types that the user can access.

The repository just needs to know what types the user is authorized to access in some space so that it can open the PIT against the appropriate indices for those types.
Re-reading the issue, Luke suggested this as an option in #99044 (comment):

Don't implement typeToNamespaceMap in openPointInTimeForType, and simply open the PIT against the indices for all allowed types in the request. If permissions change later, the correct namespace filters should still be applied in find. The tradeoff is that you are allowing the PIT to be open against some types which could potentially not be allowed in any namespaces, however as you aren't able to query those with find, this may be fine.

I think this approach is simpler and doesn't compromise security. See my suggestion below: #109062 (comment)

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

…s-pit-search

…changes

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

pgayvallet

self-review (in addition to the previous, non-closed points)

pgayvallet · 2021-08-20T12:39:14Z

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

   * @returns {promise} - { saved_objects: [{ id, type, version, attributes }], total, per_page, page }
   */
  public async find<T = unknown, A = unknown>(options: SavedObjectsFindOptions) {
-    throwErrorIfNamespaceSpecified(options);


Not sure why this was there, there isn't a namespace property on SavedObjectsFindOptions

Dead code! thanks for removing it

pgayvallet · 2021-08-20T12:46:21Z

x-pack/test/saved_object_api_integration/common/suites/export.ts

+        if (id) {
+          // "bulkGet" was unauthorized, which returns a forbidden error
+          await expectSavedObjectForbiddenBulkGet(type)(response);
+        } else {
+          expect(response.body).to.eql({
+            statusCode: 403,
+            error: 'Forbidden',
+            message: `unauthorized`,
+          });
+        }


Previously, per-id unauthorized were returning a 403 (due to bulkGet returning an error) and per-type were returning an empty export instead.

This PR has the side effect to change that, to now returns a 403 for per-type export when the user don't have the permissions for any of the types.

Overall, I think this is an improvement in term of consistency.

elasticmachine · 2021-08-20T12:47:35Z

Pinging @elastic/kibana-core (Team:Core)

rudolf · 2021-08-20T15:07:02Z

src/core/server/saved_objects/service/saved_objects_client.ts

   */
  preference?: string;
+  /**
+   * An optional list of namespaces to be used by the PIT.


Our separation between core / security makes documentation really hard here. But in the absence of another good place to write documentation it feels like we should maybe add some details about how this option behaves when the spaces plugin is enabled? Otherwise it's very difficult to figure out how to use this.

Suggested change

* An optional list of namespaces to be used by the PIT.

* An optional list of namespaces to be used by the PIT.

*

* When the spaces plugin is enabled:

*. - this will default to the user's current space as determined by the URL

* - if namespaces is specified the user's current space will be ignored

* - `['*']` will search across all available spaces

Yea, that's fair. I can't think of any better place...

lukeelmers

Changes here all LGTM, thanks for taking this one on 🚀

jportner

Minor nits below, otherwise LGTM, thanks for doing this!

jportner · 2021-08-20T21:10:18Z

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

    type: string | string[],
    options: SavedObjectsOpenPointInTimeOptions = {}
  ) {
    throwErrorIfNamespaceSpecified(options);


You can get rid of this too

jportner · 2021-08-20T21:18:45Z

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

      });

-      test(`supplements options with the current namespace`, async () => {
+      test(`translates options.namespace: ['*']`, async () => {


Nit

Suggested change

test(`translates options.namespace: ['*']`, async () => {

test(`translates options.namespaces: ['*']`, async () => {

jportner · 2021-08-20T21:21:28Z

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

-          foo: 'bar',
-          namespace: currentSpace.expectedNamespace,
+          keepAlive: '2m',
+          namespaces: [currentSpace.expectedNamespace ?? 'default'],


Nit

Suggested change

namespaces: [currentSpace.expectedNamespace ?? 'default'],

namespaces: [currentSpace.expectedNamespace ?? DEFAULT_SPACE_ID],

…s-pit-search

kibanamachine · 2021-08-23T09:43:44Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2244	2245	+1

History

💚 Build #147416 succeeded 16c0b0a
💚 Build #147294 succeeded b9a1b38
💚 Build #147271 succeeded 3024a47
💚 Build #147234 succeeded 7f7d453
💔 Build #147227 failed 3c0e37c

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

) * initial modifications * change approach for openPointInTime and add tests for spaces wrapper changes * fix and add security wrapper tests * fix export security FTR tests * update generated doc * add tests for PIT finder * NIT * improve doc * nits

kibanamachine · 2021-08-23T10:06:20Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…109603) * initial modifications * change approach for openPointInTime and add tests for spaces wrapper changes * fix and add security wrapper tests * fix export security FTR tests * update generated doc * add tests for PIT finder * NIT * improve doc * nits Co-authored-by: Pierre Gayvallet <pierre.gayvallet@gmail.com>

initial modifications

15af802

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.16.0 Feature:Saved Objects labels Aug 18, 2021

pgayvallet commented Aug 18, 2021

View reviewed changes

pgayvallet requested review from jportner and lukeelmers August 18, 2021 09:17

pgayvallet added the Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Aug 18, 2021

lukeelmers reviewed Aug 18, 2021

View reviewed changes

jportner reviewed Aug 19, 2021

View reviewed changes

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts Outdated Show resolved Hide resolved

pgayvallet added 3 commits August 20, 2021 08:55

Merge remote-tracking branch 'upstream/master' into kbn-99044-multi-n…

315d9b2

…s-pit-search

change approach for openPointInTime and add tests for spaces wrapper …

3fbfcad

…changes

fix and add security wrapper tests

3c0e37c

pgayvallet commented Aug 20, 2021

View reviewed changes

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts Show resolved Hide resolved

pgayvallet added 4 commits August 20, 2021 12:29

fix export security FTR tests

edf1a19

update generated doc

7f7d453

add tests for PIT finder

3024a47

NIT

b9a1b38

pgayvallet commented Aug 20, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review August 20, 2021 12:47

pgayvallet requested review from a team as code owners August 20, 2021 12:47

pgayvallet requested review from jportner and lukeelmers August 20, 2021 12:47

rudolf reviewed Aug 20, 2021

View reviewed changes

improve doc

16c0b0a

lukeelmers approved these changes Aug 20, 2021

View reviewed changes

rudolf approved these changes Aug 20, 2021

View reviewed changes

jportner approved these changes Aug 20, 2021

View reviewed changes

pgayvallet added 2 commits August 23, 2021 09:06

Merge remote-tracking branch 'upstream/master' into kbn-99044-multi-n…

e3ef2e7

…s-pit-search

nits

9d5fb6f

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

pgayvallet merged commit 5c5e191 into elastic:master Aug 23, 2021

kibanamachine mentioned this pull request Aug 23, 2021

[7.x] Add multiple namespaces support to PIT search and finder (#109062) #109603

Merged

pgayvallet mentioned this pull request Dec 6, 2021

use PIT search for savedObjectTagging plugin #120476

Merged

jportner mentioned this pull request Apr 29, 2022

Allow the SavedObjectsClient to work outside of a space #131254

Open

		throw error;
		throw SavedObjectsErrorHelpers.decorateForbiddenError(new Error(status));

-   * An optional list of namespaces to be used by the PIT.
+   * An optional list of namespaces to be used by the PIT.
+   *
+   * When the spaces plugin is enabled:
+   *.  - this will default to the user's current space as determined by the URL
+   *  - if namespaces is specified the user's current space will be ignored
+   *  - `['*']` will search across all available spaces

	test(`translates options.namespace: ['*']`, async () => {
	test(`translates options.namespaces: ['*']`, async () => {

	namespaces: [currentSpace.expectedNamespace ?? 'default'],
	namespaces: [currentSpace.expectedNamespace ?? DEFAULT_SPACE_ID],

Add multiple namespaces support to PIT search and finder #109062

Add multiple namespaces support to PIT search and finder #109062

Uh oh!

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

pgayvallet 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Aug 19, 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!

pgayvallet Aug 20, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Aug 19, 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!

pgayvallet Aug 20, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner 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!

Uh oh!

Uh oh!

pgayvallet 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!

elasticmachine commented Aug 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Aug 20, 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!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pgayvallet commented Aug 18, 2021 •

edited

Loading

pgayvallet left a comment •

edited

Loading

jportner Aug 19, 2021 •

edited

Loading

pgayvallet Aug 20, 2021 •

edited

Loading

jportner Aug 19, 2021 •

edited

Loading

pgayvallet Aug 20, 2021 •

edited

Loading

jportner left a comment •

edited

Loading

pgayvallet Aug 20, 2021 •

edited

Loading