Give Duo MFA a stronger hint if users continue without answering the challenge

Summary: See PHI912. Also, clean up some leftover copy/pastey code here.

Test Plan: {F6182333}

Reviewers: amckinley

Reviewed By: amckinley

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

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -123,6 +123,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
       ->setUserPHID($viewer->getPHID())
       ->setSessionPHID($viewer->getSession()->getPHID())
       ->setFactorPHID($config->getPHID())
+      ->setIsNewChallenge(true)
       ->setWorkflowKey($engine->getWorkflowKey());
   }
 
@@ -283,8 +284,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-clock-o', 'red');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-clock-o', 'red');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -295,8 +299,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
   private function newAnsweredControl(
     PhabricatorAuthFactorResult $result) {
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-check-circle-o', 'green');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-check-circle-o', 'green');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -309,8 +316,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-times', 'red');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-times', 'red');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -323,8 +333,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-commenting', 'green');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-commenting', 'green');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)

src/applications/auth/factor/PhabricatorAuthFactorResult.php

@@ -10,6 +10,7 @@ final class PhabricatorAuthFactorResult
   private $errorMessage;
   private $value;
   private $issuedChallenges = array();
+  private $icon;
 
   public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) {
     if (!$challenge->getIsAnsweredChallenge()) {
@@ -92,4 +93,13 @@ final class PhabricatorAuthFactorResult
     return $this->issuedChallenges;
   }
 
+  public function setIcon(PHUIIconView $icon) {
+    $this->icon = $icon;
+    return $this;
+  }
+
+  public function getIcon() {
+    return $this->icon;
+  }
+
 }

src/applications/auth/factor/PhabricatorDuoAuthFactor.php

@@ -612,7 +612,22 @@ final class PhabricatorDuoAuthFactor
         return $this->newResult()
           ->setAnsweredChallenge($challenge);
       case 'waiting':
-        // No result yet, we'll render a default state later on.
+        // If we didn't just issue this challenge, give the user a stronger
+        // hint that they need to follow the instructions.
+        if (!$challenge->getIsNewChallenge()) {
+          return $this->newResult()
+            ->setIsContinue(true)
+            ->setIcon(
+              id(new PHUIIconView())
+              ->setIcon('fa-exclamation-triangle', 'yellow'))
+            ->setErrorMessage(
+              pht(
+                'You must approve the challenge which was sent to your '.
+                'phone. Open the Duo application and confirm the challenge, '.
+                'then continue.'));
+        }
+
+        // Otherwise, we'll construct a default message later on.
         break;
       default:
       case 'deny':
@@ -666,8 +681,7 @@ final class PhabricatorDuoAuthFactor
   public function getRequestHasChallengeResponse(
     PhabricatorAuthFactorConfig $config,
     AphrontRequest $request) {
-    $value = $this->getChallengeResponseFromRequest($config, $request);
-    return (bool)strlen($value);
+    return false;
   }
 
   protected function newResultFromChallengeResponse(
@@ -675,41 +689,7 @@ final class PhabricatorDuoAuthFactor
     PhabricatorUser $viewer,
     AphrontRequest $request,
     array $challenges) {
-
-    $challenge = $this->getChallengeForCurrentContext(
-      $config,
-      $viewer,
-      $challenges);
-
-    $code = $this->getChallengeResponseFromRequest(
-      $config,
-      $request);
-
-    $result = $this->newResult()
-      ->setValue($code);
-
-    if ($challenge->getIsAnsweredChallenge()) {
-      return $result->setAnsweredChallenge($challenge);
-    }
-
-    if (phutil_hashes_are_identical($code, $challenge->getChallengeKey())) {
-      $ttl = PhabricatorTime::getNow() + phutil_units('15 minutes in seconds');
-
-      $challenge
-        ->markChallengeAsAnswered($ttl);
-
-      return $result->setAnsweredChallenge($challenge);
-    }
-
-    if (strlen($code)) {
-      $error_message = pht('Invalid');
-    } else {
-      $error_message = pht('Required');
-    }
-
-    $result->setErrorMessage($error_message);
-
-    return $result;
+    return $this->newResult();
   }
 
   private function newDuoFuture(PhabricatorAuthFactorProvider $provider) {

src/applications/auth/storage/PhabricatorAuthChallenge.php

@@ -16,6 +16,7 @@ final class PhabricatorAuthChallenge
   protected $properties = array();
 
   private $responseToken;
+  private $isNewChallenge;
 
   const HTTPKEY = '__hisec.challenges__';
   const TOKEN_DIGEST_KEY = 'auth.challenge.token';
@@ -241,6 +242,15 @@ final class PhabricatorAuthChallenge
     return $this->properties[$key];
   }
 
+  public function setIsNewChallenge($is_new_challenge) {
+    $this->isNewChallenge = $is_new_challenge;
+    return $this;
+  }
+
+  public function getIsNewChallenge() {
+    return $this->isNewChallenge;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */