Use modern UI and policies in OAuth client editing

Summary: Updates this stuff a bit: - Add a global create permission for OAuth applications. The primary goal is to reduce attack surface area by making it more difficult for an adversary to do anything which requires that they create and configure an OAuth application/client. Normal users shouldn't generally need to create applications, OAuth is complex, and doing things with user accounts is inherently somewhat administrative. - Use normal policies to check create and edit permissions, now that we have infrastructure for it. - Use modern UI kit. Test Plan: - Created a client. - Edited a client. - Tried to create a client as a non-admin. - Tried to edit a client I don't own. {F131511} {F131512} {F131513} {F131514} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D8562
2024-12-19 12:00:55 +01:00 · 2014-03-18 13:30:48 -07:00 · 2014-03-18 13:30:48 -07:00 · 34c890b7e1
commit 34c890b7e1
parent 995a890565
7 changed files with 138 additions and 148 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -1718,6 +1718,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthServerAuthController' => 'applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php',
    'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/PhabricatorOAuthServerAuthorizationCode.php',
    'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'applications/oauthserver/panel/PhabricatorOAuthServerAuthorizationsSettingsPanel.php',
+    'PhabricatorOAuthServerCapabilityCreateClients' => 'applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php',
    'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/PhabricatorOAuthServerClient.php',
    'PhabricatorOAuthServerClientQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerClientQuery.php',
    'PhabricatorOAuthServerConsoleController' => 'applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php',
@ -4471,6 +4472,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController',
    'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO',
    'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel',
+    'PhabricatorOAuthServerCapabilityCreateClients' => 'PhabricatorPolicyCapability',
    'PhabricatorOAuthServerClient' =>
    array(
      0 => 'PhabricatorOAuthServerDAO',
--- a/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php
+++ b/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php
@ -48,4 +48,12 @@ final class PhabricatorApplicationOAuthServer extends PhabricatorApplication {
    );
  }

+  protected function getCustomCapabilities() {
+    return array(
+      PhabricatorOAuthServerCapabilityCreateClients::CAPABILITY => array(
+        'default' => PhabricatorPolicies::POLICY_ADMIN,
+      ),
+    );
+  }
+
 }
--- a/src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php
+++ b/src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php
@ -0,0 +1,20 @@
+<?php
+
+final class PhabricatorOAuthServerCapabilityCreateClients
+  extends PhabricatorPolicyCapability {
+
+  const CAPABILITY = 'oauthserver.create';
+
+  public function getCapabilityKey() {
+    return self::CAPABILITY;
+  }
+
+  public function getCapabilityName() {
+    return pht('Can Create OAuth Applications');
+  }
+
+  public function describeCapabilityRejection() {
+    return pht('You do not have permission to create OAuth applications.');
+  }
+
+}
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php
@ -1,7 +1,7 @@
 <?php

 abstract class PhabricatorOAuthServerController
