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