From 02fb5fea89e675861a318b59b8e888c5a056d45d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jan 2012 20:26:38 -0800 Subject: [PATCH] Allow configuration of a minimum password length, unify password reset interfaces Summary: - We have a hard-coded minimum length of 3 right now (and 1 in the other interface), which is sort of silly. - Provide a more reasonable default, and allow it to be configured. - We have two password reset interfaces, one of which no longer actually requires you to verify you own the account. This is more than a bit derp. - Merge the interfaces into one, using either an email token or the account's current password to let you change the password. Test Plan: - Reset password on an account. - Changed password on an account. - Created a new account, logged in, set the password. - Tried to set a too-short password, got an error. Reviewers: btrahan, jungejason, nh Reviewed By: jungejason CC: aran, jungejason Maniphest Tasks: T766 Differential Revision: https://secure.phabricator.com/D1374 --- conf/default.conf.php | 4 + src/__phutil_library_map__.php | 2 - ...AphrontDefaultApplicationConfiguration.php | 1 - .../PhabricatorEmailTokenController.php | 8 +- .../PhabricatorResetPasswordController.php | 108 ------------------ .../controller/resetpassword/__init__.php | 23 ---- ...torUserPasswordSettingsPanelController.php | 94 +++++++++++---- 7 files changed, 82 insertions(+), 158 deletions(-) delete mode 100644 src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php delete mode 100644 src/applications/auth/controller/resetpassword/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 4dce114e39..3335464354 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -327,6 +327,10 @@ return array( // information remains consistent across both systems. 'account.editable' => true, + // When users set or reset a password, it must have at least this many + // characters. + 'account.minimum-password-length' => 8, + // -- Facebook ------------------------------------------------------------ // diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 82481826a2..7f7fe2bb8f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -642,7 +642,6 @@ 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', @@ -1292,7 +1291,6 @@ 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 ae32fdc08f..1586eebe11 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -134,7 +134,6 @@ class AphrontDefaultApplicationConfiguration 'email/$' => 'PhabricatorEmailLoginController', 'etoken/(?P\w+)/$' => 'PhabricatorEmailTokenController', 'refresh/$' => 'PhabricatorRefreshCSRFController', - 'reset/$' => 'PhabricatorResetPasswordController', 'validate/$' => 'PhabricatorLoginValidateController', ), diff --git a/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php b/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php index d93e0ae97e..2a875164cf 100644 --- a/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php +++ b/src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php @@ -81,11 +81,17 @@ class PhabricatorEmailTokenController extends PhabricatorAuthController { $request->setCookie('phusr', $target_user->getUsername()); $request->setCookie('phsid', $session_key); + if (PhabricatorEnv::getEnvConfig('account.editable')) { + $next = '/settings/page/password/?token='.$token; + } else { + $next = '/'; + } + $uri = new PhutilURI('/login/validate/'); $uri->setQueryParams( array( 'phusr' => $target_user->getUsername(), - 'next' => '/login/reset/', + 'next' => $next, )); return id(new AphrontRedirectResponse()) diff --git a/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php b/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php deleted file mode 100644 index a15adae465..0000000000 --- a/src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php +++ /dev/null @@ -1,108 +0,0 @@ -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 deleted file mode 100644 index 54d5e45e42..0000000000 --- a/src/applications/auth/controller/resetpassword/__init__.php +++ /dev/null @@ -1,23 +0,0 @@ -getStr('token'); + if ($token) { + $valid_token = $user->validateEmailToken($token); + } else { + $valid_token = false; + } + + $e_old = true; + $e_new = true; + $e_conf = true; + $errors = array(); if ($request->isFormPost()) { - if ($user->comparePassword($request->getStr('old_pw'))) { - $pass = $request->getStr('new_pw'); - $conf = $request->getStr('conf_pw'); - if ($pass === $conf) { - if (strlen($pass)) { - $user->setPassword($pass); - // This write is unguarded because the CSRF token has already - // been checked in the call to $request->isFormPost() and - // the CSRF token depends on the password hash, so when it - // is changed here the CSRF token check will fail. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $user->save(); - unset($unguarded); - return id(new AphrontRedirectResponse()) - ->setURI('/settings/page/password/?saved=true'); - } else { - $errors[] = 'Your new password is too short.'; - } - } else { - $errors[] = 'New password and confirmation do not match.'; + if (!$valid_token) { + if (!$user->comparePassword($request->getStr('old_pw'))) { + $errors[] = 'The old password you entered is incorrect.'; + $e_old = 'Invalid'; } - } else { - $errors[] = 'The old password you entered is incorrect.'; + } + + $pass = $request->getStr('new_pw'); + $conf = $request->getStr('conf_pw'); + + if (strlen($pass) < $min_len) { + $errors[] = 'Your new password is too short.'; + $e_new = 'Too Short'; + } + + if ($pass !== $conf) { + $errors[] = 'New password and confirmation do not match.'; + $e_conf = 'Invalid'; + } + + if (!$errors) { + $user->setPassword($pass); + // This write is unguarded because the CSRF token has already + // been checked in the call to $request->isFormPost() and + // the CSRF token depends on the password hash, so when it + // is changed here the CSRF token check will fail. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $user->save(); + unset($unguarded); + + if ($valid_token) { + // If this is a password set/reset, kick the user to the home page + // after we update their account. + $next = '/'; + } else { + $next = '/settings/page/password/?saved=true'; + } + + return id(new AphrontRedirectResponse())->setURI($next); } } @@ -74,22 +108,36 @@ class PhabricatorUserPasswordSettingsPanelController $notice->setErrors($errors); } + $len_caption = null; + if ($min_len) { + $len_caption = 'Minimum password length: '.$min_len.' characters.'; + } + $form = new AphrontFormView(); $form ->setUser($user) - ->appendChild( + ->addHiddenInput('token', $token); + + if (!$valid_token) { + $form->appendChild( id(new AphrontFormPasswordControl()) ->setLabel('Old Password') + ->setError($e_old) ->setName('old_pw')); + } + $form ->appendChild( id(new AphrontFormPasswordControl()) ->setLabel('New Password') + ->setError($e_new) ->setName('new_pw')); $form ->appendChild( id(new AphrontFormPasswordControl()) ->setLabel('Confirm Password') + ->setCaption($len_caption) + ->setError($e_conf) ->setName('conf_pw')); $form ->appendChild(