Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SavedObjects should support importing/exporting even if they're hidden from the API #82027

Closed
rudolf opened this issue Oct 29, 2020 · 25 comments · Fixed by #90178
Closed

SavedObjects should support importing/exporting even if they're hidden from the API #82027

rudolf opened this issue Oct 29, 2020 · 25 comments · Fixed by #90178
Assignees
Labels
Feature:Saved Objects NeededFor:Alerting Services Project:RemoveLegacyMultitenancy Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@rudolf
Copy link
Contributor

rudolf commented Oct 29, 2020

Some saved object types would like to prevent users interacting with them through the auto-generated saved objects API but still allow users to import and export these objects for backup purposes.

Examples:

We could start by still allowing exports for objects with hidden: true and management.importableAndExportable: true. However cases and alerts implement their own security in their API layer and we should not just allow any user to export any of these objects.

@rudolf rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects labels Oct 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

We could start by still allowing exports for objects with hidden: true and management.importableAndExportable: true

Regarding tags: the tag SO type is currently hidden: false, importableAndExportable: true as we needed to be able to include tags when exporting objects assigned to them. The side effect being them listed in the SO management section, which we want to avoid.

Your suggestion would work, as long as hidden types will still be considered as non-visible from the SO management section. In that case, we could change the type's definition to hidden: true, importableAndExportable: true (and change the tag client to use the now hidden tag as includedHiddenTypes) and that would answer the tag type needs: exportable, but no listed on SOM.

This proposition would still mean that all hidden types would never appear on the SO management section. That seems reasonable, but I lack enough knowledge of hidden types specific needs regarding import / export to be sure. Maybe we will need an additional meta, such as hidden(|visible)FromManagement to allow hidden types to opt in/out of being listed from the SOM page when they are hidden: true, importableAndExportable: true?

@kobelb maybe you have a better vision that us on that?

@kobelb
Copy link
Contributor

kobelb commented Nov 3, 2020

We could start by still allowing exports for objects with hidden: true and management.importableAndExportable: true. However cases and alerts implement their own security in their API layer and we should not just allow any user to export any of these objects.

I think it's an interesting suggestion. From the authorization perspective, I think we could make it work as we have a "Saved Object Management" feature which conceptually allows the user to import/export anything that we consider to be a saved-object. So even if we end up skipping the authorization implemented in the custom clients, we could still ensure that the end-user is authorized for the import/export operation itself. As far as I'm aware, it'd take additional effort to do authorization on import/export, as this relies on the authorization implemented within SavedObjectsClient#bulkCreate and SavedObjectsClient#bulkGet.

However, this alone wouldn't fix situations like Alerting, as they need to create tasks with task-manager and create API keys on import.

@kobelb maybe you have a better vision that us on that?

I wouldn't say that I have a "better" anything than you all, but like always, I have an opinion I'm willing to share 😄

At the moment, importableAndExportable controls whether or not the saved-object is listed on the main saved-object management screen and whether or not it is displayed within the "relationships flyout" and is exported when the "include related objects" checkbox is selected. What if we split this into two booleans: importableAndExportable.showInMainList and importableAndExportable.includeInReferences (with better names after a good bike-shedding). Would that solve our issues with tags?

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 4, 2020

As far as I'm aware, it'd take additional effort to do authorization on import/export, as this relies on the authorization implemented within SavedObjectsClient#bulkCreate and SavedObjectsClient#bulkGet.

atm users can directly call the endpoints for import or export without using the UI. This authorization 'issue' is already present. I thought we were fine by assuming that read permission to a SO type grants export permission, and that write permission to the type grants import permission?

At the moment, importableAndExportable controls whether or not the saved-object is listed on the main saved-object management screen and whether or not it is displayed within the "relationships flyout" and is exported when the "include related objects" checkbox is selected. What if we split this into two booleans: importableAndExportable.showInMainList and importableAndExportable.includeInReferences (with better names after a good bike-shedding). Would that solve our issues with tags?

