From eee8d194eb48ede0e839a0a9f828d088be565e1a Mon Sep 17 00:00:00 2001
From: Bob Trahan <btrahan@phacility.com>
Date: Fri, 6 Feb 2015 15:32:55 -0800
Subject: [PATCH] OAuthServer - default "whoami" scope and refine scope-asking
 workflow

Summary: Ref T7153. The "whoami" scope should be default and always on, because otherwise we can't do anything at all. Also, if a client doesn't want a certain scope, don't bother asking the user for it. To get there, had to add "scope" to the definition of a client.

Test Plan: applied the patch to a phabricator "client" and a phabricator "server" as far as oauth shenanigans go. Then I tried to login / register with oauth. If the "client" was configured to ask for "always on" access I got that in the dialogue, and otherwise no additional scope questions were present. Verified scope was properly granted in either case.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7153

Differential Revision: https://secure.phabricator.com/D11705
---
 .../PhabricatorOAuthServerScope.php           | 27 ++++++++++++--
 .../PhabricatorOAuthServerAuthController.php  | 37 +++++++++----------
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/src/applications/oauthserver/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/PhabricatorOAuthServerScope.php
index be3b066a6e..38707fa3c3 100644
--- a/src/applications/oauthserver/PhabricatorOAuthServerScope.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServerScope.php
@@ -18,22 +18,42 @@ final class PhabricatorOAuthServerScope {
     );
   }
 
-  static public function getCheckboxControl($current_scopes) {
+  static public function getDefaultScope() {
+    return self::SCOPE_WHOAMI;
+  }
+
+  static public 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;
     }
-    $checkboxes->setLabel('Scope');
 
-    return $checkboxes;
+    if ($have_options) {
+      $checkboxes->setLabel(pht('Scope'));
+      return $checkboxes;
+    }
+
+    return null;
   }
 
   static private function getCheckboxLabel($scope) {
@@ -58,6 +78,7 @@ final class PhabricatorOAuthServerScope {
         $requested_scopes[$scope] = 1;
       }
     }
+    $requested_scopes[self::getDefaultScope()] = 1;
     return $requested_scopes;
   }
 
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
index e38d276b93..c95aaaa823 100644
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@@ -13,7 +13,7 @@ final class PhabricatorOAuthServerAuthController
 
     $server        = new PhabricatorOAuthServer();
     $client_phid   = $request->getStr('client_id');
-    $scope         = $request->getStr('scope', array());
+    $scope         = $request->getStr('scope');
     $redirect_uri  = $request->getStr('redirect_uri');
     $response_type = $request->getStr('response_type');
 
@@ -119,6 +119,13 @@ final class PhabricatorOAuthServerAuthController
               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')));
       }
 
       // NOTE: We're always requiring a confirmation dialog to redirect.
@@ -130,7 +137,6 @@ final class PhabricatorOAuthServerAuthController
       list($is_authorized, $authorization) = $auth_info;
 
       if ($request->isFormPost()) {
-        // TODO: We should probably validate this more? It feels a little funky.
         $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
 
         if ($authorization) {
@@ -194,25 +200,16 @@ final class PhabricatorOAuthServerAuthController
     }
 
     // Here, we're confirming authorization for the application.
-
-    if ($scope) {
-      if ($authorization) {
-        $desired_scopes = array_merge($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.'));
-      }
+    if ($authorization) {
+      $desired_scopes = array_merge($scope, $authorization->getScope());
     } else {
-      $desired_scopes = array(
-        PhabricatorOAuthServerScope::SCOPE_WHOAMI         => 1,
-        PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1,
-      );
+      $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.'));
     }
 
     $form = id(new AphrontFormView())