mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-22 21:40:55 +01:00
Remove all readers and all nontrivial writers for "accountType" and "accountDomain" on "ExternalAccount"
Summary: Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`. Remove this key. Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703. This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType". Test Plan: - Linked an OAuth1 account (JIRA). - Linked an OAuth2 account (Asana). - Used `bin/auth refresh` to cycle OAuth tokens. - Grepped for affected symbols. - Published an Asana update. - Published a JIRA link. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13493, T6703 Differential Revision: https://secure.phabricator.com/D21019
This commit is contained in:
parent
b8f0613b30
commit
84b5ad09e6
12 changed files with 104 additions and 121 deletions
21
resources/sql/autopatches/20200222.xident.02.dropkey.php
Normal file
21
resources/sql/autopatches/20200222.xident.02.dropkey.php
Normal file
|
@ -0,0 +1,21 @@
|
|||
<?php
|
||||
|
||||
// See T13493. This table previously had a UNIQUE KEY on "<accountType,
|
||||
// accountDomain, accountID>", 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.
|
||||
}
|
|
@ -116,14 +116,21 @@ final class PhabricatorAuthLoginController
|
|||
}
|
||||
} 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(
|
||||
|
|
|
@ -18,16 +18,6 @@ final class PhabricatorAuthManagementRefreshWorkflow
|
|||
'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 @@ final class PhabricatorAuthManagementRefreshWorkflow
|
|||
}
|
||||
}
|
||||
|
||||
|
||||
$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 @@ final class PhabricatorAuthManagementRefreshWorkflow
|
|||
}
|
||||
|
||||
$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",
|
||||
|
|
|
@ -20,6 +20,10 @@ abstract class PhabricatorAuthProvider extends Phobject {
|
|||
return $this->providerConfig;
|
||||
}
|
||||
|
||||
public function getProviderConfigPHID() {
|
||||
return $this->getProviderConfig()->getPHID();
|
||||
}
|
||||
|
||||
public function getConfigurationHelp() {
|
||||
return null;
|
||||
}
|
||||
|
@ -360,11 +364,18 @@ abstract class PhabricatorAuthProvider extends Phobject {
|
|||
$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() {
|
||||
|
|
|
@ -199,7 +199,7 @@ abstract class PhabricatorOAuth2AuthProvider
|
|||
PhabricatorExternalAccount $account,
|
||||
$force_refresh = false) {
|
||||
|
||||
if ($account->getProviderKey() !== $this->getProviderKey()) {
|
||||
if ($account->getProviderConfigPHID() !== $this->getProviderConfigPHID()) {
|
||||
throw new Exception(pht('Account does not match provider!'));
|
||||
}
|
||||
|
||||
|
|
|
@ -15,8 +15,6 @@ final class PhabricatorExternalAccountQuery
|
|||
|
||||
private $ids;
|
||||
private $phids;
|
||||
private $accountTypes;
|
||||
private $accountDomains;
|
||||
private $accountIDs;
|
||||
private $userPHIDs;
|
||||
private $needImages;
|
||||
|
@ -34,16 +32,6 @@ final class PhabricatorExternalAccountQuery
|
|||
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 @@ final class PhabricatorExternalAccountQuery
|
|||
$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,
|
||||
|
|
|
@ -35,8 +35,10 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge {
|
|||
$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,
|
||||
|
|
|
@ -30,7 +30,10 @@ final class DoorkeeperBridgeJIRA extends DoorkeeperBridge {
|
|||
$accounts = id(new PhabricatorExternalAccountQuery())
|
||||
->setViewer($viewer)
|
||||
->withUserPHIDs(array($viewer->getPHID()))
|
||||
->withAccountTypes(array($provider->getProviderType()))
|
||||
->withProviderConfigPHIDs(
|
||||
array(
|
||||
$provider->getProviderConfigPHID(),
|
||||
))
|
||||
->requireCapabilities(
|
||||
array(
|
||||
PhabricatorPolicyCapability::CAN_VIEW,
|
||||
|
|
|
@ -65,8 +65,10 @@ final class PhabricatorAsanaConfigOptions
|
|||
$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,
|
||||
|
|
|
@ -525,20 +525,7 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker {
|
|||
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 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker {
|
|||
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 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker {
|
|||
))
|
||||
->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.
|
||||
|
|
|
@ -74,14 +74,17 @@ final class DoorkeeperJIRAFeedWorker extends DoorkeeperFeedWorker {
|
|||
$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.
|
||||
|
|
|
@ -7,10 +7,7 @@ final class PhabricatorExternalAccount
|
|||
PhabricatorDestructibleInterface {
|
||||
|
||||
protected $userPHID;
|
||||
protected $accountType;
|
||||
protected $accountDomain;
|
||||
protected $accountSecret;
|
||||
protected $accountID;
|
||||
protected $displayName;
|
||||
protected $username;
|
||||
protected $realName;
|
||||
|
@ -21,6 +18,12 @@ final class PhabricatorExternalAccount
|
|||
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 @@ final class PhabricatorExternalAccount
|
|||
'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 @@ final class PhabricatorExternalAccount
|
|||
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;
|
||||
|
|
Loading…
Reference in a new issue