From aacc62463d619fb4c8bdf8a660b4ede2b0cc8083 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 May 2019 10:55:55 -0700 Subject: [PATCH] Prevent editing and deleting comments in locked conversations Summary: Ref T13289. This tightens up a couple of corner cases around locked threads. Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes. Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment. Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action. Test Plan: - On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed). - On a locked task, tried to remove another user's comments as an administrator. This works. - On a normal task, edited comments normally. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13289 Differential Revision: https://secure.phabricator.com/D20551 --- ...cationTransactionCommentEditController.php | 19 ++++++++++++++ ...tionTransactionCommentRemoveController.php | 20 +++++++++++++++ ...torApplicationTransactionCommentEditor.php | 4 +++ .../PhabricatorApplicationTransactionView.php | 7 ++++++ src/view/phui/PHUITimelineEventView.php | 25 +++++++++++++++++-- 5 files changed, 73 insertions(+), 2 deletions(-) diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php index 1682a7d136..4529704c2f 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php @@ -31,6 +31,25 @@ final class PhabricatorApplicationTransactionCommentEditController $done_uri = $obj_handle->getURI(); + // If an object is locked, you can't edit comments on it. Two reasons to + // lock threads are to calm contentious issues and to freeze state for + // auditing, and editing comments serves neither goal. + + $object = $xaction->getObject(); + $can_interact = PhabricatorPolicyFilter::hasCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_INTERACT); + if (!$can_interact) { + return $this->newDialog() + ->setTitle(pht('Conversation Locked')) + ->appendParagraph( + pht( + 'You can not edit this comment because the conversation is '. + 'locked.')) + ->addCancelButton($done_uri); + } + if ($request->isFormOrHisecPost()) { $text = $request->getStr('text'); diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php index 381dfe1176..f81535e4ae 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php @@ -32,6 +32,26 @@ final class PhabricatorApplicationTransactionCommentRemoveController $done_uri = $obj_handle->getURI(); + // We allow administrative removal of comments even if an object is locked, + // so you can lock a flamewar and then go clean it up. Locked threads may + // not otherwise be edited, and non-administrators can not remove comments + // from locked threads. + + $object = $xaction->getObject(); + $can_interact = PhabricatorPolicyFilter::hasCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_INTERACT); + if (!$can_interact && !$viewer->getIsAdmin()) { + return $this->newDialog() + ->setTitle(pht('Conversation Locked')) + ->appendParagraph( + pht( + 'You can not remove this comment because the conversation is '. + 'locked.')) + ->addCancelButton($done_uri); + } + if ($request->isFormOrHisecPost()) { $comment = $xaction->getApplicationTransactionCommentObject() ->setContent('') diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php index d963ea2ecb..22acb3312f 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php @@ -189,6 +189,10 @@ final class PhabricatorApplicationTransactionCommentEditor $actor, $xaction, PhabricatorPolicyCapability::CAN_EDIT); + PhabricatorPolicyFilter::requireCapability( + $actor, + $xaction->getObject(), + PhabricatorPolicyCapability::CAN_INTERACT); } } diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index 4d738877b8..7a24bf8ff8 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -505,12 +505,19 @@ class PhabricatorApplicationTransactionView extends AphrontView { if ($has_edit_capability && !$has_removed_comment) { $event->setIsEditable(true); } + if ($has_edit_capability || $viewer->getIsAdmin()) { if (!$has_removed_comment) { $event->setIsRemovable(true); } } } + + $can_interact = PhabricatorPolicyFilter::hasCapability( + $viewer, + $xaction->getObject(), + PhabricatorPolicyCapability::CAN_INTERACT); + $event->setCanInteract($can_interact); } $comment = $this->renderTransactionContent($xaction); diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index 5013611084..e4f11b2b1d 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -32,6 +32,7 @@ final class PHUITimelineEventView extends AphrontView { private $isSilent; private $isMFA; private $isLockOverride; + private $canInteract; public function setAuthorPHID($author_phid) { $this->authorPHID = $author_phid; @@ -114,6 +115,15 @@ final class PHUITimelineEventView extends AphrontView { return $this->isEditable; } + public function setCanInteract($can_interact) { + $this->canInteract = $can_interact; + return $this; + } + + public function getCanInteract() { + return $this->canInteract; + } + public function setIsRemovable($is_removable) { $this->isRemovable = $is_removable; return $this; @@ -650,6 +660,10 @@ final class PHUITimelineEventView extends AphrontView { private function getMenuItems($anchor) { $xaction_phid = $this->getTransactionPHID(); + $can_interact = $this->getCanInteract(); + $viewer = $this->getViewer(); + $is_admin = $viewer->getIsAdmin(); + $items = array(); if ($this->getIsEditable()) { @@ -658,6 +672,7 @@ final class PHUITimelineEventView extends AphrontView { ->setHref('/transactions/edit/'.$xaction_phid.'/') ->setName(pht('Edit Comment')) ->addSigil('transaction-edit') + ->setDisabled(!$can_interact) ->setMetadata( array( 'anchor' => $anchor, @@ -727,17 +742,23 @@ final class PHUITimelineEventView extends AphrontView { $items[] = id(new PhabricatorActionView()) ->setType(PhabricatorActionView::TYPE_DIVIDER); - $items[] = id(new PhabricatorActionView()) + $remove_item = id(new PhabricatorActionView()) ->setIcon('fa-trash-o') ->setHref('/transactions/remove/'.$xaction_phid.'/') ->setName(pht('Remove Comment')) - ->setColor(PhabricatorActionView::RED) ->addSigil('transaction-remove') ->setMetadata( array( 'anchor' => $anchor, )); + if (!$is_admin && !$can_interact) { + $remove_item->setDisabled(!$is_admin && !$can_interact); + } else { + $remove_item->setColor(PhabricatorActionView::RED); + } + + $items[] = $remove_item; } return $items;