ROADMAP_VIEW permission won't work properly

[spamgy@…]

When remove permission "ROADMAP_VIEW" from anonimous, only menu changes. If anonimous user put ​http://..../roadmap in his browser, he wil get full functionality of roadmap

[Christian Boos]

Indeed, the only visible effect of removing ROADMAP_VIEW is that the default main navigation link to the Roadmap is not shown.

Going there with /roadmap works, as unexpected ;-)

Note there's some discussion in #3022 which might be relevant here. But as long as we have ROADMAP_VIEW, we might as well enforce it.

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

The permissions checking in roadmap.py also prevents milestones from being viewed on the roadmap with TracFineGrainedPermissions are used to grant permission to view milestones. I'll work up a patch and functional test case.

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

Patch against Trac 1.0

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

t11028-r11307-1.patch​ implements the one-line fix. Getting a functional test working has been quite a bit more involved, but is forthcoming.

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

Patch against r11682 of the trunk.

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

t11028-r11682-1.patch​ adds a functional test case. Since ticket:10984:t10984-r11483-1.patch​ is still pending, I had to add the testenv.grant_perm and testenv.revoke_perm methods in this patch as well, and their functionality has also been expanded to accept a list or tuple of permissions as an argument.

One thing I struggled with for quite a while is that when revoking permissions the permission change doesn't seem to take effect without restarting the web server. Even more strange, I didn't seem to have this problem with the patch in ticket:10984:t10984-r11483-1.patch​.

On the other hand, if I add a restart of the web server to testenv.grant_perm, I get the following traceback:

ETraceback (most recent call last):
  File "./trac/ticket/tests/functional.py", line 1859, in <module>
    unittest.main(defaultTest='functionalSuite')
  File "/usr/lib/python2.7/unittest/main.py", line 95, in __init__
    self.runTests()
  File "/usr/lib/python2.7/unittest/main.py", line 229, in runTests
    self.result = testRunner.run(self.test)
  File "/usr/lib/python2.7/unittest/runner.py", line 151, in run
    test(result)
  File "/usr/lib/python2.7/unittest/suite.py", line 70, in __call__
    return self.run(*args, **kwds)
  File "/usr/lib/python2.7/unittest/suite.py", line 108, in run
    test(result)
  File "/usr/lib/python2.7/unittest/suite.py", line 70, in __call__
    return self.run(*args, **kwds)
  File "/home/user/Workspace/t11051/trac-trunk/trac/test.py", line 143, in run
    self.tearDown()
  File "/home/user/Workspace/t11051/trac-trunk/trac/tests/functional/__init__.py", line 126, in tearDown
    self._testenv.stop()
  File "/home/user/Workspace/t11051/trac-trunk/trac/tests/functional/testenv.py", line 240, in stop
    os.kill(self.pid, signal.SIGTERM)
OSError: [Errno 3] No such process

I also did some manual testing of TracFineGrainedPermissions. When anonymous user has ROADMAP_VIEW but not MILESTONE_VIEW and the following authz configuration is in place, only Milestone1 is seen on the roadmap, as we would expect.

[milestone:milestone1@*]
* = MILESTONE_VIEW

Other changes:

