1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-09 16:32:39 +01:00

Implement child/descendant query rules in Projects

Summary:
Ref T10010. This adds infrastructure for querying projects by type, depth, parent and ancestor.

I needed to revise the "extended policy check" cycle detection rules. When, e.g., querying a grandchild, they incorrectly detected a cycle because both the child and grandchild needed to check the policy of the grandparent.

Instead, simplify it to just do a basic runaway calldepth check. There are many other safety mechanisms to make it so this can't ever occur.

(Cycle detection does have existing test coverage, and those tests still pass, it just takes a little longer to detect the cycle internally.)

There is still no way to create subprojects in the UI.

Test Plan: Added and executed unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010

Differential Revision: https://secure.phabricator.com/D14862
This commit is contained in:
epriestley 2015-12-23 07:01:27 -08:00
parent 3068639ccf
commit 70f6bf306f
7 changed files with 255 additions and 55 deletions

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_project.project
ADD projectPathKey BINARY(4) NOT NULL;

View file

@ -0,0 +1,24 @@
<?php
$table = new PhabricatorProject();
$conn_w = $table->establishConnection('w');
foreach (new LiskMigrationIterator($table) as $project) {
$path = $project->getProjectPath();
$key = $project->getProjectPathKey();
if (strlen($path) && ($key !== "\0\0\0\0")) {
continue;
}
$path_key = PhabricatorHash::digestForIndex($project->getPHID());
$path_key = substr($path_key, 0, 4);
queryfx(
$conn_w,
'UPDATE %T SET projectPath = %s, projectPathKey = %s WHERE id = %d',
$project->getTableName(),
$path_key,
$path_key,
$project->getID());
}

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_project.project
ADD UNIQUE KEY `key_pathkey` (projectPathKey);

View file

@ -243,50 +243,6 @@ final class PhabricatorPolicyFilter extends Phobject {
}
private function applyExtendedPolicyChecks(array $extended_objects) {
// First, we're going to detect cycles and reject any objects which are
// part of a cycle. We don't want to loop forever if an object has a
// self-referential or nonsense policy.
static $in_flight = array();
$all_phids = array();
foreach ($extended_objects as $key => $object) {
$phid = $object->getPHID();
if (isset($in_flight[$phid])) {
// TODO: This could be more user-friendly.
$this->rejectObject($extended_objects[$key], false, '<cycle>');
unset($extended_objects[$key]);
continue;
}
// We might throw from rejectObject(), so we don't want to actually mark
// anything as in-flight until we survive this entire step.
$all_phids[$phid] = $phid;
}
foreach ($all_phids as $phid) {
$in_flight[$phid] = true;
}
$caught = null;
try {
$extended_objects = $this->executeExtendedPolicyChecks($extended_objects);
} catch (Exception $ex) {
$caught = $ex;
}
foreach ($all_phids as $phid) {
unset($in_flight[$phid]);
}
if ($caught) {
throw $caught;
}
return $extended_objects;
}
private function executeExtendedPolicyChecks(array $extended_objects) {
$viewer = $this->viewer;
$filter_capabilities = $this->capabilities;
@ -396,10 +352,11 @@ final class PhabricatorPolicyFilter extends Phobject {
}
if ($objects_in) {
$objects_out = id(new PhabricatorPolicyFilter())
->setViewer($viewer)
->requireCapabilities($capabilities)
->apply($objects_in);
$objects_out = $this->executeExtendedPolicyChecks(
$viewer,
$capabilities,
$objects_in,
$key_map);
$objects_out = mpull($objects_out, null, 'getPHID');
} else {
$objects_out = array();
@ -435,6 +392,53 @@ final class PhabricatorPolicyFilter extends Phobject {
return $extended_objects;
}
private function executeExtendedPolicyChecks(
PhabricatorUser $viewer,
array $capabilities,
array $objects,
array $key_map) {
// Do crude cycle detection by seeing if we have a huge stack depth.
// Although more sophisticated cycle detection is possible in theory,
// it is difficult with hierarchical objects like subprojects. Many other
// checks make it difficult to create cycles normally, so just do a
// simple check here to limit damage.
static $depth;
$depth++;
if ($depth > 32) {
foreach ($objects as $key => $object) {
$this->rejectObject($objects[$key], false, '<cycle>');
unset($objects[$key]);
continue;
}
}
if (!$objects) {
return array();
}
$caught = null;
try {
$result = id(new PhabricatorPolicyFilter())
->setViewer($viewer)
->requireCapabilities($capabilities)
->apply($objects);
} catch (Exception $ex) {
$caught = $ex;
}
$depth--;
if ($caught) {
throw $caught;
}
return $result;
}
private function checkCapability(
PhabricatorPolicyInterface $object,
$capability) {

View file

@ -113,6 +113,80 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
$this->assertTrue($caught instanceof Exception);
}
public function testAncestryQueries() {
$user = $this->createUser();
$user->save();
$ancestor = $this->createProject($user);
$parent = $this->createProject($user, $ancestor);
$child = $this->createProject($user, $parent);
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withAncestorProjectPHIDs(array($ancestor->getPHID()))
->execute();
$this->assertEqual(2, count($projects));
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withParentProjectPHIDs(array($ancestor->getPHID()))
->execute();
$this->assertEqual(1, count($projects));
$this->assertEqual(
$parent->getPHID(),
head($projects)->getPHID());
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withAncestorProjectPHIDs(array($ancestor->getPHID()))
->withDepthBetween(2, null)
->execute();
$this->assertEqual(1, count($projects));
$this->assertEqual(
$child->getPHID(),
head($projects)->getPHID());
$parent2 = $this->createProject($user, $ancestor);
$child2 = $this->createProject($user, $parent2);
$grandchild2 = $this->createProject($user, $child2);
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withAncestorProjectPHIDs(array($ancestor->getPHID()))
->execute();
$this->assertEqual(5, count($projects));
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withParentProjectPHIDs(array($ancestor->getPHID()))
->execute();
$this->assertEqual(2, count($projects));
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withAncestorProjectPHIDs(array($ancestor->getPHID()))
->withDepthBetween(2, null)
->execute();
$this->assertEqual(3, count($projects));
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withAncestorProjectPHIDs(array($ancestor->getPHID()))
->withDepthBetween(3, null)
->execute();
$this->assertEqual(1, count($projects));
$projects = id(new PhabricatorProjectQuery())
->setViewer($user)
->withPHIDs(
array(
$child->getPHID(),
$grandchild2->getPHID(),
))
->execute();
$this->assertEqual(2, count($projects));
}
public function testParentProject() {
$user = $this->createUser();
$user->save();

View file

@ -11,6 +11,11 @@ final class PhabricatorProjectQuery
private $nameTokens;
private $icons;
private $colors;
private $ancestorPHIDs;
private $parentPHIDs;
private $isMilestone;
private $minDepth;
private $maxDepth;
private $status = 'status-any';
const STATUS_ANY = 'status-any';
@ -69,6 +74,27 @@ final class PhabricatorProjectQuery
return $this;
}
public function withParentProjectPHIDs($parent_phids) {
$this->parentPHIDs = $parent_phids;
return $this;
}
public function withAncestorProjectPHIDs($ancestor_phids) {
$this->ancestorPHIDs = $ancestor_phids;
return $this;
}
public function withIsMilestone($is_milestone) {
$this->isMilestone = $is_milestone;
return $this;
}
public function withDepthBetween($min, $max) {
$this->minDepth = $min;
$this->maxDepth = $max;
return $this;
}
public function needMembers($need_members) {
$this->needMembers = $need_members;
return $this;
@ -337,6 +363,65 @@ final class PhabricatorProjectQuery
$this->colors);
}
if ($this->parentPHIDs !== null) {
$where[] = qsprintf(
$conn,
'parentProjectPHID IN (%Ls)',
$this->parentPHIDs);
}
if ($this->ancestorPHIDs !== null) {
$ancestor_paths = queryfx_all(
$conn,
'SELECT projectPath, projectDepth FROM %T WHERE phid IN (%Ls)',
id(new PhabricatorProject())->getTableName(),
$this->ancestorPHIDs);
if (!$ancestor_paths) {
throw new PhabricatorEmptyQueryException();
}
$sql = array();
foreach ($ancestor_paths as $ancestor_path) {
$sql[] = qsprintf(
$conn,
'(projectPath LIKE %> AND projectDepth > %d)',
$ancestor_path['projectPath'],
$ancestor_path['projectDepth']);
}
$where[] = '('.implode(' OR ', $sql).')';
$where[] = qsprintf(
$conn,
'parentProjectPHID IS NOT NULL');
}
if ($this->isMilestone !== null) {
if ($this->isMilestone) {
$where[] = qsprintf(
$conn,
'milestoneNumber IS NOT NULL');
} else {
$where[] = qsprintf(
$conn,
'milestoneNumber IS NULL');
}
}
if ($this->minDepth !== null) {
$where[] = qsprintf(
$conn,
'projectDepth >= %d',
$this->minDepth);
}
if ($this->maxDepth !== null) {
$where[] = qsprintf(
$conn,
'projectDepth <= %d',
$this->maxDepth);
}
return $where;
}

