From d8394b2ee053b7874bf30fbac385d3bebbf31cb6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 20 Jun 2013 11:17:53 -0700 Subject: [PATCH] Prepare for db-driven auth configuration by making proviers operate in dual modes Summary: Ref T1536. This sets us for the "Config -> Database" migration. Basically: - If stuff is defined in the database, respect the database stuff (no installs have anything defined yet since they can't reach the interfaces/code). - Otherwise, respect the config stuff (all installs currently do this). Test Plan: Saw database stuff respected when database stuff was defined; saw config stuff respected otherwise. Reviewers: chad, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6240 --- .../config/PhabricatorAuthEditController.php | 19 ++- .../auth/provider/PhabricatorAuthProvider.php | 111 ++++++++++++++---- .../provider/PhabricatorAuthProviderLDAP.php | 25 ++-- .../provider/PhabricatorAuthProviderOAuth.php | 41 +++++-- .../PhabricatorAuthProviderOAuthDisqus.php | 19 +-- .../PhabricatorAuthProviderOAuthFacebook.php | 23 ++-- .../PhabricatorAuthProviderOAuthGitHub.php | 18 +-- .../PhabricatorAuthProviderOAuthGoogle.php | 17 +-- .../PhabricatorAuthProviderPassword.php | 8 +- 9 files changed, 186 insertions(+), 95 deletions(-) diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php index c06beaa4e9..1f50db00aa 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php @@ -60,12 +60,8 @@ final class PhabricatorAuthEditController throw new Exception("This provider is already configured!"); } - $config = id(new PhabricatorAuthProviderConfig()) - ->setProviderClass(get_class($provider)) - ->setShouldAllowLogin(1) - ->setShouldAllowRegistration(1) - ->setShouldAllowLink(1) - ->setShouldAllowUnlink(1); + $config = $provider->getDefaultProviderConfig(); + $provider->attachProviderConfig($config); $is_new = true; } @@ -87,11 +83,6 @@ final class PhabricatorAuthEditController if (!$errors) { if ($is_new) { - $xactions[] = id(new PhabricatorAuthProviderConfigTransaction()) - ->setTransactionType( - PhabricatorAuthProviderConfigTransaction::TYPE_ENABLE) - ->setNewValue(1); - $config->setProviderType($provider->getProviderType()); $config->setProviderDomain($provider->getProviderDomain()); } @@ -177,7 +168,11 @@ final class PhabricatorAuthEditController $status_tag = id(new PhabricatorTagView()) ->setType(PhabricatorTagView::TYPE_STATE); - if ($config->getIsEnabled()) { + if ($is_new) { + $status_tag + ->setName(pht('New Provider')) + ->setBackgroundColor('blue'); + } else if ($config->getIsEnabled()) { $status_tag ->setName(pht('Enabled')) ->setBackgroundColor('green'); diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 8c347b5099..3674c94c89 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -9,12 +9,26 @@ abstract class PhabricatorAuthProvider { return $this; } + public function hasProviderConfig() { + return (bool)$this->providerConfig; + } + public function getProviderConfig() { - if ($this->config === null) { + if ($this->providerConfig === null) { throw new Exception( "Call attachProviderConfig() before getProviderConfig()!"); } - return $this->config; + return $this->providerConfig; + } + + public function getDefaultProviderConfig() { + return id(new PhabricatorAuthProviderConfig()) + ->setProviderClass(get_class($this)) + ->setIsEnabled(1) + ->setShouldAllowLogin(1) + ->setShouldAllowRegistration(1) + ->setShouldAllowLink(1) + ->setShouldAllowUnlink(1); } public function getNameForCreate() { @@ -56,23 +70,52 @@ abstract class PhabricatorAuthProvider { if ($providers === null) { $objects = self::getAllBaseProviders(); + $configs = id(new PhabricatorAuthProviderConfigQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->execute(); + $providers = array(); - $from_class_map = array(); - foreach ($objects as $object) { - $from_class = get_class($object); - $object_providers = $object->createProviders(); - assert_instances_of($object_providers, 'PhabricatorAuthProvider'); - foreach ($object_providers as $provider) { - $key = $provider->getProviderKey(); - if (isset($providers[$key])) { - $first_class = $from_class_map[$key]; - throw new Exception( - "PhabricatorAuthProviders '{$first_class}' and '{$from_class}' ". - "both created authentication providers identified by key ". - "'{$key}'. Provider keys must be unique."); + if ($configs) { + foreach ($configs as $config) { + if (!isset($objects[$config->getProviderClass()])) { + // This configuration is for a provider which is not installed. + continue; + } + + $object = clone $objects[$config->getProviderClass()]; + $object->attachProviderConfig($config); + + $key = $object->getProviderKey(); + if (isset($providers[$key])) { + throw new Exception( + pht( + "Two authentication providers use the same provider key ". + "('%s'). Each provider must be identified by a unique ". + "key.", + $key)); + } + $providers[$key] = $object; + } + } else { + // TODO: Remove this once we transition to be completely database + // driven. + $from_class_map = array(); + foreach ($objects as $object) { + $from_class = get_class($object); + $object_providers = $object->createProviders(); + assert_instances_of($object_providers, 'PhabricatorAuthProvider'); + foreach ($object_providers as $provider) { + $key = $provider->getProviderKey(); + if (isset($providers[$key])) { + $first_class = $from_class_map[$key]; + throw new Exception( + "PhabricatorAuthProviders '{$first_class}' and ". + "'{$from_class}' both created authentication providers ". + "identified by key '{$key}'. Provider keys must be unique."); + } + $providers[$key] = $provider; + $from_class_map[$key] = $from_class; } - $providers[$key] = $provider; - $from_class_map[$key] = $from_class; } } } @@ -98,13 +141,39 @@ abstract class PhabricatorAuthProvider { abstract public function getAdapter(); public function isEnabled() { + if ($this->hasProviderConfig()) { + return $this->getProviderConfig()->getIsEnabled(); + } return true; } - abstract public function shouldAllowLogin(); - abstract public function shouldAllowRegistration(); - abstract public function shouldAllowAccountLink(); - abstract public function shouldAllowAccountUnlink(); + public function shouldAllowLogin() { + if ($this->hasProviderConfig()) { + return $this->getProviderConfig()->getShouldAllowLogin(); + } + return true; + } + + public function shouldAllowRegistration() { + if ($this->hasProviderConfig()) { + return $this->getProviderConfig()->getShouldAllowRegistration(); + } + return true; + } + + public function shouldAllowAccountLink() { + if ($this->hasProviderConfig()) { + return $this->getProviderConfig()->getShouldAllowLink(); + } + return true; + } + + public function shouldAllowAccountUnlink() { + if ($this->hasProviderConfig()) { + return $this->getProviderConfig()->getShouldAllowUnlink(); + } + return true; + } public function buildLoginForm( PhabricatorAuthStartController $controller) { diff --git a/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php b/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php index 8f88990023..9f21b8c9af 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php @@ -15,8 +15,17 @@ final class PhabricatorAuthProviderLDAP 'LDAP credentials to log in to Phabricator.'); } + public function getDefaultProviderConfig() { + return parent::getDefaultProviderConfig() + ->setProperty(self::KEY_PORT, 389) + ->setProperty(self::KEY_VERSION, 3); + } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && PhabricatorEnv::getEnvConfig('ldap.auth-enabled'); } @@ -49,22 +58,6 @@ final class PhabricatorAuthProviderLDAP return $this->adapter; } - public function shouldAllowLogin() { - return true; - } - - public function shouldAllowRegistration() { - return true; - } - - public function shouldAllowAccountLink() { - return true; - } - - public function shouldAllowAccountUnlink() { - return true; - } - protected function renderLoginForm(AphrontRequest $request, $mode) { $viewer = $request->getUser(); diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php index 6e76ee1ebb..883cd9099e 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php @@ -22,18 +22,31 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && $this->getOAuthClientID() && $this->getOAuthClientSecret(); } protected function configureAdapter(PhutilAuthAdapterOAuth $adapter) { - if ($this->getOAuthClientID()) { - $adapter->setClientID($this->getOAuthClientID()); - } - if ($this->getOAuthClientSecret()) { - $adapter->setClientSecret($this->getOAuthClientSecret()); + if ($this->hasProviderConfig()) { + $config = $this->getProviderConfig(); + $adapter->setClientID($config->getProperty(self::PROPERTY_APP_ID)); + $adapter->setClientSecret( + new PhutilOpaqueEnvelope( + $config->getProperty(self::PROPERTY_APP_SECRET))); + } else { + if ($this->getOAuthClientID()) { + $adapter->setClientID($this->getOAuthClientID()); + } + + if ($this->getOAuthClientSecret()) { + $adapter->setClientSecret($this->getOAuthClientSecret()); + } } $adapter->setRedirectURI($this->getLoginURI()); @@ -174,13 +187,21 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { const PROPERTY_APP_SECRET = 'oauth:app:secret'; public function readFormValuesFromProvider() { - $secret = $this->getOAuthClientSecret(); - if ($secret) { - $secret = $secret->openEnvelope(); + + if ($this->hasProviderConfig()) { + $config = $this->getProviderConfig(); + $id = $config->getProperty(self::PROPERTY_APP_ID); + $secret = $config->getProperty(self::PROPERTY_APP_SECRET); + } else { + $id = $this->getOAuthClientID(); + $secret = $this->getOAuthClientSecret(); + if ($secret) { + $secret = $secret->openEnvelope(); + } } return array( - self::PROPERTY_APP_ID => $this->getOAuthClientID(), + self::PROPERTY_APP_ID => $id, self::PROPERTY_APP_SECRET => $secret, ); } @@ -208,7 +229,7 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { if (!strlen($values[$key_secret])) { $errors[] = pht('Application secret is required.'); - $issues[$key_id] = pht('Required'); + $issues[$key_secret] = pht('Required'); } // If the user has not changed the secret, don't update it (that is, diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuthDisqus.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuthDisqus.php index eb866c6121..3f4a88ed09 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuthDisqus.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuthDisqus.php @@ -16,6 +16,10 @@ final class PhabricatorAuthProviderOAuthDisqus } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && PhabricatorEnv::getEnvConfig('disqus.auth-enabled'); } @@ -32,19 +36,18 @@ final class PhabricatorAuthProviderOAuthDisqus return null; } - public function shouldAllowLogin() { - return true; - } - public function shouldAllowRegistration() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowRegistration(); + } return PhabricatorEnv::getEnvConfig('disqus.registration-enabled'); } - public function shouldAllowAccountLink() { - return true; - } - public function shouldAllowAccountUnlink() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowAccountUnlink(); + } + return !PhabricatorEnv::getEnvConfig('disqus.auth-permanent'); } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php index 607e5aff9d..cfb8d84a6b 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php @@ -9,6 +9,11 @@ final class PhabricatorAuthProviderOAuthFacebook return pht('Facebook'); } + public function getDefaultProviderConfig() { + return parent::getDefaultProviderConfig() + ->setProperty(self::KEY_REQUIRE_SECURE, 1); + } + protected function newOAuthAdapter() { $secure_only = PhabricatorEnv::getEnvConfig('facebook.require-https-auth'); return id(new PhutilAuthAdapterOAuthFacebook()) @@ -20,6 +25,10 @@ final class PhabricatorAuthProviderOAuthFacebook } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && PhabricatorEnv::getEnvConfig('facebook.auth-enabled'); } @@ -36,19 +45,17 @@ final class PhabricatorAuthProviderOAuthFacebook return null; } - public function shouldAllowLogin() { - return true; - } - public function shouldAllowRegistration() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowRegistration(); + } return PhabricatorEnv::getEnvConfig('facebook.registration-enabled'); } - public function shouldAllowAccountLink() { - return true; - } - public function shouldAllowAccountUnlink() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowAccountUnlink(); + } return !PhabricatorEnv::getEnvConfig('facebook.auth-permanent'); } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuthGitHub.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuthGitHub.php index fdb6bdeccd..f0c09fb25a 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuthGitHub.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuthGitHub.php @@ -16,6 +16,10 @@ final class PhabricatorAuthProviderOAuthGitHub } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && PhabricatorEnv::getEnvConfig('github.auth-enabled'); } @@ -32,19 +36,17 @@ final class PhabricatorAuthProviderOAuthGitHub return null; } - public function shouldAllowLogin() { - return true; - } - public function shouldAllowRegistration() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowRegistration(); + } return PhabricatorEnv::getEnvConfig('github.registration-enabled'); } - public function shouldAllowAccountLink() { - return true; - } - public function shouldAllowAccountUnlink() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowAccountUnlink(); + } return !PhabricatorEnv::getEnvConfig('github.auth-permanent'); } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuthGoogle.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuthGoogle.php index a9dfd98820..70ea28fa7e 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuthGoogle.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuthGoogle.php @@ -16,6 +16,9 @@ final class PhabricatorAuthProviderOAuthGoogle } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } return parent::isEnabled() && PhabricatorEnv::getEnvConfig('google.auth-enabled'); } @@ -32,19 +35,17 @@ final class PhabricatorAuthProviderOAuthGoogle return null; } - public function shouldAllowLogin() { - return true; - } - public function shouldAllowRegistration() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowRegistration(); + } return PhabricatorEnv::getEnvConfig('google.registration-enabled'); } - public function shouldAllowAccountLink() { - return true; - } - public function shouldAllowAccountUnlink() { + if ($this->hasProviderConfig()) { + return parent::shouldAllowAccountUnlink(); + } return !PhabricatorEnv::getEnvConfig('google.auth-permanent'); } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php index 42a4e813f6..6795ad64a0 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php @@ -15,6 +15,10 @@ final class PhabricatorAuthProviderPassword } public function isEnabled() { + if ($this->hasProviderConfig()) { + return parent::isEnabled(); + } + return parent::isEnabled() && PhabricatorEnv::getEnvConfig('auth.password-auth-enabled'); } @@ -34,10 +38,6 @@ final class PhabricatorAuthProviderPassword return '100-'.$this->getProviderName(); } - public function shouldAllowLogin() { - return true; - } - public function shouldAllowRegistration() { // TODO: Hard code this as "false" for now so we don't inadvertantly open // up password registration where it did not previously exist.