Policy - move some owners code into an editor class and check policy better

Summary: Ref T7094. We basically need to make sure folks can see repositories before making owners packages about code within. This cleans up things a little bit by moving a bunch of logic out of the storage class and into an editor class.

Test Plan: made a package and it worked! deleted a package and it worked! discovered buggy behavior in more complicated edits and filed T7127; note this bug exists before and after this diff.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11652

src/__phutil_library_map__.php

@@ -2071,6 +2071,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnersOwner' => 'applications/owners/storage/PhabricatorOwnersOwner.php',
     'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php',
     'PhabricatorOwnersPackageDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageDatasource.php',
+    'PhabricatorOwnersPackageEditor' => 'applications/owners/editor/PhabricatorOwnersPackageEditor.php',
     'PhabricatorOwnersPackagePHIDType' => 'applications/owners/phid/PhabricatorOwnersPackagePHIDType.php',
     'PhabricatorOwnersPackagePathValidator' => 'applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php',
     'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php',
@@ -5315,6 +5316,7 @@ phutil_register_library_map(array(
       'PhabricatorPolicyInterface',
     ),
     'PhabricatorOwnersPackageDatasource' => 'PhabricatorTypeaheadDatasource',
+    'PhabricatorOwnersPackageEditor' => 'PhabricatorEditor',
     'PhabricatorOwnersPackagePHIDType' => 'PhabricatorPHIDType',
     'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorOwnersPackageTestCase' => 'PhabricatorTestCase',

src/applications/owners/controller/PhabricatorOwnersDeleteController.php

@@ -19,8 +19,10 @@ final class PhabricatorOwnersDeleteController
     }
 
     if ($request->isDialogFormPost()) {
-      $package->attachActorPHID($user->getPHID());
-      $package->delete();
+      id(new PhabricatorOwnersPackageEditor())
+        ->setActor($user)
+        ->setPackage($package)
+        ->delete();
       return id(new AphrontRedirectResponse())->setURI('/owners/');
     }
 

src/applications/owners/controller/PhabricatorOwnersEditController.php

@@ -87,9 +87,11 @@ final class PhabricatorOwnersEditController
         $package->attachUnsavedPaths($path_refs);
         $package->attachOldAuditingEnabled($old_auditing_enabled);
         $package->attachOldPrimaryOwnerPHID($old_primary);
