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 @@ -517,6 +517,11 @@ ->withUserPHIDs(array($viewer->getPHID())) ->withChallengeTTLBetween($now, null) ->execute(); + + PhabricatorAuthChallenge::newChallengeResponsesFromRequest( + $challenges, + $request); + $challenge_map = mgroup($challenges, 'getFactorPHID'); $validation_results = array(); @@ -710,6 +715,7 @@ ->setUser($viewer) ->appendRemarkupInstructions(''); + $answered = array(); foreach ($factors as $factor) { $result = $validation_results[$factor->getPHID()]; @@ -718,10 +724,23 @@ $form, $viewer, $result); + + $answered_challenge = $result->getAnsweredChallenge(); + if ($answered_challenge) { + $answered[] = $answered_challenge; + } } $form->appendRemarkupInstructions(''); + if ($answered) { + $http_params = PhabricatorAuthChallenge::newHTTPParametersFromChallenges( + $answered); + foreach ($http_params as $key => $value) { + $form->addHiddenInput($key, $value); + } + } + return $form; } 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 @@ -165,4 +165,38 @@ AphrontRequest $request, array $challenges); + final protected function newAutomaticControl( + PhabricatorAuthFactorResult $result) { + + $is_answered = (bool)$result->getAnsweredChallenge(); + if ($is_answered) { + return $this->newAnsweredControl($result); + } + + $is_wait = $result->getIsWait(); + if ($is_wait) { + return $this->newWaitControl($result); + } + + return null; + } + + private function newWaitControl( + PhabricatorAuthFactorResult $result) { + + $error = $result->getErrorMessage(); + + return id(new AphrontFormMarkupControl()) + ->setValue($error) + ->setError(pht('Wait')); + } + + private function newAnsweredControl( + PhabricatorAuthFactorResult $result) { + + return id(new AphrontFormMarkupControl()) + ->setValue(pht('Answered!')); + } + + } 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 @@ -202,15 +202,11 @@ PhabricatorUser $viewer, PhabricatorAuthFactorResult $result) { - $value = $result->getValue(); - $error = $result->getErrorMessage(); - $is_wait = $result->getIsWait(); - - if ($is_wait) { - $control = id(new AphrontFormMarkupControl()) - ->setValue($error) - ->setError(pht('Wait')); - } else { + $control = $this->newAutomaticControl($result); + if (!$control) { + $value = $result->getValue(); + $error = $result->getErrorMessage(); + $control = id(new PHUIFormNumberControl()) ->setName($this->getParameterName($config, 'totpcode')) ->setDisableAutocomplete(true) @@ -321,6 +317,12 @@ $challenge = head($challenges); + // If the client has already provided a valid answer to this challenge and + // submitted a token proving they answered it, we're all set. + if ($challenge->getIsAnsweredChallenge()) { + return $result->setAnsweredChallenge($challenge); + } + $challenge_timestep = (int)$challenge->getChallengeKey(); $current_timestep = $this->getCurrentTimestep(); diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php --- a/src/applications/auth/storage/PhabricatorAuthChallenge.php +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -17,6 +17,7 @@ private $responseToken; + const HTTPKEY = '__hisec.challenges__'; const TOKEN_DIGEST_KEY = 'auth.challenge.token'; public static function initializeNewChallenge() { @@ -24,6 +25,89 @@ ->setIsCompleted(0); } + public static function newHTTPParametersFromChallenges(array $challenges) { + assert_instances_of($challenges, __CLASS__); + + $token_list = array(); + foreach ($challenges as $challenge) { + $token = $challenge->getResponseToken(); + if ($token) { + $token_list[] = sprintf( + '%s:%s', + $challenge->getPHID(), + $token->openEnvelope()); + } + } + + if (!$token_list) { + return array(); + } + + $token_list = implode(' ', $token_list); + + return array( + self::HTTPKEY => $token_list, + ); + } + + public static function newChallengeResponsesFromRequest( + array $challenges, + AphrontRequest $request) { + assert_instances_of($challenges, __CLASS__); + + $token_list = $request->getStr(self::HTTPKEY); + $token_list = explode(' ', $token_list); + + $token_map = array(); + foreach ($token_list as $token_element) { + $token_element = trim($token_element, ' '); + + if (!strlen($token_element)) { + continue; + } + + // NOTE: This error message is intentionally not printing the token to + // avoid disclosing it. As a result, it isn't terribly useful, but no + // normal user should ever end up here. + if (!preg_match('/^[^:]+:/', $token_element)) { + throw new Exception( + pht( + 'This request included an improperly formatted MFA challenge '. + 'token and can not be processed.')); + } + + list($phid, $token) = explode(':', $token_element, 2); + + if (isset($token_map[$phid])) { + throw new Exception( + pht( + 'This request improperly specifies an MFA challenge token ("%s") '. + 'multiple times and can not be processed.', + $phid)); + } + + $token_map[$phid] = new PhutilOpaqueEnvelope($token); + } + + $challenges = mpull($challenges, null, 'getPHID'); + + $now = PhabricatorTime::getNow(); + foreach ($challenges as $challenge_phid => $challenge) { + // If the response window has expired, don't attach the token. + if ($challenge->getResponseTTL() < $now) { + continue; + } + + $token = idx($token_map, $challenge_phid); + if (!$token) { + continue; + } + + $challenge->setResponseToken($token); + } + } + + protected function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( @@ -58,12 +142,17 @@ return true; } - // TODO: A challenge is "reused" if it has been answered previously and - // the request doesn't include proof that the client provided the answer. - // Since we aren't tracking client responses yet, any answered challenge - // is always a reused challenge for now. + if (!$this->getIsAnsweredChallenge()) { + return false; + } + + // If the challenge has been answered but the client has provided a token + // proving that they answered it, this is still a valid response. + if ($this->getResponseToken()) { + return false; + } - return $this->getIsAnsweredChallenge(); + return true; } public function getIsAnsweredChallenge() { @@ -75,7 +164,8 @@ $token = new PhutilOpaqueEnvelope($token); return $this - ->setResponseToken($token, $ttl) + ->setResponseToken($token) + ->setResponseTTL($ttl) ->save(); } @@ -85,7 +175,7 @@ ->save(); } - public function setResponseToken(PhutilOpaqueEnvelope $token, $ttl) { + public function setResponseToken(PhutilOpaqueEnvelope $token) { if (!$this->getUserPHID()) { throw new PhutilInvalidStateException('setUserPHID'); } @@ -97,15 +187,6 @@ 'set a new response token.')); } - $now = PhabricatorTime::getNow(); - if ($ttl < $now) { - throw new Exception( - pht( - 'Response TTL is invalid: TTLs must be an epoch timestamp '. - 'coresponding to a future time (did you use a relative TTL by '. - 'mistake?).')); - } - if (preg_match('/ /', $token->openEnvelope())) { throw new Exception( pht( @@ -129,11 +210,14 @@ } $this->responseToken = $token; - $this->responseTTL = $ttl; return $this; } + public function getResponseToken() { + return $this->responseToken; + } + public function setResponseDigest($value) { throw new Exception( pht(