From 9d0332c2c0a4c3332b249eaf882b77adda9fd0da Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:05:23 -0700 Subject: [PATCH] Modernize OAuthserver and provide more context on "no permission" exception Summary: Ref T7173. Depends on D14049. Now that Phacility can install custom exception handlers, this puts enough information on the exception so that we can figure out what to do with it. - Generally modernize some of this code. - Add some more information to PolicyExceptions so the new RequestExceptionHandler can handle them properly. Test Plan: Failed authorizations, then succeeded authorizations. See next diff. Reviewers: chad Reviewed By: chad Maniphest Tasks: T7173 Differential Revision: https://secure.phabricator.com/D14050 --- src/__phutil_library_map__.php | 4 +- .../fund/phortune/FundBackerProduct.php | 13 ++-- .../PhabricatorOAuthServerAuthController.php | 62 +++++++------------ .../PhabricatorOAuthServerController.php | 57 ++--------------- .../PhabricatorOAuthServerTestController.php | 31 ++-------- .../PhabricatorOAuthServerTokenController.php | 22 ++++--- .../exception/PhabricatorPolicyException.php | 30 +++++++++ .../policy/filter/PhabricatorPolicyFilter.php | 11 +++- .../policy/storage/PhabricatorPolicy.php | 6 ++ 9 files changed, 106 insertions(+), 130 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 262d144ecd..aba0b8a1c5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -6376,7 +6376,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServer' => 'Phobject', 'PhabricatorOAuthServerAccessToken' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerApplication' => 'PhabricatorApplication', - 'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController', + 'PhabricatorOAuthServerAuthController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorOAuthServerClient' => array( @@ -6394,7 +6394,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerScope' => 'Phobject', 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', 'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController', - 'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController', + 'PhabricatorOAuthServerTokenController' => 'PhabricatorOAuthServerController', 'PhabricatorObjectHandle' => array( 'Phobject', 'PhabricatorPolicyInterface', diff --git a/src/applications/fund/phortune/FundBackerProduct.php b/src/applications/fund/phortune/FundBackerProduct.php index 79ffc559bb..679a1a41d2 100644 --- a/src/applications/fund/phortune/FundBackerProduct.php +++ b/src/applications/fund/phortune/FundBackerProduct.php @@ -21,10 +21,15 @@ final class FundBackerProduct extends PhortuneProductImplementation { public function getName(PhortuneProduct $product) { $initiative = $this->getInitiative(); - return pht( - 'Fund %s %s', - $initiative->getMonogram(), - $initiative->getName()); + + if (!$initiative) { + return pht('Fund '); + } else { + return pht( + 'Fund %s %s', + $initiative->getMonogram(), + $initiative->getName()); + } } public function getPriceAsCurrency(PhortuneProduct $product) { diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php index 4ff83b4d5f..d473e1557a 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php @@ -1,20 +1,15 @@ getViewer(); - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $server = new PhabricatorOAuthServer(); - $client_phid = $request->getStr('client_id'); - $scope = $request->getStr('scope'); - $redirect_uri = $request->getStr('redirect_uri'); + $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'); // state is an opaque value the client sent us for their own purposes @@ -30,6 +25,19 @@ final class PhabricatorOAuthServerAuthController phutil_tag('strong', array(), 'client_id'))); } + // We require that users must be able to see an OAuth application + // in order to authorize it. This allows an application's visibility + // policy to be used to restrict authorized users. + try { + $client = id(new PhabricatorOAuthServerClientQuery()) + ->setViewer($viewer) + ->withPHIDs(array($client_phid)) + ->executeOne(); + } catch (PhabricatorPolicyException $ex) { + $ex->setContext(self::CONTEXT_AUTHORIZE); + throw $ex; + } + $server->setUser($viewer); $is_authorized = false; $authorization = null; @@ -39,24 +47,6 @@ final class PhabricatorOAuthServerAuthController // one giant try / catch around all the exciting database stuff so we // can return a 'server_error' response if something goes wrong! try { - try { - $client = id(new PhabricatorOAuthServerClientQuery()) - ->setViewer($viewer) - ->withPHIDs(array($client_phid)) - ->executeOne(); - } catch (PhabricatorPolicyException $ex) { - // We require that users must be able to see an OAuth application - // in order to authorize it. This allows an application's visibility - // policy to be used to restrict authorized users. - - // None of the OAuth error responses are a perfect fit for this, but - // 'invalid_client' seems closest. - return $this->buildErrorResponse( - 'invalid_client', - pht('Not Authorized'), - pht('You are not authorized to authenticate.')); - } - if (!$client) { return $this->buildErrorResponse( 'invalid_request', @@ -211,6 +201,7 @@ final class PhabricatorOAuthServerAuthController } else { $desired_scopes = $scope; } + if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { return $this->buildErrorResponse( 'invalid_scope', @@ -236,8 +227,8 @@ final class PhabricatorOAuthServerAuthController 'error_description' => $cancel_msg, )); - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() + ->setShortTitle(pht('Authorize Access')) ->setTitle(pht('Authorize "%s"?', $name)) ->setSubmitURI($request->getRequestURI()->getPath()) ->setWidth(AphrontDialogView::WIDTH_FORM) @@ -250,23 +241,18 @@ final class PhabricatorOAuthServerAuthController ->appendChild($form->buildLayoutView()) ->addSubmitButton(pht('Authorize Access')) ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } private function buildErrorResponse($code, $title, $message) { $viewer = $this->getRequest()->getUser(); - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('OAuth: %s', $title)) ->appendParagraph($message) ->appendParagraph( pht('OAuth Error Code: %s', phutil_tag('tt', array(), $code))) ->addCancelButton('/', pht('Alas!')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php index 22388c8766..00be8a1e8d 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php @@ -3,58 +3,13 @@ abstract class PhabricatorOAuthServerController extends PhabricatorController { - public function buildStandardPageResponse($view, array $data) { - $user = $this->getRequest()->getUser(); - $page = $this->buildStandardPageView(); - $page->setApplicationName(pht('OAuth Server')); - $page->setBaseURI('/oauthserver/'); - $page->setTitle(idx($data, 'title')); + const CONTEXT_AUTHORIZE = 'oauthserver.authorize'; - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/oauthserver/')); - $nav->addLabel(pht('Clients')); - $nav->addFilter('client/create', pht('Create Client')); - foreach ($this->getExtraClientFilters() as $filter) { - $nav->addFilter($filter['url'], $filter['label']); - } - $nav->addFilter('client', pht('My Clients')); - $nav->selectFilter($this->getFilter(), 'clientauthorization'); - - $nav->appendChild($view); - - $page->appendChild($nav); - - $response = new AphrontWebpageResponse(); - return $response->setContent($page->render()); + protected function buildApplicationCrumbs() { + // We're specifically not putting an "OAuth Server" application crumb + // on these pages because it doesn't make sense to send users there on + // the auth workflows. + return new PHUICrumbsView(); } - protected function getFilter() { - return 'clientauthorization'; - } - - protected function getExtraClientFilters() { - return array(); - } - - protected function getHighlightPHIDs() { - $phids = array(); - $request = $this->getRequest(); - $edited = $request->getStr('edited'); - $new = $request->getStr('new'); - if ($edited) { - $phids[$edited] = $edited; - } - if ($new) { - $phids[$new] = $new; - } - return $phids; - } - - protected function buildErrorView($error_message) { - $error = new PHUIInfoView(); - $error->setSeverity(PHUIInfoView::SEVERITY_ERROR); - $error->setTitle($error_message); - - return $error; - } } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php index d95fdf7d43..e5966518f5 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php @@ -3,26 +3,13 @@ final class PhabricatorOAuthServerTestController extends PhabricatorOAuthServerController { - private $id; - - public function shouldRequireLogin() { - return true; - } - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $panels = array(); - $results = array(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); $client = id(new PhabricatorOAuthServerClientQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->executeOne(); if (!$client) { return new Aphront404Response(); @@ -37,16 +24,13 @@ final class PhabricatorOAuthServerTestController ->withClientPHIDs(array($client->getPHID())) ->executeOne(); if ($authorization) { - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('Already Authorized')) ->appendParagraph( pht( 'You have already authorized this application to access your '. 'account.')) ->addCancelButton($view_uri, pht('Close')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } if ($request->isFormPost()) { @@ -65,8 +49,7 @@ final class PhabricatorOAuthServerTestController // TODO: It would be nice to put scope options in this dialog, maybe? - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('Authorize Application?')) ->appendParagraph( pht( @@ -75,7 +58,5 @@ final class PhabricatorOAuthServerTestController phutil_tag('strong', array(), $client->getName()))) ->addCancelButton($view_uri) ->addSubmitButton(pht('Authorize Application')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php index aa9b8f71a0..7e35574f61 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php @@ -1,7 +1,7 @@ getRequest(); - $grant_type = $request->getStr('grant_type'); - $code = $request->getStr('code'); - $redirect_uri = $request->getStr('redirect_uri'); - $client_phid = $request->getStr('client_id'); + public function handleRequest(AphrontRequest $request) { + $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(); + $response = new PhabricatorOAuthResponse(); + $server = new PhabricatorOAuthServer(); + if ($grant_type != 'authorization_code') { $response->setError('unsupported_grant_type'); $response->setErrorDescription( @@ -32,11 +32,13 @@ final class PhabricatorOAuthServerTokenController 'authorization_code')); return $response; } + if (!$code) { $response->setError('invalid_request'); $response->setErrorDescription(pht('Required parameter code missing.')); return $response; } + if (!$client_phid) { $response->setError('invalid_request'); $response->setErrorDescription( @@ -45,6 +47,7 @@ final class PhabricatorOAuthServerTokenController 'client_id')); return $response; } + if (!$client_secret) { $response->setError('invalid_request'); $response->setErrorDescription( @@ -53,6 +56,7 @@ final class PhabricatorOAuthServerTokenController 'client_secret')); 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 { diff --git a/src/applications/policy/exception/PhabricatorPolicyException.php b/src/applications/policy/exception/PhabricatorPolicyException.php index 782e1350f0..7d58666d65 100644 --- a/src/applications/policy/exception/PhabricatorPolicyException.php +++ b/src/applications/policy/exception/PhabricatorPolicyException.php @@ -6,6 +6,9 @@ final class PhabricatorPolicyException extends Exception { private $rejection; private $capabilityName; private $moreInfo = array(); + private $objectPHID; + private $context; + private $capability; public function setTitle($title) { $this->title = $title; @@ -43,4 +46,31 @@ final class PhabricatorPolicyException extends Exception { return $this->moreInfo; } + public function setObjectPHID($object_phid) { + $this->objectPHID = $object_phid; + return $this; + } + + public function getObjectPHID() { + return $this->objectPHID; + } + + public function setContext($context) { + $this->context = $context; + return $this; + } + + public function getContext() { + return $this->context; + } + + public function setCapability($capability) { + $this->capability = $capability; + return $this; + } + + public function getCapability() { + return $this->capability; + } + } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index dcff36d338..f9be22ad52 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -589,7 +589,9 @@ final class PhabricatorPolicyFilter extends Phobject { $exception = id(new PhabricatorPolicyException($full_message)) ->setTitle($access_denied) + ->setObjectPHID($object->getPHID()) ->setRejection($rejection) + ->setCapability($capability) ->setCapabilityName($capability_name) ->setMoreInfo($details); @@ -710,6 +712,11 @@ final class PhabricatorPolicyFilter extends Phobject { $objects = $policy->getRuleObjects(); $action = null; foreach ($policy->getRules() as $rule) { + if (!is_array($rule)) { + // Reject, this policy rule is invalid. + return false; + } + $rule_object = idx($objects, idx($rule, 'rule')); if (!$rule_object) { // Reject, this policy has a bogus rule. @@ -831,7 +838,9 @@ final class PhabricatorPolicyFilter extends Phobject { $exception = id(new PhabricatorPolicyException($full_message)) ->setTitle($access_denied) - ->setRejection($rejection); + ->setObjectPHID($object->getPHID()) + ->setRejection($rejection) + ->setCapability(PhabricatorPolicyCapability::CAN_VIEW); throw $exception; } diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index c23858b0a8..93afb35dc9 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -321,6 +321,12 @@ final class PhabricatorPolicy $classes = array(); foreach ($this->getRules() as $rule) { + if (!is_array($rule)) { + // This rule is invalid. We'll reject it later, but don't need to + // extract anything from it for now. + continue; + } + $class = idx($rule, 'rule'); try { if (class_exists($class)) {