Merge lp:~abentley/launchpad/reject-private-recipe into lp:launchpad

[Aaron Bentley]

= Summary =
Fix bug #607908: recipes with private branches should fail earlier

== Proposed fix ==
Raise an exception when creating or updating SourcePackageRecipeData that uses
private branches. Forward the exception to the user as a user error.

== Pre-implementation notes ==
None

== Implementation details ==
I fixed some lint, e.g. whitespace between 'And' and '('.

== Tests ==
bin/test -t test_creation -t setRecipeText -t test_edit_recipe_private_branch test_sourcepackagerecipe

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/sourcepackagerecipedata.py
  lib/lp/code/errors.py
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/testing/factory.py
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

./lib/lp/code/model/sourcepackagerecipedata.py
     154: E202 whitespace before ')'
./lib/lp/code/model/tests/test_sourcepackagerecipe.py
     255: E231 missing whitespace after ','
     279: E231 missing whitespace after ','
     287: E231 missing whitespace after ','
     294: E231 missing whitespace after ','
     302: E231 missing whitespace after ','
     328: E231 missing whitespace after ','
     388: E231 missing whitespace after ','
     685: Line exceeds 78 characters.
./lib/lp/testing/factory.py
     160: 'logout' imported but unused
     846: E231 missing whitespace after ','
    2752: E231 missing whitespace after ','
    2754: E302 expected 2 blank lines, found 1
    2781: E301 expected 1 blank line, found 0
    1052: Line exceeds 78 characters.
    2755: Line exceeds 78 characters.
./lib/lp/code/browser/sourcepackagerecipe.py
     133: E231 missing whitespace after ','
     242: E202 whitespace before ']'
     354: E231 missing whitespace after ','
     386: E303 too many blank lines (3)
./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
     244: E231 missing whitespace after ','

[Robert Collins (lifeless)]

