1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-18 03:20:59 +01:00

Implement query and policy rules for subprojects

Summary:
Ref T10010. This implements technical groundwork for subprojects. Specifically, it implements policy rules like Phriction:

  - to see a project, you must be able to see all of its parents (and the project itself).
  - you can edit a project if you can edit any of its parents (or the project itself).

To facilitiate this, we load all project ancestors when querying projects so we can do the view/edit checks.

This does NOT yet implement:

  - proper membership rules for these projects (up next);
  - any kind of UI to let users create subprojects.

Test Plan:
  - Added unit tests.
  - Executed unit tests.
  - Browsed Projects (no change in behavior is expected).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010

Differential Revision: https://secure.phabricator.com/D14861
This commit is contained in:
epriestley 2015-12-23 03:42:51 -08:00
parent 16d8e806a0
commit 3068639ccf
10 changed files with 405 additions and 15 deletions

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_project.project
ADD projectPath VARBINARY(64) NOT NULL;

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_project.project
ADD projectDepth INT UNSIGNED NOT NULL;

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_project.project
ADD KEY `key_path` (projectPath, projectDepth);

View file

@ -7131,6 +7131,7 @@ phutil_register_library_map(array(
'PhabricatorApplicationTransactionInterface', 'PhabricatorApplicationTransactionInterface',
'PhabricatorFlaggableInterface', 'PhabricatorFlaggableInterface',
'PhabricatorPolicyInterface', 'PhabricatorPolicyInterface',
'PhabricatorExtendedPolicyInterface',
'PhabricatorSubscribableInterface', 'PhabricatorSubscribableInterface',
'PhabricatorCustomFieldInterface', 'PhabricatorCustomFieldInterface',
'PhabricatorDestructibleInterface', 'PhabricatorDestructibleInterface',

View file

@ -321,6 +321,7 @@ abstract class PhabricatorConfigSchemaSpec extends Phobject {
break; break;
case 'phid': case 'phid':
case 'policy'; case 'policy';
case 'hashpath64':
$column_type = 'varbinary(64)'; $column_type = 'varbinary(64)';
break; break;
case 'bytes64': case 'bytes64':

View file

@ -113,6 +113,59 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
$this->assertTrue($caught instanceof Exception); $this->assertTrue($caught instanceof Exception);
} }
public function testParentProject() {
$user = $this->createUser();
$user->save();
$parent = $this->createProject($user);
$child = $this->createProject($user, $parent);
$this->assertTrue(true);
$child = $this->refreshProject($child, $user);
$this->assertEqual(
$parent->getPHID(),
$child->getParentProject()->getPHID());
$this->assertEqual(1, (int)$child->getProjectDepth());
$this->assertFalse(
$child->isUserMember($user->getPHID()));
$this->assertFalse(
$child->getParentProject()->isUserMember($user->getPHID()));
$this->joinProject($child, $user);
$child = $this->refreshProject($child, $user);
$this->assertTrue(
$child->isUserMember($user->getPHID()));
$this->assertTrue(
$child->getParentProject()->isUserMember($user->getPHID()));
// Test that hiding a parent hides the child.
$user2 = $this->createUser();
$user2->save();
// Second user can see the project for now.
$this->assertTrue((bool)$this->refreshProject($child, $user2));
// Hide the parent.
$this->setViewPolicy($parent, $user, $user->getPHID());
// First user (who can see the parent because they are a member of
// the child) can see the project.
$this->assertTrue((bool)$this->refreshProject($child, $user));
// Second user can not, because they can't see the parent.
$this->assertFalse((bool)$this->refreshProject($child, $user2));
}
private function attemptProjectEdit( private function attemptProjectEdit(
PhabricatorProject $proj, PhabricatorProject $proj,
PhabricatorUser $user, PhabricatorUser $user,
@ -126,10 +179,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
$xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME); $xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME);
$xaction->setNewValue($new_name); $xaction->setNewValue($new_name);
$editor = new PhabricatorProjectTransactionEditor(); $this->applyTransactions($proj, $user, array($xaction));
$editor->setActor($user);
$editor->setContentSource(PhabricatorContentSource::newConsoleSource());
$editor->applyTransactions($proj, array($xaction));
return true; return true;
} }
@ -270,10 +320,43 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
} }
} }
private function createProject(PhabricatorUser $user) { private function createProject(
PhabricatorUser $user,
PhabricatorProject $parent = null) {
$project = PhabricatorProject::initializeNewProject($user); $project = PhabricatorProject::initializeNewProject($user);
$project->setName(pht('Test Project %d', mt_rand()));
$project->save(); $name = pht('Test Project %d', mt_rand());
$xactions = array();
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
->setNewValue($name);
if ($parent) {
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorProjectTransaction::TYPE_PARENT)
->setNewValue($parent->getPHID());
}
$this->applyTransactions($project, $user, $xactions);
return $project;
}
private function setViewPolicy(
PhabricatorProject $project,
PhabricatorUser $user,
$policy) {
$xactions = array();
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
->setNewValue($policy);
$this->applyTransactions($project, $user, $xactions);
return $project; return $project;
} }
@ -359,13 +442,21 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
->setMetadataValue('edge:type', $edge_type) ->setMetadataValue('edge:type', $edge_type)
->setNewValue($spec); ->setNewValue($spec);
$this->applyTransactions($project, $user, $xactions);
return $project;
}
private function applyTransactions(
PhabricatorProject $project,
PhabricatorUser $user,
array $xactions) {
$editor = id(new PhabricatorProjectTransactionEditor()) $editor = id(new PhabricatorProjectTransactionEditor())
->setActor($user) ->setActor($user)
->setContentSource(PhabricatorContentSource::newConsoleSource()) ->setContentSource(PhabricatorContentSource::newConsoleSource())
->setContinueOnNoEffect(true) ->setContinueOnNoEffect(true)
->applyTransactions($project, $xactions); ->applyTransactions($project, $xactions);
return $project;
} }

