launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/checkcopy-permissions-betterer into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #777911 in Launchpad itself: "Testing for the ability of the security team to copy any package into the Security pocket should be moved from test_distroseries to test_copypackage."
  https://bugs.launchpad.net/launchpad/+bug/777911
  Bug #780429 in Launchpad itself: "CopyChecker.checkCopy uses check_permission badly"
  https://bugs.launchpad.net/launchpad/+bug/780429

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/checkcopy-permissions-betterer/+merge/60483

CopyChecker.checkCopy recently gained a check to assist in the perpetuation of the ubuntu-security permission special case. It is bad for three reasons: 1) check_permission isn't to be used in model code, nor code that takes a user explicitly for permission checking -- and checkCopy falls in both categories. 2) It always checks for launchpad.Append on the primary archive, rather than the archive being copied to. 3) The hack is only needed in Archive.syncSource(s), so it should not plague the rest of the codebase.

This branch moves the special case back up into Archive, changing the _copySources method underlying syncSource(s) to call do_copy(check_permissions=False), returning to the pre-r12988 behaviour. I've also replaced the (now failing) DSD-based copy test with one of the actual API used by ubuntu-security.
-- 
https://code.launchpad.net/~wgrant/launchpad/checkcopy-permissions-betterer/+merge/60483
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/checkcopy-permissions-betterer into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-09 15:50:52 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-10 10:45:55 +0000
@@ -1248,33 +1248,6 @@
         self.assertPackageCopied(
             derived_series, 'my-src-name', versions['parent'], view)
 
-    def test_sync_append_main_archive(self):
-        # A user with lp.Append on the main archive (e.g. members of
-        # ubuntu-security on an ubuntu series) can sync packages.
-        # XXX: rvb 2011-05-05 bug=777911: This check should be refactored
-        # and moved to lib/lp/soyuz/scripts/tests/test_copypackage.py.
-        versions = {
-            'base': '1.0',
-            'derived': '1.0derived1',
-            'parent': '1.0-1',
-        }
-        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        derived_series, parent_series, sourcepackagename = self._setUpDSD(
-            'my-src-name', distribution=ubuntu, versions=versions)
-        ubuntu_security = getUtility(IPersonSet).getByName('ubuntu-security')
-        set_derived_series_sync_feature_flag(self)
-        with person_logged_in(ubuntu_security):
-            self.assertTrue(
-                check_permission(
-                    'launchpad.Append', derived_series.main_archive))
-            view = self._syncAndGetView(
-                derived_series, ubuntu_security, ['my-src-name'])
-
-            self.assertTrue(view.canPerformSync())
-
-        self.assertPackageCopied(
-            derived_series, 'my-src-name', versions['parent'], view)
-
 
 class TestDistroSeriesNeedsPackagesView(TestCaseWithFactory):
     """Test the distroseries +needs-packaging view."""

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-05-09 17:55:48 +0000
+++ lib/lp/soyuz/model/archive.py	2011-05-10 10:45:55 +0000
@@ -1523,9 +1523,14 @@
         else:
             series = None
 
-        # Perform the copy, may raise CannotCopy.
+        # Perform the copy, may raise CannotCopy. Don't do any further
+        # permission checking: this method is protected by
+        # launchpad.Append, which is mostly more restrictive than archive
+        # permissions, except that it also allows ubuntu-security to
+        # copy packages they wouldn't otherwise be able to.
         do_copy(
-            sources, self, series, pocket, include_binaries, person=person)
+            sources, self, series, pocket, include_binaries, person=person,
+            check_permissions=False)
 
     def getAuthToken(self, person):
         """See `IArchive`."""

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-05-05 15:01:04 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-05-10 10:45:55 +0000
@@ -420,14 +420,7 @@
                     person, series, sourcepackagename, destination_component,
                     pocket, strict_component=strict_component)
                 if reason is not None:
-                    # launchpad.Append on the main archive is sufficient
-                    # to copy arbitrary packages. This allows for
-                    # ubuntu's security team to sync sources into the
-                    # primary archive (bypassing the queue and
-                    # annoncements).
-                    if not check_permission(
-                        'launchpad.Append', series.main_archive):
-                        raise CannotCopy(reason)
+                    raise CannotCopy(reason)
 
         if series not in self.archive.distribution.series:
             raise CannotCopy(

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-05-05 13:01:50 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-05-10 10:45:55 +0000
@@ -1744,3 +1744,37 @@
         two_hours_earlier = one_hour_earlier - one_hour_step
         self.assertEqual(3, cprov_archive.getPublishedSources(
             created_since_date=two_hours_earlier).count())
+
+
+class TestSyncSource(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_security_team_can_copy_to_primary(self):
+        # A member of ubuntu-security can use syncSource on any package
+        # in the Ubuntu primary archive, regardless of their normal
+        # upload permissions.
+        # This is until we can open syncSource up more widely and sort
+        # out the permissions that everyone needs.
+        with celebrity_logged_in('admin'):
+            security_person = self.factory.makePerson()
+            getUtility(ILaunchpadCelebrities).ubuntu_security.addMember(
+                security_person, security_person)
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        source = self.factory.makeSourcePackagePublishingHistory(
+            archive=self.factory.makeArchive(purpose=ArchivePurpose.PPA),
+            distroseries=ubuntu.currentseries)
+        self.assertEqual(
+            0,
+            ubuntu.main_archive.getPublishedSources(
+                name=source.source_package_name).count())
+        with person_logged_in(security_person):
+            ubuntu.main_archive.syncSource(
+                source_name=source.source_package_name,
+                version=source.source_package_version,
+                from_archive=source.archive,
+                to_pocket='Security')
+        self.assertEqual(
+            1,
+            ubuntu.main_archive.getPublishedSources(
+                name=source.source_package_name).count())