launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/minimise-bfj-interfaces into lp:launchpad.

Commit message:
Create new IBuildFarmJobDB and IPackageBuildDB interfaces, with the minimal attributes required for the deprecated BuildFarmJob/PackageBuild objects.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/minimise-bfj-interfaces/+merge/142058

Create new IBuildFarmJobDB and IPackageBuildDB interfaces, with the minimal attributes required for the deprecated BuildFarmJob/PackageBuild objects. This lets the security proxies verify that all attribute access (ignoring rSP) goes through the concrete BPB/SPRB/TTB objects, so we can trap writes in those objects.
-- 
https://code.launchpad.net/~wgrant/launchpad/minimise-bfj-interfaces/+merge/142058
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/minimise-bfj-interfaces into lp:launchpad.

=== modified file 'lib/lp/buildmaster/configure.zcml'
--- lib/lp/buildmaster/configure.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/buildmaster/configure.zcml	2013-01-07 05:31:20 +0000
@@ -48,13 +48,7 @@
     <class
         class="lp.buildmaster.model.buildfarmjob.BuildFarmJob">
         <allow
-            interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob" />
-        <require
-            permission="launchpad.Edit"
-            set_attributes="status"/>
-        <require
-            permission="launchpad.Admin"
-            set_attributes="failure_count"/>
+            interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJobDB" />
     </class>
     <securedutility
         component="lp.buildmaster.model.buildfarmjob.BuildFarmJob"
@@ -74,12 +68,8 @@
     <class
         class="lp.buildmaster.model.packagebuild.PackageBuild">
         <allow
-            interface="lp.buildmaster.interfaces.packagebuild.IPackageBuild" />
-        <require
-            permission="launchpad.Edit"
-            set_attributes="dependencies"/>
+            interface="lp.buildmaster.interfaces.packagebuild.IPackageBuildDB" />
     </class>
-
     <securedutility
         component="lp.buildmaster.model.packagebuild.PackageBuild"
         provides="lp.buildmaster.interfaces.packagebuild.IPackageBuildSource">

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py	2013-01-07 02:40:55 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py	2013-01-07 05:31:20 +0000
@@ -160,6 +160,27 @@
         """Job's finished.  Delete its supporting data."""
 
 
+class IBuildFarmJobDB(Interface):
+    """Operations on a `BuildFarmJob` DB row.
+
+    This is deprecated while it's flattened into the concrete implementations.
+    """
+
+    id = Attribute('The build farm job ID.')
+
+    job_type = Choice(
+        title=_("Job type"), required=True, readonly=True,
+        vocabulary=BuildFarmJobType,
+        description=_("The specific type of job."))
+
+    def getSpecificJob():
+        """Return the specific build job associated with this record.
+
+        :raises InconsistentBuildFarmJobError: if a specific job could not be
+            returned.
+        """
+
+
 class IBuildFarmJob(Interface):
     """Operations that jobs for the build farm must implement."""
 
@@ -262,13 +283,6 @@
         default=0,
         description=_("Number of consecutive failures for this job."))
 
-    def getSpecificJob():
-        """Return the specific build job associated with this record.
-
-        :raises InconsistentBuildFarmJobError: if a specific job could not be
-            returned.
-        """
-
     def makeJob():
         """Create the specific job relating this with an lp.services.job.
 

=== modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'
--- lib/lp/buildmaster/interfaces/packagebuild.py	2011-12-24 16:54:44 +0000
+++ lib/lp/buildmaster/interfaces/packagebuild.py	2013-01-07 05:31:20 +0000
@@ -32,11 +32,18 @@
 from lp.soyuz.interfaces.archive import IArchive
 
 
+class IPackageBuildDB(Interface):
+    """Operations on a `PackageBuild` DB row.
+
+    This is deprecated while it's flattened into the concrete implementations.
+    """
+
+    id = Attribute('The package build ID.')
+
+
 class IPackageBuild(IBuildFarmJob):
     """Attributes and operations specific to package build jobs."""
 
-    id = Attribute('The package build ID.')
-
     archive = exported(
         Reference(
             title=_('Archive'), schema=IArchive,
@@ -95,7 +102,7 @@
 
     def getLogFromSlave(build):
         """Get last buildlog from slave. 