View file

@ -26,6 +26,7 @@ final class PhabricatorProjectTransactionEditor
$types[] = PhabricatorProjectTransaction::TYPE_ICON; $types[] = PhabricatorProjectTransaction::TYPE_ICON;
$types[] = PhabricatorProjectTransaction::TYPE_COLOR; $types[] = PhabricatorProjectTransaction::TYPE_COLOR;
$types[] = PhabricatorProjectTransaction::TYPE_LOCKED; $types[] = PhabricatorProjectTransaction::TYPE_LOCKED;
$types[] = PhabricatorProjectTransaction::TYPE_PARENT;
return $types; return $types;
} }
@ -52,6 +53,8 @@ final class PhabricatorProjectTransactionEditor
return $object->getColor(); return $object->getColor();
case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_LOCKED:
return (int)$object->getIsMembershipLocked(); return (int)$object->getIsMembershipLocked();
case PhabricatorProjectTransaction::TYPE_PARENT:
return null;
} }
return parent::getCustomTransactionOldValue($object, $xaction); return parent::getCustomTransactionOldValue($object, $xaction);
@ -69,6 +72,7 @@ final class PhabricatorProjectTransactionEditor
case PhabricatorProjectTransaction::TYPE_ICON: case PhabricatorProjectTransaction::TYPE_ICON:
case PhabricatorProjectTransaction::TYPE_COLOR: case PhabricatorProjectTransaction::TYPE_COLOR:
case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_LOCKED:
case PhabricatorProjectTransaction::TYPE_PARENT:
return $xaction->getNewValue(); return $xaction->getNewValue();
} }
@ -102,6 +106,9 @@ final class PhabricatorProjectTransactionEditor
case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_LOCKED:
$object->setIsMembershipLocked($xaction->getNewValue()); $object->setIsMembershipLocked($xaction->getNewValue());
return; return;
case PhabricatorProjectTransaction::TYPE_PARENT:
$object->setParentProjectPHID($xaction->getNewValue());
return;
} }
return parent::applyCustomInternalTransaction($object, $xaction); return parent::applyCustomInternalTransaction($object, $xaction);
@ -153,6 +160,7 @@ final class PhabricatorProjectTransactionEditor
case PhabricatorProjectTransaction::TYPE_ICON: case PhabricatorProjectTransaction::TYPE_ICON:
case PhabricatorProjectTransaction::TYPE_COLOR: case PhabricatorProjectTransaction::TYPE_COLOR:
case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_LOCKED:
case PhabricatorProjectTransaction::TYPE_PARENT:
return; return;
} }
@ -324,7 +332,77 @@ final class PhabricatorProjectTransactionEditor
} }
break; break;
case PhabricatorProjectTransaction::TYPE_PARENT:
if (!$xactions) {
break;
}
$xaction = last($xactions);
if (!$this->getIsNewObject()) {
$errors[] = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Invalid'),
pht(
'You can only set a parent project when creating a project '.
'for the first time.'),
$xaction);
break;
}
$parent_phid = $xaction->getNewValue();
$projects = id(new PhabricatorProjectQuery())
->setViewer($this->requireActor())
->withPHIDs(array($parent_phid))
->requireCapabilities(
array(
PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT,
))
->execute();
if (!$projects) {
$errors[] = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Invalid'),
pht(
'Parent project PHID ("%s") must be the PHID of a valid, '.
'visible project which you have permission to edit.',
$parent_phid),
$xaction);
break;
}
$project = head($projects);
if ($project->isMilestone()) {
$errors[] = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Invalid'),
pht(
'Parent project PHID ("%s") must not be a milestone. '.
'Milestones may not have subprojects.',
$parent_phid),
$xaction);
break;
}
$limit = PhabricatorProject::getProjectDepthLimit();
if ($project->getProjectDepth() >= ($limit - 1)) {
$errors[] = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Invalid'),
pht(
'You can not create a subproject under this parent because '.
'it would nest projects too deeply. The maximum nesting '.
'depth of projects is %s.',
new PhutilNumber($limit)),
$xaction);
break;
}
$object->attachParentProject($project);
break;
} }
return $errors; return $errors;

