Page MenuHomePhabricator

Create a security issue task type with additional attributes
Open, Stalled, NormalPublic

Description

Task types in phabricator give us a way to tie a specific edit form to some subset of tasks based on:

  1. The form used to create the task can be tied to a task type. In this case, the form can be sticky and any time you edit the task it will use the same form
  2. Herald rules can change a task type based on any valid herald conditions. We can use this to make any task with the Security project automatically become a security task. Once the herald rule has changed the type and any edits to the task will use the correct form with custom fields specific to security tasks.

This way members of the security team can have team-specific fields without cluttering the global task edit forms. We could also keep the 'report security bug' form simple - only subsequent edits of the form would include extra fields.

Event Timeline

mmodell triaged this task as Normal priority.Sep 12 2018, 8:35 PM
mmodell created this task.
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
mmodell renamed this task from Should security tasks be a custom type maniphest? to Should security tasks be a custom type in maniphest?.Sep 12 2018, 8:37 PM
mmodell updated the task description. (Show Details)
. @20after4 and I are talking about doing a few things:

Creating a task type 'security'
Have due date and risk rating on the advanced form for creation
Have a simple form for creation as the one that exist now
see if we like this

convert existing open tasks we want to use the new task type?

The SECURITY ISSUE task type can be created by using Form 48

mmodell moved this task from Backlog to Working on it on the User-MModell board.Oct 1 2018, 5:57 PM
mmodell closed this task as Resolved.EditedOct 22 2018, 4:48 PM

It's been done for a while.

chasemp renamed this task from Should security tasks be a custom type in maniphest? to Create a security issue task type with additional attributes.Nov 8 2018, 4:29 PM
chasemp reopened this task as Open.Nov 8 2018, 4:33 PM
chasemp added a subscriber: Anomie.

I'm reopening to keep the narrative on this subtype complete. We noticed that users who had edit perms on task were not able to modify the task. @mmodell changed some permission on the view I believe? I'm not sure if this has resolved the issue.

Here is a current use case for example:

@mmodell thoughts?

Tgr added a subscriber: Tgr.Nov 8 2018, 4:53 PM

Yeah, editing the task title and description (and adding a comment) is possible but none of the metadata fields (status, priority, assignee, tags, subscribers) are editable, except the workboard column.

@chasemp: in addition to the security policy on the task, the policy on form 48 also affects who can edit. Things are further confused by the fact that we have an acl*security_team which is separate from Security.

I would have figured the form would have a policy for changing the form itself (fields etc) and then tasks would have a policy for the forms implementation. It sounds like the task can be less permissive than the form (which allows edit for the actual form) but not more?

3 possible solutions:

  1. Add people to acl*security_team
  2. Add more groups, such as Security, to the policy on form 48
  3. Add more (visible) fields to the fallback which is form 51.
mmodell added a comment.EditedNov 8 2018, 5:01 PM

The form's permissions affect who can use the form. When editing the task, users see the first form they are allowed to use, based on the order that's set by clicking "reorder edit forms" on the left sidebar here

I would have figured the form would have a policy for changing the form itself (fields etc) and then tasks would have a policy for the forms implementation. It sounds like the task can be less permissive than the form (which allows edit for the actual form) but not more?

The policy for who can edit the forms themself is controlled by the application policy. Phabricator's forms and policies are quite confusing, somewhat more so once you add types into the mix. It does work pretty well it's just not exactly intuitive.

The form ultimately controls what fields a given user can edit. The task policy controls who can see the task at all, or edit it at all.

Ok thanks @mmodell, I want to run the use cases / workflow by the rest of acl*security_team, and I'm learning about the form/task stuff on the fly so I appreciate your patience. I'll sync up with you sooner than later.

For now I've added Security to https://phabricator.wikimedia.org/transactions/editengine/maniphest.task/view/48/ (option #2)

This should get us over the current hump (I think?) as those tasks are really only visible to Security anyway.

Yeah, editing the task title and description (and adding a comment) is possible but none of the metadata fields (status, priority, assignee, tags, subscribers) are editable, except the workboard column.

I'm wondering if this isn't just fine tbh after the addition of Security

@Tgr and @Anomie can you see if modifying those tasks as needed works out now to confirm? Thanks

Anomie added a comment.Nov 8 2018, 5:09 PM

The fields on the tasks are now showing as editable for me, thanks.

Tgr added a comment.Nov 10 2018, 5:38 PM

Yeah, editing the task title and description (and adding a comment) is possible but none of the metadata fields (status, priority, assignee, tags, subscribers) are editable, except the workboard column.

