Edgewall Software
Modify

Opened 10 years ago

Closed 3 years ago

#8778 closed enhancement (fixed)

Permission that uniquely controls assignment of tickets to milestones

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: ticket system Version: 0.11.5
Severity: normal Keywords: permission consider
Cc: Branch:
Release Notes:

The TICKET_CHG_MILESTONE action is used internally to grant/deny changing the ticket milestone. DefaultPermissionPolicy grants the action when the user has MILESTONE_VIEW for the milestone. See CookBook/PermissionPolicies#RestrictChangingTicketMilestone for details on implementing fine-grained access control for changing the ticket milestone.

API Changes:

Description

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.

Attachments (0)

Change History (17)

in reply to:  description comment:1 by Ryan Ollos <ryano@…>, 10 years ago

Summary: Permission to uniquely control assignment of tickets to milestonesPermission that uniquely controls assignment of tickets to milestones

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.

comment:2 by Christian Boos, 10 years ago

Keywords: permission consider added
Milestone: next-major-0.1X

comment:3 by Ryan J Ollos <ryano@…>, 8 years ago

Discussion of this issue on the mailing list.

comment:4 by Ryan J Ollos <ryano@…>, 7 years ago

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.

comment:5 by Ryan J Ollos, 5 years ago

Reporter: changed from ryano@… to Ryan J Ollos

comment:6 by anonymous, 5 years ago

Could this be fixed?

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

grouppermissions
anonymousCHANGESET_VIEW MILESTONE_VIEW REPORT_SQL_VIEW REPORT_VIEW ROADMAP_VIEW SEARCH_VIEW TICKET_VIEW TIMELINE_VIEW WIKI_VIEW
authenticatedTICKET_APPEND TICKET_CREATE TICKET_EDIT_CC VOTE_MODIFY VOTE_VIEW

comment:7 by Ryan J Ollos, 5 years ago

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.

comment:8 by Ryan J Ollos, 5 years ago

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.
  1. 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.
  1. 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).

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:9 by Christian Boos, 5 years ago

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.

comment:10 by Ryan J Ollos, 3 years ago

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

comment:11 by Ryan J Ollos, 3 years ago

Milestone: next-major-releases1.2.1
Owner: set to Ryan J Ollos
Status: newassigned

comment:12 by Ryan J Ollos, 3 years ago

Milestone: 1.2.11.2.2

comment:13 by Ryan J Ollos, 3 years ago

Milestone: 1.2.2
Resolution: duplicate
Status: assignedclosed

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

comment:14 by Ryan J Ollos, 3 years ago

Milestone: 1.3.2
Resolution: duplicate
Status: closedreopened

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].

TODO Edit 1.3/TracFineGrainedPermissions.

Version 1, edited 3 years ago by Ryan J Ollos (previous) (next) (diff)

comment:15 by Ryan J Ollos, 3 years ago

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.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:16 by Ryan J Ollos, 3 years ago

Status: reopenedassigned

comment:17 by Ryan J Ollos, 3 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in r15817.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.