launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/derive-permissions-bug-643369 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/derive-permissions-bug-643369/+merge/60515

= Summary =
IDistroSeries.deriveDistroSeries() now needs lp.edit on the distro to work instead of the old kludge for requiring an admin or soyuz team member.

== Implementation details ==
Rip out the old code that checked the passed user argument, and move the method delcaration to the "Edit" interface so that lp.edit is now required.  This means for derived distros, you need to be a driver, and for Ubuntu you need to be the distro owner.  (There's historical baggage behind that)

== Tests ==
bin/test -cvv test_derivedistroseries
-- 
https://code.launchpad.net/~julian-edwards/launchpad/derive-permissions-bug-643369/+merge/60515
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/derive-permissions-bug-643369 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2011-04-17 18:00:45 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-05-10 14:21:55 +0000
@@ -172,16 +172,6 @@
                 "'%s': %s" % (version, error))
 
 
-class IDistroSeriesEditRestricted(Interface):
-    """IDistroSeries properties which require launchpad.Edit."""
-
-    @rename_parameters_as(dateexpected='date_targeted')
-    @export_factory_operation(
-        IMilestone, ['name', 'dateexpected', 'summary', 'code_name'])
-    def newMilestone(name, dateexpected=None, summary=None, code_name=None):
-        """Create a new milestone for this DistroSeries."""
-
-
 class IDistroSeriesPublic(
     ISeriesMixin, IHasAppointedDriver, IHasOwner, IBugTarget,
     ISpecificationGoal, IHasMilestones, IHasOfficialBugTags,
@@ -832,16 +822,35 @@
         :param format: The SourcePackageFormat to check.
         """
 
+    @operation_returns_collection_of(Interface)
+    @export_read_operation()
+    def getDerivedSeries():
+        """Get all `DistroSeries` derived from this one."""
+
+
+class IDistroSeriesEditRestricted(Interface):
+    """IDistroSeries properties which require launchpad.Edit."""
+
+    @rename_parameters_as(dateexpected='date_targeted')
+    @export_factory_operation(
+        IMilestone, ['name', 'dateexpected', 'summary', 'code_name'])
+    def newMilestone(name, dateexpected=None, summary=None, code_name=None):
+        """Create a new milestone for this DistroSeries."""
+
     @operation_parameters(
-        name=copy_field(name, required=True),
-        displayname=copy_field(displayname, required=False),
-        title=copy_field(title, required=False),
+        name=copy_field(IDistroSeriesPublic['name'], required=True),
+        displayname=copy_field(
+            IDistroSeriesPublic['displayname'], required=False),
+        title=copy_field(IDistroSeriesPublic['title'], required=False),
         summary=TextLine(
             title=_("The summary of the distroseries to derive."),
             required=False),
-        description=copy_field(description, required=False),
-        version=copy_field(version, required=False),
-        distribution=copy_field(distribution, required=False),
+        description=copy_field(
+            IDistroSeriesPublic['description'], required=False),
+        version=copy_field(
+            IDistroSeriesPublic['version'], required=False),
+        distribution=copy_field(
+            IDistroSeriesPublic['distribution'], required=False),
         architectures=List(
             title=_("The list of architectures to copy to the derived "
             "distroseries."), value_type=TextLine(),
@@ -891,10 +900,6 @@
             will be.
         """
 
-    @operation_returns_collection_of(Interface)
-    @export_read_operation()
-    def getDerivedSeries():
-        """Get all `DistroSeries` derived from this one."""
 
 
 class IDistroSeries(IDistroSeriesEditRestricted, IDistroSeriesPublic,

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-05-05 15:44:17 +0000
+++ lib/lp/registry/model/distroseries.py	2011-05-10 14:21:55 +0000
@@ -1952,11 +1952,6 @@
                            description=None, version=None,
                            architectures=(), packagesets=(), rebuild=False):
         """See `IDistroSeries`."""
-        # XXX StevenK bug=643369 This should be in the security adapter
-        # This should be allowed if the user is a driver for self.parent
-        # or the child.parent's drivers.
-        if not (user.inTeam('soyuz-team') or user.inTeam('admins')):
-            raise Unauthorized
         if distribution is None:
             distribution = self.distribution
         child = IStore(self).find(

=== modified file 'lib/lp/registry/tests/test_derivedistroseries.py'
--- lib/lp/registry/tests/test_derivedistroseries.py	2011-03-24 12:34:09 +0000
+++ lib/lp/registry/tests/test_derivedistroseries.py	2011-05-10 14:21:55 +0000
@@ -16,11 +16,11 @@
     IInitialiseDistroSeriesJobSource,
     )
 from lp.testing import (
+    ANONYMOUS,
     login,
-    logout,
+    login_person,
     TestCaseWithFactory,
     )
-from lp.testing.sampledata import ADMIN_EMAIL
 
 
 class TestDeriveDistroSeries(TestCaseWithFactory):
@@ -29,17 +29,15 @@
 
     def setUp(self):
         super(TestDeriveDistroSeries, self).setUp()
-        self.soyuz = self.factory.makeTeam(name='soyuz-team')
         self.parent = self.factory.makeDistroSeries()
+        removeSecurityProxy(self.parent).driver = self.factory.makePerson()
         self.child = self.factory.makeDistroSeries()
+        login_person(self.parent.driver)
 
     def test_no_permission_to_call(self):
-        login(ADMIN_EMAIL)
-        person = self.factory.makePerson()
-        logout()
+        login(ANONYMOUS)
         self.assertRaises(
-            Unauthorized, self.parent.deriveDistroSeries, person,
-            self.child.name)
+            Unauthorized, getattr, self.parent, "deriveDistroSeries")
 
     def test_no_distroseries_and_no_arguments(self):
         """Test that calling deriveDistroSeries() when the distroseries
@@ -48,7 +46,7 @@
         self.assertRaisesWithContent(
             DerivationError,
             'Display Name needs to be set when creating a distroseries.',
-            self.parent.deriveDistroSeries, self.soyuz.teamowner,
+            self.parent.deriveDistroSeries, self.parent.driver,
             'newdistro')
 
     def test_parent_is_not_set(self):
@@ -59,19 +57,19 @@
             DerivationError,
             ("DistroSeries {self.child.name} parent series is "
              "{self.parent.name}, but it must not be set").format(self=self),
-            self.parent.deriveDistroSeries, self.soyuz.teamowner,
+            self.parent.deriveDistroSeries, self.parent.driver,
             self.child.name, self.child.distribution)
 
     def test_create_new_distroseries(self):
         self.parent.deriveDistroSeries(
-            self.soyuz.teamowner, self.child.name, self.child.distribution)
+            self.parent.driver, self.child.name, self.child.distribution)
         [job] = list(
             getUtility(IInitialiseDistroSeriesJobSource).iterReady())
         self.assertEqual(job.distroseries, self.child)
 
     def test_create_fully_new_distroseries(self):
         self.parent.deriveDistroSeries(
-            self.soyuz.teamowner, 'deribuntu', displayname='Deribuntu',
+            self.parent.driver, 'deribuntu', displayname='Deribuntu',
             title='The Deribuntu', summary='Deribuntu',
             description='Deribuntu is great', version='11.11')
         [job] = list(
@@ -84,7 +82,7 @@
         self.factory.makeDistroSeries(name='bar')
         bar = self.factory.makeDistroSeries(name='bar')
         self.parent.deriveDistroSeries(
-            self.soyuz.teamowner, 'bar', distribution=bar.parent,
+            self.parent.driver, 'bar', distribution=bar.parent,
             displayname='Bar', title='The Bar', summary='Bar',
             description='Bar is good', version='1.0')
         [job] = list(