I'm wondering if this isn't just fine tbh after the addition of Security

  • Status and assignee are mainly used by the people working on a task, so Security makes sense.
  • Priority is mainly used by the team overseeing the area, so Security or even acl*security_team would be fine.
  • Tags are IMO more akin to editing the description, anyone can do useful work on those; plus, some people use personal tags to track issues relevant to them. It's also a relatively harmless field (although all the others are too) so I'd make editing it more open. (I guess the biggest concern there is an attacker who somehow gains access removing things from security to make them harder to track, but that doesn't seem like a big risk.)
  • Subscribers are the most borderline - non-security-members who are more familiar with the relevant community inviting people knowing more about an issue is a good thing, OTOH it can result (even with good intentions) in the task being visible to too many people / potentially easily accessible to an attacker who can steal wiki accounts. Not sure about that one.

Subscribers are the most borderline - non-security-members who are more familiar with the relevant community inviting people knowing more about an issue is a good thing, OTOH it can result (even with good intentions) in the task being visible to too many people / potentially easily accessible to an attacker who can steal wiki accounts. Not sure about that one.

IIRC this was mainly just for the author, relevant developers, and other people who either need to know about the bug (as they need to fix it) or already knew about it anyway. People shouldn't be adding their friends to security tasks just because like what was suggested at T207323. I don't know the extent to which it makes sense to enforce extra controls on who can subscribe people however. Is there at least a warning notice about this?

Subscribers are the most borderline - non-security-members who are more familiar with the relevant community inviting people knowing more about an issue is a good thing, OTOH it can result (even with good intentions) in the task being visible to too many people / potentially easily accessible to an attacker who can steal wiki accounts. Not sure about that one.

IIRC this was mainly just for the author, relevant developers, and other people who either need to know about the bug (as they need to fix it) or already knew about it anyway. People shouldn't be adding their friends to security tasks just because like what was suggested at T207323. I don't know the extent to which it makes sense to enforce extra controls on who can subscribe people however. Is there at least a warning notice about this?

(response for you both and just to expound a bit generally)

At the moment all of this conversation only relates to tasks created using (or converted to) the subtask type in discussion. We have had an issue with chains of CC users (where one person is added who adds another and so on) for which we don't have a reason or idea of why they are currently CC'd on a task. This is further compounded by our desire to keep (as much as possible) security tasks limited to those with 2fa on their accounts in phab.

This is part of what I'm thinking about here definitely. Recently this caused confusion and worry with a few tasks where they were being overloaded across specific issues (one problem) and that meant the spread on users who could access was a bit out of control (separate problem for sure). 43 CC'd users in a case where it became very problematic to contain our immediate responses to issues. Quite a few no one I talked to knew why many/most of those were CC'd and even in a few cases the CC'd users themselves had no idea :)

The new subtype is being piloted for a few reasons that all boil down to needing more and different fields and subtypes being the native mechanism to make that happen. In parallel it potentially provides us with some new and better elements of defining appropriate access to security tasks. But part of the pilot (so to speak) is figuring out how forms and views are best utilized here. Before anything goes much further than the handful of tasks at play now it is my intention define expectations better, esp for things that are changing.

mmodell reassigned this task from mmodell to chasemp.Jan 15 2019, 4:16 PM

Assigning this to chase since my part is mostly done. Please feel free to assign it back to me or create a subtask assigned to me if there is something more I can do to help.

Alroilim closed this task as Declined.Feb 2 2019, 7:16 PM
Alroilim removed chasemp as the assignee of this task.
Alroilim lowered the priority of this task from Normal to Lowest.
Alroilim set Due Date to Feb 1 2019, 9:00 PM.
Alroilim updated the task description. (Show Details)
Alroilim removed subscribers: Krenair, Tgr, Anomie and 7 others.
Restricted Application changed the subtype of this task from "Task" to "Deadline". · View Herald TranscriptFeb 2 2019, 7:16 PM
Gopavasanth reopened this task as Open.Feb 2 2019, 7:44 PM
Gopavasanth assigned this task to chasemp.
Gopavasanth added a subscriber: Tgr.
Aklapper raised the priority of this task from Lowest to Normal.Feb 23 2019, 6:19 AM
Aklapper removed Due Date.
Aklapper updated the task description. (Show Details)
Aklapper added subscribers: Krenair, Anomie, sbassett and 6 others.
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptFeb 23 2019, 6:19 AM

Should we resolve this task? It seems like the new type is being used successfully. @chasemp: What do you think?

Should we resolve this task? It seems like the new type is being used successfully. @chasemp: What do you think?

I talked to @johnben about this yesterday and we want to make this the default for security tasks but I know that will require some massaging and working out a few kinks with you. Should I make that a new task?

Anomie removed a subscriber: Anomie.May 16 2019, 5:36 PM

@chasemp: this task is sufficient I think. What kinks need working out? Switching the default is fairly straightforward, just let me know what else needs massaging.

mmodell changed the task status from Open to Stalled.Jul 30 2019, 5:57 PM
mmodell moved this task from Working on it to Blocked or Stalled on the User-MModell board.
mmodell moved this task from Blocked or Stalled to Done on the User-MModell board.Wed, Sep 4, 11:56 PM