Permission that uniquely controls assignment of tickets to milestones

[Ryan J Ollos]

I have found that the MILESTONE_VIEW permission allows a user to change the milestone with which a ticket is associated. That is, it causes the milestone drop-down list to be populated with the milestones and allows the user to change which milestone a ticket is associated with.

It seems incorrect for a *VIEW permission to grant a change action. Also, it seems desirable to allow a user to view milestones, but not change the milestone assignment for a particular ticket. In my situation, the configuration control board (lead software developers) are in charge of assigning tickets to milestones, and it would be better if all users with MILESTONE_VIEW permissions did not have this level of control.

Therefore, it would be nice to have a separate permissions to control the assignment of tickets to milestones. This permission should be named either TICKET_ASSIGN_MILESTONE or MILESTONE_ASSIGN_TICKET. I suspect the former is a better fit for the permissions naming schema (See: TracDev/Proposals/EvenFinerGrainedPermissions).

Note, this permission should be different than the MILESTONE_MODIFY, which controls modifying the description, due date, and other milestone data.

[Ryan Ollos <ryano@…>]

Replying to ryano@…:

I have found that the MILESTONE_VIEW permission allows a user to change the milestone with which a ticket is associated. That is, it causes the milestone drop-down list to be populated with the milestones and allows the user to change which milestone a ticket is associated with.

I attempted to capture the existing behavior in the TracPermissions documentation with this change.

[Ryan J Ollos <ryano@…>]

Discussion of this issue on the ​mailing list.

[Ryan J Ollos <ryano@…>]

Now that I understand this better, I see the a user must have TICKET_MODIFY + MILESTONE_VIEW in order to set the milestone for a ticket.

[anonymous]

Could this be fixed?

We want users to be able to see the milestone, but not assign tickets to it..

group	permissions
anonymous	CHANGESET_VIEW MILESTONE_VIEW REPORT_SQL_VIEW REPORT_VIEW ROADMAP_VIEW SEARCH_VIEW TICKET_VIEW TIMELINE_VIEW WIKI_VIEW
authenticated	TICKET_APPEND TICKET_CREATE TICKET_EDIT_CC VOTE_MODIFY VOTE_VIEW

[Ryan J Ollos]

I don't think we should add another permission, but it might be possible using TracFineGrainedPermissions. I might post a recipe here in a while if the TracFineGrainedPermissions approach works well.

[Ryan J Ollos]

I considered several possible solutions.

1. In comment:11:ticket:10984 I suggested we might add modify_perm and view_perm attributes to TracTicketsCustomFields. If we also allow attributes to be added to existing built-in fields in this section, the following could be possible:

   # Create a custom permission
   [extra-permissions]
   _perms = TICKET_CHGPROP_MILESTONE
   
   # Require the custom permission for changing the milestone field
   [ticket-custom]
   milestone.modify_perm = TICKET_CHGPROP_MILESTONE
   

   With this approach, we could avoid bloat in the default permissions while making the fine-grained control possible. We are just adding a TICKET_CHGPROP permission that only applies to the Milestone field.

2. Thinking about the solution in (1) leads to me to consider whether we can enable a fine-grained permissions approach using TICKET_CHGPROP. Working from the branch created in #10984, the following patch,

   --- a/trac/ticket/web_ui.py
   +++ b/trac/ticket/web_ui.py
   @@@ -1485,8 -1479,7 +1485,9 @@@ class TicketModule(Component)
                     milestones = [Milestone(self.env, opt)
                                   for opt in field['options']]
                     milestones = [m for m in milestones
   -                               if 'MILESTONE_VIEW' in req.perm(m.resource)]
   +                               if 'TICKET_CHGPROP' in req.perm(m.resource)
   +                               and 'MILESTONE_VIEW' in req.perm(m.resource)]
                     field['editable'] = milestones != []
                     groups = group_milestones(milestones, ticket.exists
                         and 'TICKET_ADMIN' in req.perm(ticket.resource))
                     field['options'] = []
   

   enables the following TracFineGrainedPermissions configuration to be used:

   [milestone:*]
   anonymous = !TICKET_CHGPROP
   

   The anonymous user with MILESTONE_VIEW will be able to see the ticket's milestone in the ticket properties box and navigate to that milestone, however the anonymous user won't be able to change the milestone.

