Page MenuHomePhabricator

When a dashboard has inconsistent panel policies: keep doing nothing
ClosedPublic

Authored by epriestley on Apr 3 2019, 4:26 PM.
Tags
None
Referenced Files
F12854674: D20377.id48615.diff
Fri, Mar 29, 8:48 AM
Unknown Object (File)
Tue, Mar 12, 9:13 AM
Unknown Object (File)
Feb 10 2024, 3:15 AM
Unknown Object (File)
Feb 5 2024, 5:21 PM
Unknown Object (File)
Feb 3 2024, 10:00 PM
Unknown Object (File)
Jan 26 2024, 6:36 PM
Unknown Object (File)
Jan 2 2024, 3:47 PM
Unknown Object (File)
Dec 21 2023, 9:31 AM
Subscribers
None

Details

Summary

Depends on D20376. Ref T8033. It's possible to put a bunch of secret panels on a public dashboard, and not obvious that the dashboard won't be very useful.

This was more of an issue long ago (when the dashboard broke or all the panels completely vanished or something?). Nowadays, the panels render "You don't have permission to view this" so it's likely easy to explain/fix. Still, we can warn about this.

But, for now, don't, since a lot of this works better now and it's not really clear that this is particularly valuable. We can revisit this after all the connected changes have more of a chance to settle.

Test Plan

(Earlier behavior, not how things look in the final version.)

Screen Shot 2019-04-03 at 9.22.52 AM.png (1×1 px, 285 KB)

Diff Detail

Repository
rP Phabricator
Branch
portal23
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22517
Build 30830: Run Core Tests
Build 30829: arc lint + arc unit

Event Timeline

amckinley added inline comments.
src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
224–242

Since we show this warning dialogue every time we open the editor, I kinda feel like this should just be an error state and not allow panels to exist in dashboards that won't be visible to all the dashboard viewers.

This revision is now accepted and ready to land.Apr 3 2019, 7:28 PM

I think I'm possibly going to try to do this a little later on:

  • Give panels an optional containerPHID, which points at a dashboard (or another panel, in the case of tab panels).
  • If a panel has a containerPHID, it uses that container's policies, and the panel policies can not be edited.
  • When you create a panel on a dashboard, it gets the dashboard's containerPHID.

I may also (try to?) make all panels always only work like this. The problem is that would break some flows, like:

  • Sharing a "Company News" panel across several different dashboards.
  • Creating a standalone panel and embedding it in a task or wiki page.

I think these flows are conceptually reasonable? But maybe we should just give up on them. The picture may become clearer as Facts gets built out.

These flows could be provided in other ways, e.g.:

  • Provide a "Wiki Document Panel" or a "Paste Panel" that embeds a wiki document / paste, use that instead of text panels.
  • Remove the {W...} rule, assume it's only ever used to embed query panels, add a {query ...} rule. Might make sense if we end up doing chart embeds with {chart ...}.
  • Maybe both of these use cases are totally made up and we don't need to support them at all.

