Page MenuHomePhabricator

Inform editor when panels have more restrictive policy than dashboard
Closed, WontfixPublic

Description

There should at least be a warning saying something like "panel policy makes the dashboard policy more restrictive than it is set to".

When I had a dashboard with a single private panel it was hard to figure out why the dashboard wasn't showing for logged-out users,

Related Objects

Event Timeline

eadler created this task.May 2 2015, 3:03 AM
eadler raised the priority of this task from to Needs Triage.
eadler updated the task description. (Show Details)
eadler added a project: Dashboards.
eadler added a subscriber: eadler.
chad added a subscriber: chad.May 2 2015, 3:17 AM

Dashboards for certain users is T4103

eadler added a comment.May 2 2015, 3:18 AM

This may be related, but is a little more specific: right now adding a private panel to the dashboard makes the whole thing go private. IMHO it should render, but without the panel the user can't see.

chad added a comment.May 2 2015, 3:20 AM

That's not a design pattern we follow, make stuff just disappear when you don't have access.

eadler added a comment.May 2 2015, 3:21 AM

In this case you *do* have access (to the dashboard, and to all the other panels) to everything but a single panel. Just that one panel should disappear.

chad added a comment.May 2 2015, 3:25 AM

That's pretty much my point. It's more confusing if things randomly disappear depending on the viewer, leads to confusion between admins, users, projects. We'd rather solve the core problem and have whole different dashboards which always work, than trying to have users keep track of who has access to what panel, when, where, and all the confusion that would lead to.

eadler added a comment.May 2 2015, 3:26 AM

Ah, I see. In that case, this task should morph to: inform the user, when in "management" mode that one of the panels may cause the whole dashboard to be unviewable.

eadler renamed this task from Private panels shouldn't cause dashboards to become private to Inform editor when panels have more restrictive policy than dashboard.May 2 2015, 3:27 AM
eadler updated the task description. (Show Details)
chad added a comment.May 2 2015, 3:27 AM

Yeah I don't know how easy that is to figure out, but we should if we can.

avivey added a subscriber: avivey.May 2 2015, 8:34 PM

We could also replace the [body of] the private panel with "You do not have permissions to view this panel". It would be easier to implement, and only slightly less useful.

eadler added a comment.May 2 2015, 8:36 PM

@avivey that was my original proposal by I think @chad didn't like it.

chad added a comment.May 2 2015, 8:45 PM

Wasn't obvious to me that was your suggestion, I was only objecting to it vanishing.

chad added a comment.May 2 2015, 8:45 PM

Pre-warning the dashboard creators still seems like the right way to solve the problem though.

chad added a comment.May 2 2015, 8:54 PM

Or do both future engineer who may take on this task!

I think the original proposal was to make the panel disappear. This isn't something we'd be likely to implement, for the reasons @chad gave: it's super confusing and doesn't give users any tools to understand the behavior.

Making the panel render with a policy error message is entirely reasonable, and a step we should take. This makes it easy to understand that there's a problem, what the problem is, why it's happening, and how to fix it.

Helping administrators catch these errors while constructing a dashboard is more complex. In the general case, it's impossible to determine if one policy is a strict subset of another policy. We could warn about policies differing, although this might have so many false positives that it isn't useful.

There's also similar but less-severe issue where you create a panel using a functional query that uses viewer(), either explicitly or implicitly (by using a prebuilt query like "Active Revisions", which internally uses something equivalent to viewer()), the show the dashboard to logged-out users. This is probably the most common non-policy dashboard construction issue. There's a milder variant where you just, say, build a panel using a query that uses a project which some other users can't see.

A tool which might be useful for dealing with this is "View Dashboard As <Other User>...", but we can't implement this in the general case without violating policies. We could implement a weird hybrid "View Dashboard As <Other User> while still applying all your policies too...", but I think this would be worse than nothing, since it would often be misleading and wrong.

We can implement "View Dashboard as Logged-Out User...", although this is a bit complicated so I'm not sure if it's worthwhile.

More simply, we could just document or surface these warnings more prominently.

epriestley added a subscriber: dereckson.

T6560 is essentially the same issue, but specifically with tabbed panels.

