Simplify configuration parsing in svn_authz module

[Ryan J Ollos]

While working on #12637 I saw some places we could simplify the code and make the configuration parsing more robust.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12649_authzsourcepolicy_refactoring. The changes are based on the branch from #12637. Changes to parse and remove of join function are API changes, but in practice I don't expect a plugin would have any reason to use the functions.

[Ryan J Ollos]

Committed to trunk in [15345:15346].

[Jun Omae]

In Subversion and Trac, user name is case-sensitive by default. However, after changes, the user names are converted to lower case.

>>> from pprint import pprint
>>> from trac.versioncontrol.svn_authz import AuthzSourcePolicy
>>> from trac.versioncontrol.svn_authz import parse
>>> with open('/tmp/svnauthz.txt', 'w') as f:
...   f.write("""\
... [/path/to]
... foo1 = rw
... [mod:/path/to]
... foo2 = rw
... [/Path/To]
... Bar1 = rw
... [Mod:/Path/To]
... Bar2 = rw
... """)
...
>>> result = parse('/tmp/svnauthz.txt', ['', 'mod', 'Mod'])
>>> pprint(result)
{'': {u'/Path/To': {u'bar1': True}, u'/path/to': {u'foo1': True}},
 u'Mod': {u'/Path/To': {u'bar2': True}},
 u'mod': {u'/path/to': {u'foo2': True}}}

[Jun Omae]

See also ​python - ConfigParser reads capital keys and make them lower case - Stack Overflow.

[Ryan J Ollos]

Replying to Jun Omae:

See also ​python - ConfigParser reads capital keys and make them lower case - Stack Overflow.

Based on that, I'd propose the following patch, which passes tests on OSX:

trac/versioncontrol/svn_authz.py