View file

@ -130,6 +130,27 @@ final class PhabricatorProjectQuery
} }
protected function willFilterPage(array $projects) { protected function willFilterPage(array $projects) {
$project_phids = array();
$ancestor_paths = array();
foreach ($projects as $project) {
$project_phids[] = $project->getPHID();
foreach ($project->getAncestorProjectPaths() as $path) {
$ancestor_paths[$path] = $path;
}
}
if ($ancestor_paths) {
$ancestors = id(new PhabricatorProject())->loadAllWhere(
'projectPath IN (%Ls)',
$ancestor_paths);
} else {
$ancestors = array();
}
$projects = $this->linkProjectGraph($projects, $ancestors);
$viewer_phid = $this->getViewer()->getPHID(); $viewer_phid = $this->getViewer()->getPHID();
$project_phids = mpull($projects, 'getPHID'); $project_phids = mpull($projects, 'getPHID');
@ -154,6 +175,7 @@ final class PhabricatorProjectQuery
$edge_query->execute(); $edge_query->execute();
$membership_projects = array();
foreach ($projects as $project) { foreach ($projects as $project) {
$project_phid = $project->getPHID(); $project_phid = $project->getPHID();
@ -161,9 +183,9 @@ final class PhabricatorProjectQuery
array($project_phid), array($project_phid),
array($member_type)); array($member_type));
$project->setIsUserMember( if (in_array($viewer_phid, $member_phids)) {
$viewer_phid, $membership_projects[$project_phid] = $project;
in_array($viewer_phid, $member_phids)); }
if ($this->needMembers) { if ($this->needMembers) {
$project->attachMemberPHIDs($member_phids); $project->attachMemberPHIDs($member_phids);
@ -180,6 +202,14 @@ final class PhabricatorProjectQuery
} }
} }
$all_graph = $this->getAllReachableAncestors($projects);
$member_graph = $this->getAllReachableAncestors($membership_projects);
foreach ($all_graph as $phid => $project) {
$is_member = isset($member_graph[$phid]);
$project->setIsUserMember($viewer_phid, $is_member);
}
return $projects; return $projects;
} }
@ -360,4 +390,88 @@ final class PhabricatorProjectQuery
return 'p'; return 'p';
} }
private function linkProjectGraph(array $projects, array $ancestors) {
$ancestor_map = mpull($ancestors, null, 'getPHID');
$projects_map = mpull($projects, null, 'getPHID');
$all_map = $projects_map + $ancestor_map;
$done = array();
foreach ($projects as $key => $project) {
$seen = array($project->getPHID() => true);
if (!$this->linkProject($project, $all_map, $done, $seen)) {
$this->didRejectResult($project);
unset($projects[$key]);
continue;
}
foreach ($project->getAncestorProjects() as $ancestor) {
$seen[$ancestor->getPHID()] = true;
}
}
return $projects;
}
private function linkProject($project, array $all, array $done, array $seen) {
$parent_phid = $project->getParentProjectPHID();
// This project has no parent, so just attach `null` and return.
if (!$parent_phid) {
$project->attachParentProject(null);
return true;
}
// This project has a parent, but it failed to load.
if (empty($all[$parent_phid])) {
return false;
}
// Test for graph cycles. If we encounter one, we're going to hide the
// entire cycle since we can't meaningfully resolve it.
if (isset($seen[$parent_phid])) {
return false;
}
$seen[$parent_phid] = true;
$parent = $all[$parent_phid];
$project->attachParentProject($parent);
if (!empty($done[$parent_phid])) {
return true;
}
return $this->linkProject($parent, $all, $done, $seen);
}
private function getAllReachableAncestors(array $projects) {
$ancestors = array();
$seen = mpull($projects, null, 'getPHID');
$stack = $projects;
while ($stack) {
$project = array_pop($stack);
$phid = $project->getPHID();
$ancestors[$phid] = $project;
$parent_phid = $project->getParentProjectPHID();
if (!$parent_phid) {
continue;
}
if (isset($seen[$parent_phid])) {
continue;
}
$seen[$parent_phid] = true;
$stack[] = $project->getParentProject();
}
return $ancestors;
}
} }

