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

[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.

[Ryan J Ollos <ryan.j.ollos@…>]

• ​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.

[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.

[Ryan J Ollos <ryan.j.ollos@…>]

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?

[Ryan J Ollos <ryan.j.ollos@…>]

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.

[Ryan J Ollos]

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

[Jun Omae]

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.

[Christian Boos]

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)) 

[Ryan J Ollos]

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.

[Ryan J Ollos]

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?

[Ryan J Ollos]

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