From 15cc475cbd8c60d1c3524f0bda06f5fce7f762e7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Mar 2019 15:32:23 -0700 Subject: [PATCH] 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 --- ...cationTransactionCommentEditController.php | 27 +++- ...tionTransactionCommentRemoveController.php | 11 +- ...torApplicationTransactionCommentEditor.php | 121 ++++++++++++++++++ ...habricatorApplicationTransactionEditor.php | 3 +- 4 files changed, 151 insertions(+), 11 deletions(-) diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php index a93f16a688..1682a7d136 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php +++ b/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); } } diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php index c52b087273..381dfe1176 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php +++ b/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; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php index f9db0e238e..d963ea2ecb 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php +++ b/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); + } } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 3e5e9c23c9..06c9b43216 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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();