Optimize preflight checks for SavedObjectsClient wrappers

[jportner]

Currently, the SavedObjectsRepository (SOR) conducts "pre-flight" checks on saved objects for write operations:

• create
• bulkCreate
• update
• bulkUpdate
• delete
• incrementCounter
• updateObjectsSpaces

Only multi-namespace saved object types will trigger a pre-flight check, because we need to ensure that the target object exists in the current space before allowing the operation to proceed. Single-namespace types do not require such a check because ID serialization ensures that changes are isolated to the current space.

In #102950, we have identified a need to add pre-flight checks to the Secure SavedObjectsClient (SOC) wrapper too. We should consider optimizing pre-flight checks so that a single check can be used for the SOR and its SOC wrappers. We have already implemented a workaround for this problem in the updateObjectsSpaces method, but there may be a better way to handle this across the board.

Benefits from such optimization will also be realized when implementing #82725.

Some ideas:

• Change the SOR to add "internal" options for these methods that the SOC wrappers can use to indicate a pre-flight check has already been made for target object(s). This means that if the SOC wrapper deems it appropriate to make a pre-flight check, the SOR will honor that result and it will not make an additional pre-flight check; this is the approach currently used by updateObjectsSpaces.
• Change the SOR to expose a callback with the pre-flight check results for the SOC wrappers to consume Edit: this was worded poorly, what I should have said was: Change the SOR to allow the SOC wrappers to pass down a callback to handle the pre-flight check results.
• ???

[elasticmachine]

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

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[pgayvallet]

This could be a very long discussion, but:

To be honest, in my opinion, the correct solution would probably be to have distinct APIs between the exposed API of the SOR/SOC, and the internal API between the SOR and its wrappers. That would allow to follow what was done for updateObjectsSpaces without leaking these internal parameters to the public API, by allowing the wrapper to use internal options not exposed on the public types/contracts

However this is way easier said than done, as the wrappers are optional, so we would need some sort of facade on top of the SOR when accessing it directly without wrappers, which seems basically like a full rewrite of a lot of stuff, which doesn't sound very realistic right now.

So, if we exclude this option,

Please correct me if anything is wrong hereIf

Change the SOR to add "internal" options for these methods that the SOC wrappers can use to indicate a pre-flight check has already been made for target object(s). This means that if the SOC wrapper deems it appropriate to make a pre-flight check, the SOR will honor that result and it will not make an additional pre-flight check; this is the approach currently used by updateObjectsSpaces.

Not a big fan of that thing (already said so when we did implement the workaround for updateObjectsSpaces).

• it leaks internal options to the public API
• it introduce duplication, as the preflight check implementation is distinct between the SOR and the wrappers requiring to also perform them.

However, the main upside I see is that the preflight checks are effectively performed only once.

OTOH,

Change the SOR to expose a callback with the pre-flight check results for the SOC wrappers to consumer

I would be fine exposing (yet another) kinda-internal API such as preflightGetNamespaces from the SOR if it's necessary, however

• We still have this awkward non-separation between the public and internal API of the SOR. If we want to expose preflightGetNamespaces to wrappers, there isn't currently a way to NOT also expose it on the public contract of the repository and client
• If this would solve the code duplication of the implementation of the preflight checks, I think it would have the drawback of having possibly the preflight checks be performed multiple times, as we may have situations where both a wrapper and the SOR code need to perform the preflight.
• Are we sure the existing preflightGetNamespaces implementation is ok for all the identified additional checks we want to do. I see for instance that currently, this method throws if the document is not in the specified namespace, and I feel like this is not necessarily the behavior we'll always want?

Overall, I don't like either option that much, but I don't see anything else that wouldn't be introducing major changes.

As already said, Imho the correct design would be to change to wrapper system to have distinct exposed APIs. The base idea is something like

type PublicGetOptions {
  // ...
}

type InternalGetOptions = PublicGetOptions & {
  // ...
}

interface PublicRepository {
   get(options: PublicGetOptions): GetResponse;
}

interface InternalRepository {
   get(options: InternalGetOptions): GetResponse;
}

