Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13175840
D21019.id.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
15 KB
Referenced Files
None
Subscribers
None
D21019.id.diff
View Options
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
Details
Attached
Mime Type
text/plain
Expires
Thu, May 9, 7:53 AM (3 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6270995
Default Alt Text
D21019.id.diff (15 KB)
Attached To
Mode
D21019: Remove all readers and all nontrivial writers for "accountType" and "accountDomain" on "ExternalAccount"
Attached
Detach File
Event Timeline
Log In to Comment