Make OAuth scope handling more flexible

Summary:
Ref T7303. Currently, our handling of "scope" is fairly rigid and adheres to the spec, but some of these behaviors don't make much sense in practice.

Soften some behaviors and make them more flexible:

**Soft Failure on Unknown Permissions**: If a client asks for a permission we don't know about, just warn that we don't recognize it instead of fataling. In particular, I plan to make `offline_access` and `whoami` implicit. Older clients that request these permissions will still work fine as long as we don't hard-fatal.

**Move `user.whoami` to ALWAYS scope**: Make `whoami` a default permission. We've already done this, in effect; this just formalizes it.

**Tokens no longer expire**: Make `offline_access` (infinite-duration tokens) a default permission. I think the OAuth model doesn't map well to reality. It is common for other providers to issue "temporary" tokens with a duration of multiple years, and the refesh workflow is sort of silly. We can add a "temporary" scope later if we need temporary tokens.

This flow was potentially extra silly with the "log out of Phacility" use case, where we might need to have you log in again before we could log you out, which is bizarre and senseless. Avoid this nonsense.

**Move away from granular permissions**: Users currently get to pick-and-choose which permissions they grant, but this likely rarely/never works in practice and is fairly hostile since applications can't communicate which permissions they need. Applications which can actually operate with only some subset of permissions can make separate requests (e.g., when you activate "cool feature X", it asks for X permission). I think applications that do this are rare; pretty much everything just asks for tons of permissions and everyone grants them.

Making this all-or-nothing is better for well-behaved applications and better for users. It's also slightly better for overzealous applications that ask for more than they need, but whatever. Users can make an informed decision, hopefully, and I plan to let administrators force applications to a subset of permissions once we introduce meaningful scopes.

Test Plan:
  - Generated tokens.
  - Used tokens.
  - Authorized an instance.
  - Faked some bogus scopes, got clean authorization.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7303

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

src/applications/oauthserver/PhabricatorOAuthServer.php

@@ -177,19 +177,6 @@ final class PhabricatorOAuthServer extends Phobject {
       return null;
     }
 
-    // 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 $authorization;
   }
 

src/applications/oauthserver/PhabricatorOAuthServerScope.php

