Begin cleaning up OAuth scope handling

Summary:
Ref T7303. OAuth scope handling never got fully modernized and is a bit of a mess.

Also introduce implicit "ALWAYS" and "NEVER" scopes.

Always give tokens access to meta-methods like `conduit.getcapabilities` and `conduit.query`. These do not expose user information.

Test Plan:
  - Used a token to call `user.whoami`.
  - Used a token to call `conduit.query`.
  - Used a token to try to call `user.query`, got rebuffed.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7303

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

src/applications/conduit/call/ConduitCall.php

@@ -65,10 +65,6 @@ final class ConduitCall extends Phobject {
     return $this->handler->shouldAllowUnguardedWrites();
   }
 
-  public function getRequiredScope() {
-    return $this->handler->getRequiredScope();
-  }
-
   public function getErrorDescription($code) {
     return $this->handler->getErrorDescription($code);
   }

src/applications/conduit/controller/PhabricatorConduitAPIController.php

@@ -45,7 +45,6 @@ final class PhabricatorConduitAPIController
       $auth_error = null;
       $conduit_username = '-';
       if ($call->shouldRequireAuthentication()) {
-        $metadata['scope'] = $call->getRequiredScope();
         $auth_error = $this->authenticateUser($api_request, $metadata, $method);
         // If we've explicitly authenticated the user here and either done
         // CSRF validation or are using a non-web authentication mechanism.
@@ -185,11 +184,6 @@ final class PhabricatorConduitAPIController
       // First, verify the signature.
       try {
         $protocol_data = $metadata;
-
-        // TODO: We should stop writing this into the protocol data when
-        // processing a request.
-        unset($protocol_data['scope']);
-
         ConduitClient::verifySignature(
           $method,
           $api_request->getAllParameters(),
@@ -362,11 +356,8 @@ final class PhabricatorConduitAPIController
         $user);
     }
 
-    // handle oauth
     $access_token = idx($metadata, 'access_token');
-    $method_scope = idx($metadata, 'scope');
-    if ($access_token &&
-        $method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) {
+    if ($access_token) {
       $token = id(new PhabricatorOAuthServerAccessToken())
         ->loadOneWhere('token = %s', $access_token);
       if (!$token) {
@@ -377,25 +368,33 @@ final class PhabricatorConduitAPIController
       }
 
       $oauth_server = new PhabricatorOAuthServer();
-      $valid = $oauth_server->validateAccessToken($token,
-                                                  $method_scope);
-      if (!$valid) {
+      $authorization = $oauth_server->authorizeToken($token);
+      if (!$authorization) {
         return array(
           'ERR-INVALID-AUTH',
-          pht('Access token is invalid.'),
+          pht('Access token is invalid or expired.'),
         );
       }
 
-      // valid token, so let's log in the user!
-      $user_phid = $token->getUserPHID();
-      $user = id(new PhabricatorUser())
-        ->loadOneWhere('phid = %s', $user_phid);
+      $user = id(new PhabricatorPeopleQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs(array($token->getUserPHID()))
+        ->executeOne();
       if (!$user) {
         return array(
           'ERR-INVALID-AUTH',
           pht('Access token is for invalid user.'),
         );
       }
+
+      $ok = $this->authorizeOAuthMethodAccess($authorization, $method);
+      if (!$ok) {
+        return array(
+          'ERR-OAUTH-ACCESS',
+          pht('You do not have authorization to call this method.'),
+        );
+      }
+
       return $this->validateAuthenticatedUser(
         $api_request,
         $user);
@@ -642,4 +641,30 @@ final class PhabricatorConduitAPIController
     return array($metadata, $params);
   }
 
+  private function authorizeOAuthMethodAccess(
+    PhabricatorOAuthClientAuthorization $authorization,
+    $method_name) {
+
+    $method = ConduitAPIMethod::getConduitMethod($method_name);
+    if (!$method) {
+      return false;
+    }
+
+    $required_scope = $method->getRequiredScope();
+    switch ($required_scope) {
+      case ConduitAPIMethod::SCOPE_ALWAYS:
+        return true;
+      case ConduitAPIMethod::SCOPE_NEVER:
+        return false;
+    }
+
+    $authorization_scope = $authorization->getScope();
+    if (!empty($authorization_scope[$required_scope])) {
+      return true;
+    }
+
+    return false;
+  }
+
+
 }

src/applications/conduit/controller/PhabricatorConduitConsoleController.php

@@ -142,6 +142,36 @@ final class PhabricatorConduitConsoleController
       pht('Errors'),
       $error_description);
 
+
+    $scope = $method->getRequiredScope();
+    switch ($scope) {
+      case ConduitAPIMethod::SCOPE_ALWAYS:
+        $oauth_icon = 'fa-globe green';
+        $oauth_description = pht(
+          'OAuth clients may always call this method.');
+        break;
+      case ConduitAPIMethod::SCOPE_NEVER:
+        $oauth_icon = 'fa-ban red';
+        $oauth_description = pht(
+          'OAuth clients may never call this method.');
+        break;
+      default:
+        $oauth_icon = 'fa-unlock-alt blue';
+        $oauth_description = pht(
+          'OAuth clients may call this method after requesting access to '.
+          'the "%s" scope.',
+          $scope);
+        break;
+    }
+
+    $view->addProperty(
+      pht('OAuth Scope'),
+      array(
+        id(new PHUIIconView())->setIcon($oauth_icon),
+        ' ',
+        $oauth_description,
+      ));
+
     $view->addSectionHeader(
       pht('Description'), PHUIPropertyListView::ICON_SUMMARY);
     $view->addTextContent(

src/applications/conduit/method/ConduitAPIMethod.php

@@ -15,6 +15,8 @@ abstract class ConduitAPIMethod
   const METHOD_STATUS_UNSTABLE    = 'unstable';
   const METHOD_STATUS_DEPRECATED  = 'deprecated';
 
+  const SCOPE_NEVER = 'scope.never';
+  const SCOPE_ALWAYS = 'scope.always';
 
   /**
    * Get a short, human-readable text summary of the method.
@@ -108,8 +110,7 @@ abstract class ConduitAPIMethod
   }
 
   public function getRequiredScope() {
-    // by default, conduit methods are not accessible via OAuth
-    return PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE;
+    return self::SCOPE_NEVER;
   }
 
   public function executeMethod(ConduitAPIRequest $request) {

src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php

@@ -24,6 +24,10 @@ final class ConduitGetCapabilitiesConduitAPIMethod extends ConduitAPIMethod {
     return 'dict<string, any>';
   }
 
+  public function getRequiredScope() {
+    return self::SCOPE_ALWAYS;
+  }
+
   protected function execute(ConduitAPIRequest $request) {
     $authentication = array(
       'token',

src/applications/conduit/method/ConduitQueryConduitAPIMethod.php

@@ -18,6 +18,10 @@ final class ConduitQueryConduitAPIMethod extends ConduitAPIMethod {
     return 'dict<dict>';
   }
 
+  public function getRequiredScope() {
+    return self::SCOPE_ALWAYS;
+  }
+
   protected function execute(ConduitAPIRequest $request) {
     $methods = id(new PhabricatorConduitMethodQuery())
       ->setViewer($request->getUser())

src/applications/oauthserver/PhabricatorOAuthServer.php

@@ -29,7 +29,6 @@
 final class PhabricatorOAuthServer extends Phobject {
 
   const AUTHORIZATION_CODE_TIMEOUT = 300;
-  const ACCESS_TOKEN_TIMEOUT       = 3600;
 
   private $user;
   private $client;
@@ -158,39 +157,35 @@ final class PhabricatorOAuthServer extends Phobject {
   /**
    * @task token
    */
-  public function validateAccessToken(
-    PhabricatorOAuthServerAccessToken $token,
-    $required_scope) {
+  public function authorizeToken(
+    PhabricatorOAuthServerAccessToken $token) {
 
-    $created_time    = $token->getDateCreated();
-    $must_be_used_by = $created_time + self::ACCESS_TOKEN_TIMEOUT;
-    $expired         = time() > $must_be_used_by;
-    $authorization   = id(new PhabricatorOAuthClientAuthorization())
-      ->loadOneWhere(
-        'userPHID = %s AND clientPHID = %s',
-        $token->getUserPHID(),
-        $token->getClientPHID());
+    $user_phid = $token->getUserPHID();
+    $client_phid = $token->getClientPHID();
 
+    $authorization = id(new PhabricatorOAuthClientAuthorizationQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withUserPHIDs(array($user_phid))
+      ->withClientPHIDs(array($client_phid))
+      ->executeOne();
     if (!$authorization) {
-      return false;
-    }
-    $token_scope = $authorization->getScope();
-    if (!isset($token_scope[$required_scope])) {
-      return false;
+      return null;
     }
 
-    $valid = true;
-    if ($expired) {
-      $valid = false;
-      // check if the scope includes "offline_access", which makes the
-      // token valid despite being expired
-      if (isset(
-        $token_scope[PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS])) {
-        $valid = true;
+    // TODO: This should probably be reworked; expiration should be an
+    // exclusive property of the token. For now, this logic reads: tokens for
+    // authorizations with "offline_access" never expire.
+
+    $is_expired = $token->isExpired();
+    if ($is_expired) {
+      $offline_access = PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS;
+      $authorization_scope = $authorization->getScope();
+      if (empty($authorization_scope[$offline_access])) {
+        return null;
       }
     }
 
-    return $valid;
+    return $authorization;
   }
 
   /**

src/applications/oauthserver/PhabricatorOAuthServerScope.php

@@ -4,13 +4,7 @@ final class PhabricatorOAuthServerScope extends Phobject {
 
   const SCOPE_OFFLINE_ACCESS = 'offline_access';
   const SCOPE_WHOAMI         = 'whoami';
-  const SCOPE_NOT_ACCESSIBLE = 'not_accessible';
 
-  /*
-   * Note this does not contain SCOPE_NOT_ACCESSIBLE which is magic
-   * used to simplify code for data that is not currently accessible
-   * via OAuth.
-   */
   public static function getScopesDict() {
     return array(
       self::SCOPE_OFFLINE_ACCESS => 1,

src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php

@@ -144,7 +144,7 @@ final class PhabricatorOAuthServerTokenController
       $result = array(
         'access_token' => $access_token->getToken(),
         'token_type'   => 'Bearer',
-        'expires_in'   => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT,
+        'expires_in'   => $access_token->getExpiresDuration(),
       );
       return $response->setContent($result);
     } catch (Exception $e) {

src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php

@@ -7,7 +7,7 @@ final class PhabricatorOAuthClientAuthorizationQuery
   private $userPHIDs;
   private $clientPHIDs;
 
-  public function witHPHIDs(array $phids) {
+  public function withPHIDs(array $phids) {
     $this->phids = $phids;
     return $this;
   }
@@ -22,19 +22,12 @@ final class PhabricatorOAuthClientAuthorizationQuery
     return $this;
   }
 
+  public function newResultObject() {
+    return new PhabricatorOAuthClientAuthorization();
+  }
+
   protected function loadPage() {
-    $table  = new PhabricatorOAuthClientAuthorization();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T auth %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+    return $this->loadStandardPage($this->newResultObject());
   }
 
   protected function willFilterPage(array $authorizations) {
@@ -49,43 +42,44 @@ final class PhabricatorOAuthClientAuthorizationQuery
 
     foreach ($authorizations as $key => $authorization) {
       $client = idx($clients, $authorization->getClientPHID());
+
       if (!$client) {
+        $this->didRejectResult($authorization);
         unset($authorizations[$key]);
         continue;
       }
+
       $authorization->attachClient($client);
     }
 
     return $authorizations;
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
-    $where = array();
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->userPHIDs) {
+    if ($this->userPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'userPHID IN (%Ls)',
         $this->userPHIDs);
     }
 
-    if ($this->clientPHIDs) {
+    if ($this->clientPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'clientPHID IN (%Ls)',
         $this->clientPHIDs);
     }
 
-    $where[] = $this->buildPagingClause($conn_r);
-
-    return $this->formatWhereClause($where);
+    return $where;
   }
 
   public function getQueryApplicationClass() {

src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php

@@ -22,4 +22,18 @@ final class PhabricatorOAuthServerAccessToken
     ) + parent::getConfiguration();
   }
 
+  public function isExpired() {
+    $now = PhabricatorTime::getNow();
+    $expires_epoch = $this->getExpiresEpoch();
+    return ($now > $expires_epoch);
+  }
+
+  public function getExpiresEpoch() {
+    return $this->getDateCreated() + 3600;
+  }
+
+  public function getExpiresDuration() {
+    return PhabricatorTime::getNow() - $this->getExpiresEpoch();
+  }
+
 }