Allow authentication providers to store and customize additional configuration

Summary: Ref T1536. None of this code is reachable. For the new web UI for auth edits, give providers more and better customization options for handling the form. Allow them to format transactions. Also fix the "Auth" application icon. Test Plan: {F46718} Reviewers: btrahan, chad Reviewed By: chad CC: aran Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6221
2024-12-23 05:50:55 +01:00 · 2013-06-18 10:02:34 -07:00 · 2013-06-18 10:02:34 -07:00 · e58f383d91
commit e58f383d91
parent 1eafc2b638
7 changed files with 210 additions and 44 deletions
--- a/src/applications/auth/application/PhabricatorApplicationAuth.php
+++ b/src/applications/auth/application/PhabricatorApplicationAuth.php
@ -10,6 +10,10 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication {
    return '/auth/';
  }

+  public function getIconName() {
+    return 'authentication';
+  }
+
  public function buildMainMenuItems(
    PhabricatorUser $user,
    PhabricatorController $controller = null) {
--- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php
@ -77,34 +77,48 @@ final class PhabricatorAuthEditController
    $v_unlink = $config->getShouldAllowUnlink();

    if ($request->isFormPost()) {
+
+      $properties = $provider->readFormValuesFromRequest($request);
+      list($errors, $issues, $properties) = $provider->processEditForm(
+        $request,
+        $properties);
+
      $xactions = array();

-      if ($is_new) {
+      if (!$errors) {
+        if ($is_new) {
+          $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
+            ->setTransactionType(
+              PhabricatorAuthProviderConfigTransaction::TYPE_ENABLE)
+            ->setNewValue(1);
+
+          $config->setProviderType($provider->getProviderType());
+          $config->setProviderDomain($provider->getProviderDomain());
+        }
+
        $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
          ->setTransactionType(
-            PhabricatorAuthProviderConfigTransaction::TYPE_ENABLE)
-          ->setNewValue(1);
+            PhabricatorAuthProviderConfigTransaction::TYPE_REGISTRATION)
+          ->setNewValue($request->getInt('allowRegistration', 0));

-        $config->setProviderType($provider->getProviderType());
-        $config->setProviderDomain($provider->getProviderDomain());
-      }
+        $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
+          ->setTransactionType(
+            PhabricatorAuthProviderConfigTransaction::TYPE_LINK)
+          ->setNewValue($request->getInt('allowLink', 0));

-      $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
-        ->setTransactionType(
-          PhabricatorAuthProviderConfigTransaction::TYPE_REGISTRATION)
-        ->setNewValue($request->getInt('allowRegistration', 0));
+        $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
+          ->setTransactionType(
+            PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK)
+          ->setNewValue($request->getInt('allowUnlink', 0));

-      $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
-        ->setTransactionType(
-          PhabricatorAuthProviderConfigTransaction::TYPE_LINK)
-        ->setNewValue($request->getInt('allowLink', 0));
+        foreach ($properties as $key => $value) {
+          $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
+            ->setTransactionType(
+              PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY)
+            ->setMetadataValue('auth:property', $key)
+            ->setNewValue($value);
+        }

-      $xactions[] = id(new PhabricatorAuthProviderConfigTransaction())
-        ->setTransactionType(
-          PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK)
-        ->setNewValue($request->getInt('allowUnlink', 0));
-
-      if (!$errors) {
        $editor = id(new PhabricatorAuthProviderConfigEditor())
          ->setActor($viewer)
          ->setContentSourceFromRequest($request)
@ -114,6 +128,9 @@ final class PhabricatorAuthEditController
        return id(new AphrontRedirectResponse())->setURI(
          $this->getApplicationURI());
      }
+    } else {
+      $properties = $provider->readFormValuesFromProvider();
+      $issues = array();
    }

    if ($errors) {
@ -204,7 +221,7 @@ final class PhabricatorAuthEditController
            $str_unlink,
            $v_unlink));

