From dd2b10b8f859e88a08499b334287a985781a6314 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 May 2016 12:22:40 -0700 Subject: [PATCH] Guarantee repositories have unique local paths Summary: Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't. Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key. (We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.) Test Plan: - Ran migrations. - Grepped for `local-path`. - Listed and moved paths with `bin/repository`. - Created a new repository, verified its local path populated correctly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4039 Differential Revision: https://secure.phabricator.com/D15837 --- .../autopatches/20160503.repo.01.lpath.sql | 2 + .../autopatches/20160503.repo.02.lpathkey.sql | 2 + .../20160503.repo.03.lpathmigrate.php | 57 +++++++++++++++++++ .../DiffusionRepositoryEditMainController.php | 4 +- ...ffusionRepositoryEditStorageController.php | 8 +-- ...fusionRepositoryStorageManagementPanel.php | 2 +- .../RepositoryCreateConduitAPIMethod.php | 3 +- .../editor/PhabricatorRepositoryEditor.php | 8 +-- .../PhabricatorWorkingCopyTestCase.php | 2 +- ...icatorRepositoryManagementEditWorkflow.php | 11 ---- ...rRepositoryManagementMovePathsWorkflow.php | 6 +- .../storage/PhabricatorRepository.php | 17 ++++-- .../PhabricatorRepositoryTestCase.php | 4 +- 13 files changed, 89 insertions(+), 37 deletions(-) create mode 100644 resources/sql/autopatches/20160503.repo.01.lpath.sql create mode 100644 resources/sql/autopatches/20160503.repo.02.lpathkey.sql create mode 100644 resources/sql/autopatches/20160503.repo.03.lpathmigrate.php diff --git a/resources/sql/autopatches/20160503.repo.01.lpath.sql b/resources/sql/autopatches/20160503.repo.01.lpath.sql new file mode 100644 index 0000000000..437dfb1317 --- /dev/null +++ b/resources/sql/autopatches/20160503.repo.01.lpath.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository + ADD localPath VARCHAR(128) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160503.repo.02.lpathkey.sql b/resources/sql/autopatches/20160503.repo.02.lpathkey.sql new file mode 100644 index 0000000000..b630d87c26 --- /dev/null +++ b/resources/sql/autopatches/20160503.repo.02.lpathkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository + ADD UNIQUE KEY `key_local` (localPath); diff --git a/resources/sql/autopatches/20160503.repo.03.lpathmigrate.php b/resources/sql/autopatches/20160503.repo.03.lpathmigrate.php new file mode 100644 index 0000000000..d810b074c9 --- /dev/null +++ b/resources/sql/autopatches/20160503.repo.03.lpathmigrate.php @@ -0,0 +1,57 @@ +establishConnection('w'); + +$default_path = PhabricatorEnv::getEnvConfig('repository.default-local-path'); +$default_path = rtrim($default_path, '/'); + +foreach (new LiskMigrationIterator($table) as $repository) { + $local_path = $repository->getLocalPath(); + if (strlen($local_path)) { + // Repository already has a modern, unique local path. + continue; + } + + $local_path = $repository->getDetail('local-path'); + if (!strlen($local_path)) { + // Repository does not have a local path using the older format. + continue; + } + + $random = Filesystem::readRandomCharacters(8); + + // Try the configured path first, then a default path, then a path with some + // random noise. + $paths = array( + $local_path, + $default_path.'/'.$repository->getID().'/', + $default_path.'/'.$repository->getID().'-'.$random.'/', + ); + + foreach ($paths as $path) { + // Set, then get the path to normalize it. + $repository->setLocalPath($path); + $path = $repository->getLocalPath(); + + try { + queryfx( + $conn_w, + 'UPDATE %T SET localPath = %s WHERE id = %d', + $table->getTableName(), + $path, + $repository->getID()); + + echo tsprintf( + "%s\n", + pht( + 'Assigned repository "%s" to local path "%s".', + $repository->getDisplayName(), + $path)); + + break; + } catch (AphrontDuplicateKeyQueryException $ex) { + // Ignore, try the next one. + } + } +} diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index d8a4cf40ba..08fc9778bc 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -631,9 +631,7 @@ final class DiffusionRepositoryEditMainController pht('Storage Service'), $v_service); - $view->addProperty( - pht('Storage Path'), - $repository->getHumanReadableDetail('local-path')); + $view->addProperty(pht('Storage Path'), $repository->getLocalPath()); return $view; } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditStorageController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditStorageController.php index f3a492e3b3..6aebd13500 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditStorageController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditStorageController.php @@ -15,7 +15,7 @@ final class DiffusionRepositoryEditStorageController $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); - $v_local = $repository->getHumanReadableDetail('local-path'); + $v_local = $repository->getLocalPath(); $errors = array(); $crumbs = $this->buildApplicationCrumbs(); @@ -51,11 +51,7 @@ final class DiffusionRepositoryEditStorageController ->appendRemarkupInstructions( pht( "You can not adjust the local path for this repository from the ". - "web interface. To edit it, run this command:\n\n %s", - sprintf( - 'phabricator/ $ ./bin/repository edit %s --as %s --local-path ...', - $repository->getMonogram(), - $viewer->getUsername()))) + 'web interface.')) ->appendChild( id(new AphrontFormSubmitControl()) ->addCancelButton($edit_uri, pht('Done'))); diff --git a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php index 734b4c6f8e..0c723bd9e6 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php @@ -40,7 +40,7 @@ final class DiffusionRepositoryStorageManagementPanel ->setViewer($viewer); if ($repository->usesLocalWorkingCopy()) { - $storage_path = $repository->getHumanReadableDetail('local-path'); + $storage_path = $repository->getLocalPath(); } else { $storage_path = phutil_tag('em', array(), pht('No Local Working Copy')); } diff --git a/src/applications/repository/conduit/RepositoryCreateConduitAPIMethod.php b/src/applications/repository/conduit/RepositoryCreateConduitAPIMethod.php index ad4b62104e..bec9b3b9a3 100644 --- a/src/applications/repository/conduit/RepositoryCreateConduitAPIMethod.php +++ b/src/applications/repository/conduit/RepositoryCreateConduitAPIMethod.php @@ -110,7 +110,6 @@ final class RepositoryCreateConduitAPIMethod 'description' => $request->getValue('description'), 'tracking-enabled' => (bool)$request->getValue('tracking', true), 'remote-uri' => $remote_uri, - 'local-path' => $local_path, 'branch-filter' => array_fill_keys( $request->getValue('branchFilter', array()), true), @@ -128,6 +127,8 @@ final class RepositoryCreateConduitAPIMethod $repository->setDetail($key, $value); } + $repository->setLocalPath($local_path); + try { $repository->save(); } catch (AphrontDuplicateKeyQueryException $ex) { diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 3dc842d94f..c528c91c99 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -86,7 +86,7 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_REMOTE_URI: return $object->getDetail('remote-uri'); case PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH: - return $object->getDetail('local-path'); + return $object->getLocalPath(); case PhabricatorRepositoryTransaction::TYPE_HOSTING: return $object->isHosted(); case PhabricatorRepositoryTransaction::TYPE_PROTOCOL_HTTP: @@ -209,7 +209,7 @@ final class PhabricatorRepositoryEditor $object->setDetail('remote-uri', $xaction->getNewValue()); break; case PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH: - $object->setDetail('local-path', $xaction->getNewValue()); + $object->setLocalPath($xaction->getNewValue()); break; case PhabricatorRepositoryTransaction::TYPE_HOSTING: return $object->setHosted($xaction->getNewValue()); @@ -706,7 +706,7 @@ final class PhabricatorRepositoryEditor // If the repository does not have a local path yet, assign it one based // on its ID. We can't do this earlier because we won't have an ID yet. - $local_path = $object->getDetail('local-path'); + $local_path = $object->getLocalPath(); if (!strlen($local_path)) { $local_key = 'repository.default-local-path'; @@ -716,7 +716,7 @@ final class PhabricatorRepositoryEditor $id = $object->getID(); $local_path = "{$local_root}/{$id}/"; - $object->setDetail('local-path', $local_path); + $object->setLocalPath($local_path); $object->save(); } diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php index 5a9be99212..5b37a9e655 100644 --- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php +++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php @@ -65,7 +65,7 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase { ->setCallsign($callsign) ->setName(pht('Test Repo "%s"', $callsign)) ->setVersionControlSystem($vcs_type) - ->setDetail('local-path', dirname($local).'/'.$callsign) + ->setLocalPath(dirname($local).'/'.$callsign) ->setDetail('remote-uri', 'file://'.$dir->getPath().'/'); $this->didConstructRepository($repo); diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php index cc33f1021c..8bf31c63d9 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php @@ -21,11 +21,6 @@ final class PhabricatorRepositoryManagementEditWorkflow 'param' => 'user', 'help' => pht('Edit as user.'), ), - array( - 'name' => 'local-path', - 'param' => 'path', - 'help' => pht('Edit the local path.'), - ), array( 'name' => 'serve-http', 'param' => 'string', @@ -83,7 +78,6 @@ final class PhabricatorRepositoryManagementEditWorkflow $xactions = array(); - $type_local_path = PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH; $type_protocol_http = PhabricatorRepositoryTransaction::TYPE_PROTOCOL_HTTP; $type_protocol_ssh = PhabricatorRepositoryTransaction::TYPE_PROTOCOL_SSH; @@ -93,11 +87,6 @@ final class PhabricatorRepositoryManagementEditWorkflow PhabricatorRepository::SERVE_READWRITE, ); - if ($args->getArg('local-path')) { - $xactions[] = id(new PhabricatorRepositoryTransaction()) - ->setTransactionType($type_local_path) - ->setNewValue($args->getArg('local-path')); - } $serve_http = $args->getArg('serve-http'); if ($serve_http && in_array($serve_http, $allowed_serve_modes)) { $xactions[] = id(new PhabricatorRepositoryTransaction()) diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php index f0146847ae..baee437883 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php @@ -128,14 +128,12 @@ final class PhabricatorRepositoryManagementMovePathsWorkflow } $repo = $row['repository']; - $details = $repo->getDetails(); - $details['local-path'] = $row['dst']; queryfx( $repo->establishConnection('w'), - 'UPDATE %T SET details = %s WHERE id = %d', + 'UPDATE %T SET localPath = %s WHERE id = %d', $repo->getTableName(), - phutil_json_encode($details), + $row['dst'], $repo->getID()); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index be77852be8..9785a32611 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -62,6 +62,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO protected $credentialPHID; protected $almanacServicePHID; protected $spacePHID; + protected $localPath; private $commitCount = self::ATTACHABLE; private $mostRecentCommit = self::ATTACHABLE; @@ -107,6 +108,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'pushPolicy' => 'policy', 'credentialPHID' => 'phid?', 'almanacServicePHID' => 'phid?', + 'localPath' => 'text128?', ), self::CONFIG_KEY_SCHEMA => array( 'callsign' => array( @@ -123,6 +125,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'columns' => array('repositorySlug'), 'unique' => true, ), + 'key_local' => array( + 'columns' => array('localPath'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -216,6 +222,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $monograms; } + public function setLocalPath($path) { + // Convert any extra slashes ("//") in the path to a single slash ("/"). + $path = preg_replace('(//+)', '/', $path); + + return parent::setLocalPath($path); + } + public function getDetail($key, $default = null) { return idx($this->details, $key, $default); } @@ -279,10 +292,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO )); } - public function getLocalPath() { - return $this->getDetail('local-path'); - } - public function getSubversionBaseURI($commit = null) { $subpath = $this->getDetail('svn-subpath'); if (!strlen($subpath)) { diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php index c6d2278262..63bae03d80 100644 --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -70,12 +70,12 @@ final class PhabricatorRepositoryTestCase $repo->setDetail('hosting-enabled', true); - $repo->setDetail('local-path', '/var/repo/SVN'); + $repo->setLocalPath('/var/repo/SVN'); $this->assertEqual( 'file:///var/repo/SVN', $repo->getSubversionPathURI()); - $repo->setDetail('local-path', '/var/repo/SVN/'); + $repo->setLocalPath('/var/repo/SVN/'); $this->assertEqual( 'file:///var/repo/SVN', $repo->getSubversionPathURI());