Page MenuHomePhabricator

Setting a restricted dashboard as the topmost item in the global menu can lock some users out
Closed, ResolvedPublic

Description

We're trying to create default dashboards for a couple of user roles we defined (e.g. Engineering, Sales, etc) so we decided to create dashboards that were only visible to these groups. The problem we had was that we put the Engineering dashboard first in the global menu and those users that didn't have a custom dashboard installed would see an Access Restricted page, with nowhere to go (since going to the Home would try to open the dashboard that they couldn't see)

Reproduction steps

  1. Log in as admin and create a dashboard that only members of a given project can see
  2. Set that dashboard as the topmost item in the global menu
  3. Log in as a user that can't see that dashboard and doesn't have a dashboard installed
  4. You should now see the error page

Version information: Phacility hosted

We did have a Home dashboard visible to everyone that we expected would be shown to those users that couldn't see the other dashboards.

Event Timeline

D18152 should improve this: we'll still select the item as the default item, but will now render the navigation normally so the user can click other items:

Screen Shot 2017-06-23 at 12.06.40 PM.png (1×1 px, 134 KB)

I expect that to deploy tomorrow morning.

I think the better fix here is to stop selecting this as the default, and instead select the first item the user can actually see. However, we don't currently have a method to test "can the current viewer see this item?" so this is a larger change that I don't want to try to sneak in before the stable cut in a few hours.

epriestley lowered the priority of this task from Normal to Low.Jun 26 2017, 2:02 PM
epriestley removed a project: Customer Impact.

For private reasons affecting the reporting install, this no longer has customer impact.

The underlying problem here is that we select a default item from the list of configurations. However, each configuration may generate zero or more actual entries in the menu.

If it generates zero, we select an item which the viewer likely can't see.

If it generates more than one, this operation may not make much sense either.

We could:

  1. Continue selecting a configuration, but give configurations a method like canViewerSeeThisThing($viewer), or just do this via PolicyInterface.
  2. Instead of selecting a configuration, select a specific item.
  3. Before selecting a configuration, generate its items and ignore configurations with zero items (this is basically (1) where the policy test is "does this generate any items?").
  4. Rework the ability for configurations to generate 2+ items -- this capability may not really make sense.

I think we can probably make a better choice here later on.

In particular, if we don't need the capability in (4), removing it might be best. We don't currently use this capability, but if we want to add types of items which can generate several menu items (for example, "all children of wiki page X", or "links to all chatrooms with tag Y" or whatever -- any sort of thing where you configure one "item", and it dynamically produces a list of 2+ links in the actual menu at rendering time) we'd like to keep it. I suspect this capability will turn out to be useful sooner or later, and expanding it will probably lead to (or, at least, towards) a natural solution for this problem.

T12956 has another situation where letting MenuItem generate 2+ items may be bad: we want to let a menu item steal the selection, but it's muddy to implement if each MenuItem can return several actual views.

Somewhat related, if you have a disabled "Home" as your top item, we still show that regardless of the active dashboard below it.