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

Read-only UI behaviour for system managed dashboards installed as part of fleet packages #140364

Closed
Tracked by #172393
rudolf opened this issue Sep 9, 2022 · 26 comments · Fixed by #166204
Closed
Tracked by #172393
Assignees
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort NeededFor:Fleet Needed for Fleet Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects

Comments

@rudolf
Copy link
Contributor

rudolf commented Sep 9, 2022

In #70461 the fleet team asked for the ability to mark saved objects installed as part of a fleet package as "system managed" and for such saved objects to have special behaviours. Although this could apply to many types of saved objects the primary ask was for dashboards (and any references it could have to visualizations, data views, searches etc).

The request could be broken down into two parts:

  1. (Tracked in [Saved Objects] Prevent users from altering managed assets #70461) a server-side control that guarantees no changes could be made to a system managed saved object as all such changes might be lost on a package upgrade
  2. (This issue) UI changes:
    1. highlight to a user that the dashboard they're viewing is system managed and
    2. a user flow where users are guided to make a copy of the system managed dashboard first, and then being allowed to make changes to their copy

Since (2) will have to be application specific and does not need to be blocked on (1) I created this separate issue so that we can track these as separate concerns.

@rudolf rudolf added Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas NeededFor:Fleet Needed for Fleet Team labels Sep 9, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson
Copy link
Contributor

One piece of this that might require some changes in the saved object system, and should be figured out first is how we determine which objects are read only / system managed. I don't believe there is any system in place that we could extend to support this.

Once we can Figure out some way to store that information, accessing it and changing the UI to limit edits / guide users to clone the dashboard will be fairly straightforward.

@ThomThomson ThomThomson added loe:needs-research This issue requires some research before it can be worked on or estimated impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Sep 9, 2022
@rudolf
Copy link
Contributor Author

rudolf commented Dec 20, 2022

how we determine which objects are read only / system managed

Although there isn't any read-only capability, today fleet packages use a tag to indicate that a saved object is "system managed" and discourage users from editing it. This tag has a fixed ID which could make it somewhat simpler to detect it's presence https://github.com/elastic/kibana/blob/main/x-pack/plugins/fleet/server/services/epm/kibana/assets/tag_assets.ts#L22

The other option would be to have a new readonly field in the saved object itself. This would require updating the saved objects inside fleet packages but might provide more control. Perhaps not every "system managed" dashboard should be read-only.

@elastic/fleet Do you have an opinion on what might be the best way to persist a dashboard's read-only status?

@rudolf
Copy link
Contributor Author

rudolf commented Dec 20, 2022

some background context: the presentation team recently added the ability for dashboards to have a read-only UI that hides the edit button. So if we can agree on the best mechanism for fleet packages to indicate that they prefer this read-only UI the presenation team thinks it would be trivial to hook that up.

@hop-dev
Copy link
Contributor

hop-dev commented Dec 21, 2022

Currently on Fleet we use _meta.managed to mark index and component templates as managed e.g:

{
      "name": "logs-elastic_agent.apm_server@custom",
      "component_template": {
        "template": {
          "settings": {}
        },
        "_meta": {
          "package": {
            "name": "elastic_agent"
          },
          "managed_by": "fleet",
          "managed": true
        }
      }
    }

From a saved objects perspective I can see why we might want something generic like readonly, to me readonly suggests the object could never be updated, whereas managed suggests it cannot be updated by the user but can be by kibana.

@rudolf
Copy link
Contributor Author

rudolf commented Dec 21, 2022

@hop-dev All fleet package installed saved objects get the managed tag right? But does it mean that we want to prevent users from editing all these saved objects? There is a sense that if I edit a managed dashboard I could break it. But maybe for some packages we might want to encourage users to modify a dashboard whereas others we really want to avoid them touching it.

@rudolf
Copy link
Contributor Author

rudolf commented Dec 21, 2022

Also to clarify this is purely a UI feature. "readonly" won't prevent a user from editing a dashboard through the API or by importing it. But it could protects against casual users accidently changing something that's critical for an integration.

@hop-dev
Copy link
Contributor

hop-dev commented Dec 21, 2022

All fleet package installed saved objects get the managed tag right?

Yes that's right

But does it mean that we want to prevent users from editing all these saved objects?

Definitely for dashboards the aspiration is that the user has to copy them to edit them as discussed in #70461. I can't think of an asset type where we wouldn't want to have this approach, but dashboards are the ones most likely to be edited by the user.

But maybe for some packages we might want to encourage users to modify a dashboard

I can't think of an existing package where this would be the case.

Also to clarify this is purely a UI feature. "readonly" won't prevent a user from editing a dashboard through the API or by importing it

Understood 👍

@ThomThomson
Copy link
Contributor

Am I understanding correctly, that the information for which saved objects are managed / read-only is already present? Either in the managed tag, or in this _meta.managed key? If so, can we get access to that in a standardized way client-side?

@kpollich
Copy link
Member

kpollich commented Jan 3, 2023

Am I understanding correctly, that the information for which saved objects are managed / read-only is already present? Either in the managed tag, or in this _meta.managed key? If so, can we get access to that in a standardized way client-side?

@ThomThomson - Fleet does include the managed tag for all installed assets here:

async function ensureManagedTag(
opts: Pick<TagAssetsParams, 'spaceId' | 'savedObjectTagClient'>
): Promise<string> {
const { spaceId, savedObjectTagClient } = opts;
const managedTagId = getManagedTagId(spaceId);
const managedTag = await savedObjectTagClient.get(managedTagId).catch(() => {});
if (managedTag) return managedTagId;
const legacyManagedTag = await savedObjectTagClient.get(LEGACY_MANAGED_TAG_ID).catch(() => {});
if (legacyManagedTag) return LEGACY_MANAGED_TAG_ID;
await savedObjectTagClient.create(
{
name: MANAGED_TAG_NAME,
description: '',
color: TAG_COLOR,
},
{ id: managedTagId, overwrite: true, refresh: false }
);
return managedTagId;
}

Fleet also includes managed: true and managed_by: "fleet" in the _meta property for installed assets as well here:

/**
* Build common metadata object for Elasticsearch assets installed by Fleet. Result should be
* stored on a `_meta` field on the generated assets.
*/
export function getESAssetMetadata({
packageName,
}: { packageName?: string } = {}): ESAssetMetadata {
const meta: ESAssetMetadata = {
managed_by: MANAGED_BY_DEFAULT,
managed: true,
};
if (packageName) {
meta.package = {
name: packageName,
};
}
return meta;
}

Let me know if this is enough to answer your questions above!

@ThomThomson
Copy link
Contributor

That's great that the information is present! @rudolf, is there any chance we could add a simple method to saved objects, or even the saved objects client that can return a boolean like isManaged?

@rudolf
Copy link
Contributor Author

rudolf commented Jan 10, 2023

I agree that a standardised isManaged API method would be the best way. The implementation is somewhat complex because the managed tag id changes based on the space and could also be a "legacy" id #144066

It would make the most sense for the isManaged API method to live next to this managed tag id logic to ensure they stay consistent. At the moment that's the fleet plugin. But if we're changing the behaviour of other Apps like Dashboard then I can see value in having this more of a Kibana platform type concept than something that's dependant on the fleet plugin. In that case it might make most sense to move the managed tag logic and isManaged API to the saved object tagging plugin.

@rudolf
Copy link
Contributor Author

rudolf commented Jan 12, 2023

We discussed this in our team meeting. The managed tag has the nice advantage of automatically being visibily across most of the UI as a clear sign to a user. But it comes with some disadvantages like needing different tag ids per space which then makes it complex to determine if a saved object is really managed. It seems to us like a better "contract" between fleet and other plugins' Apps might be if we add a new root level property to all saved objects like is_managed. Then when fleet installs packages it ensures this field is always set to true.

We wouldn't provide an API method / sugar around this property, probably just documenting the purpose of the property is enough so that the dashboard app can just read the value and change it's behaviour accordingly.

@rayafratkina
Copy link
Contributor

cc @sixstringcode @vadimkibana

@sixstringcode sixstringcode self-assigned this Jan 12, 2023
@sixstringcode
Copy link

+1. This is something that we have been considering part of the long-term roadmap for the Content Management project. Will get something together for when Vadim is back to talk about if the 'managed' property should be part of that effort.

@rudolf rudolf self-assigned this Feb 7, 2023
@rudolf
Copy link
Contributor Author

rudolf commented Feb 15, 2023

Plan:

  1. Add a managed root level field to all saved objects

  2. Add a managed: boolean flag to the import typescript API to set the managed value on all imported saved objects which defaults to false.
    Note: we will not expose this from the import HTTP API, this option is meant to be available to plugins only.

    This would allow fleet to set managed=true value on all imported saved objects that are installed as part of a package.

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Mar 10, 2023

If priorities allow for it (and we still need to do the work), I can pick this up after returning to the office near the end of March.

Please note that changes to the saved objects repository are needed too and those may have higher priority.

I'll be OOO from 13 to 20 March.

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Apr 6, 2023

@ThomThomson, @joshdover FYI, I'm working on #140364 (comment) in #154515 and will follow up with a second PR to finish those parts off.

@ThomThomson
Copy link
Contributor

ThomThomson commented Apr 6, 2023

@TinaHeiligers, amazing, looking forward to it!

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Apr 25, 2023

@ThomThomson, @joshdover, the PR for importing objects as managed is open for review merged!
I added as much detail as I could [update: to the PR description]. Let me know if you have any questions!

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Apr 27, 2023

@elastic/kibana-presentation do you want to use this issue to continue with the UI work or will you open a new issue for that?

@nreese
Copy link
Contributor

nreese commented Apr 28, 2023

do you want to use this issue to continue with the UI work or will you open a new issue for that?

I have added this question to your team sync discussion notes. It will be a few days before we reply with an answer

@nreese
Copy link
Contributor

nreese commented May 2, 2023

@TinaHeiligers we discussed as a team and would like to track presentation effort with this issue. So please keep this open for us

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented May 4, 2023

@nreese I'm adding your name to the issue for visibility.
Please keep me in the loop on the progress here.

@TinaHeiligers TinaHeiligers removed their assignment Jun 6, 2023
@ThomThomson ThomThomson added loe:medium Medium Level of Effort and removed loe:needs-research This issue requires some research before it can be worked on or estimated labels Sep 7, 2023
@ThomThomson ThomThomson self-assigned this Sep 11, 2023
@ThomThomson ThomThomson added loe:small Small Level of Effort and removed loe:medium Medium Level of Effort labels Sep 11, 2023
@drewdaemon
Copy link
Contributor

drewdaemon commented Sep 19, 2023

Correct me if I'm wrong, but I think the scope of the Fleet ask is larger than dashboards. Integrations can include

  • Visualizations
    • Lens
    • Legacy editors
  • Maps
  • Dashboards
  • Saved searches
  • Data views
  • Tags
  • and several solution-specific asset types such as security rules

I happened upon this issue and have created a similar one for the visualization editors, but I would expect all the relevant Kibana content editors to respect managed content. I'm wondering if there's room for a meta issue for this?

@joshdover
Copy link
Member

@drewdaemon You are right that the scope is larger. I think the thinking here was that Dashboards are the most commonly used asset type and the one users are most likely to customize so we'd start there. I could be wrong, but my assumption was also that with the move to viz-by-value that fixing this for dashboards covers most integration viz's.

  • Maps

We only have 7 integrations that contain maps today so I think this is low prio.

  • Data views

We actually want users to be able to modify some parts of Data views, such as runtime fields. See #120340. Any input you have there could be helpful as well.

  • Tags

Yeah we should probably prioritize this one 👍

  • and several solution-specific asset types such as security rules

Security rules have their own flow for handling this that supports 3-way merge of user-modifications with the updated rule on package upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort NeededFor:Fleet Needed for Fleet Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
No open projects
Dashboard
  
Inbox
Development

Successfully merging a pull request may close this issue.