[Dashboard] Add Readonly State For Managed Dashboards #166204

ThomThomson · 2023-09-11T20:02:18Z

Summary

This PR adds a new managed mode to Dashboards. When a user with write permissions opens a managed Dashboard they will be unable to hit the edit button - and will instead be prompted to clone the Dashboard first.

Managed State

The managed state is determined via the new root level managed property on the saved object which was added in this PR.

Non-Editable

As part of this PR, managed Dashboard should not be editable in any capacity. For managed Dashboards, this PR disallows:

Clicking edit when in a managed dashboard in view mode
Clicking on the pencil icon on a managed dashboard from the dashboard listing page.
Modifying description / tags via the details section of the listing page.
Adding an edit mode param directly into the URL via &_a=(viewMode:edit)

Badge

A new badge appears when viewing a managed Dashboard:

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

ThomThomson · 2023-09-12T19:43:53Z

@amyjtechwriter do you have any suggestions for the copy on this badge? It is currently:

This dashboard is system managed. Clone this dashboard to make changes.

In the future we'll likely rename this to duplicate along with all of the other usages of the clone verbiage.

@andreadelrio do you think a badge is an alright solution for this? I figured that users in this managed dashboard will never see the unsaved changes badge so it was an okay place to put it. When the user has no write permissions, the glasses icon appears roughly in that spot as well.

TinaHeiligers · 2023-09-13T19:41:13Z

@ThomThomson Thank you for taking read-only dashboards on, I've been waiting on it since merging #154515.

LMK if you've got questions with SOs' managed property. It's only integrated through SO import at the moment (the only way for plugin's to set managed:true.

ThomThomson · 2023-09-13T20:09:33Z

Yeah! Sorry for such a long turnaround on this one, it fell through the cracks. I've been testing with a managed Dashboard that came with the OBLT data - it indeed does have the managed flag properly set.

elasticmachine · 2023-09-13T20:10:07Z

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

ThomThomson · 2023-09-13T20:10:22Z

@elasticmachine merge upstream

nreese · 2023-09-13T20:45:30Z

Users can still get access to managed dashboard in edit mode by clicking "edit" action on dashboard listing page.

Users can also get access to edit mode view "Copy to dashboard" button from another dashboard and save dialog

Then, once in edit mode, managed dashboard can be modified.

several things should happen

managed dashboards should appear in "Copy to dashboard" list as disabled with a tooltip saying why its disabled. This will also apply to "save to" dialog when saving lens, maps, etc
dashboard listing page should not provide edit action to managed dashboards
a managed dashboard should ignore viewmode or reset viewmode to view. This will cover future use cases where an application sends users to a managed dashboard in edit mode.

ThomThomson · 2023-09-13T21:56:41Z

All good feedback @nreese!

Done. The edit action only shows up for non-managed Dashboards. I also made the new inspector readonly for managed dashboards.
Done-ish. The Dashboard picker uses an EUI combo box which has no easy way afaict to make an option disabled, we'd need to get into custom item rendering etc. Instead, I opted to simply omit managed Dashboards.
This is a good idea, and something I was looking into (e.g. you can also inject edit mode directly into the URL to force a managed Dashboard into edit mode). I ended up fixing it by stopping the event in both the updateInput method, and in the reducer whenever the user tries to enter edit mode when the dashboard is managed.

andreadelrio · 2023-09-13T22:14:47Z

@andreadelrio do you think a badge is an alright solution for this? I figured that users in this managed dashboard will never see the unsaved changes badge so it was an okay place to put it. When the user has no write permissions, the glasses icon appears roughly in that spot as well.

I think that works 👍

…temIsEditable methods for tableListView, update jest tests

ThomThomson · 2023-09-18T14:21:02Z

@elasticmachine merge upstream

nreese · 2023-09-18T18:00:47Z

packages/content-management/table_list_view_table/src/table_list_view_table.tsx

@@ -457,6 +458,8 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
        return item.references.find(({ id: refId }) => refId === _id) as SavedObjectsReference;
      });

+      const isReadonly = contentEditor.isReadonly || !itemIsEditable?.(item);


I don't think !itemIsEditable?.(item) check is right. This will return true when itemIsEditable is not provided. Items should not be readonly when itemIsEditable is not provided.

Good catch, fixed this in 7029844

