Fix OAuth Client Authorization bugs

Summary: ajtrichards reported an error creating a brand new authorization. fixed that and generally made this flow work well

Test Plan:
- created a fresh test client
-- noted "new=<PHID>" with appropriate highlighting
- visited http://phabricator.dev/oauthserver/auth/?client_id=PHID-OASC-jwgdrqdpzomtxyg3q3yf&response_type=code&scope=offline_access
-- clicked "cancel", verified result
-- clicked "approve", verfied result
- visited http://phabricator.dev/oauthserver/auth/?client_id=PHID-OASC-jwgdrqdpzomtxyg3q3yf&response_type=code&scope=whoami
-- noted got the dialog
-- noted that it had the union of desired and existing so user could update it properly! (NB - its up to the client to react to whatever specific scope(s) the user decides to grant)
-- noted it actually updated when I hit "approve"

Reviewers: epriestley, ajtrichards

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T933

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

src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php

@@ -115,13 +115,18 @@ extends PhabricatorAuthController {
         $scope = PhabricatorOAuthServerScope::scopesListToDict($scope);
       }
 
-      $authorization = $server->userHasAuthorizedClient($scope);
-      if ($authorization) {
+      list($is_authorized,
+           $authorization) = $server->userHasAuthorizedClient($scope);
+      if ($is_authorized) {
         $return_auth_code = true;
         $unguarded_write  = AphrontWriteGuard::beginScopedUnguardedWrites();
       } else if ($request->isFormPost()) {
         $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
-        $authorization    = $server->authorizeClient($scope);
+        if ($authorization) {
+          $authorization->setScope($scope)->save();
+        } else {
+          $authorization  = $server->authorizeClient($scope);
+        }
         $return_auth_code = true;
         $unguarded_write  = null;
       } else {
@@ -140,7 +145,7 @@ extends PhabricatorAuthController {
           'scope' => $authorization->getScopeString(),
         );
         $response->setContent($content);
-        return $response->setClientURI($uri);
+        return $response;
       }
       unset($unguarded_write);
     } catch (Exception $e) {
@@ -167,7 +172,12 @@ extends PhabricatorAuthController {
       "Phabricator account data?";
 
     if ($scope) {
-      $desired_scopes = $scope;
+      if ($authorization) {
+        $desired_scopes = array_merge($scope,
+                                      $authorization->getScope());
+      } else {
+        $desired_scopes = $scope;
+      }
       if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) {
         $response->setError('invalid_scope');
         $response->setErrorDescription(
@@ -182,7 +192,7 @@ extends PhabricatorAuthController {
       );
     }
 
-    $cancel_uri = $this->getClientURI($client, $redirect_uri);
+    $cancel_uri = clone $uri;
     $cancel_params = array(
       'error' => 'access_denied',
       'error_description' =>
@@ -197,7 +207,7 @@ extends PhabricatorAuthController {
         ->setValue($description)
       )
       ->appendChild(
-        PhabricatorOAuthServerScope::getCheckboxControl()
+        PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes)
       )
       ->appendChild(
         id(new AphrontFormSubmitControl())

src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php

@@ -109,6 +109,8 @@ extends PhabricatorOAuthClientBaseController {
         $bad_redirect = true;
       } else {
         $client->save();
+        // refresh the phid in case its a create
+        $phid = $client->getPHID();
         if ($this->isClientEdit()) {
           return id(new AphrontRedirectResponse())
             ->setURI('/oauthserver/client/?edited='.$phid);

src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php

@@ -110,8 +110,14 @@ final class PhabricatorOAuthServerScope {
     return empty($unknown_scopes);
   }
 
-  static public function scopesListToDict($scope_list) {
-    return array_fill_keys($scope_list, 1);
+  /**
+   * Transforms a space-delimited scopes list into a scopes dict. The list
+   * should be validated by @{method:validateScopesList} before
+   * transformation.
+   */
+   static public function scopesListToDict($scope_list) {
+    $scopes = explode(' ', $scope_list);
+    return array_fill_keys($scopes, 1);
   }
 
 }

src/applications/oauthserver/server/PhabricatorOAuthServer.php

@@ -84,6 +84,7 @@ final class PhabricatorOAuthServer {
 
   /**
    * @task auth
+   * @return tuple <bool hasAuthorized, ClientAuthorization or null>
    */
   public function userHasAuthorizedClient(array $scope) {
 
@@ -91,6 +92,9 @@ final class PhabricatorOAuthServer {
       loadOneWhere('userPHID = %s AND clientPHID = %s',
                    $this->getUser()->getPHID(),
                    $this->getClient()->getPHID());
+    if (empty($authorization)) {
+      return array(false, null);
+    }
 
     if ($scope) {
       $missing_scope = array_diff_key($scope,
@@ -100,10 +104,10 @@ final class PhabricatorOAuthServer {
     }
 
     if ($missing_scope) {
-      return false;
+      return array(false, $authorization);
     }
 
-    return $authorization;
+    return array(true, $authorization);
   }
 
   /**