View file

@ -33,6 +33,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
protected $projectPath;
protected $projectDepth;
protected $projectPathKey;
private $memberPHIDs = self::ATTACHABLE;
private $watcherPHIDs = self::ATTACHABLE;
@ -196,6 +197,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
'milestoneNumber' => 'uint32?',
'projectPath' => 'hashpath64',
'projectDepth' => 'uint32',
'projectPathKey' => 'bytes4',
),
self::CONFIG_KEY_SCHEMA => array(
'key_phid' => null,
@ -224,6 +226,10 @@ final class PhabricatorProject extends PhabricatorProjectDAO
'key_path' => array(
'columns' => array('projectPath', 'projectDepth'),
),
'key_pathkey' => array(
'columns' => array('projectPathKey'),
'unique' => true,
),
),
) + parent::getConfiguration();
}
@ -310,6 +316,12 @@ final class PhabricatorProject extends PhabricatorProjectDAO
$this->setPHID($this->generatePHID());
}
if (!strlen($this->getProjectPathKey())) {
$hash = PhabricatorHash::digestForIndex($this->getPHID());
$hash = substr($hash, 0, 4);
$this->setProjectPathKey($hash);
}
$path = array();
$depth = 0;
if ($this->parentProjectPHID) {
@ -317,15 +329,12 @@ final class PhabricatorProject extends PhabricatorProjectDAO
$path[] = $parent->getProjectPath();
$depth = $parent->getProjectDepth() + 1;
}
$hash = PhabricatorHash::digestForIndex($this->getPHID());
$path[] = substr($hash, 0, 4);
$path[] = $this->getProjectPathKey();
$path = implode('', $path);
$limit = self::getProjectDepthLimit();
if (strlen($path) > ($limit * 4)) {
throw new Exception(
pht('Unable to save project: path length is too long.'));
if ($depth >= $limit) {
throw new Exception(pht('Project depth is too great.'));
}
$this->setProjectPath($path);
@ -403,7 +412,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
$path = $this->getProjectPath();
$parent_length = (strlen($path) - 4);
for ($ii = $parent_length; $ii >= 0; $ii -= 4) {
for ($ii = $parent_length; $ii > 0; $ii -= 4) {
$parts[] = substr($path, 0, $ii);
}