Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#10961 closed defect (fixed)

Version control folder permissions

Reported by: chamith.malinda@… Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: version control/browser Version: 0.12
Severity: normal Keywords: authzpolicy
Cc: dev@…
Release Notes:

Check permissions for Repository and Node using is_viewable method in order to work with authz policy.

API Changes:

Description (last modified by Ryan J Ollos)

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.

Attachments (0)

Change History (19)

comment:1 Changed 6 years ago by Remy Blank

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

comment:2 Changed 6 years ago by Christian Boos

Keywords: authzpolicy verify added
Milestone: next-stable-1.0.x

Theoretically AuthzPolicy should be able to handle this as well…

comment:3 Changed 6 years ago by Christian Boos

Description: modified (diff)

comment:4 Changed 5 years ago by theYT <dev@…>

Cc: dev@… added

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

     
    326326            return True
    327327
    328328    def process_request(self, req):
    329         req.perm.require('BROWSER_VIEW')
    330 
    331329        presel = req.args.get('preselected')
    332330        if presel and (presel + '/').startswith(req.href.browser() + '/'):
    333331            req.redirect(presel)
     
    389387                                                   version=rev_or_latest))
    390388            display_rev = repos.display_rev
    391389
     390        if node:
     391            req.perm(node.resource).require('BROWSER_VIEW')
     392        else:
     393            req.perm.require('BROWSER_VIEW')
     394
    392395        # Prepare template data
    393396        path_links = get_path_links(req.href, reponame, path, rev,
    394397                                    order, desc)

comment:5 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)

comment:6 Changed 5 years ago by Ryan J Ollos

Keywords: verify removed
Owner: set to Ryan J Ollos
Status: newassigned

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

    diff --git a/trac/versioncontrol/web_ui/browser.py b/trac/versioncontrol/web_ui/
    index b07466e..030854f 100644
    a b class BrowserModule(Component):  
    326326            return True
    327327
    328328    def process_request(self, req):
    329         req.perm.require('BROWSER_VIEW')
    330 
    331329        presel = req.args.get('preselected')
    332330        if presel and (presel + '/').startswith(req.href.browser() + '/'):
    333331            req.redirect(presel)
    class BrowserModule(Component):  
    341339        desc = 'desc' in req.args
    342340        xhr = req.get_header('X-Requested-With') == 'XMLHttpRequest'
    343341
     342        req.perm('source', path).require('BROWSER_VIEW')
     343
    344344        rm = RepositoryManager(self.env)
    345345        all_repositories = rm.get_all_repositories()
    346346        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

    diff --git a/trac/versioncontrol/web_ui/browser.py b/trac/versioncontrol/web_ui/
    index b07466e..c49d7b5 100644
    a b class BrowserModule(Component):  
    326326            return True
    327327
    328328    def process_request(self, req):
    329         req.perm.require('BROWSER_VIEW')
    330 
    331329        presel = req.args.get('preselected')
    332330        if presel and (presel + '/').startswith(req.href.browser() + '/'):
    333331            req.redirect(presel)
    class BrowserModule(Component):  
    342340        xhr = req.get_header('X-Requested-With') == 'XMLHttpRequest'
    343341
    344342        rm = RepositoryManager(self.env)
     343        for realm in rm.get_resource_realms():
     344            req.perm(realm, path).require('BROWSER_VIEW')
     345
    345346        all_repositories = rm.get_all_repositories()
    346347        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.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:7 Changed 5 years ago by 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

    diff --git a/trac/versioncontrol/web_ui/browser.py b/trac/versioncontrol/web_ui/browser.py
    index b07466e..fb20174 100644
    a b from genshi.builder import tag  
    2424from trac.config import ListOption, BoolOption, Option
    2525from trac.core import *
    2626from trac.mimeview.api import IHTMLPreviewAnnotator, Mimeview, is_binary
    27 from trac.perm import IPermissionRequestor
     27from trac.perm import IPermissionRequestor, PermissionError
    2828from trac.resource import Resource, ResourceNotFound
    2929from trac.util import as_bool, embedded_numbers
    3030from trac.util.datefmt import http_date, to_datetime, utc
    class BrowserModule(Component):  
    326326            return True
    327327
    328328    def process_request(self, req):
    329         req.perm.require('BROWSER_VIEW')
    330 
    331329        presel = req.args.get('preselected')
    332330        if presel and (presel + '/').startswith(req.href.browser() + '/'):
    333331            req.redirect(presel)
    class BrowserModule(Component):  
    355353        if not repos and reponame:
    356354            raise ResourceNotFound(_("Repository '%(repo)s' not found",
    357355                                     repo=reponame))
     356        if not repos.is_viewable(req.perm):
     357            raise PermissionError('BROWSER_VIEW', repos.resource, self.env)
    358358
    359359        if reponame and reponame != repos.reponame: # Redirect alias
    360360            qs = req.query_string
    class BrowserModule(Component):  
    398398            repo_data = self._render_repository_index(
    399399                                        context, all_repositories, order, desc)
    400400        if node:
     401            if not node.is_viewable(req.perm):
     402                raise PermissionError('BROWSER_VIEW' if node.isdir else
     403                                      'FILE_VIEW', node.resource, self.env)
    401404            if node.isdir:
    402405                if format in ('zip',): # extension point here...
    403406                    self._render_zip(req, context, repos, node, rev)
Last edited 5 years ago by Jun Omae (previous) (diff)

comment:8 Changed 5 years ago by 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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:9 Changed 5 years ago by Jun Omae

Owner: changed from Ryan J Ollos to Jun Omae

Okay. I'll push the patch with tests.

comment:10 Changed 5 years ago by 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

comment:11 Changed 5 years ago by Jun Omae

Milestone: next-stable-1.0.x1.0.2

comment:12 Changed 5 years ago by Jun Omae

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:13 in reply to:  10 ; Changed 5 years ago by 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.

comment:14 in reply to:  13 ; Changed 5 years ago by 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.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:15 in reply to:  14 ; Changed 5 years ago by Jun Omae

Resolution: fixed
Status: closedreopened

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

comment:16 in reply to:  15 Changed 5 years ago by theYT <dev@…>

Replying to jomae:

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

comment:17 Changed 5 years ago by Jun Omae

Resolution: fixed
Status: reopenedclosed

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

comment:18 in reply to:  17 ; Changed 4 years ago by 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.)

comment:19 in reply to:  18 Changed 4 years ago by 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].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.