Convert OAuthServer to Transactions + EditEngine

Summary: Ref T7303. This application is currently stone-age tech (no transactions, hard "delete" action). Bring it up to modern specs. Test Plan: - Created and edited an OAuth application. - Viewed transaction record. - Tried to create something with no name, invalid redirect URI, etc. Was gently rebuffed with detailed explanatory errors. Reviewers: chad Reviewed By: chad Maniphest Tasks: T7303 Differential Revision: https://secure.phabricator.com/D15609
2016-04-04 09:27:42 -07:00
parent e2685a248b
commit 57f016b166
13 changed files with 416 additions and 166 deletions
--- a/src/applications/oauthserver/PhabricatorOAuthServer.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServer.php
@@ -188,24 +188,49 @@ final class PhabricatorOAuthServer extends Phobject {
    return $authorization;
  }

+  public function validateRedirectURI($uri) {
+    try {
+      $this->assertValidRedirectURI($uri);
+      return true;
+    } catch (Exception $ex) {
+      return false;
+    }
+  }
+
  /**
   * See http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2
   * for details on what makes a given redirect URI "valid".
   */
-  public function validateRedirectURI(PhutilURI $uri) {
-    if (!PhabricatorEnv::isValidRemoteURIForLink($uri)) {
-      return false;
+  public function assertValidRedirectURI($raw_uri) {
+    // This covers basics like reasonable formatting and the existence of a
+    // protocol.
+    PhabricatorEnv::requireValidRemoteURIForLink($raw_uri);
+
+    $uri = new PhutilURI($raw_uri);
+
+    $fragment = $uri->getFragment();
+    if (strlen($fragment)) {
+      throw new Exception(
+        pht(
+          'OAuth application redirect URIs must not contain URI '.
+          'fragments, but the URI "%s" has a fragment ("%s").',
+          $raw_uri,
+          $fragment));
    }

-    if ($uri->getFragment()) {
-      return false;
+    $protocol = $uri->getProtocol();
+    switch ($protocol) {
+      case 'http':
+      case 'https':
+        break;
+      default:
+        throw new Exception(
+          pht(
+            'OAuth application redirect URIs must only use the "http" or '.
+            '"https" protocols, but the URI "%s" uses the "%s" protocol.',
+            $raw_uri,
+            $protocol));
    }
-
-    if (!$uri->getDomain()) {
-      return false;
-    }
-
-    return true;
  }

  /**
--- a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
+++ b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
@@ -51,10 +51,10 @@ final class PhabricatorOAuthServerApplication extends PhabricatorApplication {
          => 'PhabricatorOAuthClientListController',
        'auth/' => 'PhabricatorOAuthServerAuthController',
        'token/' => 'PhabricatorOAuthServerTokenController',
+        $this->getEditRoutePattern('edit/') =>
+          'PhabricatorOAuthClientEditController',
        'client/' => array(
-          'create/' => 'PhabricatorOAuthClientEditController',
          '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/client/PhabricatorOAuthClientEditController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
@@ -4,129 +4,9 @@ final class PhabricatorOAuthClientEditController
  extends PhabricatorOAuthClientController {

  public function handleRequest(AphrontRequest $request) {
-    $viewer = $this->getViewer();
-    $id = $request->getURIData('id');
-
-    if ($id) {
-      $client = id(new PhabricatorOAuthServerClientQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($id))
-        ->requireCapabilities(
-          array(
-            PhabricatorPolicyCapability::CAN_VIEW,
-            PhabricatorPolicyCapability::CAN_EDIT,
-          ))
-        ->executeOne();
-      if (!$client) {
-        return new Aphront404Response();
-      }
-
-      $title = pht('Edit OAuth Application: %s', $client->getName());
-      $submit_button = pht('Save Application');
-      $crumb_text = pht('Edit');
-      $cancel_uri = $client->getViewURI();
-      $is_new = false;
-    } else {
-      $this->requireApplicationCapability(
-        PhabricatorOAuthServerCreateClientsCapability::CAPABILITY);
-
-      $client = PhabricatorOAuthServerClient::initializeNewClient($viewer);
-
-      $title = pht('Create OAuth Application');
-      $submit_button = pht('Create Application');
-      $crumb_text = pht('Create Application');
-      $cancel_uri = $this->getApplicationURI();
-      $is_new = true;
-    }
-
-    $errors = array();
-    $e_redirect = true;
-    $e_name = true;
-    if ($request->isFormPost()) {
-      $redirect_uri = $request->getStr('redirect_uri');
-      $client->setName($request->getStr('name'));
-      $client->setRedirectURI($redirect_uri);
-
-      if (!strlen($client->getName())) {
-        $errors[] = pht('You must choose a name for this OAuth application.');
-        $e_name = pht('Required');
-      }
-
-      $server = new PhabricatorOAuthServer();
-      $uri = new PhutilURI($redirect_uri);
-      if (!$server->validateRedirectURI($uri)) {
-        $errors[] = pht(
-          'Redirect URI must be a fully qualified domain name '.
-          'with no fragments. See %s for more information on the correct '.
-          'format.',
-          'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2');
-        $e_redirect = pht('Invalid');
-      }
-
-      $client->setViewPolicy($request->getStr('viewPolicy'));
-      $client->setEditPolicy($request->getStr('editPolicy'));
-      if (!$errors) {
-        $client->save();
-        $view_uri = $client->getViewURI();
-        return id(new AphrontRedirectResponse())->setURI($view_uri);
-      }
-    }
-
-    $policies = id(new PhabricatorPolicyQuery())
-      ->setViewer($viewer)
-      ->setObject($client)
-      ->execute();
-
-    $form = id(new AphrontFormView())
-      ->setUser($viewer)
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('Name'))
-          ->setName('name')
-          ->setValue($client->getName())
-          ->setError($e_name))
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('Redirect URI'))
-          ->setName('redirect_uri')
-          ->setValue($client->getRedirectURI())
-          ->setError($e_redirect))
-      ->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
-          ->setPolicyObject($client)
-          ->setPolicies($policies)
-          ->setName('viewPolicy'))
-      ->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(PhabricatorPolicyCapability::CAN_EDIT)
-          ->setPolicyObject($client)
-          ->setPolicies($policies)
-          ->setName('editPolicy'))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->addCancelButton($cancel_uri)
-          ->setValue($submit_button));
-
-    $crumbs = $this->buildApplicationCrumbs();
-    if (!$is_new) {
-      $crumbs->addTextCrumb(
-        $client->getName(),
-        $client->getViewURI());
-    }
-    $crumbs->addTextCrumb($crumb_text);
-
-    $box = id(new PHUIObjectBoxView())
-      ->setHeaderText($title)
-      ->setFormErrors($errors)
-      ->setForm($form);
-
-    return $this->newPage()
-      ->setCrumbs($crumbs)
-      ->setTitle($title)
-      ->appendChild($box);
+    return id(new PhabricatorOAuthServerEditEngine())
+      ->setController($this)
+      ->buildResponse();
  }

 }
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php
@@ -3,38 +3,22 @@
 final class PhabricatorOAuthClientListController
  extends PhabricatorOAuthClientController {

-  private $queryKey;
-
  public function shouldAllowPublic() {
    return true;
  }

-  public function willProcessRequest(array $data) {
-    $this->queryKey = idx($data, 'queryKey');
-  }
-
-  public function processRequest() {
-    $controller = id(new PhabricatorApplicationSearchController())
-      ->setQueryKey($this->queryKey)
-      ->setSearchEngine(new PhabricatorOAuthServerClientSearchEngine())
-      ->setNavigation($this->buildSideNavView());
-
-    return $this->delegateToController($controller);
+  public function handleRequest(AphrontRequest $request) {
+    return id(new PhabricatorOAuthServerClientSearchEngine())
+      ->setController($this)
+      ->buildResponse();
  }

  protected function buildApplicationCrumbs() {
    $crumbs = parent::buildApplicationCrumbs();

-    $can_create = $this->hasApplicationCapability(
-        PhabricatorOAuthServerCreateClientsCapability::CAPABILITY);
-
-    $crumbs->addAction(
-      id(new PHUIListItemView())
-        ->setHref($this->getApplicationURI('client/create/'))
-        ->setName(pht('Create Application'))
-        ->setDisabled(!$can_create)
-        ->setWorkflow(!$can_create)
-        ->setIcon('fa-plus-square'));
+    id(new PhabricatorOAuthServerEditEngine())
+      ->setViewer($this->getViewer())
+      ->addActionToCrumbs($crumbs);

    return $crumbs;
  }
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
@@ -22,6 +22,11 @@ final class PhabricatorOAuthClientViewController
    $crumbs = $this->buildApplicationCrumbs();
    $crumbs->addTextCrumb($client->getName());

+    $timeline = $this->buildTransactionTimeline(
+      $client,
+      new PhabricatorOAuthServerTransactionQuery());
+    $timeline->setShouldTerminate(true);
+
    $box = id(new PHUIObjectBoxView())
      ->setHeader($header)
      ->addPropertyList($properties);
@@ -31,7 +36,11 @@ final class PhabricatorOAuthClientViewController
    return $this->newPage()
      ->setCrumbs($crumbs)
      ->setTitle($title)
-      ->appendChild($box);
+      ->appendChild(
+        array(
+          $box,
+          $timeline,
+        ));
  }

  private function buildHeaderView(PhabricatorOAuthServerClient $client) {
--- a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php
+++ b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php
@@ -0,0 +1,100 @@
+<?php
+
+final class PhabricatorOAuthServerEditEngine
+  extends PhabricatorEditEngine {
+
+  const ENGINECONST = 'oauthserver.application';
+
+  public function isEngineConfigurable() {
+    return false;
+  }
+
+  public function getEngineName() {
+    return pht('OAuth Applications');
+  }
+
+  public function getSummaryHeader() {
+    return pht('Edit OAuth Applications');
+  }
+
+  public function getSummaryText() {
+    return pht('This engine manages OAuth client applications.');
+  }
+
+  public function getEngineApplicationClass() {
+    return 'PhabricatorOAuthServerApplication';
+  }
+
+  protected function newEditableObject() {
+    return PhabricatorOAuthServerClient::initializeNewClient(
+      $this->getViewer());
+  }
+
+  protected function newObjectQuery() {
+    return id(new PhabricatorOAuthServerClientQuery());
+  }
+
+  protected function getObjectCreateTitleText($object) {
+    return pht('Create OAuth Server');
+  }
+
+  protected function getObjectCreateButtonText($object) {
+    return pht('Create OAuth Server');
+  }
+
+  protected function getObjectEditTitleText($object) {
+    return pht('Edit OAuth Server: %s', $object->getName());
+  }
+
+  protected function getObjectEditShortText($object) {
+    return pht('Edit OAuth Server');
+  }
+
+  protected function getObjectCreateShortText() {
+    return pht('Create OAuth Server');
+  }
+
+  protected function getObjectName() {
+    return pht('OAuth Server');
+  }
+
+  protected function getObjectViewURI($object) {
+    return $object->getViewURI();
+  }
+
+  protected function getCreateNewObjectPolicy() {
+    return $this->getApplication()->getPolicy(
+      PhabricatorOAuthServerCreateClientsCapability::CAPABILITY);
+  }
+
+  protected function buildCustomEditFields($object) {
+    return array(
+      id(new PhabricatorTextEditField())
+        ->setKey('name')
+        ->setLabel(pht('Name'))
+        ->setIsRequired(true)
+        ->setTransactionType(PhabricatorOAuthServerTransaction::TYPE_NAME)
+        ->setDescription(pht('The name of the OAuth application.'))
+        ->setConduitDescription(pht('Rename the application.'))
+        ->setConduitTypeDescription(pht('New application name.'))
+        ->setValue($object->getName()),
+      id(new PhabricatorTextEditField())
+        ->setKey('redirectURI')
+        ->setLabel(pht('Redirect URI'))
+        ->setIsRequired(true)
+        ->setTransactionType(
+          PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI)
+        ->setDescription(
+          pht('The redirect URI for OAuth handshakes.'))
+        ->setConduitDescription(
+          pht(
+            'Change where this application redirects users to during OAuth '.
+            'handshakes.'))
+        ->setConduitTypeDescription(
+          pht(
+            'New OAuth application redirect URI.'))
+        ->setValue($object->getRedirectURI()),
+    );
+  }
+
+}
--- a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php
+++ b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php
@@ -0,0 +1,137 @@
+<?php
+
+final class PhabricatorOAuthServerEditor
+  extends PhabricatorApplicationTransactionEditor {
+
+  public function getEditorApplicationClass() {
+    return 'PhabricatorOAuthServerApplication';
+  }
+
+  public function getEditorObjectsDescription() {
+    return pht('OAuth Applications');
+  }
+
+  public function getTransactionTypes() {
+    $types = parent::getTransactionTypes();
+
+    $types[] = PhabricatorOAuthServerTransaction::TYPE_NAME;
+    $types[] = PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI;
+
+    $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
+    $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
+
+    return $types;
+  }
+
+  protected function getCustomTransactionOldValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorOAuthServerTransaction::TYPE_NAME:
+        return $object->getName();
+      case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI:
+        return $object->getRedirectURI();
+    }
+  }
+
+  protected function getCustomTransactionNewValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorOAuthServerTransaction::TYPE_NAME:
+      case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI:
+        return $xaction->getNewValue();
+    }
+  }
+
+  protected function applyCustomInternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorOAuthServerTransaction::TYPE_NAME:
+        $object->setName($xaction->getNewValue());
+        return;
+      case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI:
+        $object->setRedirectURI($xaction->getNewValue());
+        return;
+    }
+
+    return parent::applyCustomInternalTransaction($object, $xaction);
+  }
+
+  protected function applyCustomExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorOAuthServerTransaction::TYPE_NAME:
+      case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI:
+        return;
+    }
+
+    return parent::applyCustomExternalTransaction($object, $xaction);
+  }
+
+  protected function validateTransaction(
+    PhabricatorLiskDAO $object,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateTransaction($object, $type, $xactions);
+
+    switch ($type) {
+      case PhabricatorOAuthServerTransaction::TYPE_NAME:
+        $missing = $this->validateIsEmptyTextField(
+          $object->getName(),
+          $xactions);
+
+        if ($missing) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            pht('OAuth applications must have a name.'),
+            nonempty(last($xactions), null));
+
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+        }
+        break;
+      case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI:
+        $missing = $this->validateIsEmptyTextField(
+          $object->getRedirectURI(),
+          $xactions);
+        if ($missing) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            pht('OAuth applications must have a valid redirect URI.'),
+            nonempty(last($xactions), null));
+
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+        } else {
+          foreach ($xactions as $xaction) {
+            $redirect_uri = $xaction->getNewValue();
+
+            try {
+              $server = new PhabricatorOAuthServer();
+              $server->assertValidRedirectURI($redirect_uri);
+            } catch (Exception $ex) {
+              $errors[] = new PhabricatorApplicationTransactionValidationError(
+                $type,
+                pht('Invalid'),
+                $ex->getMessage(),
+                $xaction);
+            }
+          }
+        }
+        break;
+    }
+
+    return $errors;
+  }
+
+}
--- a/src/applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php
+++ b/src/applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php
@@ -0,0 +1,10 @@
+<?php
+
+final class PhabricatorOAuthServerTransactionQuery
+  extends PhabricatorApplicationTransactionQuery {
+
+  public function getTemplateApplicationTransaction() {
+    return new PhabricatorOAuthServerTransaction();
+  }
+
+}
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
@@ -4,6 +4,7 @@ final class PhabricatorOAuthServerClient
  extends PhabricatorOAuthServerDAO
  implements
    PhabricatorPolicyInterface,
+    PhabricatorApplicationTransactionInterface,
    PhabricatorDestructibleInterface {

  protected $secret;
@@ -16,7 +17,7 @@ final class PhabricatorOAuthServerClient

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

  public function getViewURI() {
@@ -92,8 +93,32 @@ final class PhabricatorOAuthServerClient
    return null;
  }

+
+/* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
+
+
+  public function getApplicationTransactionEditor() {
+    return new PhabricatorOAuthServerEditor();
+  }
+
+  public function getApplicationTransactionObject() {
+    return $this;
+  }
+
+  public function getApplicationTransactionTemplate() {
+    return new PhabricatorOAuthServerTransaction();
+  }
+
+  public function willRenderTimeline(
+    PhabricatorApplicationTransactionView $timeline,
+    AphrontRequest $request) {
+    return $timeline;
+  }
+
+
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */

+
  public function destroyObjectPermanently(
    PhabricatorDestructionEngine $engine) {

--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php
@@ -0,0 +1,52 @@
+<?php
+
+final class PhabricatorOAuthServerTransaction
+  extends PhabricatorApplicationTransaction {
+
+  const TYPE_NAME = 'oauthserver.name';
+  const TYPE_REDIRECT_URI = 'oauthserver.redirect-uri';
+
+  public function getApplicationName() {
+    return 'oauth_server';
+  }
+
+  public function getTableName() {
+    return 'oauth_server_transaction';
+  }
+
+  public function getApplicationTransactionType() {
+    return PhabricatorOAuthServerClientPHIDType::TYPECONST;
+  }
+
+  public function getApplicationTransactionCommentObject() {
+    return null;
+  }
+
+  public function getTitle() {
+    $author_phid = $this->getAuthorPHID();
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_CREATE:
+        return pht(
+          '%s created this OAuth application.',
+          $this->renderHandleLink($author_phid));
+      case self::TYPE_NAME:
+        return pht(
+          '%s renamed this application from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $old,
+          $new);
+      case self::TYPE_REDIRECT_URI:
+        return pht(
+          '%s changed the application redirect URI from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $old,
+          $new);
+    }
+
+    return parent::getTitle();
+  }
+
+}