View file

@ -5,6 +5,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
PhabricatorApplicationTransactionInterface, PhabricatorApplicationTransactionInterface,
PhabricatorFlaggableInterface, PhabricatorFlaggableInterface,
PhabricatorPolicyInterface, PhabricatorPolicyInterface,
PhabricatorExtendedPolicyInterface,
PhabricatorSubscribableInterface, PhabricatorSubscribableInterface,
PhabricatorCustomFieldInterface, PhabricatorCustomFieldInterface,
PhabricatorDestructibleInterface, PhabricatorDestructibleInterface,
@ -30,6 +31,9 @@ final class PhabricatorProject extends PhabricatorProjectDAO
protected $hasSubprojects; protected $hasSubprojects;
protected $milestoneNumber; protected $milestoneNumber;
protected $projectPath;
protected $projectDepth;
private $memberPHIDs = self::ATTACHABLE; private $memberPHIDs = self::ATTACHABLE;
private $watcherPHIDs = self::ATTACHABLE; private $watcherPHIDs = self::ATTACHABLE;
private $sparseWatchers = self::ATTACHABLE; private $sparseWatchers = self::ATTACHABLE;
@ -69,7 +73,8 @@ final class PhabricatorProject extends PhabricatorProjectDAO
->attachSlugs(array()) ->attachSlugs(array())
->setHasWorkboard(0) ->setHasWorkboard(0)
->setHasMilestones(0) ->setHasMilestones(0)
->setHasSubprojects(0); ->setHasSubprojects(0)
->attachParentProject(null);
} }
public function getCapabilities() { public function getCapabilities() {
@ -92,6 +97,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
} }
public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
$can_edit = PhabricatorPolicyCapability::CAN_EDIT;
switch ($capability) { switch ($capability) {
case PhabricatorPolicyCapability::CAN_VIEW: case PhabricatorPolicyCapability::CAN_VIEW:
@ -101,9 +107,18 @@ final class PhabricatorProject extends PhabricatorProjectDAO
} }
break; break;
case PhabricatorPolicyCapability::CAN_EDIT: case PhabricatorPolicyCapability::CAN_EDIT:
$parent = $this->getParentProject();
if ($parent) {
$can_edit_parent = PhabricatorPolicyFilter::hasCapability(
$viewer,
$parent,
$can_edit);
if ($can_edit_parent) {
return true;
}
}
break; break;
case PhabricatorPolicyCapability::CAN_JOIN: case PhabricatorPolicyCapability::CAN_JOIN:
$can_edit = PhabricatorPolicyCapability::CAN_EDIT;
if (PhabricatorPolicyFilter::hasCapability($viewer, $this, $can_edit)) { if (PhabricatorPolicyFilter::hasCapability($viewer, $this, $can_edit)) {
// Project editors can always join a project. // Project editors can always join a project.
return true; return true;
@ -115,6 +130,9 @@ final class PhabricatorProject extends PhabricatorProjectDAO
} }
public function describeAutomaticCapability($capability) { public function describeAutomaticCapability($capability) {
// TODO: Clarify the additional rules that parent and subprojects imply.
switch ($capability) { switch ($capability) {
case PhabricatorPolicyCapability::CAN_VIEW: case PhabricatorPolicyCapability::CAN_VIEW:
return pht('Members of a project can always view it.'); return pht('Members of a project can always view it.');
@ -124,6 +142,25 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return null; return null;
} }
public function getExtendedPolicy($capability, PhabricatorUser $viewer) {
$extended = array();
switch ($capability) {
case PhabricatorPolicyCapability::CAN_VIEW:
$parent = $this->getParentProject();
if ($parent) {
$extended[] = array(
$parent,
PhabricatorPolicyCapability::CAN_VIEW,
);
}
break;
}
return $extended;
}
public function isUserMember($user_phid) { public function isUserMember($user_phid) {
if ($this->memberPHIDs !== self::ATTACHABLE) { if ($this->memberPHIDs !== self::ATTACHABLE) {
return in_array($user_phid, $this->memberPHIDs); return in_array($user_phid, $this->memberPHIDs);
@ -157,6 +194,8 @@ final class PhabricatorProject extends PhabricatorProjectDAO
'hasMilestones' => 'bool', 'hasMilestones' => 'bool',
'hasSubprojects' => 'bool', 'hasSubprojects' => 'bool',
'milestoneNumber' => 'uint32?', 'milestoneNumber' => 'uint32?',
'projectPath' => 'hashpath64',
'projectDepth' => 'uint32',
), ),
self::CONFIG_KEY_SCHEMA => array( self::CONFIG_KEY_SCHEMA => array(
'key_phid' => null, 'key_phid' => null,
@ -182,6 +221,9 @@ final class PhabricatorProject extends PhabricatorProjectDAO
'columns' => array('primarySlug'), 'columns' => array('primarySlug'),
'unique' => true, 'unique' => true,
), ),
'key_path' => array(
'columns' => array('projectPath', 'projectDepth'),
),
), ),
) + parent::getConfiguration(); ) + parent::getConfiguration();
} }
@ -264,6 +306,31 @@ final class PhabricatorProject extends PhabricatorProjectDAO
$this->setMailKey(Filesystem::readRandomCharacters(20)); $this->setMailKey(Filesystem::readRandomCharacters(20));
} }
if (!strlen($this->getPHID())) {
$this->setPHID($this->generatePHID());
}
$path = array();
$depth = 0;
if ($this->parentProjectPHID) {
$parent = $this->getParentProject();
$path[] = $parent->getProjectPath();
$depth = $parent->getProjectDepth() + 1;
}
$hash = PhabricatorHash::digestForIndex($this->getPHID());
$path[] = substr($hash, 0, 4);
$path = implode('', $path);
$limit = self::getProjectDepthLimit();
if (strlen($path) > ($limit * 4)) {
throw new Exception(
pht('Unable to save project: path length is too long.'));
}
$this->setProjectPath($path);
$this->setProjectDepth($depth);
$this->openTransaction(); $this->openTransaction();
$result = parent::save(); $result = parent::save();
$this->updateDatasourceTokens(); $this->updateDatasourceTokens();
@ -272,6 +339,12 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return $result; return $result;
} }
public static function getProjectDepthLimit() {
// This is limited by how many path hashes we can fit in the path
// column.
return 16;
}
public function updateDatasourceTokens() { public function updateDatasourceTokens() {
$table = self::TABLE_DATASOURCE_TOKEN; $table = self::TABLE_DATASOURCE_TOKEN;
$conn_w = $this->establishConnection('w'); $conn_w = $this->establishConnection('w');
@ -319,11 +392,36 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return $this->assertAttached($this->parentProject); return $this->assertAttached($this->parentProject);
} }
public function attachParentProject(PhabricatorProject $project) { public function attachParentProject(PhabricatorProject $project = null) {
$this->parentProject = $project; $this->parentProject = $project;
return $this; return $this;
} }
public function getAncestorProjectPaths() {
$parts = array();
$path = $this->getProjectPath();
$parent_length = (strlen($path) - 4);
for ($ii = $parent_length; $ii >= 0; $ii -= 4) {
$parts[] = substr($path, 0, $ii);
}
return $parts;
}
public function getAncestorProjects() {
$ancestors = array();
$cursor = $this->getParentProject();
while ($cursor) {
$ancestors[] = $cursor;
$cursor = $cursor->getParentProject();
}
return $ancestors;
}
/* -( PhabricatorSubscribableInterface )----------------------------------- */ /* -( PhabricatorSubscribableInterface )----------------------------------- */

View file

@ -10,6 +10,7 @@ final class PhabricatorProjectTransaction
const TYPE_ICON = 'project:icon'; const TYPE_ICON = 'project:icon';
const TYPE_COLOR = 'project:color'; const TYPE_COLOR = 'project:color';
const TYPE_LOCKED = 'project:locked'; const TYPE_LOCKED = 'project:locked';
const TYPE_PARENT = 'project:parent';
// NOTE: This is deprecated, members are just a normal edge now. // NOTE: This is deprecated, members are just a normal edge now.
const TYPE_MEMBERS = 'project:members'; const TYPE_MEMBERS = 'project:members';