#11166 closed defect (fixed)
TracLinks to non-existent and forbidden reports should not be followable
Reported by: | 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 |
||
API Changes: | |||
Internal Changes: |
Description (last modified by )
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)
follow-up: 3 comment:1 by , 12 years ago
follow-up: 9 comment:2 by , 12 years ago
- bb78cb89 (refactoring) Move the
return
statement out of thetry
and into anelse
clause of thetry...except
.
comment:3 by , 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 thereport_setup
method. I was thinking of adding methodsreport_create
andreport_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 , 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 attribute → TracLinks 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 , 11 years ago
Milestone: | → 1.0.2 |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:6 by , 11 years ago
Changes have been rebased and prepared in rjollos.git:t11166.2. Forbidden ReportLinks still need to be handled.
comment:7 by , 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 , 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))
comment:9 by , 11 years ago
Replying to Ryan J Ollos <ryan.j.ollos@…>:
- bb78cb89 (refactoring) Move the
return
statement out of thetry
and into anelse
clause of thetry...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 , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Committed to 1.0-stable in [11986:11987]. Merged to trunk in [11989].
comment:12 by , 11 years ago
Keywords: | traclinks permissions → traclinks, permissions |
---|
comment:13 by , 11 years ago
Keywords: | traclink added; traclinks removed |
---|
comment:14 by , 11 years ago
Keywords: | traclinks added; traclink removed |
---|
Milestone
class was being indirectly imported from theroadmap
module.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 anhref
attribute in the case that the report doesn't exist.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 thereport_setup
method. I was thinking of adding methodsreport_create
andreport_update
in order to facilitate that, but wanted to ask for feedback before doing so.