Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#11166 closed defect (fixed)

TracLinks to non-existent and forbidden reports should not be followable

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: report system Version: 1.0-stable
Severity: normal Keywords: permissions, traclinks
Cc: Branch:
Release Notes:

TracLinks to missing reports are rendered as non-followable with the missing class, and TracLinks that the user doesn't have permission to view are rendered as non-followable with the forbidden class.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos <ryan.j.ollos@…>)

For example, the non-existent report {10000} is rendered as a followable link. The HTML is:

<a class="report" href="/report/10000">{10000}</a>

but should be:

<a class="missing report">{10000}</a>

resulting in:

{10000}

Also, when the user doesn't have permission to view a report the TracLink is rendered as a link that can be followed, but the behavior should be similar to when the report doesn't exist.

Attachments (0)

Change History (14)

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 12 years ago

  • b27a1cfd (unrelated refactoring) The Milestone class was being indirectly imported from the roadmap module.
  • 6f0e3cea Add a get_report method and use it to determine how the report link should be formatted. The report link is formatted with the missing class and without an href attribute in the case that the report doesn't exist.
  • 9b6497b6 Utilize get_report method to eliminate code duplication.

The quality of the Report test cases in wiki_syntax.py have been diminished somewhat because every report TracLink is of the "missing" type. It looks like we'll need to add some reports in the report_setup method. I was thinking of adding methods report_create and report_update in order to facilitate that, but wanted to ask for feedback before doing so.

comment:2 by Ryan J Ollos <ryan.j.ollos@…>, 12 years ago

  • bb78cb89 (refactoring) Move the return statement out of the try and into an else clause of the try...except.

in reply to:  1 comment:3 by Ryan J Ollos <ryan.j.ollos@…>, 12 years ago

Replying to Ryan J Ollos <ryan.j.ollos@…>:

The quality of the Report test cases in wiki_syntax.py have been diminished somewhat because every report TracLink is of the "missing" type. It looks like we'll need to add some reports in the report_setup method. I was thinking of adding methods report_create and report_update in order to facilitate that, but wanted to ask for feedback before doing so.

Instead of adding methods to the ReportModule class, I added a function in the test fixture setup method for creating reports, following along with what Jun did for a test case in the th:BookmarkPlugin.

  • a3725d73 Added test cases for 'non-missing' reports.

So now I have 5 commits pending for this ticket, however commit (4) and (5) could be squashed into commit (2). Commits (1) and (3) should probably continue to stand alone. Should I rebase the changes on a new branch, or just leave them as they are?

comment:4 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Description: modified (diff)
Keywords: permissions added
Summary: TracLinks to non-existent reports should have the "missing" class and should not have an href attributeTracLinks to non-existent and forbidden reports should not be followable

I'm expanding scope of this ticket to deal with users not having REPORT_VIEW.

Currently the wiki TracLink is the only TracLink to utilize the forbidden class, but it looks like we could use it here as well.

comment:5 by Ryan J Ollos, 11 years ago

Milestone: 1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:6 by Ryan J Ollos, 11 years ago

Changes have been rebased and prepared in rjollos.git:t11166.2. Forbidden ReportLinks still need to be handled.

comment:7 by Jun Omae, 11 years ago

Hmm, an invalid syntax with Python 2.5.

...
  File "/home/jun66j5/src/trac/edgewall/github/trac/ticket/tests/wikisyntax.py", line 189
    with tc.env.db_transaction as db:
          ^
SyntaxError: invalid syntax
make: *** [unit-test] Error 1

Anyway, non-existent report links work fine.

comment:8 by Christian Boos, 11 years ago

Isn't that just a missing from __future__ import with_statement?

Alternatively, try this?

def create_report(id):
    tc.env.db_transaction("""
        INSERT INTO report (id,title,query,description) 
        VALUES (%s,%s,'SELECT 1','')""", (id, 'Report %s' % id)) 

in reply to:  2 comment:9 by Ryan J Ollos, 11 years ago

Replying to Ryan J Ollos <ryan.j.ollos@…>:

  • bb78cb89 (refactoring) Move the return statement out of the try and into an else clause of the try...except.

On closer look, it's not strictly necessary to use an else, the return could just be placed after and outside of the try / except block, however I think it might make the return paths easier to read, and more obvious that they are all determined from checking for the existence of the report.

comment:10 by Ryan J Ollos, 11 years ago

Release Notes: modified (diff)

Thanks for the feedback. Suggestions incorporated and forbidden report links handled in rjollos.git:t11166.3. To be consistent, we'd want to utilize the forbidden class for other TracLinks, and add title attributes for missing and forbidden reports.

It would be nice to be able to do permissions-based testing in the wiki syntax tests (wikisyntax.py). For the report TracLinks, we'd even like the test to be more complex than just granting / revoking REPORT_VIEW, because that wouldn't reveal if for example the permissions check has been implemented as 'REPORT_VIEW' in formatter.req.perm rather than 'REPORT_VIEW' in formatter.req.perm('report', id).

For functional tests I've used FunctionalTestEnvironment.enable_authz_permpolicy, however I think for the wiki syntax tests it would be nice to avoid having a dependency on tracopt.perm.authz_policy and ConfigObj. I'm not entirely sure yet, but maybe the key is to extend MockPerm to allow some specific policies to be specified, such as {'report:3': '!REPORT_VIEW'}.

Any ideas or suggestions on this "Mock permission" issue?

comment:11 by Ryan J Ollos, 11 years ago

Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [11986:11987]. Merged to trunk in [11989].

comment:12 by Ryan J Ollos, 11 years ago

Keywords: traclinks permissions → traclinks, permissions

comment:13 by Ryan J Ollos, 11 years ago

Keywords: traclink added; traclinks removed

comment:14 by Ryan J Ollos, 11 years ago

Keywords: traclinks added; traclink removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
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.