Edgewall Software

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11697 closed defect (fixed)

ReportModule uses magic number -1 where None should be used — at Version 6

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: report system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fine-grained permissions can be used to restrict access to the report listing page using the resource id -1. Fine-grained permissions can be used to grant the REPORT_CREATE permission for the report realm (using [report:*] in the authz file).

API Changes:

Description

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.

Change History (6)

comment:1 by Ryan J Ollos, 5 years ago

Proposed changes in log:rjollos.git:t11697.

comment:2 by Ryan J Ollos, 5 years ago

Milestone: 1.0.31.1.3

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

comment:3 by Ryan J Ollos, 5 years ago

Milestone: 1.1.31.1.2
Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Fixed in [13048].

comment:4 by Ryan J Ollos, 5 years ago

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

    index d3dacf9..2740d61 100644
    a b class ReportModule(Component):  
    150150    def process_request(self, req):
    151151        # did the user ask for any special report?
    152152        id = int(req.args.get('id', -1))
    153         if id != -1:
    154             req.perm('report', id).require('REPORT_VIEW')
    155         else:
    156             req.perm.require('REPORT_VIEW')
     153        req.perm('report', id).require('REPORT_VIEW')
    157154
    158155        data = {}
    159156        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.

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

comment:5 by Ryan J Ollos, 5 years ago

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:

comment:6 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

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

Note: See TracTickets for help on using tickets.