-        $package->attachActorPHID($user->getPHID());
         try {
-          $package->save();
+          id(new PhabricatorOwnersPackageEditor())
+            ->setActor($user)
+            ->setPackage($package)
+            ->save();
           return id(new AphrontRedirectResponse())
             ->setURI('/owners/package/'.$package->getID().'/');
         } catch (AphrontDuplicateKeyQueryException $ex) {

src/applications/owners/editor/PhabricatorOwnersPackageEditor.php

@@ -0,0 +1,198 @@
+<?php
+
+final class PhabricatorOwnersPackageEditor extends PhabricatorEditor {
+
+  private $package;
+
+  public function setPackage(PhabricatorOwnersPackage $package) {
+    $this->package = $package;
+    return $this;
+  }
+
+  public function getPackage() {
+    return $this->package;
+  }
+
+  public function save() {
+    $actor = $this->getActor();
+    $package = $this->getPackage();
+    $package->attachActorPHID($actor->getPHID());
+
+    if ($package->getID()) {
+      $is_new = false;
+    } else {
+      $is_new = true;
+    }
+
+    $package->openTransaction();
+
+      $ret = $package->save();
+
+      $add_owners = array();
+      $remove_owners = array();
+      $all_owners = array();
+      if ($package->getUnsavedOwners()) {
+        $new_owners = array_fill_keys($package->getUnsavedOwners(), true);
+        $cur_owners = array();
+        foreach ($package->loadOwners() as $owner) {
+          if (empty($new_owners[$owner->getUserPHID()])) {
+            $remove_owners[$owner->getUserPHID()] = true;
+            $owner->delete();
+            continue;
+          }
+          $cur_owners[$owner->getUserPHID()] = true;
+        }
+
+        $add_owners = array_diff_key($new_owners, $cur_owners);
+        $all_owners = array_merge(
+          array($package->getPrimaryOwnerPHID() => true),
+          $new_owners,
+          $remove_owners);
+        foreach ($add_owners as $phid => $ignored) {
+          $owner = new PhabricatorOwnersOwner();
+          $owner->setPackageID($package->getID());
+          $owner->setUserPHID($phid);
+          $owner->save();
+        }
+        $package->attachUnsavedOwners(array());
+      }
+
+      $add_paths = array();
+      $remove_paths = array();
+      $touched_repos = array();
+      if ($package->getUnsavedPaths()) {
+        $new_paths = igroup(
+          $package->getUnsavedPaths(),
+          'repositoryPHID',
+          'path');
+        $cur_paths = $package->loadPaths();
+        foreach ($cur_paths as $key => $path) {
+          $repository_phid = $path->getRepositoryPHID();
+          $new_path = head(idx(
+            idx($new_paths, $repository_phid, array()),
+            $path->getPath(),
+            array()));
+          $excluded = $path->getExcluded();
+          if ($new_path === false ||
+              idx($new_path, 'excluded') != $excluded) {
+            $touched_repos[$repository_phid] = true;
+            $remove_paths[$repository_phid][$path->getPath()] = $excluded;
+            $path->delete();
+            unset($cur_paths[$key]);
+          }
+        }
+
+        $cur_paths = mgroup($cur_paths, 'getRepositoryPHID', 'getPath');
+        $repositories = id(new PhabricatorRepositoryQuery())
+          ->setViewer($actor)
+          ->withPHIDs(array_keys($cur_paths))
+          ->execute();
+        $repositories = mpull($repositories, null, 'getPHID');
+        foreach ($new_paths as $repository_phid => $paths) {
+          $repository = idx($repositories, $repository_phid);
+          if (!$repository) {
+            continue;
+          }
+          foreach ($paths as $path => $dicts) {
+            $path = ltrim($path, '/');
+            // build query to validate path
+            $drequest = DiffusionRequest::newFromDictionary(
+              array(
+                'user' => $actor,
+                'repository'  => $repository,
+                'path'        => $path,
+              ));
+            $results = DiffusionBrowseResultSet::newFromConduit(
+              DiffusionQuery::callConduitWithDiffusionRequest(
+                $actor,
+                $drequest,
+                'diffusion.browsequery',
+                array(
+                  'commit' => $drequest->getCommit(),
+                  'path' => $path,
+                  'needValidityOnly' => true,
+                )));
+            $valid = $results->isValidResults();
+            $is_directory = true;
+            if (!$valid) {
+              switch ($results->getReasonForEmptyResultSet()) {
+                case DiffusionBrowseResultSet::REASON_IS_FILE:
+                  $valid = true;
+                  $is_directory = false;
+                  break;
+                case DiffusionBrowseResultSet::REASON_IS_EMPTY:
+                  $valid = true;
+                  break;
+              }
+            }
+            if ($is_directory && substr($path, -1) != '/') {
+              $path .= '/';
+            }
+            if (substr($path, 0, 1) != '/') {
+              $path = '/'.$path;
+            }
+            if (empty($cur_paths[$repository_phid][$path]) && $valid) {
+              $touched_repos[$repository_phid] = true;
+              $excluded = idx(reset($dicts), 'excluded', 0);
+              $add_paths[$repository_phid][$path] = $excluded;
+              $obj = new PhabricatorOwnersPath();
+              $obj->setPackageID($package->getID());
+              $obj->setRepositoryPHID($repository_phid);
+              $obj->setPath($path);
+              $obj->setExcluded($excluded);
+              $obj->save();
+            }
+          }
+        }
+        $package->attachUnsavedPaths(array());
+      }
+
+    $package->saveTransaction();
+
+    if ($is_new) {
+      $mail = new PackageCreateMail($package);
+    } else {
+      $mail = new PackageModifyMail(
+        $package,
+        array_keys($add_owners),
+        array_keys($remove_owners),
+        array_keys($all_owners),
+        array_keys($touched_repos),
+        $add_paths,
+        $remove_paths);
+    }
+    $mail->setActor($actor);
+    $mail->send();
+
+    return $ret;
+  }
+
+  public function delete() {
+    $actor = $this->getActor();
+    $package = $this->getPackage();
+    $package->attachActorPHID($actor->getPHID());
+
+    $mails = id(new PackageDeleteMail($package))
+      ->setActor($actor)
+      ->prepareMails();
+
+    $package->openTransaction();
+
+      foreach ($package->loadOwners() as $owner) {
+        $owner->delete();
+      }
+      foreach ($package->loadPaths() as $path) {
+        $path->delete();
+      }
+      $ret = $package->delete();
+
+    $package->saveTransaction();
+
+    foreach ($mails as $mail) {
+      $mail->saveAndSend();
+    }
+
+    return $ret;
+  }
+
+}

src/applications/owners/storage/PhabricatorOwnersPackage.php

@@ -9,8 +9,8 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO
   protected $description;
   protected $primaryOwnerPHID;
 
-  private $unsavedOwners;
-  private $unsavedPaths;
+  private $unsavedOwners = self::ATTACHABLE;
+  private $unsavedPaths = self::ATTACHABLE;
   private $actorPHID;
   private $oldPrimaryOwnerPHID;
   private $oldAuditingEnabled;
@@ -68,11 +68,19 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO
     return $this;
   }
 