flash moved this task from Backlog to Lyuba on the Dashboards board.Jun 18 2015, 8:25 PM
flash moved this task from Lyuba to Backlog on the Dashboards board.
flash moved this task from Backlog to Lyuba on the Dashboards board.
epriestley moved this task from Lyuba to Backlog on the Dashboards board.Jun 18 2015, 8:26 PM

What about the following solution:
Display the workboard to the user, if he is able to see it, but don't show the panels which he can't see?

Yes, I'd expect to do this:

  • Users who look at a dashboard but can't see some of the panels see placeholders for them instead ("Restricted Panel"). They can click this to figure out why they can't see it.
  • In the manage UI, where possible, try to callout policy problems to administrators (for example, by putting a note on the panel like "this panel has a more restrictive policy than the dashboard does").

The second behavior is difficult for reasons discussed above (briefly: it's hard to tell if two policies are stronger or weaker than one another in the general case), but we've added a partial ordering on strength to policies since that discussion, and it can power callouts in the most common cases ("Public" vs "All Users", notably).

In this case...

epriestley moved this task from Backlog to v2 on the Dashboards board.Feb 19 2016, 3:18 PM
epriestley edited projects, added Dashboards (v2); removed Dashboards.
Mnkras added a subscriber: Mnkras.Feb 23 2016, 3:32 AM

I just ran into a fun issue that @avivey helped me figure out,

I changed my instance to allow public access but ran into an issue where the homepage would show a login screen even though the policy on the dashboard application, and the default dashboard were set to public.

I had to go into the dashboard and change each of the panels from "All Users" to Public before it would stop showing the Login screen.

I expected that not to happen. Instead of the login screen I would expect a restricted screen of something telling me its a restrictive policy on a panel, and also on the dashboard's manage screen a notification that there are more restrictive policies.

An E_PERM panel seems reasonable here, but I'd like to add a use case for hiding.

Perhaps this wasn't the right solution but it was the one I thought of naturally using the app:

I wanted to inform users of alternative dashboard to install based on what projects they were members of, since I have a "default dashboard". In this case I was expecting it to just hide the panel and show the dashboard as normal. Instead I now have a box with the user doing the "if elseif elseif" with a list of default dashboards for my different users, with direct links to install them.

urzds added a subscriber: urzds.Aug 29 2016, 9:04 AM
chad moved this task from Backlog to Planned on the Dashboards (v2) board.Feb 3 2017, 7:08 PM
chad added a comment.Feb 11 2017, 2:55 AM

Do we have a good reason for having separate polices for panels?

Unless you can define a different default dashboard for logged in and logged out users, yeah.
The problem I see:

  • I run a phabricator instance where the main part is only accesible for logged in users, but logged out users can still see some tasks.
  • My problem: I want to inform my users about different upcoming events etc, for example per text panel
  • The logged out users should not be able to see these messages, so I restricted some panels
  • The whole problem would be easy to solve, if admins can define: logged in users get dashboard a), logged out users get dashboard b) in my situation

Panels can currently appear on multiple dashboards, so we can't use the dashboard's policy for them directly. If we could, you could always see any panel by creating a new dashboard you have permission to see, adding every panel to it, and looking at the panel you wanted.

Panels can also be embedded anywhere, with {Wxxx}, in which case they can't use any container's policy since there is no container except whatever we're embedding the thing in (and, again, you could see anything by putting {Wxxx} on a wiki page you could see). Although this does not see tons and tons of use, I believe it does see some, and is a pretty good solution to some problems (e.g., putting dynamic lists of stuff on wiki pages).

We could stop allowing panels to appear on multiple dashboards, get rid of {Wxxx}, and instead make panels specific to one dashboard and make "copy dashboard" create copies of every panel too. This would be simpler in a lot of ways (no more policy weirdness), but {Wxxx} would stop working and we'd start getting a lot of copies of panels. I think "Copy Dashboard" is generally a questionable feature today, and contributes to the difficulty of managing/finding dashboards because there are so many meaningless "Copy of Copy of New Template Dashboard" dashboards. I'm really happy that we were able to build the new Profile Menu stuff without needing to implement a "Copy" operation, and it feels like we have a much better balance of things there, so I'm hesitant to make "Copy" start doing even more copying. I also think {Wxxx} is conceptually very powerful and I'm hesitant to throw it away even though I don't think it has seen a great deal of use yet.

