From 113a2773dd5bb41921dedc2205820d528ca42db6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Feb 2019 10:38:20 -0800 Subject: [PATCH] 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 --- src/__phutil_library_map__.php | 2 + .../PhabricatorPeopleUsernameMailEngine.php | 60 +++++++++++++++++++ .../people/storage/PhabricatorUser.php | 49 --------------- .../PhabricatorUserUsernameTransaction.php | 15 ++++- 4 files changed, 74 insertions(+), 52 deletions(-) create mode 100644 src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7bc8670661..b9d468ea3c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3899,6 +3899,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleTransactionQuery' => 'applications/people/query/PhabricatorPeopleTransactionQuery.php', 'PhabricatorPeopleUserFunctionDatasource' => 'applications/people/typeahead/PhabricatorPeopleUserFunctionDatasource.php', 'PhabricatorPeopleUserPHIDType' => 'applications/people/phid/PhabricatorPeopleUserPHIDType.php', + 'PhabricatorPeopleUsernameMailEngine' => 'applications/people/mail/PhabricatorPeopleUsernameMailEngine.php', 'PhabricatorPeopleWelcomeController' => 'applications/people/controller/PhabricatorPeopleWelcomeController.php', 'PhabricatorPeopleWelcomeMailEngine' => 'applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php', 'PhabricatorPhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorPhabricatorAuthProvider.php', @@ -9895,6 +9896,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorPeopleUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorPeopleUserPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorPeopleUsernameMailEngine' => 'PhabricatorPeopleMailEngine', 'PhabricatorPeopleWelcomeController' => 'PhabricatorPeopleController', 'PhabricatorPeopleWelcomeMailEngine' => 'PhabricatorPeopleMailEngine', 'PhabricatorPhabricatorAuthProvider' => 'PhabricatorOAuth2AuthProvider', diff --git a/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php b/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php new file mode 100644 index 0000000000..c954b7c38e --- /dev/null +++ b/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php @@ -0,0 +1,60 @@ +newUsername = $new_username; + return $this; + } + + public function getNewUsername() { + return $this->newUsername; + } + + public function setOldUsername($old_username) { + $this->oldUsername = $old_username; + return $this; + } + + public function getOldUsername() { + return $this->oldUsername; + } + + public function validateMail() { + return; + } + + protected function newMail() { + $sender = $this->getSender(); + $recipient = $this->getRecipient(); + + $sender_username = $sender->getUsername(); + $sender_realname = $sender->getRealName(); + + $old_username = $this->getOldUsername(); + $new_username = $this->getNewUsername(); + + $body = sprintf( + "%s\n\n %s\n %s\n", + pht( + '%s (%s) has changed your Phabricator username.', + $sender_username, + $sender_realname), + pht( + 'Old Username: %s', + $old_username), + pht( + 'New Username: %s', + $new_username)); + + return id(new PhabricatorMetaMTAMail()) + ->addTos(array($recipient->getPHID())) + ->setSubject(pht('[Phabricator] Username Changed')) + ->setBody($body); + } + +} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 0b18c292c2..70d2d9fb4e 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -555,55 +555,6 @@ final class PhabricatorUser } } - public function sendUsernameChangeEmail( - PhabricatorUser $admin, - $old_username) { - - $admin_username = $admin->getUserName(); - $admin_realname = $admin->getRealName(); - $new_username = $this->getUserName(); - - $password_instructions = null; - if (PhabricatorPasswordAuthProvider::getPasswordProvider()) { - $engine = new PhabricatorAuthSessionEngine(); - $uri = $engine->getOneTimeLoginURI( - $this, - null, - PhabricatorAuthSessionEngine::ONETIME_USERNAME); - $password_instructions = sprintf( - "%s\n\n %s\n\n%s\n", - pht( - "If you use a password to login, you'll need to reset it ". - "before you can login again. You can reset your password by ". - "following this link:"), - $uri, - pht( - "And, of course, you'll need to use your new username to login ". - "from now on. If you use OAuth to login, nothing should change.")); - } - - $body = sprintf( - "%s\n\n %s\n %s\n\n%s", - pht( - '%s (%s) has changed your Phabricator username.', - $admin_username, - $admin_realname), - pht( - 'Old Username: %s', - $old_username), - pht( - 'New Username: %s', - $new_username), - $password_instructions); - - $mail = id(new PhabricatorMetaMTAMail()) - ->addTos(array($this->getPHID())) - ->setForceDelivery(true) - ->setSubject(pht('[Phabricator] Username Changed')) - ->setBody($body) - ->saveAndSend(); - } - public static function describeValidUsername() { return pht( 'Usernames must contain only numbers, letters, period, underscore and '. diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php index f2226d0010..b436b76716 100644 --- a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php @@ -18,18 +18,27 @@ final class PhabricatorUserUsernameTransaction } public function applyExternalEffects($object, $value) { + $actor = $this->getActor(); $user = $object; + $old_username = $this->getOldValue(); + $new_username = $this->getNewValue(); + $this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME) - ->setOldValue($this->getOldValue()) - ->setNewValue($value) + ->setOldValue($old_username) + ->setNewValue($new_username) ->save(); // The SSH key cache currently includes usernames, so dirty it. See T12554 // for discussion. PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache(); - $user->sendUsernameChangeEmail($this->getActor(), $this->getOldValue()); + id(new PhabricatorPeopleUsernameMailEngine()) + ->setSender($actor) + ->setRecipient($object) + ->setOldUsername($old_username) + ->setNewUsername($new_username) + ->sendMail(); } public function getTitle() {