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(); }