We could make all panels have the most-open policy (e.g., public or all users) with no separate visibility policy. This is basically fine for most panels (query panels) but maybe not so good for "Text" panels (which may have sensitive/internal information).

I'm also not sure what the policy model is going to be on "Fact" panels (charts/graphs) or "Facts" queries in general. I believe charts will generally have to ignore policies: if you ask for a chart of task close rates and we take the time to policy check 100K tasks before showing it to you, the chart will take a week to generate. So this will create some policy problems with charts like "Open tasks tagged with: security", and maybe stuff like "Quarterly revenue for my team" being OK but "Quarterly revenue for the enemy team" not being OK. We'll have to navigate these issues when we get there.

I think Facts will probably handle these issues inside the Facts application (maybe with a "Report" object which has policies and defines a base dataset, and then controls to adjust how a report is shown, so you'd give your team access to your revenue by having someone with a lot of permission generate a "Revenue for team X" report, making it "Visible to: Team X", and then the team could fiddle with the report from there to change date ranges and colors and stuff like that). But it's possible that access to Facts will end up basically being all-or-nothing and the policies will have to go on very specific output, and putting them on panels might make sense.

We could, perhaps, do something like this:

  • Get rid of "Copy": you now have to lovingly craft new dashboards by hand from scratch. I think there's at least somewhat less need for "Copy" now with the new dashboard mechanics on Home: you can add a few extra panels on a second dashboard without needing to replace the main dashboard, now. If we also fix stuff like T7216 and generally make query panels easier to build, this should also reduce the need for "Copy" at least somewhat.
  • Panels become either bound to a dashboard (if created from a dashboard) or standalone (if created directly). Panels on a dashboard are permanently associated with that dashboard and have that dashboard's view/edit policies. Standalone panels have customizable policies as they do now.

I think that's a better world in a lot of ways, at the cost of initial dashboard creation getting a bit harder? But I'm not sure that's really a problem now because you can stick multiple dashboards on Home, keep the old Home too if you want, and have more flexibility with stuff like Favorites. Before, if you wanted to install a personal dashboard over Home, you probably needed to be able to put most of the Home panels on it to get anything useful since that information would be gone otherwise, and I think that was the motivation for "Copy".

We could even keep "Copy" and just filter the panel list to "standalone panels only" by default, but I think "Copy" is bad for our health in the long term and I'd like to at least try getting rid of it.

chad added a comment.EditedFeb 11 2017, 3:02 PM

I'm hoping to make panel creation dramatically simpler, so building a new dashboard from scratch is maybe a 1-2 minute task. Getting rid of copy and maybe also hiding policy controls when creating panels in context of the Arrange Panels page seems like a good way to minimize this.

If this were a new project, panel policies could probably be skipped until v2 and community interest was shown.

hiding policy controls when creating panels in context of the Arrange Panels page seems like a good way to minimize this

Yeah, I'd imagine that the policy controls just vanish if you create a panel from a dashboard: the panel is "part of the dashboard" instead.

If this were a new project, panel policies could probably be skipped until v2 and community interest was shown.

I don't really remember, but I think one of the motivations might have been to get tab panels in v1? If panels don't have policies and we try to avoid this whole issue, we can't cheat like we currently do on tab panels and have you build them by just picking other panels: we have to actually build UI to let you create a panel while creating another panel. This is pretty clearly where we should end up no matter what since the current UX is pretty sketchy, but maaaaaaybe enough work that'd we still choose to cheat on v1 today if we were starting from scratch.

chad triaged this task as Normal priority.Feb 15 2017, 2:14 AM
epriestley moved this task from v2 to Next on the Dashboards board.Mar 31 2019, 9:56 PM
epriestley edited projects, added Dashboards; removed Dashboards (v2).
epriestley closed this task as Wontfix.Apr 10 2019, 4:52 PM
epriestley claimed this task.

D20377 initially added the warning this task originally suggests, but we weren't really thrilled about it so it eventually landed as some minor cleanup with no actual new warning.

Broadly, our behavior in the presence of mixed-policy panels is now much better than it was in the past, and it's not clear that this warning is actually helpful anymore. At the least, I'd like to wait until the round of dashboard/portal changes under T13083 stabilizes before revisiting this. I currently think it's likely that I'll introduce a containerPHID onto panels which binds panel policies to some container's policies, further mooting this.