+ Suggested use: provide as kwargs to ISourcePackageRecipeSource.new
+ :param branches: The list of branches to use in the recipe. (If
+ unspecified, a branch will be autogenerated.
+ """

You are missing an end ) there.

review: Approve

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-07-23 03:53:46 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-07-30 19:51:04 +0000
@@ -34,8 +34,8 @@
     LaunchpadView, Link, Navigation, NavigationMenu, stepthrough, structured)
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
-from lp.code.errors import ForbiddenInstruction
-from lp.code.errors import BuildAlreadyPending
+from lp.code.errors import (
+    BuildAlreadyPending, ForbiddenInstruction, PrivateBranchRecipe)
 from lp.code.interfaces.branch import NoSuchBranch
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
@@ -316,6 +316,9 @@
             self.setFieldError(
                 'recipe_text', '%s is not a branch on Launchpad.' % e.name)
             return
+        except PrivateBranchRecipe, e:
+            self.setFieldError('recipe_text', str(e))
+            return
 
         self.next_url = canonical_url(source_package_recipe)
 
@@ -371,9 +374,13 @@
                 # XXX: bug=592513 We shouldn't be hardcoding "run" here.
                 self.setFieldError(
                     'recipe_text',
-                    'The bzr-builder instruction "run" is not permitted here.'
-                    )
-                return
+                    'The bzr-builder instruction "run" is not permitted'
+                    ' here.')
+                return
+            except PrivateBranchRecipe, e:
+                self.setFieldError('recipe_text', str(e))
+                return
+
 
 
         distros = data.pop('distros')

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-07-29 18:33:13 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-07-30 19:51:04 +0000
@@ -28,7 +28,8 @@
 from lp.code.interfaces.sourcepackagerecipe import MINIMAL_RECIPE_TEXT
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.model.processor import ProcessorFamily
-from lp.testing import ANONYMOUS, BrowserTestCase, login, logout
+from lp.testing import (
+    ANONYMOUS, BrowserTestCase, login, logout, person_logged_in)
 from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
 
 
@@ -303,6 +304,18 @@
             get_message_text(browser, 2),
             'There is already a recipe owned by Master Chef with this name.')
 
+    def test_create_recipe_private_branch(self):
+        # If a user tries to create source package recipe with a private
+        # base branch, they should get an error.
+        branch = self.factory.makeAnyBranch(private=True, owner=self.user)
+        with person_logged_in(self.user):
+            bzr_identity = branch.bzr_identity
+        recipe_text = MINIMAL_RECIPE_TEXT % bzr_identity
+        browser = self.createRecipe(recipe_text)
+        self.assertEqual(
+            get_message_text(browser, 2),
+            'Recipe may not refer to private branch: %s' % bzr_identity)
+
 
 class TestSourcePackageRecipeEditView(TestCaseForRecipe):
     """Test the editing behaviour of a source package recipe."""
@@ -475,6 +488,21 @@
         self.assertTextMatchesExpressionIgnoreWhitespace(
             pattern, main_text)
 
+    def test_edit_recipe_private_branch(self):
+        # If a user tries to set source package recipe to use a private
+        # branch, they should get an error.
+        recipe = self.factory.makeSourcePackageRecipe(owner=self.user)
+        branch = self.factory.makeAnyBranch(private=True, owner=self.user)
+        with person_logged_in(self.user):
+            bzr_identity = branch.bzr_identity
+        recipe_text = MINIMAL_RECIPE_TEXT % bzr_identity
+        browser = self.getViewBrowser(recipe, '+edit')
+        browser.getControl('Recipe text').value = recipe_text
+        browser.getControl('Update Recipe').click()
+        self.assertEqual(
+            get_message_text(browser, 1),
+            'Recipe may not refer to private branch: %s' % bzr_identity)
+
 
 class TestSourcePackageRecipeView(TestCaseForRecipe):
 

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2010-07-20 12:35:09 +0000
+++ lib/lp/code/errors.py	2010-07-30 19:51:04 +0000
@@ -16,6 +16,7 @@
     'ClaimReviewFailed',
     'ForbiddenInstruction',
     'InvalidBranchMergeProposal',
+    'PrivateBranchRecipe',
     'ReviewNotPending',
     'TooManyBuilds',
     'TooNewRecipeFormat',
@@ -53,6 +54,16 @@
     webservice_error(400) #Bad request.
 
 
+class PrivateBranchRecipe(Exception):
+
+    def __init__(self, branch):
+        message = (
+            'Recipe may not refer to private branch: %s' %
+            branch.bzr_identity)
+        self.branch = branch
+        Exception.__init__(self, message)
+
+
 class ReviewNotPending(Exception):
     """The requested review is not in a pending state."""
 

=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2010-06-17 20:55:11 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2010-07-30 19:51:04 +0000
@@ -27,7 +27,8 @@
 from canonical.database.enumcol import EnumCol
 from canonical.launchpad.interfaces.lpstorm import IStore
 
-from lp.code.errors import ForbiddenInstruction, TooNewRecipeFormat
+from lp.code.errors import (
+    ForbiddenInstruction, PrivateBranchRecipe, TooNewRecipeFormat)
 from lp.code.model.branch import Branch
 from lp.code.interfaces.branch import NoSuchBranch
 from lp.code.interfaces.branchlookup import IBranchLookup
@@ -147,7 +148,7 @@
                     SourcePackageRecipeData.base_branch == branch),
                 Select(
                     SourcePackageRecipeData.sourcepackage_recipe_id,
-                    And (
+                    And(
                         _SourcePackageRecipeDataInstruction.recipe_data_id ==
                         SourcePackageRecipeData.id,
                         _SourcePackageRecipeDataInstruction.branch == branch)
@@ -203,6 +204,10 @@
                 raise ForbiddenInstruction(str(instruction))
             db_branch = getUtility(IBranchLookup).getByUrl(
                 instruction.recipe_branch.url)
+            if db_branch is None:
+                raise NoSuchBranch(instruction.recipe_branch.url)
+            if db_branch.private:
+                raise PrivateBranchRecipe(db_branch)
             r[instruction.recipe_branch.url] = db_branch
             r.update(self._scanInstructions(instruction.recipe_branch))
         return r
@@ -224,8 +229,6 @@
             line_number += 1
             comment = None
             db_branch = branch_map[instruction.recipe_branch.url]
-            if db_branch is None:
-                raise NoSuchBranch(instruction.recipe_branch.url)
             insn = _SourcePackageRecipeDataInstruction(
                 instruction.recipe_branch.name, type, comment,
                 line_number, db_branch, instruction.recipe_branch.revspec,
@@ -247,6 +250,8 @@
         base_branch = branch_lookup.getByUrl(builder_recipe.url)
         if base_branch is None:
             raise NoSuchBranch(builder_recipe.url)
+        if base_branch.private:
+            raise PrivateBranchRecipe(base_branch)
         if builder_recipe.revspec is not None:
             self.revspec = unicode(builder_recipe.revspec)
         self._recordInstructions(

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-07-28 16:44:00 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-07-30 19:51:04 +0000
@@ -32,8 +32,8 @@
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.errors import (
-    BuildAlreadyPending, ForbiddenInstruction, TooManyBuilds,
-    TooNewRecipeFormat)
+    BuildAlreadyPending, ForbiddenInstruction, PrivateBranchRecipe,
+    TooManyBuilds, TooNewRecipeFormat)
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
 from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -72,25 +72,64 @@
             registrant=registrant, owner=owner, distroseries=[distroseries],
             name=name, description=description, builder_recipe=builder_recipe)
 
+    def makeRecipeComponents(self, branches=()):
+        """Return a dict of values that can be used to make a recipe.
+
+        Suggested use: provide as kwargs to ISourcePackageRecipeSource.new
+        :param branches: The list of branches to use in the recipe.  (If
+            unspecified, a branch will be autogenerated.)
+        """
+        registrant = self.factory.makePerson()
+        return dict(
+            registrant=registrant,
+            owner = self.factory.makeTeam(owner=registrant),
+            distroseries = [self.factory.makeDistroSeries()],
+            name = self.factory.getUniqueString(u'recipe-name'),
+            description = self.factory.getUniqueString(u'recipe-description'),
+            builder_recipe = self.factory.makeRecipe(*branches))
+
     def test_creation(self):
         # The metadata supplied when a SourcePackageRecipe is created is
         # present on the new object.
-        registrant = self.factory.makePerson()
-        owner = self.factory.makeTeam(owner=registrant)
-        distroseries = self.factory.makeDistroSeries()
-        name = self.factory.getUniqueString(u'recipe-name')
-        description = self.factory.getUniqueString(u'recipe-description')
-        builder_recipe = self.factory.makeRecipe()
-        recipe = getUtility(ISourcePackageRecipeSource).new(
-            registrant=registrant, owner=owner, distroseries=[distroseries],
-            name=name, description=description, builder_recipe=builder_recipe)
+        components = self.makeRecipeComponents()
+        recipe = getUtility(ISourcePackageRecipeSource).new(**components)
         transaction.commit()
         self.assertEquals(
-            (registrant, owner, set([distroseries]), name),
+            (components['registrant'], components['owner'],
+             set(components['distroseries']), components['name']),
             (recipe.registrant, recipe.owner, set(recipe.distroseries),
              recipe.name))
         self.assertEqual(True, recipe.is_stale)
 
+    def test_creation_private_base_branch(self):
+        """An exception should be raised if the base branch is private."""
+        owner = self.factory.makePerson()
+        with person_logged_in(owner):
+            branch = self.factory.makeAnyBranch(private=True, owner=owner)
+            components = self.makeRecipeComponents(branches=[branch])
+            recipe_source = getUtility(ISourcePackageRecipeSource)
+            e = self.assertRaises(
+                PrivateBranchRecipe, recipe_source.new, **components)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                branch.bzr_identity, str(e))
+
+    def test_creation_private_referenced_branch(self):
+        """An exception should be raised if a referenced branch is private."""
+        owner = self.factory.makePerson()
+        with person_logged_in(owner):
+            base_branch = self.factory.makeAnyBranch(owner=owner)
+            referenced_branch = self.factory.makeAnyBranch(
+                private=True, owner=owner)
+            branches = [base_branch, referenced_branch]
+            components = self.makeRecipeComponents(branches=branches)
+            recipe_source = getUtility(ISourcePackageRecipeSource)
+            e = self.assertRaises(
+                PrivateBranchRecipe, recipe_source.new, **components)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                referenced_branch.bzr_identity, str(e))
+
     def test_exists(self):
         # Test ISourcePackageRecipeSource.exists
         recipe = self.factory.makeSourcePackageRecipe()
@@ -389,10 +428,11 @@
     def test_view_private(self):
         """Recipes with private branches are restricted."""
         owner = self.factory.makePerson()
-        branch = self.factory.makeAnyBranch(owner=owner, private=True)
+        branch = self.factory.makeAnyBranch(owner=owner)
         with person_logged_in(owner):
             recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
             self.assertTrue(check_permission('launchpad.View', recipe))
+        removeSecurityProxy(branch).private=True
         with person_logged_in(self.factory.makePerson()):
             self.assertFalse(check_permission('launchpad.View', recipe))
         self.assertFalse(check_permission('launchpad.View', recipe))
@@ -703,6 +743,35 @@
         recipe.setRecipeText(recipe_text=recipe_text2)
         self.assertEqual(recipe_text2, recipe.recipe_text)
 
+    def test_setRecipeText_private_base_branch(self):
+        source_package_recipe = self.factory.makeSourcePackageRecipe()
+        with person_logged_in(source_package_recipe.owner):
+            branch = self.factory.makeAnyBranch(
+                private=True, owner=source_package_recipe.owner)
+            recipe_text = self.factory.makeRecipeText(branch)
+            e = self.assertRaises(
+                PrivateBranchRecipe, source_package_recipe.setRecipeText,
+                recipe_text)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                branch.bzr_identity, str(e))
+
+    def test_setRecipeText_private_referenced_branch(self):
+        source_package_recipe = self.factory.makeSourcePackageRecipe()
+        with person_logged_in(source_package_recipe.owner):
+            base_branch = self.factory.makeAnyBranch(
+                owner=source_package_recipe.owner)
+            referenced_branch = self.factory.makeAnyBranch(
+                private=True, owner=source_package_recipe.owner)
+            recipe_text = self.factory.makeRecipeText(
+                base_branch, referenced_branch)
+            e = self.assertRaises(
+                PrivateBranchRecipe, source_package_recipe.setRecipeText,
+                recipe_text)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                referenced_branch.bzr_identity, str(e))
+
     def test_getRecipe(self):
         """Person.getRecipe returns the named recipe."""
         recipe, user = self.makeRecipe()[:-1]

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-07-22 08:15:29 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-07-30 19:51:04 +0000
@@ -131,11 +131,12 @@
     def test_view_private_branch(self):
         """Recipebuilds with private branches are restricted."""
         owner = self.factory.makePerson()
-        branch = self.factory.makeAnyBranch(owner=owner, private=True)
+        branch = self.factory.makeAnyBranch(owner=owner)
         with person_logged_in(owner):
             recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
             build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
             self.assertTrue(check_permission('launchpad.View', build))
+        removeSecurityProxy(branch).private = True
         with person_logged_in(self.factory.makePerson()):
             self.assertFalse(check_permission('launchpad.View', build))
         login(ANONYMOUS)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-30 16:30:12 +0000
+++ lib/lp/testing/factory.py	2010-07-30 19:51:04 +0000
@@ -37,6 +37,7 @@
 from types import InstanceType
 import warnings
 
+from bzrlib.plugins.builder.recipe import BaseRecipeBranch
 import pytz
 
 from twisted.python.util import mergeFunctionMetadata
@@ -2767,7 +2768,8 @@
 # security wrappers for them, as well as for objects created by
 # other Python libraries.
 unwrapped_types = (
-    DSCFile, InstanceType, MergeDirective2, Message, datetime, int, str, unicode)
+    BaseRecipeBranch, DSCFile, InstanceType, MergeDirective2, Message,
+    datetime, int, str, unicode,)
 
 
 def is_security_proxied_or_harmless(obj):