@@ -2,120 +2,20 @@
 
 final class PhabricatorOAuthServerScope extends Phobject {
 
-  const SCOPE_OFFLINE_ACCESS = 'offline_access';
-  const SCOPE_WHOAMI         = 'whoami';
-
-  public static function getScopesDict() {
-    return array(
-      self::SCOPE_OFFLINE_ACCESS => 1,
-      self::SCOPE_WHOAMI         => 1,
-    );
+  public static function getScopeMap() {
+    return array();
   }
 
-  public static function getDefaultScope() {
-    return self::SCOPE_WHOAMI;
-  }
+  public static function filterScope(array $scope) {
+    $valid_scopes = self::getScopeMap();
 
-  public static function getCheckboxControl(
-    array $current_scopes) {
-
-    $have_options = false;
-    $scopes = self::getScopesDict();
-    $scope_keys = array_keys($scopes);
-    sort($scope_keys);
-    $default_scope = self::getDefaultScope();
-
-    $checkboxes = new AphrontFormCheckboxControl();
-    foreach ($scope_keys as $scope) {
-      if ($scope == $default_scope) {
-        continue;
-      }
-      if (!isset($current_scopes[$scope])) {
-        continue;
-      }
-
-      $checkboxes->addCheckbox(
-        $name = $scope,
-        $value = 1,
-        $label = self::getCheckboxLabel($scope),
-        $checked = isset($current_scopes[$scope]));
-      $have_options = true;
-    }
-
-    if ($have_options) {
-      $checkboxes->setLabel(pht('Scope'));
-      return $checkboxes;
-    }
-
-    return null;
-  }
-
-  private static function getCheckboxLabel($scope) {
-    $label = null;
-    switch ($scope) {
-      case self::SCOPE_OFFLINE_ACCESS:
-        $label = pht('Make access tokens granted to this client never expire.');
-        break;
-      case self::SCOPE_WHOAMI:
-        $label = pht('Read access to Conduit method %s.', 'user.whoami');
-        break;
-    }
-
-    return $label;
-  }
-
-  public static function getScopesFromRequest(AphrontRequest $request) {
-    $scopes = self::getScopesDict();
-    $requested_scopes = array();
-    foreach ($scopes as $scope => $bit) {
-      if ($request->getBool($scope)) {
-        $requested_scopes[$scope] = 1;
+    foreach ($scope as $key => $scope_item) {
+      if (!isset($valid_scopes[$scope_item])) {
+        unset($scope[$key]);
       }
     }
-    $requested_scopes[self::getDefaultScope()] = 1;
-    return $requested_scopes;
-  }
 
-  /**
-   * A scopes list is considered valid if each scope is a known scope
-   * and each scope is seen only once. Otherwise, the list is invalid.
-   */
-  public static function validateScopesList($scope_list) {
-    $scopes       = explode(' ', $scope_list);
-    $known_scopes = self::getScopesDict();
-    $seen_scopes  = array();
-    foreach ($scopes as $scope) {
-      if (!isset($known_scopes[$scope])) {
-        return false;
-      }
-      if (isset($seen_scopes[$scope])) {
-        return false;
-      }
-      $seen_scopes[$scope] = 1;
-    }
-
-    return true;
-  }
-
-  /**
-   * A scopes dictionary is considered valid if each key is a known scope.
-   * Otherwise, the dictionary is invalid.
-   */
-  public static function validateScopesDict($scope_dict) {
-    $known_scopes   = self::getScopesDict();
-    $unknown_scopes = array_diff_key($scope_dict,
-                                     $known_scopes);
-    return empty($unknown_scopes);
-  }
-
-  /**
-   * Transforms a space-delimited scopes list into a scopes dict. The list
-   * should be validated by @{method:validateScopesList} before
-   * transformation.
-   */
-   public static function scopesListToDict($scope_list) {
-    $scopes = explode(' ', $scope_list);
-    return array_fill_keys($scopes, 1);
+    return $scope;
   }
 
 }

src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php

@@ -14,7 +14,6 @@ final class PhabricatorOAuthServerAuthController
 
     $server = new PhabricatorOAuthServer();
     $client_phid = $request->getStr('client_id');
-    $scope = $request->getStr('scope');
     $redirect_uri = $request->getStr('redirect_uri');
     $response_type = $request->getStr('response_type');
 
@@ -114,24 +113,11 @@ final class PhabricatorOAuthServerAuthController
             implode(', ', array('code'))));
       }
 
-      if ($scope) {
-        if (!PhabricatorOAuthServerScope::validateScopesList($scope)) {
-          return $this->buildErrorResponse(
-            'invalid_scope',
-            pht('Invalid Scope'),
-            pht(
-              'Request parameter %s specifies an unsupported scope.',
-              phutil_tag('strong', array(), 'scope')));
-        }
-        $scope = PhabricatorOAuthServerScope::scopesListToDict($scope);
-      } else {
-        return $this->buildErrorResponse(
-          'invalid_request',
-          pht('Malformed Request'),
-          pht(
-            'Required parameter %s was not present in the request.',
-            phutil_tag('strong', array(), 'scope')));
-      }
+
+      $requested_scope = $request->getStrList('scope');
+      $requested_scope = array_fuse($requested_scope);
+
+      $scope = PhabricatorOAuthServerScope::filterScope($requested_scope);
 
       // NOTE: We're always requiring a confirmation dialog to redirect.
       // Partly this is a general defense against redirect attacks, and
@@ -142,8 +128,6 @@ final class PhabricatorOAuthServerAuthController
       list($is_authorized, $authorization) = $auth_info;
 
       if ($request->isFormPost()) {
-        $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
-
         if ($authorization) {
           $authorization->setScope($scope)->save();
         } else {
@@ -212,16 +196,9 @@ final class PhabricatorOAuthServerAuthController
 
     // Here, we're confirming authorization for the application.
     if ($authorization) {
-      $desired_scopes = array_merge($scope, $authorization->getScope());
+      $missing_scope = array_diff_key($scope, $authorization->getScope());
     } else {
-      $desired_scopes = $scope;
-    }
-
-    if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) {
-      return $this->buildErrorResponse(
-        'invalid_scope',
-        pht('Invalid Scope'),
-        pht('The requested scope is invalid, unknown, or malformed.'));
+      $missing_scope = $scope;
     }
 
     $form = id(new AphrontFormView())
@@ -230,9 +207,7 @@ final class PhabricatorOAuthServerAuthController
       ->addHiddenInput('response_type', $response_type)
       ->addHiddenInput('state', $state)
       ->addHiddenInput('scope', $request->getStr('scope'))
-      ->setUser($viewer)
-      ->appendChild(
-        PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes));
+      ->setUser($viewer);
 
     $cancel_msg = pht('The user declined to authorize this application.');
     $cancel_uri = $this->addQueryParams(
@@ -242,7 +217,7 @@ final class PhabricatorOAuthServerAuthController
         'error_description' => $cancel_msg,
       ));
 
