Remove one-time login from username change email

Summary:
Depends on D20100. Ref T7732. Ref T13244. This is a bit of an adventure.

Long ago, passwords were digested with usernames as part of the salt. This was a mistake: it meant that your password becomes invalid if your username is changed.

(I think very very long ago, some other hashing may also have used usernames -- perhaps session hashing or CSRF hashing?)

To work around this, the "username change" email included a one-time login link and some language about resetting your password.

This flaw was fixed when passwords were moved to shared infrastructure (they're now salted more cleanly on a per-digest basis), and since D18908 (about a year ago) we've transparently upgraded password digests on use.

Although it's still technically possible that a username change could invalidate your password, it requires:

- You set the password on a version of Phabricator earlier than ~2018 Week 5 (about a year ago).
- You haven't logged into a version of Phabricator newer than that using your password since then.
- Your username is changed.

This probably affects more than zero users, but I suspect not //many// more than zero. These users can always use "Forgot password?" to recover account access.

Since the value of this is almost certainly very near zero now and declining over time, just get rid of it. Also move the actual mail out of `PhabricatorUser`, ala the similar recent change to welcome mail in D19989.

Test Plan: Changed a user's username, reviewed resulting mail with `bin/mail show-outbound`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244, T7732

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

epriestley 7 years ago 113a2773 9632c704

+74 -52

4 changed files

expand all

src

__phutil_library_map__.php

applications

people

mail

PhabricatorPeopleUsernameMailEngine.php

storage

PhabricatorUser.php

xaction

PhabricatorUserUsernameTransaction.php

src/__phutil_library_map__.php

··· 3899 3899 'PhabricatorPeopleTransactionQuery' => 'applications/people/query/PhabricatorPeopleTransactionQuery.php', 3900 3900 'PhabricatorPeopleUserFunctionDatasource' => 'applications/people/typeahead/PhabricatorPeopleUserFunctionDatasource.php', 3901 3901 'PhabricatorPeopleUserPHIDType' => 'applications/people/phid/PhabricatorPeopleUserPHIDType.php', 3902 + 'PhabricatorPeopleUsernameMailEngine' => 'applications/people/mail/PhabricatorPeopleUsernameMailEngine.php', 3902 3903 'PhabricatorPeopleWelcomeController' => 'applications/people/controller/PhabricatorPeopleWelcomeController.php', 3903 3904 'PhabricatorPeopleWelcomeMailEngine' => 'applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php', 3904 3905 'PhabricatorPhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorPhabricatorAuthProvider.php', ··· 9895 9896 'PhabricatorPeopleTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 9896 9897 'PhabricatorPeopleUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 9897 9898 'PhabricatorPeopleUserPHIDType' => 'PhabricatorPHIDType', 9899 + 'PhabricatorPeopleUsernameMailEngine' => 'PhabricatorPeopleMailEngine', 9898 9900 'PhabricatorPeopleWelcomeController' => 'PhabricatorPeopleController', 9899 9901 'PhabricatorPeopleWelcomeMailEngine' => 'PhabricatorPeopleMailEngine', 9900 9902 'PhabricatorPhabricatorAuthProvider' => 'PhabricatorOAuth2AuthProvider',

+60

src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php

··· 1 + <?php 2 + 3 + final class PhabricatorPeopleUsernameMailEngine 4 + extends PhabricatorPeopleMailEngine { 5 + 6 + private $oldUsername; 7 + private $newUsername; 8 + 9 + public function setNewUsername($new_username) { 10 + $this->newUsername = $new_username; 11 + return $this; 12 + } 13 + 14 + public function getNewUsername() { 15 + return $this->newUsername; 16 + } 17 + 18 + public function setOldUsername($old_username) { 19 + $this->oldUsername = $old_username; 20 + return $this; 21 + } 22 + 23 + public function getOldUsername() { 24 + return $this->oldUsername; 25 + } 26 + 27 + public function validateMail() { 28 + return; 29 + } 30 + 31 + protected function newMail() { 32 + $sender = $this->getSender(); 33 + $recipient = $this->getRecipient(); 34 + 35 + $sender_username = $sender->getUsername(); 36 + $sender_realname = $sender->getRealName(); 37 + 38 + $old_username = $this->getOldUsername(); 39 + $new_username = $this->getNewUsername(); 40 + 41 + $body = sprintf( 42 + "%s\n\n %s\n %s\n", 43 + pht( 44 + '%s (%s) has changed your Phabricator username.', 45 + $sender_username, 46 + $sender_realname), 47 + pht( 48 + 'Old Username: %s', 49 + $old_username), 50 + pht( 51 + 'New Username: %s', 52 + $new_username)); 53 + 54 + return id(new PhabricatorMetaMTAMail()) 55 + ->addTos(array($recipient->getPHID())) 56 + ->setSubject(pht('[Phabricator] Username Changed')) 57 + ->setBody($body); 58 + } 59 + 60 + }

-49

src/applications/people/storage/PhabricatorUser.php

··· 555 555 } 556 556 } 557 557 558 - public function sendUsernameChangeEmail( 559 - PhabricatorUser $admin, 560 - $old_username) { 561 - 562 - $admin_username = $admin->getUserName(); 563 - $admin_realname = $admin->getRealName(); 564 - $new_username = $this->getUserName(); 565 - 566 - $password_instructions = null; 567 - if (PhabricatorPasswordAuthProvider::getPasswordProvider()) { 568 - $engine = new PhabricatorAuthSessionEngine(); 569 - $uri = $engine->getOneTimeLoginURI( 570 - $this, 571 - null, 572 - PhabricatorAuthSessionEngine::ONETIME_USERNAME); 573 - $password_instructions = sprintf( 574 - "%s\n\n %s\n\n%s\n", 575 - pht( 576 - "If you use a password to login, you'll need to reset it ". 577 - "before you can login again. You can reset your password by ". 578 - "following this link:"), 579 - $uri, 580 - pht( 581 - "And, of course, you'll need to use your new username to login ". 582 - "from now on. If you use OAuth to login, nothing should change.")); 583 - } 584 - 585 - $body = sprintf( 586 - "%s\n\n %s\n %s\n\n%s", 587 - pht( 588 - '%s (%s) has changed your Phabricator username.', 589 - $admin_username, 590 - $admin_realname), 591 - pht( 592 - 'Old Username: %s', 593 - $old_username), 594 - pht( 595 - 'New Username: %s', 596 - $new_username), 597 - $password_instructions); 598 - 599 - $mail = id(new PhabricatorMetaMTAMail()) 600 - ->addTos(array($this->getPHID())) 601 - ->setForceDelivery(true) 602 - ->setSubject(pht('[Phabricator] Username Changed')) 603 - ->setBody($body) 604 - ->saveAndSend(); 605 - } 606 - 607 558 public static function describeValidUsername() { 608 559 return pht( 609 560 'Usernames must contain only numbers, letters, period, underscore and '.

+12 -3

src/applications/people/xaction/PhabricatorUserUsernameTransaction.php

··· 18 18 } 19 19 20 20 public function applyExternalEffects($object, $value) { 21 + $actor = $this->getActor(); 21 22 $user = $object; 23 + 24 + $old_username = $this->getOldValue(); 25 + $new_username = $this->getNewValue(); 22 26 23 27 $this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME) 24 - ->setOldValue($this->getOldValue()) 25 - ->setNewValue($value) 28 + ->setOldValue($old_username) 29 + ->setNewValue($new_username) 26 30 ->save(); 27 31 28 32 // The SSH key cache currently includes usernames, so dirty it. See T12554 29 33 // for discussion. 30 34 PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache(); 31 35 32 - $user->sendUsernameChangeEmail($this->getActor(), $this->getOldValue()); 36 + id(new PhabricatorPeopleUsernameMailEngine()) 37 + ->setSender($actor) 38 + ->setRecipient($object) 39 + ->setOldUsername($old_username) 40 + ->setNewUsername($new_username) 41 + ->sendMail(); 33 42 } 34 43 35 44 public function getTitle() {

Configure Feed

Configure Feed