ReportModule uses magic number -1 where None should be used

[Ryan J Ollos]

I found this refactoring from a while back on one of my local branches. The use of -1 in ReportModule.process_request should be replaced with None in order to be more Pythonic. At least, that is the first thought I had.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11697.

[Ryan J Ollos]

On second thought, the change should go on the trunk since it could affect plugins implementing IRequestFilter, ITemplateStreamFilter and possibly others.

[Ryan J Ollos]

Fixed in [13048].

[Ryan J Ollos]

I have been sloppy. I thought this change was no big deal and I'd just get it out of the way, but I should have known better.

The AuthzPolicy maps a resource id of None to *, so we don't want to use None here. However, it wasn't possible even before the change to restrict access to the report list page. I think at least we can fix that.

In comment:30:ticket:11176 I had noted that it wasn't possible to explicitly grant access to the Report List page using an authz policy and proposed we use report:0 to refer to the report list page. However, report:-1 should have already worked for restricting fine-grained access. While I don't really like using -1 for the report list page, the following minor change would allow resource:-1 to be used in an authz policy to refer to the report list page. The net change after reverting [13048] would be:

trac/ticket/report.py

@@ -150 +150 @@
     def process_request(self, req):
         # did the user ask for any special report?
         id = int(req.args.get('id', -1))
-        if id != -1:
-            req.perm('report', id).require('REPORT_VIEW')
-        else:
-            req.perm.require('REPORT_VIEW')
+        req.perm('report', id).require('REPORT_VIEW')
 
         data = {}
         action = req.args.get('action', 'view')

It seems like that would be a simple, backward-compatible change to make for now, and we can either document -1 as the "resource id" for the report list page, or improve on that in the future.

Side note: it probably make more sense to make RegressionTestTicket11176 a unit test rather than a functional test since code in the templates is not under test. The unit tests will be faster and are easier to implement correctly because they don't depend on the state of the environment being shared with the other tests. I could create a new ticket to be handled later for replacing the functional tests with unit tests.

[Ryan J Ollos]

To make -1 function as a resource id for granting fine-grained access to the report list page, as a follow-on to #11152 / [11824] we also need to do fine-grained permission checks on the -1 resource id when determining which navigation item to render.

The check needs to be implemented in two locations:

• trunk/trac/ticket/query.py@12826:868#L859​
• trunk/trac/ticket/report.py@13048:130#L124​

[Ryan J Ollos]

Proposed changes to fix regression and other fine-grained permission issues in log:rjollos.git:t11697.1.

[Christian Boos]

Looks good to me, this special case has always been troublesome.

I'd suggest to replace -1 with {ReportModule,self}.REPORT_LIST_ID, so that the intent is clearer (and REPORT_LIST_ID set to -1 in ReportModule, of course). We may consider at one point having alphanum ids for reports, so we wouldn't have to change everything again.

[Ryan J Ollos]

Yeah, that seems like a good idea. The change is now in [c702d1a5/rjollos.git]. I'll test again this evening before pushing the changes.

[Ryan J Ollos]

Changes committed to trunk in [13055]. More work will be done in milestone:1.1.3 by adding additional tests (comment:4), defining and documenting proper behavior for the report and ticket realms.

If we keep -1 as the resource id for the Report List page, then it might also make sense to use -1 as the resource id for the Query page. That would allow an authz policy for granting access to the query page such as the following:

[ticket:-1]
* = TICKET_VIEW

[Ryan J Ollos]

Replying to rjollos:

Changes committed to trunk in [13055]. More work will be done in milestone:1.1.3 by adding additional tests (comment:4), defining and documenting proper behavior for the report and ticket realms.

I'll also look into backporting these changes to branches/1.0-stable​.

[Ryan J Ollos]

I noticed an error in [13055#file1]. The following change should be made:

-        show_query_link = 'TICKET_VIEW' in req.perm('query') and \
+        show_query_link = 'TICKET_VIEW' in req.perm('ticket') and \

query is not a realm on which permission checks are implemented. The proper realm is ticket.

[Ryan J Ollos]

In [13067]: Fixed regression described in comment:12.