Merge lp:~cjwatson/launchpad/archive-getallpermissions into lp:launchpad

[Colin Watson]

Add exported Archive.getAllPermissions method.

== Summary ==

Bug 1030936: The Ubuntu Developer Membership Board would like to be able to correlate the list of permissions granted on the Ubuntu archive against the membership of the ~ubuntu-dev team. Unfortunately, due to the variety of possible permission types and targets, it's prohibitively expensive to get such a list using the current API.

== Proposed fix ==

Add an exported Archive.getAllPermissions method, amounting to ArchivePermission.selectBy(archive=archive).

== LOC Rationale ==

+50. I have 3911 lines of credit; this should come out in the wash.

== Tests ==

bin/test -vvct lib/lp/soyuz/stories/webservice/xx-archive.txt

== Demo and Q/A ==

Look through the contents of lp.distributions["ubuntu"].main_archive.getAllPermissions() on qastaging.

== Lint ==

One pre-existing false positive and some pre-existing doctest cruft which I don't think is worth attempting to fix here:

./lib/lp/soyuz/model/archive.py
     346: redefinition of function 'private' from line 342
./lib/lp/soyuz/stories/webservice/xx-archive.txt
      43: want exceeds 78 characters.
      47: want exceeds 78 characters.
     173: want exceeds 78 characters.
     190: want exceeds 78 characters.
     207: want exceeds 78 characters.
     224: want exceeds 78 characters.
     397: want exceeds 78 characters.
     458: want exceeds 78 characters.
     589: want exceeds 78 characters.
     648: want exceeds 78 characters.

[Robert Collins (lifeless)]

This has a hidden late evaluation case, which if you're dealing with a
large data set will punish you: you are returning team/person objects,
but they are not always visible to the user, so you need to eager
evaluate visibility (or push it down into your query).

[Robert Collins (lifeless)]

(Unless you intend to always return permissions even if the person
calling the API can't see the involved teams [not a problem for
Ubuntu, but its implicit in the model...] - that would require a
different approach.)

[Steve Kowalik (stevenk)]

I don't think we need to worry about visibility for this case -- you can't create ArchivePermissions for PPAs, and the governance around adding permissions for the Ubuntu primary archive would quickly quash a private team, so I think it's fine.

review: Approve (code)

[William Grant (wgrant)]

ArchivePermissions can be created for PPAs, and private archives (PPA or distribution) will probably want private teams in future. We need to either restrict private teams from participating here, or preload permissions.

review: Needs Fixing (code)

[William Grant (wgrant)]

Doesn't fix the private team permission check case, but it's unlikely to be a problem right now, and we don't have a truly good solution yet.

review: Approve (code)

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2012-08-08 12:05:10 +0000
+++ lib/lp/_schema_circular_imports.py	2012-08-10 17:20:25 +0000
@@ -393,6 +393,7 @@
 patch_collection_property(IArchive, 'dependencies', IArchiveDependency)
 patch_collection_property(
     IArchive, 'enabled_restricted_families', IProcessorFamily)
+patch_collection_return_type(IArchive, 'getAllPermissions', IArchivePermission)
 patch_collection_return_type(
     IArchive, 'getPermissionsForPerson', IArchivePermission)
 patch_collection_return_type(

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	2012-07-23 11:51:21 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2012-08-10 17:20:25 +0000
@@ -12,25 +12,31 @@
     Unauthorized as LRUnauthorized,
     )
 from testtools import ExpectedException
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.enums import (
+    ArchivePermissionType,
     ArchivePurpose,
     PackagePublishingStatus,
     )
