Page MenuHomePhabricator

Users CCed in private tasks should be able to access them
Closed, DuplicatePublic

Description

We have a new document that describes our Security setup: https://www.mediawiki.org/wiki/Phabricator/Security

Upstream is going in the direction of assuring first that the View policy is always enforced, and only then look for exceptions. This means that members of the teams within the policy (Security...), assigned, and author will be fine, but other users CCed are considered exceptions. View policy and email notifications will be aligned, either you get both or none.

Proposed solution for CCed users in Wikimedia Phabricator:

  • Only members of teams within the Edit policy can add other users in CC.
  • CC users can view, receive notifications, and comment.
  • CC users cannot edit the task fields and cannot CC other users.
  • The View & comment permissions CC users had in Bugzilla will be respected, for the users with account in Phabricator when we update the ACLs; whoever comes after will need to be added manually.
  • The right solution is to create specific projects to group users trusted in specific areas. Ideally, CCing users by their username should be the exception among the exceptions.

More:

Previous description

This is not working currently:

In T50#4, @flimport wrote:

csteipp wrote on 2014-04-21 16:01:48 (UTC)

In bugzilla, we have a Security product. Bugs in that product can only be seen by people in the security group.

  • By default, the reporter and anyone CC'ed can also see the bug, which is important since,
  • The reporter will want to track the status of the bug
  • We often have non-security people working on the bug, and they need visibility to the discussion, but we wouldn't want to give them access to all security bugs.

Short term solution:

In T75869#787373, @Qgil wrote:

A short term solution (...) can be found by removing the "Security or Sensitive Bug" dropdown and setting the view policy for that task manually, but this is a delicate operation that not everybody can do...

See also: T475: Authors of private tasks should be able to access them

There is a related report upstream: Adding a CC to a Maniphest Task should give View rights for that user

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@chasemp has posted a thoughtful comment upstream where (if I understood him correctly) he opposes to upstream's plans to implement this feature at some point.

Let's discuss here whether we need this feature for Wikimedia or not. At least csteipp should be also involved in this discussion, since he is the one who defined the requirement (explained how it works in Bugzilla today).

Questions:

  • It seems that we are indeed CCing users to private reports in Bugzilla, giving them access to read and comment only in the reports where they are CCed. Are we interested in keeping this functionality in Phabricator?
  • How is this working in RT, and what do we want to do with post-RT tasks in our Phabricator?

@Qgil: it seems @csteipp hasn't yet created an account on this instance of phabricator so he won't have received any notifications.

I'm going to assign this to you since it's not currently something I can work on without decisions being finalized.

Questions:

  • It seems that we are indeed CCing users to private reports in Bugzilla, giving them access to read and comment only in the reports where they are CCed. Are we interested in keeping this functionality in Phabricator?

Yes! This really is critical to my workflow. I can't manage fixing security bugs without this feature.

Qgil triaged this task as High priority.Oct 6 2014, 8:42 PM

We need to decide urgently whether the current status is a blocker or not for the RT and/or Bugzilla migrations.

The current status, as I understand it is: users CCed receive notifications from the moment they are added until they are removed, but they can't access the report itself.

(Assigned To users do have always access, and Author users will have it as soon as T475 is implemented -- again, if I understand the current situation correctly)

Since upstream is not supporting this feature currently, if this is a blocker then we will need to plan what it takes to develop the feature. It would be useful that @chasemp, @csteipp, @Aklapper and... (who responds officially for RT, @Dzahn?) agree on the next steps. I'm here trying to help reaching to a plan that @mmodell can implement with the priority agreed.

The title is slightly misleading, I spoke a bit with @csteipp about this issue (and hope to chat a bit more) but essentially we can do everything we want to do, the only caveat is having to add some group/user to the ACL that allows the cc person to access the task. No way should this be a blocker, at worst it's an extra step, at best it solidifies upstreams ideas on security and permissions.

Unless the idea that someone who isn't part of the security edit grouping for a task being able to add a CC user is necessary. Those who are, can currently.

A slightly better description would be "Allow non-members of the 'Editable By' grouping to add CC users who violate the 'Viewable by' policy"

Yeah, after talking to Chase more, the current functionality will work, it's just extra work.