3. I'm not sure the solution in (2) is correct because usually we check permissions for the resource that is being acted upon. When changing the milestone we aren't acting on the milestone resource but acting on the ticket resource. That would suggest that we should instead check MILESTONE_VIEW on the ticket resource.

   diff --cc trac/ticket/web_ui.py
   index 66767c0,d540d0c..0000000
   --- a/trac/ticket/web_ui.py
   +++ b/trac/ticket/web_ui.py
   @@@ -1485,8 -1479,7 +1485,9 @@@ class TicketModule(Component)
                     milestones = [Milestone(self.env, opt)
                                   for opt in field['options']]
                     milestones = [m for m in milestones
   -                               if 'MILESTONE_VIEW' in req.perm(m.resource)]
   +                               if 'MILESTONE_VIEW' in req.perm(ticket.resource)
   +                               and 'MILESTONE_VIEW' in req.perm(m.resource)]
                     field['editable'] = milestones != []
                     groups = group_milestones(milestones, ticket.exists
                         and 'TICKET_ADMIN' in req.perm(ticket.resource))
                     field['options'] = []
   

   In this scenario revoking MILESTONE_VIEW for the ticket resource prevents the user from assigning a ticket to a milestone. One limitation of this approach in contrast to (2) is that it isn't possible to allow the user to view all milestones but only assign tickets to specified milestone.

It's also worth considering comment:2:ticket:11707, and whether a specific action could be defined for changing the milestone property in an idealized permissions API, e.g.: 'change_milestone' in perm(ticket_resource).

[Christian Boos]

I'm really not sure about the above. Sure, perhaps the current permission model can be tweaked into expressing what is needed here, but it feels wrong to me to use a permission REALM_… on an instance from another realm, be it TICKET_CHGPROP on a milestone or MILESTONE_VIEW on a ticket. This is mostly because I'd really like at some point that we get rid of this legacy way of naming the actions so that we could just use a predicate on an object ('view' in perm(milestone)).

So yes, according to the ideas from the idealized permission API, I think this specific need could be solved by being able to refine more precisely the object, i.e. 'edit' in perm(ticket.field('milestone')) with a new .field method enabling to qualify a particular facet of the resource, as this can't be consider as a real child resource, like perhaps a comment could.

Note that the TracFineGrainedPermissions could be extended quite naturally to support this:

[ticket:*::milestone]
anonymous = !edit

Sorry, no concrete course of action proposed, and you seem to have a working solution now, so don't consider my comment as a negative, we could well go with the pragmatic solution you proposed first, then when the time is ripe go with something along the lines described here.

[Ryan J Ollos]

I agree that this isn't an idea solution, and I'm interested in investigating the finer grained permissions model. In the meantime I was on the fence about just closing this ticket, or implementing the less than ideal solution. I'm leaning towards the latter. With changes in log:rjollos.git:t8778_milestone_edit_permission, the following permission policy could be added to the CookBook:

# -*- coding: utf-8 -*-
#
# Copyright (C) 2017 Edgewall Software
# All rights reserved.
#

from trac.core import Component, implements
from trac.perm import IPermissionPolicy, IPermissionRequestor


class TicketMilestonePolicy(Component):

    implements(IPermissionPolicy, IPermissionRequestor)

    def get_permission_actions(self):
        perm = 'TICKET_MODIFY_MILESTONE'
        return [perm, ('MILESTONE_MODIFY', [perm])]

    def check_permission(self, action, username, resource, perm):
        if action == 'MILESTONE_VIEW' and \
                resource is not None and \
                resource.realm == 'ticket' and \
                resource.id is not None:
            return 'TICKET_MODIFY_MILESTONE' in perm

[Ryan J Ollos]

After further consideration, I think it's better to defer this change and focus on #8653.

[Ryan J Ollos]

I considered a different way to handle this after #12719. Like WIKI_CHANGE_READONLY used in #12719, adding TICKET_EDIT_MILESTONE moves the logic to DefaultTicketPolicy. By not defining the action through an IPermissionRequestor we avoid cluttering the user interface with fine-grained permissions.

Users wishing to restrict the ability to set the ticket milestone can define TICKET_EDIT_MILESTONE through ExtraPermissionsProvider, or create a replacement for DefaultTicketPolicy.

Proposed changes in [a4a2fdf2/rjollos.git].

DONE Edit 1.3/TracFineGrainedPermissions.

[Ryan J Ollos]

TICKET_CHG_MILESTONE appears to be more consistent with existing permission names (TICKET_CHGPROP), as EDIT is associated with text fields.

Added CookBook/PermissionPolicies@9.

[Ryan J Ollos]

Committed to trunk in r15817.

[Ryan J Ollos]

Defect discovered, which will be fixed in #13316.