From 9a0dd55442d780c78c5a8695817e1643b048738b Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Wed, 25 Apr 2018 17:00:09 -0700
Subject: [PATCH 01/14] Extend PhabricatorPolicyCodex interface to handle
 "interesting" policy defaults

Summary:
Fixes T13128. Ref PHI590. This is a rough-and-ready implementation of a new `PhabricatorPolicyCodex->compareToDefaultPolicy()` method that subclasses can override to handle special cases of policy defaults. Also implements a `PolicyCodex` for Phriction documents, because the default policy of a Phriction document is the policy of the root document.

I might break this change into two parts, one of which maintains the current behavior and another which implements `PhrictionDocumentPolicyCodex`.

Test Plan: Created some Phriction docs, fiddled with policies, observed expected colors in the header. Will test more comprehensively after review for basic reasonable-ness.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, swisspol

Maniphest Tasks: T13128

Differential Revision: https://secure.phabricator.com/D19409
---
 src/__phutil_library_map__.php                |  5 ++
 .../codex/PhrictionDocumentPolicyCodex.php    | 82 +++++++++++++++++++
 .../controller/PhrictionEditController.php    | 13 ++-
 .../phriction/storage/PhrictionDocument.php   | 28 +++----
 .../policy/codex/PhabricatorPolicyCodex.php   | 21 +++++
 .../PhabricatorPolicyStrengthConstants.php    |  9 ++
 .../PhabricatorPolicyExplainController.php    | 44 +++++++---
 .../policy/storage/PhabricatorPolicy.php      | 17 ++--
 src/view/phui/PHUIHeaderView.php              | 59 ++++++++-----
 9 files changed, 218 insertions(+), 60 deletions(-)
 create mode 100644 src/applications/phriction/codex/PhrictionDocumentPolicyCodex.php
 create mode 100644 src/applications/policy/constants/PhabricatorPolicyStrengthConstants.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9de57b2530..c21315f45e 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3867,6 +3867,7 @@ phutil_register_library_map(array(
     'PhabricatorPolicyRequestExceptionHandler' => 'aphront/handler/PhabricatorPolicyRequestExceptionHandler.php',
     'PhabricatorPolicyRule' => 'applications/policy/rule/PhabricatorPolicyRule.php',
     'PhabricatorPolicySearchEngineExtension' => 'applications/policy/engineextension/PhabricatorPolicySearchEngineExtension.php',
+    'PhabricatorPolicyStrengthConstants' => 'applications/policy/constants/PhabricatorPolicyStrengthConstants.php',
     'PhabricatorPolicyTestCase' => 'applications/policy/__tests__/PhabricatorPolicyTestCase.php',
     'PhabricatorPolicyTestObject' => 'applications/policy/__tests__/PhabricatorPolicyTestObject.php',
     'PhabricatorPolicyType' => 'applications/policy/constants/PhabricatorPolicyType.php',
@@ -4998,6 +4999,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentMoveToTransaction' => 'applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php',
     'PhrictionDocumentPHIDType' => 'applications/phriction/phid/PhrictionDocumentPHIDType.php',
     'PhrictionDocumentPathHeraldField' => 'applications/phriction/herald/PhrictionDocumentPathHeraldField.php',
+    'PhrictionDocumentPolicyCodex' => 'applications/phriction/codex/PhrictionDocumentPolicyCodex.php',
     'PhrictionDocumentQuery' => 'applications/phriction/query/PhrictionDocumentQuery.php',
     'PhrictionDocumentSearchConduitAPIMethod' => 'applications/phriction/conduit/PhrictionDocumentSearchConduitAPIMethod.php',
     'PhrictionDocumentSearchEngine' => 'applications/phriction/query/PhrictionDocumentSearchEngine.php',
@@ -9669,6 +9671,7 @@ phutil_register_library_map(array(
     'PhabricatorPolicyRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler',
     'PhabricatorPolicyRule' => 'Phobject',
     'PhabricatorPolicySearchEngineExtension' => 'PhabricatorSearchEngineExtension',
+    'PhabricatorPolicyStrengthConstants' => 'PhabricatorPolicyConstants',
     'PhabricatorPolicyTestCase' => 'PhabricatorTestCase',
     'PhabricatorPolicyTestObject' => array(
       'Phobject',
@@ -11060,6 +11063,7 @@ phutil_register_library_map(array(
       'PhabricatorProjectInterface',
       'PhabricatorApplicationTransactionInterface',
       'PhabricatorConduitResultInterface',
+      'PhabricatorPolicyCodexInterface',
     ),
     'PhrictionDocumentAuthorHeraldField' => 'PhrictionDocumentHeraldField',
     'PhrictionDocumentContentHeraldField' => 'PhrictionDocumentHeraldField',
@@ -11076,6 +11080,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentMoveToTransaction' => 'PhrictionDocumentTransactionType',
     'PhrictionDocumentPHIDType' => 'PhabricatorPHIDType',
     'PhrictionDocumentPathHeraldField' => 'PhrictionDocumentHeraldField',
+    'PhrictionDocumentPolicyCodex' => 'PhabricatorPolicyCodex',
     'PhrictionDocumentQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhrictionDocumentSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
     'PhrictionDocumentSearchEngine' => 'PhabricatorApplicationSearchEngine',
diff --git a/src/applications/phriction/codex/PhrictionDocumentPolicyCodex.php b/src/applications/phriction/codex/PhrictionDocumentPolicyCodex.php
new file mode 100644
index 0000000000..748bec705b
--- /dev/null
+++ b/src/applications/phriction/codex/PhrictionDocumentPolicyCodex.php
@@ -0,0 +1,82 @@
+<?php
+
+final class PhrictionDocumentPolicyCodex
+  extends PhabricatorPolicyCodex {
+
+  public function getPolicySpecialRuleDescriptions() {
+    $object = $this->getObject();
+    $strongest_policy = $this->getStrongestPolicy();
+
+    $rules = array();
+    $rules[] = $this->newRule()
+      ->setDescription(
+        pht('To view a wiki document, you must also be able to view all '.
+            'of its ancestors. The most-restrictive view policy of this '.
+            'document\'s ancestors is "%s".',
+            $strongest_policy->getShortName()))
+      ->setCapabilities(array(PhabricatorPolicyCapability::CAN_VIEW));
+
+    $rules[] = $this->newRule()
+      ->setDescription(
+        pht('To edit a wiki document, you must also be able to view all '.
+            'of its ancestors.'))
+      ->setCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT));
+
+    return $rules;
+  }
+
+  public function getDefaultPolicy() {
+    $ancestors = $this->getObject()->getAncestors();
+    if ($ancestors) {
+      $root = head($ancestors);
+    } else {
+      $root = $this->getObject();
+    }
+
+    $root_policy_phid = $root->getPolicy($this->getCapability());
+
+    return id(new PhabricatorPolicyQuery())
+            ->setViewer($this->getViewer())
+            ->withPHIDs(array($root_policy_phid))
+            ->executeOne();
+  }
+
+  public function compareToDefaultPolicy(PhabricatorPolicy $policy) {
+    $root_policy = $this->getDefaultPolicy();
+    $strongest_policy = $this->getStrongestPolicy();
+
+    // Note that we never return 'weaker', because Phriction documents can
+    // never have weaker permissions than their parents. If this object has
+    // been set to weaker permissions anyway, return 'adjusted'.
+    if ($root_policy == $strongest_policy) {
+      $strength = null;
+    } else if ($strongest_policy->isStrongerThan($root_policy)) {
+      $strength = PhabricatorPolicyStrengthConstants::STRONGER;
+    } else {
+      $strength = PhabricatorPolicyStrengthConstants::ADJUSTED;
+    }
+    return $strength;
+  }
+
+  private function getStrongestPolicy() {
+    $ancestors = $this->getObject()->getAncestors();
+    $ancestors[] = $this->getObject();
+
+    $strongest_policy = $this->getDefaultPolicy();
+    foreach ($ancestors as $ancestor) {
+      $ancestor_policy_phid = $ancestor->getPolicy($this->getCapability());
+
+      $ancestor_policy = id(new PhabricatorPolicyQuery())
+        ->setViewer($this->getViewer())
+        ->withPHIDs(array($ancestor_policy_phid))
+        ->executeOne();
+
+      if ($ancestor_policy->isStrongerThan($strongest_policy)) {
+        $strongest_policy = $ancestor_policy;
+      }
+    }
+
+    return $strongest_policy;
+  }
+
+}
diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php
index e7e2b40d87..006488b274 100644
--- a/src/applications/phriction/controller/PhrictionEditController.php
+++ b/src/applications/phriction/controller/PhrictionEditController.php
@@ -219,6 +219,13 @@ final class PhrictionEditController
       ->execute();
     $view_capability = PhabricatorPolicyCapability::CAN_VIEW;
     $edit_capability = PhabricatorPolicyCapability::CAN_EDIT;
+    $codex = id(PhabricatorPolicyCodex::newFromObject($document, $viewer))
+      ->setCapability($view_capability);
+
+    $view_capability_description = $codex->getPolicySpecialRuleForCapability(
+      PhabricatorPolicyCapability::CAN_VIEW)->getDescription();
+    $edit_capability_description = $codex->getPolicySpecialRuleForCapability(
+      PhabricatorPolicyCapability::CAN_EDIT)->getDescription();
 
     $form = id(new AphrontFormView())
       ->setUser($viewer)
@@ -264,16 +271,14 @@ final class PhrictionEditController
           ->setPolicyObject($document)
           ->setCapability($view_capability)
           ->setPolicies($policies)
-          ->setCaption(
-            $document->describeAutomaticCapability($view_capability)))
+          ->setCaption($view_capability_description))
       ->appendChild(
         id(new AphrontFormPolicyControl())
           ->setName('editPolicy')
           ->setPolicyObject($document)
           ->setCapability($edit_capability)
           ->setPolicies($policies)
-          ->setCaption(
-            $document->describeAutomaticCapability($edit_capability)))
+          ->setCaption($edit_capability_description))
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setLabel(pht('Edit Notes'))
diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php
index 729d3e4ca5..f482579cea 100644
--- a/src/applications/phriction/storage/PhrictionDocument.php
+++ b/src/applications/phriction/storage/PhrictionDocument.php
@@ -11,7 +11,8 @@ final class PhrictionDocument extends PhrictionDAO
     PhabricatorFerretInterface,
     PhabricatorProjectInterface,
     PhabricatorApplicationTransactionInterface,
-    PhabricatorConduitResultInterface {
+    PhabricatorConduitResultInterface,
+    PhabricatorPolicyCodexInterface {
 
   protected $slug;
   protected $depth;
@@ -200,22 +201,6 @@ final class PhrictionDocument extends PhrictionDAO
     return false;
   }
 
-  public function describeAutomaticCapability($capability) {
-
-    switch ($capability) {
-      case PhabricatorPolicyCapability::CAN_VIEW:
-        return pht(
-          'To view a wiki document, you must also be able to view all '.
-          'of its parents.');
-      case PhabricatorPolicyCapability::CAN_EDIT:
-        return pht(
-          'To edit a wiki document, you must also be able to view all '.
-          'of its parents.');
-    }
-
-    return null;
-  }
-
 
 /* -(  PhabricatorSubscribableInterface  )----------------------------------- */
 
@@ -328,4 +313,13 @@ final class PhrictionDocument extends PhrictionDAO
         ->setAttachmentKey('content'),
     );
   }