However these are massive changes in the SavedObjectsClientWrapperFactory, SavedObjectsClientFactory and all underlying classes and the associated code implementation (and I'm sure my naive example also got 99 other problems)...

So, as much as I hate it, I would go with a mix of the two suggested options:

• Change the SOR to add "internal" options for these methods that the SOC wrappers can use to indicate a pre-flight check has already been made for target object(s)
• Also expose the preflight APIs to the wrapper, to avoid code duplication. Ideally these 'wrapper-only' APIs would not be exposed on the public Client/Repository contract (really not sure of the feasibility though)

WDYT?

[jportner]

To be honest, in my opinion, the correct solution would probably be to have distinct APIs between the exposed API of the SOR/SOC, and the internal API between the SOR and its wrappers.

Agreed

OTOH,

Change the SOR to expose a callback with the pre-flight check results for the SOC wrappers to consumer

Sorry, I worded that poorly, what I should have said was: Change the SOR to allow the SOC wrappers to pass down a callback to handle the pre-flight check results. Now I've edited the issue description to reflect that.

I would be fine exposing (yet another) kinda-internal API such as preflightGetNamespaces from the SOR if it's necessary, however

• We still have this awkward non-separation between the public and internal API of the SOR. If we want to expose preflightGetNamespaces to wrappers, there isn't currently a way to NOT also expose it on the public contract of the repository and client

Agreed

• If this would solve the code duplication of the implementation of the preflight checks, I think it would have the drawback of having possibly the preflight checks be performed multiple times, as we may have situations where both a wrapper and the SOR code need to perform the preflight.

The implementation that I'm envisioning would solve the duplication and would ensure that only a single preflight check is performed for each SOC method call.

Click to expand and see example

For example, add the following option to bulkCreate:

export interface SavedObjectsBulkCreateOptions extends SavedObjectsBaseOptions {
  ...
 preflightCallbacks?: Array<(preflightResults: Array<SavedObject<T>>) => Array<SavedObject<T>>>
}

SavedObjectsRepository

async bulkCreate<T = unknown>(...) => {
  // do validation
  let preflightResult = await this.client.mget(...);
  if (options.preflightCallbacks) {
    for (callback of preflightCallbacks) {
      preflightResult = callback([...preflightResult]);
    }
  }
  // do create
}

SavedObjectsClient

async bulkCreate<T = unknown>(...) => {
  const preflightCallback = (preflightResult: Array<SavedObject<T>>) => {
    // do authZ
    if (authorized) {
      return preflightResult;
    }
    throw new Error("Not authorized");
  }
  const preflightCallbacks = [...(options.preflightCallbacks ?? []), preflightCallback];
  return this.baseClient.bulkCreate(objects, { ...options, preflightCallbacks }));
}

I know that adding callbacks like this isn't the most desirable pattern to use, but it might be appropriate here for the following reasons:

1. Only a few consumers (SOC wrappers) will ever need to use these callbacks, so we shouldn't have to worry about complexity for "outside consumers" (plugins)
2. This allows us to potentially provide a more consistent validation experience and will allow us to simplify our existing integration tests. For example, trying to create an object of an invalid type will result in a 400 error instead of a 403 error, because the SOR validation would happen before the SOC authorization.
3. We don't need to expose preflight APIs to SOC wrappers

• Are we sure the existing preflightGetNamespaces implementation is ok for all the identified additional checks we want to do. I see for instance that currently, this method throws if the document is not in the specified namespace, and I feel like this is not necessarily the behavior we'll always want?

That implementation is for non-bulk operations. But yes, as long as we have all of the root fields for the saved object, that is enough for us to do all additional checks we will need for initialNamespaces and for OLS. Yes we would need to change it:

• PR for this feature: change to not throw a 409 until after the preflight callbacks are processed (to avoid information disclosure).
• PR for Allow "unresolvable conflict" errors to be circumvented using initialNamespaces #102950: change to not throw a 409 at all if initialNamespaces + overwrite: true are specified.

So, as much as I hate it, I would go with a mix of the two suggested options:

• Change the SOR to add "internal" options for these methods that the SOC wrappers can use to indicate a pre-flight check has already been made for target object(s)
• Also expose the preflight APIs to the wrapper, to avoid code duplication. Ideally these 'wrapper-only' APIs would not be exposed on the public Client/Repository contract (really not sure of the feasibility though)

