Private ticket permission users can get ticket counts that include tickets they're not allowed to view

[jevans]

A user with private tickets permissions can still query how many tickets meet criteria even if they can't see the tickets listed or view them.

For instance they can type in query?status=!closed&priority=critical to get a count of how many critical defects are open.

I originally wrote this ticket on the PrivateTicketsPlugin (​#3674) but heard that it's a problem in Trac core.

[osimons]

Looked into this one, and the Query module and related features such as the TicketQuery macro do not check for permissions. The only place where permission checks do happen is when listing individual tickets in the query_results.html template.

That means:

• As you found, the total does not get adjusted if individual tickets are not permitted to display due to some custom permission
• A format=count for the macro will just fetch the gross count regardless of permissions
• If no tickets in an existing group is allowed for display, we still display the group header and count (should be removed)
• And, no doubt the pagination code also works with the full totals regardless of permissions - haven't looked into this, but when seeing the usage of LIMIT and OFFSET in the Query.execute() method I suppose that also works with gross numbers

For something like the ​th:PrivateTicketsPlugin where likely only a small percentage of tickets should actually be recognized for a given user, this will no doubt lead to a 'sub-optimal' display…

To fix this, I suggest moving the permission checks into the Query.execute() and Query.count() methods instead of doing it in template. That however looks to be a non-trivial change.

[Remy Blank]

Replying to osimons:

To fix this, I suggest moving the permission checks into the Query.execute() and Query.count() methods instead of doing it in template. That however looks to be a non-trivial change.

This means that the result set of Query.execute() will not be only the result of an SQL query, but also of some extra filtering (in this case, permission checks). This would open the door to solving #7558 quite eaily.

[Christian Boos]

osimons described the implementation issues quite accurately. It's something I've also pondered while working on the pagination and the ticket query macro. At that time, it seemed too costly and complicated to take the permissions into account, for the general case where fine grained ticket permissions are probably not so commonly used. It also meant to rewrite the pagination code completely, as I don't see how the COUNT/OFFSET/LIMIT logic can possibly work if permission checks have to be done, not to mention that fetching all those tickets would also partially defeat the memory optimization goals we had for implementing #216 in the first place.

Just consider the performance costs for a Wiki page containing a matrix of [[TicketQuery(...,format=count)]] macros. If you have a dozen or so of them, care must be taken that the page remains viewable at all, especially considering that #6436 is still not solved.

So I'd say we should have an efficient way to retrieve the tickets when many queries are performed in a request (the above use case), eventually by implementing a ticket cache, then rewrite the pagination logic so that it operates on the subset of tickets that match the filtering conditions (as Remy said, there could be more non-SQL checks than just the permission ones), and see if that's a viable option.

Alternatively, we could just make clear that the numbers shown concern all existing tickets, even non viewable ones. After all, that's what can happen with changesets: you could eventually look at r7000 if you have the permissions, so you know there are at least 6999 more changesets, even if none of them are viewable…

[Stephen Compall <stephen.compall@…>]

patch for Query.execute (not count) against 0.11.2.1

[trac-dev@…]

Simple implementation

[Artur Frysiak <trac-dev@…>]

I attached​ working implementation of comment:1 (patch againts trunk r7941).

[Christian Boos]

How does getting away with OFFSET/LIMIT affect the performance?

I think the approach presented in attachment:query.patch​ is probably fine, as the speed and memory costs are after all mostly due to Genshi, but I'd like to see some numbers before going further.

[Remy Blank]

Postponing until we get some measurements of the performance penalty, as requested in comment:6.

[Hermann Husen <hh@…>]

Extension to query.patch

[Hermann Husen <hh@…>]

May I add a contribution to attachment:query.patch​?

Instead of processing all rows, it appears practical to me to do a SQL re-query (LIMIT max OFFSET self.offset), if the result set of items with TICKET_VIEW permission has more than one page.

As a precondition, the SQL result set must be limited by (WHERE ID IN <permitted IDs>)

See attachment:query_execute.py​ as an extension example for function execute of attachment:query.patch​.

The code is not beautiful (some duplicate code) but should fit for demonstration.

Obviously self.constraints['id'] can be used to trigger an appropriate WHERE ID IN <permitted IDs> SQL-constraint in function get_sql.

Drawback: Apparently, if self.constraints[id] is used for ID filtering purposes, the complete list of IDs is appended via GET-parameters to the pagination links. Is this necessary?

[Jörg Schulz <jschulz@…>]

modification of query.patch

[Jörg Schulz <jschulz@…>]

As the attachment:query.patch​ removes the OFFSET/LIMIT constraints it always returns the complete list of tickets (that match the query and for which the user has TICKET_VIEW permission) and not only those that should be displayed on the current page (maybe that's the reason why the patch removes the paginator?).

attachment:reenable_pages.diff​ reenables pages. it only returns the list of tickets that should be displayed on the current page (as OFFSET/LIMIT did before).

if the sql-/permission-query results in 100000 tickets, then all those tickets are discarded and only <max> items are returned. that's far from optimal, but i can see no way to get around this, because the determination of the TICKET_VIEW permission can't be done by an sql-query. the decision should be left to the permission component. maybe it can be expanded in a way that it returns an additional sql-where-clause that repesents the actual TICKET_VIEW rules.