nreese · 2023-09-18T18:02:33Z

packages/content-management/table_list_view_table/src/table_list_view_table.tsx

   */
-  showEditActionForItem?(item: T): boolean;
+  itemIsEditable?(item: T): boolean;


Why is itemIsEditable dashboard implemenation checking for item.managed? Shouldn't that information already be in item? So instead of this change, just check that item.managed here instead of making itemIsEditable implemenation do it. This will reduce code duplication.

It made sense to me to unify the callback into one function that determined if the itemIsEditable for both the edit action + the inspect item action.

If the table list view gets any more edit functionality in the future, the same function can be reused.

The generic type UserContentCommonSchema isn't necessarily a saved object. We could require UserContentCommonSchema to have a managed?: boolean field

It allows the implementor to determine for themselves what makes an item readonly or managed etc. For instance, Visualizations have a readonly attribute.

We could leave the function as itemIsEditable, and also have a default implementation that checks managed state.

The problem with leaving managed behavior up to each implementation is that will result in inconsistent behavior across Kibana and potential bugs as teams forget to add checks for managed.

I would be in favor of adding managed to UserContentCommonSchema. Then the table component can provide consistent behavior for managed flag in a single location.

I can add managed to UserContentCommonSchema and check it within the table list view, but it's important to note that it won't automatically make any managed saved objects non-editable. This is because the implementor is also in charge of actually fetching the objects, and mapping them to UserContentCommonSchema.

Would it make sense to make managed required? That why tslint would fail when findItems does not provide a value?

I prefer option 2.

Wasn't there mention of 'by reference' saved objects in some of the managed dashboards? That would require support for managed saved objects for other types as well. Having a consistent behavior across listing pages would be ideal.

there will be a pattern for this managed state

Are you saying that this is a new top level SO property handled by core available on all saved object? That's great to hear 👍

Sorry about mentioning Drew in my comment above. I went too quick when looking at the line change in the IDE

Regarding the showEditActionForItem, looking at it again it seems to be a tech debt that we have. It will have to be removed in favor of using the existing rowItemActions (which need to support the edit action).

Thanks for clarifying, I agree then to go with option 2. Could you update the comments on top of the showEditActionForItem and editItem props to indicate that those will not have any effect if the content item has the managed prop set to true?

Slightly off topic:
Do all SO that have the managed prop set to true automatically get the managed tag (in their references array)?

Okay, I've made the managed state override the itemIsEditable callback, and have added a comment to explain that in 4679bb1.

Yes, the managed state is a new top level SO property handled by core, thanks to @TinaHeiligers for that! And I'm not quite sure how the managed tag appears, but I assume it's more of a holdover from before the new property. Tina would know more.

Thanks. What I meant for the comments is to add that information to the TableListViewTableProps interface. We want to warn consumers that those props won't have any effect if the SO has managed set to true.

// packages/content-management/table_list_view_table/src/table_list_view_table.tsx ... /** * Edit action onClick handler. Edit action not provided when property is not provided */ editItem?(item: T): void; /** * Handler to set edit action visiblity per item. */ showEditActionForItem?(item: T): boolean;

Done in 5cbd79c

TinaHeiligers · 2023-09-18T19:41:47Z

packages/kbn-content-management-utils/src/saved_object_content_storage.ts