-    $provider->extendEditForm($form);
+    $provider->extendEditForm($request, $form, $properties, $issues);

    $form
      ->appendChild(
@ -224,6 +241,10 @@ final class PhabricatorAuthEditController
        ->setViewer($viewer)
        ->execute();

+      foreach ($xactions as $xaction) {
+        $xaction->setProvider($provider);
+      }
+
      $xaction_view = id(new PhabricatorApplicationTransactionView())
        ->setUser($viewer)
        ->setTransactions($xactions);
--- a/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php
+++ b/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php
@ -33,8 +33,9 @@ final class PhabricatorAuthProviderConfigEditor
      case PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK:
        return (int)$object->getShouldAllowUnlink();
      case PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY:
-        // TODO
-        throw new Exception("TODO");
+        $key = $xaction->getMetadataValue(
+          PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY);
+        return $object->getProperty($key);
    }
  }

@ -66,8 +67,9 @@ final class PhabricatorAuthProviderConfigEditor
      case PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK:
        return $object->setShouldAllowUnlink($v);
      case PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY:
-        // TODO
-        throw new Exception("TODO");
+        $key = $xaction->getMetadataValue(
+          PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY);
+        return $object->setProperty($key, $v);
    }
  }

--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@ -125,11 +125,6 @@ abstract class PhabricatorAuthProvider {
    throw new Exception("Not implemented!");
  }

-  public function extendEditForm(AphrontFormView $form) {
-
-  }
-
-
  public function createProviders() {
    return array($this);
  }
@ -258,4 +253,36 @@ abstract class PhabricatorAuthProvider {
    return false;
  }

+  public function renderConfigPropertyTransactionTitle(
+    PhabricatorAuthProviderConfigTransaction $xaction) {
+
+    return null;
+  }
+
+  public function readFormValuesFromProvider() {
+    return array();
+  }
+
+  public function readFormValuesFromRequest(AphrontRequest $request) {
+    return array();
+  }
+
+  public function processEditForm(
+    AphrontRequest $request,
+    array $values) {
+
+    $errors = array();
+    $issues = array();
+
+    return array($errors, $issues, $values);
+  }
+
+  public function extendEditForm(
+    AphrontRequest $request,
+    AphrontFormView $form,
+    array $values,
+    array $issues) {
+    return;
+  }
+
 }
--- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
+++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
@ -170,35 +170,127 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
    return array($this->loadOrCreateAccount($account_id), $response);
  }

-  public function extendEditForm(
-    AphrontFormView $form) {
-
-    $v_id = $this->getOAuthClientID();
+  const PROPERTY_APP_ID = 'oauth:app:id';
+  const PROPERTY_APP_SECRET = 'oauth:app:secret';

+  public function readFormValuesFromProvider() {
    $secret = $this->getOAuthClientSecret();
    if ($secret) {
-      $v_secret = str_repeat('*', strlen($secret->openEnvelope()));
-    } else {
-      $v_secret = '';
+      $secret = $secret->openEnvelope();
    }

-    $e_id = strlen($v_id) ? null : true;
-    $e_secret = strlen($v_secret) ? null : true;
+    return array(
+      self::PROPERTY_APP_ID     => $this->getOAuthClientID(),
+      self::PROPERTY_APP_SECRET => $secret,
+    );
+  }
+
+  public function readFormValuesFromRequest(AphrontRequest $request) {
+    return array(
+      self::PROPERTY_APP_ID     => $request->getStr(self::PROPERTY_APP_ID),
+      self::PROPERTY_APP_SECRET => $request->getStr(self::PROPERTY_APP_SECRET),
+    );
+  }
+
+  public function processEditForm(
+    AphrontRequest $request,
+    array $values) {
+    $errors = array();
+    $issues = array();
+
+    $key_id = self::PROPERTY_APP_ID;
+    $key_secret = self::PROPERTY_APP_SECRET;
+
+    if (!strlen($values[$key_id])) {
+      $errors[] = pht('Application ID is required.');
+      $issues[$key_id] = pht('Required');
+    }
+
+    if (!strlen($values[$key_secret])) {
+      $errors[] = pht('Application secret is required.');
+      $issues[$key_id] = pht('Required');
+    }
+
+    // If the user has not changed the secret, don't update it (that is,
+    // don't cause a bunch of "****" to be written to the database).
+    if (preg_match('/^[*]+$/', $values[$key_secret])) {
+      unset($values[$key_secret]);
+    }
+
+    return array($errors, $issues, $values);
+  }
+
+  public function extendEditForm(
+    AphrontRequest $request,
+    AphrontFormView $form,
+    array $values,
+    array $issues) {
+
+    $key_id = self::PROPERTY_APP_ID;
+    $key_secret = self::PROPERTY_APP_SECRET;
+
+    $v_id = $values[$key_id];
+    $v_secret = $values[$key_secret];
+    if ($v_secret) {
+      $v_secret = str_repeat('*', strlen($v_secret));
+    }
+
+    $e_id = idx($issues, $key_id, $request->isFormPost() ? null : true);
+    $e_secret = idx($issues, $key_secret, $request->isFormPost() ? null : true);

    $form
      ->appendChild(
        id(new AphrontFormTextControl())
          ->setLabel(pht('OAuth App ID'))
-          ->setName('oauth:app:id')
+          ->setName($key_id)
          ->setValue($v_id)
          ->setError($e_id))
      ->appendChild(
        id(new AphrontFormPasswordControl())
          ->setLabel(pht('OAuth App Secret'))
-          ->setName('oauth:app:secret')
+          ->setName($key_secret)
          ->setValue($v_secret)
          ->setError($e_secret));
+  }

+  public function renderConfigPropertyTransactionTitle(
+    PhabricatorAuthProviderConfigTransaction $xaction) {
+
+    $author_phid = $xaction->getAuthorPHID();
+    $old = $xaction->getOldValue();
+    $new = $xaction->getNewValue();
+    $key = $xaction->getMetadataValue(
+      PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY);
+
+    switch ($key) {
+      case self::PROPERTY_APP_ID:
+        if (strlen($old)) {
+          return pht(
+            '%s updated the OAuth application ID for this provider from '.
+            '"%s" to "%s".',
+            $xaction->renderHandleLink($author_phid),
+            $old,
+            $new);
+        } else {
+          return pht(
+            '%s set the OAuth application ID for this provider to '.
+            '"%s".',
+            $xaction->renderHandleLink($author_phid),
+            $new);
+        }
+      case self::PROPERTY_APP_SECRET:
+        if (strlen($old)) {
+          return pht(
+            '%s updated the OAuth application secret for this provider.',
+            $xaction->renderHandleLink($author_phid));
+        } else {
+          return pht(
+            '%s set the OAuth application seceret for this provider.',
+            $xaction->renderHandleLink($author_phid));
+        }
+    }
+
+    return parent::renderConfigPropertyTransactionTitle($xaction);
  }

 }
--- a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
+++ b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
@ -9,6 +9,19 @@ final class PhabricatorAuthProviderConfigTransaction
  const TYPE_UNLINK         = 'config:unlink';
  const TYPE_PROPERTY       = 'config:property';

+  const PROPERTY_KEY        = 'auth:property';
+
+  private $provider;
+
+  public function setProvider(PhabricatorAuthProvider $provider) {
+    $this->provider = $provider;
+    return $this;
+  }
+
+  public function getProvider() {
+    return $this->provider;
+  }
+
  public function getApplicationName() {
    return 'auth';
  }
@ -113,7 +126,14 @@ final class PhabricatorAuthProviderConfigTransaction
        }
        break;
      case self::TYPE_PROPERTY:
-        // TODO
+        $provider = $this->getProvider();
+        if ($provider) {
+          $title = $provider->renderConfigPropertyTransactionTitle($this);
+          if (strlen($title)) {
+            return $title;
+          }
+        }
+
        return pht(
          '%s edited a property of this provider.',
          $this->renderHandleLink($author_phid));
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@ -163,7 +163,7 @@ abstract class PhabricatorApplicationTransaction
    return $this->handles;
  }

-  protected function renderHandleLink($phid) {
+  public function renderHandleLink($phid) {
    if ($this->renderingTarget == self::TARGET_HTML) {
      return $this->getHandle($phid)->renderLink();
    } else {
@ -171,7 +171,7 @@ abstract class PhabricatorApplicationTransaction
    }
  }

-  protected function renderHandleList(array $phids) {
+  public function renderHandleList(array $phids) {
    $links = array();
    foreach ($phids as $phid) {
      $links[] = $this->renderHandleLink($phid);