From 40d3bcb8910b37d91ceae09dfd2693190612e045 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 11 Nov 2016 12:35:22 -0800
Subject: [PATCH] Fix a complicated object caching issue with the policy filter

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
---
 .../project/query/PhabricatorProjectQuery.php      | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php
index 87c6bb805e..44055b7260 100644
--- a/src/applications/project/query/PhabricatorProjectQuery.php
+++ b/src/applications/project/query/PhabricatorProjectQuery.php
@@ -260,14 +260,14 @@ final class PhabricatorProjectQuery
 
     $all_graph = $this->getAllReachableAncestors($projects);
 
-    if ($this->needAncestorMembers || $this->needWatchers) {
-      $src_projects = $all_graph;
-    } else {
-      $src_projects = $projects;
-    }
+    // NOTE: Although we may not need much information about ancestors, we
+    // always need to test if the viewer is a member, because we will return
+    // ancestor projects to the policy filter via ExtendedPolicy calls. If
+    // 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();
-    foreach ($src_projects as $project) {
+    foreach ($all_graph as $project) {
       // For milestones, we need parent members.
       if ($project->isMilestone()) {
         $parent_phid = $project->getParentProjectPHID();
@@ -306,7 +306,7 @@ final class PhabricatorProjectQuery
     }
 
     $membership_projects = array();
-    foreach ($src_projects as $project) {
+    foreach ($all_graph as $project) {
       $project_phid = $project->getPHID();
 
       if ($project->isMilestone()) {