Fix some repository URI handling issues in Git and Mercurial

Summary:
See <https://github.com/facebook/phabricator/issues/467>. @dctrwatson also ran into an issue where we were trying to `setPass()` a GitURI.

  - For Git and Mercurial, properly generate credential URIs where relevant.
  - Don't try to `setPass()` on Git-style URIs.

This isn't perfect but should clean things up a bit.

Test Plan: Added unit tests. Lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: dctrwatson, aran

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

scripts/repository/test_connection.php

@@ -1,95 +1,6 @@
 #!/usr/bin/env php
 <?php
 
-$root = dirname(dirname(dirname(__FILE__)));
-require_once $root.'/scripts/__init_script__.php';
-
-if (empty($argv[1])) {
-  echo "usage: test_connection.php <repository_callsign>\n";
-  exit(1);
-}
-
-echo phutil_console_wrap(
-  phutil_console_format(
-    'This script will test that you have configured valid credentials for '.
-    'access to a repository, so the Phabricator daemons can pull from it. '.
-    'You should run this as the **same user you will run the daemons as**, '.
-    'from the **same machine they will run from**. Doing this will help '.
-    'detect various problems with your configuration, such as SSH issues.'));
-
-list($whoami) = execx('whoami');
-$whoami = trim($whoami);
-
-$ok = phutil_console_confirm("Do you want to continue as '{$whoami}'?");
-if (!$ok) {
-  die(1);
-}
-
-$callsign = $argv[1];
-echo "Loading '{$callsign}' repository...\n";
-$repository = id(new PhabricatorRepository())->loadOneWhere(
-  'callsign = %s',
-  $argv[1]);
-if (!$repository) {
-  throw new Exception("No such repository exists!");
-}
-
-$vcs = $repository->getVersionControlSystem();
-
-PhutilServiceProfiler::installEchoListener();
-
-echo phutil_console_format(
-  "\n".
-  "**NOTE:** If you are prompted for an SSH password in the next step, the\n".
-  "daemon won't work because it doesn't have the password and can't respond\n".
-  "to an interactive prompt. Instead of typing the password, it will hang\n".
-  "forever when prompted. There are several ways to resolve this:\n\n".
-  "  - Run the daemon inside an ssh-agent session where you have unlocked\n".
-  "    the key (most secure, but most complicated).\n".
-  "  - Generate a new, passwordless certificate for the daemon to use\n".
-  "    (usually quite easy).\n".
-  "  - Remove the passphrase from the key with `ssh-keygen -p`\n".
-  "    (easy, but questionable).");
-
-phutil_console_confirm('Did you read all that?', $default_no = false);
-
-echo "Trying to connect to the remote...\n";
-switch ($vcs) {
-  case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-    $err = $repository->passthruRemoteCommand(
-      '--limit 1 log %s',
-      $repository->getRemoteURI());
-    break;
-  case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-    // Do an ls-remote on a nonexistent ref, which we expect to just return
-    // nothing.
-    $err = $repository->passthruRemoteCommand(
-      'ls-remote %s %s',
-      $repository->getRemoteURI(),
-      'just-testing');
-    break;
-  case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-    // TODO: 'hg id' doesn't support --insecure so we can't tell it not to
-    // spew. If 'hg id' eventually supports --insecure, consider using it.
-    echo "(It is safe to ignore any 'certificate with fingerprint ... not ".
-         "verified' warnings, although you may want to configure Mercurial ".
-         "to recognize the server's fingerprint/certificate.)\n";
-    $err = $repository->passthruRemoteCommand(
-      'id --rev tip %s',
-      $repository->getRemoteURI());
-    break;
-  default:
-    throw new Exception("Unsupported repository type.");
-}
-
-if ($err) {
-  echo phutil_console_format(
-    "<bg:red>** FAIL **</bg> Connection failed. The credentials for this ".
-    "repository appear to be incorrectly configured.\n");
-  exit(1);
-} else {
-  echo phutil_console_format(
-    "<bg:green>** OKAY **</bg> Connection successful. The credentials for ".
-    "this repository appear to be correctly configured.\n");
-}
+echo "This script is obsolete. Use `bin/repository` to manage repositories.\n";
+exit(1);
 