• Moved call_in_workdir from testenv.py to the subclass in svntestenv.py. It was accessing a member variable in the subclass so it didn't make any sense to have the method in the superclass.
• Added a utility method call_in_dir to testenv.py which I think will be useful in the future, particularly when adding tests for TracFineGrainedPermissions (which is something I had started on in this ticket before realizing how much work it will be and deciding it was not necessary for this patch; however, I will do some work to that end in #11067, #11069 and other authzpolicy tickets).

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

I suppose this ticket is not directly related to authzpolicy.

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

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

Other changes:

• Moved call_in_workdir from testenv.py to the subclass in svntestenv.py. It was accessing a member variable in the subclass so it didn't make any sense to have the method in the superclass.
• Added a utility method call_in_dir to testenv.py which I think will be useful in the future, particularly when adding tests for TracFineGrainedPermissions (which is something I had started on in this ticket before realizing how much work it will be and deciding it was not necessary for this patch; however, I will do some work to that end in #11067, #11069 and other authzpolicy tickets).

These changes are unrelated to the subject of this ticket, so I can break them out into a separate patch if you'd like (per TracDev/SubmittingPatches#Whatisagoodpatch - no unrelated changes). I also removed the #!/usr/bin/python from the files that have test classes and therefore wouldn't be executed directly, and that change isn't related to the fix so could also be part of another patch.

The other changes to the tester and testenv where necessary to accomplish the fix in this ticket, but I could provide those in a separate patch too if that would make testing and integration easier.

[Christian Boos]

I spent some time today to review some related patches, here and on #10984 and #10957.

I came up with the following series of changes:

• [9825ddf6/cboos.git] Added grant_perm and revoke_perm methods to the FunctionalTestEnvironment class.
• [afa91f16/cboos.git] testenv: use grant_perm instead of _tracadmin
• [718ca987/cboos.git] testenv: add copyright notice and remove #!/usr/bin/python line.
• [5e8f97ad/cboos.git] testenv: refactor call_in_workdir into call_in_dir.
• [d99e2ae1/cboos.git] tester: add go_to_url == go + url check + "no internal, thank you".
• [84fc4291/cboos.git] 1.0.2dev: refuse to show the /roadmap if ROADMAP_VIEW is missing
• [f7ca825e/cboos.git] testenv: revoke_perm does a config touch to force reload of perms.

In particular, the first was cherry-picked from ​4d83b31, the intermediate ones are piece-wise application of the patch from this ticket, and the last one is an addition of mine which sheds some light on the revoke issue you had. If the above is OK for you, I'll commit these and finish up those tickets.

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

Thanks for cleaning up that mash of patches. I should learn to use Git and make things easier for myself and others!

I have a small, yet to be committed change to grant_perm / revoke_perm. I realized when reading TracPermissions#GrantingPrivileges yesterday, that we should be able to expand the list/tuple to a space-separated string, and only call trac-admin once.

We can replace:

for p in perm: 
    self._tracadmin('permission', 'add', user, p) 

with:

self._tracadmin('permission', 'add', user, ' '.join(perm))

Should self.get_trac_environment().config.touch() be called in grant_perm as well rather than self.restart()?

The use of try/finally looks smart. I think that will also be useful to apply in some of the other pending patches that revoke and grant perms.

[Christian Boos]

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

Thanks for cleaning up that mash of patches. I should learn to use Git and make things easier for myself and others!

Well, git add -p is really handy for turning a patch with several different topics into a clean sequence of commits ;-)

[…] we should be able to expand the list/tuple to a space-separated string, and only call trac-admin once.

with:

self._tracadmin('permission', 'add', user, ' '.join(perm))

Or:

self._tracadmin('permission', 'add', user, *perm)

(untested)

Should self.get_trac_environment().config.touch() be called in grant_perm as well rather than self.restart()?

Ah right, didn't see the self.restart() there. And it's strange it was a problem for me in revoke_perm and not in grant_perm. I didn't spend time digging on the error yet.

The use of try/finally looks smart. I think that will also be useful to apply in some of the other pending patches that revoke and grant perms.

<git-tips> You can simply add my repository as a remote in your working copy, then do a fetch, make the modifications mentioned above and then rebase your other patches above this branch (keeping them as separate branches or not).

For example:

$ git remote add cboos http://svn.edgewall.org/git/trac/devs/cboos
$ git fetch cboos ryano-integration-r11697:t11028
$ git checkout -b t10957.2 t10957
$ git rebase trunk t10957.2 --onto t11028

• never rebase an already published branch, hence the creation of a .2 branch
• note that my repos:cboos.git@ryano-integration-r11697 branch was rebased on 1.0-stable, but your t10957 is based on trunk; that's why the more complex syntax of rebase is needed (had it been already based on 1.0-stable, it would have been a simple matter of git rebase t11028 t10957)

</git-tips>

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

Thanks for the tips! I'm making the changes we've discussed and should have them pushed by this evening.

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

Replying to cboos:

Ah right, didn't see the self.restart() there. And it's strange it was a problem for me in revoke_perm and not in grant_perm. I didn't spend time digging on the error yet.

I was seeing the same behavior.

The new branch with the patches from #10957 rebased can be found ​here.

• ​1435d191: Modified grant_perm / revoke_perm so that trac-admin is only called once.
• ​fbbf8305: self.get_trac_environment().config.touch() is called in grant_perm rather than self.restart().
• ​3f3b5ac0: use try/finally in test case. I wrapped the entire test, starting from the logout, to make sure that the environment state is restored at the end of the test case in case of a failure at any line. Should we do the same for RegressionTestTicket11028: ​c835331d? (the diff on both of those looks complex, but the only changes are the addition/move of a try/finally and indentation.)
• ​01e5283a: Add error message is included in the exception text to help with debugging errors when calling trac-admin.

There's still a problem with the last change. _trac_admin will throw an exception if revoking a permission that the user doesn't have. Should we cause grant_perm and revoke_perm to pass silently in the presence of errors like this?

In addition to your tips, the content at TracTeam/Repositories was very helpful. Now that I know how to add remotes and rebase changes, I'll post all my patches through git rather than as unified diffs.

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

I can't figure out what is going on here. The changes I listed above aren't shown when I view the ​commit log for the t10957.2 branch. The commits seems to be associated with tree: 1435d19133 … whatever that means.

[Christian Boos]

I'm not sure but it looks like you pushed a detached HEAD… If you still have ​1435d191 in your local repository, then simply do git checkout -b t10957.3 ​1435d191 and push that ref, so I can pull it. I couldn't fetch directly ​1435d191 (well, it's certainly possible, but my git fu ends here at this time ;-) ).

[Christian Boos]

So… what now? This one look quite central to a number of your other changes. As I explained in comment:15, if you don't associate a ref to your commits, I won't be able to fetch these changes. Another possibility is that I commit my changes (comment:9) and you restart on those if needed.

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

Sorry for the delay. I'm not sure exactly how the issue has been resolved, but after trying a few things and doing a push operation, the commits now appear in the t10957.2, so I expect you can probably fetch them now.

[Christian Boos]

Committed first set of refactorings in r11761.

I then noticed that there was a tracd server still running after the tests are finished… Re-running the functional tests a second time will then fail.

Looking closer, it's the restart done in grant_perm which is called even there's no server started yet (from FunctionalTestEnvironment.create). Even though we're going to replace restart with config.touch later on, I think it was better to fix that first (r11762).

[Christian Boos]

• replacement of restart with config.touch committed in r11764
• fix for this issue committed in r11765
• regression test improvement committed in r11766

Other changesets mentioned in comment:13 will be dealt with part of #10957.

[Christian Boos]

Thanks!