From c3bdcb4ca85487921909f0202aa760e8ed61404a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 4 Feb 2017 16:06:57 -0800 Subject: [PATCH] (stable) 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 --- ...bricatorDashboardPanelRenderController.php | 1 + ...habricatorDashboardPanelViewController.php | 1 + ...bricatorDashboardRemovePanelController.php | 29 ++++++++--- ...abricatorDashboardPanelRenderingEngine.php | 52 +++++++++++++------ .../PhabricatorDashboardRenderingEngine.php | 1 + .../PhabricatorDashboardTabsPanelType.php | 1 + .../PhabricatorDashboardRemarkupRule.php | 1 + 7 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/applications/dashboard/controller/PhabricatorDashboardPanelRenderController.php b/src/applications/dashboard/controller/PhabricatorDashboardPanelRenderController.php index 0a0e7e30c0..8f25a0f9bb 100644 --- a/src/applications/dashboard/controller/PhabricatorDashboardPanelRenderController.php +++ b/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')) diff --git a/src/applications/dashboard/controller/PhabricatorDashboardPanelViewController.php b/src/applications/dashboard/controller/PhabricatorDashboardPanelViewController.php index 9fd9e1840d..4b5f1b45be 100644 --- a/src/applications/dashboard/controller/PhabricatorDashboardPanelViewController.php +++ b/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(); diff --git a/src/applications/dashboard/controller/PhabricatorDashboardRemovePanelController.php b/src/applications/dashboard/controller/PhabricatorDashboardRemovePanelController.php index 2fa8e83a10..c30b06c596 100644 --- a/src/applications/dashboard/controller/PhabricatorDashboardRemovePanelController.php +++ b/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) - ->withPHIDs(array($v_panel)) - ->executeOne(); - if (!$panel) { + + $panel_on_dashboard = false; + $layout = $dashboard->getLayoutConfigObject(); + $columns = $layout->getPanelLocations(); + 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')); diff --git a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php b/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php index 286d749a18..fbd5aaa2bb 100644 --- a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php +++ b/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 ($dashboard_id) { - $edit_uri->setQueryParam('dashboardID', $dashboard_id); + + if ($panel) { + $panel_id = $panel->getID(); + + $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( - '/dashboard/removepanel/'.$dashboard_id.'/')) - ->setQueryParam('panelPHID', $panel->getPHID()); + $panel_phid = $this->getPanelPHID(); + + $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; } diff --git a/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php b/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php index f9af84973e..d1e1d3111c 100644 --- a/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php +++ b/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); diff --git a/src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php b/src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php index 4a0dae02f1..3cb758a11a 100644 --- a/src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php +++ b/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 { diff --git a/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php b/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php index 6b0a48dc01..d96852b10e 100644 --- a/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php +++ b/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();