[Saved Objects] adds support for including hidden types in saved objects client by gmmorris · Pull Request #66879 · elastic/kibana

gmmorris · 2020-05-18T10:18:38Z

Summary

As part of the work needed for RBAC & Feature Controls support in Alerting (#43994) we've identified a need to make the Alert Saved Object type a hidden type.

As we still need support for Security and Spaces, we wish to sue the standard SavedObjectsClient and its middleware, but currently this isn't possible with hidden types.

To address that, this PR adds support for creating a client which includes hidden types.

Checklist

Delete any items that are not applicable to this PR.

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dev Docs

Saved Objects

The SavedObjectClient's getScopedClient, createScopedRepository and createInternalRepository can now all take a list of types to be included in the underlying repository.

This can be used to create a client which has access to hidden types, like so:

core.savedObjects.getScopedClient(request, { includedHiddenTypes: ['hiddenType'] })

This would create a SavedObjects client scoped to a user by the specified request with access to a hidden type called hiddenType.

Encrypted Saved Objects

The EncryptedSavedObject plugin no longer exposes a single client as part of its Start contract, instead it exposes a getClient api that exposes the client api. This getClient can now also specify a list of hidden types to gain access to which are hidden by default.

For example, given a Kibana Platform plugin which has specified encryptedSavedObjects as a Setup dependency:

const encryptedSavedObjectsClient = plugins.encryptedSavedObjects.getClient(['hiddenType']);
return encryptedSavedObjectsClient.getDecryptedAsInternalUser('hiddenType',  '123',   { namespace: 'some-namespace' });

rudolf

Left some nits, but 👍 for Platform changes.

Left some comments on the encrypted saved objects plugin API which I think should be discussed with the security team.

src/core/server/saved_objects/service/lib/scoped_client_provider.test.js

rudolf · 2020-05-18T10:45:36Z

x-pack/plugins/encrypted_saved_objects/server/plugin.ts

        service.registerType(typeRegistration),
      __legacyCompat: { registerLegacyAPI: (legacyAPI: LegacyAPI) => (this.legacyAPI = legacyAPI) },
      usingEphemeralEncryptionKey,
+      startWithHiddenTypes: (includedHiddenTypes: string[]) =>


I haven't looked through all the encrypted saved objects plugin's code, so I'll leave this up to the security team, but this API is different from what I would have expected because we're exposing two almost identical API's from setup and start.

Perhaps when there's a hidden type registration through registerType we could automatically include this hidden type into the underlying repository or alternatively there could be a setup API like includeHiddenTypes: (includedHiddenTypes: string[]) which just sets a includedHiddenTypes class property which is then read when creating the getDecryptedAsInternalUser api is created.

Yeah, I was a little thrown by the fact that the Start api is the client, rather than returning an api for creating a client.
Given that it felt like maybe the Setup api should provide a way of returning a version of the start api with the additional hidden type, otherwise all encryptedSavedObjects clients in Kibana will, by default, have access to the hidden types... which seems weird to me. 🤔

But yeah, I'm open to changing this to whatever makes sense to the Security team obviously.

Would love to hear @legrego 's thoughts

Yes you're right, my suggestion will open up hidden types to all consumers which isn't what we want.

Yeah, I was a little thrown by the fact that the Start api is the client, rather than returning an api for creating a client.

@azasypkin do you recall if there's a reason we expose the client as the start contract, rather than exposing a function to create the client? It seems that if we can expose the function to create, then we can use that function to optionally construct a client which includes access to hidden types.

I'm happy to expose a suitable api on the Start contract if you'd like.
We'd need to retain the existing API as removing would be a breaking change, but at least we'd have a single api for including hidden types and just a straight up regular client, and perhaps we remove the old api in 8.0?

I'm happy to expose a suitable api on the Start contract if you'd like.

That would be great, thanks! Let's wait to hear from @azasypkin in case I'm overlooking something thouh.

We'd need to retain the existing API as removing would be a breaking change

We don't offer any guarantees for plugin API stability yet, so introducing a breaking change is perfectly OK to do from my perspective. Encrypted Saved Objects are still pretty new, and it's a relatively esoteric feature, so I don't expect that there is broad (or any) adoption of this outside of Kibana itself.

@azasypkin do you recall if there's a reason we expose the client as the start contract, rather than exposing a function to create the client?

No particular reason, just nobody needed anything more advanced at that time.

I'm happy to expose a suitable api on the Start contract if you'd like.
That would be great, thanks! Let's wait to hear from @azasypkin in case I'm overlooking something thouh.

💯

We don't offer any guarantees for plugin API stability yet, so introducing a breaking change is perfectly OK to do from my perspective. Encrypted Saved Objects are still pretty new, and it's a relatively esoteric feature, so I don't expect that there is broad (or any) adoption of this outside of Kibana itself.

Definitely agree, let's not create a tech debt when we have a chance to avoid that.

Cool, it shall be done :)