Of course, the first case gives us a new policy problem (when the paste or wiki document isn't visible to the viewer), so even though it might make panel management easier it probably doesn't make the policy situation easier.

At least for now, I'll be happy if we can stabilize this on EditEngine.


Since we show this warning dialogue every time we open the editor, I kinda feel like this should just be an error state and not allow panels to exist in dashboards that won't be visible to all the dashboard viewers.

I think this is generally hard.

One issue is that we can't determine whether most policies are equivalent or not. For example, "Visible to Members of Project: Sheep" and "Visible to Members of Project: Four-Legged Animals" may be the same effective policy, but we can't tell without iterating over every member. And even if they are the same policy today, they may be different policies tomorrow.

So I don't think we can make an assertion like "won't be visible to all viewers". We'd have to make an assertion like "panels must have the same policy as the dashboard, or a strictly weaker policy".

But I think this causes some issues. Suppose you maintain a "Company News" text panel which is "Visible to Members of Project: Employees". This panel is embedded on several dashboards: Engineering Dashboard, Design Dashboard, Operations Dashboard. Those dashboards are respectively "Visible to Members of Project: Engineering", and so on. This configuration is a legitimate configuration: everyone who can see these dashboards will be able to see the news, provided they're a member of a department and a member of the Employees project, which should always be true (but this is an organizational business rule that Phabricator does not know about).

Now, consider:

  1. You edit and save "Engineering Dashboard".
  2. You edit and save "Company News".

What happens in both cases?

In both cases, the panel is part of a dashboard with a different policy, and the policies may permit different sets of users to view the panel. If we raise an error, we're making a fairly reasonable/valid configuration (I think?) invalid. If we don't raise an error, adding a user to "Engineers" (only) and then viewing the dashboard as them produces a panel they can't see.

(One practical approach could be to make "Engineering" a subproject of "Employees" so the rule is enforced by Phabricator, but that not a fix in the general case of arbitrary "Members of X" policies.)

At least for now, I'll be happy if we can stabilize this on EditEngine.

That is, I'm definitely looking at this change as a "harm reduction" change and hoping we can find a better approach that prevents the issue later.

We'd have to make an assertion like "panels must have the same policy as the dashboard, or a strictly weaker policy".

Yeah, that was more what I had in mind, but I agree that your example use case is a desirable feature that would break that^^ rule. Thinking about it more, if we're building this out with the goal of making panels reusable/shareable at all, we have to support policy at the panel level.

My only objection to the code as-is is that we're going to be delivering this "warning" whenever anyone edits dashboards that have these not-strictly-superset policies, which seems not-great to me. I think the real issue is that we're showing a warning only to the editing user, when it's more likely than not that the editor is doing the right thing. How about we instead show the warning to the user who's looking at a dashboard that includes a panel they can't see? Something like:

"Warning: panel with ID PHID-0xDEADBEEF is present on this dashboard, but you don't have permission to see it. This is probably a misconfiguration; contact dashboard owner susan to correct this.

Maybe you have an example in your back pocket of when it would be desirable to have dashboards that include panels that some users who can see the dashboard can't see, but I'm having a hard time thinking of any. Maybe you have an "Engineering Dashboard" that hides some panels from users who aren't members of the "Engineering Managers" project? But then I feel like users should just make an "Engineering Managers" dashboard that has a superset of the "Engineering Dashboard" panels on it.

How about we instead show the warning to the user who's looking at a dashboard that includes a panel they can't see?

We do show approximately this warning already: the panel renders as "Restricted panel: You don't have permission to see this."

We don't show the "contact susan" bit, but Dashboards don't have a single user owner. (Although the viewer who lacks permissions could click the "Edit" icon to go the dashboard detail page and then click "Edit Dashboard" to learn the edit policy if they're savvy and don't know from context.)

This additional warning is just trying to prevent the issue in the first place, particularly in the case that the dashboard is Public and some panels are less-than-public, since random anonymous viewers may not have any easy way to contact Susan. That is, I think the primary original driver behind T8033 was users putting "All Users" panels on a "Public" dashboard, not some complex maze of mutual-membership in sub-subprojects with Lunar Phase policy rules or whatever.

It's possible that we don't really need this anymore -- at the time, I think one panel immediately banished the whole board to the shadow realm, while everything degrades pretty cleanly now.

(I don't think less-visible panels on a dashboard have any valid use cases, but we can't safely elevate panel visibility to the dashboard's visibility level because then you can create a board for just you, add every panel to it, and see every panel, even secret panels like W123 My Personal Diary in A Panel For Absolutely No Reason).

It's possible that we don't really need this anymore -- at the time, I think one panel immediately banished the whole board to the shadow realm, while everything degrades pretty cleanly now.

Having now read T8033, this all makes slightly more sense. I think your comment from 2015 is going to turn out to be prescient:

We could warn about policies differing, although this might have so many false positives that it isn't useful.

I think the use case of "warn editors that their public dashboard is about to have non-public panels" is the most(only?) compelling case for showing this warning, because that's probably never the editor's intent, and logged out users might not have a way to contact any users of the install to complain that a dashboard is broken.

I'd slightly prefer to change this to specifically catch the motivator of T8033, or just ship it as-is and see if anyone complains.

  • Just remove the weird warning for now, let's see if we actually need it once the dust settles.
epriestley retitled this revision from When a dashboard has inconsistent panel policies, show a warning to When a dashboard has inconsistent panel policies: keep doing nothing.Apr 9 2019, 9:04 PM
epriestley edited the summary of this revision. (Show Details)
epriestley edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.