Version control folder permissions

[chamith.malinda@…]

Hi,

I want to restrict several folders to several trac users. So I used FineGrainedPermissions.

If we say john not allowed to view the trunk/src/some/location then I put below in the /var/www/trac/conf/authzpolicy.conf file.

[repository:test_repo@*/source:trunk/src/some/location/*@*]
john = !BROWSER_VIEW, !FILE_VIEW

But it not getting effected. (I already give the permissions to group which the john also in for BROWSER_VIEW and FILE_VIEW in admin panel permissions) I need restrict the trunk/src/some/location folder only to john.

[Remy Blank]

You should use AuthzSourcePolicy for controlling access to repositories, not AuthzPolicy.

[Christian Boos]

Theoretically AuthzPolicy should be able to handle this as well…

[theYT <dev@…>]

I think it is needed to check BROWSER_VIEW permission with resource when processing request.

Patch (tested with r12742):

trac/versioncontrol/web_ui/browser.py

@@ -326 +326 @@
             return True
 
     def process_request(self, req):
-        req.perm.require('BROWSER_VIEW')
-
         presel = req.args.get('preselected')
         if presel and (presel + '/').startswith(req.href.browser() + '/'):
             req.redirect(presel)
@@ -389 +387 @@
                                                    version=rev_or_latest))
             display_rev = repos.display_rev
 
+        if node:
+            req.perm(node.resource).require('BROWSER_VIEW')
+        else:
+            req.perm.require('BROWSER_VIEW')
+
         # Prepare template data
         path_links = get_path_links(req.href, reponame, path, rev,
                                     order, desc)

[Ryan J Ollos]

Defect confirmed.

It is not entirely clear to me what the allowed realms should be. On the one hand we have two aliases for source TracLinks: TracLinks. However, browser is not registered as a realm with the ResourceManager: tags/trac-1.0.1/trac/versioncontrol/api.py@:366#L364​.

With the following change,

trac/versioncontrol/web_ui/browser.py

@@ -326 +326 @@
             return True
 
     def process_request(self, req):
-        req.perm.require('BROWSER_VIEW')
-
         presel = req.args.get('preselected')
         if presel and (presel + '/').startswith(req.href.browser() + '/'):
             req.redirect(presel)
@@ -341 +339 @@
         desc = 'desc' in req.args
         xhr = req.get_header('X-Requested-With') == 'XMLHttpRequest'
 
+        req.perm('source', path).require('BROWSER_VIEW')
+
         rm = RepositoryManager(self.env)
         all_repositories = rm.get_all_repositories()
         reponame, repos, path = rm.get_repository_by_path(path)

we get a PermissionError with a proper message:

BROWSER_VIEW privileges are required to perform this operation on path /teo/trac. You don't have the required permissions.

If we change the realm of the permission check to the repository realm the message isn't quite as good:

BROWSER_VIEW privileges are required to perform this operation on Repository /teo/trac. You don't have the required permissions.

If browser was returned by the ResourceManager, we could check all 3 realms:

trac/versioncontrol/web_ui/browser.py

@@ -326 +326 @@
             return True
 
     def process_request(self, req):
-        req.perm.require('BROWSER_VIEW')
-
         presel = req.args.get('preselected')
         if presel and (presel + '/').startswith(req.href.browser() + '/'):
             req.redirect(presel)
@@ -342 +340 @@
         xhr = req.get_header('X-Requested-With') == 'XMLHttpRequest'
 
         rm = RepositoryManager(self.env)
+        for realm in rm.get_resource_realms():
+            req.perm(realm, path).require('BROWSER_VIEW')
+
         all_repositories = rm.get_all_repositories()
         reponame, repos, path = rm.get_repository_by_path(path)

However, I tend to think we should just use source as the realm for TracFineGrainedPermissions checks. Please let me know if you have other opinions on how the fix should be implemented.

[Jun Omae]

We could use is_viewable() method of Repository and Node classes to check viewable rather than directly using req.perm and the realms.

See tags/trac-1.0.1/trac/versioncontrol/api.py#L960​ and #L1078​.

trac/versioncontrol/web_ui/browser.py

@@ -24 +24 @@
 from trac.config import ListOption, BoolOption, Option
 from trac.core import *
 from trac.mimeview.api import IHTMLPreviewAnnotator, Mimeview, is_binary
-from trac.perm import IPermissionRequestor
+from trac.perm import IPermissionRequestor, PermissionError
 from trac.resource import Resource, ResourceNotFound
 from trac.util import as_bool, embedded_numbers
 from trac.util.datefmt import http_date, to_datetime, utc
@@ -326 +326 @@
             return True
 
     def process_request(self, req):
-        req.perm.require('BROWSER_VIEW')
-
         presel = req.args.get('preselected')
         if presel and (presel + '/').startswith(req.href.browser() + '/'):
             req.redirect(presel)
@@ -355 +353 @@
         if not repos and reponame:
             raise ResourceNotFound(_("Repository '%(repo)s' not found",
                                      repo=reponame))
+        if not repos.is_viewable(req.perm):
+            raise PermissionError('BROWSER_VIEW', repos.resource, self.env)
 
         if reponame and reponame != repos.reponame: # Redirect alias
             qs = req.query_string
@@ -398 +398 @@
             repo_data = self._render_repository_index(
                                         context, all_repositories, order, desc)
         if node:
+            if not node.is_viewable(req.perm):
+                raise PermissionError('BROWSER_VIEW' if node.isdir else
+                                      'FILE_VIEW', node.resource, self.env)
             if node.isdir:
                 if format in ('zip',): # extension point here...
                     self._render_zip(req, context, repos, node, rev)

[Ryan J Ollos]

Using the is_viewable methods looks like the way to go. Please feel free to take control of the ticket and push the change.

[Jun Omae]

Okay. I'll push the patch with tests.

[Jun Omae]

Proposed changes in jomae.git@t10961_1.0.

BTW, I noticed that authz policy doesn't work for default repository with like this. That's a defect?

[repository:@*]
foobar = !BROWSER_VIEW, !FILE_VIEW

[repository:(default)@*]
foobar = !BROWSER_VIEW, !FILE_VIEW

The following works for default repository but makes all repositories to be not viewable.

[repository:*@*]
foobar = !BROWSER_VIEW, !FILE_VIEW

[Jun Omae]

Committed in [12814] and merged to trunk in [12815].

[theYT <dev@…>]

I think permission check to the repository is redundant. (Line 357-358​)

After [12815], if administrator wants to allow someone access to the specific resource only (not all repository resources), the user should also have permission to the repository (root).

Example: ('user1' does not have BROWSER_VIEW, FILE_VIEW permissions as default)

[repository:test_repo@*/source:path/to/resource@*]
user1 = BROWSER_VIEW, FILE_VIEW

