Relax the same-origin check in the Git commit discovery daemon

Summary:
Git accepts either "git@x:/path" or "ssh://git@x.com/path" URIs to mean the
exact same thing, which is causing some false positives and confusion,
particularly because we sometimes mutate URIs.

Since this is just a sanity check, we don't really care about the username,
domain or credentials -- matching the paths is good enough. We're just trying to
make it hard to shoot yourself in the foot by copy-pasting the same local path
into two repositories and forgetting to change one, like I did. :P

Relax the check to only verify the paths are the same.

Test Plan:
  - Ran unit tests, which should fully cover things.
  - Ran commit discovery daemon in debug mode on incorrectly and correctly
configured repositories.

Reviewers: ajtrichards, jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T710

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

src/__phutil_library_map__.php

@@ -623,6 +623,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryEditController' => 'applications/repository/controller/edit',
     'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/git',
     'PhabricatorRepositoryGitCommitDiscoveryDaemon' => 'applications/repository/daemon/commitdiscovery/git',
+    'PhabricatorRepositoryGitCommitDiscoveryDaemonTestCase' => 'applications/repository/daemon/commitdiscovery/git/__tests__',
     'PhabricatorRepositoryGitCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/git',
     'PhabricatorRepositoryGitFetchDaemon' => 'applications/repository/daemon/gitfetch',
     'PhabricatorRepositoryGitHubNotification' => 'applications/repository/storage/githubnotification',
@@ -638,6 +639,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySvnCommitDiscoveryDaemon' => 'applications/repository/daemon/commitdiscovery/svn',
     'PhabricatorRepositorySvnCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/svn',
     'PhabricatorRepositorySymbol' => 'applications/repository/storage/symbol',
+    'PhabricatorRepositoryTestCase' => 'applications/repository/storage/repository/__tests__',
     'PhabricatorRepositoryType' => 'applications/repository/constants/repositorytype',
     'PhabricatorS3FileStorageEngine' => 'applications/files/engine/s3',
     'PhabricatorSQLPatchList' => 'infrastructure/setup/sql',
@@ -1267,6 +1269,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryEditController' => 'PhabricatorRepositoryController',
     'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
     'PhabricatorRepositoryGitCommitDiscoveryDaemon' => 'PhabricatorRepositoryCommitDiscoveryDaemon',
+    'PhabricatorRepositoryGitCommitDiscoveryDaemonTestCase' => 'PhabricatorTestCase',
     'PhabricatorRepositoryGitCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
     'PhabricatorRepositoryGitFetchDaemon' => 'PhabricatorRepositoryPullLocalDaemon',
     'PhabricatorRepositoryGitHubNotification' => 'PhabricatorRepositoryDAO',
@@ -1282,6 +1285,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySvnCommitDiscoveryDaemon' => 'PhabricatorRepositoryCommitDiscoveryDaemon',
     'PhabricatorRepositorySvnCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
     'PhabricatorRepositorySymbol' => 'PhabricatorRepositoryDAO',
+    'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase',
     'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine',
     'PhabricatorSearchAttachController' => 'PhabricatorSearchController',
     'PhabricatorSearchBaseController' => 'PhabricatorController',

src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php

@@ -39,14 +39,10 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
         "Expected 'Fetch URL' in 'git remote show -n origin'.");
     }
 
-    $remote = $matches[1];
-    $expect = $repository->getRemoteURI();
-    if ($remote != $expect) {
-      $local_path = $repository->getLocalPath();
-      throw new Exception(
-        "Working copy '{$local_path}' has origin URL '{$remote}', but the ".
-        "configured URL '{$expect}' is expected. Refusing to proceed.");
-    }
+    self::verifySameGitOrigin(
+      $matches[1],
+      $repository->getRemoteURI(),
+      $repository->getLocalPath());
 
     list($stdout) = $repository->execxLocalCommand(
       'branch -r --verbose --no-abbrev');
@@ -118,4 +114,21 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
     }
   }
 
+  public static function verifySameGitOrigin($remote, $expect, $where) {
+    $remote_uri = PhabricatorRepository::newPhutilURIFromGitURI($remote);
+    $expect_uri = PhabricatorRepository::newPhutilURIFromGitURI($expect);
+
+    $remote_path = $remote_uri->getPath();
+    $expect_path = $expect_uri->getPath();
+
+    if ($remote_path != $expect_path) {
+      throw new Exception(
+        "Working copy at '{$where}' has a mismatched origin URL. It has ".
+        "origin URL '{$remote}' (with remote path '{$remote_path}'), but the ".
+        "configured URL '{$expect}' (with remote path '{$expect_path}') is ".
+        "expected. Refusing to proceed because this may indicate that the ".
+        "working copy is actually some other repository.");
+    }
+  }
+
 }

src/applications/repository/daemon/commitdiscovery/git/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'applications/diffusion/query/branch/git');
 phutil_require_module('phabricator', 'applications/repository/constants/repositorytype');
 phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/base');
+phutil_require_module('phabricator', 'applications/repository/storage/repository');
 
 
 phutil_require_source('PhabricatorRepositoryGitCommitDiscoveryDaemon.php');

src/applications/repository/daemon/commitdiscovery/git/__tests__/PhabricatorRepositoryGitCommitDiscoveryDaemonTestCase.php

@@ -0,0 +1,89 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorRepositoryGitCommitDiscoveryDaemonTestCase
+  extends PhabricatorTestCase {
+
+  public function testVerifySameGitOrigin() {
+    $cases = array(
+      array(
+        'ssh://user@domain.com/path.git',
+        'ssh://user@domain.com/path.git',
+        true,
+        'Identical paths should pass.',
+      ),
+      array(
+        'ssh://user@domain.com/path.git',
+        'https://user@domain.com/path.git',
+        true,
+        'Protocol changes should pass.',
+      ),
+      array(
+        'ssh://user@domain.com/path.git',
+        'git@domain.com:path.git',
+        true,
+        'Git implicit SSH should pass.',
+      ),
+      array(
+        'ssh://user@gitserv001.com/path.git',
+        'ssh://user@gitserv002.com/path.git',
+        true,
+        'Domain changes should pass.',
+      ),
+      array(
+        'ssh://alincoln@domain.com/path.git',
+        'ssh://htaft@domain.com/path.git',
+        true,
+        'User/auth changes should pass.',
+      ),
+      array(
+        'ssh://user@domain.com/apples.git',
+        'ssh://user@domain.com/bananas.git',
+        false,
+        'Path changes should fail.',
+      ),
+      array(
+        'ssh://user@domain.com/apples.git',
+        'git@domain.com:bananas.git',
+        false,
+        'Git implicit SSH path changes should fail.',
+      ),
+    );
+
+    foreach ($cases as $case) {
+      list($remote, $config, $expect, $message) = $case;
+
+      $ex = null;
+      try {
+        PhabricatorRepositoryGitCommitDiscoveryDaemon::verifySameGitOrigin(
+          $remote,
+          $config,
+          '(a test case)');
+      } catch (Exception $exception) {
+        $ex = $exception;
+      }
+
+      $this->assertEqual(
+        $expect,
+        !$ex,
+        "Verification that '{$remote}' and '{$config}' are the same origin ".
+        "had a different outcome than expected: {$message}");
+    }
+  }
+
+}

src/applications/repository/daemon/commitdiscovery/git/__tests__/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/git');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+
+
+phutil_require_source('PhabricatorRepositoryGitCommitDiscoveryDaemonTestCase.php');

src/applications/repository/storage/repository/PhabricatorRepository.php

@@ -57,19 +57,30 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO {
     return $this;
   }
 
+  public static function newPhutilURIFromGitURI($raw_uri) {
+    // If there's no protocol (git implicit SSH) reformat the URI to be a
+    // normal URI. These git URIs look like "user@domain.com:path" instead of
+    // "ssh://user@domain/path".
+
+    $uri = new PhutilURI($raw_uri);
+    if (!$uri->getProtocol()) {
+      list($domain, $path) = explode(':', $raw_uri, 2);
+      $uri = new PhutilURI('ssh://'.$domain.'/'.$path);
+    }
+
+    return $uri;
+  }
+
   public function getRemoteURI() {
     $raw_uri = $this->getDetail('remote-uri');
 
     $vcs = $this->getVersionControlSystem();
     $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT);
 
-    // If there's no protocol (git implicit SSH) reformat the URI to be a
-    // normal URI. These git URIs look like "user@domain.com:path" instead of
-    // "ssh://user@domain/path".
+    if ($is_git) {
+      $uri = self::newPhutilURIFromGitURI($raw_uri);
+    } else {
       $uri = new PhutilURI($raw_uri);
-    if ($is_git && !$uri->getProtocol()) {
-      list($domain, $path) = explode(':', $raw_uri, 2);
-      $uri = new PhutilURI('ssh://'.$domain.'/'.$path);
     }
 
     if ($this->isSSHProtocol($uri->getProtocol())) {

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

@@ -0,0 +1,37 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorRepositoryTestCase
+  extends PhabricatorTestCase {
+
+  public function testParseGitURI() {
+    static $map = array(
+      'ssh://user@domain.com/path.git'  => 'ssh://user@domain.com/path.git',
+      'user@domain.com:path.git'        => 'ssh://user@domain.com/path.git',
+    );
+
+    foreach ($map as $raw => $expect) {
+      $uri = PhabricatorRepository::newPhutilURIFromGitURI($raw);
+      $this->assertEqual(
+        $expect,
+        (string)$uri,
+        "Normalized Git URI '{$raw}'");
+    }
+  }
+
+}

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

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/repository/storage/repository');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+
+
+phutil_require_source('PhabricatorRepositoryTestCase.php');