-extends PhabricatorController {
+  extends PhabricatorController {

  public function buildStandardPageResponse($view, array $data) {
    $user = $this->getRequest()->getUser();
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php
@ -1,15 +1,14 @@
 <?php

-/**
- * @group oauthserver
- */
 abstract class PhabricatorOAuthClientBaseController
-extends PhabricatorOAuthServerController {
+  extends PhabricatorOAuthServerController {

  private $clientPHID;
+
  protected function getClientPHID() {
    return $this->clientPHID;
  }
+
  private function setClientPHID($phid) {
    $this->clientPHID = $phid;
    return $this;
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
@ -1,173 +1,117 @@
 <?php

-/**
- * @group oauthserver
- */
 final class PhabricatorOAuthClientEditController
-extends PhabricatorOAuthClientBaseController {
-
-  private $isEdit;
-  protected function isClientEdit() {
-    return $this->isEdit;
-  }
-  private function setIsClientEdit($is_edit) {
-    $this->isEdit = (bool) $is_edit;
-    return $this;
-  }
-
-  protected function getExtraClientFilters() {
-    if ($this->isClientEdit()) {
-      $filters = array(
-        array('url'   => $this->getFilter(),
-              'label' => 'Edit Client')
-      );
-    } else {
-      $filters = array();
-    }
-    return $filters;
-  }
-
-  public function getFilter() {
-    if ($this->isClientEdit()) {
-      $filter = 'client/edit/'.$this->getClientPHID();
-    } else {
-      $filter = 'client/create';
-    }
-    return $filter;
-  }
+  extends PhabricatorOAuthClientBaseController {

  public function processRequest() {
    $request = $this->getRequest();
-    $current_user  = $request->getUser();
-    $error         = null;
-    $bad_redirect  = false;
-    $phid          = $this->getClientPHID();
-    // if we have a phid, then we're editing
-    $this->setIsClientEdit($phid);
+    $viewer = $request->getUser();

-    if ($this->isClientEdit()) {
-      $client = id(new PhabricatorOAuthServerClient())
-        ->loadOneWhere('phid = %s',
-                       $phid);
-      $title  = 'Edit OAuth Client';
-      // validate the client
-      if (empty($client)) {
+    $phid = $this->getClientPHID();
+    if ($phid) {
+      $client = id(new PhabricatorOAuthServerClientQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($phid))
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
+        ->executeOne();
+      if (!$client) {
        return new Aphront404Response();
      }
-      if ($client->getCreatorPHID() != $current_user->getPHID()) {
-        $message = 'Access denied to edit client with id '.$phid.'. '.
-                   'Only the user who created the client has permission to '.
-                   'edit the client.';
-        return id(new Aphront403Response())
-          ->setForbiddenText($message);
-      }
-      $submit_button = 'Save OAuth Client';
-      $secret        = null;
-    // new client - much simpler
+
+      $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 {
-      $client        = new PhabricatorOAuthServerClient();
-      $title         = 'Create OAuth Client';
-      $submit_button = 'Create OAuth Client';
-      $secret        = Filesystem::readRandomCharacters(32);
+      $this->requireApplicationCapability(
+        PhabricatorOAuthServerCapabilityCreateClients::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 ($secret) {
-        $client->setSecret($secret);
-      }
-      $client->setCreatorPHID($current_user->getPHID());
-      $uri = new PhutilURI($redirect_uri);
-      $server = new PhabricatorOAuthServer();
-      if (!$server->validateRedirectURI($uri)) {
-        $error = new AphrontErrorView();
-        $error->setSeverity(AphrontErrorView::SEVERITY_ERROR);
-        $error->setTitle(
-          'Redirect URI must be a fully qualified domain name '.
-          'with no fragments. See '.
-          'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 '.
-          'for more information on the correct format.');
-        $bad_redirect = true;
-      } else {
-        $client->save();
-        // refresh the phid in case its a create
-        $phid = $client->getPHID();
-        if ($this->isClientEdit()) {
-          return id(new AphrontRedirectResponse())
-            ->setURI('/oauthserver/client/?edited='.$phid);
-        } else {
-          return id(new AphrontRedirectResponse())
-            ->setURI('/oauthserver/client/?new='.$phid);
-        }
-      }
+
+      if (!strlen($client->getName())) {
+        $errors[] = pht('You must choose a name for this OAuth application.');
+        $e_name = pht('Required');
      }

-    $panel = new AphrontPanelView();
-    if ($this->isClientEdit()) {
-      $delete_button = phutil_tag(
-        'a',
-        array(
-          'href' => $client->getDeleteURI(),
-          'class' => 'grey button',
-        ),
-        'Delete OAuth Client');
-      $panel->addButton($delete_button);
+      $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');
+      }
+
+      if (!$errors) {
+        $client->save();
+        $view_uri = $client->getViewURI();
+        return id(new AphrontRedirectResponse())->setURI($view_uri);
+      }
    }
-    $panel->setHeader($title);

    $form = id(new AphrontFormView())
-      ->setUser($current_user)
+      ->setUser($viewer)
      ->appendChild(
        id(new AphrontFormTextControl())
          ->setLabel('Name')
          ->setName('name')
-        ->setValue($client->getName()));
-    if ($this->isClientEdit()) {
-      $form
-        ->appendChild(
-          id(new AphrontFormTextControl())
-          ->setLabel('ID')
-          ->setValue($phid))
-        ->appendChild(
-          id(new AphrontFormStaticControl())
-          ->setLabel('Secret')
-          ->setValue($client->getSecret()));
-    }
-    $form
+          ->setValue($client->getName())
+          ->setError($e_name))
      ->appendChild(
        id(new AphrontFormTextControl())
          ->setLabel('Redirect URI')
          ->setName('redirect_uri')
          ->setValue($client->getRedirectURI())
-        ->setError($bad_redirect));
-    if ($this->isClientEdit()) {
-      $created = phabricator_datetime($client->getDateCreated(),
-                                      $current_user);
-      $updated = phabricator_datetime($client->getDateModified(),
-                                      $current_user);
-      $form
-        ->appendChild(
-          id(new AphrontFormStaticControl())
-          ->setLabel('Created')
-          ->setValue($created))
-        ->appendChild(
-          id(new AphrontFormStaticControl())
-          ->setLabel('Last Updated')
-          ->setValue($updated));
-    }
-    $form
+          ->setError($e_redirect))
      ->appendChild(
        id(new AphrontFormSubmitControl())
+          ->addCancelButton($cancel_uri)
          ->setValue($submit_button));

-    $panel->appendChild($form);
-    return $this->buildStandardPageResponse(
-      array($error,
-            $panel
+    $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->buildApplicationPage(
+      array(
+        $crumbs,
+        $box,
      ),
-      array('title' => $title));
+      array(
+        'title' => $title,
+        'device' => true,
+      ));
  }

 }
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
@ -21,6 +21,12 @@ final class PhabricatorOAuthServerClient
    return '/oauthserver/client/delete/'.$this->getPHID().'/';
  }

+  public static function initializeNewClient(PhabricatorUser $actor) {
+    return id(new PhabricatorOAuthServerClient())
+      ->setCreatorPHID($actor->getPHID())
+      ->setSecret(Filesystem::readRandomCharacters(32));
+  }
+
  public function getConfiguration() {
    return array(
      self::CONFIG_AUX_PHID => true,
@ -39,6 +45,7 @@ final class PhabricatorOAuthServerClient
  public function getCapabilities() {
    return array(
      PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
    );
  }

@ -46,14 +53,24 @@ final class PhabricatorOAuthServerClient
    switch ($capability) {
      case PhabricatorPolicyCapability::CAN_VIEW:
        return PhabricatorPolicies::POLICY_USER;
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        return PhabricatorPolicies::POLICY_NOONE;
    }
  }

  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        return ($viewer->getPHID() == $this->getCreatorPHID());
+    }
    return false;
  }

  public function describeAutomaticCapability($capability) {
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        return pht("Only an application's creator can edit it.");
+    }
    return null;
  }