PrivateTicketsPlugin is incompatible with TracAnnouncer plugin

[Rob Guttman]

This plugin appears to be incompatible with the TracAnnouncer plugin. I get this in the trac.log file:

Traceback (most recent call last):
  File /usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/api.py, line 539, in _real_send
    sf.filter_subscriptions(evt, subscriptions)
  File /usr/local/lib/python2.6/dist-packages/TracAnnouncer-0.12.1.dev-py2.6.egg/announcer/filters.py, line 67, in filter_subscriptions
    if permsys.check_permission(action, sid):
  File /usr/local/lib/python2.6/dist-packages/Trac-0.12.1-py2.6.egg/trac/perm.py, line 454, in check_permission
    perm)
  File /usr/local/lib/python2.6/dist-packages/TracPrivateTickets-2.0.2-py2.6.egg/privatetickets/policy.py, line 34, in check_permission
    TRAC_ADMIN in perm:
TypeError: argument of type NoneType is not iterable

The problem seems to stem from TracAnnouncer not sending a value for perm which the Trac permission system seems to allow and defaults its value to None.

My local fix was to change line 34 in privatetickets/policy.py to:

(perm and 'TRAC_ADMIN' in perm)

Once patched, the plugin appears to work well on Trac 0.12.

[Ryan J Ollos]

I've only briefly looked at this, but it sounds a lot like the issue in #5825 which occurred when PrivateTicketsPlugin and TracHoursPlugin were enabled. Assuming I fixed that issue correctly, the real fix was to change how TracHoursPlugin was checking for permissions.

[Ryan J Ollos]

On further inspection, it looks like this issue can probably be fixed by patching filters.py​ in the AnnouncerPlugin, as was done in [9570] for the TracHoursPlugin.

[Rob Guttman]

Ryan, thanks - Cc'ing doki_pen as well to consider your suggestion for AnnouncerPlugin.

[Steffen Hoffmann]

Replying to rjollos:

On further inspection, it looks like this issue can probably be fixed by patching filters.py​ in the AnnouncerPlugin, as was done in [9570] for the TracHoursPlugin.

Ryan, now I see, why you originally did Cc ticket #5825 to me some time ago.

The current definition for IPermissionPolicy interface still doesn't mention a None case for the perm argument, but since the PermissionSystem itself has no problem with that case, plugins implementing it IPermissionPolicy should better expect it too. However, PrivateTicketsPlugin is quite static, so I agree that we better switch AnnouncerPlugin from PermissionSystem to PermissionCache too for users piece of mind. Fact remains, that the proposed patch is the correct fix, while changing dependent plugins is still a workaround. TracHoursPlugin, AnnouncerPlugin, who's next?

Considering your ground work on this issue, changing this in AnnouncerPlugin should be almost trivial, so even I could do it. May I... ? Uh, let's see, how well it goes.

[Steffen Hoffmann]

(In [9813]) AnnouncerPlugin: Switch permission check from PermissionSystem to PermissionCache, refs #5825 and #8458.

This has been requested for playing nicer with plugins that implement IPermissionPolicy but do not deal with check_permission methode, if user permission cache is missing as additional argument there. Currently this applies i.e. to PrivateTicketsPlugin, so the same workaround has been implemented for TracHoursPlugin by Ryan Ollos before (see #5825 for details).

[Ryan J Ollos]

Replying to hasienda:

Fact remains, that the proposed patch is the correct fix, while changing dependent > plugins is still a workaround. TracHoursPlugin, AnnouncerPlugin, who's next?

Considering your ground work on this issue, changing this in AnnouncerPlugin should be almost trivial, so even I could do it. May I... ? Uh, let's see, how well it goes.

When I originally investigated this issue, I think I was convinced that I had the correct fix in place using the PermissionCache object. While (so far) I still believe this is the correct use of the Trac API, I'm not convinced that the use of PermissionStore is necessarily incorrect (though perhaps 'less correct'). I'm studying this some more, and am very interested in discussing this further.

But you make a very good point, while I was originally convinced that no changes needed to be made to PrivateTicketsPlugin, I'm thinking some changes may be in line. However, I wonder if the patch proposed in comment:description is the correct fix, or if that patch will result in the user with username not being checked for TRAC_ADMIN permission. Making a change such as:

-           'TRAC_ADMIN' in perm:
+           (perm and 'TRAC_ADMIN' in perm):

would seem to result in the check for the TRAC_ADMIN permission being skipped in the case that the PermissionCache object is not passed. That would not seem to be the behavior we want.

Perhaps we need to retrieve the PermissionCache in the check_permission method of PrivateTicketsPolicy:

0.11/privatetickets/policy.py

@@ -28 +28 @@
     ])
     
     # IPermissionPolicy(Interface)
