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

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

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

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

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,

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

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

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

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

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.