I've now exposed a getClient api and changed the few places that are using ESO to use this api instead.

Please let me know if this is what you had in mind. :)

src/core/server/saved_objects/saved_objects_service.test.ts

legrego

Thanks for introducing the ESO client!

x-pack/plugins/encrypted_saved_objects/server/mocks.ts

...ck/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts

jfsiii

Ingest changes LGTM but cc @nchaulet as well

nchaulet

I tested locally fleet with that changes everything work as expected 👍

kibanamachine · 2020-05-19T16:22:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 81afe0c

History

💚 Build #48704 succeeded ba90e13
💚 Build #48611 succeeded 60e2593

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

legrego

LGTM, thanks for the additional tests!

pmuellr

The actions, alerting and task manager changes LGTM!

pmuellr · 2020-05-19T17:33:40Z

x-pack/plugins/alerting/server/alerts_client.ts

    alertInstanceId: string;
  }) {
-    const { attributes, version } = await this.savedObjectsClient.get('alert', alertId);
+    const { attributes, version } = await this.savedObjectsClient.get<Alert>('alert', alertId);


mikecote

Alerting code LGTM

azasypkin · 2020-05-20T07:10:58Z

x-pack/plugins/encrypted_saved_objects/server/plugin.ts

-      getDecryptedAsInternalUser: (type: string, id: string, options?: SavedObjectsBaseOptions) => {
-        return this.savedObjectsSetup.getDecryptedAsInternalUser(type, id, options);
-      },
+      getClient: (includedHiddenTypes?: string[]) => this.savedObjectsSetup(includedHiddenTypes),


ask: sorry for being late here, but I have two minor asks:

can you make parameter as { includedHiddenTypes?: string[] } instead so that it's easier to understand the intention when you read consumer code (getClient(['one', 'two']) vs getClient({ includedHiddenTypes: ['one', 'two'] }) and add new options in the future?

would you mind updating README.md of the plugin to mention new API?

Thanks!

Sorry @azasypkin a Github UI quirk made me miss this last minute comment!
I' can do that as part of a follow up PR that I'll be submitting today anyway- I'll ping you to review :)

Thanks 👍

…ved objects client (#66879)" This reverts commit dfa22d1.

* master: (33 commits) [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879) [Discover] Deangularize timechart header (elastic#66532) [Discover] Improve and unskip a11y context view test (elastic#66959) [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864) docs: update RUM documentation link (elastic#67042) [QA] fixup coverage ingestion tests. (elastic#66905) [Metrics UI] Add support for multiple groupings to Metrics Explorer (and Alerts) (elastic#66503) [Metrics UI] Add sorting for name and value to Inventory View (elastic#66644) [Metrics UI] Change Metric Threshold Alert charts to use bar charts (elastic#66672) [Uptime] Use React.lazy for alert type registration (elastic#66829) [Reporting] Consolidate API Integration Test configs (elastic#66637) Allow histogram fields in average and sum aggregations (elastic#66891) Fix saved object share link (elastic#66771) move role reset into the top level after clause (elastic#66971) Automate the labels for any PRs affecting files for the Ingest Management team (elastic#67022) [SIEMDPOINT] Move endpoint to siem (elastic#66907) server.uuid so is not used (elastic#66963) Revert "[ci/stats] fix git metadata collection (elastic#66840)" [Uptime] Unmount uptime app properly (elastic#66950) [Visualize] Bar chart: Show missing values on chart setting (elastic#66375) ...

…cts client (elastic#66879) As part of the work needed for RBAC & Feature Controls support in Alerting (elastic#43994) we've identified a need to make the Alert Saved Object type a _hidden_ type. As we still need support for Security and Spaces, we wish to use the standard SavedObjectsClient and its middleware, but currently this isn't possible with _hidden_ types. To address that, this PR adds support for creating a client which includes hidden types.

…cts client (#66879) (#67091) As part of the work needed for RBAC & Feature Controls support in Alerting (#43994) we've identified a need to make the Alert Saved Object type a _hidden_ type. As we still need support for Security and Spaces, we wish to use the standard SavedObjectsClient and its middleware, but currently this isn't possible with _hidden_ types. To address that, this PR adds support for creating a client which includes hidden types.

…ent/add-support-in-url-for-hidden-toggle * 'master' of github.com:elastic/kibana: (49 commits) [Uptime] Improve responsiveness details page (elastic#67034) skip flaky suite (elastic#66669) Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124) Support api_integration/kibana/stats against remote hosts (elastic#53000) chore(NA): add module name mapper for src plugins on x-pack (elastic#67103) Change the error message on TSVB in order to be more user friendly (elastic#67090) [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059) [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732) Bump styled-component dependencies (elastic#66611) Bump react-markdown dependencies (elastic#66615) Fix Core docs links (elastic#66977) Timelion graph is not refreshing content after searching or filtering (elastic#67023) Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053) Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432) [Logs UI] Restore call to `UsageCollector.countLogs` (elastic#67051) Remove unused license check result from LP Security plugin (elastic#66966) [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879) [Discover] Deangularize timechart header (elastic#66532) [Discover] Improve and unskip a11y context view test (elastic#66959) [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864) ... # Conflicts: # x-pack/plugins/index_management/__jest__/client_integration/helpers/home.helpers.ts

added support for including hidden types in saved objects client

4f8e589

gmmorris requested review from a team as code owners May 18, 2020 10:18

gmmorris added Feature:Alerting Feature:Saved Objects release_note:enhancement v7.9.0 v8.0.0 labels May 18, 2020

gmmorris mentioned this pull request May 18, 2020

Alerting feature controls #43994

Closed

rudolf approved these changes May 18, 2020

View reviewed changes

rudolf added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:enhancement labels May 18, 2020

gmmorris added 2 commits May 18, 2020 12:30

cleaned up some test descriptions

60e2593

adds a getClient api to Encrypted Saved Objects

ba90e13

gmmorris requested a review from a team May 18, 2020 16:33

gmmorris requested a review from a team as a code owner May 18, 2020 16:33

legrego reviewed May 19, 2020

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/mocks.ts Outdated Show resolved Hide resolved

...ck/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts Show resolved Hide resolved

jfsiii approved these changes May 19, 2020

View reviewed changes

nchaulet approved these changes May 19, 2020

View reviewed changes

added acceptence test suite around hidden types

81afe0c

legrego self-requested a review May 19, 2020 16:31

legrego approved these changes May 19, 2020

View reviewed changes

pmuellr approved these changes May 19, 2020

View reviewed changes

mikecote approved these changes May 19, 2020

View reviewed changes

pgayvallet approved these changes May 20, 2020

View reviewed changes

azasypkin reviewed May 20, 2020

View reviewed changes

gmmorris merged commit dfa22d1 into elastic:master May 20, 2020

gmmorris added a commit that referenced this pull request May 20, 2020

Revert "[Saved Objects] adds support for including hidden types in sa…

8fc78c9

…ved objects client (#66879)" This reverts commit dfa22d1.

gmmorris mentioned this pull request May 20, 2020

[7.x] [Saved Objects] adds support for including hidden types in saved objects client (#66879) #67091

Merged

Conversation

gmmorris commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

Saved Objects

Encrypted Saved Objects

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rudolf May 18, 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!

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!

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jfsiii left a comment

Choose a reason for hiding this comment

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 19, 2020

💚 Build Succeeded

History

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

gmmorris commented May 18, 2020 •

edited

Loading

rudolf May 18, 2020 •

edited

Loading