Allow any transaction group to be signed with a one-shot "Sign With MFA" action

Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.

This is a one-shot gate and does not keep you in MFA.

Test Plan:
  - Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
  - Tried to sign alone, got appropriate errors.
  - Tried to sign no-op changes, got appropriate errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

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

src/__phutil_library_map__.php

@@ -2231,6 +2231,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php',
     'PhabricatorAuthLoginHandler' => 'applications/auth/handler/PhabricatorAuthLoginHandler.php',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthLogoutConduitAPIMethod.php',
+    'PhabricatorAuthMFAEditEngineExtension' => 'applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php',
     'PhabricatorAuthMainMenuBarExtension' => 'applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php',
     'PhabricatorAuthManagementCachePKCS8Workflow' => 'applications/auth/management/PhabricatorAuthManagementCachePKCS8Workflow.php',
     'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php',
@@ -7885,6 +7886,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthLoginController' => 'PhabricatorAuthController',
     'PhabricatorAuthLoginHandler' => 'Phobject',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod',
+    'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension',
     'PhabricatorAuthMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension',
     'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow',
     'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow',

src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php

@@ -0,0 +1,52 @@
+<?php
+
+final class PhabricatorAuthMFAEditEngineExtension
+  extends PhabricatorEditEngineExtension {
+
+  const EXTENSIONKEY = 'auth.mfa';
+  const FIELDKEY = 'mfa';
+
+  public function getExtensionPriority() {
+    return 12000;
+  }
+
+  public function isExtensionEnabled() {
+    return true;
+  }
+
+  public function getExtensionName() {
+    return pht('MFA');
+  }
+
+  public function supportsObject(
+    PhabricatorEditEngine $engine,
+    PhabricatorApplicationTransactionInterface $object) {
+    return true;
+  }
+
+  public function buildCustomEditFields(
+    PhabricatorEditEngine $engine,
+    PhabricatorApplicationTransactionInterface $object) {
+
+    $mfa_type = PhabricatorTransactions::TYPE_MFA;
+
+    $viewer = $engine->getViewer();
+
+    $mfa_field = id(new PhabricatorApplyEditField())
+      ->setViewer($viewer)
+      ->setKey(self::FIELDKEY)
+      ->setLabel(pht('MFA'))
+      ->setIsFormField(false)
+      ->setCommentActionLabel(pht('Sign With MFA'))
+      ->setCommentActionOrder(12000)
+      ->setActionDescription(
+        pht('You will be prompted to provide MFA when you submit.'))
+      ->setDescription(pht('Sign this transaction group with MFA.'))
+      ->setTransactionType($mfa_type);
+
+    return array(
+      $mfa_field,
+    );
+  }
+
+}

src/applications/transactions/constants/PhabricatorTransactions.php

