Fine-grained permission checks should be enforced on the Report list page

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

If a user doesn't have permission to view a report because of the TracFineGrainedPermissions policy, then on the Report list page (/report):

• The link should be inactive and have the forbidden styling.
• The report description should not be shown.

Here is an example of the desired behavior when the user only has permission to view reports 1 and 4. The anonymous group has been granted the coarse-grained REPORT_VIEW. The screenshots show the view that the anonymous user sees with the fix in place:

[report:1]
anonymous = REPORT_VIEW

[report:4]
anonymous = REPORT_VIEW

[report:*]
* =

This ticket resulted from discussion in ​th:#11047 and ​th:#11049.

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

I have a fix and functional test in place for this ticket, however if I try to run all of the functional tests I run into various test failures that have nothing to do with my changes. This is my first time running the functional tests on this particular OS, Ubuntu 12.10.

With all of the tests in trac/ticket/tests/functional.py commented out except for RegressionTestTicket4630a(),

$ PYTHONPATH=. python ./trac/ticket/tests/functional.py
F
======================================================================
FAIL: runTest (__main__.RegressionTestTicket4630a)
Test for regression of http://trac.edgewall.org/ticket/4630 a
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./trac/ticket/tests/functional.py", line 1178, in runTest
    self._tester.login('joé')
  File "/home/user/Workspace/t11176/trac.git/trac/tests/functional/tester.py", line 43, in login
    tc.find("logged in as %s" % username)
  File "/home/user/Workspace/t11176/trac.git/trac/tests/functional/better_twill.py", line 204, in better_find
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("no match to 'logged in as jo\xc3\xa9'", '/home/user/Workspace/t11176/trac.git/testenv/trac/log/RegressionTestTicket4630a.html')

----------------------------------------------------------------------
Ran 1 test in 6.045s

FAILED (failures=1)

$ python --version
Python 2.7.3

>>> import twill
>>> print twill.__version__
0.9

[Christian Boos]

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

With all of the tests in trac/ticket/tests/functional.py commented out except for RegressionTestTicket4630a(),

$ PYTHONPATH=. python ./trac/ticket/tests/functional.py
F

Yes, this is to be expected, unfortunately. It's only afterwards that I realized that r11752 prevented the modified tests to be run in isolation. We need a follow-up on this changeset.

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

Replying to cboos:

Yes, this is to be expected, unfortunately. It's only afterwards that I realized that r11752 prevented the modified tests to be run in isolation. We need a follow-up on this changeset.

Do you mean it's expected that trac/ticket/tests/functional.py will fail, but trac.tests.functional.__init__.py should succeed?

The latter sometimes succeeds, but most often fails,

