Merge lp:~rockstar/launchpad/recipe-security into lp:launchpad

[Paul Hummer]

This branch fixes bug #593221 - We changed the recipe permission to be launchpad.View. This had adverse affects because of the delegation from SourcePackageRecipe to SourcePackageRecipeData. Gary discovered that we need to commit the transaction before the delegation will work.

As reviewer, please make sure all the places I've opted to do transaction.commit are sane. Ask me why I put them where I put them, etc.

[Jelmer Vernooij (jelmer)]

Sorry - local email fail, so retrying here using the web UI.

The places where you transaction.commit() all make sense to me, right after you create SourcePackageRecipes. They don't all appear to be necessary though (tests still pass after I remove them), presumably because we do commits in other places (that we shouldn't rely on) as well?

Another thought; would it perhaps be possible to use Store.of().flush() rather than transaction.commit()? It seems to work in at least some of the cases.

review: Needs Information (code)

[Paul Hummer (rockstar)]

We've always used transaction.commit instead of Store.flush. transaction.commit apparently has some hooks in it.

I'd be curious to know where you found tests pass that don't need transaction.commit. I'm pretty sure it's only in places where I needed to get the tests to pass.

[Björn Tillenius (bjornt)]

On Wed, Jun 30, 2010 at 04:35:40PM -0000, Paul Hummer wrote:
> We've always used transaction.commit instead of Store.flush.
> transaction.commit apparently has some hooks in it.
>
> I'd be curious to know where you found tests pass that don't need
> transaction.commit. I'm pretty sure it's only in places where I needed
> to get the tests to pass.

I'd like to know more, why you need a transaction.commit() in browser
code. In tests it's ok, but it should be avoided in real code, since
it breaks the 'one transaction per request' model that is used
everywhere else. By doing commits you have the risk of errors happening
after the commit causing the DB to have an inconsistent state.

If the commit would be necessary, it should definitely have a big
comment, explaining why. But it seems odd that we need it.

    vote needsinformation

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Needs Information

[Robert Collins (lifeless)]

Let me second Björn's query: committing in the middle of prod code has serious consequences and we should totally avoid it. flush() is ok.

review: Needs Fixing

[Robert Collins (lifeless)]

Thanks for fixing that.

review: Approve

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-07-19 12:33:15 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-07-21 16:56:44 +0000
@@ -20,6 +20,7 @@
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
 from lazr.restful.interface import use_template
+from storm.locals import Store
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implements, Interface
@@ -304,6 +305,7 @@
                     self.user, self.user, data['name'], recipe,
                     data['description'], data['distros'],
                     data['daily_build_archive'], data['build_daily'])
+            Store.of(source_package_recipe).flush()
         except ForbiddenInstruction:
             # XXX: bug=592513 We shouldn't be hardcoding "run" here.
             self.setFieldError(

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-07-19 10:51:39 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-07-21 16:56:44 +0000
@@ -10,6 +10,7 @@
 from datetime import datetime, timedelta
 from textwrap import dedent
 
+import transaction
 from pytz import utc
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
@@ -244,6 +245,8 @@
         # You shouldn't be able to create a duplicate recipe owned by the same
         # person with the same name.
         recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)
+        transaction.commit()
+        recipe_name = recipe.name
 
         product = self.factory.makeProduct(
             name='ratatouille', displayname='Ratatouille')
@@ -254,7 +257,7 @@
         browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
         browser.getLink('Create packaging recipe').click()
 
-        browser.getControl(name='field.name').value = recipe.name
+        browser.getControl(name='field.name').value = recipe_name
         browser.getControl('Description').value = 'Make some food!'
         browser.getControl('Secret Squirrel').click()
         browser.getControl('Create Recipe').click()
@@ -575,6 +578,8 @@
         browser = self.getViewBrowser(recipe, '+request-builds')
         browser.getControl('Woody').click()
         browser.getControl('Request builds').click()
+
+        login(ANONYMOUS)
         builds = recipe.getBuilds(True)
         build_distros = [
             build.distroseries.displayname for build in builds]
@@ -819,18 +824,20 @@
         build = self.makeBuild()
         removeSecurityProxy(build).buildlog = (
             self.factory.makeLibraryFileAlias())
+        build_log_url = build.build_log_url
         browser = self.getViewBrowser(build)
         link = browser.getLink('buildlog')
-        self.assertEqual(build.build_log_url, link.url)
+        self.assertEqual(build_log_url, link.url)
 
     def test_uploadlog(self):
         """A link to the upload log is shown if available."""
         build = self.makeBuild()
         removeSecurityProxy(build).upload_log = (
             self.factory.makeLibraryFileAlias())
