Page MenuHomePhabricator

Adding a CC to a Maniphest Task should give View rights for that user
Closed, ResolvedPublic

Description

right now when you assign a Task to a user that has no access rights per the current Visible To/Editable By Policy, then view and edit are granted automatically to the user upon assignment.

A similar thing should happen when you CC someone. That user should be granted View rights on the task (this already includes the ability to comment).

Right now nothing happens, although it is implied by the Assign behaviour.

Since this will impose some performance cost (according to @epriestley), then it might be better to make this into an opt-in feature burried in Maniphests config?


See also T12952 for a case where a user wants a reviewer to have view rights regardless of view policy.

Related Objects

Event Timeline

allan.laal raised the priority of this task from to Needs Triage.
allan.laal updated the task description. (Show Details)
allan.laal added a project: Maniphest.
allan.laal added subscribers: allan.laal, epriestley.
epriestley triaged this task as Wishlist priority.Feb 12 2014, 4:22 PM
epriestley edited projects, added Policy; removed Maniphest.

Generally, I'm on the fence about this. The way it works now is a bit confusing, but making subscriptions imply visibility means we have to load subscriptions for all subscribable objects every time we do visibility checks, which is relatively costly. I'm also not sure that it's the best product decision, since the other case (you CC a user who shouldn't be able to see something, not realizing they aren't allowed to see it) seems plausible to me too.

An alternative might be to show CCs with policy failures in a disabled style.

Particularly, a case where this definitely should not work, in a slightly indirect way, is Conpherence -- mentioning users should not let them see threads.

This hasn't come up before, so I'd like to see other installs hit it and weigh in on their use cases.

An alternative might be to show CCs with policy failures in a disabled style.

I like this idea. When a CC is shown as disabled one could click on it to add View rights to it.

Facebook I think really nails this idea when a user is mentioned but does not have access. The name is greyed out and a circle-minus icon appears. You get a tooltip explaining the issue.

We are mixing two concept here, because Phabricator mixes them: CCing someone and mentioning someone. In terms of access policy, both actions are usually different:

  • When you mention someone there are many chances that you only want to mention them (like Facebook)
  • When you CC someone, you basically want to include them in the loop (like email and your regular task/bug manager).

Since Maniphest falls closer to a task manager than to Facebook, I still claim that it is worth granting permissions to users explicitly CCed in the CC field, ignoring those that are simply mentioned in descriptions and comments -- if possible at all.

chad changed the visibility from "Public (No Login Required)" to "Phacility (Project)".May 16 2014, 6:10 AM
chad changed the visibility from "Phacility (Project)" to "Public (No Login Required)".

I think CC should probably imply visibility in all cases. The only real reason CC does not imply visibility is that it has a bit of a complexity and performance cost, so it was easier not to implement it in the first version of things.

If CC does not imply visibility, rendering CC'd users who can not see the object as grey makes sense to me. But I think having CC create a policy exception is a clearer, cleaner, and more useful approach.

In the case of mentions, perhaps we should generalize the rule that's implicitly in Conpherence already (where mentions do not CC):

  • If a user can see the object already, but is not CC'd, mentioning the user CCs them.
  • If a user can not see the object, mentioning them does not CC or notify them. This could/should render grey with a tooltip ("mlincoln does not currently have permission to see this object. To share this object with her, explicitly add her as a subscriber.").

This would let mentions retain their usefulness in the common case, but make sure things are explicit in the case where policies are implicitly being changed.

Support Impact The current behavior is unexpected and confusing.

I'm going to disagree here with the utmost respect.

Allowing CC to be a side door to the 'view' security policy is really implicit, confusing and problematic. Just in a general sense, it is a positive that the security settings for issues are decoupled from the actual metadata. If a CC user is granted view to a ticket, then any user who can view a ticket can essentially render the entire Security view policy impotent and in an entirely implicit way that does not show up if I go to look at the actual view security policy on the issue. That is a really serious problem from the perspective of a security or a legal team. I do not see a "users can add CC users" permission here https://phabricator.wikimedia.org/applications/view/PhabricatorManiphestApplication/. That seems to mean that any user is who CC'd would get access to the ticket, and those users could all CC people who are then granted implicit access. That would really make my life hard, and I think anyone's life hard who deals with groups of tasks that need their access tightly controlled.

The idea of a restrictive security policy that causes any level of user confusion, to my understanding, is optional. If you want to make all view/edit public/public you are more than welcome. That means anyone who is using the security settings to greater effect definitely wants them to be an explicit and authoritative marking of user abilities. I have worked with ACL's and security settings in many contexts and it is entirely normal for a two level process to exist. From my perspective, this is equivalent to saying that every new service on a host should automatically allow itself in the firewall. That would completely devalue the firewall, and worse cause endless confusion and scare every person who is used to working with access controls.

The idea proposed by @chad I think is the most normative and valuable one. If you CC a user on a ticket they do not have access too a greyed out user would make UI hinting sense. The really compelling part for me though is that in our situation, and the situation I have used Phabricator at previously, if you do not have the ability to modify the "security view custom policy" type settings you should not, by design, be able to add a CC user who is not already allowed via that setting.

The best approach possible is going to be to have everything security related be explicit, and the answer for those who do not want to manage their security rules to to throw them open. They will be the chmod 777 users of the Phabricator world, but if I imagine a *nix security policy world where there are two parallel security policies applied to objects where one is public and easily auditable, and the other is implicit and built from a list of metadata associated that has more nuanced and confusing restrictions themselves it makes me really afraid.

Please don't do this. Or at least make it optional :)

