Prevent recursion in PermissionSystem.check_permission

[Ryan J Ollos]

Discussed in ​gdiscussion:trac-users:MNvfBPbiAS0 and other threads, there is a high likelihood of recursion when a permission policy calls Permission.has_permission (usually via ACTION in perm) within its policy.check_permission method. One possible way to avoid this, which I'd like to explore, would be to skip calling policy.check_permission​ on the policy that invoked PermissionCache.has_permission.

Using CookBook/Configuration/SignedTickets as an example, the policy would be changed:

-                any(a in perm for a in self.admin_actions):
+                any(a in perm(resource, policy=self)
+                    for a in self.admin_actions):

[Ryan J Ollos]

The following changes may need additional work, but with a small amount of testing I can say they appear to fix the issue raised on the mailing list: log:rjollos.git:t12597_perm_policy_recursion.

[Peter Suter]

I find it quite difficult to think about the implications here. Does this ever unintentionally skip an important permission check?

Might a conservative alternative be to "pre-cache" a negative decision? (untested)

trac/perm.py

@@ -588 +588 @@
             cache_decision, cache_resource = cached
             if resource == cache_resource:
                 return cache_decision
+        # In case of recursive check, disallow permission
+        self._cache[key] = (False, resource)
         perm = self
         if resource is not self._resource:
             perm = PermissionCache(self.env, self.username, resource,

[Ryan J Ollos]

I'll investigate for 1.2.1. next-stable-1.0.x should be (or soon be) for ​major defects only.

[Ryan J Ollos]

Replying to Peter Suter:

Might a conservative alternative be to "pre-cache" a negative decision? (untested)

That appears to be a simple way to solve the issue: rjollos.git:t12597_perm_policy_recursion.1.

With the change, ReadonlySignedTickets policy works with the following change:

ReadonlySignedTickets.py

@@ -15 +15 @@
         if resource is None or resource.realm != 'ticket' or \
                 resource.id is None or \
                 action in self.allowed_actions or \
-                action in ('TICKET_ADMIN', 'TRAC_ADMIN') or \
                 'TICKET_ADMIN' in perm:
             return None

[Ryan J Ollos]

Additional change in [4831f0f3/rjollos.git].

if resource ... ​ in has_permission didn't seem to be behaving as intended.

1. if resource would always evaluate to True. I suppose we could implement __nonzero__ / __bool__ on Resource so that Resource(None) would evaluate to False, but unsure whether that's generally useful.

   >>> from trac.resource import Resource
   >>> r = Resource(None)
   >>> bool(r)
   True
   

2. ACTION in perm should behave the same as ACTION in perm(None), however before the change the cached key would be different:

   >>> from trac.perm import PermissionCache
   >>> from trac.test import EnvironmentStub
   >>> env = EnvironmentStub()
   >>> perm1 = PermissionCache(env)
   >>> perm2 = PermissionCache(env)
   >>> 'PERM' in perm1
   False
   >>> perm1._cache
   {('anonymous', -9223372036584357655, 'PERM'): (False, None)}
   >>> 'PERM' in perm2(None)
   False
   >>> perm2._cache
   {('anonymous', 7976857064515516242, 'PERM'): (False, <Resource ''>)}
   

   After the change:

   >>> from trac.perm import PermissionCache
   >>> from trac.test import EnvironmentStub
   >>> env = EnvironmentStub()
   >>> perm1 = PermissionCache(env)
   >>> perm2 = PermissionCache(env)
   >>> 'PERM' in perm1
   False
   >>> perm1._cache
   {('anonymous', -9223372036583836439, 'PERM'): (False, None)}
   >>> 'PERM' in perm2(None)
   False
   >>> perm2._cache
   {('anonymous', -9223372036583836439, 'PERM'): (False, None)}
   

   Added test case in [2128800/rjollos.git].
3. Still considering, but implementing __nonzero__ behavior proposed in (1), ACTION in perm(Resource(None)) could be made to behave the same as the two cases discussed in (2): [83ada83e/rjollos.git].

[Ryan J Ollos]

Most changes committed in r15340, merged in r15341. I didn't commit [83ada83e/rjollos.git].