WDYT?

Yeah, we could expose the preflight APIs to the wrapper and allow each wrapper to pass down an optional preflightResults array so the lower wrappers and the SOR itself could skip the preflight check in that case. But I think we would need several preflight APIs (one for bulkCreate, one for bulkUpdate, etc...) and that might get messy.

As already said, Imho the correct design would be to change to wrapper system to have distinct exposed APIs.
...
However these are massive changes in the SavedObjectsClientWrapperFactory, SavedObjectsClientFactory and all underlying classes and the associated code implementation (and I'm sure my naive example also got 99 other problems)...

Responding to this out of order, but: I think we should consider biting the bullet and doing this now. It appears it would only affect Core code and the three wrappers, so it wouldn't be that bad.

[pgayvallet]

Sorry, I worded that poorly, what I should have said was: Change the SOR to allow the SOC wrappers to pass down a callback to handle the pre-flight check results

Ok, it's a bit clearer now. Still a few questions though

async bulkCreate<T = unknown>(...) => {
  const preflightCallback = (preflightResult: Array<SavedObject<T>>) => {
    // do authZ
    if (authorized) {
      return preflightResult;
    }
    throw new Error("Not authorized");
  }
  const preflightCallbacks = [...(options.preflightCallbacks ?? []), preflightCallback];
  return this.baseClient.bulkCreate(objects, { ...options, preflightCallbacks }));
}

Not sure to get how the // do authZ line will be implemented, as this code is going to live in core's SOC? Is this supposed to effectively only be 'implemented' by security's SOC?

I know that adding callbacks like this isn't the most desirable pattern to use, but it might be appropriate here for the following reasons:

I agree with the 3 reasons you enumerated here. However I feel like adding internal callbacks parameters to the public API is even worse than adding internal 'plain' parameters to it, so you may be right with

think we should consider biting the bullet and doing this now. It appears it would only affect Core code and the three wrappers, so it wouldn't be that bad.

I would love to do that. It would also allow to solve some quite old issue (e.g the space parameter that is, I believe, already meant to be an 'internal' option as the space wrapper throws when specified)

I'm still wondering if, with the license change and all, if this wrapper pattern that was introduced only for isolation of concerns between xpack and oss, still is the best approach, and if some hook-based enhancement wouldn't be more powerful and appropriate. However totally changing the enhancement API of the SOR/SOC seems like even more work, and may even be plainly unrealistic.

So, if we try to look at the design changes that this public/private distinction would cause.

My main concern is that the wrapper currently shared the exact same API as the client and the client IS the public API. As the wrapper currently act as 'delegates', we can't easily change that.

ATM, the SOW and SOC shares the same interface. However, we can, or not, have one, or more, wrappers on top of the client, which makes the initial public-to-internal parameters conversion hard to do:

Which is why I think we'll need some kind of facade / additional front layer in top of the SOC/SOW/SOR chain to handle this conversion in a single place.

This new facade will also be the public API, allowing us to have a distinct internal API for the actual SOC/SOW/SOR communication and chaining.

That way, we could have:

interface SavedObjectClientFacade {
  get(opts: PublicGetOptions)
}

interface InternalSavedObjectClient {
  get(opts: InternalGetOptions)
}

Note that we would need to have a way to convert these PublicGetOptions to InternalGetOptions within the facade, before effectively calling the SOW/SOC chain with them. From what I see atm, most of these internal options are optional, so the conversion could just be convert(opts: PublicGetOptions): InternalGetOptions => return opts, but I feel like we may encounter scenarios where it's slightly more complex. In that case, I'm not sure how (and who) we could handle this conversion.

Note that this facade pattern could also allow us to have public and internal return types, if we add a similar internal->public return conversion similar to what we'll need to do with the input.

interface SavedObjectClientFacade {
  get(opts: PublicGetOptions): PublicGetResponse
}

interface InternalSavedObjectClientContract {
  get(opts: InternalGetOptions): InternalGetResponse
}

with the wrapper factory being

export type SavedObjectsClientWrapperFactory = (
  options: SavedObjectsClientWrapperOptions
) => InternalSavedObjectClientContract;

this could potentially ease the communication between wrappers and the client if we need some kind of before - exec - after processing. I don't know if we currently have needs for this though in our security and space wrappers? You probably know better than I do.

