diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2198,6 +2198,7 @@ 'PhabricatorAuthEditController' => 'applications/auth/controller/config/PhabricatorAuthEditController.php', 'PhabricatorAuthFactor' => 'applications/auth/factor/PhabricatorAuthFactor.php', 'PhabricatorAuthFactorConfig' => 'applications/auth/storage/PhabricatorAuthFactorConfig.php', + 'PhabricatorAuthFactorResult' => 'applications/auth/factor/PhabricatorAuthFactorResult.php', 'PhabricatorAuthFactorTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTestCase.php', 'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php', 'PhabricatorAuthHMACKey' => 'applications/auth/storage/PhabricatorAuthHMACKey.php', @@ -7833,6 +7834,7 @@ 'PhabricatorAuthEditController' => 'PhabricatorAuthProviderConfigController', 'PhabricatorAuthFactor' => 'Phobject', 'PhabricatorAuthFactorConfig' => 'PhabricatorAuthDAO', + 'PhabricatorAuthFactorResult' => 'Phobject', 'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase', 'PhabricatorAuthFinishController' => 'PhabricatorAuthController', 'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO', diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -496,14 +496,25 @@ $id = $factor->getID(); $impl = $factor->requireImplementation(); - $validation_results[$id] = $impl->processValidateFactorForm( + $validation_result = $impl->processValidateFactorForm( $factor, $viewer, $request); - if (!$impl->isFactorValid($factor, $validation_results[$id])) { + if (!($validation_result instanceof PhabricatorAuthFactorResult)) { + throw new Exception( + pht( + 'Expected "processValidateFactorForm()" to return an object '. + 'of class "%s"; got something else (from "%s").', + 'PhabricatorAuthFactorResult', + get_class($impl))); + } + + if (!$validation_result->getIsValid()) { $ok = false; } + + $validation_results[$id] = $validation_result; } if ($ok) { @@ -595,17 +606,20 @@ array $validation_results, PhabricatorUser $viewer, AphrontRequest $request) { + assert_instances_of($validation_results, 'PhabricatorAuthFactorResult'); $form = id(new AphrontFormView()) ->setUser($viewer) ->appendRemarkupInstructions(''); foreach ($factors as $factor) { + $result = idx($validation_results, $factor->getID()); + $factor->requireImplementation()->renderValidateFactorForm( $factor, $form, $viewer, - idx($validation_results, $factor->getID())); + $result); } $form->appendRemarkupInstructions(''); diff --git a/src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php b/src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php --- a/src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php +++ b/src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php @@ -7,6 +7,7 @@ private $factorValidationResults; public function setFactorValidationResults(array $results) { + assert_instances_of($results, 'PhabricatorAuthFactorResult'); $this->factorValidationResults = $results; return $this; } diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -14,19 +14,13 @@ PhabricatorAuthFactorConfig $config, AphrontFormView $form, PhabricatorUser $viewer, - $validation_result); + PhabricatorAuthFactorResult $validation_result = null); abstract public function processValidateFactorForm( PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer, AphrontRequest $request); - public function isFactorValid( - PhabricatorAuthFactorConfig $config, - $validation_result) { - return (idx($validation_result, 'valid') === true); - } - public function getParameterName( PhabricatorAuthFactorConfig $config, $name) { diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php @@ -0,0 +1,37 @@ +isValid = $is_valid; + return $this; + } + + public function getIsValid() { + return $this->isValid; + } + + public function setHint($hint) { + $this->hint = $hint; + return $this; + } + + public function getHint() { + return $this->hint; + } + + public function setValue($value) { + $this->value = $value; + return $this; + } + + public function getValue() { + return $this->value; + } + +} diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -154,10 +154,14 @@ PhabricatorAuthFactorConfig $config, AphrontFormView $form, PhabricatorUser $viewer, - $validation_result) { + PhabricatorAuthFactorResult $validation_result = null) { - if (!$validation_result) { - $validation_result = array(); + if ($validation_result) { + $value = $validation_result->getValue(); + $hint = $validation_result->getHint(); + } else { + $value = null; + $hint = true; } $form->appendChild( @@ -166,8 +170,8 @@ ->setLabel(pht('App Code')) ->setDisableAutocomplete(true) ->setCaption(pht('Factor Name: %s', $config->getFactorName())) - ->setValue(idx($validation_result, 'value')) - ->setError(idx($validation_result, 'error', true))); + ->setValue($value) + ->setError($hint)); } public function processValidateFactorForm( @@ -178,21 +182,22 @@ $code = $request->getStr($this->getParameterName($config, 'totpcode')); $key = new PhutilOpaqueEnvelope($config->getFactorSecret()); + $result = id(new PhabricatorAuthFactorResult()) + ->setValue($code); + if (self::verifyTOTPCode($viewer, $key, $code)) { - return array( - 'error' => null, - 'value' => $code, - 'valid' => true, - ); + $result->setIsValid(true); } else { - return array( - 'error' => strlen($code) ? pht('Invalid') : pht('Required'), - 'value' => $code, - 'valid' => false, - ); + if (strlen($code)) { + $hint = pht('Invalid'); + } else { + $hint = pht('Required'); + } + $result->setHint($hint); } - } + return $result; + } public static function generateNewTOTPKey() { return strtoupper(Filesystem::readRandomCharacters(32));