+
+/* -(  PhabricatorPolicyCodexInterface  )------------------------------------ */
+
+
+    public function newPolicyCodex() {
+      return new PhrictionDocumentPolicyCodex();
+    }
+
+
 }
diff --git a/src/applications/policy/codex/PhabricatorPolicyCodex.php b/src/applications/policy/codex/PhabricatorPolicyCodex.php
index 43a1cc5a1d..060a798e0a 100644
--- a/src/applications/policy/codex/PhabricatorPolicyCodex.php
+++ b/src/applications/policy/codex/PhabricatorPolicyCodex.php
@@ -29,6 +29,27 @@ abstract class PhabricatorPolicyCodex
     return array();
   }
 
+  public function getDefaultPolicy() {
+    return PhabricatorPolicyQuery::getDefaultPolicyForObject(
+      $this->viewer,
+      $this->object,
+      $this->capability);
+  }
+
+  public function compareToDefaultPolicy(PhabricatorPolicy $policy) {
+    return null;
+  }
+
+  final public function getPolicySpecialRuleForCapability($capability) {
+    foreach ($this->getPolicySpecialRuleDescriptions() as $rule) {
+      if (in_array($capability, $rule->getCapabilities())) {
+        return $rule;
+      }
+    }
+
+    return null;
+  }
+
   final protected function newRule() {
     return new PhabricatorPolicyCodexRuleDescription();
   }
