← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~rvb/launchpad/private-ppa-bug-890927-2 into lp:launchpad

 

Raphaël Badin has proposed merging lp:~rvb/launchpad/private-ppa-bug-890927-2 into lp:launchpad with lp:~rvb/launchpad/private-ppa-bug-890927 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #890927 in Launchpad itself: "private PPA Archive:+index timeouts"
  https://bugs.launchpad.net/launchpad/+bug/890927

For more details, see:
https://code.launchpad.net/~rvb/launchpad/private-ppa-bug-890927-2/+merge/84083

This branch improves LaunchpadSecurityPolicy.checkPermission in a way that allows the sub checks performed by a DelegatedAuthorization to be cached.

= Details =
This branch:
- changes ViewSourcePackagePublishingHistory to inherit from DelegatedAuthorization instead of manually delegation the check to obj.archive;
- adds an IDelegatedAuthorization interface;
- refactors LaunchpadSecurityPolicy to make it handle checks for authorizations implementing IDelegatedAuthorization;
- changes the utility function record_two_runs to also have it clear the permission cache before each run;
- adds a test_ppa_index_queries_count to lib/lp/soyuz/browser/tests/test_archive_packages.py.

= Pre-imp =
I spoke with Gary about this.  Another possibility would have been to have LaunchpadSecurityPolicy expose the cache via an interface and use that inside DelegatedAuthorization.{checkAuthenticated, checkUnauthenticated}. We decided that the cache should be kept an "implementation detail" inside LaunchpadSecurityPolicy and so the only clean solution is to have the checks performed by LaunchpadSecurityPolicy explicitly take care of authorizations inheriting from DelegatedAuthorization.

= Tests =
./bin/test -vvc test_archive_packages test_ppa_index_queries_count

= Q/A =
Check the Repeated SQL Statements of the oops created by:
https://qastaging.launchpad.net/~canonical-ux/+archive/walled-garden/++oops++
This query: http://paste.ubuntu.com/755903/ should not be listed in there.

Also, https://qastaging.launchpad.net/~canonical-isd-hackers/+archive/ppa/+index should not timeout anymore.
-- 
https://code.launchpad.net/~rvb/launchpad/private-ppa-bug-890927-2/+merge/84083
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/private-ppa-bug-890927-2 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-11-22 13:36:28 +0000
+++ lib/canonical/launchpad/security.py	2011-12-01 11:20:36 +0000
@@ -2410,13 +2410,13 @@
         yield self.obj.archive
 
 
-class ViewSourcePackagePublishingHistory(ViewArchive):
+class ViewSourcePackagePublishingHistory(DelegatedAuthorization):
     """Restrict viewing of source publications."""
     permission = "launchpad.View"
     usedfor = ISourcePackagePublishingHistory
 
-    def __init__(self, obj):
-        super(ViewSourcePackagePublishingHistory, self).__init__(obj.archive)
+    def iter_objects(self):
+        yield self.obj.archive
 
 
 class EditPublishing(DelegatedAuthorization):

=== 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-01 11:20:36 +0000
@@ -42,7 +42,10 @@
 from canonical.launchpad.webapp.metazcml import ILaunchpadPermission
 from canonical.lazr.canonicalurl import nearest_adapter
 from canonical.lazr.interfaces import IObjectPrivacy
-from lp.app.interfaces.security import IAuthorization
+from lp.app.interfaces.security import (
+    IAuthorization,
+    IDelegatedAuthorization,
+    )
 
 
 LAUNCHPAD_SECURITY_POLICY_CACHE_KEY = 'launchpad.security_policy_cache'
@@ -164,24 +167,28 @@
         participations = [participation
                           for participation in self.participations
                           if participation.principal is not system_user]
+        authorization = queryAdapter(
+            objecttoauthorize, IAuthorization, permission)
         if len(participations) == 0:
             principal = None
             cache = None
         elif len(participations) > 1:
             raise RuntimeError("More than one principal participating.")
         else:
+            # Check the cache.
             participation = participations[0]
