From b8a515cb29d817287a1e046c589ef5f79d814aa7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jan 2018 16:20:01 -0800 Subject: [PATCH] Bring new password validation into AuthPasswordEngine Summary: Ref T13043. We have ~4 copies of this logic (registration, lost password recovery, set password, set VCS password). Currently it varies a bit from case to case, but since it's all going to be basically identical once account passwords swap to the new infrastructure, bring it into the Engine so it can live in one place. This also fixes VCS passwords not being affected by `account.minimum-password-length`. Test Plan: Hit all errors in "VCS Password" panel. Successfully changed password. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13043 Differential Revision: https://secure.phabricator.com/D18902 --- src/__phutil_library_map__.php | 2 + .../engine/PhabricatorAuthPasswordEngine.php | 73 ++++++++++++++++ .../PhabricatorAuthPasswordException.php | 28 ++++++ .../DiffusionSetPasswordSettingsPanel.php | 86 ++++++------------- 4 files changed, 130 insertions(+), 59 deletions(-) create mode 100644 src/applications/auth/password/PhabricatorAuthPasswordException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index efd8c0ed26..55d7aa3b5f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2092,6 +2092,7 @@ phutil_register_library_map(array( 'PhabricatorAuthPassword' => 'applications/auth/storage/PhabricatorAuthPassword.php', 'PhabricatorAuthPasswordEditor' => 'applications/auth/editor/PhabricatorAuthPasswordEditor.php', 'PhabricatorAuthPasswordEngine' => 'applications/auth/engine/PhabricatorAuthPasswordEngine.php', + 'PhabricatorAuthPasswordException' => 'applications/auth/password/PhabricatorAuthPasswordException.php', 'PhabricatorAuthPasswordPHIDType' => 'applications/auth/phid/PhabricatorAuthPasswordPHIDType.php', 'PhabricatorAuthPasswordQuery' => 'applications/auth/query/PhabricatorAuthPasswordQuery.php', 'PhabricatorAuthPasswordResetTemporaryTokenType' => 'applications/auth/tokentype/PhabricatorAuthPasswordResetTemporaryTokenType.php', @@ -7387,6 +7388,7 @@ phutil_register_library_map(array( ), 'PhabricatorAuthPasswordEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorAuthPasswordEngine' => 'Phobject', + 'PhabricatorAuthPasswordException' => 'Exception', 'PhabricatorAuthPasswordPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthPasswordQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthPasswordResetTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType', diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php index 3ac83c3c59..66d32ad4dc 100644 --- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php @@ -54,6 +54,79 @@ final class PhabricatorAuthPasswordEngine return $this->upgradeHashers; } + public function checkNewPassword( + PhutilOpaqueEnvelope $password, + PhutilOpaqueEnvelope $confirm, + $can_skip = false) { + + $raw_password = $password->openEnvelope(); + + if (!strlen($raw_password)) { + if ($can_skip) { + throw new PhabricatorAuthPasswordException( + pht('You must choose a password or skip this step.'), + pht('Required')); + } else { + throw new PhabricatorAuthPasswordException( + pht('You must choose a password.'), + pht('Required')); + } + } + + $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length'); + $min_len = (int)$min_len; + if ($min_len) { + if (strlen($raw_password) < $min_len) { + throw new PhabricatorAuthPasswordException( + pht( + 'The selected password is too short. Passwords must be a minimum '. + 'of %s characters long.', + new PhutilNumber($min_len)), + pht('Too Short')); + } + } + + $raw_confirm = $confirm->openEnvelope(); + + if (!strlen($raw_confirm)) { + throw new PhabricatorAuthPasswordException( + pht('You must confirm the selected password.'), + null, + pht('Required')); + } + + if ($raw_password !== $raw_confirm) { + throw new PhabricatorAuthPasswordException( + pht('The password and confirmation do not match.'), + pht('Invalid'), + pht('Invalid')); + } + + if (PhabricatorCommonPasswords::isCommonPassword($raw_password)) { + throw new PhabricatorAuthPasswordException( + pht( + 'The selected password is very weak: it is one of the most common '. + 'passwords in use. Choose a stronger password.'), + pht('Very Weak')); + } + + if ($this->isRevokedPassword($password)) { + throw new PhabricatorAuthPasswordException( + pht( + 'The password you entered has been revoked. You can not reuse '. + 'a password which has been revoked. Choose a new password.'), + pht('Revoked')); + } + + if (!$this->isUniquePassword($password)) { + throw new PhabricatorAuthPasswordException( + pht( + 'The password you entered is the same as another password '. + 'associated with your account. Each password must be unique.'), + pht('Not Unique')); + } + } + public function isValidPassword(PhutilOpaqueEnvelope $envelope) { $this->requireSetup(); diff --git a/src/applications/auth/password/PhabricatorAuthPasswordException.php b/src/applications/auth/password/PhabricatorAuthPasswordException.php new file mode 100644 index 0000000000..d4b13bb10c --- /dev/null +++ b/src/applications/auth/password/PhabricatorAuthPasswordException.php @@ -0,0 +1,28 @@ +passwordError = $password_error; + $this->confirmError = $confirm_error; + + parent::__construct($message); + } + + public function getPasswordError() { + return $this->passwordError; + } + + public function getConfirmError() { + return $this->confirmError; + } + +} diff --git a/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php b/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php index 2da51e0624..1899302223 100644 --- a/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php +++ b/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php @@ -58,6 +58,19 @@ final class DiffusionSetPasswordSettingsPanel extends PhabricatorSettingsPanel { $e_password = true; $e_confirm = true; + $content_source = PhabricatorContentSource::newFromRequest($request); + + // NOTE: This test is against $viewer (not $user), so that the error + // message below makes sense in the case that the two are different, + // and because an admin reusing their own password is bad, while + // system agents generally do not have passwords anyway. + + $engine = id(new PhabricatorAuthPasswordEngine()) + ->setViewer($viewer) + ->setContentSource($content_source) + ->setObject($viewer) + ->setPasswordType($vcs_type); + if ($request->isFormPost()) { if ($request->getBool('remove')) { if ($vcspassword->getID()) { @@ -68,71 +81,26 @@ final class DiffusionSetPasswordSettingsPanel extends PhabricatorSettingsPanel { $new_password = $request->getStr('password'); $confirm = $request->getStr('confirm'); - if (!strlen($new_password)) { - $e_password = pht('Required'); - $errors[] = pht('Password is required.'); - } else { - $e_password = null; - } - if (!strlen($confirm)) { - $e_confirm = pht('Required'); - $errors[] = pht('You must confirm the new password.'); - } else { + $envelope = new PhutilOpaqueEnvelope($new_password); + $confirm_envelope = new PhutilOpaqueEnvelope($confirm); + + try { + $engine->checkNewPassword($envelope, $confirm_envelope); + $e_password = null; $e_confirm = null; + } catch (PhabricatorAuthPasswordException $ex) { + $errors[] = $ex->getMessage(); + $e_password = $ex->getPasswordError(); + $e_confirm = $ex->getConfirmError(); } if (!$errors) { - $envelope = new PhutilOpaqueEnvelope($new_password); - $content_source = PhabricatorContentSource::newFromRequest($request); + $vcspassword + ->setPassword($envelope, $user) + ->save(); - // NOTE: This test is against $viewer (not $user), so that the error - // message below makes sense in the case that the two are different, - // and because an admin reusing their own password is bad, while - // system agents generally do not have passwords anyway. - - $engine = id(new PhabricatorAuthPasswordEngine()) - ->setViewer($viewer) - ->setContentSource($content_source) - ->setObject($viewer) - ->setPasswordType($vcs_type); - - $same_password = !$engine->isUniquePassword($envelope); - $revoked_password = $engine->isRevokedPassword($envelope); - - if ($new_password !== $confirm) { - $e_password = pht('Does Not Match'); - $e_confirm = pht('Does Not Match'); - $errors[] = pht('Password and confirmation do not match.'); - } else if ($revoked_password) { - $e_password = pht('Revoked'); - $e_confirm = pht('Revoked'); - $errors[] = pht( - 'This password has been revoked. You must choose a new, unique '. - 'password.'); - } else if ($same_password) { - $e_password = pht('Not Unique'); - $e_confirm = pht('Not Unique'); - $errors[] = pht( - 'This password is the same as another password associated '. - 'with your account. You must use a unique password for '. - 'VCS access.'); - } else if ( - PhabricatorCommonPasswords::isCommonPassword($new_password)) { - $e_password = pht('Very Weak'); - $e_confirm = pht('Very Weak'); - $errors[] = pht( - 'This password is extremely weak: it is one of the most common '. - 'passwords in use. Choose a stronger password.'); - } - - - if (!$errors) { - $vcspassword->setPassword($envelope, $user); - $vcspassword->save(); - - return id(new AphrontRedirectResponse())->setURI($panel_uri); - } + return id(new AphrontRedirectResponse())->setURI($panel_uri); } }