showInList and includedWhenReferenced (which would defaults to showInList && !hidden) maybe? Note that we would still need to adapt our import logic to allow importing types that are includedWhenReferenced, else the import would broke.

We got two export needs for tags (sorry, should probably have started with that):

  • From the SO management section, user exporting objects referencing tags should also export tags

This would be addressed by the proposition

  • From the tags management section, user can select tags, and export these with all objects referencing them.

For that case, as we are exporting from the wrong side of the relation, I was planning to manually find all objects referencing the tags, and perform an export 'by objects', using fetchByObjects

async function fetchByObjects({
objects,
exportSizeLimit,
namespace,
savedObjectsClient,
}: SavedObjectsFetchByObjectOptions) {

This would be working - for now - as directly using exportSavedObjectsToStream (as it is done by spaces to copy to space) does not perform any check on importableAndExportable or hidden (as we manually provide a SO client to the function). We have an issue opened to move this export API to the SO client though - #82077, but it should still be able to work then, as we could just create a SO client with the required tag type.

@kobelb
Copy link
Contributor

kobelb commented Nov 4, 2020

atm users can directly call the endpoints for import or export without using the UI. This authorization 'issue' is already present. I thought we were fine by assuming that read permission to a SO type grants export permission, and that write permission to the type grants import permission?

I agree with your description of the current behavior, and I agree that it's fine to keep it this way.

I was attempting to propose that we change it so that you could only use import/export if you have the saved-object management feature. The import/export authorization would still be performed, but the custom authorization in the custom clients would just be skipped.

As far as I'm aware, the custom authorization in the custom clients is being implemented so that end-users can have access to a subset of saved-object types. For example, Alerting wanted to have a singular alert saved-object, even though there are different "alert types" that different users should have access to. We still want to perform authorization to ensure that the end-user is able to export the saved-object, but if we now say that they can export any saved-object if they have the saved-object management feature, we can sneak around needing to do the custom client authorization for import/export.

The side effect being them listed in the SO management section, which we want to avoid.

Why don't we want tags to be listed in the SO management section? It seems like this would allow us to easily export all tags and their related saved-objects using the existing UI and logic. In my opinion, ideally all Kibana saved-objects would be exportable/importable in saved-object management, as this allows users a single place to export all of their saved-objects. Not being able to export a tag, unless it happens to be related to another saved-object that you're exporting, seems like a weird user experience.

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 5, 2020

I was attempting to propose that we change it so that you could only use import/export if you have the saved-object management feature. The import/export authorization would still be performed, but the custom authorization in the custom clients would just be skipped.

So add an api-level check on the endpoints and then use the internal client to perform the operation?

Why don't we want tags to be listed in the SO management section? It seems like this would allow us to easily export all tags and their related saved-objects using the existing UI and logic

So, technically first: No it doesn't. The relation between a SO and a tag is in the 'wrong' direction for that to work (it's SO->TAG, not TAG->SO). Exporting a tag from the SO management section actually just literally do that, it exports the tag object, alone. This is why we will be adding an 'export tag and relative objects' action from the tag management section, as we need a 'reverse relation' logic when generating the list of objects to export (find all objects assigned to the tags, then use the same references gathering logic we already do, but from these objects instead of the tags).

Now, functionally: Product want tags to be a distinct concept/feature than SOs. The fact that we are using SOs under the hood should remains, as much as possible, an implementation detail. This is why tag-related actions (such as 'export tags and their relatives') should be performed from the tag management section, and why ideally tags should remains 'invisible' in SO management.

@kobelb
Copy link
Contributor

kobelb commented Nov 5, 2020

So add an api-level check on the endpoints and then use the internal client to perform the operation?

That's what I was initially thinking, yeah.

So, technically first: No it doesn't. The relation between a SO and a tag is in the 'wrong' direction for that to work (it's SO->TAG, not TAG->SO). Exporting a tag from the SO management section actually just literally do that, it exports the tag object, alone.

