Edgewall Software
Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11697 closed defect (fixed)

ReportModule uses magic number -1 where None should be used

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: report system Version:
Severity: normal Keywords:
Cc:
Release Notes:
  • Fine-grained permissions can be used to restrict access to the Report List page using the resource id -1.
  • Fine-grained permission checks are implemented on the Report List resource (for ReportModule) and the ticket realm (for QueryModule) before displaying the mainnav and contextual navigation items.
  • Fine-grained permissions can be used to grant the REPORT_CREATE permission for the report realm (using [report:*] in the authz file).
API Changes:

Added class attribute REPORT_LIST_ID to the ReportModule class, containing the resource id of the Report List page.

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.

Attachments (0)

Change History (13)

comment:1 Changed 4 years ago by Ryan J Ollos

Proposed changes in log:rjollos.git:t11697.

comment:2 Changed 4 years ago by Ryan J Ollos

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 Changed 4 years ago by Ryan J Ollos

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

Fixed in [13048].

comment:4 Changed 4 years ago by 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

    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 4 years ago by Ryan J Ollos (previous) (diff)

comment:5 Changed 4 years ago by 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:

comment:6 Changed 4 years ago by Ryan J Ollos

Release Notes: modified (diff)

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

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Ryan J Ollos

API Changes: modified (diff)
Release Notes: modified (diff)

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
Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:10 Changed 4 years ago by Ryan J Ollos

Release Notes: modified (diff)

comment:11 in reply to:  9 Changed 4 years ago by Ryan J Ollos

Keywords: refactoring removed

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.

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by Ryan J Ollos

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.