Page MenuHomePhabricator
Files F15394078

D21016.diff
No OneTemporary
Actions

D21016.diff
View Options

diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -67,7 +67,7 @@
->setWorkflowKey($workflow_key)
->requireHighSecurityToken($viewer, $request, $done_uri);
- $account->delete();
+ $account->unlinkAccount();
id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
$viewer,
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
@@ -216,6 +216,7 @@
->setViewer($viewer)
->withProviderConfigPHIDs(array($config->getPHID()))
->withAccountIDs($raw_identifiers)
+ ->needAccountIdentifiers(true)
->execute();
if (!$accounts) {
$account = $this->newExternalAccount()
@@ -232,6 +233,17 @@
implode(', ', $raw_identifiers)));
}
+ // See T13493. Add all the identifiers to the account. In the case where
+ // an account initially has a lower-quality identifier (like an email
+ // address) and later adds a higher-quality identifier (like a GUID), this
+ // allows us to automatically upgrade toward the higher-quality identifier
+ // and survive API changes which remove the lower-quality identifier more
+ // gracefully.
+
+ foreach ($identifiers as $identifier) {
+ $account->appendIdentifier($identifier);
+ }
+
return $this->didUpdateAccount($account);
}
@@ -351,7 +363,8 @@
return id(new PhabricatorExternalAccount())
->setAccountType($adapter->getAdapterType())
->setAccountDomain($adapter->getAdapterDomain())
- ->setProviderConfigPHID($config->getPHID());
+ ->setProviderConfigPHID($config->getPHID())
+ ->attachAccountIdentifiers(array());
}
public function getLoginOrder() {
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
@@ -22,6 +22,7 @@
private $needImages;
private $accountSecrets;
private $providerConfigPHIDs;
+ private $needAccountIdentifiers;
public function withUserPHIDs(array $user_phids) {
$this->userPHIDs = $user_phids;
@@ -63,6 +64,11 @@
return $this;
}
+ public function needAccountIdentifiers($need) {
+ $this->needAccountIdentifiers = $need;
+ return $this;
+ }
+
public function withProviderConfigPHIDs(array $phids) {
$this->providerConfigPHIDs = $phids;
return $this;
@@ -132,6 +138,23 @@
}
}
+ if ($this->needAccountIdentifiers) {
+ $account_phids = mpull($accounts, 'getPHID');
+
+ $identifiers = id(new PhabricatorExternalAccountIdentifierQuery())
+ ->setViewer($viewer)
+ ->setParentQuery($this)
+ ->withExternalAccountPHIDs($account_phids)
+ ->execute();
+
+ $identifiers = mgroup($identifiers, 'getExternalAccountPHID');
+ foreach ($accounts as $account) {
+ $account_phid = $account->getPHID();
+ $account_identifiers = idx($identifiers, $account_phid, array());
+ $account->attachAccountIdentifiers($account_identifiers);
+ }
+ }
+
return $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
@@ -23,6 +23,7 @@
private $profileImageFile = self::ATTACHABLE;
private $providerConfig = self::ATTACHABLE;
+ private $accountIdentifiers = self::ATTACHABLE;
public function getProfileImageFile() {
return $this->assertAttached($this->profileImageFile);
@@ -78,7 +79,48 @@
if (!$this->getAccountSecret()) {
$this->setAccountSecret(Filesystem::readRandomCharacters(32));
}
- return parent::save();
+
+ $this->openTransaction();
+
+ $result = parent::save();
+
+ $account_phid = $this->getPHID();
+ $config_phid = $this->getProviderConfigPHID();
+
+ if ($this->accountIdentifiers !== self::ATTACHABLE) {
+ foreach ($this->getAccountIdentifiers() as $identifier) {
+ $identifier
+ ->setExternalAccountPHID($account_phid)
+ ->setProviderConfigPHID($config_phid)
+ ->save();
+ }
+ }
+
+ $this->saveTransaction();
+
+ return $result;
+ }
+
+ public function unlinkAccount() {
+
+ // When unlinking an account, we disassociate it from the user and
+ // remove all the identifying information. We retain the PHID, the
+ // object itself, and the "ExternalAccountIdentifier" objects in the
+ // external table.
+
+ // TODO: This unlinks (but does not destroy) any profile image.
+
+ return $this
+ ->setUserPHID(null)
+ ->setDisplayName(null)
+ ->setUsername(null)
+ ->setRealName(null)
+ ->setEmail(null)
+ ->setEmailVerified(0)
+ ->setProfileImagePHID(null)
+ ->setAccountURI(null)
+ ->setProperties(array())
+ ->save();
}
public function setProperty($key, $value) {
@@ -131,6 +173,44 @@
return $this->assertAttached($this->providerConfig);
}
+ public function getAccountIdentifiers() {
+ $raw = $this->assertAttached($this->accountIdentifiers);
+ return array_values($raw);
+ }
+
+ public function attachAccountIdentifiers(array $identifiers) {
+ assert_instances_of($identifiers, 'PhabricatorExternalAccountIdentifier');
+ $this->accountIdentifiers = mpull($identifiers, null, 'getIdentifierRaw');
+ return $this;
+ }
+
+ public function appendIdentifier(
+ PhabricatorExternalAccountIdentifier $identifier) {
+
+ $this->assertAttached($this->accountIdentifiers);
+
+ $map = $this->accountIdentifiers;
+ $raw = $identifier->getIdentifierRaw();
+
+ $old = idx($map, $raw);
+ $new = $identifier;
+
+ if ($old === null) {
+ $result = $new;
+ } else {
+ // Here, we already know about an identifier and have rediscovered it.
+
+ // We could copy properties from the new version of the identifier here,
+ // or merge them in some other way (for example, update a "last seen
+ // from the provider" timestamp), but no such properties currently exist.
+ $result = $old;
+ }
+
+ $this->accountIdentifiers[$raw] = $result;
+
+ return $this;
+ }
+
/* -( PhabricatorPolicyInterface )----------------------------------------- */
@@ -182,6 +262,8 @@
$engine->destroyObject($identifier);
}
+ // TODO: This may leave a profile image behind.
+
$this->delete();
}
diff --git a/src/applications/people/storage/PhabricatorExternalAccountIdentifier.php b/src/applications/people/storage/PhabricatorExternalAccountIdentifier.php
--- a/src/applications/people/storage/PhabricatorExternalAccountIdentifier.php
+++ b/src/applications/people/storage/PhabricatorExternalAccountIdentifier.php
@@ -36,7 +36,10 @@
public function save() {
$identifier_raw = $this->getIdentifierRaw();
- $this->identiferHash = PhabricatorHash::digestForIndex($identifier_raw);
+
+ $identifier_hash = PhabricatorHash::digestForIndex($identifier_raw);
+ $this->setIdentifierHash($identifier_hash);
+
return parent::save();
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 16, 11:19 PM (1 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7389103
Default Alt Text
D21016.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility