From cd677161e16a5528d7a77031dbf20b823fb73907 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Thu, 1 Jan 2015 08:05:51 -0800 Subject: [PATCH] Do not CC users without permissions to view an object Summary: Ref T4411 I'm not quite sure if this is the right place for this as it will be difficult to provide proper user feedback of why we removed a particular subscriber. Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments? On the other hand in some cases we cannot really give user feedback why a user was not subscribed (e.g.: commits & diffs) Adding a diff to a repo where the user mentioned has no view permissions the subscriber is currently still added. Still would have to find where this is donet... Any other places? Unrelated: Is there any way to remove a subscriber from a commit/audit ? Test Plan: - Edited tasks with the mentioned user having view permissions to this specific task and without - Raised concern with a commit and commented on the audit with the user having view permissions to the repo and without - Added a commit to a repo with and without the mentioned user having permissions - Mention a user in a task & commit comment with and without permissions - Mentioning a user in a diff description & comments with and without permissions to the specific diff Reviewers: chad, #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: chad, Korvin, epriestley Maniphest Tasks: T4411 Differential Revision: https://secure.phabricator.com/D11049 --- resources/celerity/map.php | 6 ++-- ...iphestTaskDescriptionPreviewController.php | 3 +- .../ManiphestTaskDetailController.php | 1 + .../ManiphestTransactionPreviewController.php | 1 + .../ManiphestTransactionSaveController.php | 13 ++----- .../markup/PhabricatorMentionRemarkupRule.php | 35 +++++++++++++++++-- ...habricatorApplicationTransactionEditor.php | 31 ++++++++++++---- .../markup/PhabricatorMarkupEngine.php | 19 +++++++++- webroot/rsrc/css/core/remarkup.css | 5 +++ 9 files changed, 89 insertions(+), 25 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 595adecebc..8aae4a81b5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'f588bfc3', + 'core.pkg.css' => 'd83f8c13', 'core.pkg.js' => '44aac665', 'darkconsole.pkg.js' => 'd326843f', 'differential.pkg.css' => '8af45893', @@ -104,7 +104,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => 'ca42b69f', - 'rsrc/css/core/remarkup.css' => 'a2d3f9c4', + 'rsrc/css/core/remarkup.css' => '7604f12e', 'rsrc/css/core/syntax.css' => '56c1ba38', 'rsrc/css/core/z-index.css' => '44e1d311', 'rsrc/css/diviner/diviner-shared.css' => '38813222', @@ -725,7 +725,7 @@ return array( 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => 'bbae734c', 'phabricator-profile-css' => '28f433ef', - 'phabricator-remarkup-css' => 'a2d3f9c4', + 'phabricator-remarkup-css' => '7604f12e', 'phabricator-search-results-css' => 'f240504c', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-side-menu-view-css' => '90eafc85', diff --git a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php b/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php index 06f01786b3..a1a484727a 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php @@ -13,7 +13,8 @@ final class ManiphestTaskDescriptionPreviewController $output = PhabricatorMarkupEngine::renderOneObject( $task, ManiphestTask::MARKUP_FIELD_DESCRIPTION, - $request->getUser()); + $request->getUser(), + $task); $content = phutil_tag_div('phabricator-remarkup', $output); diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 29acdc3ad9..d5da76246f 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -124,6 +124,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $engine = new PhabricatorMarkupEngine(); $engine->setViewer($user); + $engine->setContextObject($task); $engine->addObject($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION); $timeline = $this->buildTransactionTimeline( diff --git a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php index 25f78c00df..36fe8d7493 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php @@ -113,6 +113,7 @@ final class ManiphestTransactionPreviewController extends ManiphestController { $engine = new PhabricatorMarkupEngine(); $engine->setViewer($user); + $engine->setContextObject($task); if ($transaction->hasComment()) { $engine->addObject( $transaction->getComment(), diff --git a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php index 6f6e60df27..e68a281ee1 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php @@ -22,16 +22,7 @@ final class ManiphestTransactionSaveController extends ManiphestController { $action = $request->getStr('action'); - // Compute new CCs added by @mentions. Several things can cause CCs to - // be added as side effects: mentions, explicit CCs, users who aren't - // CC'd interacting with the task, and ownership changes. We build up a - // list of all the CCs and then construct a transaction for them at the - // end if necessary. - $added_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions( - $user, - array( - $request->getStr('comments'), - )); + $added_ccs = array(); $cc_transaction = new ManiphestTransaction(); $cc_transaction @@ -67,7 +58,7 @@ final class ManiphestTransactionSaveController extends ManiphestController { case PhabricatorTransactions::TYPE_SUBSCRIBERS: // Accumulate the new explicit CCs into the array that we'll add in // the CC transaction later. - $added_ccs = array_merge($added_ccs, $request->getArr('ccs')); + $added_ccs = $request->getArr('ccs'); // Throw away the primary transaction. $transaction = null; diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 622b28998e..6750e34f0d 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -92,27 +92,52 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { } $engine->setTextMetadata($mentioned_key, $mentioned); + $context_object = $engine->getConfig('contextObject'); foreach ($metadata as $username => $tokens) { $exists = isset($actual_users[$username]); + $user_has_no_permission = false; if ($exists) { $user = $actual_users[$username]; Javelin::initBehavior('phabricator-hovercards'); + // Check if the user has view access to the object she was mentioned in + if ($context_object + && $context_object instanceof PhabricatorPolicyInterface) { + if (!PhabricatorPolicyFilter::hasCapability( + $user, + $context_object, + PhabricatorPolicyCapability::CAN_VIEW)) { + // User mentioned has no permission to this object + $user_has_no_permission = true; + } + } + $user_href = '/p/'.$user->getUserName().'/'; if ($engine->isHTMLMailMode()) { $user_href = PhabricatorEnv::getProductionURI($user_href); + + if ($user_has_no_permission) { + $colors = ' + border-color: #92969D; + color: #92969D; + background-color: #F7F7F7;'; + } else { + $colors = ' + border-color: #f1f7ff; + color: #19558d; + background-color: #f1f7ff;'; + } + $tag = phutil_tag( 'a', array( 'href' => $user_href, - 'style' => 'background-color: #f1f7ff; - border-color: #f1f7ff; + 'style' => $colors.' border: 1px solid transparent; border-radius: 3px; - color: #19558d; font-weight: bold; padding: 0 4px;', ), @@ -124,6 +149,10 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { ->setName('@'.$user->getUserName()) ->setHref($user_href); + if ($user_has_no_permission) { + $tag->addClass('phabricator-remarkup-mention-nopermission'); + } + if (!$user->isUserActivated()) { $tag->setDotColor(PHUITagView::COLOR_GREY); } else { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index af0fa02a76..22182ddaa7 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1053,13 +1053,32 @@ abstract class PhabricatorApplicationTransactionEditor $phids = array_diff($phids, $this->subscribers); } - foreach ($phids as $key => $phid) { - if ($object->isAutomaticallySubscribed($phid)) { - unset($phids[$key]); - } - } - $phids = array_values($phids); + if ($phids) { + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getActor()) + ->withPHIDs($phids) + ->execute(); + $users = mpull($users, null, 'getPHID'); + foreach ($phids as $key => $phid) { + // Do not subscribe mentioned users + // who do not have VIEW Permissions + if ($object instanceof PhabricatorPolicyInterface + && !PhabricatorPolicyFilter::hasCapability( + $users[$phid], + $object, + PhabricatorPolicyCapability::CAN_VIEW) + ) { + unset($phids[$key]); + } else { + if ($object->isAutomaticallySubscribed($phid)) { + unset($phids[$key]); + } + } + } + $phids = array_values($phids); + } + // No else here to properly return null should we unset all subscriber if (!$phids) { return null; } diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index c7f6934216..590301ae29 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -41,6 +41,7 @@ final class PhabricatorMarkupEngine { private $objects = array(); private $viewer; + private $contextObject; private $version = 14; @@ -54,15 +55,18 @@ final class PhabricatorMarkupEngine { * @param PhabricatorMarkupInterface The object to render. * @param string The field to render. * @param PhabricatorUser User viewing the markup. + * @param object A context object for policy checks * @return string Marked up output. * @task markup */ public static function renderOneObject( PhabricatorMarkupInterface $object, $field, - PhabricatorUser $viewer) { + PhabricatorUser $viewer, + $context_object = null) { return id(new PhabricatorMarkupEngine()) ->setViewer($viewer) + ->setContextObject($context_object) ->addObject($object, $field) ->process() ->getOutput($object, $field); @@ -116,6 +120,7 @@ final class PhabricatorMarkupEngine { foreach ($objects as $key => $info) { $engines[$key] = $info['object']->newMarkupEngine($info['field']); $engines[$key]->setConfig('viewer', $this->viewer); + $engines[$key]->setConfig('contextObject', $this->contextObject); } // Load or build the preprocessor caches. @@ -290,6 +295,18 @@ final class PhabricatorMarkupEngine { return $this; } + /** + * Set the context object. Used to implement object permissions. + * + * @param The object in which context this remarkup is used. + * @return this + * @task markup + */ + public function setContextObject($object) { + $this->contextObject = $object; + return $this; + } + /* -( Engine Construction )------------------------------------------------ */ diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index 5f25d69d8d..f5314d2231 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -208,6 +208,11 @@ background: #ffaaaa; } +.phabricator-remarkup-mention-nopermission .phui-tag-core { + background: {$lightgreybackground}; + color: {$lightgreytext}; +} + .phabricator-remarkup .remarkup-note { margin: 16px 0; padding: 12px;