Edgewall Software
Modify

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#11744 closed defect (fixed)

[PATCH] permissions determined by svn_authz aren't the same as determined by Subversion

Reported by: ash@… Owned by: Jun Omae
Priority: normal Milestone: 1.0.14
Component: version control/browser Version: 1.0.1
Severity: normal Keywords: svnauthz authzsourcepolicy
Cc: Branch:
Release Notes:

Fix inconsistency between AuthzSourcePolicy and svnauthz file.

API Changes:
Internal Changes:

Description

It's necessary for me to restrict access to areas of the Subversion repository using a Subversion path-based authorization file. Therefore, I need Trac to restrict access in the same way.

However, the Trac AuthzSourcePolicy doesn't interpret the authorization file in the same way that Subversion does. Specifically, Subversion will check all the entries for a particular path and grant access to a particular entity if any of those entries gives permission. Trac on the other hand uses the permissions for the first entry that matches and doesn't check subsequent entries.

Example: if I have a file along the lines of

[groups]
everyone = dave.public, joe.private

[repo:/]
@everyone = rw

[repo:/hidden]
@everyone =
joe.private = rw

then Subversion would allow joe.private access to /hidden, but Trac would deny it. Obviously, I would like the visibility to Subversion clients to be the same as the visibility in the Trac source browser.

Using Trac 1.0.1 with Python 2.7.

The attached patch appears to fix the problem for me. So far, I have only tested it with a minimal configuration like the one above.

Attachments (1)

svn_authz.patch (608 bytes ) - added by ash@… 6 years ago.
Patch to make svn_authz behave more like Subversion

Download all attachments as: .zip

Change History (15)

by ash@…, 6 years ago

Attachment: svn_authz.patch added

Patch to make svn_authz behave more like Subversion

comment:1 by ash@…, 6 years ago

Keywords: authzsourcepolicy added

comment:2 by Jun Omae, 6 years ago

Keywords: svnauthz verify added; svn_authz removed
Milestone: undecided

comment:3 by Ryan J Ollos, 6 years ago

Keywords: patch added

comment:4 by ash@…, 4 years ago

Note that I have been using this patch for more than two years now with a folder permissions structure that is reasonably complex. I haven't observed any further differences in how Trac and Subversion handle permissions.

comment:5 by Jun Omae, 4 years ago

Milestone: undecidednext-stable-1.0.x
Owner: set to Jun Omae
Status: newassigned

I'll verify it. We should add unit tests for it.

comment:6 by Jun Omae, 4 years ago

In [c2ecc3ec0/jomae.git], applied the suggested patch and added unit tests. However, the unit tests fail. The patch is incomplete.

$ python setup.py -q test -s trac.versioncontrol.tests.svn_authz.suite
........F.......
======================================================================
FAIL: test_multiple_entries (trac.versioncontrol.tests.svn_authz.AuthzSourcePolicyTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/src/tracdev/git/trac/versioncontrol/tests/svn_authz.py", line 400, in test_multiple_entries
    self.assertPathPerm(True,  'joe',       '',      '/multiple/foo')
  File "/src/tracdev/git/trac/versioncontrol/tests/svn_authz.py", line 266, in assertPathPerm
    self.assertEqual(result, check)
AssertionError: True != False

----------------------------------------------------------------------
Ran 16 tests in 0.114s

FAILED (failures=1)

We could confirm behavior of svnauthz file using svn.repos.authz_check_access().

>>> from tracopt.versioncontrol.svn.svn_fs import _import_svn
>>> _import_svn()
>>> from tracopt.versioncontrol.svn.svn_fs import Pool, repos
>>> from trac.util.text import print_table
>>>
>>> with open('/tmp/svnauthz.txt', 'w') as f:
...     f.write("""\
... [/multiple]
... $authenticated = r
... [/multiple/foo]
... joe =
... $authenticated =
... * = r
... [/multiple/bar]
... * =
... john = r
... $anonymous = r
... [/multiple/baz]
... $anonymous = r
... * =
... jane = r
... [repos:/multiple/bar]
... joe = r
... john =
... """)
...
>>> def f():
...   pool = Pool()
...   authzdb = repos.authz_read('/tmp/svnauthz.txt', True, pool())
...   header = ['Path', 'anonymous', 'joe', 'john', 'jane', 'someone']
...   data = []
...   for repos_name in ('', 'repos'):
...     for path in ('/multiple/foo', '/multiple/bar', '/multiple/baz'):
...       line = [repos_name + ':' + path if repos_name else path]
...       line.extend(
...         repos.authz_check_access(authzdb, repos_name, path, user, repos.svn_authz_read, pool())
...         for user in (None, 'joe', 'john', 'jane', 'someone'))
...       data.append(line)
...   print_table(data, header)
...
>>> f()

Path                 anonymous  joe  john  jane  someone
--------------------------------------------------------
/multiple/foo        1          1    1     1     1
/multiple/bar        1          0    1     0     0
/multiple/baz        1          0    0     1     0
repos:/multiple/foo  1          1    1     1     1
repos:/multiple/bar  1          1    0     0     0
repos:/multiple/baz  1          0    0     1     0
Last edited 4 years ago by Jun Omae (previous) (diff)

comment:7 by Jun Omae, 4 years ago

Keywords: verify patch removed

comment:9 by ash@…, 4 years ago

Thanks for looking at this and fixing it. On my site, I haven't (until now, but I expect that to change in future) used repository names in front of the path names in the authorizations file, just the path names on their own. Is that sufficient to explain why I haven't seen the fault that your unit tests have revealed?

comment:10 by Jun Omae, 4 years ago

Before the changes, the policy component would allow john access to [repos:/path/to] with the following configuration:

[/path/to]
john = r
[repos:/path/to]
john =

However, svnauthz feature wouldn't allow john access to [repos:/path/to].

comment:11 by Jun Omae, 4 years ago

Milestone: next-stable-1.0.x1.0.14

Additional small changes in [30ed3e03f/jomae.git].

I'll push proposed changes if there are no objections from other developers.

comment:12 by Jun Omae, 4 years ago

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

Committed in [15300] and merged in [15301-15302].

comment:13 by Ryan J Ollos, 4 years ago

One small thing, on trunk we have mkdtemp, which could be used in [15301#file2].

comment:14 by Jun Omae, 4 years ago

Thanks for review and reminding. I forgot to execute svn ci on trunk ;-) I just pushed with your suggestion in [15302].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
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.