-            if IApplicationRequest.providedBy(participation):
-                wd = participation.annotations.setdefault(
-                    LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
-                    weakref.WeakKeyDictionary())
-                cache = wd.setdefault(objecttoauthorize, {})
-                if permission in cache:
-                    return cache[permission]
-            else:
-                cache = None
+            cache = self._getCache(
+                participation, objecttoauthorize, permission)
+            if cache is not None and permission in cache:
+                return cache[permission]
             principal = participation.principal
-
+            # If the authorization is a DelegatedAuthorization: check
+            # the cache for the "sub-objects".
+            if IDelegatedAuthorization.providedBy(authorization):
+                res = self._queryCacheForMultipleObjects(
+                    participation, authorization.iter_objects(), permission)
+                if res is not None:
+                    return res
         if (principal is not None and
             not isinstance(principal, UnauthenticatedPrincipal)):
             access_level = self._getPrincipalsAccessLevel(
@@ -208,16 +215,19 @@
             #
             # The IAuthorization is a named adapter from objecttoauthorize,
             # providing IAuthorization, named after the permission.
-            authorization = queryAdapter(
-                objecttoauthorize, IAuthorization, permission)
             if authorization is None:
                 return False
             else:
-                if ILaunchpadPrincipal.providedBy(principal):
-                    result = authorization.checkAccountAuthenticated(
-                        principal.account)
+                if IDelegatedAuthorization.providedBy(authorization):
+                    result = self._checkAndCacheMultiple(
+                        participation, authorization.iter_objects(),
+                        permission)
                 else:
-                    result = authorization.checkUnauthenticated()
+                    if ILaunchpadPrincipal.providedBy(principal):
+                        result = authorization.checkAccountAuthenticated(
+                            principal.account)
+                    else:
+                        result = authorization.checkUnauthenticated()
                 if type(result) is not bool:
                     warnings.warn(
                         'authorization returning non-bool value: %r' %
@@ -226,6 +236,51 @@
                     cache[permission] = result
                 return bool(result)
 
+    def _checkAndCacheMultiple(self, participation, iterator, permission):
+        account = (
+            participation.principal.account
+            if ILaunchpadPrincipal.providedBy(participation.principal)
+            else None)
+        result = True
+        for sub_obj in iterator:
+            adapter = queryAdapter(sub_obj, IAuthorization, permission)
+            if adapter is None:
+                return False
+            else:
+                check = adapter.checkAccountAuthenticated(account)
+                sub_cache = self._getCache(
+                    participation, removeAllProxies(sub_obj),
+                    permission)
+                if sub_cache is not None:
+                    sub_cache[permission] = result
+                if check is False:
+                    return False
+        return True
+
+    def _getCache(self, participation, objecttoauthorize, permission):
+        cache = None
+        if IApplicationRequest.providedBy(participation):
+            wd = participation.annotations.setdefault(
+                LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
+                weakref.WeakKeyDictionary())
+            cache = wd.setdefault(objecttoauthorize, {})
+        return cache
+
+    def _queryCacheForMultipleObjects(self, participation, iterator,
+                                      permission):
+        sub_checks_all_true = True
+        for sub_obj in iterator:
+            cache = self._getCache(
+                participation, removeAllProxies(sub_obj), permission)
+            if cache is not None and permission in cache:
+                if cache[permission] is False:
+                    return False
+            else:
+                sub_checks_all_true = False
+        if sub_checks_all_true:
+            return True
+        return None
+
 
 def precache_permission_for_objects(participation, permission_name, objects):
     """Precaches the permission for the objects into the policy cache."""
@@ -259,7 +314,8 @@
             # LaunchpadBrowserRequest provides a ``clearSecurityPolicyCache``
             # method, but it is not in an interface, and not implemented by
             # all classes that implement IApplicationRequest.
-            del p.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_KEY]
+            if LAUNCHPAD_SECURITY_POLICY_CACHE_KEY in p.annotations:
+                del p.annotations[LAUNCHPAD_SECURITY_POLICY_CACHE_KEY]
 
 
 class LaunchpadPermissiveSecurityPolicy(PermissiveSecurityPolicy):

=== 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-01 11:20:36 +0000
@@ -7,6 +7,7 @@
 
 __all__ = [
     'IAuthorization',
+    'IDelegatedAuthorization',
     ]
 
 from zope.interface import Interface
@@ -26,3 +27,13 @@
 
         The argument `account` is the account who is authenticated.
         """
+
+
+class IDelegatedAuthorization(Interface):
+    """Authorization policy that delegates the actual authorization check
+    to a set of sub-objects.
+
+    """
+
+    def iter_objects():
+        """Returns an iterator over the sub-objects"""

=== 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-01 11:20:36 +0000
@@ -15,7 +15,10 @@
 from zope.interface import implements
 from zope.security.permission import checkPermission
 
-from lp.app.interfaces.security import IAuthorization
+from lp.app.interfaces.security import (
+    IAuthorization,
+    IDelegatedAuthorization,
+    )
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.role import IPersonRoles
 
@@ -108,6 +111,7 @@
 
 
 class DelegatedAuthorization(AuthorizationBase):
+    implements(IDelegatedAuthorization)
 
     def __init__(self, obj, forwarded_object=None, permission=None):
         super(DelegatedAuthorization, self).__init__(obj)

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2011-05-27 21:12:25 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2011-12-01 11:20:36 +0000
@@ -22,13 +22,19 @@
 from canonical.launchpad.testing.pages import get_feedback_messages
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.authentication import LaunchpadPrincipal
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.app.utilities.celebrities import ILaunchpadCelebrities
 from lp.soyuz.browser.archive import ArchiveNavigationMenu
+from lp.soyuz.enums import PackagePublishingStatus
 from lp.testing import (
+    celebrity_logged_in,
     login,
     login_person,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     )
 from lp.testing._webservice import QueryCollector
@@ -258,3 +264,48 @@
             url = canonical_url(ppa) + "/+packages"
         browser.open(url)
         self.assertThat(collector, HasQueryCount(Equals(expected_count)))
+
+
+class TestP3APackagesQueryCount(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestP3APackagesQueryCount, self).setUp()
+        self.team = self.factory.makeTeam()
+        login_person(self.team.teamowner)
+        self.person = self.factory.makePerson()
+
+        self.private_ppa = self.factory.makeArchive(
+            owner=self.team, private=True)
+        self.private_ppa.newSubscription(
+            self.person, registrant=self.team.teamowner)
+
+    def createPackage(self):
+        with celebrity_logged_in('admin'):
+            pkg = self.factory.makeBinaryPackagePublishingHistory(
+            status=PackagePublishingStatus.PUBLISHED,
+            archive=self.private_ppa)
+        return pkg
+
+    def monkeyPatchView(self, view):
+        @property
+        def page_title(self):
+            return "title"
+
+        setattr(view, 'page_title', page_title)
+        return view
+
+    def test_ppa_index_queries_count(self):
+        def ppa_index_render():
+            with person_logged_in(self.person):
+                view = create_initialized_view(
+                    self.private_ppa, '+index',
+                    principal=self.person)
+                self.monkeyPatchView(view)
+                view.render()
+        recorder1, recorder2 = record_two_runs(
+            ppa_index_render, self.createPackage, 2, 2)
+
+        self.assertThat(
+            recorder2, HasQueryCount(LessThan(recorder1.count + 1)))

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-12-01 11:20:35 +0000
+++ lib/lp/testing/__init__.py	2011-12-01 11:20:36 +0000
@@ -117,6 +117,7 @@
     start_sql_logging,
     stop_sql_logging,
     )
+from canonical.launchpad.webapp.authorization import clear_cache
 from canonical.launchpad.webapp.interaction import ANONYMOUS
 from canonical.launchpad.webapp.servers import (
     LaunchpadTestRequest,
@@ -329,6 +330,7 @@
     # called after {item_creator} has been run {first_round_number}
     # times.
     flush_database_caches()
+    clear_cache()
     with StormStatementRecorder() as recorder1:
         tested_method()
     # Run {item_creator} {second_round_number} more times.
@@ -338,6 +340,7 @@
         item_creator()
     # Record again the number of queries issued.
     flush_database_caches()
+    clear_cache()
     with StormStatementRecorder() as recorder2:
         tested_method()
     return recorder1, recorder2