src/__phutil_library_map__.php

@@ -1815,6 +1815,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryTransaction' => 'applications/repository/storage/PhabricatorRepositoryTransaction.php',
     'PhabricatorRepositoryTransactionQuery' => 'applications/repository/query/PhabricatorRepositoryTransactionQuery.php',
     'PhabricatorRepositoryType' => 'applications/repository/constants/PhabricatorRepositoryType.php',
+    'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php',
     'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php',
     'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php',
     'PhabricatorSQLPatchList' => 'infrastructure/storage/patch/PhabricatorSQLPatchList.php',
@@ -4380,6 +4381,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase',
     'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase',
     'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO',
     'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine',
     'PhabricatorSSHLog' => 'Phobject',

src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php

@@ -63,8 +63,8 @@ final class DrydockWorkingCopyBlueprintImplementation
 
     $cmd = $host_lease->getInterface('command');
     $cmd->execx(
-      'git clone --origin origin %s %s',
-      $repository->getRemoteURI(),
+      'git clone --origin origin %P %s',
+      $repository->getRemoteURIEnvelope(),
       $path);
 
     $this->log(pht('Complete.'));

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -842,8 +842,8 @@ final class PhabricatorRepositoryPullLocalDaemon
       }
 
       $future = $proxy->getRemoteCommandFuture(
-        'push --verbose --mirror -- %s',
-        $proxy->getRemoteURI());
+        'push --verbose --mirror -- %P',
+        $proxy->getRemoteURIEnvelope());
 
       $future
         ->setCWD($proxy->getLocalPath())

src/applications/repository/engine/PhabricatorRepositoryPullEngine.php

@@ -188,8 +188,8 @@ final class PhabricatorRepositoryPullEngine
         $path);
     } else {
       $repository->execxRemoteCommand(
-        'clone --bare -- %s %s',
-        $repository->getRemoteURI(),
+        'clone --bare -- %P %s',
+        $repository->getRemoteURIEnvelope(),
         $path);
     }
   }
@@ -337,8 +337,8 @@ final class PhabricatorRepositoryPullEngine
         $path);
     } else {
       $repository->execxRemoteCommand(
-        'clone --noupdate -- %s %s',
-        $repository->getRemoteURI(),
+        'clone --noupdate -- %P %s',
+        $repository->getRemoteURIEnvelope(),
         $path);
     }
   }

src/applications/repository/storage/PhabricatorRepository.php

@@ -81,7 +81,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       'callsign'    => $this->getCallsign(),
       'vcs'         => $this->getVersionControlSystem(),
       'uri'         => PhabricatorEnv::getProductionURI($this->getURI()),
-      'remoteURI'   => (string)$this->getPublicRemoteURI(),
+      'remoteURI'   => (string)$this->getRemoteURI(),
       'tracking'    => $this->getDetail('tracking-enabled'),
       'description' => $this->getDetail('description'),
     );
@@ -440,10 +440,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $args;
   }
 
-  public function getSSHLogin() {
-    return $this->getDetail('ssh-login');
-  }
-
   public function getURI() {
     return '/diffusion/'.$this->getCallsign().'/';
   }
@@ -568,6 +564,37 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
 