@@ -16,6 +16,7 @@ final class PhabricatorTransactions extends Phobject {
   const TYPE_COLUMNS = 'core:columns';
   const TYPE_SUBTYPE = 'core:subtype';
   const TYPE_HISTORY = 'core:history';
+  const TYPE_MFA = 'core:mfa';
 
   const COLOR_RED         = 'red';
   const COLOR_ORANGE      = 'orange';

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1105,6 +1105,7 @@ abstract class PhabricatorEditEngine
       $editor = $object->getApplicationTransactionEditor()
         ->setActor($viewer)
         ->setContentSourceFromRequest($request)
+        ->setCancelURI($cancel_uri)
         ->setContinueOnNoEffect(true);
 
       try {
@@ -1785,7 +1786,9 @@ abstract class PhabricatorEditEngine
     $controller = $this->getController();
     $request = $controller->getRequest();
 
-    if (!$request->isFormPost()) {
+    // NOTE: We handle hisec inside the transaction editor with "Sign With MFA"
+    // comment actions.
+    if (!$request->isFormOrHisecPost()) {
       return new Aphront400Response();
     }
 
@@ -1919,6 +1922,7 @@ abstract class PhabricatorEditEngine
       ->setContinueOnNoEffect($request->isContinueRequest())
       ->setContinueOnMissingFields(true)
       ->setContentSourceFromRequest($request)
+      ->setCancelURI($view_uri)
       ->setRaiseWarnings(!$request->getBool('editEngine.warnings'))
       ->setIsPreview($is_preview);
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -84,6 +84,10 @@ abstract class PhabricatorApplicationTransactionEditor
 
   private $transactionQueue = array();
   private $sendHistory = false;
+  private $shouldRequireMFA = false;
+  private $hasRequiredMFA = false;
+  private $request;
+  private $cancelURI;
 
   const STORAGE_ENCODING_BINARY = 'binary';
 
@@ -284,6 +288,22 @@ abstract class PhabricatorApplicationTransactionEditor
     return $this->raiseWarnings;
   }
 
+  public function setShouldRequireMFA($should_require_mfa) {
+    if ($this->hasRequiredMFA) {
+      throw new Exception(
+        pht(
+          'Call to setShouldRequireMFA() is too late: this Editor has already '.
+          'checked for MFA requirements.'));
+    }
+
+    $this->shouldRequireMFA = $should_require_mfa;
+    return $this;
+  }
+
+  public function getShouldRequireMFA() {
+    return $this->shouldRequireMFA;
+  }
+
   public function getTransactionTypesForObject($object) {
     $old = $this->object;
     try {
@@ -328,6 +348,8 @@ abstract class PhabricatorApplicationTransactionEditor
       $types[] = PhabricatorTransactions::TYPE_SPACE;
     }
 
+    $types[] = PhabricatorTransactions::TYPE_MFA;
+
     $template = $this->object->getApplicationTransactionTemplate();
     if ($template instanceof PhabricatorModularTransaction) {
       $xtypes = $template->newModularTransactionTypes();
@@ -383,6 +405,8 @@ abstract class PhabricatorApplicationTransactionEditor
         return null;
       case PhabricatorTransactions::TYPE_SUBTYPE:
         return $object->getEditEngineSubtype();
+      case PhabricatorTransactions::TYPE_MFA:
+        return null;
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
         return array_values($this->subscribers);
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
@@ -473,6 +497,8 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_SUBTYPE:
       case PhabricatorTransactions::TYPE_HISTORY:
         return $xaction->getNewValue();
+      case PhabricatorTransactions::TYPE_MFA:
+        return true;
       case PhabricatorTransactions::TYPE_SPACE:
         $space_phid = $xaction->getNewValue();
         if (!strlen($space_phid)) {
@@ -611,6 +637,7 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_CREATE:
       case PhabricatorTransactions::TYPE_HISTORY:
       case PhabricatorTransactions::TYPE_SUBTYPE:
+      case PhabricatorTransactions::TYPE_MFA:
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
@@ -673,6 +700,7 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_CREATE:
       case PhabricatorTransactions::TYPE_HISTORY:
       case PhabricatorTransactions::TYPE_SUBTYPE:
+      case PhabricatorTransactions::TYPE_MFA:
       case PhabricatorTransactions::TYPE_EDGE:
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
@@ -850,10 +878,6 @@ abstract class PhabricatorApplicationTransactionEditor
       $xaction->setIsSilentTransaction(true);
     }
 
-    if ($actor->hasHighSecuritySession()) {
-      $xaction->setIsMFATransaction(true);
-    }
-
     return $xaction;
   }
 
@@ -893,6 +917,7 @@ abstract class PhabricatorApplicationTransactionEditor
   }
 
   public function setContentSourceFromRequest(AphrontRequest $request) {
+    $this->setRequest($request);
     return $this->setContentSource(
       PhabricatorContentSource::newFromRequest($request));
   }
@@ -901,6 +926,24 @@ abstract class PhabricatorApplicationTransactionEditor
     return $this->contentSource;
   }
 
+  public function setRequest(AphrontRequest $request) {
+    $this->request = $request;
+    return $this;
+  }
+
+  public function getRequest() {
+    return $this->request;
+  }
+
+  public function setCancelURI($cancel_uri) {
+    $this->cancelURI = $cancel_uri;
+    return $this;
+  }
+
+  public function getCancelURI() {
+    return $this->cancelURI;
+  }
+
   final public function applyTransactions(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -966,9 +1009,29 @@ abstract class PhabricatorApplicationTransactionEditor
             $warnings);
         }
       }
+    }
 