below lines are also required to access the resource.

[repository:test_repo@*]
user1 = BROWSER_VIEW

Also, if user doesn't have permission to access any repository, No node / error messege was shown when accessing page /browser. (Previously, 'BROWSER_VIEW privileges are required…' message was shown.)

(Comment 10:)
If administrator adds default repository as alias to the named repository, authz policy would work.

[Jun Omae]

I think permission check to the repository is redundant. (Line 357-358​)

According to TracFineGrainedPermissions#UsageNotes for repository, it seems indeed.

(Comment 10:)
If administrator adds default repository as alias to the named repository, authz policy would work.

I know. That is just a workaround. But it isn't solved if a repository has no name.

[Jun Omae]

Replying to jomae:

I think permission check to the repository is redundant. (Line 357-358​)

According to TracFineGrainedPermissions#UsageNotes for repository, it seems indeed.

I worked for the unintended changes in jomae.git@t10961.1. Could you please try it?

(Comment 10:)
If administrator adds default repository as alias to the named repository, authz policy would work.

I know. That is just a workaround. But it isn't solved if a repository has no name.

I've worked for the behavior in jomae.git@t11293.4. In the branch, the following rule works well.

[repository:@*]
foobar = !BROWSER_VIEW, !FILE_VIEW

[theYT <dev@…>]

Replying to jomae:

Both revisions (jomae.git@t10961.1, jomae.git@t11293.4) were tested and worked as I expected. Thanks!

[Jun Omae]

Thanks for the testing. Committed jomae.git@t10961.1 in [12838] and merged to trunk in [12839].

[Peter Suter]

Replying to jomae:

[12838] and merged to trunk in [12839].

After this I get test errors when ConfigObj is not installed:

D:\Code\Trac\trunk> python trac\versioncontrol\web_ui\tests\browser.py
SKIP: fine-grained permission tests (ConfigObj not installed)
EE..FEEEEEE
======================================================================
ERROR: test_get_navigation_items_with_browser_view (__main__.BrowserModulePermissionsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac\versioncontrol\web_ui\tests\browser.py", line 129, in test_get_navigation_items_with_browser_view
    self.assertEqual('browser', self.get_navigation_items(req).next()[1])
  File "d:\code\trac\trunk\trac\versioncontrol\web_ui\browser.py", line 298, in get_navigation_items
    in rm.get_real_repositories()):
  File "d:\code\trac\trunk\trac\versioncontrol\web_ui\browser.py", line 297, in <genexpr>
    if any(repos.is_viewable(req.perm) for repos
  File "d:\code\trac\trunk\trac\versioncontrol\api.py", line 1011, in is_viewable
    return 'BROWSER_VIEW' in perm(self.resource.child('source', '/'))
  File "d:\code\trac\trunk\trac\perm.py", line 549, in has_permission
    return self._has_permission(action, resource)
  File "d:\code\trac\trunk\trac\perm.py", line 563, in _has_permission
    check_permission(action, perm.username, resource, perm)
  File "d:\code\trac\trunk\trac\perm.py", line 455, in check_permission
    perm)
  File "d:\code\trac\trunk\tracopt\perm\authz_policy.py", line 145, in check_permission
    self.parse_authz()
  File "d:\code\trac\trunk\tracopt\perm\authz_policy.py", line 188, in parse_authz
    raise ConfigurationError()
ConfigurationError: Look in the Trac log for more information.

ERROR: test_get_navigation_items_without_browser_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_no_viewable_repositories_without_browser_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_node_in_allowed_repos_with_file_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_node_in_denied_repos_with_file_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_node_with_file_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_repository_with_browser_view (__main__.BrowserModulePermissionsTestCase)
ERROR: test_repository_without_browser_view (__main__.BrowserModulePermissionsTestCase)
----------------------------------------------------------------------
Ran 11 tests in 0.102s

FAILED (failures=1, errors=8)

(After easy_install ConfigObj all is OK.)

[Jun Omae]

Replying to psuter:

After this I get test errors when ConfigObj is not installed: […]

Thanks for the catching. I didn't run tests without configobj. Fixed in [12950] and merged in [12951].