Merge lp:~bac/launchpad/bug-828914-test into lp:launchpad

[Brad Crittenden]

[r=benji][bug=828914] Use the proper security adapter when building a SourcePackageRecipe into a private PPA and the user has ArchivePermission to upload but isn't a member of the team that owns the PPA.

= Summary =

When a user is not a member of the team owning a private PPA, but does
have upload rights via an ArchivePermission, tries to request a recipe
build there is an error in the security adapter which causes an OOPS.
The problem is the wrong security adapter is being used.

== Proposed fix ==

Use getAdapter to select the proper adapter.

== Pre-implementation notes ==

Talks with Julian, Aaron, and William.

== Tests ==

bin/test -vvt test_sourcepackagerecipe

== Demo and Q/A ==

* Create a private PPA for a team you are a member.
* Using the API, create a new component uploader archive permission
  for your user.

  team = lp.people['team']
  p3a = team.ppas[0]
  p3a.newComponentUploader(person=lp.me, component_name="main")

* Leave the team.
* Now create a recipe for one of your branches and use the private
  team PPA as the build destination.
* Select 'Request build'.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/buildmaster/model/packagebuild.py
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

Will fix these lint issues later.

./lib/lp/code/model/tests/test_sourcepackagerecipe.py
     647: local variable 'build' is assigned to but never used
    1100: local variable 'build' is assigned to but never used
     105: E251 no spaces around keyword / parameter equals
     307: E225 missing whitespace around operator
     313: E225 missing whitespace around operator
     500: E225 missing whitespace around operator
     642: E225 missing whitespace around operator
    1072: E225 missing whitespace around operator
    1092: E225 missing whitespace around operator
./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
     483: Line exceeds 78 characters.
    1155: Line exceeds 78 characters.
     483: E501 line too long (80 characters)
     595: E225 missing whitespace around operator
    1042: E225 missing whitespace around operator

[Benji York (benji)]

Looks good.

review: Approve (code)

[Francis J. Lacoste (flacoste)]

If you look through the various security.py files, you'll see that we have a bunch of other places where the security policy implementation is being instantiated directly instead of using getAdapter(). These are all kind of places such problem may creep up in the future.

Care to do a sweep as a separate branch?

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-08-28 07:29:11 +0000
+++ lib/canonical/launchpad/security.py	2011-08-31 20:28:59 +0000
@@ -1649,7 +1649,8 @@
 
     def _checkBuildPermission(self, user=None):
         """Check access to `IPackageBuild` for this job."""
-        permission = ViewBinaryPackageBuild(self.obj.build)
+        permission = getAdapter(
+            self.obj.build, IAuthorization, self.permission)
         if user is None:
             return permission.checkUnauthenticated()
         else:

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2011-05-16 23:41:48 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2011-08-31 20:28:59 +0000
@@ -280,10 +280,12 @@
             specific_job.job.suspend()
 
         duration_estimate = self.estimateDuration()
