launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05781
[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