+from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
+from lp.soyuz.model.archivepermission import ArchivePermission
 from lp.testing import (
     celebrity_logged_in,
     launchpadlib_for,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     WebServiceTestCase,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import LaunchpadWebServiceCaller
 
 
@@ -40,12 +46,12 @@
     def setUp(self):
         TestCaseWithFactory.setUp(self)
         with celebrity_logged_in('admin'):
-            archive = self.factory.makeArchive(
+            self.archive = self.factory.makeArchive(
                 purpose=ArchivePurpose.PRIMARY)
             distroseries = self.factory.makeDistroSeries(
-                distribution=archive.distribution)
+                distribution=self.archive.distribution)
             person = self.factory.makePerson()
-            distro_name = archive.distribution.name
+            distro_name = self.archive.distribution.name
             distroseries_name = distroseries.name
             person_name = person.name
 
@@ -83,6 +89,24 @@
             "Not permitted to upload to the UPDATES pocket in a series "
             "in the 'DEVELOPMENT' state.", e.content)
 
+    def test_getAllPermissions_constant_query_count(self):
+        # getAllPermissions has a query count constant in the number of
+        # permissions and people.
+        with celebrity_logged_in('admin'):
+            main = getUtility(IComponentSet)["main"]
+            ArchivePermission(
+                archive=self.archive, person=self.factory.makePerson(),
+                component=main, permission=ArchivePermissionType.UPLOAD)
+        with StormStatementRecorder() as recorder_one:
+            list(self.main_archive.getAllPermissions())
+        with celebrity_logged_in('admin'):
+            ArchivePermission(
+                archive=self.archive, person=self.factory.makePerson(),
+                component=main, permission=ArchivePermissionType.UPLOAD)
+        with StormStatementRecorder() as recorder:
+            list(self.main_archive.getAllPermissions())
+        self.assertThat(recorder, HasQueryCount(Equals(recorder_one.count)))
+
 
 class TestExternalDependencies(WebServiceTestCase):
 

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-08-08 16:34:39 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-08-10 17:20:25 +0000
@@ -1160,6 +1160,16 @@
             could not be found.
         """
 
+    # Really IArchivePermission, set below to avoid circular import.
+    @operation_returns_collection_of(Interface)
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getAllPermissions():
+        """Return all `IArchivePermission` records for this archive.
+
+        :return: A list of `IArchivePermission` records.
+        """
+
     @operation_parameters(person=Reference(schema=IPerson))
     # Really IArchivePermission, set below to avoid circular import.
     @operation_returns_collection_of(Interface)

=== modified file 'lib/lp/soyuz/interfaces/archivepermission.py'
--- lib/lp/soyuz/interfaces/archivepermission.py	2012-08-01 04:18:50 +0000
+++ lib/lp/soyuz/interfaces/archivepermission.py	2012-08-10 17:20:25 +0000
@@ -63,6 +63,7 @@
             title=_("The permission type being granted."),
             values=ArchivePermissionType, readonly=False, required=True))
 
+    personID = Attribute("DB ID for person.")
     person = exported(
         PublicPersonChoice(
             title=_("Person"),
@@ -170,6 +171,12 @@
         authenticated in that context.
         """
 
