Page MenuHomePhabricator
Files F14865791

D14050.id33962.diff
No OneTemporary
Actions

D14050.id33962.diff
View Options

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 <Unknown Initiative>');
+ } 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 @@
<?php
final class PhabricatorOAuthServerAuthController
- extends PhabricatorAuthController {
+ extends PhabricatorOAuthServerController {
- public function shouldRequireLogin() {
- return true;
- }
-
- public function processRequest() {
- $request = $this->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 @@
<?php
final class PhabricatorOAuthServerTokenController
- extends PhabricatorAuthController {
+ extends PhabricatorOAuthServerController {
public function shouldRequireLogin() {
return false;
@@ -14,15 +14,15 @@
return parent::shouldAllowRestrictedParameter($parameter_name);
}
- 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');
+ 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)) {

File Metadata

Mime Type
text/plain
Expires
Sun, Feb 9, 12:10 AM (21 h, 20 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7103001
Default Alt Text
D14050.id33962.diff (16 KB)

Event Timeline

Phabricator · Built by Phacility