$ PYTHONPATH=. python ./trac/tests/functional/__init__.py
.........F
======================================================================
FAIL: runTest (trac.tests.functional.testcases.RegressionTestTicket6318)
Regression test for non-ascii usernames (#6318)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11176/trac.git/trac/tests/functional/testcases.py", line 216, in runTest
    self._tester.login(u'joé')
  File "/home/user/Workspace/t11176/trac.git/trac/tests/functional/tester.py", line 43, in login
    tc.find("logged in as %s" % username)
  File "/home/user/Workspace/t11176/trac.git/trac/tests/functional/better_twill.py", line 204, in better_find
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("no match to 'logged in as jo\xc3\xa9'", '/home/user/Workspace/t11176/trac.git/testenv/trac/log/RegressionTestTicket6318.html', '/home/user/Workspace/t11176/trac.git/testenv/trac/log/RegressionTestTicket6318.html')

----------------------------------------------------------------------
Ran 10 tests in 12.546s

FAILED (failures=1)

[Christian Boos]

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

The latter sometimes succeeds, but most often fails,

That's a bit unexpected, that one should either always fail or pass, not "sometimes"… Anything special in testenv/trac/log/RegressionTestTicket6318.html?

It's as if the .adduser didn't "take effect" before the attempt .login. I'd look at what are the timestamps in source:trunk/trac/web/auth.py@11795#L286​.

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

Replying to cboos:

That's a bit unexpected, that one should either always fail or pass, not "sometimes"… Anything special in testenv/trac/log/RegressionTestTicket6318.html?

RegressionTestTicket6318.html is empty. It seems like tracd is dying during the test run. I setup a new environment with 1.0-stable and ran PYTHONPATH=. python ./trac/tests/functional/__init__.py eight times in success just now, and it failed on runs 2 and 8.

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

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

Replying to cboos: RegressionTestTicket6318.html is empty. It seems like tracd is dying during the test run. I setup a new environment with 1.0-stable and ran PYTHONPATH=. python ./trac/tests/functional/__init__.py eight times in succession just now, and it failed on runs 2 and 8.

I made a spelling error in that comment, "success" should have been "succession". When it failed on runs 2 and 8, I saw the same error as in comment:6.

[Christian Boos]

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

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

Replying to cboos: RegressionTestTicket6318.html is empty. It seems like tracd is dying during the test run.

Wow. Nothing in testenv/trac/log/trac.log?

I setup a new environment with 1.0-stable and ran PYTHONPATH=. python ./trac/tests/functional/__init__.py eight times in succession just now, and it failed on runs 2 and 8.

I've been running these 10 tests as well in a loop for a few hours now, without seeing any error. Does adding a pause between the adduser and the login steps help?

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

Replying to cboos:

Wow. Nothing in testenv/trac/log/trac.log?

Log is attached, trac.log​. It looks like it ends right before the login for joé. If I extrapolate on the previous log entries, I would expect to see next in log:

2013-05-06 14:04:59,127 Trac[main] DEBUG: Dispatching <RequestWithSession "GET '/login'">
2013-05-06 14:04:59,127 Trac[api] WARNING: Unable to find repository '(default)' for synchronization
2013-05-06 14:04:59,147 Trac[session] DEBUG: Retrieving session for ID u'joé'

I've been running these 10 tests as well in a loop for a few hours now, without seeing any error. Does adding a pause between the adduser and the login steps help?

Between the adduser and login calls I added,

import time
time.sleep(10)

but it didn't change the result.

I tried changing joé → joe, but that didn't fix the issue. The contents of htpasswd seem fine,

admin:f1zTlXGLHLgck
user:VjOLNc9yA6l8c
joé:9LNCFt8lqLj46

I didn't have these problems when I was on Debian 6, but that VM stopped booting and these problems appeared immediately when I brought up a fresh Ubuntu 12.10 VM.

[Christian Boos]

Just to be sure: with a checkout at r11752 and no local modifications, do you see the same behavior?

Also I'm not sure if I understood correctly what you reported in comment:6: the unmodified test suite simply stops at RegressionTestTicket6318 (skipping the > 100 others), or you just commented them out so that only the local tests are executed? (that's what I did, for example).

I'm afraid I'm a bit clueless on what could happen here, and can only encourage you to continue to try other changes in order to detect what makes it fail (e.g. joé → user this one should work, at least without the adduser).

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

Replying to cboos:

Also I'm not sure if I understood correctly what you reported in comment:6: the unmodified test suite simply stops at RegressionTestTicket6318 (skipping the > 100 others), or you just commented them out so that only the local tests are executed? (that's what I did, for example).

I also commented out the imported test cases, so that only the 10 local test cases would execute.

It looks like you are right to suggest in comment:7 that the file modification times should be looked at. The problem appears to be due to the file modification timestamp having a resolution of one second on this platform,

>>> os.stat('/home/user/Workspace/t11176/trac.git/testenv/htpasswd').st_mtime
1367888231.0

so I conclude that the following steps occurs in a test run that errors,

adduser ← htpasswd mtime changed
login ← htpasswd file reloaded
adduser ← htpasswd mtime not changed within the resolution of one second by this update
login ← htpasswd file not reloaded and Trac doesn't see the user we just added

The timestamps on most platforms must have a higher resolution. By running touch, then os.stat(..).st_mtime, I find that the timestamps on this Ubuntu VM always have 0-digits of precision. The ​python docs state that the resolution will vary by platform, but I'm surprised to see a low resolution timestamp in my Ubuntu VM. My host is Windows x64, and I get a high resolution timestamp (from Cygwin):

Ryan J Ollos@RyanJOllos-PC ~
$ touch tempfile

Ryan J Ollos@RyanJOllos-PC ~
$ python
Python 2.7.3 (default, Dec 18 2012, 13:50:09)
[GCC 4.5.3] on cygwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> s = os.stat('/home/Ryan J Ollos/tempfile')
>>> s.st_mtime
1367902688.8193212

Just to be sure, here is the same test on the Ubuntu VM,

user@ubuntu:~/Workspace/t11176/trac.git$ touch tempfile
user@ubuntu:~/Workspace/t11176/trac.git$ python
Python 2.7.3 (default, Aug  1 2012, 05:16:07) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> s = os.stat('/home/user/Workspace/t11176/trac.git/tempfile')
>>> s.st_mtime
1367902831.0