diff --git a/src/applications/policy/constants/PhabricatorPolicyStrengthConstants.php b/src/applications/policy/constants/PhabricatorPolicyStrengthConstants.php
new file mode 100644
index 0000000000..9bc3c81ca2
--- /dev/null
+++ b/src/applications/policy/constants/PhabricatorPolicyStrengthConstants.php
@@ -0,0 +1,9 @@
+<?php
+
+final class PhabricatorPolicyStrengthConstants
+  extends PhabricatorPolicyConstants {
+
+  const WEAKER   = 'weaker';
+  const STRONGER = 'stronger';
+  const ADJUSTED = 'adjusted';
+}
diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php
index fc508cfa3a..a4ba355b16 100644
--- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php
+++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php
@@ -169,25 +169,47 @@ final class PhabricatorPolicyExplainController
     $capability) {
     $viewer = $this->getViewer();
 
-    $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject(
-      $viewer,
-      $object,
-      $capability);
-    if (!$default_policy) {
+
+    $strength = null;
+    if ($object instanceof PhabricatorPolicyCodexInterface) {
+      $codex = id(PhabricatorPolicyCodex::newFromObject($object, $viewer))
+        ->setCapability($capability);
+      $strength = $codex->compareToDefaultPolicy($policy);
+      $default_policy = $codex->getDefaultPolicy();
+    } else {
+      $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject(
+        $viewer,
+        $object,
+        $capability);
+
+      if ($default_policy) {
+        if ($default_policy->getPHID() == $policy->getPHID()) {
+          return;
+        }
+
+        if ($default_policy->getPHID() != $policy->getPHID()) {
+          if ($default_policy->isStrongerThan($policy)) {
+            $strength = PhabricatorPolicyStrengthConstants::WEAKER;
+          } else if ($policy->isStrongerThan($default_policy)) {
+            $strength = PhabricatorPolicyStrengthConstants::STRONGER;
+          } else {
+            $strength = PhabricatorPolicyStrengthConstants::ADJUSTED;
+          }
+        }
+      }
+    }
+
+    if (!$strength) {
       return;
     }
 
-    if ($default_policy->getPHID() == $policy->getPHID()) {
-      return;
-    }
-
-    if ($default_policy->isStrongerThan($policy)) {
+    if ($strength == PhabricatorPolicyStrengthConstants::WEAKER) {
       $info = pht(
         'This object has a less restrictive policy ("%s") than the default '.
         'policy for similar objects (which is "%s").',
         $policy->getShortName(),
         $default_policy->getShortName());
-    } else if ($policy->isStrongerThan($default_policy)) {
+    } else if ($strength == PhabricatorPolicyStrengthConstants::STRONGER) {
       $info = pht(
         'This object has a more restrictive policy ("%s") than the default '.
         'policy for similar objects (which is "%s").',
diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php
index 4141ef9c36..2df8fdf6a0 100644
--- a/src/applications/policy/storage/PhabricatorPolicy.php
+++ b/src/applications/policy/storage/PhabricatorPolicy.php
@@ -434,11 +434,12 @@ final class PhabricatorPolicy
     $capability,
     $active_only) {
 
+    $exceptions = array();
     if ($object instanceof PhabricatorPolicyCodexInterface) {
-      $codex = PhabricatorPolicyCodex::newFromObject($object, $viewer);
+      $codex = id(PhabricatorPolicyCodex::newFromObject($object, $viewer))
+        ->setCapability($capability);
       $rules = $codex->getPolicySpecialRuleDescriptions();
 
-      $exceptions = array();
       foreach ($rules as $rule) {
         $is_active = $rule->getIsActive();
         if ($is_active) {
@@ -467,11 +468,13 @@ final class PhabricatorPolicy
 
         $exceptions[] = $description;
       }
-    } else if (method_exists($object, 'describeAutomaticCapability')) {
-      $exceptions = (array)$object->describeAutomaticCapability($capability);
-      $exceptions = array_filter($exceptions);
-    } else {
-      $exceptions = array();
+    }
+
+    if (!$exceptions) {
+      if (method_exists($object, 'describeAutomaticCapability')) {
+        $exceptions = (array)$object->describeAutomaticCapability($capability);
+        $exceptions = array_filter($exceptions);
+      }
     }
 
     return $exceptions;
diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php
index 4ac4b2de54..54f4fa58ee 100644
--- a/src/view/phui/PHUIHeaderView.php
+++ b/src/view/phui/PHUIHeaderView.php
@@ -473,30 +473,47 @@ final class PHUIHeaderView extends AphrontTagView {
     // policy differs from the default policy. If it does, we'll call it out
     // as changed.
     if (!$use_space_policy) {
-      $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject(
-        $viewer,
-        $object,
-        $view_capability);
-      if ($default_policy) {
-        if ($default_policy->getPHID() != $policy->getPHID()) {
-          $container_classes[] = 'policy-adjusted';
-          if ($default_policy->isStrongerThan($policy)) {
-            // The policy has strictly been weakened. For example, the
-            // default might be "All Users" and the current policy is "Public".
-            $container_classes[] = 'policy-adjusted-weaker';
-          } else if ($policy->isStrongerThan($default_policy)) {
-            // The policy has strictly been strengthened, and is now more
-            // restrictive than the default. For example, "All Users" has
-            // been replaced with "No One".
-            $container_classes[] = 'policy-adjusted-stronger';
-          } else {
-            // The policy has been adjusted but not strictly strengthened
-            // or weakened. For example, "Members of X" has been replaced with
-            // "Members of Y".
-            $container_classes[] = 'policy-adjusted-different';
+      $strength = null;
+      if ($object instanceof PhabricatorPolicyCodexInterface) {
+        $codex = id(PhabricatorPolicyCodex::newFromObject($object, $viewer))
+          ->setCapability($view_capability);
+        $strength = $codex->compareToDefaultPolicy($policy);
+      } else {
+        $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject(
+          $viewer,
+          $object,
+          $view_capability);
+
+        if ($default_policy) {
+          if ($default_policy->getPHID() != $policy->getPHID()) {
+            if ($default_policy->isStrongerThan($policy)) {
+              $strength = PhabricatorPolicyStrengthConstants::WEAKER;
+            } else if ($policy->isStrongerThan($default_policy)) {
+              $strength = PhabricatorPolicyStrengthConstants::STRONGER;
+            } else {
+              $strength = PhabricatorPolicyStrengthConstants::ADJUSTED;
+            }
           }
         }
       }
+
+      if ($strength) {
+        if ($strength == PhabricatorPolicyStrengthConstants::WEAKER) {
+          // The policy has strictly been weakened. For example, the
+          // default might be "All Users" and the current policy is "Public".
+          $container_classes[] = 'policy-adjusted-weaker';
+        } else if ($strength == PhabricatorPolicyStrengthConstants::STRONGER) {
+          // The policy has strictly been strengthened, and is now more
+          // restrictive than the default. For example, "All Users" has
+          // been replaced with "No One".
+          $container_classes[] = 'policy-adjusted-stronger';
+        } else {
+          // The policy has been adjusted but not strictly strengthened
+          // or weakened. For example, "Members of X" has been replaced with
+          // "Members of Y".
+          $container_classes[] = 'policy-adjusted-different';
+        }
+      }
     }
 
     $policy_name = array($policy->getShortName());

From ef48a2b2eebc8bb513c3b7dca1f1e4847900c058 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 28 Apr 2018 06:12:14 -0700
Subject: [PATCH 02/14] Add a "Rule Detail" link to Herald email

Summary:
See PHI285. Ref T13130. After recent changes Herald sends email about rules, but the mail doesn't currently actually include a link to the rule.

Include a link for consistency and ease-of-use.

Test Plan: Edited a rule, looked at the resulting mail, saw a link to the rule.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19413
---
 .../herald/editor/HeraldRuleEditor.php             | 14 ++++++++++++++
 src/applications/herald/storage/HeraldRule.php     |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/src/applications/herald/editor/HeraldRuleEditor.php b/src/applications/herald/editor/HeraldRuleEditor.php
index 8c83ef6597..3ba5c4f8ac 100644
--- a/src/applications/herald/editor/HeraldRuleEditor.php
+++ b/src/applications/herald/editor/HeraldRuleEditor.php
@@ -137,4 +137,18 @@ final class HeraldRuleEditor
     return pht('[Herald]');
   }
 
+
+  protected function buildMailBody(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+
+    $body = parent::buildMailBody($object, $xactions);
+
+    $body->addLinkSection(
+      pht('RULE DETAIL'),
+      PhabricatorEnv::getProductionURI($object->getURI()));
+
+    return $body;
+  }
+
 }
diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php
index 44ef68ac03..d2b8d36f05 100644
--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@@ -254,6 +254,10 @@ final class HeraldRule extends HeraldDAO
     return 'H'.$this->getID();
   }
 
+  public function getURI() {
+    return '/'.$this->getMonogram();
+  }
+
 
 /* -(  Repetition Policies  )------------------------------------------------ */
 

From dd6e82698aebc93bd09b033e8a7a030f320ff9bf Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Mon, 30 Apr 2018 12:10:37 -0700
Subject: [PATCH 03/14] More-robust search for task assignees

Summary: See discussion in D19415.

Test Plan: Searched for some owners, found tasks as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19417
---
 src/applications/maniphest/query/ManiphestTaskQuery.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php
index edb780b724..16aa83a2ea 100644
--- a/src/applications/maniphest/query/ManiphestTaskQuery.php
+++ b/src/applications/maniphest/query/ManiphestTaskQuery.php
@@ -585,7 +585,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         'task.ownerPHID IS NOT NULL');
     }
 
-    if ($this->ownerPHIDs) {
+    if ($this->ownerPHIDs !== null) {
       $subclause[] = qsprintf(
         $conn,
         'task.ownerPHID IN (%Ls)',

From afc3099ee785c0f9cb86c702fc1d04d2be9f6af5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 28 Apr 2018 06:43:09 -0700
Subject: [PATCH 04/14] Add a view option to disable blame in Diffusion and fix
 some view transition bugs

Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.

Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.

Test Plan:
  - Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
  - Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
  - Viewed stuff in Files (no blame UI options).
  - Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13105

Differential Revision: https://secure.phabricator.com/D19414
---
 resources/celerity/map.php                    | 14 +++---
 .../DiffusionDocumentRenderingEngine.php      | 12 +++--
 .../document/PhabricatorDocumentEngine.php    | 14 ++++++
 .../PhabricatorSourceDocumentEngine.php       |  2 +-
 .../PhabricatorDocumentRenderingEngine.php    | 17 +++++++
 .../files/behavior-document-engine.js         | 45 ++++++++++++++++++-
 6 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 480c4b602d..78b3660c4c 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -388,7 +388,7 @@ return array(
     'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc',
     'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70',
     'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef',
-    'rsrc/js/application/files/behavior-document-engine.js' => 'ee0deff8',
+    'rsrc/js/application/files/behavior-document-engine.js' => '3935d8c4',
     'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab',
     'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888',
     'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '549459b8',
@@ -600,7 +600,7 @@ return array(
     'javelin-behavior-diffusion-commit-graph' => '75b83cbb',
     'javelin-behavior-diffusion-locate-file' => '6d3e1947',
     'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc',
-    'javelin-behavior-document-engine' => 'ee0deff8',
+    'javelin-behavior-document-engine' => '3935d8c4',
     'javelin-behavior-doorkeeper-tag' => '1db13e70',
     'javelin-behavior-drydock-live-operation-status' => '901935ef',
     'javelin-behavior-durable-column' => '2ae077e1',
@@ -1080,6 +1080,11 @@ return array(
       'javelin-dom',
       'javelin-vector',
     ),
+    '3935d8c4' => array(
+      'javelin-behavior',
+      'javelin-dom',
+      'javelin-stratcom',
+    ),
     '3ab51e2c' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -2105,11 +2110,6 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
-    'ee0deff8' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-stratcom',
-    ),
     'efe49472' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
index 9615b35799..800d195726 100644
--- a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
+++ b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
@@ -69,7 +69,7 @@ final class DiffusionDocumentRenderingEngine
     return;
   }
 
-  protected function willRenderRef(PhabricatorDocumentRef $ref) {
+  protected function willStageRef(PhabricatorDocumentRef $ref) {
     $drequest = $this->getDiffusionRequest();
 
     $blame_uri = (string)$drequest->generateURI(
@@ -78,9 +78,13 @@ final class DiffusionDocumentRenderingEngine
         'stable' => true,
       ));
 
-    $ref
-      ->setSymbolMetadata($this->getSymbolMetadata())
-      ->setBlameURI($blame_uri);
+    $ref->setBlameURI($blame_uri);
+  }
+
+  protected function willRenderRef(PhabricatorDocumentRef $ref) {
+    $drequest = $this->getDiffusionRequest();
+
+    $ref->setSymbolMetadata($this->getSymbolMetadata());
 
     $coverage = $drequest->loadCoverage();
     if (strlen($coverage)) {
diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php
index 85219b904c..c869f5f0d7 100644
--- a/src/applications/files/document/PhabricatorDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorDocumentEngine.php
@@ -7,6 +7,7 @@ abstract class PhabricatorDocumentEngine
   private $highlightedLines = array();
   private $encodingConfiguration;
   private $highlightingConfiguration;
+  private $blameConfiguration = true;
 
   final public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -60,6 +61,19 @@ abstract class PhabricatorDocumentEngine
     return $this->highlightingConfiguration;
   }
 
+  final public function setBlameConfiguration($blame_configuration) {
+    $this->blameConfiguration = $blame_configuration;
+    return $this;
+  }
+
+  final public function getBlameConfiguration() {
+    return $this->blameConfiguration;
+  }
+
+  final protected function getBlameEnabled() {
+    return $this->blameConfiguration;
+  }
+
   public function shouldRenderAsync(PhabricatorDocumentRef $ref) {
     return false;
   }
diff --git a/src/applications/files/document/PhabricatorSourceDocumentEngine.php b/src/applications/files/document/PhabricatorSourceDocumentEngine.php
index c88d979b4e..5d7c0cdb75 100644
--- a/src/applications/files/document/PhabricatorSourceDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorSourceDocumentEngine.php
@@ -50,7 +50,7 @@ final class PhabricatorSourceDocumentEngine
     }
 
     $options = array();
-    if ($ref->getBlameURI()) {
+    if ($ref->getBlameURI() && $this->getBlameEnabled()) {
       $content = phutil_split_lines($content);
       $blame = range(1, count($content));
       $blame = array_fuse($blame);
diff --git a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
index 155ed28dfa..2524f77821 100644
--- a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
+++ b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
@@ -69,6 +69,9 @@ abstract class PhabricatorDocumentRenderingEngine
       $engine->setHighlightingConfiguration($highlight_setting);
     }
 
+    $blame_setting = ($request->getStr('blame') !== 'off');
+    $engine->setBlameConfiguration($blame_setting);
+
     $views = array();
     foreach ($engines as $candidate_key => $candidate_engine) {
       $label = $candidate_engine->getViewAsLabel($ref);
@@ -104,6 +107,8 @@ abstract class PhabricatorDocumentRenderingEngine
       'controlID' => $control_id,
     );
 
+    $this->willStageRef($ref);
+
     if ($engine->shouldRenderAsync($ref)) {
       $content = $engine->newLoadingContent($ref);
       $config['next'] = 'render';
@@ -142,7 +147,11 @@ abstract class PhabricatorDocumentRenderingEngine
         'value' => $highlight_setting,
       ),
       'blame' => array(
+        'icon' => 'fa-backward',
+        'hide' => pht('Hide Blame'),
+        'show' => pht('Show Blame'),
         'uri' => $ref->getBlameURI(),
+        'enabled' => $blame_setting,
         'value' => null,
       ),
       'coverage' => array(
@@ -180,6 +189,7 @@ abstract class PhabricatorDocumentRenderingEngine
   }
 
   final public function newRenderResponse(PhabricatorDocumentRef $ref) {
+    $this->willStageRef($ref);
     $this->willRenderRef($ref);
 
     $request = $this->getRequest();
@@ -207,6 +217,9 @@ abstract class PhabricatorDocumentRenderingEngine
       $engine->setHighlightingConfiguration($highlight_setting);
     }
 
+    $blame_setting = ($request->getStr('blame') !== 'off');
+    $engine->setBlameConfiguration($blame_setting);
+
     try {
       $content = $engine->newDocument($ref);
     } catch (Exception $ex) {
@@ -314,6 +327,10 @@ abstract class PhabricatorDocumentRenderingEngine
     return;
   }
 
+  protected function willStageRef(PhabricatorDocumentRef $ref) {
+    return;
+  }
+
   protected function willRenderRef(PhabricatorDocumentRef $ref) {
     return;
   }
diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js
index 8b957e0ea0..caad5977bc 100644
--- a/webroot/rsrc/js/application/files/behavior-document-engine.js
+++ b/webroot/rsrc/js/application/files/behavior-document-engine.js
@@ -105,6 +105,29 @@ JX.behavior('document-engine', function(config, statics) {
 
     list.addItem(highlight_item);
 
+    var blame_item;
+    if (data.blame.uri) {
+      blame_item = new JX.PHUIXActionView()
+        .setIcon(data.blame.icon);
+
+      var onblame = JX.bind(null, function(data, e) {
+        e.prevent();
+
+        if (blame_item.getDisabled()) {
+          return;
+        }
+
+        data.blame.enabled = !data.blame.enabled;
+        onview(data);
+
+        menu.close();
+      }, data);
+
+      blame_item.setHandler(onblame);
+
+      list.addItem(blame_item);
+    }
+
     menu.setContent(list.getNode());
 
     menu.listen('open', function() {
@@ -118,8 +141,22 @@ JX.behavior('document-engine', function(config, statics) {
         if (is_selected) {
           encode_item.setDisabled(!engine.spec.canEncode);
           highlight_item.setDisabled(!engine.spec.canHighlight);
+          if (blame_item) {
+            blame_item.setDisabled(!engine.spec.canBlame);
+          }
         }
       }
+
+      if (blame_item) {
+        var blame_label;
+        if (data.blame.enabled) {
+          blame_label = data.blame.hide;
+        } else {
+          blame_label = data.blame.show;
+        }
+
+        blame_item.setName(blame_label);
+      }
     });
 
     data.menu = menu;
@@ -137,6 +174,12 @@ JX.behavior('document-engine', function(config, statics) {
       uri.setQueryParam('encode', data.encode.value);
     }
 
+    if (data.blame.enabled) {
+      uri.setQueryParam('blame', null);
+    } else {
+      uri.setQueryParam('blame', 'off');
+    }
+
     return uri.toString();
   }
 
@@ -211,7 +254,7 @@ JX.behavior('document-engine', function(config, statics) {
     JX.DOM.setContent(viewport, JX.$H(r.markup));
 
     // If this engine supports rendering blame, populate or draw it.
-    if (spec.canBlame) {
+    if (spec.canBlame && data.blame.enabled) {
       blame(data);
     }
   }

From 24305cadb9026a744a554e26b75e5a1e89c3ae32 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Apr 2018 11:51:55 -0700
Subject: [PATCH 05/14] Hide the "large" diff warning on "very large" diffs

Summary:
Ref T13110. Ref T13130. When a revision is "large" (100 - 1000 files) we hide the actual textual changes by default. When it is "very large" (more than 1000 files) we hide all the changesets by default.

For "very large" diffs, we currently still show the "large" warning, which doesn't really make sense since there aren't any actual changesets.

When a diff is "very large", don't show the "large" warning.

Test Plan:
  - Viewed a small diff (<100 files), saw no warnings.
  - Viewed a large diff (100-1000 files), saw just the large warning.
  - Viewed a very large diff (>1000 files).
    - Before: both "large" and "very large" help warnings.
    - After: just "very large" warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13110

Differential Revision: https://secure.phabricator.com/D19416
---
 .../controller/DifferentialRevisionViewController.php  | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index 9b015887a6..9672654c58 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -153,9 +153,17 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $request_uri = $request->getRequestURI();
 
+    // Revisions with more than 100 files are "large".
+    // Revisions with more than 1000 files are "very large".
     $limit = 100;
     $large = $request->getStr('large');
-    if (count($changesets) > $limit && !$large) {
+
+    $large_warning =
+      (!$this->isVeryLargeDiff()) &&
+      (count($changesets) > $limit) &&
+      (!$large);
+
+    if ($large_warning) {
       $count = count($changesets);
       $warning = new PHUIInfoView();
       $warning->setTitle(pht('Large Diff'));

From ee32c186dd822b90826514ff8fc61740ebc8bd68 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 30 Apr 2018 12:06:16 -0700
Subject: [PATCH 06/14] Stop computing ownership for changed paths for Very
 Large revisions

Summary:
Depends on D19416. Ref T13110. Ref T13130. See PHI598. When rendering a "Very Large" revision (affecting more than 1,000 files) we currently compute the package/changeset ownership map normally.

This is basically a big list of which packages own which of the files affected by the change. We use it to:

  # Show which packages own each file in the table of contents.
  # Show an "(Owns No Changed Paths)" hint in the reviewers list to help catch out-of-date packages that are no longer relevant.

However, this is expensive to build. We don't render the table of contents at all, so (1) is pointless. The value of (2) is very small on these types of changes, and certainly not worth spending many many seconds computing ownership.

Instead, just skip building out these relationships for very large changes.

Test Plan: Viewed a very large change with package owners; verified it no longer built package map data and rendered the package owners with no "(Owns No Changed Paths)" hints.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13110

Differential Revision: https://secure.phabricator.com/D19418
---
 .../DifferentialRevisionViewController.php    | 67 +++++++++++++------
 1 file changed, 47 insertions(+), 20 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index 9672654c58..9959bfabab 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1,22 +1,43 @@
 <?php
 
-final class DifferentialRevisionViewController extends DifferentialController {
+final class DifferentialRevisionViewController
+  extends DifferentialController {
 
   private $revisionID;
-  private $veryLargeDiff;
+  private $changesetCount;
 
   public function shouldAllowPublic() {
     return true;
   }
 
+  public function isLargeDiff() {
+    return ($this->getChangesetCount() > $this->getLargeDiffLimit());
+  }
+
   public function isVeryLargeDiff() {
-    return $this->veryLargeDiff;
+    return ($this->getChangesetCount() > $this->getVeryLargeDiffLimit());
+  }
+
+  public function getLargeDiffLimit() {
+    return 100;
   }
 
   public function getVeryLargeDiffLimit() {
     return 1000;
   }
 
+  public function getChangesetCount() {
+    if ($this->changesetCount === null) {
+      throw new PhutilInvalidStateException('setChangesetCount');
+    }
+    return $this->changesetCount;
+  }
+
+  public function setChangesetCount($count) {
+    $this->changesetCount = $count;
+    return $this;
+  }
+
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
     $this->revisionID = $request->getURIData('id');
@@ -82,9 +103,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
         idx($diffs, $diff_vs),
         $repository);
 
-    if (count($rendering_references) > $this->getVeryLargeDiffLimit()) {
-      $this->veryLargeDiff = true;
-    }
+    $this->setChangesetCount(count($rendering_references));
 
     if ($request->getExists('download')) {
       return $this->buildRawDiffResponse(
@@ -153,18 +172,16 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $request_uri = $request->getRequestURI();
 
-    // Revisions with more than 100 files are "large".
-    // Revisions with more than 1000 files are "very large".
-    $limit = 100;
     $large = $request->getStr('large');
 
     $large_warning =
+      ($this->isLargeDiff()) &&
       (!$this->isVeryLargeDiff()) &&
-      (count($changesets) > $limit) &&
       (!$large);
 
     if ($large_warning) {
-      $count = count($changesets);
+      $count = $this->getChangesetCount();
+
       $warning = new PHUIInfoView();
       $warning->setTitle(pht('Large Diff'));
       $warning->setSeverity(PHUIInfoView::SEVERITY_WARNING);
@@ -365,23 +382,33 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $other_view = $this->renderOtherRevisions($other_revisions);
     }
 
-    $this->buildPackageMaps($changesets);
-
     if ($this->isVeryLargeDiff()) {
       $toc_view = null;
+
+      // When rendering a "very large" diff, we skip computation of owners
+      // that own no files because it is significantly expensive and not very
+      // valuable.
+      foreach ($revision->getReviewers() as $reviewer) {
+        // Give each reviewer a dummy nonempty value so the UI does not render
+        // the "(Owns No Changed Paths)" note. If that behavior becomes more
+        // sophisticated in the future, this behavior might also need to.
+        $reviewer->attachChangesets($changesets);
+      }
     } else {
+      $this->buildPackageMaps($changesets);
+
       $toc_view = $this->buildTableOfContents(
         $changesets,
         $visible_changesets,
         $target->loadCoverageMap($viewer));
-    }
 
-    // Attach changesets to each reviewer so we can show which Owners package
-    // reviewers own no files.
-    foreach ($revision->getReviewers() as $reviewer) {
-      $reviewer_phid = $reviewer->getReviewerPHID();
-      $reviewer_changesets = $this->getPackageChangesets($reviewer_phid);
-      $reviewer->attachChangesets($reviewer_changesets);
+      // Attach changesets to each reviewer so we can show which Owners package
+      // reviewers own no files.
+      foreach ($revision->getReviewers() as $reviewer) {
+        $reviewer_phid = $reviewer->getReviewerPHID();
+        $reviewer_changesets = $this->getPackageChangesets($reviewer_phid);
+        $reviewer->attachChangesets($reviewer_changesets);
+      }
     }
 
     $tab_group = id(new PHUITabGroupView());

From 7cfac40a225bdc95080837245444afa4e122085d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 24 Apr 2018 16:13:29 -0700
Subject: [PATCH 07/14] Pass full Harbormaster URIs to Buildkite

Summary: See PHI611 for details.

Test Plan:
Ran a Buildkite build, saw Buildkite confirm receipt of these parameters in the HTTP response:

{F5562054}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19419
---
 .../step/HarbormasterBuildkiteBuildStepImplementation.php | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php
index 89d4002eef..8a522c4926 100644
--- a/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php
+++ b/src/applications/harbormaster/step/HarbormasterBuildkiteBuildStepImplementation.php
@@ -106,6 +106,14 @@ EOTEXT
       ),
       'meta_data' => array(
         'buildTargetPHID' => $build_target->getPHID(),
+
+        // See PHI611. These are undocumented secret magic.
+        'phabricator:build:id' => (int)$build->getID(),
+        'phabricator:build:url' =>
+          PhabricatorEnv::getProductionURI($build->getURI()),
+        'phabricator:buildable:id' => (int)$buildable->getID(),
+        'phabricator:buildable:url' =>
+          PhabricatorEnv::getProductionURI($buildable->getURI()),
       ),
     );
 

From fb4b9bc2fc61264f7d6f6e61e5675a1d261d679d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 1 May 2018 06:55:25 -0700
Subject: [PATCH 08/14] Fix an issue where entering the same Owners path for
 two repositories would incorrectly de-dupe the path

Summary:
Ref T13130. See <https://discourse.phabricator-community.org/t/unable-to-create-owners-package-with-same-path-in-multiple-repositories/1400/1>.

When you edit paths in Owners, we deduplicate similar paths, like `/x/y` and `/x/y/`. However, this logic currently only examines the paths, and incorrectly deduplicates the same path in different repositories.

Instead, consider the repository before deduplicating.

Test Plan:
  - Edited an Owners package and added the path "/" in two different repositories.
  - Before: only one surived the edit.
  - After: both survived.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19420
---
 .../PhabricatorOwnersPackagePathsTransaction.php       | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
index 984b26b8ac..753b6ff9e9 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
@@ -108,19 +108,23 @@ final class PhabricatorOwnersPackagePathsTransaction
     // paths now.
 
     $display_map = array();
+    $seen_map = array();
     foreach ($new as $key => $spec) {
       $display_path = $spec['path'];
       $raw_path = rtrim($display_path, '/').'/';
 
-      // If the user entered two paths which normalize to the same value
-      // (like "src/main.c" and "src/main.c/"), discard the duplicates.
-      if (isset($display_map[$raw_path])) {
+      // If the user entered two paths in the same repository which normalize
+      // to the same value (like "src/main.c" and "src/main.c/"), discard the
+      // duplicates.
+      $repository_phid = $spec['repositoryPHID'];
+      if (isset($seen_map[$repository_phid][$raw_path])) {
         unset($new[$key]);
         continue;
       }
 
       $new[$key]['path'] = $raw_path;
       $display_map[$raw_path] = $display_path;
+      $seen_map[$repository_phid][$raw_path] = true;
     }
 
     $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);

From 332f4ab66d185e3b41146f48c7bec3914445be39 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 1 May 2018 07:08:45 -0700
Subject: [PATCH 09/14] Restore support for using "arc download" to fetch files
 with no "security.alternate-file-domain"

Summary:
Fixes T13132. I removed this branch in D19156 when tightening the logic for the new CSP header, but there's a legitimate need for it: downloading files via `arc download`, or more generally being an API consumer of files.

This is not completely safe, but attacks I'm aware of (particularly, cookie fixation, where an attacker could potentially force a victim to become logged in to an account they control) are difficult and not very powerful. We already issue clear setup advice about the importance of configuring this option ("Phabricator is currently configured to serve user uploads directly from the same domain as other content. This is a security risk.") and I think there's significant value in letting API clients just GET file data without having to jump through a lot of weird hoops.

Test Plan:
  - With `security.alternate-file-domain` off, tried to `arc download` a file.
  - Before: downloaded an HTML dialog page.
  - After: downloaded the file.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13132

Differential Revision: https://secure.phabricator.com/D19421
---
 .../PhabricatorFileDataController.php         | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index bd3d933283..dd5e0adb7d 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -84,12 +84,29 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
       $response->setMimeType($file->getViewableMimeType());
     } else {
       $is_post = $request->isHTTPPost();
+      $is_public = !$viewer->isLoggedIn();
 
       // NOTE: Require POST to download files from the primary domain. If the
       // request is not a POST request but arrives on the primary domain, we
       // render a confirmation dialog. For discussion, see T13094.
 
-      $is_safe = ($is_alternate_domain || $is_lfs || $is_post);
+      // There are two exceptions to this rule:
+
+      // Git LFS requests can download with GET. This is safe (Git LFS won't
+      // execute files it downloads) and necessary to support Git LFS.
+
+      // Requests with no credentials may also download with GET. This
+      // primarily supports downloading files with `arc download` or other
+      // API clients. This is only "mostly" safe: if you aren't logged in, you
+      // are likely immune to XSS and CSRF. However, an attacker may still be
+      // able to set cookies on this domain (for example, to fixate your
+      // session). For now, we accept these risks because users running
+      // Phabricator in this mode are knowingly accepting a security risk
+      // against setup advice, and there's significant value in having
+      // API development against test and production installs work the same
+      // way.
+
+      $is_safe = ($is_alternate_domain || $is_post || $is_lfs || $is_public);
       if (!$is_safe) {
         return $this->newDialog()
           ->setSubmitURI($file->getDownloadURI())

From 5784e3d3c0cf817512023b3d797c03290ae26a8c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 May 2018 09:20:08 -0700
Subject: [PATCH 10/14] Omit "type" attribute from "<source />" tags in
 "<video>" to trick Chrome into playing them

Summary:
Fixes T13135. See PHI633. For at least some video files with legitimate MIME type "video/quicktime", Chrome can play them but refuses to if the `<source />` tag has a `type="video/quicktime"` attribute.

To trick Chrome into giving these videos the old college try, omit the "type" attribute. Chrome then tries to play the video, seems to realize it can, and we're back on track.

Since the "type" attribute is theoretically only useful to help browsers select among multiple different alternatives and we're only presenting one alternative, this seems likely safe and reasonable. Omitting "type" also validates. It's hard to be certain that this won't cause any collateral damage, but intuitively it seems like it should be safe and I wasn't able to identify any problems.

Test Plan:
  - Watched a "video/quicktime" MP4 cat video in Chrome/Safari/Firefox.
  - See T13135 for discussion, context, and discussion of the behavior of some smaller reproduction cases.

Reviewers: amckinley, asherkin

Reviewed By: amckinley

Maniphest Tasks: T13135

Differential Revision: https://secure.phabricator.com/D19424
---
 .../markup/PhabricatorEmbedFileRemarkupRule.php    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php b/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
index 9cbc119c84..983dea4a02 100644
--- a/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
@@ -260,6 +260,18 @@ final class PhabricatorEmbedFileRemarkupRule
       $autoplay = null;
     }
 
+    if ($is_video) {
+      // See T13135. Chrome refuses to play videos with type "video/quicktime",
+      // even though it may actually be able to play them. The least awful fix
+      // based on available information is to simply omit the "type" attribute
+      // from `<source />` tags. This causes Chrome to try to play the video
+      // and realize it can, and does not appear to produce any bad behavior in
+      // any other browser.
+      $mime_type = null;
+    } else {
+      $mime_type = $file->getMimeType();
+    }
+
     return $this->newTag(
       $tag,
       array(
@@ -274,7 +286,7 @@ final class PhabricatorEmbedFileRemarkupRule
         'source',
         array(
           'src' => $file->getBestURI(),
-          'type' => $file->getMimeType(),
+          'type' => $mime_type,
         )));
   }
 

From af295341c8639d3c065db4308a40b9f370929288 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 May 2018 12:20:57 -0700
Subject: [PATCH 11/14] Classify changesets as "generated" at creation time, in
 addition to display time

Summary:
Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.

An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.

To support these, move toward a world where:

  - Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
  - Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make `arc` mark every file as "generated" and sneak past review rules in the future.

Here, the `differential.generated-paths` config can mark a file as "generated" with a trusted attribute. The `@generated`-in-content rule can mark a file as "generated" with an untrusted attribute.

Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.

Test Plan:
  - Created a revision touching several files, some generated and some not.
  - Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19425
---
 .../parser/DifferentialChangesetParser.php    |  6 ++
 .../storage/DifferentialChangeset.php         | 91 ++++++++++++++++++-
 .../differential/storage/DifferentialDiff.php | 45 +++++++++
 3 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
index 1eeb4c8452..beb1b5a9a7 100644
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -542,6 +542,12 @@ final class DifferentialChangesetParser extends Phobject {
     PhutilEventEngine::dispatchEvent($event);
 
     $generated = $event->getValue('is_generated');
+
+    $attribute = $this->changeset->isGeneratedChangeset();
+    if ($attribute) {
+      $generated = true;
+    }
+
     $this->specialAttributes[self::ATTR_GENERATED] = $generated;
   }
 
diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php
index 4832f452e6..27d3ad4d64 100644
--- a/src/applications/differential/storage/DifferentialChangeset.php
+++ b/src/applications/differential/storage/DifferentialChangeset.php
@@ -12,7 +12,7 @@ final class DifferentialChangeset
   protected $awayPaths;
   protected $changeType;
   protected $fileType;
-  protected $metadata;
+  protected $metadata = array();
   protected $oldProperties;
   protected $newProperties;
   protected $addLines;
@@ -24,6 +24,11 @@ final class DifferentialChangeset
 
   const TABLE_CACHE = 'differential_changeset_parse_cache';
 
+  const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted';
+  const METADATA_UNTRUSTED_ATTRIBUTES = 'attributes.untrusted';
+
+  const ATTRIBUTE_GENERATED = 'generated';
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_SERIALIZATION => array(
@@ -266,6 +271,90 @@ final class DifferentialChangeset
     return null;
   }
 
+  public function setChangesetMetadata($key, $value) {
+    if (!is_array($this->metadata)) {
+      $this->metadata = array();
+    }
+
+    $this->metadata[$key] = $value;
+
+    return $this;
+  }
+
+  public function getChangesetMetadata($key, $default = null) {
+    if (!is_array($this->metadata)) {
+      return $default;
+    }
+
+    return idx($this->metadata, $key, $default);
+  }
+
+  private function setInternalChangesetAttribute($trusted, $key, $value) {
+    if ($trusted) {
+      $meta_key = self::METADATA_TRUSTED_ATTRIBUTES;
+    } else {
+      $meta_key = self::METADATA_UNTRUSTED_ATTRIBUTES;
+    }
+
+    $attributes = $this->getChangesetMetadata($meta_key, array());
+    $attributes[$key] = $value;
+    $this->setChangesetMetadata($meta_key, $attributes);
+
+    return $this;
+  }
+
+  private function getInternalChangesetAttributes($trusted) {
+    if ($trusted) {
+      $meta_key = self::METADATA_TRUSTED_ATTRIBUTES;
+    } else {
+      $meta_key = self::METADATA_UNTRUSTED_ATTRIBUTES;
+    }
+
+    return $this->getChangesetMetadata($meta_key, array());
+  }
+
+  public function setTrustedChangesetAttribute($key, $value) {
+    return $this->setInternalChangesetAttribute(true, $key, $value);
+  }
+
+  public function getTrustedChangesetAttributes() {
+    return $this->getInternalChangesetAttributes(true);
+  }
+
+  public function getTrustedChangesetAttribute($key, $default = null) {
+    $map = $this->getTrustedChangesetAttributes();
+    return idx($map, $key, $default);
+  }
+
+  public function setUntrustedChangesetAttribute($key, $value) {
+    return $this->setInternalChangesetAttribute(false, $key, $value);
+  }
+
+  public function getUntrustedChangesetAttributes() {
+    return $this->getInternalChangesetAttributes(false);
+  }
+
+  public function getUntrustedChangesetAttribute($key, $default = null) {
+    $map = $this->getUntrustedChangesetAttributes();
+    return idx($map, $key, $default);
+  }
+
+  public function getChangesetAttributes() {
+    // Prefer trusted values over untrusted values when both exist.
+    return
+      $this->getTrustedChangesetAttributes() +
+      $this->getUntrustedChangesetAttributes();
+  }
+
+  public function getChangesetAttribute($key, $default = null) {
+    $map = $this->getChangesetAttributes();
+    return idx($map, $key, $default);
+  }
+
+  public function isGeneratedChangeset() {
+    return $this->getChangesetAttribute(self::ATTRIBUTE_GENERATED);
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php
index cb392c4306..cd8364c8aa 100644
--- a/src/applications/differential/storage/DifferentialDiff.php
+++ b/src/applications/differential/storage/DifferentialDiff.php
@@ -226,6 +226,8 @@ final class DifferentialDiff
       $changeset->setAddLines($add_lines);
       $changeset->setDelLines($del_lines);
 
+      self::detectGeneratedCode($changeset);
+
       $diff->addUnsavedChangeset($changeset);
     }
     $diff->setLineCount($lines);
@@ -821,5 +823,48 @@ final class DifferentialDiff
     );
   }
 
+  private static function detectGeneratedCode(
+    DifferentialChangeset $changeset) {
+
+    $is_generated_trusted = self::isTrustedGeneratedCode($changeset);
+
+    $changeset->setTrustedChangesetAttribute(
+      DifferentialChangeset::ATTRIBUTE_GENERATED,
+      $is_generated_trusted);
+
+    $is_generated_untrusted = self::isUntrustedGeneratedCode($changeset);
+
+    $changeset->setUntrustedChangesetAttribute(
+      DifferentialChangeset::ATTRIBUTE_GENERATED,
+      $is_generated_untrusted);
+  }
+
+  private static function isTrustedGeneratedCode(
+    DifferentialChangeset $changeset) {
+
+    $filename = $changeset->getFilename();
+
+    $paths = PhabricatorEnv::getEnvConfig('differential.generated-paths');
+    foreach ($paths as $regexp) {
+      if (preg_match($regexp, $filename)) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  private static function isUntrustedGeneratedCode(
+    DifferentialChangeset $changeset) {
+
+    if ($changeset->getHunks()) {
+      $new_data = $changeset->makeNewFile();
+      if (strpos($new_data, '@'.'generated') !== false) {
+        return true;
+      }
+    }
+
+    return false;
+  }
 
 }

From 5e2af4b9b5cd87539609f1d5d00cb591904548e7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 May 2018 13:06:19 -0700
Subject: [PATCH 12/14] Prepare to support an "Ignore generated files" flag in
 Owners

Summary:
Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision.

There's no way to actualy configure a package to have this behavior yet.

Test Plan:
  - Created a revision affecting a generated file and a non-generated file.
    - When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it.
    - Normally: lots of packages owning it).
  - Created a revision affecting only generated files.
    - When I faked things, saw no Owners actions trigger.
    - Normally: some packages added reviewers or subscribers.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19426
---
 .../controller/DifferentialController.php     | 10 +++
 .../editor/DifferentialTransactionEditor.php  | 26 ++++--
 .../HeraldDifferentialRevisionAdapter.php     |  5 +-
 .../differential/storage/DifferentialDiff.php | 18 ++--
 .../storage/PhabricatorOwnersPackage.php      | 82 ++++++++++++++++++-
 5 files changed, 121 insertions(+), 20 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php
index 1860a07745..8fe5b5caca 100644
--- a/src/applications/differential/controller/DifferentialController.php
+++ b/src/applications/differential/controller/DifferentialController.php
@@ -76,6 +76,16 @@ abstract class DifferentialController extends PhabricatorController {
         $repository_phid,
         $changeset_path);
 
+      // If this particular changeset is generated code and the package does
+      // not match generated code, remove it from the list.
+      if ($changeset->isGeneratedChangeset()) {
+        foreach ($packages as $key => $package) {
+          if ($package->getMustMatchUngeneratedPaths()) {
+            unset($packages[$key]);
+          }
+        }
+      }
+
       $this->pathPackageMap[$changeset_path] = $packages;
       foreach ($packages as $package) {
         $this->packageChangesetMap[$package->getPHID()][] = $changeset;
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 82740459b0..3271248ce8 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -7,11 +7,13 @@ final class DifferentialTransactionEditor
   private $isCloseByCommit;
   private $repositoryPHIDOverride = false;
   private $didExpandInlineState = false;
-  private $affectedPaths;
   private $firstBroadcast = false;
   private $wasBroadcasting;
   private $isDraftDemotion;
 
+  private $ownersDiff;
+  private $ownersChangesets;
+
   public function getEditorApplicationClass() {
     return 'PhabricatorDifferentialApplication';
   }
@@ -968,13 +970,20 @@ final class DifferentialTransactionEditor
       return array();
     }
 
-    if (!$this->affectedPaths) {
+    $diff = $this->ownersDiff;
+    $changesets = $this->ownersChangesets;
+
+    $this->ownersDiff = null;
+    $this->ownersChangesets = null;
+
+    if (!$changesets) {
       return array();
     }
 
-    $packages = PhabricatorOwnersPackage::loadAffectedPackages(
+    $packages = PhabricatorOwnersPackage::loadAffectedPackagesForChangesets(
       $repository,
-      $this->affectedPaths);
+      $diff,
+      $changesets);
     if (!$packages) {
       return array();
     }
@@ -1255,9 +1264,12 @@ final class DifferentialTransactionEditor
       $paths[] = $path_prefix.'/'.$changeset->getFilename();
     }
 
-    // Save the affected paths; we'll use them later to query Owners. This
-    // uses the un-expanded paths.
-    $this->affectedPaths = $paths;
+    // If this change affected paths, save the changesets so we can apply
+    // Owners rules to them later.
+    if ($paths) {
+      $this->ownersDiff = $diff;
+      $this->ownersChangesets = $changesets;
+    }
 
     // Mark this as also touching all parent paths, so you can see all pending
     // changes to any file within a directory.
diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php
index 9f7f8fe3f2..858a42dbc9 100644
--- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php
+++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php
@@ -120,9 +120,10 @@ final class HeraldDifferentialRevisionAdapter
 
       $repository = $this->loadRepository();
       if ($repository) {
-        $packages = PhabricatorOwnersPackage::loadAffectedPackages(
+        $packages = PhabricatorOwnersPackage::loadAffectedPackagesForChangesets(
           $repository,
-          $this->loadAffectedPaths());
+          $this->getDiff(),
+          $this->loadChangesets());
         $this->affectedPackages = $packages;
       }
     }
diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php
index cd8364c8aa..f121a1cb67 100644
--- a/src/applications/differential/storage/DifferentialDiff.php
+++ b/src/applications/differential/storage/DifferentialDiff.php
@@ -827,16 +827,18 @@ final class DifferentialDiff
     DifferentialChangeset $changeset) {
 
     $is_generated_trusted = self::isTrustedGeneratedCode($changeset);
-
-    $changeset->setTrustedChangesetAttribute(
-      DifferentialChangeset::ATTRIBUTE_GENERATED,
-      $is_generated_trusted);
+    if ($is_generated_trusted) {
+      $changeset->setTrustedChangesetAttribute(
+        DifferentialChangeset::ATTRIBUTE_GENERATED,
+        $is_generated_trusted);
+    }
 
     $is_generated_untrusted = self::isUntrustedGeneratedCode($changeset);
-
-    $changeset->setUntrustedChangesetAttribute(
-      DifferentialChangeset::ATTRIBUTE_GENERATED,
-      $is_generated_untrusted);
+    if ($is_generated_untrusted) {
+      $changeset->setUntrustedChangesetAttribute(
+        DifferentialChangeset::ATTRIBUTE_GENERATED,
+        $is_generated_untrusted);
+    }
   }
 
   private static function isTrustedGeneratedCode(
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index c76864702c..5e856190ec 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -147,9 +147,9 @@ final class PhabricatorOwnersPackage
     return ($this->getStatus() == self::STATUS_ARCHIVED);
   }
 
-  public function setName($name) {
-    $this->name = $name;
-    return $this;
+  public function getMustMatchUngeneratedPaths() {
+    // TODO: For now, there's no way to actually configure this.
+    return false;
   }
 
   public function loadOwners() {
@@ -181,6 +181,82 @@ final class PhabricatorOwnersPackage
     return self::loadPackagesForPaths($repository, $paths);
   }
 
+  public static function loadAffectedPackagesForChangesets(
+    PhabricatorRepository $repository,
+    DifferentialDiff $diff,
+    array $changesets) {
+    assert_instances_of($changesets, 'DifferentialChangeset');
+
+    $paths_all = array();
+    $paths_ungenerated = array();
+
+    foreach ($changesets as $changeset) {
+      $path = $changeset->getAbsoluteRepositoryPath($repository, $diff);
+
+      $paths_all[] = $path;
+
+      if (!$changeset->isGeneratedChangeset()) {
+        $paths_ungenerated[] = $path;
+      }
+    }
+
+    if (!$paths_all) {
+      return array();
+    }
+
+    $packages_all = self::loadAffectedPackages(
+      $repository,
+      $paths_all);
+
+    // If there are no generated changesets, we can't possibly need to throw
+    // away any packages for matching only generated paths. Just return the
+    // full set of packages.
+    if ($paths_ungenerated === $paths_all) {
+      return $packages_all;
+    }
+
+    $must_match_ungenerated = array();
+    foreach ($packages_all as $package) {
+      if ($package->getMustMatchUngeneratedPaths()) {
+        $must_match_ungenerated[] = $package;
+      }
+    }
+
+    // If no affected packages have the "Ignore Generated Paths" flag set, we
+    // can't possibly need to throw any away.
+    if (!$must_match_ungenerated) {
+      return $packages_all;
+    }
+
+    if ($paths_ungenerated) {
+      $packages_ungenerated = self::loadAffectedPackages(
+        $repository,
+        $paths_ungenerated);
+    } else {
+      $packages_ungenerated = array();
+    }
+
+    // We have some generated paths, and some packages that ignore generated
+    // paths. Take all the packages which:
+    //
+    //   - ignore generated paths; and
+    //   - didn't match any ungenerated paths
+    //
+    // ...and remove them from the list.
+
+    $must_match_ungenerated = mpull($must_match_ungenerated, null, 'getID');
+    $packages_ungenerated = mpull($packages_ungenerated, null, 'getID');
+    $packages_all = mpull($packages_all, null, 'getID');
+
+    foreach ($must_match_ungenerated as $package_id => $package) {
+      if (!isset($packages_ungenerated[$package_id])) {
+        unset($packages_all[$package_id]);
+      }
+    }
+
+    return $packages_all;
+  }
+
   public static function loadOwningPackages($repository, $path) {
     if (empty($path)) {
       return array();

From dc510354c339209c4da1ee25eea126b1881c8b64 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 May 2018 13:41:03 -0700
Subject: [PATCH 13/14] Remove explicit "mailKey" from Owners packages

Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.

We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.

Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130, T13065

Differential Revision: https://secure.phabricator.com/D19427
---
 .../20180504.owners.01.mailkey.php            | 26 +++++++++++++++++++
 .../autopatches/20180504.owners.02.rmkey.sql  |  2 ++
 .../storage/PhabricatorOwnersPackage.php      | 15 ++---------
 3 files changed, 30 insertions(+), 13 deletions(-)
 create mode 100644 resources/sql/autopatches/20180504.owners.01.mailkey.php
 create mode 100644 resources/sql/autopatches/20180504.owners.02.rmkey.sql

diff --git a/resources/sql/autopatches/20180504.owners.01.mailkey.php b/resources/sql/autopatches/20180504.owners.01.mailkey.php
new file mode 100644
index 0000000000..c1b5550f9f
--- /dev/null
+++ b/resources/sql/autopatches/20180504.owners.01.mailkey.php
@@ -0,0 +1,26 @@
+<?php
+
+$packages_table = new PhabricatorOwnersPackage();
+$packages_conn = $packages_table->establishConnection('w');
+$packages_name = $packages_table->getTableName();
+
+$properties_table = new PhabricatorMetaMTAMailProperties();
+$conn = $properties_table->establishConnection('w');
+
+$iterator = new LiskRawMigrationIterator($packages_conn, $packages_name);
+foreach ($iterator as $package) {
+  queryfx(
+    $conn,
+    'INSERT IGNORE INTO %T
+        (objectPHID, mailProperties, dateCreated, dateModified)
+      VALUES
+        (%s, %s, %d, %d)',
+    $properties_table->getTableName(),
+    $package['phid'],
+    phutil_json_encode(
+      array(
+        'mailKey' => $package['mailKey'],
+      )),
+    PhabricatorTime::getNow(),
+    PhabricatorTime::getNow());
+}
diff --git a/resources/sql/autopatches/20180504.owners.02.rmkey.sql b/resources/sql/autopatches/20180504.owners.02.rmkey.sql
new file mode 100644
index 0000000000..5b8f240307
--- /dev/null
+++ b/resources/sql/autopatches/20180504.owners.02.rmkey.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  DROP mailKey;
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 5e856190ec..37ded17f1b 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -16,7 +16,6 @@ final class PhabricatorOwnersPackage
   protected $auditingEnabled;
   protected $autoReview;
   protected $description;
-  protected $mailKey;
   protected $status;
   protected $viewPolicy;
   protected $editPolicy;
@@ -122,7 +121,6 @@ final class PhabricatorOwnersPackage
         'name' => 'sort',
         'description' => 'text',
         'auditingEnabled' => 'bool',
-        'mailKey' => 'bytes20',
         'status' => 'text32',
         'autoReview' => 'text32',
         'dominion' => 'text32',
@@ -130,17 +128,8 @@ final class PhabricatorOwnersPackage
     ) + parent::getConfiguration();
   }
 
-  public function generatePHID() {
-    return PhabricatorPHID::generateNewPHID(
-      PhabricatorOwnersPackagePHIDType::TYPECONST);
-  }
-
-  public function save() {
-    if (!$this->getMailKey()) {
-      $this->setMailKey(Filesystem::readRandomCharacters(20));
-    }
-
-    return parent::save();
+  public function getPHIDType() {
+    return PhabricatorOwnersPackagePHIDType::TYPECONST;
   }
 
   public function isArchived() {

From 4a98e0ff65a44a208920f553a935f734726e93b2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 May 2018 14:36:07 -0700
Subject: [PATCH 14/14] Allow Owners packages to be configured to ignore
 generated paths in Differential

Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.

This is still a little rough. A couple limitations:

  - It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
  - The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
  - No documentation yet (which could mitigate the other two issues a bit).

But the actual behavior seems to work fine.

Test Plan:
  - Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
  - Removed the attribute from the ignore list.
  - Tried to set invalid attributes, got sensible errors.
  - Queried a package with Conduit, got the ignored attribute list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19428
---
 .../20180504.owners.03.properties.sql         |  2 +
 .../20180504.owners.04.default.sql            |  2 +
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorOwnersDetailController.php     | 10 +++
 .../PhabricatorOwnersPackageEditEngine.php    |  7 ++
 .../storage/PhabricatorOwnersPackage.php      | 39 ++++++++-
 ...ricatorOwnersPackageIgnoredTransaction.php | 85 +++++++++++++++++++
 7 files changed, 145 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20180504.owners.03.properties.sql
 create mode 100644 resources/sql/autopatches/20180504.owners.04.default.sql
 create mode 100644 src/applications/owners/xaction/PhabricatorOwnersPackageIgnoredTransaction.php

diff --git a/resources/sql/autopatches/20180504.owners.03.properties.sql b/resources/sql/autopatches/20180504.owners.03.properties.sql
new file mode 100644
index 0000000000..d7a90ed1c5
--- /dev/null
+++ b/resources/sql/autopatches/20180504.owners.03.properties.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  ADD properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT};
diff --git a/resources/sql/autopatches/20180504.owners.04.default.sql b/resources/sql/autopatches/20180504.owners.04.default.sql
new file mode 100644
index 0000000000..c4c7ff044d
--- /dev/null
+++ b/resources/sql/autopatches/20180504.owners.04.default.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_owners.owners_package
+  SET properties = '{}' WHERE properties = '';
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index c21315f45e..78ae048715 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3572,6 +3572,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnersPackageFerretEngine' => 'applications/owners/search/PhabricatorOwnersPackageFerretEngine.php',
     'PhabricatorOwnersPackageFulltextEngine' => 'applications/owners/search/PhabricatorOwnersPackageFulltextEngine.php',
     'PhabricatorOwnersPackageFunctionDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php',
+    'PhabricatorOwnersPackageIgnoredTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageIgnoredTransaction.php',
     'PhabricatorOwnersPackageNameNgrams' => 'applications/owners/storage/PhabricatorOwnersPackageNameNgrams.php',
     'PhabricatorOwnersPackageNameTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageNameTransaction.php',
     'PhabricatorOwnersPackageOwnerDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php',
@@ -9321,6 +9322,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnersPackageFerretEngine' => 'PhabricatorFerretEngine',
     'PhabricatorOwnersPackageFulltextEngine' => 'PhabricatorFulltextEngine',
     'PhabricatorOwnersPackageFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
+    'PhabricatorOwnersPackageIgnoredTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageNameNgrams' => 'PhabricatorSearchNgrams',
     'PhabricatorOwnersPackageNameTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageOwnerDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index c7e96594f3..1caf12a82d 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -201,6 +201,16 @@ final class PhabricatorOwnersDetailController
     }
     $view->addProperty(pht('Auditing'), $auditing);
 
+    $ignored = $package->getIgnoredPathAttributes();
+    $ignored = array_keys($ignored);
+    if ($ignored) {
+      $ignored = implode(', ', $ignored);
+    } else {
+      $ignored = phutil_tag('em', array(), pht('None'));
+    }
+
+    $view->addProperty(pht('Ignored Attributes'), $ignored);
+
     $description = $package->getDescription();
     if (strlen($description)) {
       $description = new PHUIRemarkupView($viewer, $description);
diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
index 4adaebc582..8c48727a0b 100644
--- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
@@ -162,6 +162,13 @@ EOTEXT
         ->setIsConduitOnly(true)
         ->setValue($object->getStatus())
         ->setOptions($object->getStatusNameMap()),
+      id(new PhabricatorStringListEditField())
+        ->setKey('ignored')
+        ->setLabel(pht('Ignored Attributes'))
+        ->setDescription(pht('Ignore paths with any of these attributes.'))
+        ->setTransactionType(
+          PhabricatorOwnersPackageIgnoredTransaction::TRANSACTIONTYPE)
+        ->setValue(array_keys($object->getIgnoredPathAttributes())),
       id(new PhabricatorConduitEditField())
         ->setKey('paths.set')
         ->setLabel(pht('Paths'))
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 37ded17f1b..7e2a348c89 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -20,6 +20,7 @@ final class PhabricatorOwnersPackage
   protected $viewPolicy;
   protected $editPolicy;
   protected $dominion;
+  protected $properties = array();
 
   private $paths = self::ATTACHABLE;
   private $owners = self::ATTACHABLE;
@@ -40,6 +41,8 @@ final class PhabricatorOwnersPackage
   const DOMINION_STRONG = 'strong';
   const DOMINION_WEAK = 'weak';
 
+  const PROPERTY_IGNORED = 'ignored';
+
   public static function initializeNewPackage(PhabricatorUser $actor) {
     $app = id(new PhabricatorApplicationQuery())
       ->setViewer($actor)
@@ -117,6 +120,9 @@ final class PhabricatorOwnersPackage
       // This information is better available from the history table.
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_AUX_PHID => true,
+      self::CONFIG_SERIALIZATION => array(
+        'properties' => self::SERIALIZATION_JSON,
+      ),
       self::CONFIG_COLUMN_SCHEMA => array(
         'name' => 'sort',
         'description' => 'text',
@@ -137,8 +143,25 @@ final class PhabricatorOwnersPackage
   }
 
   public function getMustMatchUngeneratedPaths() {
-    // TODO: For now, there's no way to actually configure this.
-    return false;
+    $ignore_attributes = $this->getIgnoredPathAttributes();
+    return !empty($ignore_attributes['generated']);
+  }
+
+  public function getPackageProperty($key, $default = null) {
+    return idx($this->properties, $key, $default);
+  }
+
+  public function setPackageProperty($key, $value) {
+    $this->properties[$key] = $value;
+    return $this;
+  }
+
+  public function getIgnoredPathAttributes() {
+    return $this->getPackageProperty(self::PROPERTY_IGNORED, array());
+  }
+
+  public function setIgnoredPathAttributes(array $attributes) {
+    return $this->setPackageProperty(self::PROPERTY_IGNORED, $attributes);
   }
 
   public function loadOwners() {
@@ -679,6 +702,10 @@ final class PhabricatorOwnersPackage
         ->setKey('dominion')
         ->setType('map<string, wild>')
         ->setDescription(pht('Dominion setting information.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('ignored')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Ignored attribute information.')),
     );
   }
 
@@ -732,6 +759,13 @@ final class PhabricatorOwnersPackage
       'short' => $dominion_short,
     );
 
+    // Force this to always emit as a JSON object even if empty, never as
+    // a JSON list.
+    $ignored = $this->getIgnoredPathAttributes();
+    if (!$ignored) {
+      $ignored = (object)array();
+    }
+
     return array(
       'name' => $this->getName(),
       'description' => $this->getDescription(),
@@ -740,6 +774,7 @@ final class PhabricatorOwnersPackage
       'review' => $review,
       'audit' => $audit,
       'dominion' => $dominion,
+      'ignored' => $ignored,
     );
   }
 
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackageIgnoredTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackageIgnoredTransaction.php
new file mode 100644
index 0000000000..b3ad6a58f8
--- /dev/null
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackageIgnoredTransaction.php
@@ -0,0 +1,85 @@
+<?php
+
+final class PhabricatorOwnersPackageIgnoredTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.ignored';
+
+  public function generateOldValue($object) {
+    return $object->getIgnoredPathAttributes();
+  }
+
+  public function generateNewValue($object, $value) {
+    return array_fill_keys($value, true);
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setIgnoredPathAttributes($value);
+  }
+
+  public function getTitle() {
+    $old = array_keys($this->getOldValue());
+    $new = array_keys($this->getNewValue());
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+
+    $all_n = new PhutilNumber(count($add) + count($rem));
+    $add_n = phutil_count($add);
+    $rem_n = phutil_count($rem);
+
+    if ($new && $old) {
+      return pht(
+        '%s changed %s ignored attribute(s), added %s: %s; removed %s: %s.',
+        $this->renderAuthor(),
+        $all_n,
+        $add_n,
+        $this->renderValueList($add),
+        $rem_n,
+        $this->renderValueList($rem));
+    } else if ($new) {
+      return pht(
+        '%s changed %s ignored attribute(s), added %s: %s.',
+        $this->renderAuthor(),
+        $all_n,
+        $add_n,
+        $this->rendervalueList($add));
+    } else {
+      return pht(
+        '%s changed %s ignored attribute(s), removed %s: %s.',
+        $this->renderAuthor(),
+        $all_n,
+        $rem_n,
+        $this->rendervalueList($rem));
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $valid_attributes = array(
+      'generated' => true,
+    );
+
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+
+      foreach ($new as $attribute) {
+        if (isset($valid_attributes[$attribute])) {
+          continue;
+        }
+
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Changeset attribute "%s" is not valid. Valid changeset '.
+            'attributes are: %s.',
+            $attribute,
+            implode(', ', array_keys($valid_attributes))),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+}