From 3b92da22f4333d5bfa050d0da4397648147cee09 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 14 Aug 2018 14:29:19 -0700
Subject: [PATCH] Move the hierarchical edit policy check in Phriction from
 requireCapabilities() to validateTransactions()

Summary:
Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`.

This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects).

The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object).

Test Plan:
  - Created a child as a user with permission to do so to make sure I didn't break that.
  - Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19584
---
 .../editor/PhrictionTransactionEditor.php     | 52 -------------------
 .../PhrictionDocumentTitleTransaction.php     | 23 ++++++++
 2 files changed, 23 insertions(+), 52 deletions(-)

diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php
index e6bf46c150..b9f2c0e2ca 100644
--- a/src/applications/phriction/editor/PhrictionTransactionEditor.php
+++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php
@@ -516,58 +516,6 @@ final class PhrictionTransactionEditor
     }
     return $error;
   }
-  protected function requireCapabilities(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    /*
-     * New objects have a special case. If a user can't see
-     *   x/y
-     * then definitely don't let them make some
-     *   x/y/z
-     * We need to load the direct parent to handle this case.
-     */
-    if ($this->getIsNewObject()) {
-      $actor = $this->requireActor();
-      $parent_doc = null;
-      $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug());
-      // No ancestral slugs is "/"; the first person gets to play with "/".
-      if ($ancestral_slugs) {
-        $parent = end($ancestral_slugs);
-        $parent_doc = id(new PhrictionDocumentQuery())
-          ->setViewer($actor)
-          ->withSlugs(array($parent))
-          ->executeOne();
-        // If the $actor can't see the $parent_doc then they can't create
-        // the child $object; throw a policy exception.
-        if (!$parent_doc) {
-          id(new PhabricatorPolicyFilter())
-            ->setViewer($actor)
-            ->raisePolicyExceptions(true)
-            ->rejectObject(
-              $object,
-              $object->getEditPolicy(),
-              PhabricatorPolicyCapability::CAN_EDIT);
-        }
-
-        // If the $actor can't edit the $parent_doc then they can't create
-        // the child $object; throw a policy exception.
-        if (!PhabricatorPolicyFilter::hasCapability(
-          $actor,
-          $parent_doc,
-          PhabricatorPolicyCapability::CAN_EDIT)) {
-          id(new PhabricatorPolicyFilter())
-            ->setViewer($actor)
-            ->raisePolicyExceptions(true)
-            ->rejectObject(
-              $object,
-              $object->getEditPolicy(),
-              PhabricatorPolicyCapability::CAN_EDIT);
-        }
-      }
-    }
-    return parent::requireCapabilities($object, $xaction);
-  }
 
   protected function supportsSearch() {
     return true;
diff --git a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php
index 4f1ba850a7..5c97d1bec8 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php
@@ -91,6 +91,29 @@ final class PhrictionDocumentTitleTransaction
         pht('Documents must have a title.'));
     }
 
+    if ($this->isNewObject()) {
+      // No ancestral slugs is "/". No ancestry checks apply when creating the
+      // root document.
+      $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug());
+      if ($ancestral_slugs) {
+        // You must be able to view and edit the parent document to create a new
+        // child.
+        $parent_document = id(new PhrictionDocumentQuery())
+          ->setViewer($this->getActor())
+          ->withSlugs(array(last($ancestral_slugs)))
+          ->requireCapabilities(
+            array(
+              PhabricatorPolicyCapability::CAN_VIEW,
+              PhabricatorPolicyCapability::CAN_EDIT,
+            ))
+          ->executeOne();
+        if (!$parent_document) {
+          $errors[] = $this->newInvalidError(
+            pht('You can not create a document which does not have a parent.'));
+        }
+      }
+    }
+
     return $errors;
   }