-    def check_permission(self, action, username, resource, perm):
+    def check_permission(self, action, username=None, resource=None, perm=None):
+        
+        if username is None:
+            username = 'anonymous'
+        
+        if perm is None:
+            perm = PermissionCache(self.env, username)
+        
         if username == 'anonymous' or \
            action in self.ignore_permissions or \
            'TRAC_ADMIN' in perm:

However, I have to wonder why the The ​check_permission method of the PermissionSystem object passes the perm object straight through to the check_permission methods of all policies implementing the IPermissionPolicy interface without a check for whether the perm object is None, and if so, retrieving the PermissionCache.

Note also that None is not mentioned as a possible value for perm in the ​IPermissionPolicy interface, therefore it is not explicitly implied that a developer should be checking for this possibility when implementing the interface.

This seems to be another case of some clarifications needing to be made to the Trac API documentation ... or perhaps I'm overlooking something clues?

Let me know what you think. Perhaps a post to the TracDev mailing list is in order.

[osimons]

If a plugin initiates a permission check that has None for either username, resource or perm, then that plugin has a bug. Policy implementations should not need to check these values - or try to recreate them. And yes, perm is a PermissionCache object.

I don't know any of the code in these plugins other than what is listed in this ticket, but please find the root permission checks that cause None to be introduced. It will affect any plugin or Trac code implementing IPermissionPolicy, so if this isn't fixed then bugs will start appearing at other arbitrary permission policy plugins too.

[osimons]

Actually, it seems PermissionSystem.check_permission() is somewhat more lenient and also does the username mapping of None -> anonymous. Some sub-systems don't work with resources, so that could be expected to be None at times.

Still, perm should not be None. Does it makes sense to check permissions without perm?

[Ryan J Ollos]

Replying to osimons:

Still, perm should not be None. Does it makes sense to check permissions without perm?

That makes sense, but PermissionSystem.check_permission() has a default argument of None (as seen ​here), which seems misleading.

What we are seeing is that the default policies (DefaultPermissionPolicy, LegacyAttachmentPolicy) don't seem to have a problem with perm=None, but PrivateTicketsPolicy does. hasienda fixed in [9813] the offending code in the AnnouncerPlugin that had a call to PermissionSystem.check_permission() without passing a PermissionCache object, but I had bet there are plugins all over trac-hacks that make a call to PermissionSystem.check_permission() without passing a PermissionCache object, so to his point, the issue in this ticket and in #5825 is likely to surface again as PrivateTicketsPlugin is used in combination with other plugins that have this offending pattern.

But it sounds like you are saying that no changes need to be made to PrivateTicketsPlugin, and we will just need to apply the fix as in [9570], [9813] to other plugins if this bug is reported again.

Based on your statements, the proper use of the API would seem to be one of the following (ignoring resource in these examples) :

1.

permsys = PermissionSystem()
perm = PermissionCache(self.env, user)
permsys.check_permission(action, user, perm=perm)

2.

req.perm.has_permission(action)

3.

perm = PermissionCache(self.env, user)
perm.has_permission(action)

(2) and (3) appear to be equivalent, and usage of one over the other would seem to just depend on whether the req object is passed to the method that makes this call. It would seem that either would be preferred over (1), and therefore the preferred way to check permissions is through PermissionCache.has_permission(), not PermissionSystem.check_permission().

[osimons]

The correct way of checking permissions is to do action in perm(resource) which through PermissionCache using __contains__ and __call__ ensures that this gets mapped correctly.

Usually, perm is available via req.perm (with correct req.perm.username) but if not then a cache object must be created:

perm = PermissionCache(env, user)
action in perm(resource)
# or for raising PermissionError directly
perm(resource).require(action)
# also works, but negates fine-grained permissions where available
# "action in perm" or "perm.require(action)"

Permission checks without resources should never be done for domains that support resources (like ticket, wiki and so on). If resources are not included, plugins like privatetickets will not be able to do selectively respond properly to permission checks for particular IDs - and such checks may inadvertently breach the security that other policies intend to enforce.

If resource is missing, it can generally be fetched from model if that is available (like ticket.resource) or created/used directly:

from trac.resource import Resource
resource = Resource('ticket', 42)
# or indirectly by calling perm with the details:
action in req.perm('ticket', 42)

[Ryan J Ollos]

Replying to osimons:

The correct way of checking permissions is to do action in perm(resource) which through PermissionCache using __contains__ and __call__ ensures that this gets mapped correctly.

Ahh, thank you! I see now that the usage is well ​documented, but I was completely overlooking it. I also didn't understand the significance of passing resource, but your explanation helps a lot with that.

It looks like we will need to refactor [9569], [9570] and [9813] accordingly.

[Steffen Hoffmann]

Replying to rjollos:

It looks like we will need to refactor [9569], [9570] and [9813] accordingly.

This is what I was instantly thinking too after reading Odd's comment.

Actually I had seen the comments in perm.py before, when doing research before applying [9813], but was certainly missing the point of the fine-grained permission check (i.e. per ticket). Let's see how hard it is to (re-)get perm for the AnnouncerPlugin. I even thought about filtering earlier in the process, but this seems ok after looking a little longer into the code - can't be done earlier than after all subscriptions (including pseudo-subscription default triggers) have been collected, right?

A refinement is certainly mandatory here. So good to have the ticket with AnnouncerPlugin for now.

Another thought of mine has been performance. From AccountManagerPlugin I know, there are some Trac installations out there with >700 active users. Is there a performance penalty for PermissionCache compared to PermissionSystem, if we speak about hundreds of users, not the only a couple of? After all the PermissionCache is (created) per user, while the PermissionSystem object just has all user permission in. But this is just a feeling, and maybe I've still not understood all that correctly. And most importantly I've never profiled any change/patch ever before.

[Steffen Hoffmann]

Replying to rjollos:

[!...] But it sounds like you are saying that no changes need to be made to PrivateTicketsPlugin, and we will just need to apply the fix as in [9570], [9813] to other plugins if this bug is reported again.

I can't totally agree here. Being prepared to at least fail more gracefully than with a Traceback, if other (dependent) plugins do it wrong, seem always like a good idea, despite most Python Tracebacks look very helpful indeed and I started to really love them. That there's room for doing the permission checking incorrectly is indisputable per existence of this ticket and all connected ones.

An Yes, I think we should point trac-dev to this discussion/issue here, since at least the embedded documentation could profit from some more hints regarding use of the perm attribute. I'll do this tomorrow, if not anyone here is faster.

[Ryan J Ollos]

(In [9817]) Refactored permission check fixes made in [9569] and [9570]. Thanks you osimons for the pointers! Refs #8458, #5825.

[Steffen Hoffmann]

announcerplugin/trunk/announcer/filters.py

@@ -65 +65 @@
             if not auth:
                 sid = 'anonymous'
             perm = PermissionCache(self.env, sid)
-            if perm.has_permission(action):
+            if event.realm == 'ticket':
+                resource = event.target.id
+            elif event.realm == 'wiki':
+                resource = event.target.name
+            else:
+                resource = event.target.name
+
+            self.log.debug(
+                'filter_subscriptions: checking resource %s in realm %s'
+                % (resource, event.realm))
+            if action in perm(event.realm, resource):
                 yield subscription
             else:
                 self.log.debug(

While I tested this successfully here I think hard about what looks to me like a serious/unacceptable flaw of that fix:

Is there a better way to determine the resource identifier per realm?

Looking at the definitions of wiki page object and the ticket object I failed to see a way to deduce the resource identifier for an arbitrary realm. ScreenshotsPlugin, TagsPlugin and certainly others too introduce all their own realm, and we may want to respect per-resource restrictions as discussed before. Hardcoding that here is too ugly, but how could it be done better (and with less code I guess)?

[Ryan J Ollos]

Replying to hasienda:

I can't totally agree here. Being prepared to at least fail more gracefully than with a Traceback, if other (dependent) plugins do it wrong, seem always like a good idea, despite most Python Tracebacks look very helpful indeed and I started to really love them. That there's room for doing the permission checking incorrectly is indisputable per existence of this ticket and all connected ones.

Your logic seems sound, and since this ticket has been reassigned from PrivateTicketsPlugin to AnnouncerPlugin we may want to generate another ticket for the issue with the PrivateTicketsPlugin once we figure out the best way to resolve this once and for all.

[Steffen Hoffmann]

Replying to rjollos:

![...] Your logic seems sound, and since this ticket has been reassigned from PrivateTicketsPlugin to AnnouncerPlugin we may want to generate another ticket for the issue with the PrivateTicketsPlugin once we figure out the best way to resolve this once and for all.

Thanks. Don't know, if rather we should have created a new ticket for AnnouncerPlugin, since a patch candidate for PrivateTicketsPlugin is still attached here, but you may still copy/transfer it as well - later.

Anyway, I'll proceed with my preliminary solution for now, since there has been no better suggestion so far. OTOH I've just now ​asked at the Trac-dev mailing-list, so there's a good chance for a better solution.

[Steffen Hoffmann]

(In [12311]) AnnouncerPlugin: Further improve ticket permission checks, refs #5825, #8458 and #8577.

This is a late follow-up to changeset [9813] after more in-deep discussion on permission checking for whole Trac realms and specific Trac resources in #8458. With my original patch proposal from 04-Feb-2011 in mind I call this an aged and really matured changeset, and that's not so bad after all. Furthermore code from [12304] gets improved here as well.

Special thanks to Odd Simon Simonsen, Ryan J. Ollos and Christian Boos for their help on my way towards better understanding Trac permissions.