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

@@ -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',

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);
 

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(

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(),

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
+    $added_ccs = array();
-    // 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'),
-      ));
 
     $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;

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;
+              'style' => $colors.'
-                border-color: #f1f7ff;
                 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 {

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 ($phids) {
-      if ($object->isAutomaticallySubscribed($phid)) {
+      $users = id(new PhabricatorPeopleQuery())
-        unset($phids[$key]);
+        ->setViewer($this->getActor())
-      }
+        ->withPHIDs($phids)
-    }
+        ->execute();
-    $phids = array_values($phids);
+      $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;
     }

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  )------------------------------------------------ */
 

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;