Proxy Diffusion Conduit API calls

Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.

This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.

In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).

Test Plan:
  - With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
  - With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7020

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

src/applications/conduit/call/ConduitCall.php

@@ -43,6 +43,10 @@ final class ConduitCall {
     $this->request = new ConduitAPIRequest($params);
   }
 
+  public function getAPIRequest() {
+    return $this->request;
+  }
+
   public function setUser(PhabricatorUser $user) {
     $this->user = $user;
     return $this;

src/applications/conduit/controller/PhabricatorConduitAPIController.php

@@ -34,11 +34,14 @@ final class PhabricatorConduitAPIController
 
       $result = null;
 
-      // TODO: Straighten out the auth pathway here. We shouldn't be creating
+      // TODO: The relationship between ConduitAPIRequest and ConduitCall is a
-      // a ConduitAPIRequest at this level, but some of the auth code expects
+      // little odd here and could probably be improved. Specifically, the
-      // it. Landing a halfway version of this to unblock T945.
+      // APIRequest is a sub-object of the Call, which does not parallel the
+      // role of AphrontRequest (which is an indepenent object).
+      // In particular, the setUser() and getUser() existing independently on
+      // the Call and APIRequest is very awkward.
 
-      $api_request = new ConduitAPIRequest($params);
+      $api_request = $call->getAPIRequest();
 
       $allow_unguarded_writes = false;
       $auth_error = null;
@@ -284,6 +287,9 @@ final class PhabricatorConduitAPIController
         }
 
         $user = PhabricatorUser::getOmnipotentUser();
+
+        // Flag this as an intracluster request.
+        $api_request->setIsClusterRequest(true);
       }
 
       return $this->validateAuthenticatedUser(
@@ -380,6 +386,9 @@ final class PhabricatorConduitAPIController
               'cluster address range. Requests signed with cluster API '.
               'tokens must originate from within the cluster.'),);
         }
+
+        // Flag this as an intracluster request.
+        $api_request->setIsClusterRequest(true);
       }
 
       $user = $token->getObject();

src/applications/conduit/protocol/ConduitAPIRequest.php

@@ -4,6 +4,7 @@ final class ConduitAPIRequest {
 
   protected $params;
   private $user;
+  private $isClusterRequest = false;
 
   public function __construct(array $params) {
     $this->params = $params;
@@ -42,4 +43,13 @@ final class ConduitAPIRequest {
     return $this->user;
   }
 
+  public function setIsClusterRequest($is_cluster_request) {
+    $this->isClusterRequest = $is_cluster_request;
+    return $this;
+  }
+
+  public function getIsClusterRequest() {
+    return $this->isClusterRequest;
+  }
+
 }

src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php

@@ -105,9 +105,36 @@ abstract class DiffusionQueryConduitAPIMethod
         'initFromConduit' => false,
       ));
 
-    $this->setDiffusionRequest($drequest);
+    // Figure out whether we're going to handle this request on this device,
+    // or proxy it to another node in the cluster.
 
