From 0327a5fc69ae10563c18300668a94f7b8431ab7e Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 1 Mar 2012 14:46:18 -0800 Subject: [PATCH] OAuthServer polish and random sauce Summary: This diff makes the OAuthServer more compliant with the spec by - making it return well-formatted error codes with error types from the spec. - making it respect the "state" variable, which is a transparent variable the client passes and the server passes back - making it be super, duper compliant with respect to redirect uris -- if specified in authorization step, check if its valid relative to the client registered URI and if so save it -- if specified in authorization step, check if its been specified in the access step and error if it doesn't match or doesn't exist -- note we don't make any use of it in the access step which seems strange but hey, that's what the spec says! This diff makes the OAuthServer suck less by - making the "cancel" button do something in the user authorization flow - making the client list view and client edit view be a bit more usable around client secrets - fixing a few bugs I managed to introduce along the way Test Plan: - create a test phabricator client, updated my conf, and then linked and unlinked phabricator to itself - wrote some tests for PhabricatorOAuthServer -- they pass! -- these validate the various validate URI checks - tried a few important authorization calls -- http://phabricator.dev/oauthserver/auth/?client_id=X&state=test&redirect_uri=http://www.evil.com --- verified error'd from mismatching redirect uri's --- verified state parameter in response --- verified did not redirect to client redirect uri -- http://phabricator.dev/oauthserver/auth/?client_id=X w/ existing authorization --- got redirected to proper client url with error that response_type not specified -- http://phabricator.dev/oauthserver/auth/?client_id=X&response_type=code w/ existing authorization --- got redirected to proper client url with pertinent code! - tried a few important access calls -- verified appropriate errors if missing any required parameters -- verified good access code with appropriate other variables resulted in an access token - verified that if redirect_uri set correctly in authorization required for access and errors if differs at all / only succeeds if exactly the same Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley, ajtrichards Maniphest Tasks: T889, T906, T897 Differential Revision: https://secure.phabricator.com/D1727 --- .../112.oauthaccesscoderedirecturi.sql | 3 + src/__phutil_library_map__.php | 2 + src/aphront/response/base/AphrontResponse.php | 3 + .../PhabricatorOAuthProviderPhabricator.php | 15 +- .../PhabricatorOAuthServerAuthController.php | 214 ++++++++++++------ .../oauthserver/controller/auth/__init__.php | 1 - .../PhabricatorOAuthClientEditController.php | 29 ++- .../controller/client/edit/__init__.php | 2 +- .../PhabricatorOAuthClientListController.php | 3 + .../PhabricatorOAuthServerTokenController.php | 148 ++++++++---- .../oauthserver/controller/token/__init__.php | 1 + .../response/PhabricatorOAuthResponse.php | 94 +++++--- .../scope/PhabricatorOAuthServerScope.php | 36 +++ .../server/PhabricatorOAuthServer.php | 54 ++++- .../oauthserver/server/__init__.php | 1 + .../PhabricatorOAuthServerTestCase.php | 85 +++++++ .../oauthserver/server/__tests__/__init__.php | 15 ++ ...habricatorOAuthServerAuthorizationCode.php | 1 + .../PhabricatorOAuthClientAuthorization.php | 2 +- 19 files changed, 537 insertions(+), 172 deletions(-) create mode 100644 resources/sql/patches/112.oauthaccesscoderedirecturi.sql create mode 100644 src/applications/oauthserver/server/__tests__/PhabricatorOAuthServerTestCase.php create mode 100644 src/applications/oauthserver/server/__tests__/__init__.php diff --git a/resources/sql/patches/112.oauthaccesscoderedirecturi.sql b/resources/sql/patches/112.oauthaccesscoderedirecturi.sql new file mode 100644 index 0000000000..f151c438ad --- /dev/null +++ b/resources/sql/patches/112.oauthaccesscoderedirecturi.sql @@ -0,0 +1,3 @@ +ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthserverauthorizationcode` + ADD `redirectURI` varchar(255) NOT NULL + diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index da22898fe2..bdd1fdebe0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -645,6 +645,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerController' => 'applications/oauthserver/controller/base', 'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/base', 'PhabricatorOAuthServerScope' => 'applications/oauthserver/scope', + 'PhabricatorOAuthServerTestCase' => 'applications/oauthserver/server/__tests__', 'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/test', 'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/token', 'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink', @@ -1408,6 +1409,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerClient' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerController' => 'PhabricatorController', 'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO', + 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', 'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController', 'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController', diff --git a/src/aphront/response/base/AphrontResponse.php b/src/aphront/response/base/AphrontResponse.php index f3bdc74674..437fb9df85 100644 --- a/src/aphront/response/base/AphrontResponse.php +++ b/src/aphront/response/base/AphrontResponse.php @@ -114,6 +114,9 @@ abstract class AphrontResponse { $headers[] = array( 'Cache-Control', 'private, no-cache, no-store, must-revalidate'); + $headers[] = array( + 'Pragma', + 'no-cache'); $headers[] = array( 'Expires', 'Sat, 01 Jan 2000 00:00:00 GMT'); diff --git a/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php b/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php index 1d011a64b3..d5954d27e3 100644 --- a/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php +++ b/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php @@ -18,9 +18,20 @@ final class PhabricatorOAuthProviderPhabricator extends PhabricatorOAuthProvider { - private $userData; + public function getExtraAuthParameters() { + return array( + 'response_type' => 'code', + ); + } + + public function getExtraTokenParameters() { + return array( + 'grant_type' => 'authorization_code', + ); + + } public function decodeTokenResponse($response) { $decoded = json_decode($response, true); if (!is_array($decoded)) { @@ -85,7 +96,7 @@ extends PhabricatorOAuthProvider { } public function getMinimumScope() { - return 'email'; + return 'whoami'; } public function setUserData($data) { diff --git a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php index 836bfa9d13..1fc10c09ef 100644 --- a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php @@ -27,78 +27,135 @@ extends PhabricatorAuthController { } public function processRequest() { - $request = $this->getRequest(); - $current_user = $request->getUser(); - $server = new PhabricatorOAuthServer($current_user); - $client_phid = $request->getStr('client_id'); - $scope = $request->getStr('scope'); - $redirect_uri = $request->getStr('redirect_uri'); - $response = new PhabricatorOAuthResponse(); - $errors = array(); + $request = $this->getRequest(); + $current_user = $request->getUser(); + $server = new PhabricatorOAuthServer(); + $client_phid = $request->getStr('client_id'); + $scope = $request->getStr('scope'); + $redirect_uri = $request->getStr('redirect_uri'); + $state = $request->getStr('state'); + $response_type = $request->getStr('response_type'); + $response = new PhabricatorOAuthResponse(); + // state is an opaque value the client sent us for their own purposes + // we just need to send it right back to them in the response! + if ($state) { + $response->setState($state); + } if (!$client_phid) { - return $response->setMalformed( + $response->setError('invalid_request'); + $response->setErrorDescription( 'Required parameter client_id not specified.' ); + return $response; } - $client = id(new PhabricatorOAuthServerClient()) - ->loadOneWhere('phid = %s', $client_phid); - if (!$client) { - return $response->setNotFound( - 'Client with id '.$client_phid.' not found. ' - ); - } + $server->setUser($current_user); - $server->setClient($client); - if ($server->userHasAuthorizedClient()) { - $return_auth_code = true; - $unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites(); - } else if ($request->isFormPost()) { - $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request); - $server->authorizeClient($scope); - $return_auth_code = true; - $unguarded_write = null; - } else { - $return_auth_code = false; - $unguarded_write = null; - } - - if ($return_auth_code) { - // step 1 -- generate authorization code - $auth_code = - $server->generateAuthorizationCode(); - - // step 2 -- error or return it - if (!$auth_code) { - $errors[] = 'Failed to generate an authorization code. '. - 'Try again.'; - } else { - $client_uri = new PhutilURI($client->getRedirectURI()); - if (!$redirect_uri) { - $uri = $client_uri; - } else { - $redirect_uri = new PhutilURI($redirect_uri); - if ($redirect_uri->getDomain() != - $client_uri->getDomain()) { - $uri = $client_uri; - } else { - $uri = $redirect_uri; - } - } - - $uri->setQueryParam('code', $auth_code->getCode()); - return $response->setRedirect($uri); + // one giant try / catch around all the exciting database stuff so we + // can return a 'server_error' response if something goes wrong! + try { + $client = id(new PhabricatorOAuthServerClient()) + ->loadOneWhere('phid = %s', $client_phid); + if (!$client) { + $response->setError('invalid_request'); + $response->setErrorDescription( + 'Client with id '.$client_phid.' not found.' + ); + return $response; } - } - unset($unguarded_write); + $server->setClient($client); + if ($redirect_uri) { + $client_uri = new PhutilURI($client->getRedirectURI()); + $redirect_uri = new PhutilURI($redirect_uri); + if (!($server->validateSecondaryRedirectURI($redirect_uri, + $client_uri))) { + $response->setError('invalid_request'); + $response->setErrorDescription( + 'The specified redirect URI is invalid. The redirect URI '. + 'must be a fully-qualified domain with no fragments and '. + 'must have the same domain and at least the same query '. + 'parameters as the redirect URI the client registered.' + ); + return $response; + } + $uri = $redirect_uri; + $access_token_uri = $uri; + } else { + $uri = new PhutilURI($client->getRedirectURI()); + $access_token_uri = null; + } + // we've now validated this request enough overall such that we + // can safely redirect to the client with the response + $response->setClientURI($uri); - $error_view = null; - if ($errors) { - $error_view = new AphrontErrorView(); - $error_view->setTitle('Authorization Code Errors'); - $error_view->setErrors($errors); + if (empty($response_type)) { + $response->setError('invalid_request'); + $response->setErrorDescription( + 'Required parameter response_type not specified.' + ); + return $response; + } + if ($response_type != 'code') { + $response->setError('unsupported_response_type'); + $response->setErrorDescription( + 'The authorization server does not support obtaining an '. + 'authorization code using the specified response_type. '. + 'You must specify the response_type as "code".' + ); + return $response; + } + if ($scope) { + if (!PhabricatorOAuthServerScope::validateScopesList($scope)) { + $response->setError('invalid_scope'); + $response->setErrorDescription( + 'The requested scope is invalid, unknown, or malformed.' + ); + return $response; + } + $scope = PhabricatorOAuthServerScope::scopesListToDict($scope); + } + + $authorization = $server->userHasAuthorizedClient($scope); + if ($authorization) { + $return_auth_code = true; + $unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites(); + } else if ($request->isFormPost()) { + $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request); + $authorization = $server->authorizeClient($scope); + $return_auth_code = true; + $unguarded_write = null; + } else { + $return_auth_code = false; + $unguarded_write = null; + } + + if ($return_auth_code) { + // step 1 -- generate authorization code + $auth_code = + $server->generateAuthorizationCode($access_token_uri); + + // step 2 return it + $content = array( + 'code' => $auth_code->getCode(), + 'scope' => $authorization->getScopeString(), + ); + $response->setContent($content); + return $response->setClientURI($uri); + } + unset($unguarded_write); + } catch (Exception $e) { + // Note we could try harder to determine between a server_error + // vs temporarily_unavailable. Good enough though. + $response->setError('server_error'); + $response->setErrorDescription( + 'The authorization server encountered an unexpected condition '. + 'which prevented it from fulfilling the request. ' + ); + return $response; } + // display time -- make a nice form for the user to grant the client + // access to the granularity specified by $scope $name = phutil_escape_html($client->getName()); $title = 'Authorize ' . $name . '?'; $panel = new AphrontPanelView(); @@ -109,10 +166,30 @@ extends PhabricatorAuthController { "Do want to authorize {$name} to access your ". "Phabricator account data?"; - $desired_scopes = array( - PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1, - PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1 + if ($scope) { + $desired_scopes = $scope; + if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { + $response->setError('invalid_scope'); + $response->setErrorDescription( + 'The requested scope is invalid, unknown, or malformed.' + ); + return $response; + } + } else { + $desired_scopes = array( + PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1, + PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1 + ); + } + + $cancel_uri = $this->getClientURI($client, $redirect_uri); + $cancel_params = array( + 'error' => 'access_denied', + 'error_description' => + 'The resource owner (aka the user) denied the request.' ); + $cancel_uri->setQueryParams($cancel_params); + $form = id(new AphrontFormView()) ->setUser($current_user) ->appendChild( @@ -125,15 +202,14 @@ extends PhabricatorAuthController { ->appendChild( id(new AphrontFormSubmitControl()) ->setValue('Authorize') - ->addCancelButton('/') + ->addCancelButton($cancel_uri) ); - // TODO -- T889 (make "cancel" do something more sensible) $panel->appendChild($form); return $this->buildStandardPageResponse( - array($error_view, - $panel), + $panel, array('title' => $title)); } + } diff --git a/src/applications/oauthserver/controller/auth/__init__.php b/src/applications/oauthserver/controller/auth/__init__.php index 8a0cd0c8a6..3ab5ab427a 100644 --- a/src/applications/oauthserver/controller/auth/__init__.php +++ b/src/applications/oauthserver/controller/auth/__init__.php @@ -15,7 +15,6 @@ phutil_require_module('phabricator', 'applications/oauthserver/storage/client'); phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); -phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phutil', 'markup'); diff --git a/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php b/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php index 20a71b0880..9d3063274b 100644 --- a/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php +++ b/src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php @@ -78,25 +78,33 @@ extends PhabricatorOAuthClientBaseController { ->setForbiddenText($message); } $submit_button = 'Save OAuth Client'; + $secret = null; // new client - much simpler } else { - $client = new PhabricatorOAuthServerClient(); - $title = 'Create OAuth Client'; + $client = new PhabricatorOAuthServerClient(); + $title = 'Create OAuth Client'; $submit_button = 'Create OAuth Client'; + $secret = Filesystem::readRandomCharacters(32); } if ($request->isFormPost()) { $redirect_uri = $request->getStr('redirect_uri'); $client->setName($request->getStr('name')); $client->setRedirectURI($redirect_uri); - $client->setSecret(Filesystem::readRandomCharacters(32)); + if ($secret) { + $client->setSecret($secret); + } $client->setCreatorPHID($current_user->getPHID()); $uri = new PhutilURI($redirect_uri); - if (!$this->validateRedirectURI($uri)) { + $server = new PhabricatorOAuthServer(); + if (!$server->validateRedirectURI($uri)) { $error = new AphrontErrorView(); $error->setSeverity(AphrontErrorView::SEVERITY_ERROR); $error->setTitle( - 'Redirect URI must be a fully qualified domain name.' + 'Redirect URI must be a fully qualified domain name '. + 'with no fragments. See '. + 'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 '. + 'for more information on the correct format.' ); $bad_redirect = true; } else { @@ -140,7 +148,7 @@ extends PhabricatorOAuthClientBaseController { ->setValue($phid) ) ->appendChild( - id(new AphrontFormTextControl()) + id(new AphrontFormStaticControl()) ->setLabel('Secret') ->setValue($client->getSecret()) ); @@ -185,13 +193,4 @@ extends PhabricatorOAuthClientBaseController { ); } - private function validateRedirectURI(PhutilURI $uri) { - if (PhabricatorEnv::isValidRemoteWebResource($uri)) { - if ($uri->getDomain()) { - return true; - } - } - return false; - } - } diff --git a/src/applications/oauthserver/controller/client/edit/__init__.php b/src/applications/oauthserver/controller/client/edit/__init__.php index d7ec2ce1bb..869cec1bb1 100644 --- a/src/applications/oauthserver/controller/client/edit/__init__.php +++ b/src/applications/oauthserver/controller/client/edit/__init__.php @@ -10,8 +10,8 @@ phutil_require_module('phabricator', 'aphront/response/403'); phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/oauthserver/controller/client/base'); +phutil_require_module('phabricator', 'applications/oauthserver/server'); phutil_require_module('phabricator', 'applications/oauthserver/storage/client'); -phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); diff --git a/src/applications/oauthserver/controller/client/list/PhabricatorOAuthClientListController.php b/src/applications/oauthserver/controller/client/list/PhabricatorOAuthClientListController.php index 4ab49702eb..43b389f51d 100644 --- a/src/applications/oauthserver/controller/client/list/PhabricatorOAuthClientListController.php +++ b/src/applications/oauthserver/controller/client/list/PhabricatorOAuthClientListController.php @@ -47,6 +47,7 @@ extends PhabricatorOAuthClientBaseController { phutil_escape_html($client->getName()) ), $client->getPHID(), + $client->getSecret(), phutil_render_tag( 'a', array( @@ -88,6 +89,7 @@ extends PhabricatorOAuthClientBaseController { array( 'Client', 'ID', + 'Secret', 'Redirect URI', '', )); @@ -96,6 +98,7 @@ extends PhabricatorOAuthClientBaseController { '', '', '', + '', 'action', )); if (empty($rows)) { diff --git a/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php index d61ba06f96..7bc06b8785 100644 --- a/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php @@ -28,77 +28,133 @@ extends PhabricatorAuthController { public function processRequest() { $request = $this->getRequest(); + $grant_type = $request->getStr('grant_type'); $code = $request->getStr('code'); + $redirect_uri = $request->getStr('redirect_uri'); $client_phid = $request->getStr('client_id'); $client_secret = $request->getStr('client_secret'); $response = new PhabricatorOAuthResponse(); $server = new PhabricatorOAuthServer(); + if ($grant_type != 'authorization_code') { + $response->setError('unsupported_grant_type'); + $response->setErrorDescription( + 'Only grant_type authorization_code is supported.' + ); + return $response; + } if (!$code) { - return $response->setMalformed( + $response->setError('invalid_request'); + $response->setErrorDescription( 'Required parameter code missing.' ); + return $response; } if (!$client_phid) { - return $response->setMalformed( + $response->setError('invalid_request'); + $response->setErrorDescription( 'Required parameter client_id missing.' ); + return $response; } if (!$client_secret) { - return $response->setMalformed( + $response->setError('invalid_request'); + $response->setErrorDescription( 'Required parameter client_secret missing.' ); + return $response; } + // one giant try / catch around all the exciting database stuff so we + // can return a 'server_error' response if something goes wrong! + try { + $auth_code = id(new PhabricatorOAuthServerAuthorizationCode()) + ->loadOneWhere('code = %s', + $code); + if (!$auth_code) { + $response->setError('invalid_grant'); + $response->setErrorDescription( + 'Authorization code '.$code.' not found.' + ); + return $response; + } - $client = id(new PhabricatorOAuthServerClient()) - ->loadOneWhere('phid = %s', $client_phid); - if (!$client) { - return $response->setNotFound( - 'Client with client_id '.$client_phid.' not found.' - ); - } - $server->setClient($client); + // if we have an auth code redirect URI, there must be a redirect_uri + // in the request and it must match the auth code redirect uri *exactly* + $auth_code_redirect_uri = $auth_code->getRedirectURI(); + if ($auth_code_redirect_uri) { + $auth_code_redirect_uri = new PhutilURI($auth_code_redirect_uri); + $redirect_uri = new PhutilURI($redirect_uri); + if (!$redirect_uri->getDomain() || + $redirect_uri != $auth_code_redirect_uri) { + $response->setError('invalid_grant'); + $response->setErrorDescription( + 'Redirect uri in request must exactly match redirect uri '. + 'from authorization code.' + ); + return $response; + } + } else if ($redirect_uri) { + $response->setError('invalid_grant'); + $response->setErrorDescription( + 'Redirect uri in request and no redirect uri in authorization '. + 'code. The two must exactly match.' + ); + return $response; + } - $auth_code = id(new PhabricatorOAuthServerAuthorizationCode()) - ->loadOneWhere('code = %s', $code); - if (!$auth_code) { - return $response->setNotFound( - 'Authorization code '.$code.' not found.' - ); - } + $client = id(new PhabricatorOAuthServerClient()) + ->loadOneWhere('phid = %s', + $client_phid); + if (!$client) { + $response->setError('invalid_client'); + $response->setErrorDescription( + 'Client with client_id '.$client_phid.' not found.' + ); + return $response; + } + $server->setClient($client); - $user_phid = $auth_code->getUserPHID(); - $user = id(new PhabricatorUser()) - ->loadOneWhere('phid = %s', $user_phid); - if (!$user) { - return $response->setNotFound( - 'User with phid '.$user_phid.' not found.' - ); - } - $server->setUser($user); + $user_phid = $auth_code->getUserPHID(); + $user = id(new PhabricatorUser()) + ->loadOneWhere('phid = %s', $user_phid); + if (!$user) { + $response->setError('invalid_grant'); + $response->setErrorDescription( + 'User with phid '.$user_phid.' not found.' + ); + return $response; + } + $server->setUser($user); - $test_code = new PhabricatorOAuthServerAuthorizationCode(); - $test_code->setClientSecret($client_secret); - $test_code->setClientPHID($client_phid); - $is_good_code = $server->validateAuthorizationCode($auth_code, - $test_code); - if (!$is_good_code) { - return $response->setMalformed( - 'Invalid authorization code '.$code.'.' - ); - } + $test_code = new PhabricatorOAuthServerAuthorizationCode(); + $test_code->setClientSecret($client_secret); + $test_code->setClientPHID($client_phid); + $is_good_code = $server->validateAuthorizationCode($auth_code, + $test_code); + if (!$is_good_code) { + $response->setError('invalid_grant'); + $response->setErrorDescription( + 'Invalid authorization code '.$code.'.' + ); + return $response; + } - $scope = AphrontWriteGuard::beginScopedUnguardedWrites(); - $access_token = $server->generateAccessToken(); - if ($access_token) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $access_token = $server->generateAccessToken(); $auth_code->delete(); + unset($unguarded); $result = array( - 'access_token' => $access_token->getToken(), - 'token_type' => 'Bearer', - 'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT, - ); + 'access_token' => $access_token->getToken(), + 'token_type' => 'Bearer', + 'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT, + ); return $response->setContent($result); + } catch (Exception $e) { + $response->setError('server_error'); + $response->setErrorDescription( + 'The authorization server encountered an unexpected condition '. + 'which prevented it from fulfilling the request.' + ); + return $response; } - - return $response->setMalformed('Request is malformed in some way.'); } } diff --git a/src/applications/oauthserver/controller/token/__init__.php b/src/applications/oauthserver/controller/token/__init__.php index e7cb9e94a3..1ac94adb82 100644 --- a/src/applications/oauthserver/controller/token/__init__.php +++ b/src/applications/oauthserver/controller/token/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/oauthserver/storage/authoriza phutil_require_module('phabricator', 'applications/oauthserver/storage/client'); phutil_require_module('phabricator', 'applications/people/storage/user'); +phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/oauthserver/response/PhabricatorOAuthResponse.php b/src/applications/oauthserver/response/PhabricatorOAuthResponse.php index 6f01c4b0d2..49f574d47b 100644 --- a/src/applications/oauthserver/response/PhabricatorOAuthResponse.php +++ b/src/applications/oauthserver/response/PhabricatorOAuthResponse.php @@ -21,52 +21,68 @@ */ final class PhabricatorOAuthResponse extends AphrontResponse { + private $state; private $content; - private $uri; + private $clientURI; + private $error; + private $errorDescription; + private function getState() { + return $this->state; + } + public function setState($state) { + $this->state = $state; + return $this; + } + + private function getContent() { + return $this->content; + } public function setContent($content) { $this->content = $content; return $this; } - private function getContent() { - return $this->content; - } - private function setURI($uri) { - $this->uri = $uri; + private function getClientURI() { + return $this->clientURI; + } + public function setClientURI(PhutilURI $uri) { + $this->setHTTPResponseCode(302); + $this->clientURI = $uri; return $this; } - private function getURI() { - return $this->uri; + private function getFullURI() { + $base_uri = $this->getClientURI(); + $query_params = $this->buildResponseDict(); + foreach ($query_params as $key => $value) { + $base_uri->setQueryParam($key, $value); + } + return $base_uri; } - public function setMalformed($malformed) { - if ($malformed) { + private function getError() { + return $this->error; + } + public function setError($error) { + // errors sometimes redirect to the client (302) but otherwise + // the spec says all code 400 + if (!$this->getClientURI()) { $this->setHTTPResponseCode(400); - $this->setContent(array('error' => $malformed)); } + $this->error = $error; return $this; } - public function setNotFound($not_found) { - if ($not_found) { - $this->setHTTPResponseCode(404); - $this->setContent(array('error' => $not_found)); - } - return $this; + private function getErrorDescription() { + return $this->errorDescription; } - - public function setRedirect(PhutilURI $uri) { - if ($uri) { - $this->setHTTPResponseCode(302); - $this->setURI($uri); - $this->setContent(null); - } + public function setErrorDescription($error_description) { + $this->errorDescription = $error_description; return $this; } public function __construct() { - $this->setHTTPResponseCode(200); + $this->setHTTPResponseCode(200); // assume the best return $this; } @@ -74,20 +90,34 @@ final class PhabricatorOAuthResponse extends AphrontResponse { $headers = array( array('Content-Type', 'application/json'), ); - if ($this->getURI()) { - $headers[] = array('Location', $this->getURI()); + if ($this->getClientURI()) { + $headers[] = array('Location', $this->getFullURI()); } // TODO -- T844 set headers with X-Auth-Scopes, etc $headers = array_merge(parent::getHeaders(), $headers); return $headers; } - public function buildResponseString() { - $content = $this->getContent(); - if ($content) { - return $this->encodeJSONForHTTPResponse($content); + private function buildResponseDict() { + if ($this->getError()) { + $content = array( + 'error' => $this->getError(), + 'error_description' => $this->getErrorDescription(), + ); + $this->setContent($content); } - return ''; + + $content = $this->getContent(); + if (!$content) { + return ''; + } + if ($this->getState()) { + $content['state'] = $this->getState(); + } + return $content; } + public function buildResponseString() { + return $this->encodeJSONForHTTPResponse($this->buildResponseDict()); + } } diff --git a/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php index 22246549ab..d57905a091 100644 --- a/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php +++ b/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php @@ -78,4 +78,40 @@ final class PhabricatorOAuthServerScope { 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. + */ + static public 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. + */ + static public function validateScopesDict($scope_dict) { + $known_scopes = self::getScopesDict(); + $unknown_scopes = array_diff_key($scope_dict, + $known_scopes); + return empty($unknown_scopes); + } + + static public function scopesListToDict($scope_list) { + return array_fill_keys($scope_list, 1); + } + } diff --git a/src/applications/oauthserver/server/PhabricatorOAuthServer.php b/src/applications/oauthserver/server/PhabricatorOAuthServer.php index d60e3c4323..ea9d15859c 100644 --- a/src/applications/oauthserver/server/PhabricatorOAuthServer.php +++ b/src/applications/oauthserver/server/PhabricatorOAuthServer.php @@ -85,18 +85,25 @@ final class PhabricatorOAuthServer { /** * @task auth */ - public function userHasAuthorizedClient() { + public function userHasAuthorizedClient(array $scope) { $authorization = id(new PhabricatorOAuthClientAuthorization())-> loadOneWhere('userPHID = %s AND clientPHID = %s', $this->getUser()->getPHID(), - $this->getClient->getPHID()); + $this->getClient()->getPHID()); - if (empty($authorization)) { + if ($scope) { + $missing_scope = array_diff_key($scope, + $authorization->getScope()); + } else { + $missing_scope = false; + } + + if ($missing_scope) { return false; } - return true; + return $authorization; } /** @@ -115,7 +122,7 @@ final class PhabricatorOAuthServer { /** * @task auth */ - public function generateAuthorizationCode() { + public function generateAuthorizationCode(PhutilURI $redirect_uri) { $code = Filesystem::readRandomCharacters(32); $client = $this->getClient(); @@ -125,6 +132,7 @@ final class PhabricatorOAuthServer { $authorization_code->setClientPHID($client->getPHID()); $authorization_code->setClientSecret($client->getSecret()); $authorization_code->setUserPHID($this->getUser()->getPHID()); + $authorization_code->setRedirectURI((string) $redirect_uri); $authorization_code->save(); return $authorization_code; @@ -191,6 +199,7 @@ final class PhabricatorOAuthServer { return false; } + $valid = true; if ($expired) { $valid = false; // check if the scope includes "offline_access", which makes the @@ -205,4 +214,39 @@ final class PhabricatorOAuthServer { return $valid; } + /** + * See http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 + * for details on what makes a given redirect URI "valid". + */ + public function validateRedirectURI(PhutilURI $uri) { + if (PhabricatorEnv::isValidRemoteWebResource($uri)) { + if ($uri->getFragment()) { + return false; + } + if ($uri->getDomain()) { + return true; + } + } + return false; + } + + /** + * If there's a URI specified in an OAuth request, it must be validated in + * its own right. Further, it must have the same domain and (at least) the + * same query parameters as the primary URI. + */ + public function validateSecondaryRedirectURI(PhutilURI $secondary_uri, + PhutilURI $primary_uri) { + $valid = $this->validateRedirectURI($secondary_uri); + if ($valid) { + $valid_domain = ($secondary_uri->getDomain() == + $primary_uri->getDomain()); + $good_params = $primary_uri->getQueryParams(); + $test_params = $secondary_uri->getQueryParams(); + $missing_params = array_diff_key($good_params, $test_params); + $valid = $valid_domain && empty($missing_params); + } + return $valid; + } + } diff --git a/src/applications/oauthserver/server/__init__.php b/src/applications/oauthserver/server/__init__.php index 6fa671b012..f5951c33b1 100644 --- a/src/applications/oauthserver/server/__init__.php +++ b/src/applications/oauthserver/server/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_module('phabricator', 'applications/oauthserver/storage/accesstoken'); phutil_require_module('phabricator', 'applications/oauthserver/storage/authorizationcode'); phutil_require_module('phabricator', 'applications/oauthserver/storage/clientauthorization'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'filesystem'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/oauthserver/server/__tests__/PhabricatorOAuthServerTestCase.php b/src/applications/oauthserver/server/__tests__/PhabricatorOAuthServerTestCase.php new file mode 100644 index 0000000000..fd195be301 --- /dev/null +++ b/src/applications/oauthserver/server/__tests__/PhabricatorOAuthServerTestCase.php @@ -0,0 +1,85 @@ + true, + 'http://www.google.com/' => true, + 'http://www.google.com/auth' => true, + 'www.google.com' => false, + 'http://www.google.com/auth#invalid' => false + ); + $server = new PhabricatorOAuthServer(); + foreach ($map as $input => $expected) { + $uri = new PhutilURI($input); + $result = $server->validateRedirectURI($uri); + $this->assertEqual( + $expected, + $result, + "Validation of redirect URI '{$input}'" + ); + } + } + + public function testValidateSecondaryRedirectURI() { + $server = new PhabricatorOAuthServer(); + $primary_uri = new PhutilURI('http://www.google.com'); + static $test_domain_map = array( + 'http://www.google.com' => true, + 'http://www.google.com/' => true, + 'http://www.google.com/auth' => true, + 'http://www.google.com/?auth' => true, + 'www.google.com' => false, + 'http://www.google.com/auth#invalid' => false, + 'http://www.example.com' => false + ); + foreach ($test_domain_map as $input => $expected) { + $uri = new PhutilURI($input); + $this->assertEqual( + $expected, + $server->validateSecondaryRedirectURI($uri, $primary_uri), + "Validation of redirect URI '{$input}' ". + "relative to '{$primary_uri}'" + ); + } + + $primary_uri = new PhutilURI('http://www.google.com/?auth'); + static $test_query_map = array( + 'http://www.google.com' => false, + 'http://www.google.com/' => false, + 'http://www.google.com/auth' => false, + 'http://www.google.com/?auth' => true, + 'http://www.google.com/?auth&stuff' => true, + 'http://www.google.com/?stuff' => false, + ); + foreach ($test_query_map as $input => $expected) { + $uri = new PhutilURI($input); + $this->assertEqual( + $expected, + $server->validateSecondaryRedirectURI($uri, $primary_uri), + "Validation of secondary redirect URI '{$input}' ". + "relative to '{$primary_uri}'" + ); + } + + } + +} diff --git a/src/applications/oauthserver/server/__tests__/__init__.php b/src/applications/oauthserver/server/__tests__/__init__.php new file mode 100644 index 0000000000..d5cbec1edf --- /dev/null +++ b/src/applications/oauthserver/server/__tests__/__init__.php @@ -0,0 +1,15 @@ +getScope(); $scopes = array_keys($scope); sort($scopes); - return implode(', ', $scopes); + return implode(' ', $scopes); } public function getConfiguration() {