When a comment was signed with MFA, require MFA to edit it

Summary:
Ref PHI1173. Currently, you can edit an MFA'd comment without redoing MFA. This is inconsistent with the intent of the MFA badge, since it means an un-MFA'd comment may have an "MFA" badge on it.

Instead, implement these rules:

  - If a comment was signed with MFA, you MUST MFA to edit it.
  - When removing a comment, add an extra MFA prompt if the user has MFA. This one isn't strictly required, this action is just very hard to undo and seems reasonable to MFA.

Test Plan:
  - Made normal comments and MFA comments.
  - Edited normal comments and MFA comments (got prompted).
  - Removed normal comments and MFA comments (prompted in both cases).
  - Tried to edit an MFA comment without MFA on my account, got a hard "MFA absolutely required" failure.

Reviewers: amckinley

Reviewed By: amckinley

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

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php

@@ -29,7 +29,9 @@ final class PhabricatorApplicationTransactionCommentEditController
     $handles = $viewer->loadHandles(array($phid));
     $obj_handle = $handles[$phid];
 
-    if ($request->isDialogFormPost()) {
+    $done_uri = $obj_handle->getURI();
+
+    if ($request->isFormOrHisecPost()) {
       $text = $request->getStr('text');
 
       $comment = $xaction->getApplicationTransactionCommentObject();
@@ -41,29 +43,42 @@ final class PhabricatorApplicationTransactionCommentEditController
       $editor = id(new PhabricatorApplicationTransactionCommentEditor())
         ->setActor($viewer)
         ->setContentSource(PhabricatorContentSource::newFromRequest($request))
+        ->setRequest($request)
+        ->setCancelURI($done_uri)
         ->applyEdit($xaction, $comment);
 
       if ($request->isAjax()) {
         return id(new AphrontAjaxResponse())->setContent(array());
       } else {
-        return id(new AphrontReloadResponse())->setURI($obj_handle->getURI());
+        return id(new AphrontReloadResponse())->setURI($done_uri);
       }
     }
 
+    $errors = array();
+    if ($xaction->getIsMFATransaction()) {
+      $message = pht(
+        'This comment was signed with MFA, so you will be required to '.
+        'provide MFA credentials to make changes.');
+
+      $errors[] = id(new PHUIInfoView())
+        ->setSeverity(PHUIInfoView::SEVERITY_MFA)
+        ->setErrors(array($message));
+    }
+
     $form = id(new AphrontFormView())
       ->setUser($viewer)
       ->setFullWidth(true)
       ->appendControl(
         id(new PhabricatorRemarkupControl())
-        ->setName('text')
-        ->setValue($xaction->getComment()->getContent()));
+          ->setName('text')
+          ->setValue($xaction->getComment()->getContent()));
 
     return $this->newDialog()
       ->setTitle(pht('Edit Comment'))
-      ->addHiddenInput('anchor', $request->getStr('anchor'))
+      ->appendChild($errors)
       ->appendForm($form)
       ->addSubmitButton(pht('Save Changes'))
-      ->addCancelButton($obj_handle->getURI());
+      ->addCancelButton($done_uri);
   }
 
 }

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php

@@ -30,20 +30,24 @@ final class PhabricatorApplicationTransactionCommentRemoveController
       ->withPHIDs(array($obj_phid))
       ->executeOne();
 
