Plugins should only be able to directly access UiSettings that they registered

[joshdover]

Summary

As a plugin developer, I want to be able to control who has access to any UiSettings that I register.

Acceptance Criteria

1. If plugin A registers a UiSetting
2. Only plugin A should be able to directly access it via the JavaScript clients (server and client) and from the HTTP API.
3. Other plugins should only be able to access the setting if it is exposed by the registering plugin's public runtime contract if exposed explicitly.

Resources:

• Meta issue: [meta] uiSettings service improvements #48925

Notes

Things to consider:

• Plugins can theoretically call the UiSettings HTTP API directly for any setting, so we should determine a way to prevent this if feasible. Options:
  • Include the calling plugin's id as a request header on all HTTP calls from HttpClient on client-side & use this from the UiSettings route to validate that the calling plugin was the original registrant of the setting type. Relates to Improving distributed tracing in Kibana #101711
• Need to audit for circular dependencies on setting keys in usage now. If there are circular deps, plugins will need to extract the registration into a higher-order plugin that all the other plugins depend on.

[elasticmachine]

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

[afharo]

Additional thing to consider: multiple plugins could register the same UiSetting, and we should allow it, as long as they match the same format and validation.

[mshustov]

multiple plugins could register the same UiSetting, and we should allow it, as long as they match the same format and validation.

Could you provide any real-world examples? It's not obvious why we should allow it. It creates ambiguity on ownership and creates implicit dependencies between plugins.

[afharo]

Could you provide any real-world examples?

• savedObjects:perPage is registered by the savedObjects plugin, but it's also used by savedObjectsManagement and timelion
• dateFormat[:*] are registered by core, and used in several other plugins
• visualize:enableLabs is registered by visualizations, but it's also used in visualize and timelion
• defaultRoute is registered by core, and used by kibanaReact and spaces
• ...

As I was looking for more use cases, I found that the data plugin exposes this const: https://github.com/elastic/kibana/blob/master/src/plugins/data/common/constants.ts#L17-L42

And it's imported and used all over many other plugins: vis_*, apm, canvas, data_visualizer, graph, infra, lens, maps, ml, observability, security_solution, ui_actions_enhanced & watcher. As far as I can tell, the most common ones in that list are search:includeFrozen, timepicker:refreshIntervalDefaults, timepicker:quickRanges, histogram:barTarget, histogram:maxBars, search:includeFrozen & metaFields.

Then, there's the special use case for the telemetry collector that reads all the non-default values. We might need to move that collector to core (or use a special API) to overcome the limitation.

It creates ambiguity on ownership and creates implicit dependencies between plugins.

I agree that the current scenario creates implicit dependencies. And that, currently, a plugin changing the definition of a UI Setting can potentially break other plugins. For that reason, I totally agree that a plugin should register the UI Settings it's going to use. However, having 2 settings for identical purposes, only because 2 different plugins require them is bad UX, and can lead to more inconsistencies between apps in Kibana. That's why I think that we should find a middle ground where plugins can share UI Settings, but there is a validation to ensure that the value is in the expected format.

The first thing that comes to mind is having a shared_settings plugin that would register those settings that are consumed from many places. But who would own it? Core? How would we police that the changes are welcomed by all the downstream consumers?

Personally, I wouldn't simply move those shared settings to Core because that would mean having plugins' specific data in Core.

That's why I see consumers declaring somehow the UI Settings they'll need in their plugins (probably when retrieving the client like in SOs hidden types?) 🤷‍♂️

What do you think?

[mshustov]

savedObjects:perPage is registered by the savedObjects plugin, but it's also used by savedObjectsManagement and timelion
dateFormat[:*] are registered by core, and used in several other plugins
visualize:enableLabs is registered by visualizations, but it's also used in visualize and timelion
defaultRoute is registered by core, and used by kibanaReact and spaces

I don't say the cases don't exist, but it doesn't mean they are valid. All the cross-plugin interactions should be performed via an explicit contract. All other implicit dependencies (like listed in the list) are hard to follow, maintain and test. If plugins need defaultRoute setting, they should consume it from Core API, the storage mechanism of defaultRoute is an implementation detail.

However, having 2 settings for identical purposes, only because 2 different plugins require them is bad UX, and can lead to
more inconsistencies between apps in Kibana.

In most cases, there will be a single owner of a setting. From the list above only savedObjects:perPage sounds like a setting that might exist in both savedObjects and savedObjectsManagement plugins. However, I'd investigate whether it can belong to savedObjects plugin exclusively. Except for that case, I don't think we have valid cases for plugins "sharing" uiSettings. But curious to hear about them if you have something in mind.

[TinaHeiligers]

What problem are we specifically trying to solve by restricting a settings' access to only the plugin that registers it? I agree that write access should probably be limited but plugins should not be depending on another plugins' settings for any critical functionality.
Perhaps, as a middle ground, we could think about making settings decare if they are stand-alone, read-only, or read-write?

[lukeelmers]

What problem are we specifically trying to solve by restricting a settings' access to only the plugin that registers it?

@TinaHeiligers I think the main problem this is aiming to solve is introduction of implicit dependencies between plugins, or leaking of APIs that aren't meant to be public.

If Plugin A must depend on a uiSetting that's registered by Plugin B, it is better for Plugin A to consume it explicitly via an API exposed on Plugin B's contract, rather than implicitly using the global uiSettings exposed by core.

That said, I think we do already have a type of workaround for this that has been used a few places in the codebase, and that's where plugins export a const from their index.ts with the name of their uiSetting. Other plugins that need to read/write to that setting can import this const to pass to the uiSettings service, thus introducing a dependency between plugins via requiredBundles.

Overall I'm not sure I see this as a particularly high-priority enhancement, but certainly one worth considering if we end up doing a larger refactor (perhaps for user-personalized settings).

[pgayvallet]

I think the main problem this is aiming to solve is introduction of implicit dependencies between plugins, or leaking of APIs that aren't meant to be public.

Yea, that's exactly it. That's the same problem as having plugins directly consuming HTTP endpoints exposed from other plugins: It creates implicit (and untraceable) dependencies between plugins. All interaction between plugins should be done using explicit setup/start contracts.

That being said, properly isolating uiSettings access to their respective owners, and solving all the current violations of this contract pattern, seems like a large effort, that is, realistically, probably not worth it until it blocks us for any reason. Or at least I agree that this feels like low priority.

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)