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

[Ryan J Ollos]

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.

[Ryan J Ollos]

For now, a simple change will probably be sufficient:

trac/ticket/web_ui.py

@@ -262 +262 @@
 
         def produce_event(values, status, fields, comment, cid):
             id, ts, author, type, summary, description, component = values
-            ticket = ticket_realm(id=id)
+            ticket = ticket_realm(id=id, version=cid)
             if 'TICKET_VIEW' not in req.perm(ticket):
                 return None
             resolution = fields.get('resolution')

[Ryan J Ollos]

#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')

[Ryan J Ollos]

​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

@@ -181 +181 @@
         :since: 1.3.2
         """
 
+    def apply_side_effects(req, ticket):
+        """Apply side effects after ticket has been changed.
+
+        This should not be used to make changes to the ticket,
+        rather to apply changes outside of the `Ticket` class.
+
+        :since: 1.4.2
+        """
+
 
 class IMilestoneChangeListener(Interface):
     """Extension point interface for components that require notification

trac/ticket/web_ui.py

@@ -636 +636 @@
             req.perm(ticket.resource).require('TICKET_EDIT_COMMENT')
             if self._validate_ticket(req, ticket):
                 ticket.modify_comment(change['date'], req.authname, comment)
+                self._apply_side_effects(req, ticket)
                 req.redirect(req.href.ticket(ticket.id) +
                              '#comment:%d' % cnum)
             else:
@@ -1362 +1363 @@
                            controller.__class__.__name__)
             controller.apply_action_side_effects(req, ticket, action)
 
+        self._apply_side_effects(req, ticket)
+
         # Notify. Use author=req.authname rather than ticket['reporter']
         # because ticket may be created on behalf of another user (#11949).
         event = TicketChangeEvent('created', ticket, ticket['time'],
@@ -1429 +1432 @@
                            controller.__class__.__name__)
             controller.apply_action_side_effects(req, ticket, action)
 
+        self._apply_side_effects(req, ticket)
+
         req.redirect(req.href.ticket(ticket.id) + fragment)
 
     def get_ticket_changes(self, req, ticket, selected_action):
@@ -1480 +1485 @@
         for key in field_changes:
             ticket[key] = field_changes[key]['new']
 
+    def _apply_side_effects(self, req, ticket):
+        for manipulator in self.ticket_manipulators:
+            if hasattr(manipulator, 'apply_side_effects'):
+                manipulator.apply_side_effects(req, ticket)
+
     def _query_link(self, req, name, value, text=None, class_=None):
         """Return a link to /query with the appropriate name and value"""
         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?

[Ryan J Ollos]

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

@@ -320 +320 @@
         self._old = {}
 
         for listener in TicketSystem(self.env).change_listeners:
-            listener.ticket_created(self)
+            if hasattr(listener, 'ticket_created'):
+                listener.ticket_created(self)
 
         return self.id

[Ryan J Ollos]

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.

[anonymous]

#12414 reported similar problems.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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:

0. Create ticket: version=0
1. Append comment: version=1
2. Edit comment: version=1
3. 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.

[Ryan J Ollos]

Milestone renamed