Could this possibly be due to running a 32-bit OS, and/or running on the VM? (VMWare Workstate 9.0.2) I wouldn't guess the 32-bit OS have anything to do with it, but maybe the file system or running on a VM.

My previous VM was Debian 6 x64 and I didn't have these problems there, but this is Ubuntu 12.10 x32:

user@ubuntu:~/Workspace/t11176/trac.git$ uname -a
Linux ubuntu 3.2.0-41-generic-pae #66-Ubuntu SMP Thu Apr 25 03:50:20 UTC 2013 i686 i686 i386 GNU/Linux

It seems like the best fix might be to just move both adduser calls to the front of the testcase. A different fix would be to add a sleep between the two adduser calls (in comment:11 I had added a sleep after the second adduser call, which is why it had no effect). I'd really like to understand why the resolution of the timestamps is so low, but either way it feels like we should put some fix or workaround in place regardless since others could encounter this.

---

Here are the experiment I ran that led to here (for each experiment the tests cases were run 10 times in succession by a script),

• Replace joé with user: PASS
• user is already created in FunctionalTestEnvironment.create, so replace user with user2 in RegressionTestTicket6318: INTERMITENT FAILUES with "no match to 'logged in as jo\xc3\xa9'"
• Interchange order of add/login for user and joé, so that joé is added and logs in first: PASS
• Switch add/login for user2 and joé: INTERMITENT FAILURES with "no match to 'logged in as user2'". Both users exist in the htpasswd file even when the test fails.

This led me to believe that the problem arises when we call FunctionalTestEnvironment.adduser twice in a test case and attempt to add a non-existing user each time. For the two experiments that passed, we only added one new user, since user already existed in the htpasswd at the start of the test.

• Moved both FunctionalTestEnvironment.adduser calls to start of test case: PASS
• Added self._testenv.restart() after the second adduser call: PASS
• Remove the conditional if mtime > self.mtime: in trac.web.auth.PasswordFileAuthentication so that the htpasswd file is always reloaded: PASS
• Add a time.sleep(10) before the second call to self._testenv.adduser: PASS

[Christian Boos]

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

Thanks for taking the time to explain your findings in great details!

Could this possibly be due to running a 32-bit OS, and/or running on the VM? (VMWare Workstate 9.0.2) I wouldn't guess the 32-bit OS have anything to do with it, but maybe the file system or running on a VM.

Probably a limitation of the filesystem? My Linux testing platform is Windows 7 x64 / VMWare 8.0.4 / OpenSuSE 12.3 (x86_64) and there I have xfs and ext4 filesystems, both having a subsecond time resolution. Check also with stat <file> on the command line.

It seems like the best fix might be to just move both adduser calls to the front of the testcase. A different fix would be to add a sleep between the two adduser calls (in comment:11 I had added a sleep after the second adduser call, which is why it had no effect). I'd really like to understand why the resolution of the timestamps is so low, but either way it feels like we should put some fix or workaround in place regardless since others could encounter this.

Right, the point is that there are such filesystems, and that we should either try to cope with them, or detect this situation early and print a warning. And that would be useful not only for the test suite, but also for the normal runtime, at places where we assume we can reliably detect a file modification through changes to its mtime. Yes, I'm looking at you, trac.ini ;-)

So for the specific case of this test I'd rather go with the first suggestion, the functional test suite is slow enough already, we don't need to add "sleep" statements.

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

Replying to cboos:

Probably a limitation of the filesystem? My Linux testing platform is Windows 7 x64 / VMWare 8.0.4 / OpenSuSE 12.3 (x86_64) and there I have xfs and ext4 filesystems, both having a subsecond time resolution. Check also with stat <file> on the command line.

Here is some info from my Ubuntu VM:

$ df -T
Filesystem     Type     1K-blocks    Used Available Use% Mounted on
/dev/sda2      ext3      41187904 7821984  31273684  21% /
udev           devtmpfs   1023416      12   1023404   1% /dev
tmpfs          tmpfs       412292     748    411544   1% /run
none           tmpfs         5120       0      5120   0% /run/lock
none           tmpfs      1030724    1992   1028732   1% /run/shm
$ touch tempfile
$ stat tempfile 
  File: `tempfile`
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 802h/2050d      Inode: 1581188     Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    user)   Gid: ( 1000/    user)
Access: 2013-05-08 01:45:32.000000000 -0400
Modify: 2013-05-08 01:45:32.000000000 -0400
Change: 2013-05-08 01:45:32.000000000 -0400
 Birth: -

