From ce6fc5be9092fd5989653ecaba53014869849b7a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 23 May 2019 11:41:14 -0700
Subject: [PATCH] Fix a looping workflow when trying to submit a
 partially-effectless transaction group

Summary:
Ref T13289. If you do this:

  - Subscribe to a task (so we don't generate a subscribe side-effect later).
  - Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
  - Submit the transaction group.

...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".

If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.

Instead, retain the "continue" bit through the MFA.

Also, don't show "You can't sign an empty transaction group" if there's a comment.

See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.

Test Plan:
  - Went through the workflow above.
  - Before: looping workflow.
  - After: "Continue" carries through the MFA gate.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20552
---
 src/aphront/AphrontRequest.php                             | 2 +-
 .../PhabricatorHighSecurityRequestExceptionHandler.php     | 7 +++++++
 .../editor/PhabricatorApplicationTransactionEditor.php     | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 95534c048d..469e6ba766 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -663,7 +663,7 @@ final class AphrontRequest extends Phobject {
   }
 
   public function isContinueRequest() {
-    return $this->isFormPost() && $this->getStr('__continue__');
+    return $this->isFormOrHisecPost() && $this->getStr('__continue__');
   }
 
   public function isPreviewRequest() {
diff --git a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php
index 7f4eddad45..5d1d923d0c 100644
--- a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php
+++ b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php
@@ -120,6 +120,13 @@ final class PhabricatorHighSecurityRequestExceptionHandler
       $dialog->addHiddenInput($key, $value);
     }
 
+    // See T13289. If the user hit a "some transactions have no effect" dialog
+    // and elected to continue, we want to pass that flag through the MFA
+    // dialog even though it is not normally a passthrough request parameter.
+    if ($request->isContinueRequest()) {
+      $dialog->addHiddenInput(AphrontRequest::TYPE_CONTINUE, 1);
+    }
+
     return $dialog;
   }
 
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 12fbc2a07e..f2ca9883ca 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -2535,7 +2535,7 @@ abstract class PhabricatorApplicationTransactionEditor
     // If none of the transactions have an effect, the meta-transactions also
     // have no effect. Add them to the "no effect" list so we get a full set
     // of errors for everything.
-    if (!$any_effect) {
+    if (!$any_effect && !$has_comment) {
       $no_effect += $meta_xactions;
     }
 
-- 
2.51.2