From e58f383d9111cd8728abcbe9eaa173de3a7392b3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Jun 2013 10:02:34 -0700 Subject: [PATCH] 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 --- .../PhabricatorApplicationAuth.php | 4 + .../config/PhabricatorAuthEditController.php | 63 ++++++---- .../PhabricatorAuthProviderConfigEditor.php | 10 +- .../auth/provider/PhabricatorAuthProvider.php | 37 +++++- .../provider/PhabricatorAuthProviderOAuth.php | 114 ++++++++++++++++-- ...abricatorAuthProviderConfigTransaction.php | 22 +++- .../PhabricatorApplicationTransaction.php | 4 +- 7 files changed, 210 insertions(+), 44 deletions(-) diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index 9607700b3a..8df53a63f1 100644 --- 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) { diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php index a7f4998657..c06beaa4e9 100644 --- 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); diff --git a/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php b/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php index d6694609a3..5088519ce5 100644 --- 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); } } diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index aeb3a6dfdf..8c347b5099 100644 --- 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; + } + } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php index f0c12c1f82..6e76ee1ebb 100644 --- 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); } } diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php index def9791a60..23c825caf1 100644 --- 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)); diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 64fd98d675..0874b357d0 100644 --- 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);