Edgewall Software
Modify

Opened 4 years ago

Last modified 3 years ago

#13282 new defect

Ticket timeline events can't be filtered using a permission policy

Reported by: Ryan J Ollos Owned by:
Priority: normal Milestone: 1.7.1
Component: ticket system Version:
Severity: normal Keywords:
Cc: virginia.alcaide@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

I'm trying to implement a solution for private ticket comments (th:#13776) and finding that the permission checking for timeline events is too coarse to implement a permissions policy that filters by resource realm comment and resource id cnum.

Attachments (2)

Screen Shot 2020-05-06 at 13.11.02.jpg (26.1 KB ) - added by Ryan J Ollos 4 years ago.
Screen Shot 2020-05-06 at 13.11.43.jpg (27.7 KB ) - added by Ryan J Ollos 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Ryan J Ollos, 4 years ago

For now, a simple change will probably be sufficient:

  • trac/ticket/web_ui.py

    diff --git a/trac/ticket/web_ui.py b/trac/ticket/web_ui.py
    index 53b70d454..0ae66662c 100644
    a b class TicketModule(Component):  
    262262
    263263        def produce_event(values, status, fields, comment, cid):
    264264            id, ts, author, type, summary, description, component = values
    265             ticket = ticket_realm(id=id)
     265            ticket = ticket_realm(id=id, version=cid)
    266266            if 'TICKET_VIEW' not in req.perm(ticket):
    267267                return None
    268268            resolution = fields.get('resolution')

comment:2 by Ryan J Ollos, 4 years ago

#12719 created a comment resource and performed permission checks on that resource. Looking at this again, it appears we can use version for the permission check on the ticket resource in place of the comment resource. For example:

-            comment_resource = \
-                Resource('comment', cnum, parent=ticket.resource)
-            req.perm(comment_resource).require('TICKET_EDIT_COMMENT')
+            req.perm(ticket.resource(version=cnum)).require('TICKET_EDIT_COMMENT')

comment:3 by Ryan J Ollos, 4 years ago

PrivateCommentsPlugin adds a checkbox to the comment form. It saves the private comment state to a table but needs the ticket changes to be applied first so that ticket['changetime'] can be saved in the private_comments table.

I've tried implementing this numerous ways, using IRequestFilter and ITicketActionController. I've tried using ITicketManipulator in combination with redirect listener, which I think probably works okay.

Another way to make this work is a combination of ITicketManipulator and ITicketChangeListener. The former grabs the value from the request and attaches is to the Ticket object. The latter saves the value to the database.

    def validate_ticket(self, req, ticket):
        if 'private_comment' in req.args:
            ticket.private_comment = req.args['private_comment']
        return []

    def ticket_changed(self, ticket, comment, author, old_values):
        if hasattr(ticket, 'private_comment') and ticket.private_comment:
            self._insert_private_comment(ticket, ticket['changetime'])

    def ticket_comment_modified(self, ticket, cdate, author, comment, old_comment):
        if hasattr(ticket, 'private_comment') and ticket.private_comment:
            self._insert_private_comment(ticket, cdate)
        else:
            self._delete_private_comment(ticket, cdate)

I think this has been discussed before, the issues around change listeners not having access to the request. I need to do a deep dive on some of those tickets (#11148).

Another way to accomplish this would be a method added to ITicketManipulator:

  • trac/ticket/api.py

    diff --git a/trac/ticket/api.py b/trac/ticket/api.py
    index 28e3dfaf5..c9ce57512 100644
    a b class ITicketManipulator(Interface):  
    181181        :since: 1.3.2
    182182        """
    183183
     184    def apply_side_effects(req, ticket):
     185        """Apply side effects after ticket has been changed.
     186
     187        This should not be used to make changes to the ticket,
     188        rather to apply changes outside of the `Ticket` class.
     189
     190        :since: 1.4.2
     191        """
     192
    184193
    185194class IMilestoneChangeListener(Interface):
    186195    """Extension point interface for components that require notification
  • trac/ticket/web_ui.py

    diff --git a/trac/ticket/web_ui.py b/trac/ticket/web_ui.py
    index 86067a46e..724198f0d 100644
    a b class TicketModule(Component):  
    636636            req.perm(ticket.resource).require('TICKET_EDIT_COMMENT')
    637637            if self._validate_ticket(req, ticket):
    638638                ticket.modify_comment(change['date'], req.authname, comment)
     639                self._apply_side_effects(req, ticket)
    639640                req.redirect(req.href.ticket(ticket.id) +
    640641                             '#comment:%d' % cnum)
    641642            else:
    class TicketModule(Component):  
    13621363                           controller.__class__.__name__)
    13631364            controller.apply_action_side_effects(req, ticket, action)
    13641365
     1366        self._apply_side_effects(req, ticket)
     1367
    13651368        # Notify. Use author=req.authname rather than ticket['reporter']
    13661369        # because ticket may be created on behalf of another user (#11949).
    13671370        event = TicketChangeEvent('created', ticket, ticket['time'],
    class TicketModule(Component):  
    14291432                           controller.__class__.__name__)
    14301433            controller.apply_action_side_effects(req, ticket, action)
    14311434
     1435        self._apply_side_effects(req, ticket)
     1436
    14321437        req.redirect(req.href.ticket(ticket.id) + fragment)
    14331438
    14341439    def get_ticket_changes(self, req, ticket, selected_action):
    class TicketModule(Component):  
    14801485        for key in field_changes:
    14811486            ticket[key] = field_changes[key]['new']
    14821487
     1488    def _apply_side_effects(self, req, ticket):
     1489        for manipulator in self.ticket_manipulators:
     1490            if hasattr(manipulator, 'apply_side_effects'):
     1491                manipulator.apply_side_effects(req, ticket)
     1492
    14831493    def _query_link(self, req, name, value, text=None, class_=None):
    14841494        """Return a link to /query with the appropriate name and value"""
    14851495        from trac.ticket.query import QueryModule
    def apply_side_effects(self, req, ticket):
        if 'id' in req.args:  # Only if ticket exists
            cdate = None
            if 'cnum_edit' in req.args:
                change = ticket.get_change(req.args.getint('cnum_edit'))
                cdate = change['date']
            if 'private_comment' in req.args:
                self._insert_private_comment(ticket, cdate)
            else:
                self._delete_private_comment(ticket, cdate)

With this change, ITicketManipulator has methods that get called before (validate_ticket/validate_comment) and after (apply_side_effects, or apply_ticket_side_effects to avoid name clashes) ticket changes.

Thoughts?

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

comment:4 by Ryan J Ollos, 4 years ago

I was thinking about adding an attribute check before each interface method is called. This way plugins can implement just the methods of the interface they intend to use and we don't have code like:

    def ticket_created(self):
        pass

Example:

  • trac/ticket/model.py

    diff --git a/trac/ticket/model.py b/trac/ticket/model.py
    index f4b3961e8..6d7a7fcd9 100644
    a b class Ticket(object):  
    320320        self._old = {}
    321321
    322322        for listener in TicketSystem(self.env).change_listeners:
    323             listener.ticket_created(self)
     323            if hasattr(listener, 'ticket_created'):
     324                listener.ticket_created(self)
    324325
    325326        return self.id

comment:5 by Ryan J Ollos, 4 years ago

Summary: Timeline events can't be filtered using a permission policyTicket timeline events can't be filtered using a permission policy

by Ryan J Ollos, 4 years ago

by Ryan J Ollos, 4 years ago

comment:6 by Ryan J Ollos, 4 years ago

My intent here was just to fix-up a plugin, but now I'm looking at this as a case revealing some defects and shortcoming in the architecture. The plugin does some basic things:

  1. Adds a Private Comment checkbox to the comment and edit comment forms.
  2. Creates a table private_comments for saving the resource id and version of private comments.
  3. Defines TICKET_VIEW_PRIVATE_COMMENT via IPermissionRequestor.
  4. Implements a permission policy via IPermissionPolicy that checks TICKET_VIEW_PRIVATE_COMMENT and queries private_comments.
  5. Specializes EmailDistributor to filter recipients per the permissions policy.
  6. Adds checkbox to the ticket form for setting a comment as private, shown below.

Summarizing findings:

  • Numerous changes are needed to Trac so that ticket comment visibility can be restricted based on resource id and version.
  • Ticket changetime/cnum are not known until ticket.save_changes is called (the cnum contains the resource version). ITicketManipulator methods are only called before changes are saved (thus my proposal to add a method that is called after saving changes). RequestFilter.post_process_request isn't called for the POST request because there is a redirect. It's possible that a redirect listener could be used here, I haven't revisited that solution, but it would probably be complex.
  • ITicketChangeListener might otherwise be used to save the ticket id and version in a private_comments table, but there are two problems:
    • It doesn't have access to the req object. This can be overcome by taking the value from the request in ITicketManipulator.validate_ticket or IRequestFilter.pre_process_request and adding it to the Ticket object.
    • ITicketChangeListener.ticket_comment_modified isn't called if a ticket comment edit is saved without changing the comment, so a comment edit that just changes the Private Comment checkbox state isn't saved.
Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:7 by anonymous, 4 years ago

#12414 reported similar problems.

in reply to:  7 comment:8 by Ryan J Ollos, 4 years ago

Replying to anonymous:

#12414 reported similar problems.

Thanks, unsure what that reporter is aiming to accomplish. It's likely either the same issue, or could be accomplished in another way with the current API.

comment:9 by Ryan J Ollos, 4 years ago

Considering comment:9:ticket:12414, whether ticket version is the same cnum.

Even if there were attachments or comment edits (two ways to change the ticket without incrementing the cnum)?

On current 1.4-stable, it appears they are the same. Attachments are not "permanent" and neither attachment changes nor comment edits change the version.

For the standpoint of a permissions policy, it would probably be better if every change to a ticket incremented the version. Appending ?version=N would the give a snapshot of the ticket. Consider what actually happens:

  1. Create ticket: version=0
  2. Append comment: version=1
  3. Edit comment: version=1
  4. Add attachment: version=1

When viewing a ticket version, for ?version=1 I'd expect to see state (1), and for ?version=3 I'd expect to see the state (3).

Thank you for raising that point, it's helping with investigating and my understanding everything.

comment:10 by Ryan J Ollos, 4 years ago

Milestone: 1.4.21.4.3

comment:11 by Virginia <virginia.alcaide@…>, 4 years ago

Cc: virginia.alcaide@… added

comment:12 by Ryan J Ollos, 3 years ago

Milestone: 1.4.31.5.3

comment:13 by Ryan J Ollos, 3 years ago

Milestone: 1.5.31.7

comment:14 by Ryan J Ollos, 3 years ago

Milestone: 1.71.7.1

Milestone renamed

comment:15 by Ryan J Ollos, 3 years ago

Milestone: 1.7.1
Owner: Ryan J Ollos removed
Status: assignednew

comment:16 by Ryan J Ollos, 3 years ago

Milestone: 1.7.1

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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