From 1c572d1da51fde2700aa8ee6fff22044ceb32d1c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Dec 2015 16:34:01 -0800 Subject: [PATCH] Implement a "Project Members" object policy rule Summary: Fixes T9019. Pretty much ripped from D14467. I added the "policy hint" stuff so that you can create a project with this policy immediately. I really dislike how the "hint" code works, but we //almost// never need to use it and the badness feels fairly well-contained. Also pick up a quick feedback fix from D14863. Test Plan: - Added test coverage, got it to pass. - Created a project with "Visible To: Project Members". Reviewers: joshuaspence, chad Reviewed By: chad Maniphest Tasks: T9019 Differential Revision: https://secure.phabricator.com/D14869 --- src/__phutil_library_map__.php | 2 + .../policy/filter/PhabricatorPolicyFilter.php | 5 + .../PhabricatorProjectCoreTestCase.php | 46 ++++++++- .../PhabricatorProjectTransactionEditor.php | 49 ++++++++++ .../PhabricatorProjectMembersPolicyRule.php | 97 +++++++++++++++++++ .../project/query/PhabricatorProjectQuery.php | 4 - 6 files changed, 198 insertions(+), 5 deletions(-) create mode 100644 src/applications/project/policyrule/PhabricatorProjectMembersPolicyRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bd304f920e..ca63615948 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2859,6 +2859,7 @@ phutil_register_library_map(array( 'PhabricatorProjectMemberOfProjectEdgeType' => 'applications/project/edge/PhabricatorProjectMemberOfProjectEdgeType.php', 'PhabricatorProjectMembersDatasource' => 'applications/project/typeahead/PhabricatorProjectMembersDatasource.php', 'PhabricatorProjectMembersEditController' => 'applications/project/controller/PhabricatorProjectMembersEditController.php', + 'PhabricatorProjectMembersPolicyRule' => 'applications/project/policyrule/PhabricatorProjectMembersPolicyRule.php', 'PhabricatorProjectMembersRemoveController' => 'applications/project/controller/PhabricatorProjectMembersRemoveController.php', 'PhabricatorProjectMoveController' => 'applications/project/controller/PhabricatorProjectMoveController.php', 'PhabricatorProjectNoProjectsDatasource' => 'applications/project/typeahead/PhabricatorProjectNoProjectsDatasource.php', @@ -7195,6 +7196,7 @@ phutil_register_library_map(array( 'PhabricatorProjectMemberOfProjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectMembersDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectMembersEditController' => 'PhabricatorProjectController', + 'PhabricatorProjectMembersPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorProjectMembersRemoveController' => 'PhabricatorProjectController', 'PhabricatorProjectMoveController' => 'PhabricatorProjectController', 'PhabricatorProjectNoProjectsDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index caa119e0cf..ded757f305 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -608,6 +608,11 @@ final class PhabricatorPolicyFilter extends Phobject { $rules = PhabricatorPolicyQuery::getObjectPolicyRules(null); + // Make sure we have clean, empty policy rule objects. + foreach ($rules as $key => $rule) { + $rules[$key] = clone $rule; + } + $results = array(); foreach ($map as $key => $object_list) { $rule = idx($rules, $key); diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index 8c17fc07e7..5b8e797bc2 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -287,7 +287,6 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { // In this second case, set the name first and then the slugs separately. $name2 = 'slugproject2'; - $xactions = array(); $xactions[] = id(new PhabricatorProjectTransaction()) @@ -396,6 +395,51 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $this->assertTrue((bool)$caught); } + public function testProjectMembersVisibility() { + // This is primarily testing that you can create a project and set the + // visibility or edit policy to "Project Members" immediately. + + $user1 = $this->createUser(); + $user1->save(); + + $user2 = $this->createUser(); + $user2->save(); + + $project = PhabricatorProject::initializeNewProject($user1); + $name = pht('Test Project %d', mt_rand()); + + $xactions = array(); + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME) + ->setNewValue($name); + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY) + ->setNewValue( + id(new PhabricatorProjectMembersPolicyRule()) + ->getObjectPolicyFullKey()); + + $edge_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST; + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $edge_type) + ->setNewValue( + array( + '=' => array($user1->getPHID() => $user1->getPHID()), + )); + + $this->applyTransactions($project, $user1, $xactions); + + $this->assertTrue((bool)$this->refreshProject($project, $user1)); + $this->assertFalse((bool)$this->refreshProject($project, $user2)); + + $this->leaveProject($project, $user1); + + $this->assertFalse((bool)$this->refreshProject($project, $user1)); + } + public function testParentProject() { $user = $this->createUser(); $user->save(); diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index 70daac64ed..7626729b8c 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -692,4 +692,53 @@ final class PhabricatorProjectTransactionEditor return $slugs; } + protected function adjustObjectForPolicyChecks( + PhabricatorLiskDAO $object, + array $xactions) { + + $copy = parent::adjustObjectForPolicyChecks($object, $xactions); + + $type_edge = PhabricatorTransactions::TYPE_EDGE; + $edgetype_member = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST; + + $member_xaction = null; + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() !== $type_edge) { + continue; + } + + $edgetype = $xaction->getMetadataValue('edge:type'); + if ($edgetype !== $edgetype_member) { + continue; + } + + $member_xaction = $xaction; + } + + if ($member_xaction) { + $object_phid = $object->getPHID(); + + if ($object_phid) { + $members = PhabricatorEdgeQuery::loadDestinationPHIDs( + $object_phid, + PhabricatorProjectProjectHasMemberEdgeType::EDGECONST); + } else { + $members = array(); + } + + $clone_xaction = clone $member_xaction; + $hint = $this->getPHIDTransactionNewValue($clone_xaction, $members); + $rule = new PhabricatorProjectMembersPolicyRule(); + + $hint = array_fuse($hint); + + PhabricatorPolicyRule::passTransactionHintToRule( + $copy, + $rule, + $hint); + } + + return $copy; + } + } diff --git a/src/applications/project/policyrule/PhabricatorProjectMembersPolicyRule.php b/src/applications/project/policyrule/PhabricatorProjectMembersPolicyRule.php new file mode 100644 index 0000000000..5fa57a31da --- /dev/null +++ b/src/applications/project/policyrule/PhabricatorProjectMembersPolicyRule.php @@ -0,0 +1,97 @@ +getPHID(); + if (!$viewer_phid) { + return; + } + + if (empty($this->memberships[$viewer_phid])) { + $this->memberships[$viewer_phid] = array(); + } + + foreach ($objects as $key => $object) { + $cache = $this->getTransactionHint($object); + if ($cache === null) { + continue; + } + + unset($objects[$key]); + + if (isset($cache[$viewer_phid])) { + $this->memberships[$viewer_phid][$object->getPHID()] = true; + } + } + + if (!$objects) { + return; + } + + $object_phids = mpull($objects, 'getPHID'); + $edge_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs(array($viewer_phid)) + ->withDestinationPHIDs($object_phids) + ->withEdgeTypes( + array( + PhabricatorProjectMemberOfProjectEdgeType::EDGECONST, + )); + $edge_query->execute(); + + $memberships = $edge_query->getDestinationPHIDs(); + if (!$memberships) { + return; + } + + $this->memberships[$viewer_phid] += array_fill_keys($memberships, true); + } + + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { + $viewer_phid = $viewer->getPHID(); + if (!$viewer_phid) { + return false; + } + + $memberships = idx($this->memberships, $viewer_phid); + return isset($memberships[$object->getPHID()]); + } + + public function getValueControlType() { + return self::CONTROL_TYPE_NONE; + } + + public function canApplyToObject(PhabricatorPolicyInterface $object) { + return ($object instanceof PhabricatorProject); + } + + public function getObjectPolicyKey() { + return 'project.members'; + } + + public function getObjectPolicyName() { + return pht('Project Members'); + } + + public function getObjectPolicyIcon() { + return 'fa-users'; + } + + public function getPolicyExplanation() { + return pht('Project members can take this action.'); + } + +} diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index 264cce9431..c975150fbc 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -95,10 +95,6 @@ final class PhabricatorProjectQuery return $this; } - public function getProperty() { - return $this->property; - } - public function withDepthBetween($min, $max) { $this->minDepth = $min; $this->maxDepth = $max;