You're right. When you chose to "include related objects" during export, it's only including the "children" not the "parents".

Now, functionally: Product want tags to be a distinct concept/feature than SOs. The fact that we are using SOs under the hood should remains, as much as possible, an implementation detail. This is why tag-related actions (such as 'export tags and their relatives') should be performed from the tag management section, and why ideally tags should remains 'invisible' in SO management.

If we go with this approach, it will be in contrast to how index-patterns are currently handled. Index-patterns have their own management section but also show up on the saved-object management screen. It will also potentially create a two-step process for end-users who wish to "export all of their Kibana data", because now we'll have to direct them to export all saved-objects, and if they happen to have any tags that aren't currently being referenced we'll have to instruct them to then export all of their tags.

@rudolf
Copy link
Contributor Author

rudolf commented Nov 5, 2020

I feel like there are opposing ideals of what Saved Object Management should be and perhaps this stems from us not having a clear vision for this feature.

On the one hand we want to allow users to export all their Kibana data, and essentially we're opening up the internals of our "database" to users showing them everything that's stored there so they can make a backup. On the other hand tags and spaces are modelled as magical "system" data, but that also needs to be exported somehow to be able to fully restore your data.

I wonder if some of this conflict comes from the "management" of Saved Objects. Maybe this UI should only have "create a backup" or "restore a backup". If they want to do "management" activities like selectively export a dashboard or sharing it to a different space, this should be done from the UI of that entity. So if I want to share a case or export a case I go to cases management and select the case. Anything short of a full backup happens inside the plugin. This means that cases can include all the entities, value objects, relationships (parent or child) etc when exporting.

A slightly different alternative would be to keep Saved Object Management but then we don't list all the saved objects in the system. Instead we ask all plugins "what entities do you have that a user can backup?". Cases will say, here's all the cases (and will already embed any comments etc inside). When a user imports the objects we pass these to the plugin again and say "a user wants to restore these alerts objects". This way we separate our database (the actual saved objects) from the domains that are built on top of them. It's probably still a good idea to embed value objects inside an entity, but it's not so critical how the data is modeled, it becomes an implementation detail of the plugin that users don't need to understand.

@pgayvallet
Copy link
Contributor

@kobelb

If we go with this approach, it will be in contrast to how index-patterns are currently handled. Index-patterns have their own management section but also show up on the saved-object management screen. It will also potentially create a two-step process for end-users who wish to "export all of their Kibana data", because now we'll have to direct them to export all saved-objects, and if they happen to have any tags that aren't currently being referenced we'll have to instruct them to then export all of their tags.

Valid point. This is a product decision, though. Adding @alexfrancoeur @ryankeairns in the loop FYI, but we may want to create a new issue for this specific subject, as the current one is more around the technical and architectural design of the export/import process.

@rudolf

I feel like there are opposing ideals of what Saved Object Management should be and perhaps this stems from us not having a clear vision for this feature.

Can't agree more on that

I wonder if some of this conflict comes from the "management" of Saved Objects. Maybe this UI should only have "create a backup" or "restore a backup". If they want to do "management" activities like selectively export a dashboard or sharing it to a different space, this should be done from the UI of that entity

