Carry MFA responses which have been "answered" but not "completed" through the MFA workflow

Summary:
Depends on D19894. Ref T13222. See PHI873. When you provide a correct response to an MFA challenge, we mark it as "answered".

Currently, we never let you reuse an "answered" token. That's usually fine, but if you have 2+ factors on your account and get one or more (but fewer than all of them) right when you submit the form, you need to answer them all again, possibly after waiting for a lockout period. This is needless.

When you answer a challenge correctly, add a hidden input with a code proving you got it right so you don't need to provide another answer for a little while.

Why not just put your response in a form input, e.g. `<input type="hidden" name="totp-response" value="123456" />`?

  - We may allow the "answered" response to be valid for a different amount of time than the actual answer. For TOTP, we currently allow a response to remain valid for 60 seconds, but the actual code you entered might expire sooner.
  - In some cases, there's no response we can provide (with push + approve MFA, you don't enter a code, you just tap "yes, allow this" on your phone). Conceivably, we may not be able to re-verify a push+approve code if the remote implements one-shot answers.
  - The "responseToken" stuff may end up embedded in normal forms in some cases in the future, and this approach just generally reduces the amount of plaintext MFA we have floating around.

Test Plan:
  - Added 2 MFA tokens to my account.
  - Hit the MFA prompt.
  - Provided one good response and one bad response.
  - Submitted the form.
  - Old behavior: good response gets locked out for ~120 seconds.
  - New behavior: good response is marked "answered", fixing the other response lets me submit the form.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19895

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -517,6 +517,11 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       ->withUserPHIDs(array($viewer->getPHID()))
       ->withChallengeTTLBetween($now, null)
       ->execute();
+
+    PhabricatorAuthChallenge::newChallengeResponsesFromRequest(
+      $challenges,
+      $request);
+
     $challenge_map = mgroup($challenges, 'getFactorPHID');
 
     $validation_results = array();
@@ -710,6 +715,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       ->setUser($viewer)
       ->appendRemarkupInstructions('');
 
+    $answered = array();
     foreach ($factors as $factor) {
       $result = $validation_results[$factor->getPHID()];
 
@@ -718,10 +724,23 @@ final class PhabricatorAuthSessionEngine extends Phobject {
         $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;
   }
 

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -165,4 +165,38 @@ abstract class PhabricatorAuthFactor extends Phobject {
     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!'));
+  }
+
+
 }

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -202,15 +202,11 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     PhabricatorUser $viewer,
     PhabricatorAuthFactorResult $result) {
 
-    $value = $result->getValue();
+    $control = $this->newAutomaticControl($result);
-    $error = $result->getErrorMessage();
+    if (!$control) {
-    $is_wait = $result->getIsWait();
+      $value = $result->getValue();
+      $error = $result->getErrorMessage();
 
-    if ($is_wait) {
-      $control = id(new AphrontFormMarkupControl())
-        ->setValue($error)
-        ->setError(pht('Wait'));
-    } else {
       $control = id(new PHUIFormNumberControl())
         ->setName($this->getParameterName($config, 'totpcode'))
         ->setDisableAutocomplete(true)
@@ -321,6 +317,12 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
     $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();
 

src/applications/auth/storage/PhabricatorAuthChallenge.php

@@ -17,6 +17,7 @@ final class PhabricatorAuthChallenge
 
   private $responseToken;
 
+  const HTTPKEY = '__hisec.challenges__';
   const TOKEN_DIGEST_KEY = 'auth.challenge.token';
 
   public static function initializeNewChallenge() {
@@ -24,6 +25,89 @@ final class PhabricatorAuthChallenge
       ->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 @@ final class PhabricatorAuthChallenge
       return true;
     }
 
-    // TODO: A challenge is "reused" if it has been answered previously and
+    if (!$this->getIsAnsweredChallenge()) {
-    // the request doesn't include proof that the client provided the answer.
+      return false;
-    // Since we aren't tracking client responses yet, any answered challenge
+    }
-    // is always a reused challenge for now.
 
-    return $this->getIsAnsweredChallenge();
+    // 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 true;
   }
 
   public function getIsAnsweredChallenge() {
@@ -75,7 +164,8 @@ final class PhabricatorAuthChallenge
     $token = new PhutilOpaqueEnvelope($token);
 
     return $this
-      ->setResponseToken($token, $ttl)
+      ->setResponseToken($token)
+      ->setResponseTTL($ttl)
       ->save();
   }
 
@@ -85,7 +175,7 @@ final class PhabricatorAuthChallenge
       ->save();
   }
 
-  public function setResponseToken(PhutilOpaqueEnvelope $token, $ttl) {
+  public function setResponseToken(PhutilOpaqueEnvelope $token) {
     if (!$this->getUserPHID()) {
       throw new PhutilInvalidStateException('setUserPHID');
     }
@@ -97,15 +187,6 @@ final class PhabricatorAuthChallenge
           '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 @@ final class PhabricatorAuthChallenge
     }
 
     $this->responseToken = $token;
-    $this->responseTTL = $ttl;
 
     return $this;
   }
 
+  public function getResponseToken() {
+    return $this->responseToken;
+  }
+
   public function setResponseDigest($value) {
     throw new Exception(
       pht(