Allow users who can edit a dashboard to remove invalid / restricted panels

Summary:
Ref T12207. Currently, to remove a panel from a dashboard, it must be a valid panel which you can see.

Instead, only require that the panel PHID actually be listed somewhere in the dashboard's internal list of panels.

This interacts with the "multiple instances of a panel" issue described in some more depth in T12207. In particular:

  - Currently, you can sort of add multiple copies of a panel to a dashboard, sometimes? Maybe?
  - This leads to great tragedy.

This doesn't fix up the workflow with respect to multiple copies of a panel. We still remove by panel PHID (not by column/position or internal ID) so if a dashboard has multiple copies of the same panel for some reason, I think this workflow removes one of them arbitrarily (at best) or perhaps does something worse. I'm just treating this behavior as undefined for the moment.

Test Plan:
- Removed an invalid/hidden panel from a dashboard as a user with permission to edit that dashboard.
- Tried to remove a made-up panel with a totally bogus PHID, got 404'd.
- Viewed a dashboard with a restricted panel.
- Put a hidden panel inside a tab panel, viewed it as a user who could not see it and a user who could.

Reviewers: chad

Reviewed By: chad

Subscribers: swisspol

Maniphest Tasks: T12207

Differential Revision: https://secure.phabricator.com/D17314

src/applications/dashboard/controller/PhabricatorDashboardPanelRenderController.php

@@ -34,6 +34,7 @@ final class PhabricatorDashboardPanelRenderController
     $rendered_panel = id(new PhabricatorDashboardPanelRenderingEngine())
       ->setViewer($viewer)
       ->setPanel($panel)
+      ->setPanelPHID($panel->getPHID())
       ->setParentPanelPHIDs($parent_phids)
       ->setHeaderMode($request->getStr('headerMode'))
       ->setDashboardID($request->getInt('dashboardID'))

src/applications/dashboard/controller/PhabricatorDashboardPanelViewController.php

@@ -38,6 +38,7 @@ final class PhabricatorDashboardPanelViewController
     $rendered_panel = id(new PhabricatorDashboardPanelRenderingEngine())
       ->setViewer($viewer)
       ->setPanel($panel)
+      ->setPanelPHID($panel->getPHID())
       ->setParentPanelPHIDs(array())
       ->renderPanel();
 

src/applications/dashboard/controller/PhabricatorDashboardRemovePanelController.php

@@ -20,12 +20,25 @@ final class PhabricatorDashboardRemovePanelController
       return new Aphront404Response();
     }
 
+    // NOTE: If you can edit a dashboard, you can remove panels from it even
+    // if you don't have permission to see them or they aren't valid. We only
+    // require that the panel be present on the dashboard.
+
     $v_panel = $request->getStr('panelPHID');
