Accept Conduit tokens as an authentication mechanism

Summary:
  - Ref T5955. Accept the tokens introduced in D10985 as an authentication token.
  - Ref T3628. Permit simple `curl`-compatible decoding of parameters.

Test Plan:
  - Ran some sensible `curl` API commands:

```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/user.whoami?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk" ; echo
{"result":{"phid":"PHID-USER-cvfydnwadpdj7vdon36z","userName":"admin","realName":"asdf","image":"http:\/\/local.phacility.com\/res\/1410737307T\/phabricator\/3eb28cd9\/rsrc\/image\/avatar.png","uri":"http:\/\/local.phacility.com\/p\/admin\/","roles":["admin","verified","approved","activated"]},"error_code":null,"error_info":null}
```

```
epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/differential.query?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk&ids[]=1" ; echo
{"result":[{"id":"1","phid":"PHID-DREV-v3a67ixww3ccg5lqbxee","title":"zxcb","uri":"http:\/\/local.phacility.com\/D1","dateCreated":"1418405590","dateModified":"1418405590","authorPHID":"PHID-USER-cvfydnwadpdj7vdon36z","status":"0","statusName":"Needs Review","branch":null,"summary":"","testPlan":"zxcb","lineCount":"6","activeDiffPHID":"PHID-DIFF-pzbtc5rw6pe5j2kxtlr2","diffs":["1"],"commits":[],"reviewers":[],"ccs":[],"hashes":[],"auxiliary":{"phabricator:projects":[],"phabricator:depends-on":[],"organization.sqlmigration":null},"arcanistProjectPHID":null,"repositoryPHID":null,"sourcePath":null}],"error_code":null,"error_info":null}
```

  - Ran older-style commands like `arc list` against the local install.
  - Ran commands via web console.
  - Added and ran a unit test to make sure nothing is using forbidden parameter names.
  - Terminated a token and verified it no longer works.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3628, T5955

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

src/__phutil_library_map__.php