This is a very naive draft, but do you think the approach looks correct?

[jportner]

Not sure to get how the // do authZ line will be implemented, as this code is going to live in core's SOC? Is this supposed to effectively only be 'implemented' by security's SOC?

🤦 Sorry about that, when I wrote the example for "SavedObjectsClient", I meant it was an example for "SecureSavedObjectsClientWrapper".

I'm still wondering if, with the license change and all, if this wrapper pattern that was introduced only for isolation of concerns between xpack and oss, still is the best approach, and if some hook-based enhancement wouldn't be more powerful and appropriate. However totally changing the enhancement API of the SOR/SOC seems like even more work, and may even be plainly unrealistic.

Agreed, and we'll likely need a totally different approach anyway when implementing #102177 anyway

Note that we would need to have a way to convert these PublicGetOptions to InternalGetOptions within the facade, before effectively calling the SOW/SOC chain with them. From what I see atm, most of these internal options are optional, so the conversion could just be convert(opts: PublicGetOptions): InternalGetOptions => return opts,

Agreed

but I feel like we may encounter scenarios where it's slightly more complex.

Perhaps this feeling is a bit of Saved Objects PTSD 😅

Honestly, for the things we want to enable now and in the future (deduplicate preflight checks with SOC wrappers, improve authorization checks when creating objects using initialNamespaces, and eventually to check authorization when interacting with objects that use OLS) I think we only need optional internal options. I can't think of any real additional complexity.

Note that this facade pattern could also allow us to have public and internal return types, if we add a similar internal->public return conversion similar to what we'll need to do with the input.
...
this could potentially ease the communication between wrappers and the client if we need some kind of before - exec - after processing. I don't know if we currently have needs for this though in our security and space wrappers? You probably know better than I do.

I don't think we have a need for this, but I'm interested to hear what @legrego and @azasypkin think as well.

This is a very naive draft, but do you think the approach looks correct?

Yes I think it looks correct and it makes sense to me. Shouldn't be too painful at all!

[pgayvallet]

Yes I think it looks correct and it makes sense to me

Great! then it's almost done, just have to implement the whole thing now.

Shouldn't be too painful at all!

Oh, definitely. I mean, what could possibly go wrong here 😅

Seriously though, this doesn't sounds like that much changes (for a SOR features impacting all its APIs, that is)

Do you know the current parameters that should be converted to internal? The first obvious answer is namespace I guess, but do you see any other already existing?

[jportner]

Do you know the current parameters that should be converted to internal? The first obvious answer is namespace I guess, but do you see any other already existing?

In addition to the namespace option for all methods:

---

kibana/src/core/server/saved_objects/service/lib/update_objects_spaces.ts

Lines 39 to 50 in d920682

	/**
	* The space(s) that the object to update currently exists in. This is only intended to be used by SOC wrappers.
	*
	* @internal
	*/
	spaces?: string[];
	/**
	* The version of the object to update; this is used for optimistic concurrency control. This is only intended to be used by SOC wrappers.
	*
	* @internal
	*/
	version?: string;

This SavedObjectsUpdateObjectsSpacesObject interface (used for the updateObjectsSpaces method) has two fields that are meant to be internal only. But if we implemented some sort of "preflight callback" pattern, then these would no longer be needed.

---

kibana/src/core/server/saved_objects/types.ts

Lines 142 to 149 in 59d7f33

	/**
	* This map defines each type to search for, and the namespace(s) to search for the type in; this is only intended to be used by a saved
	* object client wrapper.
	* If this is defined, it supersedes the `type` and `namespaces` fields when building the Elasticsearch query.
	* Any types that are not included in this map will be excluded entirely.
	* If a type is included but its value is undefined, the operation will search for that type in the Default namespace.
	*/
	typeToNamespacesMap?: Map<string, string[] | undefined>;

This SavedObjectsFindOptions interface (used for the find method) has a field that is only meant to be used by the Secure SOC wrapper.

---

We could also optionally get rid of the distinction between SOR's incrementCounter and incrementCounterInternal if we wanted, and instead add a field to allow internal consumers to bypass the validation checks.

[legrego]

Superseded by #133835