Page MenuHomePhabricator

Do not CC users without permissions to view an object
ClosedPublic

Authored by fabe on Dec 29 2014, 11:22 AM.

Details

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

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fabe updated this revision to Diff 26530.Dec 29 2014, 11:22 AM
fabe retitled this revision from to do not cc users without permissions to view an object.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
fabe added a comment.Dec 29 2014, 11:24 AM

Ah i forgot about the greying out users... That should provide enough feedback why a user is not mentioned

fabe updated this revision to Diff 26531.Dec 29 2014, 11:30 AM
fabe edited edge metadata.

add policy check when commenting on tasks

fabe updated this object.Dec 29 2014, 11:31 AM
fabe edited the test plan for this revision. (Show Details)
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
fabe edited the test plan for this revision. (Show Details)Dec 29 2014, 11:34 AM
fabe added a comment.Dec 29 2014, 12:09 PM

As far as i can see at the moment a remarkup rule is not aware of what context it is rendered in (which specific task / diff / commit and even which type).
I think it would generelly be useful to have that information especially for policy stuff but I'm not sure how to approach this.

epriestley requested changes to this revision.Dec 29 2014, 3:49 PM
epriestley added a reviewer: epriestley.

This looks great to me. To answer your questions...

Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments?
Ah i forgot about the greying out users... That should provide enough feedback why a user is not mentioned

This looks like the right place to me. And, yeah, I think once we grey out the mention, that will be enough feedback for users to figure it out.

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 done

Hmm, this is surprising to me. I would expect it to use the same code. If you can't figure out what's going on, let me know and I'll dig around a bit.

Unrelated: Is there any way to remove a subscriber from a commit/audit ?

Not right now. There's no technical or product reason for this, it's just that no one has asked for it yet.

As far as i can see at the moment a remarkup rule is not aware of what context it is rendered in (which specific task / diff / commit and even which type).
I think it would generelly be useful to have that information especially for policy stuff but I'm not sure how to approach this.

Yeah. I think it will have to look something like:

  • Add an $object or similar property to PhabricatorMarkupEngine, next to $viewer.
  • In PhabricatorMarkupEngine->process(), have it call $engines[$key]->setConfig('object', $this->object) next to where it passes 'viewer'.
  • Remarkup rules (like the username mention rule) can then access the contextual object with $this->getEngine()->getConfig('object') to do policy checks.
  • Then, the painful part: go find all the places in the code where we create a new PhabicatorRemarkupEngine() and pass the contextual object through a new setObject() call wherever you can. Some of them won't have a meaningful object (for example, instructions on forms), so the code should still work if there's no 'object' available.
src/applications/maniphest/controller/ManiphestTransactionSaveController.php
144–145

I think this should be unnecessary. Even though this code isn't fully modernized, it ultimately goes through the PhabricatorApplicationTransactionEditor pathway, so it should hit the other change. Or was that not the case?

Oh, is this handling "Add CCs: ..." explicitly? Let's just do mentions first in this diff, I think the various ways to "Add CCs:" will be a bit more complicated. See note later on.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1057–1059

Oh, loadPage() is supposed to be protected, it just isn't in this class for some reason. Maybe we can add a reflection-based unit test to catch this stuff: when overloading methods, we always want to retain the same visibility level (in this codebase, at least).

You should write this as:

$users = id(new PhabricatorPeopleQuery())
  ->setViewer($this->getActor())
  ->withPHIDs($phids)
  ->execute();
1065

To be completely correct, we should probably test:

if ($object instanceof PhabricatorPolicyInterface)

...before doing the hasCapability() check. I think there are no PhabricatorSubscribableInterface objects which aren't also PhabricatorPolicyInterface objects right now, but I might be forgetting something and we (or a third party application) might have such an object in the future.

If the object doesn't implement PhabricatorPolicyInterface, we can assume all users can get mentioned.

1715–1716

Specifically, I would expect the explicit CC case to go here, in a new TYPE_SUBSCRIBERS block. It would check visibility and then add an error to the $errors[] array, like "You can not add users X, Y or Z as subscribers because they can not see this object.". That should give a clean piece of feedback on explicit CCs, no matter how they were added (e.g., "Add CCs" or "Edit -> change the CCs field", or Differential commit message stuff).

Once that works, we can grey out the username tokens in the tokenizer when the user types them, to provide similar feedback to greying out mentions.

This revision now requires changes to proceed.Dec 29 2014, 3:49 PM
fabe added a comment.Dec 29 2014, 4:05 PM

I'll add the other changes. Would you already like handling the 'explicit cc' case? (It was indicated in the task that it might be too severe for the start.

src/applications/maniphest/controller/ManiphestTransactionSaveController.php
144–145

It just seems to be used when commenting on a task. If i remove i can comment an mention users having no permission and they will get added.
Manually adding cc's via the "Edit task" function is unaffected by this.
The comment above is quite suspicious:
"// Evade no-effect detection in the new editor stuff until we can switch to subscriptions."

fabe updated this revision to Diff 26534.Dec 29 2014, 4:15 PM
fabe edited edge metadata.

execute instead of loadpage and check if policy is available for the object

I think we should skip the explicit CC case for now -- just continue to let users explicitly CC anyone. I think it's probably a little severe for now, and the explicit CC vs mention CC stuff seems pretty easy to separate into two different changes. We can do the mention stuff first, make sure it works properly, see how it feels, and then move forward with the explicit CC stuff later.

