Edgewall Software
Modify

Opened 6 years ago

Last modified 7 months ago

#7608 new defect

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

Reported by: jevans Owned by:
Priority: normal Milestone: next-major-releases
Component: report system Version: 0.11.1
Severity: normal Keywords: query
Cc: osimons, stephen.compall@…, trac-dev@…, comlock@…, hh@…, jschulz@…, felix.schwarz@…, leho@…
Release Notes:
API Changes:

Description

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.

Attachments (4)

correct-query-result-count.diff (469 bytes) - added by Stephen Compall <stephen.compall@…> 6 years ago.
patch for Query.execute (not count) against 0.11.2.1
query.patch (3.1 KB) - added by trac-dev@… 6 years ago.
Simple implementation
query_execute.py (3.8 KB) - added by Hermann Husen <hh@…> 5 years ago.
Extension to query.patch
reenable_pages.diff (821 bytes) - added by Jörg Schulz <jschulz@…> 5 years ago.
modification of query.patch

Download all attachments as: .zip

Change History (16)

comment:1 follow-up: Changed 6 years ago by osimons

  • Cc osimons added

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.

comment:2 in reply to: ↑ 1 Changed 6 years ago by rblank

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.

comment:3 Changed 6 years ago by cboos

  • Component changed from general to report system
  • Keywords query added
  • Milestone set to 0.12

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…

Changed 6 years ago by Stephen Compall <stephen.compall@…>

patch for Query.execute (not count) against 0.11.2.1

comment:4 Changed 6 years ago by Stephen Compall <stephen.compall@…>

  • Cc stephen.compall@… added

Changed 6 years ago by trac-dev@…

Simple implementation

comment:5 Changed 6 years ago by Artur Frysiak <trac-dev@…>

  • Cc trac-dev@… added

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

comment:6 Changed 6 years ago by cboos

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.

comment:7 Changed 5 years ago by comlock@…

  • Cc comlock@… added

comment:8 Changed 5 years ago by rblank

  • Milestone changed from 0.12 to next-major-0.1X

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

Changed 5 years ago by Hermann Husen <hh@…>

Extension to query.patch

comment:9 Changed 5 years ago by Hermann Husen <hh@…>

  • Cc hh@… added

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?

Changed 5 years ago by Jörg Schulz <jschulz@…>

modification of query.patch

comment:10 Changed 5 years ago by Jörg Schulz <jschulz@…>

  • Cc jschulz@… added

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.

comment:11 Changed 3 years ago by fschwarz

  • Cc felix.schwarz@… added

comment:12 Changed 7 months ago by lkraav <leho@…>

  • Cc leho@… added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
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.