Right, the point is that there are such filesystems, and that we should either try to cope with them, or detect this situation early and print a warning. And that would be useful not only for the test suite, but also for the normal runtime, at places where we assume we can reliably detect a file modification through changes to its mtime. Yes, I'm looking at you, trac.ini ;-)

At the moment I'm not sure how to either detect or cope with the situation, but I plan to investigate further and follow-up.

[Christian Boos]

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

Replying to cboos:

Here is some info from my Ubuntu VM:

/dev/sda2      ext3      41187904 7821984  31273684  21% /

Having mostly used reiserfs then xfs in the last ten years, I wasn't aware that such a popular filesystem had this problem…

At the moment I'm not sure how to either detect or cope with the situation, but I plan to investigate further and follow-up.

Well, the idea I had in mind was nothing fancy, just to issue a warning once if we suspect we deal with such a filesystem. Whenever we check for the first time for such a path and we see a "round" number of seconds we can say something like: "WARNING: it looks like '%s' is on a filesystem that has a low timestamp resolution, changes might go undetected". False positives should be rare enough if the filesystem supports really supports subsecond timestamps. And if the sysadmin checks the situation and it's really a false positive, a simple touch should make the warning go away (or it's really bad luck ;-) ).

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

Replying to cboos:

Well, the idea I had in mind was nothing fancy, just to issue a warning once if we suspect we deal with such a filesystem. Whenever we check for the first time for such a path and we see a "round" number of seconds we can say something like: "WARNING: it looks like '%s' is on a filesystem that has a low timestamp resolution, changes might go undetected". False positives should be rare enough if the filesystem supports really supports subsecond timestamps. And if the sysadmin checks the situation and it's really a false positive, a simple touch should make the warning go away (or it's really bad luck ;-) ).

That seems like a good (enough) solution. It ​sounds like we have about 10^-6 resolution on the timestamp from Python. We could even check mtime, atime and ctime to see if they all have 0-digits of precision, so unless the file was just created, the probability of a false positive would be 1 in 10¹⁸ or so?

Where should the check go? There are several classes that check a file modification time, so a central location for the check seems desirable.

Here's some discussion to back up the finding about EXT3: ​SO:3805201.

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

​t11176 has a patch for Fine-grained permission checks should be enforced on the Report list page and a workaround for test case failure on an OS with a low-resolution timestamp. Methods were added to the FunctionalTestEnvironment for setting/enabling and disabling fine-grained permissions, which will be useful for adding test cases in other tickets, such as #11069 and #11078.

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

Would it be okay to target this to milestone:1.0.2?

In general, I feel that I have a grasp of what is appropriate for 1.0.x vs 1.1.x. Is it okay for me to set the milestone for tickets that have patches that I feel are ready for inclusion, and the committers can just change the milestone if they don't want to include them?

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

Manual testing was done with the permissions policy shown below. The report description is still being shown, so I will fix that issue, expand the scope of the functional tests and rebase the changes on a new branch.

[report:1]
anonymous = REPORT_VIEW

[report:4]
anonymous = REPORT_VIEW

[report:5]
anonymous = REPORT_MODIFY

[report:6]
anonymous = REPORT_MODIFY, REPORT_DELETE

[report:8]
* = REPORT_VIEW

[report:*]
* =

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

I'm reconsidering how this should work. Maybe we should hide the report entirely on the report list page when the user doesn't have permission to view it. That would mirror the behavior of the milestones on the roadmap page and additionally the title of the report wouldn't be revealed to the user that doesn't have permission to view it (see also comment:4:ticket:11166 for discussion of how report TracLinks should be rendered).

[Jun Omae]

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

I'm reconsidering how this should work. Maybe we should hide the report entirely on the report list page when the user doesn't have permission to view it.

+1. Also, the saved query as report maybe has the same issue. The saved query shows the title and description of the report.

Your changes….

            <h3><a class="${'allowed' if can_view else 'forbidden'}"
                   rel="${'nofollow' if not can_view else None}"
                   title="${'View report' if can_view else 'You don\'t have permission to view this report'}"
                   href="${href.report(id) if can_view else None}" py:choose="sort">

IMO, rel="nofollow" is not needed. When the user doesn't have REPORT_VIEW, the anchor have no href attribute. So, a web bot cannot follow.