I'm not oppose to the idea, however

  • This is heavy work for apps / solution teams, as they would all have to rethink their UI to include these new actions
  • I'm unsure there are any other need than 'create a backup' when exporting objects. Not sure we really want to have a 'export my dashboard/vis/cases' PER application ?
  • Even if we do that, I think the internals of performing an import or an export should be done by core. But exposing APIs to do so (which is really lacking at the moment) would allow to do so
  • Unsure how plugins such as 'dashboard' would have access to X-pack actions such as 'share to space'. ATM the spaces plugins registers an action to the SOM plugin to do that. If the SOM is no longer the only entry point to share to space, would the spaces plugin have to manually register this 'export to space' action to ALL applications having this need (it cannot be done the other way as you can't have oss->xpack dependency atm).

Which is why I think your second proposal is probably more doable or 'realistic'

A slightly different alternative would be to keep Saved Object Management but then we don't list all the saved objects in the system. Instead we ask all plugins "what entities do you have that a user can backup?"

I think this always has been the vision. This is what the isImportableAndExportable flag is for. What we probably did not anticipate was that a raw dump of the object was not good enough in some situations (encrypted attributes), and that an object may have dependencies (relations did not exists when SO import/export was first implemented if I remember correctly)

we ask all plugins "what entities do you have that a user can backup?". Cases will say, here's all the cases (and will already embed any comments etc inside). When a user imports the objects we pass these to the plugin again and say "a user wants to restore these alerts objects".

First obvious problem I see with a basic implementation of what to describe would be the SOM pagination / filtering / searching. As objects would now be retrieved from multiple sources, it will be a nightmare to address: all these 'objects providers' would need to support pagination, filtering and so on...

However, I agree, and the current discussion confirms it, that we need some way to let type owners have their word regarding the export and import processes. Ideally, We would still be able to retrieve the list of objects listed in SOM ourselves (and ideally++ in a single ES query against the SO index), but we would expose additional APIs to let plugins alter this list of object before the export. It could be to add dependencies that are not listed as references, redact some fields, generate a 'super' object that would be an aggregation of other (thinking cases + comments here)or any kind of processing.

The same would apply for import. SOM would still be the single entry point to import a 'dump'/'backup' or whatever, but we would be calling the type owners for each object to preprocess the objects before reinjecting them into ES. this preprocess would include data processing, creating (and adding to the import 'on the fly') objects that where aggregated during the export, and so on.

(of course, I have no idea how these changes would work with import conflicts)

@mshustov
Copy link
Contributor

mshustov commented Nov 6, 2020

I think this always has been the vision. This is what the isImportableAndExportable flag is for. What we probably did not anticipate was that a raw dump of the object was not good enough in some situations (encrypted attributes), and that an object may have dependencies (relations did not exists when SO import/export was first implemented if I remember correctly)

++

It could be to add dependencies that are not listed as references, redact some fields, generate a 'super' object that would be an aggregation of other (thinking cases + comments here)or any kind of processing.

What prevents plugins from modeling SO from the very beginning in such a way? Lack of recommendations on the best practices?

SOM would still be the single entry point to import a 'dump'/'backup' or whatever, but we would be calling the type owners for each object to preprocess the objects before reinjecting them into ES.

What about complex export cases when async actions required? LikeWhen importing alerts, task manager tasks will have to be created so these alerts run after importing from #50266 (comment) ? We don't want to abort an operation if one of the plugins stuck/throws an error. Would it be more appropriate to inform a user (via the notifications, banners, etc.) about a problem SO and navigate to a plugin owner to fix the problem in the plugin context?

@pgayvallet
Copy link
Contributor

What prevents plugins from modeling SO from the very beginning in such a way

That is probably the conversation in progress here: #72028, but in short: The fact that the SO http API are currently 'the way to go' for SO interactions, and the fact that these APIs are manipulating raw SOs probably is the main cause of our current situation.

What about complex export cases when async actions required? Like When importing alerts, task manager tasks will have to be created so these alerts run after importing from #50266 (comment) ? We don't want to abort an operation if one of the plugins stuck/throws an error. Would it be more appropriate to inform a user (via the notifications, banners, etc.) about a problem SO and navigate to a plugin owner to fix the problem in the plugin context?

It probably depends (tm) on the kind of async actions we're talking about. Decrypting SO attributes per instance, could be async (like if the hook needs to access the FS, or any other node-base reasons to have a async method). In that case, SO import/export should probably support async 'hooks'. For more complex actions (I'm mostly thinking about actions that require user input), redirecting to the type 'owner plugin' to fix in context could be an option. My main question in that case is how do you manage, from SO management, the workflow to handle multiple 'failures' requiring different user actions?

