Page MenuHomePhabricator
Files F14035037

D19895.diff
No OneTemporary
Actions

D19895.diff
View Options

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(

File Metadata

Mime Type
text/plain
Expires
Mon, Nov 11, 3:39 AM (1 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6717208
Default Alt Text
D19895.diff (8 KB)

Event Timeline

Phabricator · Built by Phacility