-    $panel = id(new PhabricatorDashboardPanelQuery())
+
-      ->setViewer($viewer)
+    $panel_on_dashboard = false;
-      ->withPHIDs(array($v_panel))
+    $layout = $dashboard->getLayoutConfigObject();
-      ->executeOne();
+    $columns = $layout->getPanelLocations();
-    if (!$panel) {
+    foreach ($columns as $column) {
+      foreach ($column as $column_panel_phid) {
+        if ($column_panel_phid == $v_panel) {
+          $panel_on_dashboard = true;
+          break;
+        }
+      }
+    }
+
+    if (!$panel_on_dashboard) {
       return new Aphront404Response();
     }
 
@@ -43,11 +56,11 @@ final class PhabricatorDashboardRemovePanelController
           ->setNewValue(
             array(
               '-' => array(
-                $panel->getPHID() => $panel->getPHID(),
+                $v_panel => $v_panel,
               ),
             ));
 
-      $layout_config->removePanel($panel->getPHID());
+      $layout_config->removePanel($v_panel);
       $dashboard->setLayoutConfigFromObject($layout_config);
 
       $editor = id(new PhabricatorDashboardTransactionEditor())
@@ -67,7 +80,7 @@ final class PhabricatorDashboardRemovePanelController
       ->appendChild(pht('Are you sure you want to remove this panel?'));
 
     return $this->newDialog()
-      ->setTitle(pht('Remove Panel %s', $panel->getMonogram()))
+      ->setTitle(pht('Remove Panel'))
       ->appendChild($form->buildLayoutView())
       ->addCancelButton($redirect_uri)
       ->addSubmitButton(pht('Remove Panel'));

src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php

@@ -7,6 +7,7 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject {
   const HEADER_MODE_EDIT   = 'edit';
 
   private $panel;
+  private $panelPHID;
   private $viewer;
   private $enableAsyncRendering;
   private $parentPanelPHIDs;
@@ -66,6 +67,15 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject {
     return $this->panel;
   }
 
+  public function setPanelPHID($panel_phid) {
+    $this->panelPHID = $panel_phid;
+    return $this;
+  }
+
+  public function getPanelPHID() {
+    return $this->panelPHID;
+  }
+
   public function renderPanel() {
     $panel = $this->getPanel();
     $viewer = $this->getViewer();
@@ -255,32 +265,40 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject {
     PHUIHeaderView $header) {
     $panel = $this->getPanel();
 
-    if (!$panel) {
-      return $header;
-    }
-
     $dashboard_id = $this->getDashboardID();
-    $edit_uri = id(new PhutilURI(
+
-      '/dashboard/panel/edit/'.$panel->getID().'/'));
+    if ($panel) {
-    if ($dashboard_id) {
+      $panel_id = $panel->getID();
-      $edit_uri->setQueryParam('dashboardID', $dashboard_id);
+
+      $edit_uri = "/dashboard/panel/edit/{$panel_id}/";
+      $edit_uri = new PhutilURI($edit_uri);
+      if ($dashboard_id) {
+        $edit_uri->setQueryParam('dashboardID', $dashboard_id);
+      }
+
+      $action_edit = id(new PHUIIconView())
+        ->setIcon('fa-pencil')
+        ->setWorkflow(true)
+        ->setHref((string)$edit_uri);
+
+      $header->addActionItem($action_edit);
     }
-    $action_edit = id(new PHUIIconView())
-      ->setIcon('fa-pencil')
-      ->setWorkflow(true)
-      ->setHref((string)$edit_uri);
-    $header->addActionItem($action_edit);
 
     if ($dashboard_id) {
-      $uri = id(new PhutilURI(
+      $panel_phid = $this->getPanelPHID();
-        '/dashboard/removepanel/'.$dashboard_id.'/'))
+
-        ->setQueryParam('panelPHID', $panel->getPHID());
+      $remove_uri = "/dashboard/removepanel/{$dashboard_id}/";
+      $remove_uri = id(new PhutilURI($remove_uri))
+        ->setQueryParam('panelPHID', $panel_phid);
+
       $action_remove = id(new PHUIIconView())
         ->setIcon('fa-trash-o')
-        ->setHref((string)$uri)
+        ->setHref((string)$remove_uri)
         ->setWorkflow(true);
+
       $header->addActionItem($action_remove);
     }
+
     return $header;
   }
 

src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php

@@ -55,6 +55,7 @@ final class PhabricatorDashboardRenderingEngine extends Phobject {
           ->setViewer($viewer)
           ->setDashboardID($dashboard->getID())
           ->setEnableAsyncRendering(true)
+          ->setPanelPHID($panel_phid)
           ->setParentPanelPHIDs(array())
           ->setHeaderMode($h_mode);
 

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php

@@ -91,6 +91,7 @@ final class PhabricatorDashboardTabsPanelType
           ->setEnableAsyncRendering(true)
           ->setParentPanelPHIDs($parent_phids)
           ->setPanel($panel)
+          ->setPanelPHID($panel->getPHID())
           ->setHeaderMode($no_headers)
           ->renderPanel();
       } else {

src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php

@@ -32,6 +32,7 @@ final class PhabricatorDashboardRemarkupRule
     return id(new PhabricatorDashboardPanelRenderingEngine())
       ->setViewer($viewer)
       ->setPanel($object)
+      ->setPanelPHID($object->getPHID())
       ->setParentPanelPHIDs($parent_phids)
       ->renderPanel();