Ideally, users who can see/edit the bug (i.e., like users cc'ed on bugzilla bugs) should be able to pull in other, unprivileged users to work on a security bug without a person from the security group being involved. As it is, they'll just have to ping someone who can add the person.

Qgil lowered the priority of this task from High to Medium.Oct 7 2014, 2:42 PM
Qgil edited projects, added Phabricator; removed Bugzilla-Migration, RT-Migration.

Ok, this task is not blocking the migrations now.

It is still unclear whether we are all on the same page. Let's split roles and permissions in the scenario desired:

  1. Authorized group (i.e. Security team) users can view and edit anything in the private tasks belonging to this group.
  2. Assigned To users become automatically members of the authorized group for the tasks they are assigned to. Therefore, they can view and edit anything as well.
  3. Authors are in the same situation than Assigned To users, for the tasks they create. Therefore, they can view and edit anything as well.
  4. CCed users... obtain view-only permissions (they can't edit, can't comment, can't CC other users)? Or should they be added to the group in the tasks they are CCed, just like Assigned To and Authors?

The thing is, as far as I can see, in Maniphest editing title & description, commenting, and adding users in CC all belong to the same "Default Edit Policy".

In T518#9002, @Qgil wrote:
  1. CCed users... obtain view-only permissions (they can't edit, can't comment, can't CC other users)? Or should they be added to the group in the tasks they are CCed, just like Assigned To and Authors?

For mediawiki security bugs, I'd prefer the CC users get edit rights, but I can live with just view and talking to the people oob. It's painful, and we loose institutional knowledge whenever we resort to email to talk about the bug while they can't comment, but we can make it work.

I think this issue is very confused, and confusing I guess, because lots of what is being described is partially true. My thoughts on this especially have become clearer in the last week or so. I will try to write a better explanation when I have a moment but I think a few ideas are particularly poignant.

  • Every ticket has it's own security policy (edit / view)
  • We created an extension that allows the appropriate settings for default security tickets at the time of filing.
  • These edit / view settings can be changed by anyone with initial Edit permissions if they choose by taking off the option for enforcing security, or we could just make that not continually apply them as is, there were requests for mitigating the problem of 'accidental reveal'. The approach was to make the settings always enforced.
  • Any group or user can be added to the edit and/or view security settings for a ticket by someone with Policy Edit permissions. That means you can add a group that is all of UI or Analytics or whoever, or all of WMF or all users (if the groupings exist in phab). They can then do whatever they need to do.
  • There is no situation where we cannot allow all CC users to do whatever the Security group wants them to be able to do per ticket.
  • But if you add a CC user who cannot edit an issue who wants to add another CC user who is not allowed via the View policy, I think that's where we enter territory we are talking about? I'm not sure what to say other than, policy ACL's are a thing and we'll have to get used to them, maybe find a way to be more liberal with initial permissions based on the type of security issue filed or something.
  • We can change the default settings for when something is filed as a security bug to be more than the security group initially, or less or whatever we want.
  • Assignee is the special case for tickets, if you assign an issue to a user they can view it _outside of the view security permission_. They can also edit the ticket _outside of the Edit permission_…for the group of metadata they can already edit globally. Upstreams idea (as far as I understand it) is that assignee is the ticket owner, and don't assign something to someone if you don't want them to be able to manage the ticket. Including adding CC’s (who are already allowed via the view policy), etc. That does not mean they are seen in the view security policy, and does not mean they seen in the edit security policy
  • Assignee is a bit confusing as far as policy interaction, not the least of which is that Assignee can ‘edit’ the ticket, but the permissions are global for saying who can adjust the policy settings. This should hopefully become much less cumbersome with T3820, which is a response by upstream to some of these ideas. The fact that editing security on any ticket at all is one finite global group should hopefully be resolved.
  • Since we have to assign who can edit policies globally, for now, we are forced to limit it to people who can edit tickets in all contexts for safety, that includes imported RT and Operations use cases.
  • View privilege users can comment / subscribe / award token / flag / create subtasks
  • Authors _can_ be excluded from a ticket they created via policy. There was some contention on how that actually worked in the past. Authors are definitely subject to policy. Mukunda and I noodled on it and he came up with an addition to the existing extension that creates a custom policy out of the box for tickets filed as Security Bugs that allows the Author via policy. We can do this kind of thing as it makes sense since policy ACLs are flexible. It partially comes down to what our definition of 'public' is, we can make numerous groups able to best triage / edit / manage 'Security Bug' tickets if that's desirable.

Bugzilla has no real concept of access control in the same terms as phabricator, that means the conversation is not apples vs. apples. There is ticket metadata and and there is policy in phabricator. I think we will benefit most by taking what we can do now and seeing how cumbersome it is from a Securities perspective, and then try to get things that will make our life easier included in the https://secure.phabricator.com/T3820 re-architecture.

Side note to understand my voicing of concerns upstream, in the normal context a user without Edit permissions cannot CC other users as that menu is hidden and locked down. But any user, including those without Edit permission AFAICT can @mention a username and get them CC'd. Thus if CC users are automatically a side-door to the view policy it renders the Edit policy impotent for controlling access to a ticket. Whether that's good or bad I guess is subjective, but I would argue it's not obvious / least-surprise behavior and so undesirable from a security perspective.

Additionally, it's very common so far for people to disable the "email me when subscribers change" in their profile since it is so noisy for many tickets, especially in our context, so keeping track of who is CC'd and who isn't from an audit perspective is a PITA. Not to mention any response would be post-information-leak.

In T518#9157, @chasemp wrote:
  • But if you add a CC user who cannot edit an issue who wants to add another CC user who is not allowed via the View policy, I think that's where we enter territory we are talking about?

Others please correct me if I'm wrong, but I believe this is not a requirement we are having from Bugzilla (I don't know RT practices enough). The current situation is that anybody CCed is allowed to access the information and be active in the ticket. If we trust them to view, we trust them to edit, and to CC others. If we have any concern in any of these aspects, then we don't CC them.

You are saying that there would be the possibility to allow view access to CCed users, but then no possibility for them to ad other people in CC. I'm not sure we want to use this view-only mode, but if we do, it make total sense not to allow them to CC others.

So maybe we don't have a problem at all?

Qgil removed Qgil as the assignee of this task.Oct 11 2014, 6:28 PM
In T518#9196, @Qgil wrote:
In T518#9157, @chasemp wrote:
  • But if you add a CC user who cannot edit an issue who wants to add another CC user who is not allowed via the View policy, I think that's where we enter territory we are talking about?

Others please correct me if I'm wrong, but I believe this is not a requirement we are having from Bugzilla

Bugzilla allows any user (no specific permissions / group membership required) who can access a ticket (e.g. product not restricted to a certain group membership) to edit the CC list of that ticket.

In T518#9196, @Qgil wrote:

The current situation is that anybody CCed is allowed to access the information and be active in the ticket. If we trust them to view, we trust them to edit, and to CC others. If we have any concern in any of these aspects, then we don't CC them.

That sounds very reasonable. Based on experience in Bugzilla I'd even call that "common sense" so far.

I'm copying Quim's comment from dup ticket 75869 so we have the discussion in one central place:

A short term solution for @JanZerebecki's situation can be found by removing the "Security or Sensitive Bug" dropdown and setting the view policy for that task manually, but this is a delicate operation that not everybody can do...

@mmodell @chasemp, this makes me think... Is the current behavior documented anywhere? As we can see with this and other cases, it differs from what we had in Bugzilla, and it is not self-evident.

What's the short-term solution here? I can't access tasks such as T70544 currently.

@MZMcBride. I just added this to the description:

In T75869#787373, @Qgil wrote:

A short term solution for @JanZerebecki's situation can be found by removing the "Security or Sensitive Bug" dropdown and setting the view policy for that task manually, but this is a delicate operation that not everybody can do...

Qgil edited projects, added Phabricator; removed Project-Management.

See the proposed solution in the description. @chasemp, I hope the summary is correct. @csteipp & co, do you agree?

In T518#809392, @Qgil wrote:

See the proposed solution in the description. @chasemp, I hope the summary is correct. @csteipp & co, do you agree?

No.

CC'ed users must be able to edit. I was ok with them only being able to view as a temporary solution, but not long term.

I understand that this is different than for ops, but for security bugs, people on the security team often are not the best people to know who should be working on bugs. I've previously asked volunteers who reported security bugs to add other volunteers who work in the same space, for example.

There is no sane way to give all users native policy access as controls to the policy interface are global. The plan as I understand it (which I don't know if it's clear above) is to sync the CC list with issue view/edit policies. i.e. if some issue is set to security-bug and you cc a user and save it they are now allowed via policy to do the same for other users.

this https://www.mediawiki.org/wiki/Phabricator/Security#Proposed_Updates_to_Process

...should probably say 'give any CC'd user view and edit policy permissions.' For which their interface would just be normal CC interaction.

AFAIK this satisfies all criteria. This would not be related to any upstream behavior, nor would it be the same for operations issues.

@mmodell is this your understanding?

this https://www.mediawiki.org/wiki/Phabricator/Security#Proposed_Updates_to_Process

...should probably say 'give any CC'd user view and edit policy permissions.' For which their interface would just be normal CC interaction.

Cool, that is exactly what I wanted! It looked like the wording explicitly did not include the edit policy, but if so, I'm happy with that.

Are there any remaining ways (i.e. that hasn't been covered in this ticket yet) that CC users would remain restricted with this plan?

Are there any remaining ways (i.e. that hasn't been covered in this ticket yet) that CC users would remain restricted with this plan?

they would not actually see the policy interface when editing a task (all interaction for non-security members would be coupled with native CC management), but otherwise not that I know of

The ACL would essentially be:

  • Author
  • Security
  • Any CC'd user
  • Anything added by Security in addition to the above

AFAIK this is what @mmodell has in mind as well.

@Krenair pointed out that non-security-members could not fully public a task. That is true since we have no way to grant them access to the policy interface.

So, in the event that an issue is solved to the extent that it should be public it would require a member of Security to unlock it for public consumption. That seems sane to me, @csteipp?

@Krenair pointed out that non-security-members could not fully public a task. That is true since we have no way to grant them access to the policy interface.

So, in the event that an issue is solved to the extent that it should be public it would require a member of Security to unlock it for public consumption. That seems sane to me, @csteipp?

Yeah, that's sane

Phabricator/Security page updated, hopefully with a correct interpretation of the discussion here. See the two FIXMEs and fix them, please.

At least for me, the actual source of confusion is this sentence:

CC users have never been restricted from viewing or accessing a task if they are allowed via view policy.

which sounds like a tautology to me. Is it relevant? Can we remove it? The point is how can CC users be added to the view policy, which is clear now.

all good to me, I included it as it has been a large point of confusion

The ACL would essentially be:

  • Author
  • Security
  • Any CC'd user
  • Anything added by Security in addition to the above

AFAIK this is what @mmodell has in mind as well.

Yeah this is what I am implementing, I will submit a change to gerrit later today for testing, assuming that it's all good, when @chasemp has time we will deploy it to production.

The FIXMEs in the wiki page are still unclear (at least to me):

  • '''FIXME''': what does "Anything added by Security in addition to the above" exactly mean? CCing projects?
  • '''FIXME''': we want this (CCed users able to CC other users) for security tasks, not for operations tasks, but is there any configuration difference between these cases or is it just a different process agreed?
In T518#824246, @Qgil wrote:

The FIXMEs in the wiki page are still unclear (at least to me):

  • '''FIXME''': what does "Anything added by Security in addition to the above" exactly mean? CCing projects?
  • '''FIXME''': we want this (CCed users able to CC other users) for security tasks, not for operations tasks, but is there any configuration difference between these cases or is it just a different process agreed?

First one, means Security can add whatever they need to the ACL outside the context of CC or Author or themselves and it will be preserved (excepting public or all users which require it to be unset as a security bug issue). It can match single users or members of a group or any other ACL constraint.

Second one, this is code within the extension that does different things entirely.

This is all post-mukunda's-new-version.

FIXMEs removed from the wiki page, although there is one last details that I still want to confirm:

Second one, this is code within the extension that does different things entirely.

This means that private tasks selected with 'security bug' or 'private issue' will get different types of policies, right?

In T518#831480, @Qgil wrote:

FIXMEs removed from the wiki page, although there is one last details that I still want to confirm:

Second one, this is code within the extension that does different things entirely.

This means that private tasks selected with 'security bug' or 'private issue' will get different types of policies, right?

yes, for example the CC policy mangling has only been proposed / implemented for 'security bug'

I just ran into this issue. Apparently there is a security issue with a bug in mobile that I cannot fix because I cannot view it. Would be good to resolve this bug! Thanks @Qgil for pointing me here!

mmodell raised the priority of this task from Medium to High.Dec 12 2014, 6:42 AM

I just ran into this issue. Apparently there is a security issue with a bug in mobile that I cannot fix because I cannot view it. Would be good to resolve this bug!

For what is worth, the user assigned to a private task is included in the policy to view and edit the task. If someone assigns this task to you, you will be able to access it just like any public task.

I think I owe everyone some kind of progress report on this one:

In the process of merging the completed changes (yesterday) with @chasemp, we ran into multiple issues that caused multiple levels of brokenness which we couldn't resolve in a 'doing it live' situation so we rolled back to the previous state and resolved to work on this further.

The new situation is as follows:

  1. Upstream phabricator has made some major breaking changes which I must adapt to in order to get the security extension working again with upstream HEAD.
  2. The Scrum extension continues to develop against HEAD so holding everything back to a historical version of phabricator is not desirable (there are other reasons against holding back the version, it's generally a good idea to stay up to date, I think we can mostly agree on that)
  3. The migration to a new version of phabricator is rather involved as the upstream has landed a lot of far-reaching changes between the end of december and beginning of january. It's going to be a big and involved update with possibility to have some significant phabricator downtime.

None of this is really happy news but it's stuff we have to deal with to stay on the bleeding edge of phabricator development. The alternative is to be left behind to watch phabricator rapidly diverge away from our obsolete fork of the code.

So the tentative plan is to go ahead with a major phabricator update as soon as I can get the code ready and coordinate with @chasemp and @Christopher around which specific revision we will be updated to.

mmodell mentioned this in Unknown Object (Diffusion Commit).Apr 8 2015, 4:33 PM

The behavior is not going to change, however soon Phabricator will visualize when a subscriber cannot access the object: https://secure.phabricator.com/rPa4cb2bb7724757a8f9dd4d68fde3bda2fd6c7895