@@ -46 +46 @@
     are retained.
     """
     parser = UnicodeConfigParser()
+    parser.optionxform = str
     parser.read(authz_file)
 
     groups = {}

trac/versioncontrol/tests/svn_authz.py

@@ -61 +61 @@
 ; Unicode module names
 [module:/c/résumé]
 bar = rw
+Foo = rw
+BAZ = r
 
 ; Unused module, not parsed
 [unused:/some/path]
@@ -88 +90 @@
                 },
                 u'/c/résumé': {
                     u'bar': True,
+                    u'Foo': True,
+                    u'BAZ': True,
                 },
             },
         }, authz)
@@ -191 +195 @@
 [scoped:/scope/dir1]
 joe = r
 [scoped:/scope/dir2]
-jane = r
+Jane = r
 
 # multiple entries
 [/multiple]
@@ -203 +207 @@
 [/multiple/bar]
 * =
 john = r
-jane = r
+Jane = r
 $anonymous = r
 [/multiple/baz]
 $anonymous = r
 * =
-jane = r
+Jane = r
 [module:/multiple/bar]
 joe = r
 john =
@@ -359 +363 @@
 joe = r
 denied =
 [module:/otherpath]
-jane = r
+Jane = r
 $anonymous = r
 [inactive:/not-in-this-instance]
 unknown = r
@@ -370 +374 @@
         self.assertRevPerm(None, 'denied')
         self.assertPathPerm(True, 'joe')
         self.assertRevPerm(True, 'joe')
-        self.assertPathPerm(True, 'jane')
-        self.assertRevPerm(True, 'jane')
+        self.assertPathPerm(True, 'Jane')
+        self.assertRevPerm(True, 'Jane')
         self.assertPathPerm(True, 'anonymous')
         self.assertRevPerm(True, 'anonymous')
 
     def test_default_permission(self):
         # By default, permissions are undecided
         self.assertPathPerm(None, 'joe', '', '/not_defined')
-        self.assertPathPerm(None, 'jane', 'repo', '/not/defined/either')
+        self.assertPathPerm(None, 'Jane', 'repo', '/not/defined/either')
 
     def test_read_write(self):
         # Allow 'r' and 'rw' entries, deny 'w' and empty entries
@@ -415 +419 @@
         # The * wildcard matches all users, including anonymous
         self.assertPathPerm(True, 'anonymous', '', '/wildcard')
         self.assertPathPerm(True, 'joe', '', '/wildcard')
-        self.assertPathPerm(True, 'jane', '', '/wildcard')
+        self.assertPathPerm(True, 'Jane', '', '/wildcard')
 
     def test_special_tokens(self):
         # The $anonymous token matches only anonymous users
@@ -424 +428 @@
         # The $authenticated token matches all authenticated users
         self.assertPathPerm(None, 'anonymous', '', '/special/authenticated')
         self.assertPathPerm(True, 'joe', '', '/special/authenticated')
-        self.assertPathPerm(True, 'jane', '', '/special/authenticated')
+        self.assertPathPerm(True, 'Jane', '', '/special/authenticated')
 
     def test_groups(self):
         # Groups are specified in a separate section and used with an @ prefix
@@ -460 +464 @@
         self.assertPathPerm(True, 'joe', 'scoped', '/dir1')
         self.assertPathPerm(None, 'joe', 'scoped', '/dir2')
         self.assertPathPerm(True, 'joe', 'scoped', '/')
-        self.assertPathPerm(None, 'jane', 'scoped', '/dir1')
-        self.assertPathPerm(True, 'jane', 'scoped', '/dir2')
-        self.assertPathPerm(True, 'jane', 'scoped', '/')
+        self.assertPathPerm(None, 'Jane', 'scoped', '/dir1')
+        self.assertPathPerm(True, 'Jane', 'scoped', '/dir2')
+        self.assertPathPerm(True, 'Jane', 'scoped', '/')
 
     def test_multiple_entries(self):
         self.assertPathPerm(True,  'anonymous', '',       '/multiple/foo')
@@ -471 +475 @@
         self.assertPathPerm(False, 'joe',       '',       '/multiple/bar')
         self.assertPathPerm(True,  'john',      '',       '/multiple/bar')
         self.assertPathPerm(True,  'anonymous', '',       '/multiple/baz')
-        self.assertPathPerm(True,  'jane',      '',       '/multiple/baz')
+        self.assertPathPerm(True,  'Jane',      '',       '/multiple/baz')
         self.assertPathPerm(False, 'joe',       '',       '/multiple/baz')
         self.assertPathPerm(True,  'anonymous', 'module', '/multiple/foo')
         self.assertPathPerm(True,  'joe',       'module', '/multiple/foo')
@@ -479 +483 @@
         self.assertPathPerm(True,  'joe',       'module', '/multiple/bar')
         self.assertPathPerm(False, 'john',      'module', '/multiple/bar')
         self.assertPathPerm(True,  'anonymous', 'module', '/multiple/baz')
-        self.assertPathPerm(True,  'jane',      'module', '/multiple/baz')
+        self.assertPathPerm(True,  'Jane',      'module', '/multiple/baz')
         self.assertPathPerm(False, 'joe',       'module', '/multiple/baz')
 
     def test_multiple_entries_with_module_and_parent_directory(self):
@@ -527 +531 @@
         self.assertRevPerm(True, 'joe', 'scoped', 123)
         self.assertRevPerm(None, 'joe', 'scoped', 456)
         self.assertRevPerm(True, 'joe', 'scoped', 789)
-        self.assertRevPerm(None, 'jane', 'scoped', 123)
-        self.assertRevPerm(True, 'jane', 'scoped', 456)
-        self.assertRevPerm(True, 'jane', 'scoped', 789)
+        self.assertRevPerm(None, 'Jane', 'scoped', 123)
+        self.assertRevPerm(True, 'Jane', 'scoped', 456)
+        self.assertRevPerm(True, 'Jane', 'scoped', 789)
         self.assertRevPerm(None, 'user', 'scoped', 123)
         self.assertRevPerm(None, 'user', 'scoped', 456)
         self.assertRevPerm(True, 'user', 'scoped', 789)

[Jun Omae]

AuthzSourcePolicy has the same issue. Proposed changes in jomae.git@t12649.1.

[Ryan J Ollos]

Changes look good. [9deaf78c/jomae.git] will probably be needed on 1.2-stable since UnicodeConfigParser replaced ConfigObj in #11982.

[Christian Boos]

I was surprised when seeing str here in your patch instead of unicode, but indeed the cast happens on the "raw" str in ConfigParser.

The tests pass for me on Windows as well, for the patch and for t12649.1.

[Jun Omae]

I noticed another issue. ConfigParser ignores a line started with rem as a comment. User named rem cannot be used in.

>>> from trac.versioncontrol.svn_authz import parse
>>> from trac.util import read_file
>>> print(read_file('/tmp/svnauthz.txt'))
[/path/to]
foo = rw
REM = rw
Rem = rw
rem = rw

>>> parse('/tmp/svnauthz.txt', [''])
{'': {u'/path/to': {u'foo': True}}}

[Christian Boos]

Well, considering all the features the standard ConfigParser has which we don't use or go in the way, it would probably be way more efficient to roll our own at this point.

[Ryan J Ollos]

I guess we would have the same problem if a plugin named an option rem. I favor either overriding the _read method, or copying the entire ConfigParser class as UnicodeConfigParser and modifying to fit our needs.

[Ryan J Ollos]

Replying to Jun Omae:

I noticed another issue. ConfigParser ignores a line started with rem as a comment. User named rem cannot be used in.

Created #12792.

[Ryan J Ollos]

Committed comment:8 changes to 1.2-stable in r15845, merged to trunk in r15846.