Page MenuHomePhabricator

Authors of private tasks should be able to access them
Closed, ResolvedPublic

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.

I just filed T474 as a private task, and now I can't access it anymore.

See also: T518: Users CCed in private tasks should be able to access them

Event Timeline

Qgil created this task.Sep 25 2014, 9:23 AM
Qgil assigned this task to mmodell.
Qgil raised the priority of this task from to Needs Triage.
Qgil updated the task description. (Show Details)
Qgil added a project: Bugzilla-Migration.
Qgil changed Security from none to None.
Qgil added subscribers: Qgil, mmodell.
Qgil added a comment.Sep 25 2014, 9:38 AM

In fact the dialog you get says:

Access Denied: Task Restricted Task
You do not have permission to view this object.
Users with the "Can View" capability:
Members of the project "security" can take this action.
The owner of a task can always view and edit it.

Who is the "owner", the Author or the Assigned?

Qgil added a comment.Sep 25 2014, 9:42 AM

Alright, I can see the private task if I'm Assigned To it, but not if I'm the author.

Qgil closed subtask Restricted Task as Invalid.Sep 25 2014, 9:42 AM
Qgil reopened subtask Restricted Task as Open.
Qgil closed subtask Restricted Task as Resolved.Sep 25 2014, 9:45 AM
Qgil changed the status of subtask Restricted Task from Resolved to Invalid.
Qgil renamed this task from Authors of private tasks should be able to access them to Authors of private tasks and anyone CCed should be able to access them .Sep 26 2014, 6:32 AM

The feature in Bugzilla is "the reporter and anyone CC'ed can also see the bug", so I added "and anyone CCed" to the subject.

The only way I know to fix this is to create a custom access list for each task that includes the reporter and the security project in the list. That isn't really ideal and it isn't really trivial either.

And for CCs we would have to modify the custom list each time ccs get modified ...

Well, this is a bit surprising, considering that it was a requirement since the beginning. Perhaps we should have needed to be more formal and explicit discussing the requirements? Anyway, let's see how we can solve this now.

Let's start with the Bugzilla use case, which is probably more narrowed and less frequent that the RT case.

Is "a custom access list" the same thing as the members of a custom project? Can projects and its members be created programmatically? If we focus on authors only, would it be helpful to create/reuse on the fly project "Security and Qgil" when Qgil creates a private task? If we take into account CCed as well, would t be helpful to create project "Task 1234" including Security + author + CCed as members?

@Aklapper, how often would you say a report must go private in Bugzilla? In a worst case scenario, we would start with the feature as is now, creating those projects manually for every new security task created.

RT most probably can't work this way at all, though.

For the sake of not compromising our Bugzilla migration even more, I'm fine leaving the security feature as is (that is, no access for reporters and CCed) as long as it is documented as a known issue and we make it priority number one after the Bugzilla migration.

I wonder whether this feature could be of any help here: T52: Phabricator should let you interact with external (non-Phabricator) users via email. Maybe we can add the email addresses of the reporter and the CCed users manually, while we find a better solution altogether? Maybe this is the deal or break for the RT-Migration, because I really don't see that happening without this feature properly implemented. Then again, Chase/Ops will decide whether this is a blocker for the RT migration or not.

Qgil triaged this task as Normal priority.Sep 29 2014, 1:43 PM
In T475#18, @Qgil wrote:

@Aklapper, how often would you say a report must go private in Bugzilla? In a worst case scenario, we would start with the feature as is now, creating those projects manually for every new security task created.

https://bugzilla.wikimedia.org/weekly-bug-summary.cgi?tops=20&days=90 states that in the last 90 days, 35 tickets have been filed under or moved to the "Security" product in Bugzilla.

RT most probably can't work this way at all, though.

I'm afraid so too.

chasemp added a comment.EditedSep 29 2014, 4:42 PM

Each ticket has view/edit permissions. I believe we tested this prior to developing the extension and there were two "punchthrough" values. If you were the author of the ticket, you could view no matter what. If you were assigned the issue, you could view no matter what. It appears that the author behavior has changed, or it wasn't clear at the time. I think the former. I asked upstream if the behavior that exists now is the one intended going forward, and they have responded that it is.

So assignee will be able to view an issue no matter what. Author will not.

@mmodell can you peek at creating a 'custom policy' out of the author and security groups in the extension?

I see now if you establish one manually for the same purpose it creates an policy phid like "PHID-PLCY-4mtjah4sxa6tcy3vqpvn" and then that is referenced in the maniphest_task table. We should be able to do create this programatically and assign in the same way we assign the simiple group now.

CC users were never meant to always have access in phabricator on its face as far as I've been aware. The reason an ACL exists to establish the security params independent of the metatdata on the ticket. If you you want to CC someone on a ticket they have to be allowed on the ACL. This is appropriate behavior for this type of system. if you don't have permission to modify the issue ACL then you shouldn't have permission to add a CC user.

just a note on the custom policies. They are stored in the policy database and not under maniphests specifically, which makes sense.

example:

1PHID-PLCY-4mtjah4sxa6tcy3vqpvn[{"action":"allow","rule":"PhabricatorPolicyRuleUsers","value":["PHID-USER-hbpzvgtfc63m7q72psil","PHID-USER-2djegxdjrhkvqfpdry4w"]}]deny14120076581412007658
This comment was removed by chasemp.

@chasemp yes I think we can create the custom access policy programmatically.

Ok here is a revision that adds custom policy for Author+Project to the security plugin: https://gerrit.wikimedia.org/r/#/c/163753/

I've tested this change locally on my test phabricator. It works as expected, giving a custom policy that looks like the following:

// | Allow   |  Users               | <ticket author>    |
// |---------|----------------------|--------------------|
// | Allow   |  Members of Project  | <Security/Ops/etc> |
// |---------|----------------------|--------------------|
// |         |  If no rules match:  | Deny               |
greg added a subscriber: greg.Sep 30 2014, 5:30 PM

Just for my own edification: is any of this stuff that Evan will want?

@greg....don't understand the question. The settings mukunda and I are talking about are related to our own internal extension.

greg added a comment.Sep 30 2014, 6:38 PM

Ah, wasn't sure it was an "internal extension" that we maintained. Sorry, I
blame my cold that I'm on day 4 of now (still going strong).

@greg yeah this is definitely not upstreamable. We can make some effort to document it and put it up on github in a form that others might find usable, it certainly could be useful to someone.

Qgil renamed this task from Authors of private tasks and anyone CCed should be able to access them to Authors of private tasks should be able to access them .Sep 30 2014, 9:31 PM
Qgil updated the task description. (Show Details)

I'd close this one but it's still awaiting review: https://gerrit.wikimedia.org/r/#/c/163753/

Qgil moved this task from Backlog to Doing on the Bugzilla-Migration board.
mmodell closed this task as Resolved.Oct 21 2014, 8:52 PM
mmodell removed a project: Patch-For-Review.
Qgil added a comment.Oct 21 2014, 9:57 PM

Ok, I guess it is not deployed yet?

I can't access T785

chasemp reopened this task as Open.Oct 21 2014, 10:02 PM

I think error I get in a testing instance is an issue:

failed to create 62291 (ERR-CONDUIT-CORE: Undefined variable: transactions)failed to create 62315 (ERR-CONDUIT-CORE: Undefined variable: transactions)
mmodell closed this task as Resolved.Oct 23 2014, 5:06 AM

ok @chasemp this error is also fixed now in https://gerrit.wikimedia.org/r/#/c/168141/

@Qgil: this should be fully resolved now.

Qgil added a comment.Oct 25 2014, 7:18 AM

Yes, now I can access {T477}. Thank you!