Edgewall Software
Modify

Opened 3 years ago

Closed 2 years ago

#12649 closed enhancement (fixed)

Simplify configuration parsing in svn_authz module

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed case-insensitive usernames in AuthzPolicy, a regression introduced in Trac 1.2.

API Changes:
  • Refactored svn_authz module to use UnicodeConfigParser.
  • Added ignorecase_option to UnicodeConfigParser (applied to 1.2-stable).

Description

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

Attachments (1)

t12649_case_sensitive.diff (5.7 KB ) - added by Ryan J Ollos 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Ryan J Ollos, 3 years ago

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.

comment:2 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in [15345:15346].

comment:4 by Jun Omae, 3 years ago

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}}}

comment:6 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: closedreopened

in reply to:  5 comment:7 by Ryan J Ollos, 3 years ago

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

    diff --git a/trac/versioncontrol/svn_authz.py b/trac/versioncontrol/svn_authz.py
    index be61197eb..1ee799b8f 100644
    a b def parse(authz_file, modules):  
    4646    are retained.
    4747    """
    4848    parser = UnicodeConfigParser()
     49    parser.optionxform = str
    4950    parser.read(authz_file)
    5051
    5152    groups = {}
  • trac/versioncontrol/tests/svn_authz.py

    diff --git a/trac/versioncontrol/tests/svn_authz.py b/trac/versioncontrol/tests/svn_authz.py
    index b7b33a0df..904e5f381 100644
    a b foo = rw  
    6161; Unicode module names
    6262[module:/c/résumé]
    6363bar = rw
     64Foo = rw
     65BAZ = r
    6466
    6567; Unused module, not parsed
    6668[unused:/some/path]
    foo = r  
    8890                },
    8991                u'/c/résumé': {
    9092                    u'bar': True,
     93                    u'Foo': True,
     94                    u'BAZ': True,
    9195                },
    9296            },
    9397        }, authz)
    user =  
    191195[scoped:/scope/dir1]
    192196joe = r
    193197[scoped:/scope/dir2]
    194 jane = r
     198Jane = r
    195199
    196200# multiple entries
    197201[/multiple]
    $authenticated =  
    203207[/multiple/bar]
    204208* =
    205209john = r
    206 jane = r
     210Jane = r
    207211$anonymous = r
    208212[/multiple/baz]
    209213$anonymous = r
    210214* =
    211 jane = r
     215Jane = r
    212216[module:/multiple/bar]
    213217joe = r
    214218john =
    joe = r  
    359363joe = r
    360364denied =
    361365[module:/otherpath]
    362 jane = r
     366Jane = r
    363367$anonymous = r
    364368[inactive:/not-in-this-instance]
    365369unknown = r
    unknown = r  
    370374        self.assertRevPerm(None, 'denied')
    371375        self.assertPathPerm(True, 'joe')
    372376        self.assertRevPerm(True, 'joe')
    373         self.assertPathPerm(True, 'jane')
    374         self.assertRevPerm(True, 'jane')
     377        self.assertPathPerm(True, 'Jane')
     378        self.assertRevPerm(True, 'Jane')
    375379        self.assertPathPerm(True, 'anonymous')
    376380        self.assertRevPerm(True, 'anonymous')
    377381
    378382    def test_default_permission(self):
    379383        # By default, permissions are undecided
    380384        self.assertPathPerm(None, 'joe', '', '/not_defined')
    381         self.assertPathPerm(None, 'jane', 'repo', '/not/defined/either')
     385        self.assertPathPerm(None, 'Jane', 'repo', '/not/defined/either')
    382386
    383387    def test_read_write(self):
    384388        # Allow 'r' and 'rw' entries, deny 'w' and empty entries
    unknown = r  
    415419        # The * wildcard matches all users, including anonymous
    416420        self.assertPathPerm(True, 'anonymous', '', '/wildcard')
    417421        self.assertPathPerm(True, 'joe', '', '/wildcard')
    418         self.assertPathPerm(True, 'jane', '', '/wildcard')
     422        self.assertPathPerm(True, 'Jane', '', '/wildcard')
    419423
    420424    def test_special_tokens(self):
    421425        # The $anonymous token matches only anonymous users
    unknown = r  
    424428        # The $authenticated token matches all authenticated users
    425429        self.assertPathPerm(None, 'anonymous', '', '/special/authenticated')
    426430        self.assertPathPerm(True, 'joe', '', '/special/authenticated')
    427         self.assertPathPerm(True, 'jane', '', '/special/authenticated')
     431        self.assertPathPerm(True, 'Jane', '', '/special/authenticated')
    428432
    429433    def test_groups(self):
    430434        # Groups are specified in a separate section and used with an @ prefix
    unknown = r  
    460464        self.assertPathPerm(True, 'joe', 'scoped', '/dir1')
    461465        self.assertPathPerm(None, 'joe', 'scoped', '/dir2')
    462466        self.assertPathPerm(True, 'joe', 'scoped', '/')
    463         self.assertPathPerm(None, 'jane', 'scoped', '/dir1')
    464         self.assertPathPerm(True, 'jane', 'scoped', '/dir2')
    465         self.assertPathPerm(True, 'jane', 'scoped', '/')
     467        self.assertPathPerm(None, 'Jane', 'scoped', '/dir1')
     468        self.assertPathPerm(True, 'Jane', 'scoped', '/dir2')
     469        self.assertPathPerm(True, 'Jane', 'scoped', '/')
    466470
    467471    def test_multiple_entries(self):
    468472        self.assertPathPerm(True,  'anonymous', '',       '/multiple/foo')
    unknown = r  
    471475        self.assertPathPerm(False, 'joe',       '',       '/multiple/bar')
    472476        self.assertPathPerm(True,  'john',      '',       '/multiple/bar')
    473477        self.assertPathPerm(True,  'anonymous', '',       '/multiple/baz')
    474         self.assertPathPerm(True,  'jane',      '',       '/multiple/baz')
     478        self.assertPathPerm(True,  'Jane',      '',       '/multiple/baz')
    475479        self.assertPathPerm(False, 'joe',       '',       '/multiple/baz')
    476480        self.assertPathPerm(True,  'anonymous', 'module', '/multiple/foo')
    477481        self.assertPathPerm(True,  'joe',       'module', '/multiple/foo')
    unknown = r  
    479483        self.assertPathPerm(True,  'joe',       'module', '/multiple/bar')
    480484        self.assertPathPerm(False, 'john',      'module', '/multiple/bar')
    481485        self.assertPathPerm(True,  'anonymous', 'module', '/multiple/baz')
    482         self.assertPathPerm(True,  'jane',      'module', '/multiple/baz')
     486        self.assertPathPerm(True,  'Jane',      'module', '/multiple/baz')
    483487        self.assertPathPerm(False, 'joe',       'module', '/multiple/baz')
    484488
    485489    def test_multiple_entries_with_module_and_parent_directory(self):
    unknown = r  
    527531        self.assertRevPerm(True, 'joe', 'scoped', 123)
    528532        self.assertRevPerm(None, 'joe', 'scoped', 456)
    529533        self.assertRevPerm(True, 'joe', 'scoped', 789)
    530         self.assertRevPerm(None, 'jane', 'scoped', 123)
    531         self.assertRevPerm(True, 'jane', 'scoped', 456)
    532         self.assertRevPerm(True, 'jane', 'scoped', 789)
     534        self.assertRevPerm(None, 'Jane', 'scoped', 123)
     535        self.assertRevPerm(True, 'Jane', 'scoped', 456)
     536        self.assertRevPerm(True, 'Jane', 'scoped', 789)
    533537        self.assertRevPerm(None, 'user', 'scoped', 123)
    534538        self.assertRevPerm(None, 'user', 'scoped', 456)
    535539        self.assertRevPerm(True, 'user', 'scoped', 789)

by Ryan J Ollos, 3 years ago

Attachment: t12649_case_sensitive.diff added

comment:8 by Jun Omae, 3 years ago

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

comment:9 by Ryan J Ollos, 3 years ago

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

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

comment:10 by Christian Boos, 3 years ago

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.

comment:11 by Jun Omae, 3 years ago

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}}}

comment:12 by Christian Boos, 3 years ago

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.

comment:13 by Ryan J Ollos, 3 years ago

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.

comment:14 by Ryan J Ollos, 3 years ago

Status: reopenedassigned

in reply to:  11 comment:15 by Ryan J Ollos, 2 years ago

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.

comment:16 by Ryan J Ollos, 2 years ago

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

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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