From d75007cf42d1169f14a8190f285d889ae6d444b4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Jan 2012 14:42:07 -0800 Subject: [PATCH] Validate logins, and simplify email password resets Summary: - There are some recent reports of login issues, see T755 and T754. I'm not really sure what's going on, but this is an attempt at getting some more information. - When we login a user by setting 'phusr' and 'phsid', send them to /login/validate/ to validate that the cookies actually got set. - Do email password resets in two steps: first, log the user in. Redirect them through validate, then give them the option to reset their password. - Don't CSRF logged-out users. It technically sort of works most of the time right now, but is silly. If we need logged-out CSRF we should generate it in some more reliable way. Test Plan: - Logged in with username/password. - Logged in with OAuth. - Logged in with email password reset. - Sent bad values to /login/validate/, got appropriate errors. - Reset password. - Verified next_uri still works. Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran, btrahan, j3kuntz Maniphest Tasks: T754, T755 Differential Revision: https://secure.phabricator.com/D1353 --- src/__phutil_library_map__.php | 4 + ...AphrontDefaultApplicationConfiguration.php | 4 +- .../PhabricatorEmailTokenController.php | 101 +++++----------- .../auth/controller/emailtoken/__init__.php | 6 +- .../login/PhabricatorLoginController.php | 23 ++-- .../auth/controller/login/__init__.php | 1 + .../oauth/PhabricatorOAuthLoginController.php | 15 ++- .../PhabricatorResetPasswordController.php | 108 ++++++++++++++++++ .../controller/resetpassword/__init__.php | 23 ++++ .../PhabricatorLoginValidateController.php | 95 +++++++++++++++ .../auth/controller/validate/__init__.php | 17 +++ .../people/storage/user/PhabricatorUser.php | 5 + 12 files changed, 306 insertions(+), 96 deletions(-) create mode 100644 src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php create mode 100644 src/applications/auth/controller/resetpassword/__init__.php create mode 100644 src/applications/auth/controller/validate/PhabricatorLoginValidateController.php create mode 100644 src/applications/auth/controller/validate/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e50eea8260..12e3646dda 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -499,6 +499,7 @@ phutil_register_library_map(array( 'PhabricatorLocalDiskFileStorageEngine' => 'applications/files/engine/localdisk', 'PhabricatorLocalTimeTestCase' => 'view/utils/__tests__', 'PhabricatorLoginController' => 'applications/auth/controller/login', + 'PhabricatorLoginValidateController' => 'applications/auth/controller/validate', 'PhabricatorLogoutController' => 'applications/auth/controller/logout', 'PhabricatorMailImplementationAdapter' => 'applications/metamta/adapter/base', 'PhabricatorMailImplementationAmazonSESAdapter' => 'applications/metamta/adapter/amazonses', @@ -640,6 +641,7 @@ phutil_register_library_map(array( 'PhabricatorRepositorySymbol' => 'applications/repository/storage/symbol', 'PhabricatorRepositoryTestCase' => 'applications/repository/storage/repository/__tests__', 'PhabricatorRepositoryType' => 'applications/repository/constants/repositorytype', + 'PhabricatorResetPasswordController' => 'applications/auth/controller/resetpassword', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/s3', 'PhabricatorSQLPatchList' => 'infrastructure/setup/sql', 'PhabricatorSearchAbstractDocument' => 'applications/search/index/abstractdocument', @@ -1165,6 +1167,7 @@ phutil_register_library_map(array( 'PhabricatorLocalDiskFileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase', 'PhabricatorLoginController' => 'PhabricatorAuthController', + 'PhabricatorLoginValidateController' => 'PhabricatorAuthController', 'PhabricatorLogoutController' => 'PhabricatorAuthController', 'PhabricatorMailImplementationAmazonSESAdapter' => 'PhabricatorMailImplementationPHPMailerLiteAdapter', 'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'PhabricatorMailImplementationAdapter', @@ -1287,6 +1290,7 @@ phutil_register_library_map(array( 'PhabricatorRepositorySvnCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker', 'PhabricatorRepositorySymbol' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase', + 'PhabricatorResetPasswordController' => 'PhabricatorAuthController', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorSearchAttachController' => 'PhabricatorSearchController', 'PhabricatorSearchBaseController' => 'PhabricatorController', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index aed6c09368..ae32fdc08f 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -1,7 +1,7 @@ 'PhabricatorEmailLoginController', 'etoken/(?P\w+)/$' => 'PhabricatorEmailTokenController', 'refresh/$' => 'PhabricatorRefreshCSRFController', + 'reset/$' => 'PhabricatorResetPasswordController', + 'validate/$' => 'PhabricatorLoginValidateController', ), '/logout/$' => 'PhabricatorLogoutController', diff --git a/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php b/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php index bc922aced2..d93e0ae97e 100644 --- a/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php +++ b/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php @@ -1,7 +1,7 @@ getUser()->getPHID()) { + $view = new AphrontRequestFailureView(); + $view->setHeader('Already Logged In'); + $view->appendChild( + '

You are already logged in.

'); + $view->appendChild( + '
'. + 'Return Home'. + '
'); + return $this->buildStandardPageResponse( + $view, + array( + 'title' => 'Already Logged In', + )); + } + $token = $this->token; $email = $request->getStr('email'); @@ -61,81 +77,18 @@ class PhabricatorEmailTokenController extends PhabricatorAuthController { )); } - if ($request->getUser()->getPHID() != $target_user->getPHID()) { - $session_key = $target_user->establishSession('web'); - $request->setCookie('phusr', $target_user->getUsername()); - $request->setCookie('phsid', $session_key); - } + $session_key = $target_user->establishSession('web'); + $request->setCookie('phusr', $target_user->getUsername()); + $request->setCookie('phsid', $session_key); - $errors = array(); - - $e_pass = true; - $e_confirm = true; - - if ($request->isFormPost()) { - $e_pass = 'Error'; - $e_confirm = 'Error'; - - $pass = $request->getStr('password'); - $confirm = $request->getStr('confirm'); - - if (strlen($pass) < 3) { - $errors[] = 'That password is ridiculously short.'; - } - - if ($pass !== $confirm) { - $errors[] = "Passwords do not match."; - } - - if (!$errors) { - $target_user->setPassword($pass); - $target_user->save(); - return id(new AphrontRedirectResponse()) - ->setURI('/'); - } - } - - if ($errors) { - $error_view = new AphrontErrorView(); - $error_view->setTitle('Password Reset Failed'); - $error_view->setErrors($errors); - } else { - $error_view = null; - } - - $form = new AphrontFormView(); - $form - ->setUser($target_user) - ->setAction('/login/etoken/'.$token.'/') - ->addHiddenInput('email', $email) - ->appendChild( - id(new AphrontFormPasswordControl()) - ->setLabel('New Password') - ->setName('password') - ->setError($e_pass)) - ->appendChild( - id(new AphrontFormPasswordControl()) - ->setLabel('Confirm Password') - ->setName('confirm') - ->setError($e_confirm)) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue('Reset Password') - ->addCancelButton('/', 'Skip')); - - $panel = new AphrontPanelView(); - $panel->setWidth(AphrontPanelView::WIDTH_FORM); - $panel->setHeader('Reset Password'); - $panel->appendChild($form); - - return $this->buildStandardPageResponse( + $uri = new PhutilURI('/login/validate/'); + $uri->setQueryParams( array( - $error_view, - $panel, - ), - array( - 'title' => 'Create New Account', + 'phusr' => $target_user->getUsername(), + 'next' => '/login/reset/', )); - } + return id(new AphrontRedirectResponse()) + ->setURI((string)$uri); + } } diff --git a/src/applications/auth/controller/emailtoken/__init__.php b/src/applications/auth/controller/emailtoken/__init__.php index f8775c76cd..b8b5598007 100644 --- a/src/applications/auth/controller/emailtoken/__init__.php +++ b/src/applications/auth/controller/emailtoken/__init__.php @@ -11,13 +11,9 @@ phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/auth/controller/base'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/env'); -phutil_require_module('phabricator', 'view/form/base'); -phutil_require_module('phabricator', 'view/form/control/password'); -phutil_require_module('phabricator', 'view/form/control/submit'); -phutil_require_module('phabricator', 'view/form/error'); -phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phabricator', 'view/page/failure'); +phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/auth/controller/login/PhabricatorLoginController.php b/src/applications/auth/controller/login/PhabricatorLoginController.php index 3d648f125f..86089dde06 100644 --- a/src/applications/auth/controller/login/PhabricatorLoginController.php +++ b/src/applications/auth/controller/login/PhabricatorLoginController.php @@ -1,7 +1,7 @@ getRequest()->getPath(); - $request->setCookie('next_uri', $next_uri); - if ($next_uri == '/login/' && !$request->isFormPost()) { - // The user went straight to /login/, so presumably they want to go - // to the dashboard upon logging in. Because, you know, that's logical. - // And people are logical. Sometimes... Fine, no they're not. - // We check for POST here because getPath() would get reset to /login/. - $request->setCookie('next_uri', '/'); + if ($next_uri == '/login/') { + $next_uri = '/'; } - // Always use $request->getCookie('next_uri', '/') after the above. + if (!$request->isFormPost()) { + $request->setCookie('next_uri', $next_uri); + } $password_auth = PhabricatorEnv::getEnvConfig('auth.password-auth-enabled'); @@ -72,8 +69,14 @@ class PhabricatorLoginController extends PhabricatorAuthController { $request->setCookie('phusr', $user->getUsername()); $request->setCookie('phsid', $session_key); + $uri = new PhutilURI('/login/validate/'); + $uri->setQueryParams( + array( + 'phusr' => $user->getUsername(), + )); + return id(new AphrontRedirectResponse()) - ->setURI($request->getCookie('next_uri', '/')); + ->setURI((string)$uri); } else { $log = PhabricatorUserLog::newLog( null, diff --git a/src/applications/auth/controller/login/__init__.php b/src/applications/auth/controller/login/__init__.php index c0d42fc85c..3d05f76225 100644 --- a/src/applications/auth/controller/login/__init__.php +++ b/src/applications/auth/controller/login/__init__.php @@ -19,6 +19,7 @@ phutil_require_module('phabricator', 'view/form/control/text'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php index c21ec54696..85989967ef 100644 --- a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php @@ -1,7 +1,7 @@ setURI('/settings/page/'.$provider_key.'/'); } - $next_uri = $request->getCookie('next_uri', '/'); - // Login with known auth. if ($oauth_info->getID()) { @@ -154,9 +152,14 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $request->setCookie('phusr', $known_user->getUsername()); $request->setCookie('phsid', $session_key); - $request->clearCookie('next_uri'); - return id(new AphrontRedirectResponse()) - ->setURI($next_uri); + + $uri = new PhutilURI('/login/validate/'); + $uri->setQueryParams( + array( + 'phusr' => $known_user->getUsername(), + )); + + return id(new AphrontRedirectResponse())->setURI((string)$uri); } $oauth_email = $provider->retrieveUserEmail(); diff --git a/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php b/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php new file mode 100644 index 0000000000..a15adae465 --- /dev/null +++ b/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php @@ -0,0 +1,108 @@ +getRequest(); + $user = $request->getUser(); + + if (!PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) { + return new Aphront400Response(); + } + + $errors = array(); + + $e_pass = true; + $e_confirm = true; + + if ($request->isFormPost()) { + $e_pass = 'Error'; + $e_confirm = 'Error'; + + $pass = $request->getStr('password'); + $confirm = $request->getStr('confirm'); + + if (strlen($pass) < 3) { + $errors[] = 'That password is ridiculously short.'; + } + + if ($pass !== $confirm) { + $errors[] = "Passwords do not match."; + } + + if (!$errors) { + + // The CSRF token depends on the user's password hash. When we change + // it, we cause the CSRF check to fail. We don't need to do a CSRF + // check here because we've already performed one in the isFormPost() + // call earlier. + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $user->setPassword($pass); + $user->save(); + unset($unguarded); + + return id(new AphrontRedirectResponse()) + ->setURI('/'); + } + } + + if ($errors) { + $error_view = new AphrontErrorView(); + $error_view->setTitle('Password Reset Failed'); + $error_view->setErrors($errors); + } else { + $error_view = null; + } + + $form = new AphrontFormView(); + $form + ->setUser($user) + ->setAction('/login/reset/') + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('New Password') + ->setName('password') + ->setError($e_pass)) + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('Confirm Password') + ->setName('confirm') + ->setError($e_confirm)) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue('Reset Password') + ->addCancelButton('/', 'Skip')); + + $panel = new AphrontPanelView(); + $panel->setWidth(AphrontPanelView::WIDTH_FORM); + $panel->setHeader('Reset Password'); + $panel->appendChild($form); + + return $this->buildStandardPageResponse( + array( + $error_view, + $panel, + ), + array( + 'title' => 'Reset Password', + )); + } + +} diff --git a/src/applications/auth/controller/resetpassword/__init__.php b/src/applications/auth/controller/resetpassword/__init__.php new file mode 100644 index 0000000000..54d5e45e42 --- /dev/null +++ b/src/applications/auth/controller/resetpassword/__init__.php @@ -0,0 +1,23 @@ +getRequest(); + + $failures = array(); + + if (!$request->getStr('phusr')) { + throw new Exception( + "Login validation is missing expected parameters!"); + } + + $expect_phusr = $request->getStr('phusr'); + $actual_phusr = $request->getCookie('phusr'); + if ($actual_phusr != $expect_phusr) { + + if ($actual_phusr) { + $cookie_info = "sent back a cookie with the value '{$actual_phusr}'."; + } else { + $cookie_info = "did not accept the cookie."; + } + + $failures[] = + "Attempted to set 'phusr' cookie to '{$expect_phusr}', but your ". + "browser {$cookie_info}"; + } + + if (!$failures) { + if (!$request->getUser()->getPHID()) { + $failures[] = "Cookies were set correctly, but your session ". + "isn't valid."; + } + } + + if ($failures) { + + $list = array(); + foreach ($failures as $failure) { + $list[] = '
  • '.phutil_escape_html($failure).'
    • '; + } + $list = ''; + + $view = new AphrontRequestFailureView(); + $view->setHeader('Login Failed'); + $view->appendChild( + '

    Login failed:

    '. + $list. + '

    Clear your cookies and try again.

    '); + $view->appendChild( + '
    '. + 'Try Again'. + '
    '); + return $this->buildStandardPageResponse( + $view, + array( + 'title' => 'Login Failed', + )); + } + + $next = nonempty( + $request->getStr('next'), + $request->getCookie('next_uri'), + '/'); + $request->clearCookie('next_uri'); + + if (strpos($next, '/') !== 0) { + $next = '/'; + } + + return id(new AphrontRedirectResponse())->setURI($next); + } + +} diff --git a/src/applications/auth/controller/validate/__init__.php b/src/applications/auth/controller/validate/__init__.php new file mode 100644 index 0000000000..e925f7989b --- /dev/null +++ b/src/applications/auth/controller/validate/__init__.php @@ -0,0 +1,17 @@ +getPHID()) { + return true; + } + // When the user posts a form, we check that it contains a valid CSRF token. // Tokens cycle each hour (every CSRF_CYLCE_FREQUENCY seconds) and we accept // either the current token, the next token (users can submit a "future" @@ -185,6 +189,7 @@ class PhabricatorUser extends PhabricatorUserDAO { return true; } } + return false; }