Page MenuHomePhabricator

D21019.diff
No OneTemporary

D21019.diff

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 @@
+<?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.
+}
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;

File Metadata

Mime Type
text/plain
Expires
May 12 2024, 6:21 AM (4 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6270995
Default Alt Text
D21019.diff (15 KB)

Event Timeline