+        upload_log_url = build.upload_log_url
         browser = self.getViewBrowser(build)
         link = browser.getLink('uploadlog')
-        self.assertEqual(build.upload_log_url, link.url)
+        self.assertEqual(upload_log_url, link.url)
 
 
 class TestSourcePackageRecipeDeleteView(TestCaseForRecipe):

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-06-16 18:18:32 +0000
+++ lib/lp/code/configure.zcml	2010-07-21 16:56:44 +0000
@@ -1013,7 +1013,7 @@
 
   <class
      class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
-    <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
+    <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
     <!-- This is needed for UploadProcessor to run. The permission isn't
          important; launchpad.Edit isn't actually held by anybody. -->
     <require permission="launchpad.Edit" set_attributes="buildstate upload_log" />
@@ -1027,7 +1027,7 @@
 
   <class
      class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
-    <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
+    <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
   </class>
 
   <securedutility
@@ -1048,8 +1048,9 @@
   <!-- SourcePackageRecipe -->
   <class
      class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">
-    <allow
-    interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"/>
+    <require permission="launchpad.View"
+      interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"
+      />
     <require
        permission="launchpad.Edit"
        set_attributes="

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-07-20 13:35:23 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-07-21 16:56:44 +0000
@@ -84,6 +84,7 @@
         recipe = getUtility(ISourcePackageRecipeSource).new(
             registrant=registrant, owner=owner, distroseries=[distroseries],
             name=name, description=description, builder_recipe=builder_recipe)
+        transaction.commit()
         self.assertEquals(
             (registrant, owner, set([distroseries]), name),
             (recipe.registrant, recipe.owner, set(recipe.distroseries),
@@ -112,6 +113,7 @@
         # SourcePackageRecipe objects implement ISourcePackageRecipe.
         recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
             self.factory.makeRecipe())
+        transaction.commit()
         self.assertProvides(recipe, ISourcePackageRecipe)
 
     def test_base_branch(self):
@@ -120,6 +122,7 @@
         builder_recipe = self.factory.makeRecipe(branch)
         sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
             builder_recipe)
+        transaction.commit()
         self.assertEquals(branch, sp_recipe.base_branch)
 
     def test_branch_links_created(self):
@@ -129,6 +132,7 @@
         builder_recipe = self.factory.makeRecipe(branch)
         sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
             builder_recipe)
+        transaction.commit()
         self.assertEquals([branch], list(sp_recipe.getReferencedBranches()))
 
     def test_multiple_branch_links_created(self):
@@ -139,6 +143,7 @@
         builder_recipe = self.factory.makeRecipe(branch1, branch2)
         sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
             builder_recipe)
+        transaction.commit()
         self.assertEquals(
             sorted([branch1, branch2]),
             sorted(sp_recipe.getReferencedBranches()))
@@ -465,6 +470,7 @@
         recipe = getUtility(ISourcePackageRecipeSource).new(
             registrant=registrant, owner=owner, distroseries=[distroseries],
             name=name, description=description, builder_recipe=builder_recipe)
+        transaction.commit()
         return recipe.builder_recipe
 
     def check_base_recipe_branch(self, branch, url, revspec=None,
@@ -637,14 +643,19 @@
         return MINIMAL_RECIPE_TEXT % branch.bzr_identity
 
     def makeRecipe(self, user=None, owner=None, recipe_text=None):
+        # rockstar 21 Jul 2010 - This function does more commits than I'd like,
+        # but it's the result of the fact that the webservice runs in a
+        # separate thread so doesn't get the database updates without those
+        # commits.
         if user is None:
             user = self.factory.makePerson()
         if owner is None:
             owner = user
-        db_distroseries = self.factory.makeDistroSeries()
+        db_distroseries = self.factory.makeSourcePackageRecipeDistroseries()
         if recipe_text is None:
             recipe_text = self.makeRecipeText()
         db_archive = self.factory.makeArchive(owner=owner, name="recipe-ppa")
+        transaction.commit()
         launchpad = launchpadlib_for('test', user,
                 service_root="http://api.launchpad.dev:8085")
         login(ANONYMOUS)

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-07-19 15:32:21 +0000
+++ lib/lp/registry/model/person.py	2010-07-21 16:56:44 +0000
@@ -2272,9 +2272,11 @@
         """See `IPerson`."""
         from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
         builder_recipe = RecipeParser(recipe_text).parse()
-        return SourcePackageRecipe.new(
+        recipe = SourcePackageRecipe.new(
             registrant, self, name, builder_recipe, description, distroseries,
             daily_build_archive, build_daily)
+        Store.of(recipe).flush()
+        return recipe
 
     def getRecipe(self, name):
         from lp.code.model.sourcepackagerecipe import SourcePackageRecipe