From 8f2c426ff2c3d8485a95aee0ffc56fd5739a31e8 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Mon, 5 Mar 2012 13:27:20 -0800 Subject: [PATCH] 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=" 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 --- .../PhabricatorOAuthServerAuthController.php | 24 +++++++++++++------ .../PhabricatorOAuthClientEditController.php | 2 ++ .../scope/PhabricatorOAuthServerScope.php | 10 ++++++-- .../server/PhabricatorOAuthServer.php | 8 +++++-- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php index 1fc10c09ef..373c9e167c 100644 --- a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php +++ b/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()) diff --git a/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php b/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php index 9d3063274b..ab3201636e 100644 --- a/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php +++ b/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); diff --git a/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php index d57905a091..2a34f3675c 100644 --- a/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php +++ b/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); } } diff --git a/src/applications/oauthserver/server/PhabricatorOAuthServer.php b/src/applications/oauthserver/server/PhabricatorOAuthServer.php index ea9d15859c..6f4de14fd7 100644 --- a/src/applications/oauthserver/server/PhabricatorOAuthServer.php +++ b/src/applications/oauthserver/server/PhabricatorOAuthServer.php @@ -84,6 +84,7 @@ final class PhabricatorOAuthServer { /** * @task auth + * @return tuple */ 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); } /**