Details

Reviewers

epriestley
chad

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4411: Adding a CC to a Maniphest Task should give View rights for that user

Commits

Restricted Diffusion Commit
rPcd677161e16a: 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

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

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 task: T4411: Adding a CC to a Maniphest Task should give View rights for that user.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptDec 29 2014, 11:22 AM

Herald added a subscriber: epriestley. · View Herald Transcript

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

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

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.

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

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."

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

Herald added a subscriber: Korvin. · View Herald TranscriptDec 29 2014, 4:15 PM

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.

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.

epriestley mentioned this in T6822: Fix methods which override other methods with the wrong visibility.Dec 29 2014, 4:53 PM

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.

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.

Looks great to me.

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

Screen_Shot_2014-12-30_at_7.36.45_AM.png (210×574 px, 19 KB)

@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

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.

updating for style and inlines

Herald added a reviewer: chad. · View Herald TranscriptDec 30 2014, 4:31 PM

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?

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');`.

simplify maniphest transaction fix

Awesome!

This revision is now accepted and ready to land.Jan 1 2015, 4:05 PM

Closed by commit rPcd677161e16a: Do not CC users without permissions to view an object (authored by fabe, committed by epriestley). · Explain WhyJan 1 2015, 4:06 PM

This revision was automatically updated to reflect the committed changes.

epriestley mentioned this in T6858: "Depends on Dx" generates a superfluous edge.Jan 4 2015, 5:02 PM

epriestley mentioned this in T7578: Render username mentions in Conpherence in a disabled style if the user can not see the thread.Mar 16 2015, 7:43 PM

Do not CC users without permissions to view an object
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26715

resources/celerity/map.php

src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php

src/applications/maniphest/controller/ManiphestTaskDetailController.php

src/applications/maniphest/controller/ManiphestTransactionPreviewController.php

src/applications/maniphest/controller/ManiphestTransactionSaveController.php

src/applications/people/markup/PhabricatorMentionRemarkupRule.php

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

src/infrastructure/markup/PhabricatorMarkupEngine.php

webroot/rsrc/css/core/remarkup.css

Do not CC users without permissions to view an objectClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26715

resources/celerity/map.php

src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php

src/applications/maniphest/controller/ManiphestTaskDetailController.php

src/applications/maniphest/controller/ManiphestTransactionPreviewController.php

src/applications/maniphest/controller/ManiphestTransactionSaveController.php

src/applications/people/markup/PhabricatorMentionRemarkupRule.php

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

src/infrastructure/markup/PhabricatorMarkupEngine.php

webroot/rsrc/css/core/remarkup.css

Do not CC users without permissions to view an object
ClosedPublic
Actions

Revision Contents
Changeset List