-    if ($request->isDialogFormPost()) {
+    $done_uri = $obj_handle->getURI();
+
+    if ($request->isFormOrHisecPost()) {
       $comment = $xaction->getApplicationTransactionCommentObject()
         ->setContent('')
         ->setIsRemoved(true);
 
       $editor = id(new PhabricatorApplicationTransactionCommentEditor())
         ->setActor($viewer)
+        ->setRequest($request)
+        ->setCancelURI($done_uri)
         ->setContentSource(PhabricatorContentSource::newFromRequest($request))
         ->applyEdit($xaction, $comment);
 
       if ($request->isAjax()) {
         return id(new AphrontAjaxResponse())->setContent(array());
       } else {
-        return id(new AphrontReloadResponse())->setURI($obj_handle->getURI());
+        return id(new AphrontReloadResponse())->setURI($done_uri);
       }
     }
 
@@ -54,7 +58,6 @@ final class PhabricatorApplicationTransactionCommentRemoveController
       ->setTitle(pht('Remove Comment'));
 
     $dialog
-      ->addHiddenInput('anchor', $request->getStr('anchor'))
       ->appendParagraph(
         pht(
           "Removing a comment prevents anyone (including you) from reading ".
@@ -65,7 +68,7 @@ final class PhabricatorApplicationTransactionCommentRemoveController
 
     $dialog
       ->addSubmitButton(pht('Remove Comment'))
-      ->addCancelButton($obj_handle->getURI());
+      ->addCancelButton($done_uri);
 
     return $dialog;
   }

src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php

@@ -5,6 +5,9 @@ final class PhabricatorApplicationTransactionCommentEditor
 
   private $contentSource;
   private $actingAsPHID;
+  private $request;
+  private $cancelURI;
+  private $isNewComment;
 
   public function setActingAsPHID($acting_as_phid) {
     $this->actingAsPHID = $acting_as_phid;
@@ -27,6 +30,33 @@ final class PhabricatorApplicationTransactionCommentEditor
     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;
+  }
+
+  public function setIsNewComment($is_new) {
+    $this->isNewComment = $is_new;
+    return $this;
+  }
+
+  public function getIsNewComment() {
+    return $this->isNewComment;
+  }
+
   /**
    * Edit a transaction's comment. This method effects the required create,
    * update or delete to set the transaction's comment to the provided comment.
@@ -39,6 +69,8 @@ final class PhabricatorApplicationTransactionCommentEditor
 
     $actor = $this->requireActor();
 
+    $this->applyMFAChecks($xaction, $comment);
+
     $comment->setContentSource($this->getContentSource());
     $comment->setAuthorPHID($this->getActingAsPHID());
 
@@ -160,5 +192,94 @@ final class PhabricatorApplicationTransactionCommentEditor
     }
   }
 
+  private function applyMFAChecks(
+    PhabricatorApplicationTransaction $xaction,
+    PhabricatorApplicationTransactionComment $comment) {
+    $actor = $this->requireActor();
+
+    // We don't do any MFA checks here when you're creating a comment for the
+    // first time (the parent editor handles them for us), so we can just bail
+    // out if this is the creation flow.
+    if ($this->getIsNewComment()) {
+      return;
+    }
+
+    $request = $this->getRequest();
+    if (!$request) {
+      throw new PhutilInvalidStateException('setRequest');
+    }
+
+    $cancel_uri = $this->getCancelURI();
+    if (!strlen($cancel_uri)) {
+      throw new PhutilInvalidStateException('setCancelURI');
+    }
+
+    // If you're deleting a comment, we try to prompt you for MFA if you have
+    // it configured, but do not require that you have it configured. In most
+    // cases, this is administrators removing content.
+
+    // See PHI1173. If you're editing a comment you authored and the original
+    // comment was signed with MFA, you MUST have MFA on your account and you
+    // MUST sign the edit with MFA. Otherwise, we can end up with an MFA badge
+    // on different content than what was signed.
+
+    $want_mfa = false;
+    $need_mfa = false;
+
+    if ($comment->getIsRemoved()) {
+      // Try to prompt on removal.
+      $want_mfa = true;
+    }
+
+    if ($xaction->getIsMFATransaction()) {
+      if ($actor->getPHID() === $xaction->getAuthorPHID()) {
+        // Strictly require MFA if the original transaction was signed and
+        // you're the author.
+        $want_mfa = true;
+        $need_mfa = true;
+      }
+    }
+
+    if (!$want_mfa) {
+      return;
+    }
+
+    if ($need_mfa) {
+      $factors = id(new PhabricatorAuthFactorConfigQuery())
+        ->setViewer($actor)
+        ->withUserPHIDs(array($this->getActingAsPHID()))
+        ->withFactorProviderStatuses(
+          array(
+            PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE,
+            PhabricatorAuthFactorProviderStatus::STATUS_DEPRECATED,
+          ))
+        ->execute();
+      if (!$factors) {
+        $error = new PhabricatorApplicationTransactionValidationError(
+          $xaction->getTransactionType(),
+          pht('No MFA'),
+          pht(
+            'This comment was signed with MFA, so edits to it must also be '.
+            'signed with MFA. You do not have any MFA factors attached to '.
+            'your account, so you can not sign this edit. Add MFA to your '.
+            'account in Settings.'),
+          $xaction);
+
+        throw new PhabricatorApplicationTransactionValidationException(
+          array(
+            $error,
+          ));
+      }
+    }
+
+    $workflow_key = sprintf(
+      'comment.edit(%s, %d)',
+      $xaction->getPHID(),
+      $xaction->getComment()->getID());
+
+    $hisec_token = id(new PhabricatorAuthSessionEngine())
+      ->setWorkflowKey($workflow_key)
+      ->requireHighSecurityToken($actor, $request, $cancel_uri);
+  }
 
 }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1113,7 +1113,8 @@ abstract class PhabricatorApplicationTransactionEditor
       $comment_editor = id(new PhabricatorApplicationTransactionCommentEditor())
         ->setActor($actor)
         ->setActingAsPHID($this->getActingAsPHID())
-        ->setContentSource($this->getContentSource());
+        ->setContentSource($this->getContentSource())
+        ->setIsNewComment(true);
 
       if (!$transaction_open) {
         $object->openTransaction();