diff --git a/resources/sql/autopatches/20200222.xident.02.dropkey.php b/resources/sql/autopatches/20200222.xident.02.dropkey.php new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20200222.xident.02.dropkey.php @@ -0,0 +1,21 @@ +", which is obsolete. The application now violates +// this key, so make sure it gets dropped. + +// There's no "IF EXISTS" modifier for "ALTER TABLE" so run this as a PHP patch +// instead of an SQL patch. + +$table = new PhabricatorExternalAccount(); +$conn = $table->establishConnection('w'); + +try { + queryfx( + $conn, + 'ALTER TABLE %R DROP KEY %T', + $table, + 'account_details'); +} catch (AphrontQueryException $ex) { + // Ignore. +} diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -116,14 +116,21 @@ } } else { - // If the user already has a linked account of this type, prevent them - // from linking a second account. This can happen if they swap logins - // and then refresh the account link. See T6707. We will eventually - // allow this after T2549. + // If the user already has a linked account on this provider, prevent + // them from linking a second account. This can happen if they swap + // logins and then refresh the account link. + + // There's no technical reason we can't allow you to link multiple + // accounts from a single provider; disallowing this is currently a + // product deciison. See T2549. + $existing_accounts = id(new PhabricatorExternalAccountQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->withAccountTypes(array($account->getAccountType())) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->execute(); if ($existing_accounts) { return $this->renderError( diff --git a/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php b/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php --- a/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php +++ b/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php @@ -18,16 +18,6 @@ 'param' => 'user', 'help' => pht('Refresh tokens for a given user.'), ), - array( - 'name' => 'type', - 'param' => 'provider', - 'help' => pht('Refresh tokens for a given provider type.'), - ), - array( - 'name' => 'domain', - 'param' => 'domain', - 'help' => pht('Refresh tokens for a given domain.'), - ), )); } @@ -57,17 +47,6 @@ } } - - $type = $args->getArg('type'); - if (strlen($type)) { - $query->withAccountTypes(array($type)); - } - - $domain = $args->getArg('domain'); - if (strlen($domain)) { - $query->withAccountDomains(array($domain)); - } - $accounts = $query->execute(); if (!$accounts) { @@ -82,25 +61,24 @@ } $providers = PhabricatorAuthProvider::getAllEnabledProviders(); + $providers = mpull($providers, null, 'getProviderConfigPHID'); foreach ($accounts as $account) { $console->writeOut( "%s\n", pht( - 'Refreshing account #%d (%s/%s).', - $account->getID(), - $account->getAccountType(), - $account->getAccountDomain())); + 'Refreshing account #%d.', + $account->getID())); - $key = $account->getProviderKey(); - if (empty($providers[$key])) { + $config_phid = $account->getProviderConfigPHID(); + if (empty($providers[$config_phid])) { $console->writeOut( "> %s\n", pht('Skipping, provider is not enabled or does not exist.')); continue; } - $provider = $providers[$key]; + $provider = $providers[$config_phid]; if (!($provider instanceof PhabricatorOAuth2AuthProvider)) { $console->writeOut( "> %s\n", diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -20,6 +20,10 @@ return $this->providerConfig; } + public function getProviderConfigPHID() { + return $this->getProviderConfig()->getPHID(); + } + public function getConfigurationHelp() { return null; } @@ -360,11 +364,18 @@ $config = $this->getProviderConfig(); $adapter = $this->getAdapter(); - return id(new PhabricatorExternalAccount()) - ->setAccountType($adapter->getAdapterType()) - ->setAccountDomain($adapter->getAdapterDomain()) + $account = id(new PhabricatorExternalAccount()) ->setProviderConfigPHID($config->getPHID()) ->attachAccountIdentifiers(array()); + + // TODO: Remove this when these columns are removed. They no longer have + // readers or writers (other than this callsite). + + $account + ->setAccountType($adapter->getAdapterType()) + ->setAccountDomain($adapter->getAdapterDomain()); + + return $account; } public function getLoginOrder() { diff --git a/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php b/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php --- a/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php +++ b/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php @@ -199,7 +199,7 @@ PhabricatorExternalAccount $account, $force_refresh = false) { - if ($account->getProviderKey() !== $this->getProviderKey()) { + if ($account->getProviderConfigPHID() !== $this->getProviderConfigPHID()) { throw new Exception(pht('Account does not match provider!')); } diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php --- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php +++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php @@ -15,8 +15,6 @@ private $ids; private $phids; - private $accountTypes; - private $accountDomains; private $accountIDs; private $userPHIDs; private $needImages; @@ -34,16 +32,6 @@ return $this; } - public function withAccountDomains(array $account_domains) { - $this->accountDomains = $account_domains; - return $this; - } - - public function withAccountTypes(array $account_types) { - $this->accountTypes = $account_types; - return $this; - } - public function withPHIDs(array $phids) { $this->phids = $phids; return $this; @@ -175,20 +163,6 @@ $this->phids); } - if ($this->accountTypes !== null) { - $where[] = qsprintf( - $conn, - 'accountType IN (%Ls)', - $this->accountTypes); - } - - if ($this->accountDomains !== null) { - $where[] = qsprintf( - $conn, - 'accountDomain IN (%Ls)', - $this->accountDomains); - } - if ($this->accountIDs !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php @@ -35,8 +35,10 @@ $accounts = id(new PhabricatorExternalAccountQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->withAccountTypes(array($provider->getProviderType())) - ->withAccountDomains(array($provider->getProviderDomain())) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php @@ -30,7 +30,10 @@ $accounts = id(new PhabricatorExternalAccountQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->withAccountTypes(array($provider->getProviderType())) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php b/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php --- a/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php +++ b/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php @@ -65,8 +65,10 @@ $account = id(new PhabricatorExternalAccountQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->withAccountTypes(array($provider->getProviderType())) - ->withAccountDomains(array($provider->getProviderDomain())) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php b/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php --- a/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php +++ b/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php @@ -525,20 +525,7 @@ return $phid_map; } - $provider = PhabricatorAsanaAuthProvider::getAsanaProvider(); - - $accounts = id(new PhabricatorExternalAccountQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withUserPHIDs($all_phids) - ->withAccountTypes(array($provider->getProviderType())) - ->withAccountDomains(array($provider->getProviderDomain())) - ->needAccountIdentifiers(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->execute(); + $accounts = $this->loadAsanaExternalAccounts($all_phids); foreach ($accounts as $account) { $phid_map[$account->getUserPHID()] = $this->getAsanaAccountID($account); @@ -550,19 +537,21 @@ return $phid_map; } - private function findAnyValidAsanaAccessToken(array $user_phids) { - if (!$user_phids) { - return array(null, null, null); - } - + private function loadAsanaExternalAccounts(array $user_phids) { $provider = $this->getProvider(); $viewer = $this->getViewer(); + if (!$user_phids) { + return array(); + } + $accounts = id(new PhabricatorExternalAccountQuery()) - ->setViewer($viewer) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withUserPHIDs($user_phids) - ->withAccountTypes(array($provider->getProviderType())) - ->withAccountDomains(array($provider->getProviderDomain())) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->needAccountIdentifiers(true) ->requireCapabilities( array( @@ -571,6 +560,19 @@ )) ->execute(); + return $accounts; + } + + private function findAnyValidAsanaAccessToken(array $user_phids) { + $provider = $this->getProvider(); + $viewer = $this->getViewer(); + + if (!$user_phids) { + return array(null, null, null); + } + + $accounts = $this->loadAsanaExternalAccounts($user_phids); + // Reorder accounts in the original order. // TODO: This needs to be adjusted if/when we allow you to link multiple // accounts. diff --git a/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php b/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php --- a/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php +++ b/src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php @@ -74,14 +74,17 @@ $accounts = id(new PhabricatorExternalAccountQuery()) ->setViewer($viewer) ->withUserPHIDs($try_users) - ->withAccountTypes(array($provider->getProviderType())) - ->withAccountDomains(array($domain)) + ->withProviderConfigPHIDs( + array( + $provider->getProviderConfigPHID(), + )) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_EDIT, )) ->execute(); + // Reorder accounts in the original order. // TODO: This needs to be adjusted if/when we allow you to link multiple // accounts. diff --git a/src/applications/people/storage/PhabricatorExternalAccount.php b/src/applications/people/storage/PhabricatorExternalAccount.php --- a/src/applications/people/storage/PhabricatorExternalAccount.php +++ b/src/applications/people/storage/PhabricatorExternalAccount.php @@ -7,10 +7,7 @@ PhabricatorDestructibleInterface { protected $userPHID; - protected $accountType; - protected $accountDomain; protected $accountSecret; - protected $accountID; protected $displayName; protected $username; protected $realName; @@ -21,6 +18,12 @@ protected $properties = array(); protected $providerConfigPHID; + // TODO: Remove these (see T13493). These columns are obsolete and have + // no readers and only trivial writers. + protected $accountType; + protected $accountDomain; + protected $accountID; + private $profileImageFile = self::ATTACHABLE; private $providerConfig = self::ATTACHABLE; private $accountIdentifiers = self::ATTACHABLE; @@ -60,21 +63,16 @@ 'accountURI' => 'text255?', ), self::CONFIG_KEY_SCHEMA => array( - 'account_details' => array( - 'columns' => array('accountType', 'accountDomain', 'accountID'), - 'unique' => true, - ), 'key_user' => array( 'columns' => array('userPHID'), ), + 'key_provider' => array( + 'columns' => array('providerConfigPHID', 'userPHID'), + ), ), ) + parent::getConfiguration(); } - public function getProviderKey() { - return $this->getAccountType().':'.$this->getAccountDomain(); - } - public function save() { if (!$this->getAccountSecret()) { $this->setAccountSecret(Filesystem::readRandomCharacters(32)); @@ -146,24 +144,6 @@ return true; } - public function getDisplayName() { - if (strlen($this->displayName)) { - return $this->displayName; - } - - // TODO: Figure out how much identifying information we're going to show - // to users about external accounts. For now, just show a string which is - // clearly not an error, but don't disclose any identifying information. - - $map = array( - 'email' => pht('Email User'), - ); - - $type = $this->getAccountType(); - - return idx($map, $type, pht('"%s" User', $type)); - } - public function attachProviderConfig(PhabricatorAuthProviderConfig $config) { $this->providerConfig = $config; return $this;