fabe added a comment.Dec 29 2014, 4:29 PM

the problem when creating a diff still persists but is not so easy to fix. The problem is that the diff is created with default permissions (public) and then of course the user can be mentioned in the comments too. Maybe a diff should inherit it's policies from the repository it's attached to?

Probably, yeah -- don't worry about that for now, and I can take a look and figure out how to proceed there. It might not be that complicated.

fabe updated this revision to Diff 26537.Dec 29 2014, 5:29 PM
fabe edited edge metadata.

Display user having no permissions to a task with a grey background. (actual style TBD ;) )
For a start now only in maniphest. If this is ok i'll track down all the other uses of MarkupEngine.

The preview seems to be rendered differently. Even though my code (setBackgroundColor) gets executed it won't change.
Maybe there's some css magic happening in the preview? I've got no idea where to look for that problem.

fabe added a comment.Dec 30 2014, 1:23 PM

Ah ok. rendering policy aware stuff in preview is problematic as the preview renderer just creates a bogus task and adds the text to it...
To make the preview consider policies it would have to load the complete real task. Not sure if we actually want to do this.

epriestley requested changes to this revision.Dec 30 2014, 3:41 PM
epriestley edited edge metadata.
epriestley added a subscriber: chad.

Looks great to me.

Some inlines for styling, I ended up with a tag that looks like this:

@chad can fix it up later if he comes up with a better approach.

I think fixing the preview by loading the object is desirable, but we could do that in a followup change -- it might be a bit involved. I don't think previews know which object they're previewing for right now.

src/applications/maniphest/controller/ManiphestTransactionSaveController.php
144–145

Let's not do this yet -- this prevents explicit CC's, right? We can move that to another diff later on.

src/applications/people/markup/PhabricatorMentionRemarkupRule.php
95

Prefer $context_object for consistency.

121

Let's change these properties for now:

border-color: #92969D;
color: #92969D;
background: #F7F7F7;

And keep everything else the same. I think that'll look reasonable, and it's easy to tweak later.

152–154

It might also be nice to do:

$tag->addSigil('has-tooltip');
$tag->setMetadata(
  array(
    'tip' => pht(
      'This user does not have permission to view this page.'),
    'size' => 350,
  ));

...but I think that might be overkill. Let's try without that first and see if the feature is confusing to users.

153

Let's add this:

$tag->addClass('phabricator-remarkup-mention-nopermission');

Then add this rule to webroot/rsrc/css/core/remarkup.css:

.phabricator-remarkup-mention-nopermission .phui-tag-core {
  background: {$lightgreybackground};
  color: {$lightgreytext};
}
This revision now requires changes to proceed.Dec 30 2014, 3:41 PM
chad added a comment.Dec 30 2014, 4:03 PM

I can take a look after this lands, !

fabe added inline comments.Dec 30 2014, 4:28 PM
src/applications/maniphest/controller/ManiphestTransactionSaveController.php
144–145

No this is actually needed for checking the policy when commenting on a task.
Every other place i found runs through the TransactionEditor but not the task comments.

fabe updated this revision to Diff 26616.Dec 30 2014, 4:31 PM
fabe edited edge metadata.

updating for style and inlines

chad added inline comments.Dec 30 2014, 4:57 PM
src/applications/people/markup/PhabricatorMentionRemarkupRule.php
122

We should move to CSS, inline styles are an old-old-Phabricator relic.

chad retitled this revision from do not cc users without permissions to view an object to Do not cc users without permissions to view an object.Dec 30 2014, 4:57 PM
chad edited edge metadata.
chad retitled this revision from Do not cc users without permissions to view an object to Do not CC users without permissions to view an object.

The inline styles are for HTML mail -- there isn't sufficient support among mail client for style sheets to use them.

We could probably run them through the CSS preprocessor so we can use the color constants, but we'd have to do a lot of work to actually move them to CSS and have Phabricator inline them when sending mail.

This looks great to me, except I'm still not sure about that SaveController block. It really seems like we don't need it?

src/applications/maniphest/controller/ManiphestTransactionSaveController.php
177

I still don't think the block above is required -- we'll go into the transaction editor here, and it seems to works fine locally for me.

When this block is removed I can still "Add CCs", but that's expected and desirable (for now). @mentioning users correctly greys out the ones who can't see the object. Am I just doing something wrong?

fabe added a comment.Jan 1 2015, 12:46 PM

The SaveController block is needed to prevent the user mentioned being added as a CC if she has no permission.
Without it they are greyed out correctly but are still added as CC.
In the TransactionEditor we are only catching mentions and do not look at already added subscriber transactions.
Normally i'd think the block in the ManiphestTransactionSaveController should be removed completely as the mentions are extracted in the TransactionEditor anyway.
However this seems not to be the case for maniphest comments. When i remove the block in question mentioning no longer works at all.

Ah, sorry, it looks like I messed something up locally while testing. You're correct. I think a simpler fix is available, though (see inlines).

src/applications/maniphest/controller/ManiphestTransactionSaveController.php
24–25

Replace this code with $added_ccs = array();, then...

61

Change this to $added_ccs = $request->getArr('ccs');.

fabe updated this revision to Diff 26712.Jan 1 2015, 3:50 PM

simplify maniphest transaction fix

epriestley accepted this revision.Jan 1 2015, 4:05 PM
epriestley edited edge metadata.

Awesome!

This revision is now accepted and ready to land.Jan 1 2015, 4:05 PM
This revision was automatically updated to reflect the committed changes.