+        job = specific_job.job
+        processor = specific_job.processor
         queue_entry = BuildQueue(
             estimated_duration=duration_estimate,
             job_type=self.build_farm_job_type,
-            job=specific_job.job, processor=specific_job.processor,
+            job=job, processor=processor,
             virtualized=specific_job.virtualized)
         Store.of(self).add(queue_entry)
         return queue_entry

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-08-31 12:23:13 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-08-31 20:28:59 +0000
@@ -480,9 +480,9 @@
 
         with recipe_parser_newest_version(145.115):
             recipe = dedent(u'''\
-                # bzr-builder format 145.115 deb-version {debupstream}-0~{revno}
-                %s
-                ''') % branch.bzr_identity
+              # bzr-builder format 145.115 deb-version {debupstream}-0~{revno}
+              %s
+              ''') % branch.bzr_identity
             browser = self.createRecipe(recipe, branch)
             self.assertEqual(
                 get_feedback_messages(browser.contents)[1],
@@ -592,7 +592,7 @@
 
     def test_new_sourcepackage_branch_recipe_with_related_branches(self):
         # The related branches should be rendered correctly on the page.
-        reference_branch= self.factory.makePackageBranch()
+        reference_branch = self.factory.makePackageBranch()
         (branch, ignore, related_package_branch_info) = (
                 self.factory.makeRelatedBranches(reference_branch))
         browser = self.getUserBrowser(
@@ -1039,7 +1039,7 @@
     def test_edit_sourcepackage_branch_recipe_with_related_branches(self):
         # The related branches should be rendered correctly on the page.
         with person_logged_in(self.chef):
-            reference_branch= self.factory.makePackageBranch()
+            reference_branch = self.factory.makePackageBranch()
             recipe = self.factory.makeSourcePackageRecipe(
                     owner=self.chef, branches=[reference_branch])
             (branch, ignore, related_package_branch_info) = (
@@ -1152,10 +1152,11 @@
             sourcepackagerelease=source_package_release, archive=self.ppa,
             distroseries=self.squirrel)
         builder = self.factory.makeBuilder()
-        binary_build = removeSecurityProxy(self.factory.makeBinaryPackageBuild(
-            source_package_release=source_package_release,
-            distroarchseries=self.squirrel.nominatedarchindep,
-            processor=builder.processor))
+        binary_build = removeSecurityProxy(
+            self.factory.makeBinaryPackageBuild(
+                source_package_release=source_package_release,
+                distroarchseries=self.squirrel.nominatedarchindep,
+                processor=builder.processor))
         binary_build.queueBuild()
         binary_build.status = BuildStatus.FULLYBUILT
         binary_build.date_started = datetime(2010, 04, 16, tzinfo=UTC)
@@ -1529,7 +1530,7 @@
 
 
 class TestSourcePackageRecipeBuildView(BrowserTestCase):
-    """Test behaviour of SourcePackageReciptBuildView."""
+    """Test behaviour of SourcePackageRecipeBuildView."""
 
     layer = LaunchpadFunctionalLayer
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-08-31 20:28:59 +0000
@@ -101,7 +101,7 @@
         """
         registrant = self.factory.makePerson()
         return dict(
-            registrant=registrant,
+            registrant = registrant,
             owner = self.factory.makeTeam(owner=registrant),
             distroseries = [self.factory.makeDistroSeries()],
             name = self.factory.getUniqueString(u'recipe-name'),
@@ -304,13 +304,13 @@
         store = Store.of(build)
         store.flush()
         build_job = store.find(SourcePackageRecipeBuildJob,
-                SourcePackageRecipeBuildJob.build_id==build.id).one()
+                SourcePackageRecipeBuildJob.build_id == build.id).one()
         self.assertProvides(build_job, ISourcePackageRecipeBuildJob)
         self.assertTrue(build_job.virtualized)
         job = build_job.job
         self.assertProvides(job, IJob)
         self.assertEquals(job.status, JobStatus.WAITING)
-        build_queue = store.find(BuildQueue, BuildQueue.job==job.id).one()
+        build_queue = store.find(BuildQueue, BuildQueue.job == job.id).one()
         self.assertProvides(build_queue, IBuildQueue)
         self.assertTrue(build_queue.virtualized)
 
@@ -418,6 +418,39 @@
         recipe.requestBuild(archive, recipe.owner, series,
                 PackagePublishingPocket.RELEASE)
 
+    def test_requestBuildPrivatePPAWithArchivePermission(self):
+        """User is not in PPA owner team but has ArchivePermission.
+
+        The case where the user is not in the PPA owner team but is allowed to
+        upload to the PPA via an explicit ArchivePermission takes a different
+        security path than if he were part of the team.
+        """
+
+        # Create a team private PPA.
+        team_owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=team_owner)
+        team_p3a = self.factory.makeArchive(
+            owner=team, displayname='Private PPA', name='p3a',
+            private=True)
+
+        # Create a recipe with the team P3A as the build destination.
+        recipe = self.factory.makeSourcePackageRecipe()
+
+        # Add upload component rights for the non-team person.
+        with person_logged_in(team_owner):
+            team_p3a.newComponentUploader(
+                person=recipe.owner, component_name="main")
+        (distroseries,) = list(recipe.distroseries)
+
+        # Try to request a build.  It should work.
+        with person_logged_in(recipe.owner):
+            build = recipe.requestBuild(
+                team_p3a, recipe.owner, distroseries,
+                PackagePublishingPocket.RELEASE)
+            self.assertEqual(build.archive, team_p3a)
+            self.assertEqual(build.distroseries, distroseries)
+            self.assertEqual(build.requester, recipe.owner)
+
     def test_sourcepackagerecipe_description(self):
         """Ensure that the SourcePackageRecipe has a proper description."""
         description = u'The whoozits and whatzits.'
@@ -464,7 +497,7 @@
         with person_logged_in(owner):
             recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
             self.assertTrue(check_permission('launchpad.View', recipe))
-        removeSecurityProxy(branch).explicitly_private=True
+        removeSecurityProxy(branch).explicitly_private = True
         with person_logged_in(self.factory.makePerson()):
             self.assertFalse(check_permission('launchpad.View', recipe))
         self.assertFalse(check_permission('launchpad.View', recipe))
@@ -606,12 +639,12 @@
         """SourcePackageRecipe.getPendingBuildInfo() is as expected."""
         person = self.factory.makePerson()
         archives = [self.factory.makeArchive(owner=person) for x in range(4)]
-        distroseries= self.factory.makeSourcePackageRecipeDistroseries()
+        distroseries = self.factory.makeSourcePackageRecipeDistroseries()
         recipe = self.factory.makeSourcePackageRecipe()
 
         build_info = []
         for archive in archives:
-            build = recipe.requestBuild(archive, person, distroseries)
+            recipe.requestBuild(archive, person, distroseries)
             build_info.insert(0, {
                 "distroseries": distroseries.displayname,
                 "archive": '%s/%s' %
@@ -1036,7 +1069,7 @@
         """SourcePackageRecipe.[pending_|completed_]builds is as expected."""
         person = self.factory.makePerson()
         archives = [self.factory.makeArchive(owner=person) for x in range(4)]
-        distroseries= self.factory.makeSourcePackageRecipeDistroseries()
+        distroseries = self.factory.makeSourcePackageRecipeDistroseries()
 
         recipe, user, launchpad = self.makeRecipe(person)
         distroseries = ws_object(launchpad, distroseries)
@@ -1056,7 +1089,7 @@
         """SourcePackageRecipe.getPendingBuildInfo() is as expected."""
         person = self.factory.makePerson()
         archives = [self.factory.makeArchive(owner=person) for x in range(4)]
-        distroseries= self.factory.makeSourcePackageRecipeDistroseries()
+        distroseries = self.factory.makeSourcePackageRecipeDistroseries()
 
         recipe, user, launchpad = self.makeRecipe(person)
         ws_distroseries = ws_object(launchpad, distroseries)
@@ -1064,7 +1097,7 @@
         build_info = []
         for archive in archives:
             ws_archive = ws_object(launchpad, archive)
-            build = recipe.requestBuild(
+            recipe.requestBuild(
                 archive=ws_archive, distroseries=ws_distroseries,
                 pocket=PackagePublishingPocket.RELEASE.title)
             build_info.insert(0, {