From 9b65370398ccc11163710f90eeace2398a2a3cb4 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 3 Feb 2015 11:41:15 -0800 Subject: [PATCH] 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 | 2 + .../PhabricatorOwnersDeleteController.php | 6 +- .../PhabricatorOwnersEditController.php | 6 +- .../editor/PhabricatorOwnersPackageEditor.php | 198 ++++++++++++++++++ .../storage/PhabricatorOwnersPackage.php | 189 +---------------- 5 files changed, 218 insertions(+), 183 deletions(-) create mode 100644 src/applications/owners/editor/PhabricatorOwnersPackageEditor.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5f07bc88d1..9fb262734c 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/owners/controller/PhabricatorOwnersDeleteController.php b/src/applications/owners/controller/PhabricatorOwnersDeleteController.php index 306658eb6b..4f14ddf66a 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDeleteController.php +++ b/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/'); } diff --git a/src/applications/owners/controller/PhabricatorOwnersEditController.php b/src/applications/owners/controller/PhabricatorOwnersEditController.php index 5ffdec2adb..718dc6ce2d 100644 --- a/src/applications/owners/controller/PhabricatorOwnersEditController.php +++ b/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) { diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditor.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditor.php new file mode 100644 index 0000000000..160c9039aa --- /dev/null +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditor.php @@ -0,0 +1,198 @@ +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; + } + +} diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 2bd28da94f..f690d3e403 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/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) ? '/' : '';