OAuthServer - hide client secret behind a "View Secret" action

Summary: ...also adds policies on who can view and who can edit an action. Fixes T6949. Test Plan: viewed a secret through the new UI and it worked Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T6949 Differential Revision: https://secure.phabricator.com/D11401
2024-09-20 01:08:50 +02:00 · 2015-01-14 17:27:45 -08:00 · 2015-01-14 17:27:45 -08:00 · 1cc81b1d0a
commit 1cc81b1d0a
parent 57761ce220
7 changed files with 123 additions and 16 deletions
--- a/resources/sql/autopatches/20150114.oauthserver.client.policy.sql
+++ b/resources/sql/autopatches/20150114.oauthserver.client.policy.sql
@ -0,0 +1,11 @@
+ALTER TABLE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient
+  ADD viewPolicy VARBINARY(64) NOT NULL AFTER creatorPHID;
+
+UPDATE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient
+  SET viewPolicy = 'users' WHERE viewPolicy = '';
+
+ALTER TABLE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient
+  ADD editPolicy VARBINARY(64) NOT NULL AFTER viewPolicy;
+
+UPDATE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient
+  SET editPolicy = creatorPHID WHERE viewPolicy = '';
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -2003,6 +2003,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthClientDeleteController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php',
    'PhabricatorOAuthClientEditController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php',
    'PhabricatorOAuthClientListController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php',
+    'PhabricatorOAuthClientSecretController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php',
    'PhabricatorOAuthClientViewController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php',
    'PhabricatorOAuthResponse' => 'applications/oauthserver/PhabricatorOAuthResponse.php',
    'PhabricatorOAuthServer' => 'applications/oauthserver/PhabricatorOAuthServer.php',
@ -5203,6 +5204,7 @@ phutil_register_library_map(array(
    'PhabricatorOAuthClientDeleteController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientEditController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientListController' => 'PhabricatorOAuthClientController',
+    'PhabricatorOAuthClientSecretController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthClientViewController' => 'PhabricatorOAuthClientController',
    'PhabricatorOAuthResponse' => 'AphrontResponse',
    'PhabricatorOAuthServerAccessToken' => 'PhabricatorOAuthServerDAO',
--- a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
+++ b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php
@ -51,6 +51,7 @@ final class PhabricatorOAuthServerApplication extends PhabricatorApplication {
          'delete/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientDeleteController',
          'edit/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientEditController',
          'view/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientViewController',
+          'secret/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientSecretController',
        ),
      ),
    );
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php
@ -64,6 +64,8 @@ final class PhabricatorOAuthClientEditController
        $e_redirect = pht('Invalid');
      }

+      $client->setViewPolicy($request->getStr('viewPolicy'));
+      $client->setEditPolicy($request->getStr('editPolicy'));
      if (!$errors) {
        $client->save();
        $view_uri = $client->getViewURI();
@ -71,6 +73,11 @@ final class PhabricatorOAuthClientEditController
      }
    }

+    $policies = id(new PhabricatorPolicyQuery())
+      ->setViewer($viewer)
+      ->setObject($client)
+      ->execute();
+
    $form = id(new AphrontFormView())
      ->setUser($viewer)
      ->appendChild(
@ -85,6 +92,20 @@ final class PhabricatorOAuthClientEditController
          ->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)
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php
@ -0,0 +1,70 @@
+<?php
+
+final class PhabricatorOAuthClientSecretController
+  extends PhabricatorOAuthClientController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $request->getUser();
+
+    $client = id(new PhabricatorOAuthServerClientQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($this->getClientPHID()))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+    if (!$client) {
+      return new Aphront404Response();
+    }
+
+    $view_uri = $client->getViewURI();
+    $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
+      $viewer,
+      $request,
+      $view_uri);
+
+    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));
+
+      $dialog = id(new AphrontDialogView())
+        ->setUser($viewer)
+        ->setWidth(AphrontDialogView::WIDTH_FORM)
+        ->setTitle(pht('Application Secret'))
+        ->appendChild($body)
+        ->addCancelButton($view_uri, pht('Done'));
+
+      return id(new AphrontDialogResponse())->setDialog($dialog);
+    }
+
+
+    $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business');
+
+    if ($is_serious) {
+      $body = pht(
+        'The secret associated with this oauth application will be shown in '.
+        'plain text on your screen.');
+    } else {
+      $body = pht(
+        'The secret associated with this oauth application will be shown in '.
+        'plain text on your screen. Before continuing, wrap your arms around '.
+        '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'))
+      ->addCancelButton($view_uri);
+  }
+
+}
--- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
+++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php
@ -62,6 +62,8 @@ final class PhabricatorOAuthClientViewController
      ->withClientPHIDs(array($client->getPHID()))
      ->executeOne();
    $is_authorized = (bool)$authorization;
+    $id = $client->getID();
+    $phid = $client->getPHID();

    $view = id(new PhabricatorActionListView())
      ->setUser($viewer);
@ -74,6 +76,14 @@ final class PhabricatorOAuthClientViewController
        ->setDisabled(!$can_edit)
        ->setHref($client->getEditURI()));

+    $view->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Show Application Secret'))
+        ->setIcon('fa-eye')
+        ->setHref($this->getApplicationURI("client/secret/{$phid}/"))
+        ->setDisabled(!$can_edit)
+        ->setWorkflow(true));
+
    $view->addAction(
      id(new PhabricatorActionView())
        ->setName(pht('Delete Application'))
@ -88,7 +98,7 @@ final class PhabricatorOAuthClientViewController
        ->setIcon('fa-wrench')
        ->setWorkflow(true)
        ->setDisabled($is_authorized)
-        ->setHref($this->getApplicationURI('test/'.$client->getID().'/')));
+        ->setHref($this->getApplicationURI('test/'.$id.'/')));

    return $view;
  }
@ -103,10 +113,6 @@ final class PhabricatorOAuthClientViewController
      pht('Client ID'),
      $client->getPHID());

-    $view->addProperty(
-      pht('Client Secret'),
-      $client->getSecret());
-
    $view->addProperty(
      pht('Redirect URI'),
      $client->getRedirectURI());
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php
@ -10,6 +10,8 @@ final class PhabricatorOAuthServerClient
  protected $name;
  protected $redirectURI;
  protected $creatorPHID;
+  protected $viewPolicy;
+  protected $editPolicy;

  public function getEditURI() {
    return '/oauthserver/client/edit/'.$this->getPHID().'/';
@ -26,7 +28,9 @@ final class PhabricatorOAuthServerClient
  public static function initializeNewClient(PhabricatorUser $actor) {
    return id(new PhabricatorOAuthServerClient())
      ->setCreatorPHID($actor->getPHID())
-      ->setSecret(Filesystem::readRandomCharacters(32));
+      ->setSecret(Filesystem::readRandomCharacters(32))
+      ->setViewPolicy(PhabricatorPolicies::POLICY_USER)
+      ->setEditPolicy($actor->getPHID());
  }

  protected function getConfiguration() {
@ -69,25 +73,17 @@ final class PhabricatorOAuthServerClient
  public function getPolicy($capability) {
    switch ($capability) {
      case PhabricatorPolicyCapability::CAN_VIEW:
-        return PhabricatorPolicies::POLICY_USER;
+        return $this->getViewPolicy();
      case PhabricatorPolicyCapability::CAN_EDIT:
-        return PhabricatorPolicies::POLICY_NOONE;
+        return $this->getEditPolicy();
    }
  }

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