Say, you import alerts, dashboards, cases, and they all require some user interactions to fix the import on each individual types. The workflow to allow the user to be directed, in a 'seamless' workflow, on each app to fix in context seems very complex, and maybe more work that allowing some kind of 'UI hooks' for type owners to plugin into the import process?

As a side note, I feel that this discussion may be diverging. From my understanding, the prime goal of the issue was to answer the base need of exporting hidden SO types, not to try to fix all the other problems required for #82020 regarding SO exports. Else we risk some overlaps with other issues such as #82064 or #72028. OTOH all these issues are tightly coupled, and we might want to use a single one for all the part of the 'meta' discussion, I'm not sure.

@kobelb
Copy link
Contributor

kobelb commented Nov 10, 2020

Today, a few of us had a synchronous conversation regarding some of the great questions that were raised in this issue. I've attempted to summarize what we've come to an agreement on and the topics which are still debated. For those of you who were there, please keep me honest; and for others who were unable to attend, please feel free to voice your objections to any conclusions that were reached in your absence:

  • Agreement
    • Need to provide users an easy way to export/import all of their data
    • The export/import should include some saved-objects that are hidden: true
    • When exporting, plugins should be able to perform custom logic
      • This will allow Alerting to mark their actions and Alerts as being disabled
      • Custom logic on export potentially makes bugs easier to manage
      • Sync custom logic is safer than async custom logic, but it limits which actions can be performed.
    • When importing, in the future we might want to allow custom logic to be performed, but we don't need to do so now.
      • Custom logic on import is tricky because if there's a bug, we risk creating some saved-objects and not others
    • Saved-object management
      • We shouldn't allow users to edit the raw JSON of saved-objects in the future, potentially as soon as 8.0
        • We should ensure that support engineers don't rely on this to get out of tricky situations
      • It's currently the best place to allow users to export/import all of their Kibana-specific entities. Segmenting this to exclude certain saved-objects potentially creates a disjointed user experience.
      • The ability to copy-to-space and share-to-space shouldn't only be available here with the way the privileges are currently implemented
        • There are quite a few administrator-level features available here, so a small subset of users should be granted access to the entire saved-object management feature
  • Debated
    • Should the custom export logic be sync only, or should we support async?
    • Saved-object management
      • Should it expose the ability to delete saved-objects?
        • This potentially allows end-users to violate use-case specific constraints
        • The ability to bulk-delete across saved-object type is helpful
    • Saved-object management vs tag management
      • Further discussion required regarding the UX
      • Exporting Tags from SOM does not allow to export the tagged objects implicitly, as the reference is in the wrong direction

@kobelb
Copy link
Contributor

kobelb commented Dec 4, 2020

I think this issue made us think through a lot of related issues; however, I think we should constrain it to the original purpose that @rudolf stated:

We could start by still allowing exports for objects with hidden: true and management.importableAndExportable: true.

When the developer registers a saved-object that is hidden: true and management.importableAndExportable: true, we should make the necessary changes so that it's:

  1. returned by the api/kibana/management/saved_objects/_find endpoint
  2. exportable using the api/saved_objects/_export endpoint
  3. importable using the /api/saved_objects/_import and api/saved_objects/_resolve_import_errors endpoints
  4. users with the saved-object management all feature can perform the aforementioned actions

I created #84976 as a proof-of-concept doing so for Alerting; however, there are additional changes that are required for Alerting to be able to do so. The part that I feel the most uneasy about is the changes to the privileges model. With these changes, when a user is granted access to the saved-object management feature, they're authorized to perform all CRUD operations on importable/exportable saved-objects even if they're hidden. However, they can't do so using any of the APIs besides the above because of the implementation of hidden types. @legrego, I'm interested to hear your thoughts on this.

