Fragment in PermissionError msg results in TypeError

[Ryan J Ollos]

>>> from genshi.builder import tag
>>> from trac.core import TracError
>>> from trac.perm import PermissionError
>>> from trac.util.translation import tag_
>>> from trac.util.text import to_unicode
>>> e1 = TracError(tag_("This is a %(fragment)s", fragment=tag.em("Fragment")))
>>> to_unicode(e1)
u'This is a <em>Fragment</em>'
>>> e2 = PermissionError(msg=tag_("This is a %(fragment)s", fragment=tag.em("Fragment")))
>>> to_unicode(e2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/text.py", line 74, in to_unicode
    return unicode(text)
TypeError: coercing to Unicode: need string or buffer, LazyProxy found

[Ryan J Ollos]

The proposed change that led to discovering this issue, as well as a proposed fix, can be found in log:rjollos.git:t11509. If the proposed change looks okay, unit tests for PermissionError should be added and the TracDev/Exceptions#CustomTracExceptions page needs to be updated.

[Peter Suter]

Maybe I'm missing something but in changeset:d6f2c6e1/rjollos.git you replace a check with unconditionally raising an exception:

- req.perm.require(action)
+ raise PermissionError( 

Did you maybe forget a if not action in req.perm:?

[Ryan J Ollos]

Yes, I messed-up the changes when cherry-picking between branches. Thanks for catching it. Added to log:rjollos.git:t11509.

[Ryan J Ollos]

Changes have been improved and additional tests added in log:rjollos.git:t11509.2.

[Jun Omae]

Root cause is that PermissionError.__unicode__() doesn't return a unicode instance. __unicode__ magic method must return a unicode instance.

The same issue can be produced without tag_.

>>> from trac.perm import PermissionError
>>> from trac.util.text import to_unicode
>>> to_unicode(PermissionError(action='WIKI_VIEW'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/text.py", line 74, in to_unicode
    return unicode(text)
TypeError: coercing to Unicode: need string or buffer, LazyProxy found

trac/perm.py

@@ -49 +49 @@
     def __unicode__ (self):
         if self.action:
             if self.resource:
-                return _('%(perm)s privileges are required to perform '
-                         'this operation on %(resource)s. You don\'t have the '
-                         'required permissions.',
-                         perm=self.action,
-                         resource=get_resource_name(self.env, self.resource))
+                msg = _('%(perm)s privileges are required to perform '
+                        'this operation on %(resource)s. You don\'t have the '
+                        'required permissions.',
+                        perm=self.action,
+                        resource=get_resource_name(self.env, self.resource))
             else:
-                return _('%(perm)s privileges are required to perform this '
-                         'operation. You don\'t have the required '
-                         'permissions.', perm=self.action)
+                msg = _('%(perm)s privileges are required to perform this '
+                        'operation. You don\'t have the required '
+                        'permissions.', perm=self.action)
         elif self.msg:
-            return self.msg
+            msg = self.msg
         else:
-            return _('Insufficient privileges to perform this operation.')
+            msg = _('Insufficient privileges to perform this operation.')
+        return unicode(msg)
 
 
 class IPermissionRequestor(Interface):

After the patch;

>>> e2 = PermissionError(msg=tag_("This is a %(fragment)s", fragment=tag.em("Fragment")))
>>> to_unicode(e2)
u'This is a <em>Fragment</em>'
>>> to_unicode(PermissionError(action='WIKI_VIEW'))
u"WIKI_VIEW privileges are required to perform this operation. You don't have the required permissions."

Also I don't think we should change base class of PermissionError on 1.0-stable, that is okay on trunk.

[Jun Omae]

Also I don't think we should change base class of PermissionError on 1.0-stable, that is okay on trunk.

I reconsider it. I think changing the base class to TracError is not okay on trunk, now.

Currently, the except TracError statement is not intent to catch a PermissionError exception, e.g. tags/trac-1.0.1/trac/attachment.py@:758,767-768#L750​. It is not good idea to verify such all except statements in Trac core and all plugins.

[Ryan J Ollos]

Proposed changes can be found in log:rjollos.git:t11509.3.

[Jun Omae]

Replying to jomae:

Also I don't think we should change base class of PermissionError on 1.0-stable, that is okay on trunk.

I reconsider it. I think changing the base class to TracError is not okay on trunk, now.

How about adding TracBaseError as base class of all exceptions on trunk to keep backward-compatibilities and consistencies?

 * Exception
   * TracBaseError
     * TracError
       * AdminCommandError
       * ConfigurationError
       * (all subclasses of TracError...)
     * PermissionError
     * BackupError
     * HTTPException
     * RequestDone
     * ParseError
     * GitError
       * GitErrorSha

[Ryan J Ollos]

Replying to jomae:

How about adding TracBaseError as base class of all exceptions on trunk to keep backward-compatibilities and consistencies?

That might make sense. I will give it some additional thought.

More changes:

In the initial changes I had some markup in the exception message - emphasis on the user, group and permission. While I think this might be a good idea, we should follow a consistent pattern, so I don't like the idea of making the change to a single exception message as part of this ticket. Therefore I'm planning to address those changes later on, and probably use spans with classes rather than em elements.

With the changes posted in comment:7, a Fragment in a PermissionError message is not rendered to HTML. The changes in [0f0da2e5/rjollos.git] are intended to address that.

I've added additional change that I'm still considering, which is to encapsulate code from _send_user_error into the HTTPException: [44a4f215/rjollos.git] and [c58a627a/rjollos.git]. I think this would make the code more maintainable, but I need to test more and think through the consequences.

[Jun Omae]

I got the following failure with only Python 2.5. But all tests passed with Python 2.6 and 2.7.

======================================================================
FAIL: TracError should be raised when navigating to the attachment
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/c58a627a37684f941c99549269644437815657ee/src-py25-sqlite/trac/tests/functional/testcases.py", line 31, in runTest
    tc.find('<h1>Trac Error</h1>\s+<p class="message">'
  File "/run/shm/c58a627a37684f941c99549269644437815657ee/src-py25-sqlite/trac/tests/functional/better_twill.py", line 225, in better_find
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ('no match to \'<h1>Trac Error</h1>\\s+<p class="message">Parent resource NonexistentPage doesn\'t exist</p>\'', '/run/shm/c58a627a37684f941c99549269644437815657ee/src-py25-sqlite/testenv/trac/log/TestAttachmentNonexistentParent.html')

----------------------------------------------------------------------
Ran 174 tests in 206.724s

FAILED (failures=1)

[Ryan J Ollos]

I added the TracBaseError class and fixed the failure in comment:10 (fixed in [51e244a6/rjollos.git]). It didn't seem necessary to have trac.db.tests.api​.Error inherit from TracBaseError since I wouldn't expect the class to be used outside of the trac.db.tests.api module. I'll add the TracBaseError class to Error if anyone thinks its necessary.

There's a refactoring to use add_notice. add_notice was added in [6379#file2]. I'm assuming it was just an oversight that it wasn't used in [7494].

I attempted to clean up the changes from [44a4f215/rjollos.git] and [c58a627a/rjollos.git] by moving the code to class properties.

There are some additional issues mentioned in the changeset messages. Some tests need to be written. I'm just posting this now to get feedback on the changes and the questions I've raised.

[Ryan J Ollos]

I made a mess in [12636:12637,12648]. Sorry about that.

[Ryan J Ollos]

Committed/merged changes in [12638:12639], [12640:12641], [12642:12643], [12644:12645], [12646:12647].

#11568 is a follow-on ticket for the proposed changes to the exception hierarchy.