+    def permissionsForArchive(archive):
+        """All `ArchivePermission` records for the archive.
+
+        :param archive: An `IArchive`.
+        """
+
     def permissionsForPerson(person, archive):
         """All `ArchivePermission` records for the person.
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-08-08 16:34:39 +0000
+++ lib/lp/soyuz/model/archive.py	2012-08-10 17:20:25 +0000
@@ -1058,6 +1058,11 @@
         return permission_set.checkAuthenticated(
             user, self, perm_type, item, distroseries=distroseries)
 
+    def getAllPermissions(self):
+        """See `IArchive`."""
+        permission_set = getUtility(IArchivePermissionSet)
+        return permission_set.permissionsForArchive(self)
+
     def getPermissionsForPerson(self, person):
         """See `IArchive`."""
         permission_set = getUtility(IArchivePermissionSet)

=== modified file 'lib/lp/soyuz/model/archivepermission.py'
--- lib/lp/soyuz/model/archivepermission.py	2012-08-01 04:18:50 +0000
+++ lib/lp/soyuz/model/archivepermission.py	2012-08-10 17:20:25 +0000
@@ -10,6 +10,8 @@
     'ArchivePermissionSet',
     ]
 
+from operator import attrgetter
+
 from lazr.enum import DBItem
 from sqlobject import (
     BoolCol,
@@ -30,6 +32,7 @@
 
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.sourcepackagename import (
     ISourcePackageName,
@@ -224,6 +227,16 @@
                 ISourcePackageNameSet)[sourcepackagename]
         return sourcepackagename
 
+    def _precachePersonsForPermissions(self, permissions):
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            set(map(attrgetter("personID"), permissions))))
+        return permissions
+
+    def permissionsForArchive(self, archive):
+        """See `IArchivePermissionSet`."""
+        return self._precachePersonsForPermissions(
+            ArchivePermission.selectBy(archive=archive))
+
     def permissionsForPerson(self, archive, person):
         """See `IArchivePermissionSet`."""
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)

=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt	2012-08-08 16:34:39 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2012-08-10 17:20:25 +0000
@@ -229,14 +229,7 @@
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Permission collections can be retrieved with custom operations on the
-archive.
-
-`getPermissionsForPerson` returns all the permissions that a user has.
-
-    >>> ubuntu_team = user_webservice.get("/~ubuntu-team").jsonBody()
-    >>> permissions = user_webservice.named_get(
-    ...     ubuntutest['main_archive_link'], 'getPermissionsForPerson',
-    ...     person=ubuntu_team['self_link']).jsonBody()
+archive.  First, define some general helper functions.
 
     >>> def permission_entry_sort_key(entry):
     ...      return (entry['permission'],
@@ -255,6 +248,40 @@
     ...         print entry['pocket']
     ...         print entry['distroseries_link']
 
+`getAllPermissions` returns all permissions on the archive.
+
+    >>> ubuntu_devel = user_webservice.get(
+    ...     '/distros', api_version='devel').jsonBody()['entries'][0]
+
+    >>> def show_all_permissions(archive):
+    ...     permissions = user_webservice.get(
+    ...         '%s?ws.op=getAllPermissions&ws.size=50' % archive,
+    ...         api_version='devel').jsonBody()
+    ...     show_permission_entries(permissions)
+
+    >>> show_all_permissions(ubuntu_devel['main_archive_link'])
+    Archive Upload Rights ...~carlos None mozilla-firefox None None
+    Archive Upload Rights ...~ubuntu-team main None None None
+    Archive Upload Rights ...~ubuntu-team restricted None None None
+    Archive Upload Rights ...~ubuntu-team universe None None None
+    Queue Administration Rights ...~name12 main None None None
+    Queue Administration Rights ...~name12 multiverse None None None
+    Queue Administration Rights ...~name12 restricted None None None
+    Queue Administration Rights ...~name12 universe None None None
+    Queue Administration Rights ...~no-team-memberships multiverse None None None
+    Queue Administration Rights ...~no-team-memberships universe None None None
+    Queue Administration Rights ...~ubuntu-team main None None None
+    Queue Administration Rights ...~ubuntu-team partner None None None
+    Queue Administration Rights ...~ubuntu-team restricted None None None
+    Queue Administration Rights ...~ubuntu-team universe None None None
+
+`getPermissionsForPerson` returns all the permissions that a user has.
+
+    >>> ubuntu_team = user_webservice.get("/~ubuntu-team").jsonBody()
+    >>> permissions = user_webservice.named_get(
+    ...     ubuntutest['main_archive_link'], 'getPermissionsForPerson',
+    ...     person=ubuntu_team['self_link']).jsonBody()
+
     >>> show_permission_entries(permissions)
     Archive Upload Rights ...~ubuntu-team main None None None
     Archive Upload Rights ...~ubuntu-team universe None None None
@@ -589,9 +616,6 @@
 getUploadersForPocket returns all the permissions where someone can upload
 to a particular pocket:
 
-    >>> ubuntu_devel = webservice.get(
-    ...     '/distros', api_version='devel').jsonBody()['entries'][0]
-
     >>> def show_pocket_permissions(pocket):
     ...     permissions = user_webservice.named_get(
     ...         ubuntu_devel['main_archive_link'], 'getUploadersForPocket',