revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 10:42:41 +01:00
Code Issues Releases Wiki Activity

Fix a complicated object caching issue with the policy filter

Browse source 
Summary:
Fixes T11853. To set this up:

  - Create "Project A".
  - Join "Project A".
  - Create a subproject, "Project A Subproject 1".
    - This causes Project A to become a parent project.
    - This moves you to be a member of "Project A Subproject 1" instead of "Project A" directly.
  - Create another subproject, "Project A Subproject 2".
    - Do not join this subproject.
  - Set the second subproject's policy to "Visible To: Members of Project A".
  - Try to edit the second subproject.

Before this change, this fails:

  - When querying projects, we sometime try to skip loading the viewer's membership in ancestor projects as a small optimization.
  - Via `PhabricatorExtendedPolicyInterface`, we may then return the parent project to the policy filter for extended checks.
  - The PolicyFilter has an optimization: if we're checking an object, and we already have that object, we can just use the object we already have. This is common and useful.
  - However, in this case it causes us to reuse an incomplete object (an object without proper membership information). We fail a policy check which we should pass.

Instead, don't skip loading the viewer's membership in ancestor projects.

Test Plan:
  - Did all that stuff above.
  - Could edit the subproject.
  - Ran `arc unit --everything`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11853

Differential Revision: https://secure.phabricator.com/D16840
This commit is contained in:
epriestley 2016-11-11 12:35:22 -08:00
parent ff677c1964
commit 40d3bcb891
1 changed files with 7 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
src/applications/project/query/PhabricatorProjectQuery.php
View file

@ -260,14 +260,14 @@ final class PhabricatorProjectQuery
$all_graph = $this->getAllReachableAncestors($projects); $all_graph = $this->getAllReachableAncestors($projects);
if ($this->needAncestorMembers || $this->needWatchers) { // NOTE: Although we may not need much information about ancestors, we
$src_projects = $all_graph; // always need to test if the viewer is a member, because we will return
} else { // ancestor projects to the policy filter via ExtendedPolicy calls. If
$src_projects = $projects; // we skip populating membership data on a parent, the policy framework
} // will think the user is not a member of the parent project.
$all_sources = array(); $all_sources = array();
foreach ($src_projects as $project) { foreach ($all_graph as $project) {
// For milestones, we need parent members. // For milestones, we need parent members.
if ($project->isMilestone()) { if ($project->isMilestone()) {
$parent_phid = $project->getParentProjectPHID(); $parent_phid = $project->getParentProjectPHID();
@ -306,7 +306,7 @@ final class PhabricatorProjectQuery
} }
$membership_projects = array(); $membership_projects = array();
foreach ($src_projects as $project) { foreach ($all_graph as $project) {
$project_phid = $project->getPHID(); $project_phid = $project->getPHID();
if ($project->isMilestone()) { if ($project->isMilestone()) {