@@ -1430,6 +1430,7 @@ phutil_register_library_map(array(
     'PhabricatorConduitMethodQuery' => 'applications/conduit/query/PhabricatorConduitMethodQuery.php',
     'PhabricatorConduitSearchEngine' => 'applications/conduit/query/PhabricatorConduitSearchEngine.php',
     'PhabricatorConduitSettingsPanel' => 'applications/conduit/settings/PhabricatorConduitSettingsPanel.php',
+    'PhabricatorConduitTestCase' => '__tests__/PhabricatorConduitTestCase.php',
     'PhabricatorConduitToken' => 'applications/conduit/storage/PhabricatorConduitToken.php',
     'PhabricatorConduitTokenController' => 'applications/conduit/controller/PhabricatorConduitTokenController.php',
     'PhabricatorConduitTokenEditController' => 'applications/conduit/controller/PhabricatorConduitTokenEditController.php',
@@ -4549,6 +4550,7 @@ phutil_register_library_map(array(
     'PhabricatorConduitMethodQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorConduitSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorConduitSettingsPanel' => 'PhabricatorSettingsPanel',
+    'PhabricatorConduitTestCase' => 'PhabricatorTestCase',
     'PhabricatorConduitToken' => array(
       'PhabricatorConduitDAO',
       'PhabricatorPolicyInterface',

src/__tests__/PhabricatorConduitTestCase.php

@@ -0,0 +1,19 @@
+<?php
+
+final class PhabricatorConduitTestCase extends PhabricatorTestCase {
+
+  public function testConduitMethods() {
+    $methods = id(new PhutilSymbolLoader())
+      ->setAncestorClass('ConduitAPIMethod')
+      ->loadObjects();
+
+    // We're just looking for a side effect of ConduitCall construction
+    // here: it will throw if any methods define reserved parameter names.
+
+    foreach ($methods as $method) {
+      new ConduitCall($method->getAPIMethodName(), array());
+    }
+
+    $this->assertTrue(true);
+  }
+}

src/applications/conduit/call/ConduitCall.php

@@ -18,13 +18,26 @@ final class ConduitCall {
     $this->method     = $method;
     $this->handler    = $this->buildMethodHandler($method);
 
-    $invalid_params = array_diff_key(
+    $param_types = $this->handler->defineParamTypes();
-      $params,
+
-      $this->handler->defineParamTypes());
+    foreach ($param_types as $key => $spec) {
+      if (ConduitAPIMethod::getParameterMetadataKey($key) !== null) {
+        throw new ConduitException(
+          pht(
+            'API Method "%s" defines a disallowed parameter, "%s". This '.
+            'parameter name is reserved.',
+            $method,
+            $key));
+      }
+    }
+
+    $invalid_params = array_diff_key($params, $param_types);
     if ($invalid_params) {
       throw new ConduitException(
-        "Method '{$method}' doesn't define these parameters: '".
+        pht(
-        implode("', '", array_keys($invalid_params))."'.");
+          'API Method "%s" does not define these parameters: %s.',
+          $method,
+          "'".implode("', '", array_keys($invalid_params))."'"));
     }
 
     $this->request = new ConduitAPIRequest($params);

src/applications/conduit/controller/PhabricatorConduitAPIController.php

@@ -28,12 +28,9 @@ final class PhabricatorConduitAPIController
 
     try {
 
-      $params = $this->decodeConduitParams($request, $method);
+      list($metadata, $params) = $this->decodeConduitParams($request, $method);
-      $metadata = idx($params, '__conduit__', array());
-      unset($params['__conduit__']);
 
-      $call = new ConduitCall(
+      $call = new ConduitCall($method, $params);
-        $method, $params, idx($metadata, 'isProxied', false));
 
       $result = null;
 
@@ -296,9 +293,78 @@ final class PhabricatorConduitAPIController
       );
     }
 
+    $token_string = idx($metadata, 'token');
+    if (strlen($token_string)) {
+
+      if (strlen($token_string) != 32) {
+        return array(
+          'ERR-INVALID-AUTH',
+          pht(
+            'API token "%s" has the wrong length. API tokens should be '.
+            '32 characters long.'),
+        );
+      }
+
+      $type = head(explode('-', $token_string));
+      switch ($type) {
+        case 'api':
+        case 'tmp':
+          break;
+        default:
+          return array(
+            'ERR-INVALID-AUTH',
+            pht(
+              'API token "%s" has the wrong format. API tokens should begin '.
+              'with "api-" or "tmp-" and be 32 characters long.',
+              $token_string),
+          );
+      }
+
+      $token = id(new PhabricatorConduitTokenQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withTokens(array($token_string))
+        ->withExpired(false)
+        ->executeOne();
+      if (!$token) {
+        $token = id(new PhabricatorConduitTokenQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withTokens(array($token_string))
+          ->withExpired(true)
+          ->executeOne();
+        if ($token) {
+          return array(
+            'ERR-INVALID-AUTH',
+            pht(
+              'API token "%s" was previously valid, but has expired.',
+              $token_string),
+          );
+        } else {
+          return array(
+            'ERR-INVALID-AUTH',
+            pht(
+              'API token "%s" is not valid.',
+              $token_string),
+          );
+        }
+      }
+
+      $user = $token->getObject();
+      if (!($user instanceof PhabricatorUser)) {
+        return array(
+          'ERR-INVALID-AUTH',
+          pht(
+            'API token is not associated with a valid user.'),
+        );
+      }
+
+      return $this->validateAuthenticatedUser(
+        $api_request,
+        $user);
+    }
+
     // handle oauth
-    $access_token = $request->getStr('access_token');
+    $access_token = idx($metadata, 'access_token');
-    $method_scope = $metadata['scope'];
+    $method_scope = idx($metadata, 'scope');
     if ($access_token &&
         $method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) {
       $token = id(new PhabricatorOAuthServerAccessToken())
@@ -337,7 +403,10 @@ final class PhabricatorConduitAPIController
         $user);
     }
 
-    // Handle sessionless auth. TOOD: This is super messy.
+    // Handle sessionless auth.
+    // TODO: This is super messy.
+    // TODO: Remove this in favor of token-based auth.
+
     if (isset($metadata['authUser'])) {
       $user = id(new PhabricatorUser())->loadOneWhere(
         'userName = %s',
@@ -362,6 +431,9 @@ final class PhabricatorConduitAPIController
         $user);
     }
 
+    // Handle session-based auth.
+    // TODO: Remove this in favor of token-based auth.
+
     $session_key = idx($metadata, 'sessionKey');
     if (!$session_key) {
       return array(
@@ -525,28 +597,16 @@ final class PhabricatorConduitAPIController
         $params[$key] = $decoded_value;
       }
 
-      return $params;
+      $metadata = idx($params, '__conduit__', array());
+      unset($params['__conduit__']);
+
+      return array($metadata, $params);
     }
 
     // Otherwise, look for a single parameter called 'params' which has the
-    // entire param dictionary JSON encoded. This is the usual case for remote
+    // entire param dictionary JSON encoded.
-    // requests.
-
     $params_json = $request->getStr('params');
-    if (!strlen($params_json)) {
+    if (strlen($params_json)) {
-      if ($request->getBool('allowEmptyParams')) {
-        // TODO: This is a bit messy, but otherwise you can't call
-        // "conduit.ping" from the web console.
-        $params = array();
-      } else {
-        throw new Exception(
-          "Request has no 'params' key. This may mean that an extension like ".
-          "Suhosin has dropped data from the request. Check the PHP ".
-          "configuration on your server. If you are developing a Conduit ".
-          "client, you MUST provide a 'params' parameter when making a ".
-          "Conduit request, even if the value is empty (e.g., provide '{}').");
-      }
-    } else {
       $params = json_decode($params_json, true);
       if (!is_array($params)) {
         throw new Exception(
@@ -554,9 +614,27 @@ final class PhabricatorConduitAPIController
           "'{$method}', could not decode JSON serialization. Data: ".
           $params_json);
       }
+
+      $metadata = idx($params, '__conduit__', array());
+      unset($params['__conduit__']);
+
+      return array($metadata, $params);
     }
 
-    return $params;
+    // If we do not have `params`, assume this is a simple HTTP request with
+    // HTTP key-value pairs.
+    $params = array();
+    $metadata = array();
+    foreach ($request->getPassthroughRequestData() as $key => $value) {
+      $meta_key = ConduitAPIMethod::getParameterMetadataKey($key);
+      if ($meta_key !== null) {
+        $metadata[$meta_key] = $value;
+      } else {
+        $params[$key] = $value;
+      }
+    }
+
+    return array($metadata, $params);
   }
 
 }

src/applications/conduit/controller/PhabricatorConduitConsoleController.php

@@ -67,7 +67,6 @@ final class PhabricatorConduitConsoleController
     $form
       ->setUser($request->getUser())
       ->setAction('/api/'.$this->method)
-      ->addHiddenInput('allowEmptyParams', 1)
       ->appendChild(
         id(new AphrontFormStaticControl())
           ->setLabel('Description')

src/applications/conduit/method/ConduitAPIMethod.php

@@ -149,6 +149,35 @@ abstract class ConduitAPIMethod
     return 'string-constant<'.$constants.'>';
   }
 
+  public static function getParameterMetadataKey($key) {
+    if (strncmp($key, 'api.', 4) === 0) {
+      // All keys passed beginning with "api." are always metadata keys.
+      return substr($key, 4);
+    } else {
+      switch ($key) {
+        // These are real keys which always belong to request metadata.
+        case 'access_token':
+        case 'scope':
+        case 'output':
+
+        // This is not a real metadata key; it is included here only to
+        // prevent Conduit methods from defining it.
+        case '__conduit__':
+
+        // This is prevented globally as a blanket defense against OAuth
+        // redirection attacks. It is included here to stop Conduit methods
+        // from defining it.
+        case 'code':
+
+        // This is not a real metadata key, but the presence of this
+        // parameter triggers an alternate request decoding pathway.
+        case 'params':
+          return $key;
+      }
+    }
+
+    return null;
+  }
 
 /* -(  Paging Results  )----------------------------------------------------- */
 

src/applications/conduit/query/PhabricatorConduitTokenQuery.php

@@ -6,6 +6,7 @@ final class PhabricatorConduitTokenQuery
   private $ids;
   private $objectPHIDs;
   private $expired;
+  private $tokens;
 
   public function withExpired($expired) {
     $this->expired = $expired;
@@ -22,6 +23,11 @@ final class PhabricatorConduitTokenQuery
     return $this;
   }
 
+  public function withTokens(array $tokens) {
+    $this->tokens = $tokens;
+    return $this;
+  }
+
   public function loadPage() {
     $table = new PhabricatorConduitToken();
     $conn_r = $table->establishConnection('r');
@@ -54,6 +60,13 @@ final class PhabricatorConduitTokenQuery
         $this->objectPHIDs);
     }
 
+    if ($this->tokens !== null) {
+      $where[] = qsprintf(
+        $conn_r,
+        'token IN (%Ls)',
+        $this->tokens);
+    }
+
     if ($this->expired !== null) {
       if ($this->expired) {
         $where[] = qsprintf(