D21019.id50082.diff
No OneTemporary
Actions

Size

15 KB

Referenced Files

None

Subscribers

None

D21019.id50082.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

Mime Type: text/plain
Expires: Thu, Mar 20, 9:31 AM (3 d, 12 h ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 7376818
Default Alt Text: D21019.id50082.diff (15 KB)

D21019.id50082.diffNo OneTemporaryActions

D21019.id50082.diffView Options

File Metadata

Event Timeline

D21019.id50082.diff
No OneTemporary
Actions

D21019.id50082.diff
View Options