launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/no-cross-distro-perm-bug-797599 into lp:launchpad with lp:~rvb/launchpad/init-series-bug-789091-initseries as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #797599 in Launchpad itself: "Copying archivepermissions cross-distro is wrong."
  https://bugs.launchpad.net/launchpad/+bug/797599

For more details, see:
https://code.launchpad.net/~rvb/launchpad/no-cross-distro-perm-bug-797599/+merge/64986

This branch remove the duplication of ArchivePermission when we initialize a series from a parent in another distribution.

= Tests =

./bin/test -cvv test_initialize_distroseries test_perm_copying_init_within_distro
./bin/test -cvv test_initialize_distroseries test_no_cross_distro_perm_copying

= Q/A =

On DF: initialize a series from a parent (for which one has upload rights) in another distribution and make sure the upload rights have not been transfered to the new series.
-- 
https://code.launchpad.net/~rvb/launchpad/no-cross-distro-perm-bug-797599/+merge/64986
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/no-cross-distro-perm-bug-797599 into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2011-06-17 12:41:48 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2011-06-17 12:41:58 +0000
@@ -439,16 +439,18 @@
                     parent_ps.name, parent_ps.description,
                     new_owner, distroseries=self.distroseries,
                     related_set=parent_ps)
-            # XXX: 2011-06-15 rvb bug=797599.
-            # Copying archivepermissions cross-distro is wrong.
-            self._store.execute("""
-                INSERT INTO Archivepermission
-                (person, permission, archive, packageset, explicit)
-                SELECT person, permission, %s, %s, explicit
-                FROM Archivepermission WHERE packageset = %s
-                """ % sqlvalues(
-                    self.distroseries.main_archive, child_ps.id,
-                    parent_ps.id))
+
+                # Only copy archivepermissions if this is not a cross-distro
+                # parent/child relation.
+                if self.distroseries.distribution.id in parent_distro_ids:
+                    self._store.execute("""
+                        INSERT INTO Archivepermission
+                        (person, permission, archive, packageset, explicit)
+                        SELECT person, permission, %s, %s, explicit
+                        FROM Archivepermission WHERE packageset = %s
+                        """ % sqlvalues(
+                            self.distroseries.main_archive, child_ps.id,
+                            parent_ps.id))
             parent_to_child[parent_ps] = child_ps
         # Copy the relations between sets, and the contents.
         for old_series_ps, new_series_ps in parent_to_child.items():

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-06-17 12:41:48 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-06-17 12:41:58 +0000
@@ -46,8 +46,9 @@
 
     layer = LaunchpadZopelessLayer
 
-    def setupParent(self, packages=None, format_selection=None):
-        parent = self.factory.makeDistroSeries()
+    def setupParent(self, packages=None, format_selection=None,
+                    distribution=None):
+        parent = self.factory.makeDistroSeries(distribution)
         pf = getUtility(IProcessorFamilySet).getByName('x86')
         parent_das = self.factory.makeDistroArchSeries(
             distroseries=parent, processorfamily=pf,
@@ -262,12 +263,54 @@
             direct_inclusion=True)
         parent_srcs = test1.getSourcesIncluded(direct_inclusion=True)
         self.assertEqual(parent_srcs, child_srcs)
-        # The uploader can also upload to the new distroseries.
-        self.assertTrue(
-            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
-                self.parent.main_archive, 'udev', uploader,
-                distroseries=self.parent))
-        self.assertTrue(
+
+    def test_perm_copying_init_within_distro(self):
+        # If the initialization happens within a distribution (i.e.
+        # both the parent and the child are in the same distribution),
+        # then the archive permission are copied.
+        self.parent, self.parent_das = self.setupParent()
+        uploader = self.factory.makePerson()
+        test1 = getUtility(IPackagesetSet).new(
+            u'test1', u'test 1 packageset', self.parent.owner,
+            distroseries=self.parent)
+        test1.addSources('udev')
+        getUtility(IArchivePermissionSet).newPackagesetUploader(
+            self.parent.main_archive, uploader, test1)
+        previous_series, unused = self.setupParent(
+            distribution=self.parent.distribution)
+        child = self.factory.makeDistroSeries(
+            previous_series=previous_series,
+            distribution=self.parent.distribution)
+        child = self._fullInitialize([self.parent], child=child)
+
+        # The uploader can upload to the new distroseries.
+        self.assertTrue(
+            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
+                self.parent.main_archive, 'udev', uploader,
+                distroseries=self.parent))
+        self.assertTrue(
+            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
+                child.main_archive, 'udev', uploader,
+                distroseries=child))
+
+    def test_no_cross_distro_perm_copying(self):
+        # No crss-distro archivepermissions copying should happen.
+        self.parent, self.parent_das = self.setupParent()
+        uploader = self.factory.makePerson()
+        test1 = getUtility(IPackagesetSet).new(
+            u'test1', u'test 1 packageset', self.parent.owner,
+            distroseries=self.parent)
+        test1.addSources('udev')
+        getUtility(IArchivePermissionSet).newPackagesetUploader(
+            self.parent.main_archive, uploader, test1)
+        child = self._fullInitialize([self.parent])
+
+        # The uploader cannot upload to the new distroseries.
+        self.assertTrue(
+            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
+                self.parent.main_archive, 'udev', uploader,
+                distroseries=self.parent))
+        self.assertFalse(
             getUtility(IArchivePermissionSet).isSourceUploadAllowed(
                 child.main_archive, 'udev', uploader,
                 distroseries=child))
@@ -529,23 +572,6 @@
         self.assertContentEqual(
             set(parent1_srcs).union(set(parent2_srcs)),
             child_srcs)
-        # The uploaders can also upload to the new distroseries.
-        self.assertTrue(
-            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
-                self.parent1.main_archive, 'udev', uploader1,
-                distroseries=self.parent1))
-        self.assertTrue(
-            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
-                child.main_archive, 'udev', uploader1,
-                distroseries=child))
-        self.assertTrue(
-            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
-                self.parent2.main_archive, 'libc6', uploader2,
-                distroseries=self.parent2))
-        self.assertTrue(
-            getUtility(IArchivePermissionSet).isSourceUploadAllowed(
-                child.main_archive, 'libc6', uploader2,
-                distroseries=child))
 
     def test_multiple_parents_format_selection_union(self):
         # The format selection for the derived series is the union of