+    foreach ($xactions as $xaction) {
+      $this->adjustTransactionValues($object, $xaction);
+    }
+
+    // Now that we've merged and combined transactions, check for required
+    // capabilities. Note that we're doing this before filtering
+    // transactions: if you try to apply an edit which you do not have
+    // permission to apply, we want to give you a permissions error even
+    // if the edit would have no effect.
+    $this->applyCapabilityChecks($object, $xactions);
+
+    $xactions = $this->filterTransactions($object, $xactions);
+
+    if (!$is_preview) {
       $this->willApplyTransactions($object, $xactions);
 
+      $this->hasRequiredMFA = true;
+      if ($this->getShouldRequireMFA()) {
+        $this->requireMFA($object, $xactions);
+      }
+
       if ($object->getID()) {
         $this->buildOldRecipientLists($object, $xactions);
 
@@ -994,33 +1057,6 @@ abstract class PhabricatorApplicationTransactionEditor
         $this->applyInitialEffects($object, $xactions);
       }
 
-      foreach ($xactions as $xaction) {
-        $this->adjustTransactionValues($object, $xaction);
-      }
-
-      // Now that we've merged and combined transactions, check for required
-      // capabilities. Note that we're doing this before filtering
-      // transactions: if you try to apply an edit which you do not have
-      // permission to apply, we want to give you a permissions error even
-      // if the edit would have no effect.
-      $this->applyCapabilityChecks($object, $xactions);
-
-      // See T13186. Fatal hard if this object has an older
-      // "requireCapabilities()" method. The code may rely on this method being
-      // called to apply policy checks, so err on the side of safety and fatal.
-      // TODO: Remove this check after some time has passed.
-      if (method_exists($this, 'requireCapabilities')) {
-        throw new Exception(
-          pht(
-            'Editor (of class "%s") implements obsolete policy method '.
-            'requireCapabilities(). The implementation for this Editor '.
-            'MUST be updated. See <%s> for discussion.',
-            get_class($this),
-            'https://secure.phabricator.com/T13186'));
-      }
-
-      $xactions = $this->filterTransactions($object, $xactions);
-
       // TODO: Once everything is on EditEngine, just use getIsNewObject() to
       // figure this out instead.
       $mark_as_create = false;
@@ -1580,6 +1616,10 @@ abstract class PhabricatorApplicationTransactionEditor
         // email and is only partially supported in the upstream. You don't
         // need any capabilities to apply it.
         return null;
+      case PhabricatorTransactions::TYPE_MFA:
+        // Signing a transaction group with MFA does not require permissions
+        // on its own.
+        return null;
       case PhabricatorTransactions::TYPE_EDGE:
         return $this->getLegacyRequiredEdgeCapabilities($xaction);
       default:
@@ -2272,11 +2312,19 @@ abstract class PhabricatorApplicationTransactionEditor
     array $xactions) {
 
     $type_comment = PhabricatorTransactions::TYPE_COMMENT;
+    $type_mfa = PhabricatorTransactions::TYPE_MFA;
 
     $no_effect = array();
     $has_comment = false;
     $any_effect = false;
+
+    $meta_xactions = array();
     foreach ($xactions as $key => $xaction) {
+      if ($xaction->getTransactionType() === $type_mfa) {
+        $meta_xactions[$key] = $xaction;
+        continue;
+      }
+
       if ($this->transactionHasEffect($object, $xaction)) {
         if ($xaction->getTransactionType() != $type_comment) {
           $any_effect = true;
@@ -2286,15 +2334,30 @@ abstract class PhabricatorApplicationTransactionEditor
       } else {
         $no_effect[$key] = $xaction;
       }
+
       if ($xaction->hasComment()) {
         $has_comment = true;
       }
     }
 
+    // If every transaction is a meta-transaction applying to the transaction
+    // group, these transactions are junk.
+    if (count($meta_xactions) == count($xactions)) {
+      $no_effect = $xactions;
+      $any_effect = false;
+    }
+
     if (!$no_effect) {
       return $xactions;
     }
 
+    // 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) {
+      $no_effect += $meta_xactions;
+    }
+
     if (!$this->getContinueOnNoEffect() && !$this->getIsPreview()) {
       throw new PhabricatorApplicationTransactionNoEffectException(
         $no_effect,
@@ -2375,6 +2438,12 @@ abstract class PhabricatorApplicationTransactionEditor
           $xactions,
           $type);
         break;
+      case PhabricatorTransactions::TYPE_MFA:
+        $errors[] = $this->validateMFATransactions(
+          $object,
+          $xactions,
+          $type);
+        break;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $groups = array();
         foreach ($xactions as $xaction) {
@@ -2556,6 +2625,36 @@ abstract class PhabricatorApplicationTransactionEditor
     return $errors;
   }
 
+  private function validateMFATransactions(
+    PhabricatorLiskDAO $object,
+    array $xactions,
+    $transaction_type) {
+    $errors = array();
+
+    $factors = id(new PhabricatorAuthFactorConfig())->loadAllWhere(
+      'userPHID = %s',
+      $this->getActingAsPHID());
+
+    foreach ($xactions as $xaction) {
+      if (!$factors) {
+        $errors[] = new PhabricatorApplicationTransactionValidationError(
+          $transaction_type,
+          pht('No MFA'),
+          pht(
+            'You do not have any MFA factors attached to your account, so '.
+            'you can not sign this transaction group with MFA. Add MFA to '.
+            'your account in Settings.'),
+          $xaction);
+      }
+    }
+
+    if ($xactions) {
+      $this->setShouldRequireMFA(true);
+    }
+
+    return $errors;
+  }
+
   protected function adjustObjectForPolicyChecks(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -4707,4 +4806,48 @@ abstract class PhabricatorApplicationTransactionEditor
       ->setNewValue($new_value);
   }
 
+  private function requireMFA(PhabricatorLiskDAO $object, array $xactions) {
+    $editor_class = get_class($this);
+
+    $object_phid = $object->getPHID();
+    if ($object_phid) {
+      $workflow_key = sprintf(
+        'editor(%s).phid(%s)',
+        $editor_class,
+        $object_phid);
+    } else {
+      $workflow_key = sprintf(
+        'editor(%s).new()',
+        $editor_class);
+    }
+
+    $actor = $this->getActor();
+
+    $request = $this->getRequest();
+    if ($request === null) {
+      throw new Exception(
+        pht(
+          'This transaction group requires MFA to apply, but the Editor was '.
+          'not configured with a Request. This workflow can not perform an '.
+          'MFA check.'));
+    }
+
+    $cancel_uri = $this->getCancelURI();
+    if ($cancel_uri === null) {
+      throw new Exception(
+        pht(
+          'This transaction group requires MFA to apply, but the Editor was '.
+          'not configured with a Cancel URI. This workflow can not perform '.
+          'an MFA check.'));
+    }
+
+    id(new PhabricatorAuthSessionEngine())
+      ->setWorkflowKey($workflow_key)
+      ->requireHighSecurityToken($actor, $request, $cancel_uri);
+
+    foreach ($xactions as $xaction) {
+      $xaction->setIsMFATransaction(true);
+    }
+  }
+
 }

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -473,6 +473,8 @@ abstract class PhabricatorApplicationTransaction
         return 'fa-th-large';
       case PhabricatorTransactions::TYPE_COLUMNS:
         return 'fa-columns';
+      case PhabricatorTransactions::TYPE_MFA:
+        return 'fa-vcard';
     }
 
     return 'fa-pencil';
