mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-10 23:01:04 +01:00
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
This commit is contained in:
parent
f838ad1827
commit
aacc62463d
5 changed files with 73 additions and 2 deletions
|
@ -31,6 +31,25 @@ final class PhabricatorApplicationTransactionCommentEditController
|
||||||
|
|
||||||
$done_uri = $obj_handle->getURI();
|
$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()) {
|
if ($request->isFormOrHisecPost()) {
|
||||||
$text = $request->getStr('text');
|
$text = $request->getStr('text');
|
||||||
|
|
||||||
|
|
|
@ -32,6 +32,26 @@ final class PhabricatorApplicationTransactionCommentRemoveController
|
||||||
|
|
||||||
$done_uri = $obj_handle->getURI();
|
$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()) {
|
if ($request->isFormOrHisecPost()) {
|
||||||
$comment = $xaction->getApplicationTransactionCommentObject()
|
$comment = $xaction->getApplicationTransactionCommentObject()
|
||||||
->setContent('')
|
->setContent('')
|
||||||
|
|
|
@ -189,6 +189,10 @@ final class PhabricatorApplicationTransactionCommentEditor
|
||||||
$actor,
|
$actor,
|
||||||
$xaction,
|
$xaction,
|
||||||
PhabricatorPolicyCapability::CAN_EDIT);
|
PhabricatorPolicyCapability::CAN_EDIT);
|
||||||
|
PhabricatorPolicyFilter::requireCapability(
|
||||||
|
$actor,
|
||||||
|
$xaction->getObject(),
|
||||||
|
PhabricatorPolicyCapability::CAN_INTERACT);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -505,12 +505,19 @@ class PhabricatorApplicationTransactionView extends AphrontView {
|
||||||
if ($has_edit_capability && !$has_removed_comment) {
|
if ($has_edit_capability && !$has_removed_comment) {
|
||||||
$event->setIsEditable(true);
|
$event->setIsEditable(true);
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($has_edit_capability || $viewer->getIsAdmin()) {
|
if ($has_edit_capability || $viewer->getIsAdmin()) {
|
||||||
if (!$has_removed_comment) {
|
if (!$has_removed_comment) {
|
||||||
$event->setIsRemovable(true);
|
$event->setIsRemovable(true);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$can_interact = PhabricatorPolicyFilter::hasCapability(
|
||||||
|
$viewer,
|
||||||
|
$xaction->getObject(),
|
||||||
|
PhabricatorPolicyCapability::CAN_INTERACT);
|
||||||
|
$event->setCanInteract($can_interact);
|
||||||
}
|
}
|
||||||
|
|
||||||
$comment = $this->renderTransactionContent($xaction);
|
$comment = $this->renderTransactionContent($xaction);
|
||||||
|
|
|
@ -32,6 +32,7 @@ final class PHUITimelineEventView extends AphrontView {
|
||||||
private $isSilent;
|
private $isSilent;
|
||||||
private $isMFA;
|
private $isMFA;
|
||||||
private $isLockOverride;
|
private $isLockOverride;
|
||||||
|
private $canInteract;
|
||||||
|
|
||||||
public function setAuthorPHID($author_phid) {
|
public function setAuthorPHID($author_phid) {
|
||||||
$this->authorPHID = $author_phid;
|
$this->authorPHID = $author_phid;
|
||||||
|
@ -114,6 +115,15 @@ final class PHUITimelineEventView extends AphrontView {
|
||||||
return $this->isEditable;
|
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) {
|
public function setIsRemovable($is_removable) {
|
||||||
$this->isRemovable = $is_removable;
|
$this->isRemovable = $is_removable;
|
||||||
return $this;
|
return $this;
|
||||||
|
@ -650,6 +660,10 @@ final class PHUITimelineEventView extends AphrontView {
|
||||||
private function getMenuItems($anchor) {
|
private function getMenuItems($anchor) {
|
||||||
$xaction_phid = $this->getTransactionPHID();
|
$xaction_phid = $this->getTransactionPHID();
|
||||||
|
|
||||||
|
$can_interact = $this->getCanInteract();
|
||||||
|
$viewer = $this->getViewer();
|
||||||
|
$is_admin = $viewer->getIsAdmin();
|
||||||
|
|
||||||
$items = array();
|
$items = array();
|
||||||
|
|
||||||
if ($this->getIsEditable()) {
|
if ($this->getIsEditable()) {
|
||||||
|
@ -658,6 +672,7 @@ final class PHUITimelineEventView extends AphrontView {
|
||||||
->setHref('/transactions/edit/'.$xaction_phid.'/')
|
->setHref('/transactions/edit/'.$xaction_phid.'/')
|
||||||
->setName(pht('Edit Comment'))
|
->setName(pht('Edit Comment'))
|
||||||
->addSigil('transaction-edit')
|
->addSigil('transaction-edit')
|
||||||
|
->setDisabled(!$can_interact)
|
||||||
->setMetadata(
|
->setMetadata(
|
||||||
array(
|
array(
|
||||||
'anchor' => $anchor,
|
'anchor' => $anchor,
|
||||||
|
@ -727,17 +742,23 @@ final class PHUITimelineEventView extends AphrontView {
|
||||||
$items[] = id(new PhabricatorActionView())
|
$items[] = id(new PhabricatorActionView())
|
||||||
->setType(PhabricatorActionView::TYPE_DIVIDER);
|
->setType(PhabricatorActionView::TYPE_DIVIDER);
|
||||||
|
|
||||||
$items[] = id(new PhabricatorActionView())
|
$remove_item = id(new PhabricatorActionView())
|
||||||
->setIcon('fa-trash-o')
|
->setIcon('fa-trash-o')
|
||||||
->setHref('/transactions/remove/'.$xaction_phid.'/')
|
->setHref('/transactions/remove/'.$xaction_phid.'/')
|
||||||
->setName(pht('Remove Comment'))
|
->setName(pht('Remove Comment'))
|
||||||
->setColor(PhabricatorActionView::RED)
|
|
||||||
->addSigil('transaction-remove')
|
->addSigil('transaction-remove')
|
||||||
->setMetadata(
|
->setMetadata(
|
||||||
array(
|
array(
|
||||||
'anchor' => $anchor,
|
'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;
|
return $items;
|
||||||
|
|
Loading…
Reference in a new issue