+  public function getUnsavedOwners() {
+    return $this->assertAttached($this->unsavedOwners);
+  }
+
   public function attachUnsavedPaths(array $paths) {
     $this->unsavedPaths = $paths;
     return $this;
   }
 
+  public function getUnsavedPaths() {
+    return $this->assertAttached($this->unsavedPaths);
+  }
+
   public function attachActorPHID($actor_phid) {
     $this->actorPHID = $actor_phid;
     return $this;
@@ -248,183 +256,6 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO
     return $ids;
   }
 
-  private function getActor() {
-    // TODO: This should be cleaner, but we'd likely need to move the whole
-    // thing to an Editor (T603).
-    return PhabricatorUser::getOmnipotentUser();
-  }
-
-  public function save() {
-
-    if ($this->getID()) {
-      $is_new = false;
-    } else {
-      $is_new = true;
-    }
-
-    $this->openTransaction();
-
-    $ret = parent::save();
-
-    $add_owners = array();
-    $remove_owners = array();
-    $all_owners = array();
-    if ($this->unsavedOwners) {
-      $new_owners = array_fill_keys($this->unsavedOwners, true);
-      $cur_owners = array();
-      foreach ($this->loadOwners() as $owner) {
-        if (empty($new_owners[$owner->getUserPHID()])) {
-          $remove_owners[$owner->getUserPHID()] = true;
-          $owner->delete();
-          continue;
-        }
-        $cur_owners[$owner->getUserPHID()] = true;
-      }
-
-      $add_owners = array_diff_key($new_owners, $cur_owners);
-      $all_owners = array_merge(
-        array($this->getPrimaryOwnerPHID() => true),
-        $new_owners,
-        $remove_owners);
-      foreach ($add_owners as $phid => $ignored) {
-        $owner = new PhabricatorOwnersOwner();
-        $owner->setPackageID($this->getID());
-        $owner->setUserPHID($phid);
-        $owner->save();
-      }
-      unset($this->unsavedOwners);
-    }
-
-    $add_paths = array();
-    $remove_paths = array();
-    $touched_repos = array();
-    if ($this->unsavedPaths) {
-      $new_paths = igroup($this->unsavedPaths, 'repositoryPHID', 'path');
-      $cur_paths = $this->loadPaths();
-      foreach ($cur_paths as $key => $path) {
-        $repository_phid = $path->getRepositoryPHID();
-        $new_path = head(idx(
-          idx($new_paths, $repository_phid, array()),
-          $path->getPath(),
-          array()));
-        $excluded = $path->getExcluded();
-        if (!$new_path || idx($new_path, 'excluded') != $excluded) {
-          $touched_repos[$repository_phid] = true;
-          $remove_paths[$repository_phid][$path->getPath()] = $excluded;
-          $path->delete();
-          unset($cur_paths[$key]);
-        }
-      }
-
-      $cur_paths = mgroup($cur_paths, 'getRepositoryPHID', 'getPath');
-      foreach ($new_paths as $repository_phid => $paths) {
-        // TODO: (T603) Thread policy stuff in here.
-
-        // get repository object for path validation
-        $repository = id(new PhabricatorRepository())->loadOneWhere(
-          'phid = %s',
-          $repository_phid);
-        if (!$repository) {
-          continue;
-        }
-        foreach ($paths as $path => $dicts) {
-          $path = ltrim($path, '/');
-          // build query to validate path
-          $drequest = DiffusionRequest::newFromDictionary(
-            array(
-              'user' => $this->getActor(),
-              'repository'  => $repository,
-              'path'        => $path,
-            ));
-          $results = DiffusionBrowseResultSet::newFromConduit(
-            DiffusionQuery::callConduitWithDiffusionRequest(
-              $this->getActor(),
-              $drequest,
-              'diffusion.browsequery',
-              array(
-                'commit' => $drequest->getCommit(),
-                'path' => $path,
-                'needValidityOnly' => true,
-              )));
-          $valid = $results->isValidResults();
-          $is_directory = true;
-          if (!$valid) {
-            switch ($results->getReasonForEmptyResultSet()) {
-              case DiffusionBrowseResultSet::REASON_IS_FILE:
-                $valid = true;
-                $is_directory = false;
-                break;
-              case DiffusionBrowseResultSet::REASON_IS_EMPTY:
-                $valid = true;
-                break;
-            }
-          }
-          if ($is_directory && substr($path, -1) != '/') {
-            $path .= '/';
-          }
-          if (substr($path, 0, 1) != '/') {
-            $path = '/'.$path;
-          }
-          if (empty($cur_paths[$repository_phid][$path]) && $valid) {
-            $touched_repos[$repository_phid] = true;
-            $excluded = idx(reset($dicts), 'excluded', 0);
-            $add_paths[$repository_phid][$path] = $excluded;
-            $obj = new PhabricatorOwnersPath();
-            $obj->setPackageID($this->getID());
-            $obj->setRepositoryPHID($repository_phid);
-            $obj->setPath($path);
-            $obj->setExcluded($excluded);
-            $obj->save();
-          }
-        }
-      }
-      unset($this->unsavedPaths);
-    }
-
-    $this->saveTransaction();
-
-    if ($is_new) {
-      $mail = new PackageCreateMail($this);
-    } else {
-      $mail = new PackageModifyMail(
-        $this,
-        array_keys($add_owners),
-        array_keys($remove_owners),
-        array_keys($all_owners),
-        array_keys($touched_repos),
-        $add_paths,
-        $remove_paths);
-    }
-    $mail->setActor($this->getActor());
-    $mail->send();
-
-    return $ret;
-  }
-
-  public function delete() {
-    $mails = id(new PackageDeleteMail($this))
-      ->setActor($this->getActor())
-      ->prepareMails();
-
-    $this->openTransaction();
-    foreach ($this->loadOwners() as $owner) {
-      $owner->delete();
-    }
-    foreach ($this->loadPaths() as $path) {
-      $path->delete();
-    }
-
-    $ret = parent::delete();
-
-    $this->saveTransaction();
-
-    foreach ($mails as $mail) {
-      $mail->saveAndSend();
-    }
-
-    return $ret;
-  }
-
   private static function splitPath($path) {
     $result = array('/');
     $trailing_slash = preg_match('@/$@', $path) ? '/' : '';