@@ -69,11 +69,13 @@ function savedObjectToItem<Attributes extends object>(
    error,
    namespaces,
    version,
+    managed,


By default, managed is false. The only way to change to true is to use SO import, with an option with conditions

drewdaemon · 2023-09-19T12:47:12Z

I also shouldn't be able to add a read-only visualization to a dashboard (see #70461 (comment)).

If it's out of scope here, maybe we should create another issue?

ThomThomson · 2023-09-19T13:43:34Z

@drewdaemon the way I understand that comment is that if a by reference visualization has a managed tag, it should be added to the Dashboard by value instead so the user cannot edit the original vis.

An alternative way we could go about this is by disallowing an edit action on managed visualizations. Either way, I'd say that could be done in a follow-up. Do you know of many managed by reference visualizations?

drewdaemon · 2023-09-19T15:46:58Z

@ThomThomson thinking about this more I'm not sure it matters whether users add managed assets to unmanaged dashboards as long as each editor enforces save-as functionality. Either way, I'm wondering if an issue to discuss this particular request makes sense?

the way I understand that comment is that if a by reference visualization has a managed tag, it should be added to the Dashboard by value instead so the user cannot edit the original vis.

This approach sounds reasonable, though it might be a bit magical? I think you meant this, but just for clarity, it should be based on the SO prop, not the tag.

An alternative way we could go about this is by disallowing an edit action on managed visualizations.

I don't think this constraint from the dashboard side is necessary. It's fine for the user to open a managed vis in the editor as long as they save their changes as a new visualization. We will make sure this happens in #166720.

I'd say that could be done in a follow-up.

Agreed. Just looking for an issue to track. 👍

Do you know of many managed by reference visualizations?

472 in the integrations today. We incentivize by-value in the metrics we track but there will always be legitimate cases for by-ref visualizations. In fact, one of the reasons I was given for the reason integrations try to avoid by-ref is the worry that people will edit them. That becomes moot with this effort to prevent editing.

nreese

LGTM
code review and tested in chrome

davismcphee

Code-only review, kbn-content-management-utils changes LGTM 👍

…ble. Make isEditable callback

sebelga · 2023-09-20T17:06:06Z

packages/content-management/table_list_view_table/src/table_list_view_table.tsx

@@ -451,6 +454,15 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
    items,
  });