In general, a user who can view a task can already take a screenshot of it and share it with whoever they want. I think of this as very similar to that, except that there's an audit log and the grant is revocable.

Can you explain how it's different from your point of view? That is, what sort of users do you trust to view security issues, and trust not to abuse screenshots, but not trust with "CC"?

I think the case of accidentally adding CCs is good to think about and the design here should account for that (e.g., make mentions not default-cc users who can't see the object, possibly prompt when adding explicit CCs if they'll create policy exceptions), but that we should let users do it if the workflow is explicit enough.

In particular, if I can see a ticket and want to tell someone about it, and I try to add them to CC and the thing says "you can't add them to CC", I'm just going to take a screenshot or copy/paste it to them. This seems worse in every way than letting me add them to CC?

@chasemp and I talked about this some more on IRC. We still don't have exactly the same viewpoint on where we should end up, but I think we can take an approach which everyone's onboard with for as long as possible and then possibly deviate a little at the end. Broadly, I want to implement the parts of this which would lock down or restrict capabilities first. Once we've implemented the heaviest set of restrictions, we can evaluate what holes we want to punch through them based on user feedback.

Specifically, these changes would be part of the lockdown phase. These are unambiguously desirable:

  • When a user is mentioned on an object, do not CC them if they can not already see the object.
  • When a user mention is displayed on an object and the user can not see the object, show the mention in a greyed out style.
  • When sending mail to users, we should maybe filter users who are somehow on the list but don't have CAN_VIEW (see related issue in T6367).

We can start with these, but they may be too severe:

  • When a user is explicitly CC'd on an object and they can not already see it, prevent the CC.
  • Require CAN_EDIT on an object to add a project CC (this may still be slightly confusing, because it still won't create a policy exception).
  • Herald "add CC" and "send an email" rules may need some locking down? Not 100% sure where these should go.

Based on feedback, we can then relax these rules if necessary, e.g.:

  • Prevent explicit CCs -> allow explicit CCs if actor has CAN_EDIT and answers a prompt -> allow explicit CCs if actor answers a prompt -> allow explicit CCs.
  • Require CAN_EDIT to add project CCs -> require explicit confirmation to add project CCs -> allow explicit project CCs.

I suspect we'll relax them about one step (which would require CC to punch a hole through policies), but may not need to relax them if no one actually cares about these use cases.

Generally, the bad part of this right now is that it's not clear that CC's don't punch a hole through policies. The lockdown phase will make that relationship clear, and let us collect feedback about whether that's good enough or there's a real need for CCs to provide a more convenient mechanism for policy exceptions.

As usual, reasonable and clear. Thanks man.

qgil moved this task from Backlog to Important on the Wikimedia board.

Isn't this one resolved now, thanks to object policies?

A lot of the practical cases here are resolved, but the root problem still exists. Basically, the core issue is that an object can say:

Subscribers: alincoln, htaft, gwashington

...but some or all of those users may not actually be able to see the object, or receive any updates about it, so they're subscribers in name only. I think that's confusing and not desirable (for example, you might expect @htaft to see comments you post, but he may be unable to).

There are various reasonable fixes, roughly from "make it clear that @htaft can't see the object" to "let @htaft see the object". I tend to favor actually letting @htaft see the object, but we need to add a bunch of other checks and hints if we move toward that to make sure users aren't accidentally opening objects up.

In my personal opinion adding a CC that cannot see the task is an error and like this should be treated, meaning ux should just report as error when try to save it. It is important that ux reports it since it can be overlooked by the person changing the task.

If the user want to do it then should edit before or together also the object visibility adding the persons involved.

Then the task should be renamed in something like: "Adding a CC that cannot see the object should give an error"

We would also like this behaviour, and currently use a workaround to accomplish it (which is why we never came here looking for a fix). We created a custom Create Task form that sets the default view policy to include subscribers.

I imagine most people use a similar workaround currently.

See also T13317.

If you reply to email from an object and add a new user as a recipient in the "To" or "Cc" headers, we treat that as adding them as a subscriber.

Until D20593, we would do this even if the email address was not verified. Historically, see T12237 for changing attitudes toward verified addresses, although this behavior was probably always a mistake.

In T13317, a user added noreply@admin.phacility.com to their Phacility account and became subscribed to several support issues as a result. Under the current ruleset this is harmless from a policy perspective (it did not grant them access to the issues) but this case should be considered carefully if behavior here changes.

epriestley claimed this task.

I'm going to close this in favor of T13602, which has a more cohesive/modern discussion of the issue. Broadly:

  • Recent changes have made it more obvious when users don't have permission to see an object. These will be detailed in the upcoming release notes.
  • I don't plan to make adding subscribers, reviewers, etc., ever grant view permission on its own. In particular, this case seems like a clearcut case where a user being subscribed to an object should not imply that they have view permission:
    • Alice is a member of project "Trusted Advisors".
    • Because she is a member of that project, she can see T123, which has "View Policy: Members of Trusted Advisors".
    • She subscribes to T123.
    • She is removed from "Trusted Advisors".
    • In this case, it seems unambiguously correct that Alice should no longer be able to see T123.
  • Instead, I expect to provide an explicit "Shared With: ..." relationship (perhaps with some other name), which is shown in the UI and can grant users permission to view an object even if they don't satisfy the "Visible To:" policy.
  • Interactions like adding subscribers, reviewers, etc., who can't see the object will eventually prompt you to "Share" the object with users in some form ("Alice can't see this object. Do you want to share it, bypassing view policies?").

Screen Shot 2021-02-13 at 12.50.53 PM.png (467×564 px, 35 KB)

Screen Shot 2021-02-18 at 12.13.20 PM.png (226×333 px, 14 KB)