launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/better-security-adapter-caching into lp:launchpad.

Commit message:
Improve caching of several delegated authorization checks.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1834625 in Launchpad itself: "hard timeouts from getPublishedBinaries against a source publication record in a private PPA"
  https://bugs.launchpad.net/launchpad/+bug/1834625

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/better-security-adapter-caching/+merge/369511

forwardCheckAuthenticated and forwardCheckUnauthenticated don't currently have access to the cache used by iter_authorization when doing delegated authorization checks.  In several cases, this is the only thing called by checkAuthenticated/checkUnauthenticated, and in that situation a simpler syntax is available: we can just yield the object and permission to check, and iter_authorization will do the delegated check for us.  This makes a significant difference when operating on many publishing objects in a single private archive: in the archive.getPublishedBinaries webservice method, about two-thirds of the non-trivial archive visibility checks that were previously uncached are now cached.

I think it's possible to do better, but it would require giving security adapters access to iter_authorization's cache so that forwardCheck* can use it when doing delegated checks, which is a somewhat more involved change.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/better-security-adapter-caching into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2019-03-26 20:51:38 +0000
+++ lib/lp/security.py	2019-07-01 13:28:12 +0000
@@ -304,7 +304,7 @@
         return False
 
     def checkAuthenticated(self, user):
-        return self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View')
+        yield self.obj, 'launchpad.View'
 
 
 class AnyLegitimatePerson(AuthorizationBase):
@@ -347,11 +347,10 @@
     usedfor = Interface
 
     def checkUnauthenticated(self):
-        return self.forwardCheckUnauthenticated(permission='launchpad.View')
+        yield self.obj, 'launchpad.View'
 
     def checkAuthenticated(self, user):
-        return self.forwardCheckAuthenticated(
-            user, permission='launchpad.View')
+        yield self.obj, 'launchpad.View'
 
 
 class AdminByAdminsTeam(AuthorizationBase):
@@ -2767,12 +2766,10 @@
     usedfor = IArchive
 
     def checkUnauthenticated(self):
-        return self.forwardCheckUnauthenticated(
-            permission='launchpad.SubscriberView')
+        yield self.obj, 'launchpad.SubscriberView'
 
     def checkAuthenticated(self, user):
-        return self.forwardCheckAuthenticated(
-            user, permission='launchpad.SubscriberView')
+        yield self.obj, 'launchpad.SubscriberView'
 
 
 class EditArchive(AuthorizationBase):
@@ -2998,12 +2995,10 @@
     usedfor = ISourcePackagePublishingHistory
 
     def checkUnauthenticated(self):
-        return self.forwardCheckUnauthenticated(
-            self.obj.archive, 'launchpad.SubscriberView')
+        yield self.obj.archive, 'launchpad.SubscriberView'
 
     def checkAuthenticated(self, user):
-        return self.forwardCheckAuthenticated(
-            user, self.obj.archive, 'launchpad.SubscriberView')
+        yield self.obj.archive, 'launchpad.SubscriberView'
 
 
 class EditPublishing(DelegatedAuthorization):
@@ -3294,8 +3289,7 @@
         return False
 
     def checkAuthenticated(self, user):
-        return self.forwardCheckAuthenticated(
-            user, self.obj.target, 'launchpad.Edit')
+        yield self.obj.target, 'launchpad.Edit'
 
 
 class ViewWebhookDeliveryJob(DelegatedAuthorization):

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	2018-02-01 18:44:21 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2019-07-01 13:28:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import, print_function, unicode_literals
@@ -14,7 +14,10 @@
     Unauthorized as LRUnauthorized,
     )
 from testtools import ExpectedException
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import getUtility
 
@@ -545,6 +548,34 @@
         recorder1, recorder2 = record_two_runs(get_binaries, create_bpph, 1)
         self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
+    def test_getPublishedBinaries_query_count_private_archive(self):
+        # getPublishedBinaries has a query count (almost) constant in the
+        # number of packages returned, even for private archives.
+        archive = self.factory.makeArchive(private=True)
+        uploader = self.factory.makePerson()
+        with person_logged_in(archive.owner):
+            archive.newComponentUploader(uploader, archive.default_component)
+        archive_url = api_url(archive)
+        ws = webservice_for_person(
+            uploader, permission=OAuthPermission.READ_PRIVATE)
+
+        def create_bpph():
+            with admin_logged_in():
+                self.factory.makeBinaryPackagePublishingHistory(
+                    archive=archive)
+
+        def get_binaries():
+            ws.named_get(archive_url, 'getPublishedBinaries').jsonBody()
+
+        recorder1, recorder2 = record_two_runs(get_binaries, create_bpph, 1)
+        # XXX cjwatson 2019-07-01: There are still some O(n) queries from
+        # security adapters (e.g. ViewSourcePackageRelease) that are
+        # currently hard to avoid.  To fix this properly, I think we somehow
+        # need to arrange for AuthorizationBase.forwardCheckAuthenticated to
+        # be able to use iter_authorization's cache.
+        self.assertThat(
+            recorder2, HasQueryCount(Equals(recorder1.count + 3), recorder1))
+
 
 class TestremoveCopyNotification(WebServiceTestCase):
     """Test removeCopyNotification."""