The other one is that we should show i18n/l10n text on the title attribute.

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

Replying to jomae:

IMO, rel="nofollow" is not needed.

Thanks, you are right. I will change the patch to hide reports from the list, and make sure to make any strings translatable.

When the user doesn't have REPORT_VIEW, the anchor have no href attribute. So, a web bot cannot follow.

There is an open defect affecting this behavior, #11166.

[Ryan J Ollos]

Replying to cboos:

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

It seems like the best fix might be to just move both adduser calls to the front of the testcase. …

So for the specific case of this test I'd rather go with the first suggestion, the functional test suite is slow enough already, we don't need to add "sleep" statements.

Fix for test execution failures on an OS with low resolution time stamps committed in [11826].

[Ryan J Ollos]

The behavior now is as follows:

• Global REPORT_VIEW is required for display of the View Tickets navigation item that directs to the /report page (note however that View Tickets could be present and directing to the /query page after global REPORT_VIEW is revoked, see #11152).
• Global REPORT_VIEW is required for viewing the report list page (/report).
• Global REPORT_CREATE is required to create any reports.

The changes have been prepared in rjollos.git:t11176. I'll probably expand the functional test case (RegressionTestTicket11176) before committing this work, but the basic change I had in mind for the ReportModule are now in place.

[Ryan J Ollos]

Oh, doing the permission check in the template is no good, because then "other formats" (RSS, CSV, TSV) don't match what is displayed on the report list page. I'll fix that now.

[Ryan J Ollos]

Latest revisions can be found in rjollos.git:t11176.2.

[Ryan J Ollos]

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

When the user doesn't have REPORT_VIEW, the anchor have no href attribute. So, a web bot cannot follow.

There is an open defect affecting this behavior, #11166.

I'm not sure what I was thinking with this comment as the work in #11166 does not effect the view on the Report List page.

[Ryan J Ollos]

Replying to rjollos:

The behavior now is as follows:

• Global REPORT_VIEW is required for display of the View Tickets navigation item that directs to the /report page (note however that View Tickets could be present and directing to the /query page after global REPORT_VIEW is revoked, see #11152).
• Global REPORT_VIEW is required for viewing the report list page (/report).

I think we'll want to revisit this in the future; it should be possible to explicitly grant access to the Report List page using an authz policy. A common scenario is to grant access to the Report List page and only a subset of the available reports. We might want to use report:0 to refer to the Report List page. For example,

[report:0]
user = REPORT_VIEW  # grant access to report list page

[report:1]
user = REPORT_VIEW

Alternatively we could allow a realm without a resource to be specified for a section,

[report]
user = REPORT_VIEW

[Ryan J Ollos]

Committed to 1.0-stable in [12001:12006] and merged to trunk in [12007].

[Jun Omae]

I found a minor issue. If conf/authzpolicy.conf is like the following and anonymous have REPORT_VIEW privilege, anonymous cannot access /report/2 and /report/3.

[report:1]
anonymous = REPORT_VIEW
[report:2]
anonymous =
[report:*]
anonymous =

However, if visiting/query?report=2 page, the anonymous can retrieve the title and description of report:2. If the report is a saved query, the result of tickets will be leaked.

[Ryan J Ollos]

Replying to jomae:

However, if visiting/query?report=2 page, the anonymous can retrieve the title and description of report:2. If the report is a saved query, the result of tickets will be leaked.

Thanks, I will fix it and add a test case.

[Ryan J Ollos]

Proposed changes can be found in rjollos.git:t11176.3.

[Ryan J Ollos]

Committed to 1.0-stable in [12060], and merged to trunk in [12061].

[Ryan J Ollos]

Replying to cboos:

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

At the moment I'm not sure how to either detect or cope with the situation, but I plan to investigate further and follow-up.

Well, the idea I had in mind was nothing fancy, just to issue a warning once if we suspect we deal with such a filesystem.

I've opened #11329 to propose a fix for the issue on filesystems with a low resolution timestamp.

[Ryan J Ollos]

Related changes discussed in #11697.

[Ryan J Ollos]

Replying to cboos:

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

Replying to cboos:

Here is some info from my Ubuntu VM:

/dev/sda2      ext3      41187904 7821984  31273684  21% /

Having mostly used reiserfs then xfs in the last ten years, I wasn't aware that such a popular filesystem had this problem…

I discovered today that HFS+ used by OSX also has a 1 second date resolution.