+  const isEditable = useCallback(


sebelga

LGTM! Thanks for making all the adjustments in the table list view 👍

kibana-ci · 2023-09-20T17:33:28Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: ed2555d

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #8 / core plugins application using leave confirmation when navigating to another app allows navigation if user click confirm on the confirmation dialog
[job] [logs] FTR Configs #23 / runPipeline "before all" hook in "runPipeline"
[job] [logs] FTR Configs #51 / serverless security UI Configure Case "before all" hook in "Configure Case"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/content-management-tabbed-table-list-view`	14	15	+1
`@kbn/content-management-table-list-view`	9	10	+1
`@kbn/content-management-table-list-view-table`	40	41	+1
`@kbn/content-management-utils`	123	124	+1
total			+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	354.0KB	355.5KB	+1.4KB
`eventAnnotationListing`	197.1KB	197.3KB	+143.0B
`filesManagement`	89.4KB	89.6KB	+177.0B
`graph`	387.5KB	387.7KB	+177.0B
`maps`	2.8MB	2.8MB	+177.0B
`presentationUtil`	73.1KB	73.2KB	+84.0B
`visualizations`	265.5KB	265.6KB	+136.0B
total			+2.3KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/content-management-tabbed-table-list-view`	14	15	+1
`@kbn/content-management-table-list-view`	9	10	+1
`@kbn/content-management-table-list-view-table`	58	59	+1
`@kbn/content-management-utils`	188	189	+1
total			+4

History

💔 Build #160445 failed 4679bb1
💚 Build #159729 succeeded c895a57
💚 Build #159567 succeeded ab07806
💔 Build #159224 failed 9d28907
💔 Build #159207 failed f51b9a7

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

ThomThomson added 3 commits September 11, 2023 16:01

Add managed state to Dashboard

0216597

fix types

a5556ca

fix jest test

c2e78eb

fix jest test again

f555386

ThomThomson marked this pull request as ready for review September 13, 2023 20:09

ThomThomson requested review from a team as code owners September 13, 2023 20:09

Merge branch 'main' into dashboard/managedState

76acf9d

nreese self-requested a review September 13, 2023 20:13

read only for managed Dashboards in table list view.

3d25eb1

ThomThomson requested a review from a team as a code owner September 13, 2023 21:25

ThomThomson added 2 commits September 13, 2023 17:38

omit managed Dashboards from dashboard picker

ae8210f

block dashboard from entering edit mode when managed

bcfc7af

lock down edit mode when write controls are off. Rename and combine i…

f51b9a7

…temIsEditable methods for tableListView, update jest tests

ThomThomson requested a review from a team as a code owner September 14, 2023 16:27

undo module level showWriteControls

9d28907

ThomThomson mentioned this pull request Sep 14, 2023

[Dashboard Navigation] Unit tests #166297

Merged

ThomThomson mentioned this pull request Sep 14, 2023

[Dashboard] Store view mode in local storage #166523

Merged

1 task

Merge branch 'main' into dashboard/managedState

ab07806

nreese reviewed Sep 18, 2023

View reviewed changes

ThomThomson added 2 commits September 18, 2023 14:56

default isEditable to true

7029844

check for managed state inside table list view

c895a57

TinaHeiligers reviewed Sep 18, 2023

View reviewed changes

drewdaemon approved these changes Sep 19, 2023

View reviewed changes

ThomThomson requested a review from nreese September 19, 2023 14:07

nreese approved these changes Sep 19, 2023

View reviewed changes

davismcphee approved these changes Sep 19, 2023

View reviewed changes

ThomThomson added 3 commits September 20, 2023 11:20

make managed state override itemIsEditable

4679bb1

Update comment to mention that managed items will always be non-edita…

5cbd79c

…ble. Make isEditable callback

Merge remote-tracking branch 'upstream/main' into dashboard/managedState

ed2555d

sebelga reviewed Sep 20, 2023

View reviewed changes

sebelga approved these changes Sep 20, 2023

View reviewed changes

ThomThomson merged commit 8bd2515 into elastic:main Sep 21, 2023
23 checks passed

kibanamachine added v8.11.0 backport:skip This commit does not require backporting labels Sep 21, 2023

drewdaemon mentioned this pull request Oct 5, 2023

[Lens] Prevent overwriting managed content from editor #166720

Closed

ThomThomson mentioned this pull request Oct 19, 2023

[Dashboard] Read Only Users Should Not have Access to Edit Mode #158626

Closed

drewdaemon mentioned this pull request Nov 3, 2023

Allow integration developers to edit managed content #170517

Closed

drewdaemon mentioned this pull request Mar 5, 2024

[Managed content] users can't inspect the configuration of managed dashboards #178043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Add Readonly State For Managed Dashboards #166204

[Dashboard] Add Readonly State For Managed Dashboards #166204

ThomThomson commented Sep 11, 2023 •

edited

ThomThomson commented Sep 12, 2023

TinaHeiligers commented Sep 13, 2023 •

edited

ThomThomson commented Sep 13, 2023

elasticmachine commented Sep 13, 2023

ThomThomson commented Sep 13, 2023

nreese commented Sep 13, 2023 •

edited

ThomThomson commented Sep 13, 2023

andreadelrio commented Sep 13, 2023

ThomThomson commented Sep 18, 2023

nreese Sep 18, 2023

ThomThomson Sep 18, 2023

nreese Sep 18, 2023 •

edited

ThomThomson Sep 18, 2023

nreese Sep 18, 2023

ThomThomson Sep 18, 2023

nreese Sep 18, 2023 •

edited

nreese Sep 20, 2023

sebelga Sep 20, 2023

ThomThomson Sep 20, 2023

sebelga Sep 20, 2023

ThomThomson Sep 20, 2023

TinaHeiligers Sep 18, 2023

drewdaemon commented Sep 19, 2023

ThomThomson commented Sep 19, 2023

drewdaemon commented Sep 19, 2023

nreese left a comment

davismcphee left a comment

sebelga Sep 20, 2023

sebelga left a comment

kibana-ci commented Sep 20, 2023

API count

[Dashboard] Add Readonly State For Managed Dashboards #166204

[Dashboard] Add Readonly State For Managed Dashboards #166204

Conversation

ThomThomson commented Sep 11, 2023 • edited

Summary

Managed State

Non-Editable

Badge

Checklist

ThomThomson commented Sep 12, 2023

TinaHeiligers commented Sep 13, 2023 • edited

ThomThomson commented Sep 13, 2023

elasticmachine commented Sep 13, 2023

ThomThomson commented Sep 13, 2023

nreese commented Sep 13, 2023 • edited

ThomThomson commented Sep 13, 2023

andreadelrio commented Sep 13, 2023

ThomThomson commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Sep 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Sep 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewdaemon commented Sep 19, 2023

ThomThomson commented Sep 19, 2023

drewdaemon commented Sep 19, 2023

nreese left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 20, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

API count

History

ThomThomson commented Sep 11, 2023 •

edited

TinaHeiligers commented Sep 13, 2023 •

edited

nreese commented Sep 13, 2023 •

edited

nreese Sep 18, 2023 •

edited

nreese Sep 18, 2023 •

edited