@@ -510,6 +512,8 @@ abstract class PhabricatorApplicationTransaction
             return 'sky';
         }
         break;
+      case PhabricatorTransactions::TYPE_MFA;
+        return 'pink';
     }
     return null;
   }
@@ -835,6 +839,10 @@ abstract class PhabricatorApplicationTransaction
         return pht(
           'You have not moved this object to any columns it is not '.
           'already in.');
+      case PhabricatorTransactions::TYPE_MFA:
+        return pht(
+          'You can not sign a transaction group that has no other '.
+          'effects.');
     }
 
     return pht(
@@ -1076,6 +1084,12 @@ abstract class PhabricatorApplicationTransaction
         }
         break;
 
+
+      case PhabricatorTransactions::TYPE_MFA:
+        return pht(
+          '%s signed these changes with MFA.',
+          $this->renderHandleLink($author_phid));
+
       default:
         // In developer mode, provide a better hint here about which string
         // we're missing.
@@ -1238,6 +1252,9 @@ abstract class PhabricatorApplicationTransaction
         }
         break;
 
+      case PhabricatorTransactions::TYPE_MFA:
+        return null;
+
     }
 
     return $this->getTitle();
@@ -1320,6 +1337,10 @@ abstract class PhabricatorApplicationTransaction
         // (which are shown anyway) but less interesting than any other type of
         // transaction.
         return 0.75;
+      case PhabricatorTransactions::TYPE_MFA:
+        // We want MFA signatures to render at the top of transaction groups,
+        // on top of the things they signed.
+        return 10;
     }
 
     return 1.0;
@@ -1434,6 +1455,8 @@ abstract class PhabricatorApplicationTransaction
       $this_source = $this->getContentSource()->getSource();
     }
 
+    $type_mfa = PhabricatorTransactions::TYPE_MFA;
+
     foreach ($group as $xaction) {
       // Don't group transactions by different authors.
       if ($xaction->getAuthorPHID() != $this->getAuthorPHID()) {
@@ -1477,6 +1500,13 @@ abstract class PhabricatorApplicationTransaction
       if ($is_mfa != $xaction->getIsMFATransaction()) {
         return false;
       }
+
+      // Don't group two "Sign with MFA" transactions together.
+      if ($this->getTransactionType() === $type_mfa) {
+        if ($xaction->getTransactionType() === $type_mfa) {
+          return false;
+        }
+      }
     }
 
     return true;

src/view/phui/PHUITimelineEventView.php

@@ -605,7 +605,7 @@ final class PHUITimelineEventView extends AphrontView {
       // provide a hint that it was extra authentic.
       if ($this->getIsMFA()) {
         $extra[] = id(new PHUIIconView())
-          ->setIcon('fa-vcard', 'green')
+          ->setIcon('fa-vcard', 'pink')
           ->setTooltip(pht('MFA Authenticated'));
       }
     }