-    return $this->newDialog()
+    $dialog = $this->newDialog()
       ->setShortTitle(pht('Authorize Access'))
       ->setTitle(pht('Authorize "%s"?', $name))
       ->setSubmitURI($request->getRequestURI()->getPath())
@@ -253,9 +228,41 @@ final class PhabricatorOAuthServerAuthController
           'access your Phabricator account data, including your primary '.
           'email address?',
           phutil_tag('strong', array(), $name)))
-      ->appendChild($form->buildLayoutView())
+      ->appendForm($form)
       ->addSubmitButton(pht('Authorize Access'))
       ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize'));
+
+    if ($missing_scope) {
+      $dialog->appendParagraph(
+        pht(
+          'This application has requested these additional permissions. '.
+          'Authorizing it will grant it the permissions it requests:'));
+      foreach ($missing_scope as $scope_key => $ignored) {
+        // TODO: Once we introduce more scopes, explain them here.
+      }
+    }
+
+    $unknown_scope = array_diff_key($requested_scope, $scope);
+    if ($unknown_scope) {
+      $dialog->appendParagraph(
+        pht(
+          'This application also requested additional unrecognized '.
+          'permissions. These permissions may have existed in an older '.
+          'version of Phabricator, or may be from a future version of '.
+          'Phabricator. They will not be granted.'));
+
+      $unknown_form = id(new AphrontFormView())
+        ->setViewer($viewer)
+        ->appendChild(
+          id(new AphrontFormTextControl())
+            ->setLabel(pht('Unknown Scope'))
+            ->setValue(implode(', ', array_keys($unknown_scope)))
+            ->setDisabled(true));
+
+      $dialog->appendForm($unknown_form);
+    }
+
+    return $dialog;
   }
 
 

src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php

@@ -154,8 +154,7 @@ final class PhabricatorOAuthServerTokenController
       unset($unguarded);
       $result = array(
         'access_token' => $access_token->getToken(),
-        'token_type'   => 'Bearer',
-        'expires_in'   => $access_token->getExpiresDuration(),
+        'token_type' => 'Bearer',
       );
       return $response->setContent($result);
     } catch (Exception $e) {

src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php

@@ -22,18 +22,4 @@ 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();
-  }
-
 }

src/applications/people/conduit/UserWhoAmIConduitAPIMethod.php

@@ -19,7 +19,7 @@ final class UserWhoAmIConduitAPIMethod extends UserConduitAPIMethod {
   }
 
   public function getRequiredScope() {
-    return PhabricatorOAuthServerScope::SCOPE_WHOAMI;
+    return self::SCOPE_ALWAYS;
   }
 
   protected function execute(ConduitAPIRequest $request) {

src/docs/contributor/using_oauthserver.diviner

@@ -110,11 +110,7 @@ the entire `Authorization Code Grant` flow.
 NOTE: See "Scopes" section below for more information on what data is
 currently exposed through the OAuth Server.
 
-= Scopes =
+Scopes
+======
 
-There are only two scopes supported at this time.
-
-- **offline_access** - allows an access token to work indefinitely without
- expiring.
-- **whoami** - allows the client to access the results of Conduit.whoami on
- behalf of the resource owner.
+//This section has not been written yet.//