launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05821
[Merge] lp:~allenap/launchpad/delegated-permission-caching-bug-890927 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/delegated-permission-caching-bug-890927 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/launchpad/delegated-permission-caching-bug-890927/+merge/84473
This change brings all responsibility for checking authorization (via
IAuthorization adapters) back into LaunchpadSecurityPolicy. This way
LSP provide a consistent approach for normal and delegated auth
adapters.
The change that underpins this is allowing checkAuthenticated() and
checkUnauthenticated() - from IAuthorization - to return not just True
or False but optionally an iterable argument. If an iterable is
returned the security policy treats it as a sequence of (object,
permission) tuples that must be checked for authorization; in other
words, authorization is delegated.
A new function, iter_authorization() has been factored out of
LSP.checkPermission() and extended to support this new behaviour. It
is able to cache the results of authorization checks even for
delegated authorizations.
In all I think this is a much better approach than trying to push
responsibility for delegation down into the adapters themselves. There
is too much policy (and caching) in LSP that makes little sense to try
and replicate elsewhere.
Performance of this new code is likely to be slightly poorer for
non-delegated authorization checks. However, I have run this through
ec2 and it completes (successfully) in comparable times to branches
without this change. A permission check already cached will have
almost identical performance to before.
However, performance for delegated checks is likely to be vastly
improved because of the effects of caching.
--
https://code.launchpad.net/~allenap/launchpad/delegated-permission-caching-bug-890927/+merge/84473
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/delegated-permission-caching-bug-890927 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/authorization.py'
--- lib/canonical/launchpad/webapp/authorization.py 2011-10-16 08:16:47 +0000
+++ lib/canonical/launchpad/webapp/authorization.py 2011-12-05 13:58:34 +0000
@@ -3,6 +3,11 @@
__metaclass__ = type
+from collections import (
+ deque,
+ Iterable,
+ )
+from functools import partial
import warnings
import weakref
@@ -172,13 +177,14 @@
else:
participation = participations[0]
if IApplicationRequest.providedBy(participation):
- wd = participation.annotations.setdefault(
+ participation_cache = participation.annotations.setdefault(
LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
weakref.WeakKeyDictionary())
- cache = wd.setdefault(objecttoauthorize, {})
+ cache = participation_cache.setdefault(objecttoauthorize, {})
if permission in cache:
return cache[permission]
else:
+ participation_cache = None
cache = None
principal = participation.principal
@@ -202,29 +208,91 @@
if (permission == 'launchpad.AnyPerson' and
ILaunchpadPrincipal.providedBy(principal)):
return True
- else:
- # Look for an IAuthorization adapter. If there is no
- # IAuthorization adapter then the permission is not granted.
- #
- # The IAuthorization is a named adapter from objecttoauthorize,
- # providing IAuthorization, named after the permission.
- authorization = queryAdapter(
- objecttoauthorize, IAuthorization, permission)
+
+ return all(
+ iter_authorization(
+ objecttoauthorize, permission, principal,
+ participation_cache, breadth_first=True))
+
+
+def iter_authorization(objecttoauthorize, permission, principal, cache,
+ breadth_first=True):
+ """Work through `IAuthorization` adapters for `objecttoauthorize`.
+
+ Adapters are permitted to delegate checks to other adapters, and this
+ manages that delegation such that the minimum number of checks are made,
+ subject to a breath-first check of delegations.
+
+ This also updates `cache` as it goes along, though `cache` can be `None`
+ if no caching is desired. Only leaf values are cached; the results of a
+ delegated authorization are not cached.
+ """
+ # XXX: GavinPanella 2011-12-03 bug=???: Surely for delegated checks we
+ # still need to do a lot of the other stuff in checkPermission(), like
+ # privacy checks, and the other checks for zope.Public, etc.
+
+ # Check if this calculation has already been done.
+ if cache is not None and objecttoauthorize in cache:
+ if permission in cache[objecttoauthorize]:
+ # Result cached => yield and return.
+ yield cache[objecttoauthorize][permission]
+ return
+
+ # Create a check_auth function to call checkAccountAuthenticated or
+ # checkUnauthenticated as appropriate.
+ if ILaunchpadPrincipal.providedBy(principal):
+ account = principal.account
+ check_auth = lambda authorization: (
+ authorization.checkAccountAuthenticated(account))
+ else:
+ check_auth = lambda authorization: (
+ authorization.checkUnauthenticated())
+
+ # Each entry in queue should be an iterable of (object, permission)
+ # tuples, upon which permission checks will be performed.
+ queue = deque()
+ enqueue = (queue.append if breadth_first else queue.appendleft)
+
+ # Enqueue the starting object and permission.
+ enqueue(((objecttoauthorize, permission),))
+
+ while len(queue) != 0:
+ for obj, permission in queue.popleft():
+ # Unwrap object; see checkPermission for why.
+ obj = removeAllProxies(obj)
+ # First, check the cache.
+ if cache is not None:
+ if obj in cache and permission in cache[obj]:
+ # Result cached => yield and skip to the next.
+ yield cache[obj][permission]
+ continue
+ # Get an IAuthorization for (obj, permission).
+ authorization = queryAdapter(obj, IAuthorization, permission)
if authorization is None:
- return False
- else:
- if ILaunchpadPrincipal.providedBy(principal):
- result = authorization.checkAccountAuthenticated(
- principal.account)
+ # No authorization adapter => denied.
+ yield False
+ continue
+ # We have an authorization adapter, so check it. This is one of
+ # the possibly-expensive bits that a cache can help with.
+ result = check_auth(authorization)
+ # Is the authorization adapter delegating to other objects?
+ if isinstance(result, Iterable):
+ enqueue(result)
+ continue
+ # We have a non-delegated result.
+ if result is not True and result is not False:
+ warnings.warn(
+ '%r returned %r (%r)' % (
+ authorization, result, type(result)))
+ result = bool(result)
+ # Update the cache if one has been provided.
+ if cache is not None:
+ if obj in cache:
+ cache[obj][permission] = result
else:
- result = authorization.checkUnauthenticated()
- if type(result) is not bool:
- warnings.warn(
- 'authorization returning non-bool value: %r' %
- authorization)
- if cache is not None:
- cache[permission] = result
- return bool(result)
+ cache[obj] = {permission: result}
+ # Let the world know.
+ yield result
def precache_permission_for_objects(participation, permission_name, objects):
=== modified file 'lib/lp/app/interfaces/security.py'
--- lib/lp/app/interfaces/security.py 2011-04-27 16:14:32 +0000
+++ lib/lp/app/interfaces/security.py 2011-12-05 13:58:34 +0000
@@ -16,13 +16,25 @@
"""Authorization policy for a particular object and permission."""
def checkUnauthenticated():
- """Returns True if an unauthenticated user has that permission
- on the adapted object. Otherwise returns False.
+ """Whether an unauthenticated user has `permission` on `obj`.
+
+ Returns `True` if an unauthenticated user has that permission on the
+ adapted object. Otherwise returns `False`.
+
+ If the check must be delegated to other objects, this method can
+ optionally instead generate `(object, permission)` tuples. It is then
+ the security policy's job of checking authorization of those pairs.
"""
def checkAccountAuthenticated(account):
- """Returns True if the account has that permission on the adapted
- object. Otherwise returns False.
-
- The argument `account` is the account who is authenticated.
+ """Whether an authenticated user has `permission` on `obj`.
+
+ Returns `True` if the account has that permission on the adapted
+ object. Otherwise returns `False`.
+
+ If the check must be delegated to other objects, this method can
+ optionally instead generate `(object, permission)` tuples. It is then
+ the security policy's job of checking authorization of those pairs.
+
+ :param account: The account that is authenticated.
"""
=== modified file 'lib/lp/app/security.py'
--- lib/lp/app/security.py 2011-09-07 21:53:15 +0000
+++ lib/lp/app/security.py 2011-12-05 13:58:34 +0000
@@ -11,6 +11,11 @@
'DelegatedAuthorization',
]
+from itertools import (
+ izip,
+ repeat,
+ )
+
from zope.component import queryAdapter
from zope.interface import implements
from zope.security.permission import checkPermission
@@ -127,19 +132,10 @@
"Either set forwarded_object or override iter_objects.")
yield self.forwarded_object
- def iter_adapters(self):
- return (
- queryAdapter(obj, IAuthorization, self.permission)
- for obj in self.iter_objects())
-
def checkAuthenticated(self, user):
- for adapter in self.iter_adapters():
- if adapter is None or not adapter.checkAuthenticated(user):
- return False
- return True
+ """See `IAuthorization`."""
+ return izip(self.iter_objects(), repeat(self.permission))
def checkUnauthenticated(self):
- for adapter in self.iter_adapters():
- if adapter is None or not adapter.checkUnauthenticated():
- return False
- return True
+ """See `IAuthorization`."""
+ return izip(self.iter_objects(), repeat(self.permission))
=== modified file 'lib/lp/app/tests/test_security.py'
--- lib/lp/app/tests/test_security.py 2011-09-07 21:53:15 +0000
+++ lib/lp/app/tests/test_security.py 2011-12-05 13:58:34 +0000
@@ -1,12 +1,9 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
-from zope.component import (
- getSiteManager,
- queryAdapter,
- )
+from zope.component import getSiteManager
from zope.interface import (
implements,
Interface,
@@ -18,7 +15,10 @@
AuthorizationBase,
DelegatedAuthorization,
)
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ TestCase,
+ TestCaseWithFactory,
+ )
from lp.testing.fakemethod import FakeMethod
@@ -145,56 +145,32 @@
adapter.forwardCheckAuthenticated(None, Dummy()))
-class FakeDelegatedAuthorization(DelegatedAuthorization):
- def __init__(self, obj, permission=None):
- super(FakeDelegatedAuthorization, self).__init__(
- obj, obj.child_obj, permission)
-
-
-class FakeForwardedObject:
- implements(IDummy)
-
- def __init__(self):
- self.child_obj = Dummy()
-
-
-class TestDelegatedAuthorizationBase(TestCaseWithFactory):
-
- layer = ZopelessDatabaseLayer
-
- def test_DelegatedAuthorization_same_permissions(self):
-
- permission = self.factory.getUniqueString()
- fake_obj = FakeForwardedObject()
- outer_adapter = FakeDelegatedAuthorization(fake_obj)
- outer_adapter.permission = permission
-
- inner_adapter = FakeSecurityAdapter()
- inner_adapter.permission = permission
- registerFakeSecurityAdapter(IDummy, permission, inner_adapter)
- user = object()
- outer_adapter.checkAuthenticated(user)
- outer_adapter.checkUnauthenticated()
- self.assertVectorEqual(
- (1, inner_adapter.checkAuthenticated.call_count),
- (1, inner_adapter.checkUnauthenticated.call_count))
-
- def test_DelegatedAuthorization_different_permissions(self):
- perm_inner = 'inner'
- perm_outer = 'outer'
- fake_obj = FakeForwardedObject()
- outer_adapter = FakeDelegatedAuthorization(fake_obj, perm_inner)
- registerFakeSecurityAdapter(IDummy, perm_outer, outer_adapter)
-
- inner_adapter = FakeSecurityAdapter()
- inner_adapter.permission = perm_inner
- registerFakeSecurityAdapter(IDummy, perm_inner, inner_adapter)
-
- user = object()
- adapter = queryAdapter(
- FakeForwardedObject(), IAuthorization, perm_outer)
- adapter.checkAuthenticated(user)
- adapter.checkUnauthenticated()
- self.assertVectorEqual(
- (1, inner_adapter.checkAuthenticated.call_count),
- (1, inner_adapter.checkUnauthenticated.call_count))
+class TestDelegatedAuthorization(TestCase):
+ """Tests for `DelegatedAuthorization`."""
+
+ def test_checkAuthenticated(self):
+ # DelegatedAuthorization.checkAuthenticated() punts the checks back up
+ # to the security policy by generating (object, permission) tuples.
+ # The security policy is in a much better position to, well, apply
+ # policy.
+ obj, delegated_obj = object(), object()
+ authorization = DelegatedAuthorization(
+ obj, delegated_obj, "dedicatemyselfto.Evil")
+ # By default DelegatedAuthorization.checkAuthenticated() ignores its
+ # user argument, so we pass None in below, but it is required for
+ # IAuthorization, and may be useful for subclasses.
+ self.assertEqual(
+ [(delegated_obj, "dedicatemyselfto.Evil")],
+ list(authorization.checkAuthenticated(None)))
+
+ def test_checkUnauthenticated(self):
+ # DelegatedAuthorization.checkUnauthenticated() punts the checks back
+ # up to the security policy by generating (object, permission) tuples.
+ # The security policy is in a much better position to, well, apply
+ # policy.
+ obj, delegated_obj = object(), object()
+ authorization = DelegatedAuthorization(
+ obj, delegated_obj, "dedicatemyselfto.Evil")
+ self.assertEqual(
+ [(delegated_obj, "dedicatemyselfto.Evil")],
+ list(authorization.checkUnauthenticated()))
