From 8e3ea4e034eb43178aaac1bcfd836369fb5591af Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 16 Mar 2016 05:32:55 -0700 Subject: [PATCH] Use new modular temporary auth token constants in one-time login and password reset flows Summary: Ref T10603. This converts existing hard-codes to modular constants. Also removes one small piece of code duplication. Test Plan: - Performed one-time logins. - Performed a password reset. - Verified temporary tokens were revoked properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10603 Differential Revision: https://secure.phabricator.com/D15476 --- .../PhabricatorAuthOneTimeLoginController.php | 21 +++++++------------ .../engine/PhabricatorAuthSessionEngine.php | 17 ++++----------- .../people/editor/PhabricatorUserEditor.php | 6 +++--- .../PhabricatorPasswordSettingsPanel.php | 5 +++-- 4 files changed, 18 insertions(+), 31 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index ccc66ffeb7..9ecea74d6a 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -105,23 +105,17 @@ final class PhabricatorAuthOneTimeLoginController // the link in the "Welcome" email is good enough, without requiring users // to go through a second round of email verification. + $editor = id(new PhabricatorUserEditor()) + ->setActor($target_user); + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); // Nuke the token and all other outstanding password reset tokens. // There is no particular security benefit to destroying them all, but // it should reduce HackerOne reports of nebulous harm. - - PhabricatorAuthTemporaryToken::revokeTokens( - $target_user, - array($target_user->getPHID()), - array( - PhabricatorAuthSessionEngine::ONETIME_TEMPORARY_TOKEN_TYPE, - PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE, - )); + $editor->revokePasswordResetLinks($target_user); if ($target_email) { - id(new PhabricatorUserEditor()) - ->setActor($target_user) - ->verifyEmail($target_user, $target_email); + $editor->verifyEmail($target_user, $target_email); } unset($unguarded); @@ -133,12 +127,13 @@ final class PhabricatorAuthOneTimeLoginController // We're going to let the user reset their password without knowing // the old one. Generate a one-time token for that. $key = Filesystem::readRandomCharacters(16); + $password_type = + PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthTemporaryToken()) ->setObjectPHID($target_user->getPHID()) - ->setTokenType( - PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE) + ->setTokenType($password_type) ->setTokenExpires(time() + phutil_units('1 hour in seconds')) ->setTokenCode(PhabricatorHash::digest($key)) ->save(); diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index 77ba80bccb..e6289926dd 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -39,17 +39,6 @@ final class PhabricatorAuthSessionEngine extends Phobject { const KIND_UNKNOWN = '?'; - /** - * Temporary tokens for one time logins. - */ - const ONETIME_TEMPORARY_TOKEN_TYPE = 'login:onetime'; - - - /** - * Temporary tokens for password recovery after one time login. - */ - const PASSWORD_TEMPORARY_TOKEN_TYPE = 'login:password'; - const ONETIME_RECOVER = 'recover'; const ONETIME_RESET = 'reset'; const ONETIME_WELCOME = 'welcome'; @@ -642,11 +631,12 @@ final class PhabricatorAuthSessionEngine extends Phobject { $key = Filesystem::readRandomCharacters(32); $key_hash = $this->getOneTimeLoginKeyHash($user, $email, $key); + $onetime_type = PhabricatorAuthOneTimeLoginTemporaryTokenType::TOKENTYPE; $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthTemporaryToken()) ->setObjectPHID($user->getPHID()) - ->setTokenType(self::ONETIME_TEMPORARY_TOKEN_TYPE) + ->setTokenType($onetime_type) ->setTokenExpires(time() + phutil_units('1 day in seconds')) ->setTokenCode($key_hash) ->save(); @@ -685,11 +675,12 @@ final class PhabricatorAuthSessionEngine extends Phobject { $key = null) { $key_hash = $this->getOneTimeLoginKeyHash($user, $email, $key); + $onetime_type = PhabricatorAuthOneTimeLoginTemporaryTokenType::TOKENTYPE; return id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($user) ->withObjectPHIDs(array($user->getPHID())) - ->withTokenTypes(array(self::ONETIME_TEMPORARY_TOKEN_TYPE)) + ->withTokenTypes(array($onetime_type)) ->withTokenCodes(array($key_hash)) ->withExpired(false) ->executeOne(); diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index d7885ef101..3370fb428b 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -700,7 +700,7 @@ final class PhabricatorUserEditor extends PhabricatorEditor { } } - private function revokePasswordResetLinks(PhabricatorUser $user) { + public function revokePasswordResetLinks(PhabricatorUser $user) { // Revoke any outstanding password reset links. If an attacker compromises // an account, changes the email address, and sends themselves a password // reset link, it could otherwise remain live for a short period of time @@ -710,8 +710,8 @@ final class PhabricatorUserEditor extends PhabricatorEditor { $user, array($user->getPHID()), array( - PhabricatorAuthSessionEngine::ONETIME_TEMPORARY_TOKEN_TYPE, - PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE, + PhabricatorAuthOneTimeLoginTemporaryTokenType::TOKENTYPE, + PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE, )); } diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php index 56b8fad76a..dd26890fc2 100644 --- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php @@ -40,13 +40,14 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { // the workflow from a password reset email. $key = $request->getStr('key'); + $password_type = PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; + $token = null; if ($key) { $token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($user) ->withObjectPHIDs(array($user->getPHID())) - ->withTokenTypes( - array(PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE)) + ->withTokenTypes(array($password_type)) ->withTokenCodes(array(PhabricatorHash::digest($key))) ->withExpired(false) ->executeOne();