@legrego
Copy link
Member

legrego commented Dec 4, 2020

The part that I feel the most uneasy about is the changes to the privileges model. With these changes, when a user is granted access to the saved-object management feature, they're authorized to perform all CRUD operations on importable/exportable saved-objects even if they're hidden. However, they can't do so using any of the APIs besides the above because of the implementation of hidden types.

The authorization changes to the SOM feature that you made here feel ok to me. In fact it's probably more correct than what we previously had, since the intent of those privileges was to facilitate import and export operations. We've already accepted that SOM is an administrative feature, so allowing them to do administrative actions that aren't otherwise possible seems reasonable to me.

One caveat just came to mind if we were to allow this:

For Alerting specifically, this will cause inconsistent audit records: the import/export operations will be audited as generic saved object CRUD events (as if they were a "regular" saved object). When interacting with Alerts through their dedicated APIs, we generate more specific audit events. This will be even more pronounced in 7.11 with #84113. I don't think this will cause any gaps in audit coverage, but someone searching for the alerting specific audit events could very well miss these since they'll be recorded slightly differently.

@pgayvallet
Copy link
Contributor

When the developer registers a saved-object that is hidden: true and management.importableAndExportable: true, we should make the necessary changes so that it's:

  • returned by the api/kibana/management/saved_objects/_find endpoint

I read api/saved_objects/_find endpoint and almost got a heart attack 😅

@kobelb
Copy link
Contributor

kobelb commented Dec 4, 2020

For Alerting specifically, this will cause inconsistent audit records: the import/export operations will be audited as generic saved object CRUD events (as if they were a "regular" saved object). When interacting with Alerts through their dedicated APIs, we generate more specific audit events. This will be even more pronounced in 7.11 with #84113. I don't think this will cause any gaps in audit coverage, but someone searching for the alerting specific audit events could very well miss these since they'll be recorded slightly differently.

I wonder if we can take advantage of the onExport and onImport hooks that we'll be adding in #84977 and #84980 to log the Alerting specific audit events.

@legrego
Copy link
Member

legrego commented Dec 4, 2020

I wonder if we can take advantage of the onExport and onImport hooks that we'll be adding in #84977 and #84980 to log the Alerting specific audit events.

Yeah that sounds like it'd work! I wasn't aware those were in scope for this discussion, based on the earlier comment of constraining this to the original purpose of the issue.

@pgayvallet pgayvallet assigned pgayvallet and Bamieh and unassigned pgayvallet Dec 16, 2020
@pgayvallet
Copy link
Contributor

@legrego should the onExport / onImport hooks be considered a prerequisite before starting to implement this then, or would it be acceptable if the current issue was to be done before the hook APIs (even if both are planned for 7.12)?

@legrego
Copy link
Member

legrego commented Dec 16, 2020

@legrego should the onExport / onImport hooks be considered a prerequisite before starting to implement this then, or would it be acceptable if the current issue was to be done before the hook APIs (even if both are planned for 7.12)?

Will the current issue expose this support for alerts or actions, or will we complete this initial work without turning it on for those types? I don't mind splitting up the work into different PRs, but I would like to have onExport/onImport hooks in place before we turn this on for alerts and actions specifically.

@pgayvallet
Copy link
Contributor

Will the current issue expose this support for alerts or actions, or will we complete this initial work without turning it on for those types?

From a quick look, alert and action are not currently importableAndExportable: true, so it would not impact them until we explicitly change that. I guess we're good to go then.

savedObjects.registerType({
name: 'alert',
hidden: true,
namespaceType: 'single',
migrations: getMigrations(encryptedSavedObjects),
mappings: mappings.alert,
});

savedObjects.registerType({
name: ACTION_SAVED_OBJECT_TYPE,
hidden: true,
namespaceType: 'single',
mappings: mappings.action,
migrations: getMigrations(encryptedSavedObjects),
});

@rudolf
Copy link
Contributor Author

