From 12c337098872b6af75ed7591d5a2611f860331d0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 30 Jan 2020 07:26:20 -0800
Subject: [PATCH] When issuing a "no-op" MFA token because no MFA is
 configured, don't give the timeline story a badge

Summary:
Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.

An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.

(Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)

When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.

This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.

Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.

Test Plan:
  - As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
  - As a user with MFA, renamed a user. Got badged stories in both cases.

Maniphest Tasks: T13475

Differential Revision: https://secure.phabricator.com/D20958
---
 .../data/PhabricatorAuthHighSecurityToken.php    | 16 +++++++++++++++-
 .../auth/engine/PhabricatorAuthSessionEngine.php |  3 ++-
 .../PhabricatorApplicationTransactionEditor.php  |  8 +++++---
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/applications/auth/data/PhabricatorAuthHighSecurityToken.php b/src/applications/auth/data/PhabricatorAuthHighSecurityToken.php
index 8ea1ed97f8..9d44411795 100644
--- a/src/applications/auth/data/PhabricatorAuthHighSecurityToken.php
+++ b/src/applications/auth/data/PhabricatorAuthHighSecurityToken.php
@@ -1,3 +1,17 @@
 <?php
 
-final class PhabricatorAuthHighSecurityToken extends Phobject {}
+final class PhabricatorAuthHighSecurityToken
+  extends Phobject {
+
+  private $isUnchallengedToken = false;
+
+  public function setIsUnchallengedToken($is_unchallenged_token) {
+    $this->isUnchallengedToken = $is_unchallenged_token;
+    return $this;
+  }
+
+  public function getIsUnchallengedToken() {
+    return $this->isUnchallengedToken;
+  }
+
+}
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index 7358a61a40..251c8284ef 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -493,7 +493,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     // adds an auth factor, existing sessions won't get a free pass into hisec,
     // since they never actually got marked as hisec.
     if (!$factors) {
-      return $this->issueHighSecurityToken($session, true);
+      return $this->issueHighSecurityToken($session, true)
+        ->setIsUnchallengedToken(true);
     }
 
     $this->request = $request;
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index c17ac1ec72..422605f761 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -5152,12 +5152,14 @@ abstract class PhabricatorApplicationTransactionEditor
           'an MFA check.'));
     }
 
-    id(new PhabricatorAuthSessionEngine())
+    $token = id(new PhabricatorAuthSessionEngine())
       ->setWorkflowKey($workflow_key)
       ->requireHighSecurityToken($actor, $request, $cancel_uri);
 
-    foreach ($xactions as $xaction) {
-      $xaction->setIsMFATransaction(true);
+    if (!$token->getIsUnchallengedToken()) {
+      foreach ($xactions as $xaction) {
+        $xaction->setIsMFATransaction(true);
+      }
     }
   }
 
-- 
2.51.2