diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -6376,7 +6376,7 @@ 'PhabricatorOAuthServer' => 'Phobject', 'PhabricatorOAuthServerAccessToken' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerApplication' => 'PhabricatorApplication', - 'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController', + 'PhabricatorOAuthServerAuthController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorOAuthServerClient' => array( @@ -6394,7 +6394,7 @@ '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 --- a/src/applications/fund/phortune/FundBackerProduct.php +++ b/src/applications/fund/phortune/FundBackerProduct.php @@ -21,10 +21,15 @@ 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 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php @@ -1,20 +1,15 @@ getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); - $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 @@ 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 @@ // 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 @@ } else { $desired_scopes = $scope; } + if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { return $this->buildErrorResponse( 'invalid_scope', @@ -236,8 +227,8 @@ '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 @@ ->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 --- 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 --- 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 @@ ->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 @@ // 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 @@ 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 --- 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 @@ '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 @@ 'client_id')); return $response; } + if (!$client_secret) { $response->setError('invalid_request'); $response->setErrorDescription( @@ -53,6 +56,7 @@ '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 --- a/src/applications/policy/exception/PhabricatorPolicyException.php +++ b/src/applications/policy/exception/PhabricatorPolicyException.php @@ -6,6 +6,9 @@ private $rejection; private $capabilityName; private $moreInfo = array(); + private $objectPHID; + private $context; + private $capability; public function setTitle($title) { $this->title = $title; @@ -43,4 +46,31 @@ 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 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -589,7 +589,9 @@ $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 @@ $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 @@ $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 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -321,6 +321,12 @@ $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)) {