rudolf commented Dec 17, 2020

In addition to the API/Export behaviour, hidden saved object types are also hidden from the saved object clients. In order to access a hidden type (and still scope the security to the incoming request) a plugin would have to construct a new scoped saved objects client. This means they can't use the existing scoped client exposed through the request handler context, it will also affect the ergonomics of export hooks #84980 (comment)

So I wonder if we shouldn't change the behaviour of hidden to only affect the API. This would mean it's easier for another plugin to accidentally use a hidden type registered by another plugin, but I think this is unlikely and that the better solution if we want some form of protection against developers not following our best practices would be to scope the saved objects client to each plugin only providing access to the types that the plugin registered itself.

@legrego
Copy link
Member

legrego commented Dec 17, 2020

So I wonder if we shouldn't change the behaviour of hidden to only affect the API. This would mean it's easier for another plugin to accidentally use a hidden type registered by another plugin, but I think this is unlikely and that the better solution if we want some form of protection against developers not following our best practices would be to scope the saved objects client to each plugin only providing access to the types that the plugin registered itself.

To clarify: are you proposing that plugins will automatically get access to their own hidden saved objects, but not to other plugin's hidden saved objects? If so, then I think that would be an acceptable path forward.

What I'm not keen on is for all plugins to automatically get access to all hidden types. We rely on the hidden flag to enforce custom authorization and auditing for special types, such as Spaces, Alerts, and Actions. If we suddenly allow all plugins to access these, then we run a real risk of inadvertently bypassing the custom authorization and auditing.

@pgayvallet
Copy link
Contributor

pgayvallet commented Dec 18, 2020

To clarify: are you proposing that plugins will automatically get access to their own hidden saved objects, but not to other plugin's hidden saved objects? If so, then I think that would be an acceptable path forward.

From my understanding, what @rudolf is proposing is that all hidden types will be accessible from any server-side SO client.

In addition to the API/Export behaviour, hidden saved object types are also hidden from the saved object clients. In order to access a hidden type (and still scope the security to the incoming request) a plugin would have to construct a new scoped saved objects client

@rudolf I'm not sure to see the exact need here. the export and import endpoints will be creating their own client to include all hidden (but/and exportable) types (as done in @kobelb POC here: #84976). We will likely do the same for the /api/kibana/management/saved_objects/_find route. Do you see any reason to change the hidden behavior on other (core) SO endpoints or on the client itself?

Imho all we (eventually) need would be a new context.savedObject.createClient({includedHiddenTypes}: SavedObjectsClientProviderOptions) in the route handler context to ease the way custom client are created (this is currently a pain to create a custom client from a request handler)

As a sidenote, I'm currently in the process of refactoring the export/import code (#86264) to have it accessible via the SO service/context instead of static imports, which will also help for all future improvements of these features

@kobelb
Copy link
Contributor

kobelb commented Dec 18, 2020

Imho all we (eventually) need would be a new context.savedObject.createClient({includedHiddenTypes}: SavedObjectsClientProviderOptions) in the route handler context to ease the way custom client are created (this is currently a pain to create a custom client from a request handler)

I like this approach. When a saved-object type is hidden, we want to require that all access to these saved-objects goes through the custom client. If we automatically made these hidden types accessible to the entire plugin, this weakens that protection.

@rudolf
Copy link
Contributor Author

rudolf commented Jan 7, 2021

I guess I was questioning the usefulness of hiding hidden types from the client since this doesn't provide any real security. But I agree that hiding it from the client can help prevent security bugs where a developer thinks they're working with a non-hidden saved object.

Imho all we (eventually) need would be a new context.savedObject.createClient({includedHiddenTypes}: SavedObjectsClientProviderOptions) in the route handler context to ease the way custom client are created (this is currently a pain to create a custom client from a request handler)

I'm +1 on this solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects NeededFor:Alerting Services Project:RemoveLegacyMultitenancy Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants