launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/security-policy-unauth-check into lp:launchpad.

Commit message:
Add LaunchpadSecurityPolicy.checkUnauthenticationPermission.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/security-policy-unauth-check/+merge/287844

Add LaunchpadSecurityPolicy.checkUnauthenticationPermission.  This will be useful for caveat checking, and we'll need to add an @implementer decorator once the interface exists in lazr.restful, but this can harmlessly be done in advance of that.

I hate the way the test cleanup is being done right now, but for the moment it's easier than refactoring all this to use proper utility fixtures.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/security-policy-unauth-check into lp:launchpad.

=== modified file 'lib/lp/services/webapp/authorization.py'
--- lib/lp/services/webapp/authorization.py	2015-07-08 16:05:11 +0000
+++ lib/lp/services/webapp/authorization.py	2016-03-02 20:04:39 +0000
@@ -11,6 +11,7 @@
     'LaunchpadPermissiveSecurityPolicy',
     'LaunchpadSecurityPolicy',
     'LAUNCHPAD_SECURITY_POLICY_CACHE_KEY',
+    'LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY',
     'precache_permission_for_objects',
     ]
 
@@ -59,11 +60,14 @@
     AccessLevel,
     ILaunchpadContainer,
     ILaunchpadPrincipal,
+    IPlacelessAuthUtility,
     )
 from lp.services.webapp.metazcml import ILaunchpadPermission
 
 
 LAUNCHPAD_SECURITY_POLICY_CACHE_KEY = 'launchpad.security_policy_cache'
+LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY = (
+    'launchpad.security_policy_cache.unauthenticated')
 
 
 @provider(ISecurityPolicy)
@@ -129,19 +133,14 @@
             else:
                 return AccessLevel.READ_PUBLIC
 
-    @block_implicit_flushes
-    def checkPermission(self, permission, object):
+    def _baseCheckPermission(self, permission, object, cache_key,
+                             principal=None):
         """Check the permission, object, user against the launchpad
         authorization policy.
 
         If the object is a view, then consider the object to be the view's
         context.
 
-        If we are running in read-only mode, all permission checks are
-        failed except for launchpad.View requests, which are checked
-        as normal. All other permissions are used to protect write
-        operations.
-
         Workflow:
         - If the principal is not None and its access level is not what is
           required by the permission, deny.
@@ -191,13 +190,13 @@
             participation = participations[0]
             if IApplicationRequest.providedBy(participation):
                 participation_cache = participation.annotations.setdefault(
-                    LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
-                    weakref.WeakKeyDictionary())
+                    cache_key, weakref.WeakKeyDictionary())
                 object_cache = participation_cache.setdefault(
                     objecttoauthorize, {})
                 if permission in object_cache:
                     return object_cache[permission]
-            principal = removeAllProxies(participation.principal)
+            if principal is None:
+                principal = removeAllProxies(participation.principal)
 
         if (principal is not None and
             not isinstance(principal, UnauthenticatedPrincipal)):
@@ -239,6 +238,28 @@
 
         return result
 
+    @block_implicit_flushes
+    def checkPermission(self, permission, object):
+        """Check the permission, object, user against the launchpad
+        authorization policy.
+        """
+        return self._baseCheckPermission(
+            permission, object, LAUNCHPAD_SECURITY_POLICY_CACHE_KEY)
+
+    @block_implicit_flushes
+    def checkUnauthenticatedPermission(self, permission, object):
+        """Check the permission and object against the Launchpad
+        authorization policy for an unauthenticated principal.
+
+        This is similar to `checkPermission`, but can be used to check the
+        baseline permissions that are available even without authentication.
+        """
+        auth_utility = getUtility(IPlacelessAuthUtility)
+        principal = auth_utility.unauthenticatedPrincipal()
+        return self._baseCheckPermission(
+            permission, object, LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY,
+            principal=principal)
+
 
 def iter_authorization(objecttoauthorize, permission, principal, cache,
                        breadth_first=True):
@@ -351,6 +372,8 @@
             # all classes that implement IApplicationRequest.
             if LAUNCHPAD_SECURITY_POLICY_CACHE_KEY in p.annotations:
                 del p.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_KEY]
+            if LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY in p.annotations:
+                del p.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY]
 
 
 class LaunchpadPermissiveSecurityPolicy(PermissiveSecurityPolicy):

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2016-02-10 00:51:55 +0000
+++ lib/lp/services/webapp/servers.py	2016-03-02 20:04:39 +0000
@@ -79,6 +79,7 @@
     )
 from lp.services.webapp.authorization import (
     LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
+    LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY,
     )
 from lp.services.webapp.errorlog import ErrorReportRequest
 from lp.services.webapp.interfaces import (
@@ -674,6 +675,8 @@
     def clearSecurityPolicyCache(self):
         if LAUNCHPAD_SECURITY_POLICY_CACHE_KEY in self.annotations:
             del self.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_KEY]
+        if LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY in self.annotations:
+            del self.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY]
 
     def beforeCompletion(self, transaction):
         """See `ISynchronizer`."""

=== modified file 'lib/lp/services/webapp/tests/test_authorization.py'
--- lib/lp/services/webapp/tests/test_authorization.py	2015-07-08 16:05:11 +0000
+++ lib/lp/services/webapp/tests/test_authorization.py	2016-03-02 20:04:39 +0000
@@ -9,6 +9,7 @@
 import StringIO
 
 import transaction
+from zope.app.testing import ztapi
 from zope.component import (
     provideAdapter,
     provideUtility,
@@ -27,12 +28,16 @@
 from lp.registry.interfaces.role import IPersonRoles
 from lp.services.database.interfaces import IStoreSelector
 from lp.services.privacy.interfaces import IObjectPrivacy
-from lp.services.webapp.authentication import LaunchpadPrincipal
+from lp.services.webapp.authentication import (
+    LaunchpadPrincipal,
+    PlacelessAuthUtility,
+    )
 from lp.services.webapp.authorization import (
     available_with_permission,
     check_permission,
     iter_authorization,
     LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
+    LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY,
     LaunchpadSecurityPolicy,
     precache_permission_for_objects,
     )
@@ -40,6 +45,7 @@
     AccessLevel,
     ILaunchpadContainer,
     ILaunchpadPrincipal,
+    IPlacelessAuthUtility,
     )
 from lp.services.webapp.metazcml import ILaunchpadPermission
 from lp.services.webapp.servers import (
@@ -373,6 +379,55 @@
             ['checkUnauthenticated', 'checkUnauthenticated'],
             checker_factory.calls)
 
+    def test_checkUnauthenticatedPermission_cache_unauthenticated(self):
+        # checkUnauthenticatedPermission caches the result of
+        # checkUnauthenticated for a particular object and permission.
+        # We set a principal to ensure that it is not used even if set.
+        provideUtility(PlacelessAuthUtility(), IPlacelessAuthUtility)
+        zope.testing.cleanup.addCleanUp(
+            ztapi.unprovideUtility, (IPlacelessAuthUtility,))
+        principal = FakeLaunchpadPrincipal()
+        request = self.makeRequest()
+        request.setPrincipal(principal)
+        policy = LaunchpadSecurityPolicy(request)
+        obj, permission, checker_factory = (
+            self.getObjectPermissionAndCheckerFactory())
+        # When we call checkUnauthenticatedPermission for the first time,
+        # the security policy calls the checker.
+        policy.checkUnauthenticatedPermission(permission, obj)
+        self.assertEqual(['checkUnauthenticated'], checker_factory.calls)
+        # A subsequent identical call does not call the checker.
+        policy.checkUnauthenticatedPermission(permission, obj)
+        self.assertEqual(['checkUnauthenticated'], checker_factory.calls)
+        # The result is stored in the correct cache.
+        cache = request.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY]
+        self.assertEqual({obj: {permission: False}}, dict(cache))
+
+    def test_checkUnauthenticatedPermission_commit_clears_cache(self):
+        # Committing a transaction clears the cache.
+        # We set a principal to ensure that it is not used even if set.
+        provideUtility(PlacelessAuthUtility(), IPlacelessAuthUtility)
+        zope.testing.cleanup.addCleanUp(
+            ztapi.unprovideUtility, (IPlacelessAuthUtility,))
+        principal = FakeLaunchpadPrincipal()
+        request = self.makeRequest()
+        request.setPrincipal(principal)
+        policy = LaunchpadSecurityPolicy(request)
+        obj, permission, checker_factory = (
+            self.getObjectPermissionAndCheckerFactory())
+        # When we call checkUnauthenticatedPermission before setting the
+        # principal, the security policy calls checkUnauthenticated on the
+        # checker.
+        policy.checkUnauthenticatedPermission(permission, obj)
+        self.assertEqual(['checkUnauthenticated'], checker_factory.calls)
+        transaction.commit()
+        # After committing a transaction, the policy calls
+        # checkUnauthenticated again rather than finding a value in the cache.
+        policy.checkUnauthenticatedPermission(permission, obj)
+        self.assertEqual(
+            ['checkUnauthenticated', 'checkUnauthenticated'],
+            checker_factory.calls)
+
 
 class TestLaunchpadSecurityPolicy_getPrincipalsAccessLevel(TestCase):