Merge lp:~cjwatson/launchpad/better-security-adapter-caching into lp:launchpad

Proposed by Colin Watson on 2019-07-01
Status: Merged
Merged at revision: 18999
Proposed branch: lp:~cjwatson/launchpad/better-security-adapter-caching
Merge into: lp:launchpad
Diff against target: 123 lines (+41/-16)
2 files modified
lib/lp/security.py (+8/-14)
lib/lp/soyuz/browser/tests/test_archive_webservice.py (+33/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/better-security-adapter-caching
Reviewer Review Type Date Requested Status
William Grant code 2019-07-01 Approve on 2019-07-02
Review via email: mp+369511@code.launchpad.net

Commit message

Improve caching of several delegated authorization checks.

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/security.py'
2--- lib/lp/security.py 2019-03-26 20:51:38 +0000
3+++ lib/lp/security.py 2019-07-01 13:28:12 +0000
4@@ -304,7 +304,7 @@
5 return False
6
7 def checkAuthenticated(self, user):
8- return self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View')
9+ yield self.obj, 'launchpad.View'
10
11
12 class AnyLegitimatePerson(AuthorizationBase):
13@@ -347,11 +347,10 @@
14 usedfor = Interface
15
16 def checkUnauthenticated(self):
17- return self.forwardCheckUnauthenticated(permission='launchpad.View')
18+ yield self.obj, 'launchpad.View'
19
20 def checkAuthenticated(self, user):
21- return self.forwardCheckAuthenticated(
22- user, permission='launchpad.View')
23+ yield self.obj, 'launchpad.View'
24
25
26 class AdminByAdminsTeam(AuthorizationBase):
27@@ -2767,12 +2766,10 @@
28 usedfor = IArchive
29
30 def checkUnauthenticated(self):
31- return self.forwardCheckUnauthenticated(
32- permission='launchpad.SubscriberView')
33+ yield self.obj, 'launchpad.SubscriberView'
34
35 def checkAuthenticated(self, user):
36- return self.forwardCheckAuthenticated(
37- user, permission='launchpad.SubscriberView')
38+ yield self.obj, 'launchpad.SubscriberView'
39
40
41 class EditArchive(AuthorizationBase):
42@@ -2998,12 +2995,10 @@
43 usedfor = ISourcePackagePublishingHistory
44
45 def checkUnauthenticated(self):
46- return self.forwardCheckUnauthenticated(
47- self.obj.archive, 'launchpad.SubscriberView')
48+ yield self.obj.archive, 'launchpad.SubscriberView'
49
50 def checkAuthenticated(self, user):
51- return self.forwardCheckAuthenticated(
52- user, self.obj.archive, 'launchpad.SubscriberView')
53+ yield self.obj.archive, 'launchpad.SubscriberView'
54
55
56 class EditPublishing(DelegatedAuthorization):
57@@ -3294,8 +3289,7 @@
58 return False
59
60 def checkAuthenticated(self, user):
61- return self.forwardCheckAuthenticated(
62- user, self.obj.target, 'launchpad.Edit')
63+ yield self.obj.target, 'launchpad.Edit'
64
65
66 class ViewWebhookDeliveryJob(DelegatedAuthorization):
67
68=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
69--- lib/lp/soyuz/browser/tests/test_archive_webservice.py 2018-02-01 18:44:21 +0000
70+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py 2019-07-01 13:28:12 +0000
71@@ -1,4 +1,4 @@
72-# Copyright 2010-2018 Canonical Ltd. This software is licensed under the
73+# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
74 # GNU Affero General Public License version 3 (see the file LICENSE).
75
76 from __future__ import absolute_import, print_function, unicode_literals
77@@ -14,7 +14,10 @@
78 Unauthorized as LRUnauthorized,
79 )
80 from testtools import ExpectedException
81-from testtools.matchers import MatchesStructure
82+from testtools.matchers import (
83+ Equals,
84+ MatchesStructure,
85+ )
86 import transaction
87 from zope.component import getUtility
88
89@@ -545,6 +548,34 @@
90 recorder1, recorder2 = record_two_runs(get_binaries, create_bpph, 1)
91 self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
92
93+ def test_getPublishedBinaries_query_count_private_archive(self):
94+ # getPublishedBinaries has a query count (almost) constant in the
95+ # number of packages returned, even for private archives.
96+ archive = self.factory.makeArchive(private=True)
97+ uploader = self.factory.makePerson()
98+ with person_logged_in(archive.owner):
99+ archive.newComponentUploader(uploader, archive.default_component)
100+ archive_url = api_url(archive)
101+ ws = webservice_for_person(
102+ uploader, permission=OAuthPermission.READ_PRIVATE)
103+
104+ def create_bpph():
105+ with admin_logged_in():
106+ self.factory.makeBinaryPackagePublishingHistory(
107+ archive=archive)
108+
109+ def get_binaries():
110+ ws.named_get(archive_url, 'getPublishedBinaries').jsonBody()
111+
112+ recorder1, recorder2 = record_two_runs(get_binaries, create_bpph, 1)
113+ # XXX cjwatson 2019-07-01: There are still some O(n) queries from
114+ # security adapters (e.g. ViewSourcePackageRelease) that are
115+ # currently hard to avoid. To fix this properly, I think we somehow
116+ # need to arrange for AuthorizationBase.forwardCheckAuthenticated to
117+ # be able to use iter_authorization's cache.
118+ self.assertThat(
119+ recorder2, HasQueryCount(Equals(recorder1.count + 3), recorder1))
120+
121
122 class TestremoveCopyNotification(WebServiceTestCase):
123 """Test removeCopyNotification."""