Update unusual handling of external accounts in "Password" auth provider

Summary:
Depends on D21013. Ref T13493. When users log in with most providers, the provider returns an "ExternalAccount" identifier (like an Asana account GUID) and the workflow figures out where to go from there, usually a decision to try to send the user to registration (if the external account isn't linked to anything yet) or login (if it is).

In the case of password providers, the password is really a property of an existing account, so sending the user to registration never makes sense. We can bypass the "external identifier" indirection layer and just say "username -> internal account" instead of "external GUID -> internal mapping -> internal account".

Formalize this so that "AuthProvider" can generate either a "map this external account" value or a "use this internal account" value.

This stops populating "accountID" on "password" "ExternalAccount" objects, but this was only an artifact of convenience. (These records don't really need to exist at all, but there's little harm in going down the same workflow as everything else for consistency.)

Test Plan: Logged in with a username/password. Wiped the external account table and repeated the process.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21014

epriestley 6 years ago 05eb16d6 e43ecad8

+45 -19

6 changed files

expand all

src

applications

auth

controller

PhabricatorAuthRegisterController.php

provider

PhabricatorAuthProvider.php

PhabricatorLDAPAuthProvider.php

PhabricatorOAuth1AuthProvider.php

PhabricatorOAuth2AuthProvider.php

PhabricatorPasswordAuthProvider.php

-1

src/applications/auth/controller/PhabricatorAuthRegisterController.php

··· 457 457 458 458 if (!$is_setup) { 459 459 $account->setUserPHID($user->getPHID()); 460 - $provider->willRegisterAccount($account); 461 460 $account->save(); 462 461 } 463 462

+34 -4

src/applications/auth/provider/PhabricatorAuthProvider.php

··· 186 186 return; 187 187 } 188 188 189 - public function willRegisterAccount(PhabricatorExternalAccount $account) { 190 - return; 191 - } 189 + final protected function newExternalAccountForIdentifiers( 190 + array $identifiers) { 192 191 193 - protected function loadOrCreateAccount(array $identifiers) { 194 192 assert_instances_of($identifiers, 'PhabricatorExternalAccountIdentifier'); 195 193 196 194 if (!$identifiers) { ··· 234 232 implode(', ', $raw_identifiers))); 235 233 } 236 234 235 + return $this->didUpdateAccount($account); 236 + } 237 + 238 + final protected function newExternalAccountForUser(PhabricatorUser $user) { 239 + $config = $this->getProviderConfig(); 240 + 241 + // When a user logs in with a provider like username/password, they 242 + // always already have a Phabricator account (since there's no way they 243 + // could have a username otherwise). 244 + 245 + // These users should never go to registration, so we're building a 246 + // dummy "external account" which just links directly back to their 247 + // internal account. 248 + 249 + $account = id(new PhabricatorExternalAccountQuery()) 250 + ->setViewer($user) 251 + ->withProviderConfigPHIDs(array($config->getPHID())) 252 + ->withUserPHIDs(array($user->getPHID())) 253 + ->executeOne(); 254 + if (!$account) { 255 + $account = $this->newExternalAccount() 256 + ->setUserPHID($user->getPHID()); 257 + 258 + // TODO: Remove this when "accountID" is removed; the column is not 259 + // nullable. 260 + $account->setAccountID(''); 261 + } 262 + 263 + return $this->didUpdateAccount($account); 264 + } 265 + 266 + private function didUpdateAccount(PhabricatorExternalAccount $account) { 237 267 $adapter = $this->getAdapter(); 238 268 239 269 $account->setUsername($adapter->getAccountName());

+3 -1

src/applications/auth/provider/PhabricatorLDAPAuthProvider.php

··· 180 180 } 181 181 } 182 182 183 - return array($this->loadOrCreateAccount($identifiers), $response); 183 + $account = $this->newExternalAccountForIdentifiers($identifiers); 184 + 185 + return array($account, $response); 184 186 } 185 187 186 188

+3 -1

src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php

··· 115 115 return array($account, $response); 116 116 } 117 117 118 - return array($this->loadOrCreateAccount($identifiers), $response); 118 + $account = $this->newExternalAccountForIdentifiers($identifiers); 119 + 120 + return array($account, $response); 119 121 } 120 122 121 123 public function processEditForm(

+3 -1

src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php

··· 95 95 return array($account, $response); 96 96 } 97 97 98 - return array($this->loadOrCreateAccount($identifiers), $response); 98 + $account = $this->newExternalAccountForIdentifiers($identifiers); 99 + 100 + return array($account, $response); 99 101 } 100 102 101 103 public function processEditForm(

+2 -11

src/applications/auth/provider/PhabricatorPasswordAuthProvider.php

··· 305 305 ->setObject($user); 306 306 307 307 if ($engine->isValidPassword($envelope)) { 308 - $account = $this->loadOrCreateAccount($user->getPHID()); 308 + $account = $this->newExternalAccountForUser($user); 309 309 $log_user = $user; 310 310 } 311 311 } ··· 339 339 return true; 340 340 } 341 341 342 - protected function willSaveAccount(PhabricatorExternalAccount $account) { 343 - parent::willSaveAccount($account); 344 - $account->setUserPHID($account->getAccountID()); 345 - } 346 - 347 - public function willRegisterAccount(PhabricatorExternalAccount $account) { 348 - parent::willRegisterAccount($account); 349 - $account->setAccountID($account->getUserPHID()); 350 - } 351 - 352 342 public static function getPasswordProvider() { 353 343 $providers = self::getAllEnabledProviders(); 354 344 ··· 375 365 public function shouldAllowEmailTrustConfiguration() { 376 366 return false; 377 367 } 368 + 378 369 }

Configure Feed

Configure Feed