-    return $this->getResult($request);
+    // If this is a cluster request and we need to proxy, we'll explode here
+    // to prevent infinite recursion.
+
+    $is_cluster_request = $request->getIsClusterRequest();
+
+    $repository = $drequest->getRepository();
+    $client = $repository->newConduitClient(
+      $request->getUser(),
+      $is_cluster_request);
+    if ($client) {
+      // We're proxying, so just make an intracluster call.
+      return $client->callMethodSynchronous(
+        $this->getAPIMethodName(),
+        $request->getAllParameters());
+    } else {
+
+      // We pass this flag on to prevent proxying of any other Conduit calls
+      // which we need to make in order to respond to this one. Although we
+      // could safely proxy them, we take a big performance hit in the common
+      // case, and doing more proxying wouldn't exercise any additional code so
+      // we wouldn't gain a testability/predictability benefit.
+      $drequest->setIsClusterRequest($is_cluster_request);
+
+      $this->setDiffusionRequest($drequest);
+
+      return $this->getResult($request);
+    }
   }
 
   protected function getResult(ConduitAPIRequest $request) {

src/applications/diffusion/query/DiffusionQuery.php

@@ -72,7 +72,9 @@ abstract class DiffusionQuery extends PhabricatorQuery {
 
     $params = $params + $core_params;
 
-    $client = $repository->newConduitClient($user);
+    $client = $repository->newConduitClient(
+      $user,
+      $drequest->getIsClusterRequest());
     if (!$client) {
       return id(new ConduitCall($method, $params))
         ->setUser($user)

src/applications/diffusion/request/DiffusionRequest.php

@@ -24,6 +24,7 @@ abstract class DiffusionRequest {
   protected $repositoryCommitData;
   protected $arcanistProjects;
 
+  private $isClusterRequest = false;
   private $initFromConduit = true;
   private $user;
   private $branchObject = false;
@@ -765,5 +766,13 @@ abstract class DiffusionRequest {
     }
   }
 
+  public function setIsClusterRequest($is_cluster_request) {
+    $this->isClusterRequest = $is_cluster_request;
+    return $this;
+  }
+
+  public function getIsClusterRequest() {
+    return $this->isClusterRequest;
+  }
 
 }

src/applications/repository/storage/PhabricatorRepository.php

@@ -1520,13 +1520,39 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
    * Build a new Conduit client in order to make a service call to this
    * repository.
    *
-   * If the repository is hosted locally, this method returns `null`. The
+   * If the repository is hosted locally, this method may return `null`. The
    * caller should use `ConduitCall` or other local logic to complete the
    * request.
    *
+   * By default, we will return a @{class:ConduitClient} for any repository with
+   * a service, even if that service is on the current device.
+   *
+   * We do this because this configuration does not make very much sense in a
+   * production context, but is very common in a test/development context
+   * (where the developer's machine is both the web host and the repository
+   * service). By proxying in development, we get more consistent behavior
+   * between development and production, and don't have a major untested
+   * codepath.
+   *
+   * The `$never_proxy` parameter can be used to prevent this local proxying.
+   * If the flag is passed:
+   *
+   *   - The method will return `null` (implying a local service call)
+   *     if the repository service is hosted on the current device.
+   *   - The method will throw if it would need to return a client.
+   *
+   * This is used to prevent loops in Conduit: the first request will proxy,
+   * even in development, but the second request will be identified as a
+   * cluster request and forced not to proxy.
+   *
+   * @param PhabricatorUser Viewing user.
+   * @param bool `true` to throw if a client would be returned.
    * @return ConduitClient|null Client, or `null` for local repositories.
    */
-  public function newConduitClient(PhabricatorUser $viewer) {
+  public function newConduitClient(
+    PhabricatorUser $viewer,
+    $never_proxy = false) {
+
     $service_phid = $this->getAlmanacServicePHID();
     if (!$service_phid) {
       // No service, so this is a local repository.
@@ -1561,10 +1587,32 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
           'interfaces.'));
     }
 
+    $local_device = AlmanacKeys::getDeviceID();
+
+    if ($never_proxy && !$local_device) {
+      throw new Exception(
+        pht(
+          'Unable to handle proxied service request. This device is not '.
+          'registered, so it can not identify local services. Register '.
+          'this device before sending requests here.'));
+    }
+
     $uris = array();
     foreach ($bindings as $binding) {
       $iface = $binding->getInterface();
 
+      // If we're never proxying this and it's locally satisfiable, return
+      // `null` to tell the caller to handle it locally. If we're allowed to
+      // proxy, we skip this check and may proxy the request to ourselves.
+      // (That proxied request will end up here with proxying forbidden,
+      // return `null`, and then the request will actually run.)
+
+      if ($local_device && $never_proxy) {
+        if ($iface->getDevice()->getName() == $local_device) {
+          return null;
+        }
+      }
+
       $protocol = $binding->getAlmanacPropertyValue('protocol');
       if ($protocol === 'http') {
         $uris[] = 'http://'.$iface->renderDisplayAddress().'/';
@@ -1579,6 +1627,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       }
     }
 
+    if ($never_proxy) {
+      throw new Exception(
+        pht(
+          'Refusing to proxy a repository request from a cluster host. '.
+          'Cluster hosts must correctly route their intracluster requests.'));
+    }
+
     shuffle($uris);
     $uri = head($uris);