-        
+
         :return: A Deferred that fires with the librarian ID of the log
             when the log is finished downloading.
         """

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjob.py'
--- lib/lp/buildmaster/tests/test_buildfarmjob.py	2013-01-07 03:39:28 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjob.py	2013-01-07 05:31:20 +0000
@@ -84,23 +84,24 @@
         # We flush the database updates to ensure sql defaults
         # are set for various attributes.
         flush_database_updates()
+        bfj = removeSecurityProxy(self.build_farm_job)
         self.assertEqual(
-            BuildStatus.NEEDSBUILD, self.build_farm_job.status)
+            BuildStatus.NEEDSBUILD, bfj.status)
         # The date_created is set automatically.
-        self.assertTrue(self.build_farm_job.date_created is not None)
+        self.assertTrue(bfj.date_created is not None)
         # The job type is required to create a build farm job.
         self.assertEqual(
-            BuildFarmJobType.PACKAGEBUILD, self.build_farm_job.job_type)
+            BuildFarmJobType.PACKAGEBUILD, bfj.job_type)
         # Failure count defaults to zero.
-        self.assertEqual(0, self.build_farm_job.failure_count)
+        self.assertEqual(0, bfj.failure_count)
         # Other attributes are unset by default.
-        self.assertEqual(None, self.build_farm_job.processor)
-        self.assertEqual(None, self.build_farm_job.virtualized)
-        self.assertEqual(None, self.build_farm_job.date_started)
-        self.assertEqual(None, self.build_farm_job.date_finished)
-        self.assertEqual(None, self.build_farm_job.date_first_dispatched)
-        self.assertEqual(None, self.build_farm_job.builder)
-        self.assertEqual(None, self.build_farm_job.log)
+        self.assertEqual(None, bfj.processor)
+        self.assertEqual(None, bfj.virtualized)
+        self.assertEqual(None, bfj.date_started)
+        self.assertEqual(None, bfj.date_finished)
+        self.assertEqual(None, bfj.date_first_dispatched)
+        self.assertEqual(None, bfj.builder)
+        self.assertEqual(None, bfj.log)
 
     def test_getSpecificJob_none(self):
         # An exception is raised if there is no related specific job.
@@ -124,7 +125,8 @@
         build_farm_job = getUtility(IBuildFarmJobSource).new(
             job_type=BuildFarmJobType.PACKAGEBUILD,
             date_created=ten_years_ago)
-        self.failUnlessEqual(ten_years_ago, build_farm_job.date_created)
+        self.failUnlessEqual(
+            ten_years_ago, removeSecurityProxy(build_farm_job).date_created)
 
 
 class TestBuildFarmJobMixin(TestCaseWithFactory):
@@ -164,9 +166,6 @@
         naked_bfj.date_finished = now + duration
         self.failUnlessEqual(duration, self.build_farm_job.duration)
 
-
-class TestBuildFarmJobSecurity(TestBuildFarmJobBase, TestCaseWithFactory):
-
     def test_view_build_farm_job(self):
         # Anonymous access can read public builds, but not edit.
         self.failUnlessEqual(

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2013-01-07 04:01:49 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2013-01-07 05:31:20 +0000
@@ -13,7 +13,7 @@
 
 from storm.store import Store
 from zope.component import getUtility
-from zope.security.interfaces import Unauthorized
+from zope.security.management import checkPermission
 from zope.security.proxy import removeSecurityProxy
 
 from lp.archiveuploader.uploadprocessor import parse_build_upload_leaf_name
@@ -89,8 +89,9 @@
 
     def test_default_values(self):
         # PackageBuild has a number of default values.
-        self.failUnlessEqual(None, self.package_build.distribution)
-        self.failUnlessEqual(None, self.package_build.distro_series)
+        pb = removeSecurityProxy(self.package_build)
+        self.failUnlessEqual(None, pb.distribution)
+        self.failUnlessEqual(None, pb.distro_series)
 
     def test_destroySelf_removes_BuildFarmJob(self):
         # Destroying a packagebuild also destroys the BuildFarmJob it
@@ -105,36 +106,6 @@
             BuildFarmJob, BuildFarmJob.id == build_farm_job_id)
         self.assertIs(None, result.one())
 
-    def test_view_package_build(self):
-        # Anonymous access can read public builds, but not edit.
-        self.failUnlessEqual(
-            None, self.package_build.dependencies)
-        self.assertRaises(
-            Unauthorized, setattr, self.package_build,
-            'dependencies', u'my deps')
-
-    def test_edit_package_build(self):
-        # An authenticated user who belongs to the owning archive team
-        # can edit the build.
-        login_person(self.package_build.archive.owner)
-        self.package_build.dependencies = u'My deps'
-        self.failUnlessEqual(
-            u'My deps', self.package_build.dependencies)
-
-        # But other users cannot.
-        other_person = self.factory.makePerson()
-        login_person(other_person)
-        self.assertRaises(
-            Unauthorized, setattr, self.package_build,
-            'dependencies', u'my deps')
-
-    def test_admin_package_build(self):
-        # Users with edit access can update attributes.
-        login('admin@xxxxxxxxxxxxx')
-        self.package_build.dependencies = u'My deps'
-        self.failUnlessEqual(
-            u'My deps', self.package_build.dependencies)
-
 
 class TestPackageBuildMixin(TestCaseWithFactory):
     """Test methods provided by PackageBuildMixin."""
@@ -216,6 +187,30 @@
             '%s-%s' % (now.strftime("%Y%m%d-%H%M%S"), build_cookie),
             upload_leaf)
 
+    def test_view_package_build(self):
+        # Anonymous access can read public builds, but not edit.
+        self.assertTrue(checkPermission('launchpad.View', self.package_build))
+        self.assertFalse(checkPermission('launchpad.Edit', self.package_build))
+
+    def test_edit_package_build(self):
+        # An authenticated user who belongs to the owning archive team
+        # can edit the build.
+        login_person(self.package_build.archive.owner)
+        self.assertTrue(checkPermission('launchpad.View', self.package_build))
+        self.assertTrue(checkPermission('launchpad.Edit', self.package_build))
+
+        # But other users cannot.
+        other_person = self.factory.makePerson()
+        login_person(other_person)
+        self.assertTrue(checkPermission('launchpad.View', self.package_build))
+        self.assertFalse(checkPermission('launchpad.Edit', self.package_build))
+
+    def test_admin_package_build(self):
+        # Users with edit access can update attributes.
+        login('admin@xxxxxxxxxxxxx')
+        self.assertTrue(checkPermission('launchpad.View', self.package_build))
+        self.assertTrue(checkPermission('launchpad.Edit', self.package_build))
+
 
 class TestPackageBuildSet(TestPackageBuildBase):