Improve error handling and reporting for AuthzPolicy

[Ryan J Ollos]

We can improve error handling and reporting for 3 scenarios that may be seen when using the AuthzPolicy​.

1. When an authz file has duplicated sections, Trac generates an internal error with a traceback:

   Traceback (most recent call last):
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/web/main.py", line 497, in _dispatch_request
       dispatcher.dispatch(req)
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/web/main.py", line 214, in dispatch
       resp = chosen_handler.process_request(req)
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/wiki/web_ui.py", line 126, in process_request
       req.perm(page.resource).require('WIKI_VIEW')
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/perm.py", line 578, in require
       if not self._has_permission(action, resource):
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/perm.py", line 570, in _has_permission
       check_permission(action, perm.username, resource, perm)
     File "/home/user/Workspace/t8976/teo-rjollos.git/trac/perm.py", line 462, in check_permission
       perm)
     File "/home/user/Workspace/t8976/teo-rjollos.git/tracopt/perm/authz_policy.py", line 147, in check_permission
       self.parse_authz()
     File "/home/user/Workspace/t8976/teo-rjollos.git/tracopt/perm/authz_policy.py", line 176, in parse_authz
       self.authz = ConfigObj(self.get_authz_file(), encoding='utf8')
     File "/usr/lib/python2.7/dist-packages/configobj.py", line 1230, in __init__
       self._load(infile, configspec)
     File "/usr/lib/python2.7/dist-packages/configobj.py", line 1320, in _load
       raise error
   ConfigObjError: Parsing failed with several errors.
   First error at line 4.
   

   Though the duplicate sections error is easy to reproduce, the proposed change should trap and present a user-friendly error message when any exception is thrown while ConfigObj is parsing the file (by trapping all ​ConfigObjErrors).
2. When the authz file is not found, Trac generates an internal error with a traceback:

   Traceback (most recent call last):
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/web/main.py", line 497, in _dispatch_request
       dispatcher.dispatch(req)
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/web/main.py", line 214, in dispatch
       resp = chosen_handler.process_request(req)
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/wiki/web_ui.py", line 126, in process_request
       req.perm(page.resource).require('WIKI_VIEW')
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/perm.py", line 578, in require
       if not self._has_permission(action, resource):
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/perm.py", line 570, in _has_permission
       check_permission(action, perm.username, resource, perm)
     File "/home/user/Workspace/t11260/teo-rjollos.git/trac/perm.py", line 462, in check_permission
       perm)
     File "/home/user/Workspace/t11260/teo-rjollos.git/tracopt/perm/authz_policy.py", line 149, in check_permission
       self.parse_authz()
     File "/home/user/Workspace/t11260/teo-rjollos.git/tracopt/perm/authz_policy.py", line 211, in parse_authz
       self.authz_mtime = self.get_authz_file_mtime
     File "/home/user/Workspace/t11260/teo-rjollos.git/tracopt/perm/authz_policy.py", line 178, in get_authz_file_mtime
       return os.path.getmtime(self.get_authz_file())
     File "/home/user/Workspace/t11260/lib/python2.7/genericpath.py", line 54, in getmtime
       return os.stat(filename).st_mtime
   OSError: [Errno 2] No such file or directory: '/home/user/Workspace/t11260/tracdev/conf/authzpolicy?.conf'
   

3. If there is an error in the configuration of permission policies, for example: permission_policies = AuthzPermissionPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy (the first entry should be AuthzPolicy), no error is reported. There should at least be a warning in the logs.

While ExtensionOption​ throw an exception when the specified implementation of the interface can't be found, OrderedExtensionOption​ does not. Changing the behavior of OrderedExtensionOption will have to be carefully considered.

[Ryan J Ollos]

Changes can be found in rjollos.git:t11272. The changes for (3) would best be discussed in #10285, where I already posted comments. I didn't realize the overlap until after I started working on the changes.

I ended up adding assertIs to another test class, so I wonder if we should do something to avoid the code duplication (mentioned in comment:19:ticket:11245) such as:

1. Subclass unittest.TestCase and add the methods.
2. Add unittest2 as a project dependency.

Feedback welcome and appreciated!

[Ryan J Ollos]

In order to avoid exposing information to users, a generic ConfigurationError is raised for the scenarios discussed in this ticket.

The latest changes can be found in rjollos.git:t11272.2.

[Ryan J Ollos]

Example of what would be found in the log file:

02:20:52 AM Trac[authz_policy] ERROR: Error parsing authz permission policy file: No such file or directory /home/user/Workspace/t11272/tracdev/conf/authz.con
02:20:52 AM Trac[chrome] ERROR: Error during check of EMAIL_VIEW: ConfigurationError: Look in the Trac log for more information.

Committed to 1.0-stable in [11971-11972], and merged to trunk in [11973].

I made an error and then reverted the committed, so [11969-11970] can be ignored. Should I modify the svn:mergeinfo on the trunk so that these two changesets aren't shown as eligible for merge?

[Christian Boos]

cd trunk; svn merge -r 11968-11970 ^/branches/1.0-stable --record-only

