From 329d03661f3b4e3b651b3b2ba5b12a2aa0d497e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Feb 2016 10:02:42 -0800 Subject: [PATCH] Use an extended policy to bind column and board policies together Summary: Ref T10349. Columns have the same policies as the projects they belong to. However, the current implementation just returns the policy directly. This usually works, but if the project has a policy like "Members of (This) Project", the policy filter tries to check if the viewer is a member of //the column itself//. That doesn't work, since columns don't have members. This leads to a situation where columns on "Editable By: Project Members" projects can not be edited. Instead, return a permissive base policy and then use an extended policy to bind the column policy to the project policy. Test Plan: - Edited a column on an "Editable By: Members of Project" board. - Added and ran a unit test covering this case. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10349 Differential Revision: https://secure.phabricator.com/D15268 --- src/__phutil_library_map__.php | 1 + .../PhabricatorProjectCoreTestCase.php | 61 +++++++++++++++++++ ...abricatorProjectColumnDetailController.php | 3 +- .../storage/PhabricatorProjectColumn.php | 22 ++++++- 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index aacf85ac45..45cfd6fe6a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -7305,6 +7305,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorExtendedPolicyInterface', ), 'PhabricatorProjectColumnDetailController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnEditController' => 'PhabricatorProjectBoardController', diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index ee90afcb3b..39efcadb55 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -1013,6 +1013,51 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $this->assertColumns($expect, $user, $board, $task); } + public function testColumnExtendedPolicies() { + $user = $this->createUser(); + $user->save(); + + $board = $this->createProject($user); + $column = $this->addColumn($user, $board, 0); + + // At first, the user should be able to view and edit the column. + $column = $this->refreshColumn($user, $column); + $this->assertTrue((bool)$column); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $user, + $column, + PhabricatorPolicyCapability::CAN_EDIT); + $this->assertTrue($can_edit); + + // Now, set the project edit policy to "Members of Project". This should + // disable editing. + $members_policy = id(new PhabricatorProjectMembersPolicyRule()) + ->getObjectPolicyFullKey(); + $board->setEditPolicy($members_policy)->save(); + + $column = $this->refreshColumn($user, $column); + $this->assertTrue((bool)$column); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $user, + $column, + PhabricatorPolicyCapability::CAN_EDIT); + $this->assertFalse($can_edit); + + // Now, join the project. This should make the column editable again. + $this->joinProject($board, $user); + + $column = $this->refreshColumn($user, $column); + $this->assertTrue((bool)$column); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $user, + $column, + PhabricatorPolicyCapability::CAN_EDIT); + $this->assertTrue($can_edit); + } + private function moveToColumn( PhabricatorUser $viewer, PhabricatorProject $board, @@ -1251,6 +1296,22 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { } } + private function refreshColumn( + PhabricatorUser $viewer, + PhabricatorProjectColumn $column) { + + $results = id(new PhabricatorProjectColumnQuery()) + ->setViewer($viewer) + ->withIDs(array($column->getID())) + ->execute(); + + if ($results) { + return head($results); + } else { + return null; + } + } + private function createProject( PhabricatorUser $user, PhabricatorProject $parent = null, diff --git a/src/applications/project/controller/PhabricatorProjectColumnDetailController.php b/src/applications/project/controller/PhabricatorProjectColumnDetailController.php index b0cd4ac0b4..84bdc2b89c 100644 --- a/src/applications/project/controller/PhabricatorProjectColumnDetailController.php +++ b/src/applications/project/controller/PhabricatorProjectColumnDetailController.php @@ -73,8 +73,7 @@ final class PhabricatorProjectColumnDetailController $header = id(new PHUIHeaderView()) ->setUser($viewer) - ->setHeader($column->getDisplayName()) - ->setPolicyObject($column); + ->setHeader($column->getDisplayName()); if ($column->isHidden()) { $header->setStatus('fa-ban', 'dark', pht('Hidden')); diff --git a/src/applications/project/storage/PhabricatorProjectColumn.php b/src/applications/project/storage/PhabricatorProjectColumn.php index 683039fe99..28aed0f5a8 100644 --- a/src/applications/project/storage/PhabricatorProjectColumn.php +++ b/src/applications/project/storage/PhabricatorProjectColumn.php @@ -5,7 +5,8 @@ final class PhabricatorProjectColumn implements PhabricatorApplicationTransactionInterface, PhabricatorPolicyInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorExtendedPolicyInterface { const STATUS_ACTIVE = 0; const STATUS_HIDDEN = 1; @@ -219,7 +220,14 @@ final class PhabricatorProjectColumn } public function getPolicy($capability) { - return $this->getProject()->getPolicy($capability); + // NOTE: Column policies are enforced as an extended policy which makes + // them the same as the project's policies. + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return PhabricatorPolicies::getMostOpenPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + return PhabricatorPolicies::POLICY_USER; + } } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { @@ -233,6 +241,16 @@ final class PhabricatorProjectColumn } +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + return array( + array($this->getProject(), $capability), + ); + } + + /* -( PhabricatorDestructibleInterface )----------------------------------- */ public function destroyObjectPermanently(