Modernize some OAuth Server code

Summary: Ref T7303. This inches toward properly-behaved cluster logout. - Use IDs instead of PHIDs in URIs. - Slightly more modern code. - Fix some crumb stuff. Test Plan: Created, edited, viewed, deleted, showed secret for, authorized, test-auth'd an application. Reviewers: chad Reviewed By: chad Maniphest Tasks: T7303 Differential Revision: https://secure.phabricator.com/D15592
2024-11-23 07:12:41 +01:00 · 2016-04-03 07:25:33 -07:00 · 2016-04-03 07:25:33 -07:00 · 694a8543d8
commit 694a8543d8
parent f54a2007ea
11 changed files with 59 additions and 79 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -2705,6 +2705,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthClientEditController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php',
    'PhabricatorOAuthClientListController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php',
    'PhabricatorOAuthClientSecretController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php',
+    'PhabricatorOAuthClientTestController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php',
    'PhabricatorOAuthClientViewController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php',
    'PhabricatorOAuthResponse' => 'applications/oauthserver/PhabricatorOAuthResponse.php',
    'PhabricatorOAuthServer' => 'applications/oauthserver/PhabricatorOAuthServer.php',
@ -2723,7 +2724,6 @@ phutil_register_library_map(array(
    'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/PhabricatorOAuthServerDAO.php',
    'PhabricatorOAuthServerScope' => 'applications/oauthserver/PhabricatorOAuthServerScope.php',
    'PhabricatorOAuthServerTestCase' => 'applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php',
-    'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTestController.php',
    'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php',
    'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php',
    'PhabricatorObjectHasAsanaSubtaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaSubtaskEdgeType.php',
@ -7192,6 +7192,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthClientEditController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientListController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientSecretController' => 'PhabricatorOAuthClientController',
+    'PhabricatorOAuthClientTestController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientViewController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthResponse' => 'AphrontResponse',
    'PhabricatorOAuthServer' => 'Phobject',
@ -7214,7 +7215,6 @@ phutil_register_library_map(array(
    'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO',
    'PhabricatorOAuthServerScope' => 'Phobject',
    'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase',
-    'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController',
    'PhabricatorOAuthServerTokenController' => 'PhabricatorOAuthServerController',
    'PhabricatorObjectHandle' => array(
      'Phobject',
--- a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
+++ b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
@ -50,14 +50,14 @@ final class PhabricatorOAuthServerApplication extends PhabricatorApplication {
        '(?:query/(?P<queryKey>[^/]+)/)?'
          => 'PhabricatorOAuthClientListController',
        'auth/' => 'PhabricatorOAuthServerAuthController',
-        'test/(?P<id>\d+)/' => 'PhabricatorOAuthServerTestController',
        'token/' => 'PhabricatorOAuthServerTokenController',
        'client/' => array(
          'create/' => 'PhabricatorOAuthClientEditController',
-          'delete/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientDeleteController',
-          'edit/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientEditController',
-          'view/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientViewController',
-          'secret/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientSecretController',
+          'delete/(?P<id>\d+)/' => 'PhabricatorOAuthClientDeleteController',
+          'edit/(?P<id>\d+)/' => 'PhabricatorOAuthClientEditController',
+          'view/(?P<id>\d+)/' => 'PhabricatorOAuthClientViewController',
+          'secret/(?P<id>\d+)/' => 'PhabricatorOAuthClientSecretController',
+          'test/(?P<id>\d+)/' => 'PhabricatorOAuthClientTestController',
        ),
      ),
    );
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@ -3,6 +3,12 @@
 final class PhabricatorOAuthServerAuthController
  extends PhabricatorOAuthServerController {

+  protected function buildApplicationCrumbs() {
+    // We're specifically not putting an "OAuth Server" application crumb
+    // on the auth pages because it doesn't make sense to send users there.
+    return new PHUICrumbsView();
+  }
+
  public function handleRequest(AphrontRequest $request) {
    $viewer = $this->getViewer();

--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php
@ -5,11 +5,4 @@ abstract class PhabricatorOAuthServerController

  const CONTEXT_AUTHORIZE = 'oauthserver.authorize';

-  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();
-  }
-
 }
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php
@ -3,13 +3,12 @@
 final class PhabricatorOAuthClientDeleteController
  extends PhabricatorOAuthClientController {

-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();

    $client = id(new PhabricatorOAuthServerClientQuery())
      ->setViewer($viewer)
-      ->withPHIDs(array($this->getClientPHID()))
+      ->withIDs(array($request->getURIData('id')))
      ->requireCapabilities(
        array(
          PhabricatorPolicyCapability::CAN_VIEW,
@ -20,14 +19,15 @@ final class PhabricatorOAuthClientDeleteController
      return new Aphront404Response();
    }

+    // TODO: This should be "disable", not "delete"!
+
    if ($request->isFormPost()) {
      $client->delete();
      $app_uri = $this->getApplicationURI();
      return id(new AphrontRedirectResponse())->setURI($app_uri);
    }

-    $dialog = id(new AphrontDialogView())
-      ->setUser($viewer)
+    return $this->newDialog()
      ->setTitle(pht('Delete OAuth Application?'))
      ->appendParagraph(
        pht(
@ -35,8 +35,6 @@ final class PhabricatorOAuthClientDeleteController
          phutil_tag('strong', array(), $client->getName())))
      ->addCancelButton($client->getViewURI())
      ->addSubmitButton(pht('Delete Application'));
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
  }

 }
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
@ -3,15 +3,14 @@
 final class PhabricatorOAuthClientEditController
  extends PhabricatorOAuthClientController {

-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');

-    $phid = $this->getClientPHID();
-    if ($phid) {
+    if ($id) {
      $client = id(new PhabricatorOAuthServerClientQuery())
        ->setViewer($viewer)
-        ->withPHIDs(array($phid))
+        ->withIDs(array($id))
        ->requireCapabilities(
          array(
            PhabricatorPolicyCapability::CAN_VIEW,
@ -124,14 +123,10 @@ final class PhabricatorOAuthClientEditController
      ->setFormErrors($errors)
      ->setForm($form);

-    return $this->buildApplicationPage(
-      array(
-        $crumbs,
-        $box,
-      ),
-      array(
-        'title' => $title,
-      ));
+    return $this->newPage()
+      ->setCrumbs($crumbs)
+      ->setTitle($title)
+      ->appendChild($box);
  }

 }
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php
@ -8,7 +8,7 @@ final class PhabricatorOAuthClientSecretController

    $client = id(new PhabricatorOAuthServerClientQuery())
      ->setViewer($viewer)
-      ->withPHIDs(array($this->getClientPHID()))
+      ->withIDs(array($request->getURIData('id')))
      ->requireCapabilities(
        array(
          PhabricatorPolicyCapability::CAN_VIEW,
@ -27,22 +27,20 @@ final class PhabricatorOAuthClientSecretController

    if ($request->isFormPost()) {
      $secret = $client->getSecret();
+
      $body = id(new PHUIFormLayoutView())
        ->appendChild(
          id(new AphrontFormTextAreaControl())
-          ->setLabel(pht('Plaintext'))
-          ->setReadOnly(true)
-          ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT)
-          ->setValue($secret));
+            ->setLabel(pht('Plaintext'))
+            ->setReadOnly(true)
+            ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT)
+            ->setValue($secret));

-      $dialog = id(new AphrontDialogView())
-        ->setUser($viewer)
+      return $this->newDialog()
        ->setWidth(AphrontDialogView::WIDTH_FORM)
        ->setTitle(pht('Application Secret'))
        ->appendChild($body)
        ->addCancelButton($view_uri, pht('Done'));
-
-      return id(new AphrontDialogResponse())->setDialog($dialog);
    }


@ -59,8 +57,8 @@ final class PhabricatorOAuthClientSecretController
        'your monitor to create a human shield, keeping it safe from prying '.
        'eyes. Protect company secrets!');
    }
+
    return $this->newDialog()
-      ->setUser($viewer)
      ->setTitle(pht('Really show application secret?'))
      ->appendChild($body)
      ->addSubmitButton(pht('Show Application Secret'))
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php
@ -1,7 +1,7 @@
 <?php

-final class PhabricatorOAuthServerTestController
-  extends PhabricatorOAuthServerController {
+final class PhabricatorOAuthClientTestController
+  extends PhabricatorOAuthClientController {

  public function handleRequest(AphrontRequest $request) {
    $viewer = $this->getViewer();
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
@ -3,13 +3,12 @@
 final class PhabricatorOAuthClientViewController
  extends PhabricatorOAuthClientController {

-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();

    $client = id(new PhabricatorOAuthServerClientQuery())
      ->setViewer($viewer)
-      ->withPHIDs(array($this->getClientPHID()))
+      ->withIDs(array($request->getURIData('id')))
      ->executeOne();
    if (!$client) {
      return new Aphront404Response();
@ -27,18 +26,16 @@ final class PhabricatorOAuthClientViewController
      ->setHeader($header)
      ->addPropertyList($properties);

-    return $this->buildApplicationPage(
-      array(
-        $crumbs,
-        $box,
-      ),
-      array(
-        'title' => pht('OAuth Application: %s', $client->getName()),
-      ));
+    $title = pht('OAuth Application: %s', $client->getName());
+
+    return $this->newPage()
+      ->setCrumbs($crumbs)
+      ->setTitle($title)
+      ->appendChild($box);
  }

  private function buildHeaderView(PhabricatorOAuthServerClient $client) {
-    $viewer = $this->getRequest()->getUser();
+    $viewer = $this->getViewer();

    $header = id(new PHUIHeaderView())
      ->setUser($viewer)
@ -49,7 +46,7 @@ final class PhabricatorOAuthClientViewController
  }

  private function buildActionView(PhabricatorOAuthServerClient $client) {
-    $viewer = $this->getRequest()->getUser();
+    $viewer = $this->getViewer();

    $can_edit = PhabricatorPolicyFilter::hasCapability(
      $viewer,
@ -63,7 +60,6 @@ final class PhabricatorOAuthClientViewController
      ->executeOne();
    $is_authorized = (bool)$authorization;
    $id = $client->getID();
-    $phid = $client->getPHID();

    $view = id(new PhabricatorActionListView())
      ->setUser($viewer);
@ -80,7 +76,7 @@ final class PhabricatorOAuthClientViewController
      id(new PhabricatorActionView())
        ->setName(pht('Show Application Secret'))
        ->setIcon('fa-eye')
-        ->setHref($this->getApplicationURI("client/secret/{$phid}/"))
+        ->setHref($this->getApplicationURI("client/secret/{$id}/"))
        ->setDisabled(!$can_edit)
        ->setWorkflow(true));

@ -98,7 +94,7 @@ final class PhabricatorOAuthClientViewController
        ->setIcon('fa-wrench')
        ->setWorkflow(true)
        ->setDisabled($is_authorized)
-        ->setHref($this->getApplicationURI('test/'.$id.'/')));
+        ->setHref($this->getApplicationURI("client/test/{$id}/")));

    return $view;
  }
@ -110,7 +106,7 @@ final class PhabricatorOAuthClientViewController
      ->setUser($viewer);

    $view->addProperty(
-      pht('Client ID'),
+      pht('Client PHID'),
      $client->getPHID());

    $view->addProperty(
--- a/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php
+++ b/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php
@ -79,12 +79,6 @@ final class PhabricatorOAuthServerClientSearchEngine
    return parent::buildSavedQueryFromBuiltin($query_key);
  }

-  protected function getRequiredHandlePHIDsForResultList(
-    array $clients,
-    PhabricatorSavedQuery $query) {
-    return mpull($clients, 'getCreatorPHID');
-  }
-
  protected function renderResultList(
    array $clients,
    PhabricatorSavedQuery $query,
@ -96,14 +90,11 @@ final class PhabricatorOAuthServerClientSearchEngine
    $list = id(new PHUIObjectItemListView())
      ->setUser($viewer);
    foreach ($clients as $client) {
-      $creator = $handles[$client->getCreatorPHID()];
-
      $item = id(new PHUIObjectItemView())
        ->setObjectName(pht('Application %d', $client->getID()))
        ->setHeader($client->getName())
        ->setHref($client->getViewURI())
-        ->setObject($client)
-        ->addByline(pht('Creator: %s', $creator->renderLink()));
+        ->setObject($client);

      $list->addItem($item);
    }
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
@ -15,15 +15,18 @@ final class PhabricatorOAuthServerClient
  protected $editPolicy;

  public function getEditURI() {
-    return '/oauthserver/client/edit/'.$this->getPHID().'/';
+    $id = $this->getID();
+    return "/oauthserver/client/edit/{$id}/";
  }

  public function getViewURI() {
-    return '/oauthserver/client/view/'.$this->getPHID().'/';
+    $id = $this->getID();
+    return "/oauthserver/client/view/{$id}/";
  }

  public function getDeleteURI() {
-    return '/oauthserver/client/delete/'.$this->getPHID().'/';
+    $id = $this->getID();
+    return "/oauthserver/client/delete/{$id}/";
  }

  public static function initializeNewClient(PhabricatorUser $actor) {