[Ryan J Ollos]

Record-only merge on trunk in [11974]. Thanks.

[Jun Omae]

The unit tests for authz_policy.py say the following message on Python 2.6. The tests say no warnings on 2.5 and 2.7.

$ PYTHONPATH=. ~/venv/py26/bin/python tracopt/perm/tests/authz_policy.py
../home/jun66j5/src/trac.git/tracopt/perm/authz_policy.py:193: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
  to_unicode(e.message))

We should use to_unicode(e) instead of to_unicode(e.message). Fixed in [11997].

[Ryan J Ollos]

Nice catch. I haven't been running the tests with Python 2.6 most of the time, thinking I'd catch everything by running the tests with with 2.5 and 2.7. From reading ​PEP:0352, I'd expect a warning in 2.7 as well.

There are some other instances of this in the codebase. I wonder if we should fix these as well:

user@ubuntu:~/Workspace/trac-1.0-stable$ grep -R "\be.message\b" . --exclude=*.pot --exclude-dir=.svn
./trac/mimeview/api.py:                self.log.warning("Can't use annotator '%s': %s", a, e.message)
./trac/mimeview/api.py:                         annotator=tag.em(a), error=tag.pre(e.message))))
./trac/timeline/web_ui.py:                return tag.a(label, title=to_unicode(e.message),
./trac/versioncontrol/api.py:                                error=to_unicode(e.message)))
./trac/versioncontrol/api.py:                          error=to_unicode(e.message)))
./trac/versioncontrol/web_ui/browser.py:                raise ResourceNotFound(e.message,
./trac/versioncontrol/web_ui/changeset.py:            raise ResourceNotFound(e.message, _('Invalid Changeset Number'))
./trac/versioncontrol/web_ui/util.py:            tag.p(e.message, class_="message"),

[Ryan J Ollos]

Replying to rjollos:

There are some other instances of this in the codebase. I wonder if we should fix these as well:

I'll post a patch for this in #11286.

[Jun Omae]

I caught another failure on Windows for non-English Edition:

ERROR: ConfigurationError exception should be raised if file not found.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tracopt\perm\tests\authz_policy.py", line 101, in test_get_authz_file_notfound_raises
    'get_authz_file')
  File "C:\usr\apps\python26\lib\unittest.py", line 336, in failUnlessRaises
    callableObj(*args, **kwargs)
  File "C:\usr\src\trac\trac.git\trac\util\__init__.py", line 1091, in __get__
    result = self.fn(instance)
  File "C:\usr\src\trac\trac.git\tracopt\perm\authz_policy.py", line 181, in get_authz_file
    to_unicode(e.strerror + ' ' + e.filename))
UnicodeDecodeError: 'ascii' codec can't decode byte 0x8e in position 0: ordinal not in range(128)

os.stat() with a non-existent file raises a WindowsError since Python 2.5. The value in strerror attribute of the exception is encoded with ANSI codepage.

> python26
Python 2.6.6 (r266:84297, Aug 24 2010, 18:46:32) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> try:
...
KeyboardInterrupt
>>> import os
>>> try:
...   os.stat('C:/notfound')
... except OSError, e:
...   print e
...
[Error 2] 指定されたファイルが見つかりません。: 'C:/notfound'
>>> e
WindowsError(2, '\x8ew\x92\xe8\x82\xb3\x82\xea\x82\xbd\x83t\x83@\x83C\x83\x8b\x82\xaa\x8c\xa9\x82\xc2\x82\xa9\x82\xe8\x82\xdc\x82\xb9\x82\xf1\x81B')
>>> unicode(e)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0x8e in position 10: ordinal not in range(128)

I think to_unicode should support WindowsError using mbcs encoding which is according to ANSI codepage to avoid UnicodeError. The changes is in [aa1a5a79/jomae.git].

[Ryan J Ollos]

I was using to_unicode(e.strerror + ' ' + e.filename) rather than to_unicode in order to clean up the message for presentation in the web interface when the ConfigurationError is raised.

>>> try:
... os.stat('some/nonexistent/file')
... except OSError, e:
... print e.strerror + ' ' + e.filename
... print e
... 
No such file or directory some/nonexistent/file
[Errno 2] No such file or directory: 'some/nonexistent/file'

I suppose this isn't important now that the message is going to the log.

I'm setting up to do some testing on Windows and will reply soon.

I'm wondering about the use of assert rather than one of the unittest.TestCase assert* methods.

[Ryan J Ollos]

I'm won't have a Windows VM running in which I can do testing for at least a few more days, so please don't let me hold you up on pushing that fix :)

[Ryan J Ollos]

#11293 has some suggestions for related improvements to AuthzPolicy error handling.

[Jun Omae]

Replying to rjollos:

I'm won't have a Windows VM running in which I can do testing for at least a few more days, so please don't let me hold you up on pushing that fix :)

Ok, I'll push it after test with Python 2.5-2.7 again ;)

[Jun Omae]

Pushed in [12054] and merged into trunk in [12055].