Modify

Opened 3 years ago

Last modified 21 months ago

#8458 new defect

PrivateTicketsPlugin is incompatible with TracAnnouncer plugin

Reported by: robguttman Owned by: hasienda
Priority: high Component: AnnouncerPlugin
Severity: blocker Keywords: permission None user cache
Cc: hasienda, doki_pen Trac Release: 0.12

Description

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.

Attachments (0)

Change History (19)

comment:1 Changed 3 years ago by rjollos

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.

comment:2 follow-up: Changed 3 years ago by rjollos

  • Cc hasienda added

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.

comment:3 Changed 3 years ago by robguttman

  • Cc doki_pen added

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

comment:4 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by hasienda

  • Keywords permission None user cache added

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.

comment:5 Changed 3 years ago by hasienda

(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).

comment:6 in reply to: ↑ 4 Changed 3 years ago by rjollos

  • Owner changed from coderanger to rjollos
  • Status changed from new to assigned

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

     
    2828    ]) 
    2929     
    3030    # IPermissionPolicy(Interface) 
    31     def check_permission(self, action, username, resource, perm): 
     31    def check_permission(self, action, username=None, resource=None, perm=None): 
     32         
     33        if username is None: 
     34            username = 'anonymous' 
     35         
     36        if perm is None: 
     37            perm = PermissionCache(self.env, username) 
     38         
    3239        if username == 'anonymous' or \ 
    3340           action in self.ignore_permissions or \ 
    3441           '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.

comment:7 Changed 3 years ago by 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.

comment:8 follow-up: Changed 3 years ago by 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?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by rjollos

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().

comment:10 follow-up: Changed 3 years ago by 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)

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by rjollos

  • Component changed from PrivateTicketsPlugin to AnnouncerPlugin

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.

comment:12 in reply to: ↑ 11 Changed 3 years ago by hasienda

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.

comment:13 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by hasienda

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.

comment:14 Changed 3 years ago by rjollos

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

comment:15 Changed 3 years ago by hasienda

  • announcerplugin/trunk/announcer/filters.py

    a b  
    6565            if not auth: 
    6666                sid = 'anonymous' 
    6767            perm = PermissionCache(self.env, sid) 
    68             if perm.has_permission(action): 
     68            if event.realm == 'ticket': 
     69                resource = event.target.id 
     70            elif event.realm == 'wiki': 
     71                resource = event.target.name 
     72            else: 
     73                resource = event.target.name 
     74 
     75            self.log.debug( 
     76                'filter_subscriptions: checking resource %s in realm %s' 
     77                % (resource, event.realm)) 
     78            if action in perm(event.realm, resource): 
    6979                yield subscription 
    7080            else: 
    7181                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)?

comment:16 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by rjollos

  • Summary changed from Incompatible with TracAnnouncer plugin to PrivateTicketsPlugin is incompatible with TracAnnouncer plugin

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.

comment:17 Changed 3 years ago by rjollos

  • Owner changed from rjollos to hasienda
  • Status changed from assigned to new

comment:18 in reply to: ↑ 16 Changed 3 years ago by hasienda

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.

comment:19 Changed 21 months ago by hasienda

(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.

Add Comment

Modify Ticket

Action
as new .
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.