+  /**
+   * Get the remote URI for this repository, including credentials if they're
+   * used by this repository.
+   *
+   * @return PhutilOpaqueEnvelope URI, possibly including credentials.
+   * @task uri
+   */
+  public function getRemoteURIEnvelope() {
+    $uri = $this->getRemoteURIObject();
+
+    $remote_protocol = $this->getRemoteProtocol();
+    if ($remote_protocol == 'http' || $remote_protocol == 'https') {
+      // For SVN, we use `--username` and `--password` flags separately, so
+      // don't add any credentials here.
+      if (!$this->isSVN()) {
+        $credential_phid = $this->getCredentialPHID();
+        if ($credential_phid) {
+          $key = PassphrasePasswordKey::loadFromPHID(
+            $credential_phid,
+            PhabricatorUser::getOmnipotentUser());
+
+          $uri->setUser($key->getUsernameEnvelope()->openEnvelope());
+          $uri->setPass($key->getPasswordEnvelope()->openEnvelope());
+        }
+      }
+    }
+
+    return new PhutilOpaqueEnvelope((string)$uri);
+  }
+
+
   /**
    * Get the remote URI for this repository, without authentication information.
    *
@@ -584,7 +611,12 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     // password.
     if (!$this->shouldUseSSH()) {
       $uri->setUser(null);
-      $uri->setPass(null);
+
+      // This might be a Git URI or a normal URI. If it's Git, there's no
+      // password support.
+      if ($uri instanceof PhutilURI) {
+        $uri->setPass(null);
+      }
     }
 
     return (string)$uri;
@@ -629,19 +661,11 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
     $uri = new PhutilURI($raw_uri);
     if ($uri->getProtocol()) {
-      if ($this->isSSHProtocol($uri->getProtocol())) {
-        if ($this->getSSHLogin()) {
-          $uri->setUser($this->getSSHLogin());
-        }
-      }
       return $uri;
     }
 
     $uri = new PhutilGitURI($raw_uri);
     if ($uri->getDomain()) {
-      if ($this->getSSHLogin()) {
-        $uri->setUser($this->getSSHLogin());
-      }
       return $uri;
     }
 

src/applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php

@@ -0,0 +1,95 @@
+<?php
+
+final class PhabricatorRepositoryURITestCase
+  extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testURIGeneration() {
+    $svn = PhabricatorRepositoryType::REPOSITORY_TYPE_SVN;
+    $git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;
+    $hg = PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL;
+
+    $user = $this->generateNewTestUser();
+
+    $http_secret = id(new PassphraseSecret())->setSecretData('quack')->save();
+
+    $http_credential = PassphraseCredential::initializeNewCredential($user)
+      ->setCredentialType(PassphraseCredentialTypePassword::CREDENTIAL_TYPE)
+      ->setProvidesType(PassphraseCredentialTypePassword::PROVIDES_TYPE)
+      ->setUsername('duck')
+      ->setSecretID($http_secret->getID())
+      ->save();
+
+    $repo = PhabricatorRepository::initializeNewRepository($user)
+      ->setVersionControlSystem($svn)
+      ->setName('Test Repo')
+      ->setCallsign('TESTREPO')
+      ->setCredentialPHID($http_credential->getPHID())
+      ->save();
+
+    // Test HTTP URIs.
+
+    $repo->setDetail('remote-uri', 'http://example.com/');
+    $repo->setVersionControlSystem($svn);
+
+    $this->assertEqual('http://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('http://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('http://example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    $repo->setVersionControlSystem($git);
+
+    $this->assertEqual('http://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('http://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('http://duck:quack@example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    $repo->setVersionControlSystem($hg);
+
+    $this->assertEqual('http://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('http://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('http://duck:quack@example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    // Test SSH URIs.
+
+    $repo->setDetail('remote-uri', 'ssh://example.com/');
+    $repo->setVersionControlSystem($svn);
+
+    $this->assertEqual('ssh://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('ssh://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('ssh://example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    $repo->setVersionControlSystem($git);
+
+    $this->assertEqual('ssh://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('ssh://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('ssh://example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    $repo->setVersionControlSystem($hg);
+
+    $this->assertEqual('ssh://example.com/', $repo->getRemoteURI());
+    $this->assertEqual('ssh://example.com/', $repo->getPublicRemoteURI());
+    $this->assertEqual('ssh://example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+    // Test Git URIs.
+
+    $repo->setDetail('remote-uri', 'git@example.com:path.git');
+    $repo->setVersionControlSystem($git);
+
+    $this->assertEqual('git@example.com:path.git', $repo->getRemoteURI());
+    $this->assertEqual('git@example.com:path.git', $repo->getPublicRemoteURI());
+    $this->assertEqual('git@example.com:path.git',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
+  }
+
+}