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

@@ -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',

src/applications/fund/phortune/FundBackerProduct.php

@@ -21,11 +21,16 @@ final class FundBackerProduct extends PhortuneProductImplementation {
 
   public function getName(PhortuneProduct $product) {
     $initiative = $this->getInitiative();
+
+    if (!$initiative) {
+      return pht('Fund <Unknown Initiative>');
+    } else {
       return pht(
         'Fund %s %s',
         $initiative->getMonogram(),
         $initiative->getName());
     }
+  }
 
   public function getPriceAsCurrency(PhortuneProduct $product) {
     return PhortuneCurrency::newEmptyCurrency();

src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php

@@ -1,15 +1,10 @@
 <?php
 
 final class PhabricatorOAuthServerAuthController
-  extends PhabricatorAuthController {
+  extends PhabricatorOAuthServerController {
 
-  public function shouldRequireLogin() {
+  public function handleRequest(AphrontRequest $request) {
-    return true;
+    $viewer = $this->getViewer();
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $server = new PhabricatorOAuthServer();
     $client_phid = $request->getStr('client_id');
@@ -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())
+    return $this->newDialog()
-      ->setUser($viewer)
+      ->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())
+    return $this->newDialog()
-      ->setUser($viewer)
       ->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);
   }
 
 

src/applications/oauthserver/controller/PhabricatorOAuthServerController.php

@@ -3,58 +3,13 @@
 abstract class PhabricatorOAuthServerController
   extends PhabricatorController {
 
-  public function buildStandardPageResponse($view, array $data) {
+  const CONTEXT_AUTHORIZE = 'oauthserver.authorize';
-    $user = $this->getRequest()->getUser();
-    $page = $this->buildStandardPageView();
-    $page->setApplicationName(pht('OAuth Server'));
-    $page->setBaseURI('/oauthserver/');
-    $page->setTitle(idx($data, 'title'));
 
-    $nav = new AphrontSideNavFilterView();
+  protected function buildApplicationCrumbs() {
-    $nav->setBaseURI(new PhutilURI('/oauthserver/'));
+    // We're specifically not putting an "OAuth Server" application crumb
-    $nav->addLabel(pht('Clients'));
+    // on these pages because it doesn't make sense to send users there on
-    $nav->addFilter('client/create', pht('Create Client'));
+    // the auth workflows.
-    foreach ($this->getExtraClientFilters() as $filter) {
+    return new PHUICrumbsView();
-      $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 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;
-  }
 }

src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php

@@ -3,26 +3,13 @@
 final class PhabricatorOAuthServerTestController
   extends PhabricatorOAuthServerController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function shouldRequireLogin() {
+    $id = $request->getURIData('id');
-    return true;
-  }
-
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-
-    $panels       = array();
-    $results      = array();
 
     $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())
+      return $this->newDialog()
-        ->setUser($viewer)
         ->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())
+    return $this->newDialog()
-      ->setUser($viewer)
       ->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);
   }
 }

src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php

@@ -1,7 +1,7 @@
 <?php
 
 final class PhabricatorOAuthServerTokenController
-  extends PhabricatorAuthController {
+  extends PhabricatorOAuthServerController {
 
   public function shouldRequireLogin() {
     return false;
@@ -14,8 +14,7 @@ final class PhabricatorOAuthServerTokenController
     return parent::shouldAllowRestrictedParameter($parameter_name);
   }
 
-  public function processRequest() {
+  public function handleRequest(AphrontRequest $request) {
-    $request       = $this->getRequest();
     $grant_type = $request->getStr('grant_type');
     $code = $request->getStr('code');
     $redirect_uri = $request->getStr('redirect_uri');
@@ -23,6 +22,7 @@ final class PhabricatorOAuthServerTokenController
     $client_secret = $request->getStr('client_secret');
     $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 {

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;
+  }
+
 }

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;
   }

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)) {