Require MFA implementations to return a formal result object when validating factors

Summary:
Ref T13222. See PHI873. Currently, MFA implementations return this weird sort of ad-hoc dictionary from validation, which is later used to render form/control stuff.

I want to make this more formal to handle token reuse / session binding cases, and let MFA factors share more code around challenges. Formalize this into a proper object instead of an ad-hoc bundle of properties.

Test Plan:
  - Answered a TOTP MFA prompt wrong (nothing, bad value).
  - Answered a TOTP MFA prompt properly.
  - Added new TOTP MFA, survived enrollment.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

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

src/__phutil_library_map__.php

@@ -2198,6 +2198,7 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     'PhabricatorAuthEditController' => 'PhabricatorAuthProviderConfigController',
     'PhabricatorAuthFactor' => 'Phobject',
     'PhabricatorAuthFactorConfig' => 'PhabricatorAuthDAO',
+    'PhabricatorAuthFactorResult' => 'Phobject',
     'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase',
     'PhabricatorAuthFinishController' => 'PhabricatorAuthController',
     'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO',

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -496,14 +496,25 @@ final class PhabricatorAuthSessionEngine extends Phobject {
           $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 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     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('');

src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php

@@ -7,6 +7,7 @@ final class PhabricatorAuthHighSecurityRequiredException extends Exception {
   private $factorValidationResults;
 
   public function setFactorValidationResults(array $results) {
+    assert_instances_of($results, 'PhabricatorAuthFactorResult');
     $this->factorValidationResults = $results;
     return $this;
   }

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -14,19 +14,13 @@ abstract class PhabricatorAuthFactor extends Phobject {
     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) {

src/applications/auth/factor/PhabricatorAuthFactorResult.php

@@ -0,0 +1,37 @@
+<?php
+
+final class PhabricatorAuthFactorResult
+  extends Phobject {
+
+  private $isValid = false;
+  private $hint;
+  private $value;
+
+  public function setIsValid($is_valid) {
+    $this->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;
+  }
+
+}

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -154,10 +154,14 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     PhabricatorAuthFactorConfig $config,
     AphrontFormView $form,
     PhabricatorUser $viewer,
-    $validation_result) {
+    PhabricatorAuthFactorResult $validation_result = null) {
 
-    if (!$validation_result) {
+    if ($validation_result) {
-      $validation_result = array();
+      $value = $validation_result->getValue();
+      $hint = $validation_result->getHint();
+    } else {
+      $value = null;
+      $hint = true;
     }
 
     $form->appendChild(
@@ -166,8 +170,8 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
         ->setLabel(pht('App Code'))
         ->setDisableAutocomplete(true)
         ->setCaption(pht('Factor Name: %s', $config->getFactorName()))
-        ->setValue(idx($validation_result, 'value'))
+        ->setValue($value)
-        ->setError(idx($validation_result, 'error', true)));
+        ->setError($hint));
   }
 
   public function processValidateFactorForm(
@@ -178,21 +182,22 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     $code = $request->getStr($this->getParameterName($config, 'totpcode'));
     $key = new PhutilOpaqueEnvelope($config->getFactorSecret());
 
-    if (self::verifyTOTPCode($viewer, $key, $code)) {
+    $result = id(new PhabricatorAuthFactorResult())
-      return array(
+      ->setValue($code);
-        'error' => null,
-        'value' => $code,
-        'valid' => true,
-      );
-    } else {
-      return array(
-        'error' => strlen($code) ? pht('Invalid') : pht('Required'),
-        'value' => $code,
-        'valid' => false,
-      );
-    }
-  }
 
+    if (self::verifyTOTPCode($viewer, $key, $code)) {
+      $result->setIsValid(true);
+    } else {
+      if (strlen($code)) {
+        $hint = pht('Invalid');
+      } else {
+        $hint = pht('Required');
+      }
+      $result->setHint($hint);
+    }
+
+    return $result;
+  }
 
   public static function generateNewTOTPKey() {
     return strtoupper(Filesystem::readRandomCharacters(32));