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
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
