Launchpad itself

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

  1. recipe-security
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11194
Proposed branch: lp:~rockstar/launchpad/recipe-security
Merge into: lp:launchpad
Diff against target: 205 lines (+32/-9)
5 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+2/-0)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+10/-3)
lib/lp/code/configure.zcml (+5/-4)
lib/lp/code/model/tests/test_sourcepackagerecipe.py (+12/-1)
lib/lp/registry/model/person.py (+3/-1)
To merge this branch: bzr merge lp:~rockstar/launchpad/recipe-security
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Björn Tillenius (community) Needs Information
Jelmer Vernooij (community) code Needs Information
Review via email: mp+28410@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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)
Revision history for this message
Paul Hummer (rockstar) 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.

Revision history for this message
Björn Tillenius (bjornt) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

Thanks for fixing that.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
20from lazr.lifecycle.event import ObjectModifiedEvent20from lazr.lifecycle.event import ObjectModifiedEvent
21from lazr.lifecycle.snapshot import Snapshot21from lazr.lifecycle.snapshot import Snapshot
22from lazr.restful.interface import use_template22from lazr.restful.interface import use_template
23from storm.locals import Store
23from zope.component import getUtility24from zope.component import getUtility
24from zope.event import notify25from zope.event import notify
25from zope.interface import implements, Interface26from zope.interface import implements, Interface
@@ -304,6 +305,7 @@
304 self.user, self.user, data['name'], recipe,305 self.user, self.user, data['name'], recipe,
305 data['description'], data['distros'],306 data['description'], data['distros'],
306 data['daily_build_archive'], data['build_daily'])307 data['daily_build_archive'], data['build_daily'])
308 Store.of(source_package_recipe).flush()
307 except ForbiddenInstruction:309 except ForbiddenInstruction:
308 # XXX: bug=592513 We shouldn't be hardcoding "run" here.310 # XXX: bug=592513 We shouldn't be hardcoding "run" here.
309 self.setFieldError(311 self.setFieldError(
310312
=== 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 @@
10from datetime import datetime, timedelta10from datetime import datetime, timedelta
11from textwrap import dedent11from textwrap import dedent
1212
13import transaction
13from pytz import utc14from pytz import utc
14from zope.security.interfaces import Unauthorized15from zope.security.interfaces import Unauthorized
15from zope.security.proxy import removeSecurityProxy16from zope.security.proxy import removeSecurityProxy
@@ -244,6 +245,8 @@
244 # You shouldn't be able to create a duplicate recipe owned by the same245 # You shouldn't be able to create a duplicate recipe owned by the same
245 # person with the same name.246 # person with the same name.
246 recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)247 recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)
248 transaction.commit()
249 recipe_name = recipe.name
247250
248 product = self.factory.makeProduct(251 product = self.factory.makeProduct(
249 name='ratatouille', displayname='Ratatouille')252 name='ratatouille', displayname='Ratatouille')
@@ -254,7 +257,7 @@
254 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)257 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
255 browser.getLink('Create packaging recipe').click()258 browser.getLink('Create packaging recipe').click()
256259
257 browser.getControl(name='field.name').value = recipe.name260 browser.getControl(name='field.name').value = recipe_name
258 browser.getControl('Description').value = 'Make some food!'261 browser.getControl('Description').value = 'Make some food!'
259 browser.getControl('Secret Squirrel').click()262 browser.getControl('Secret Squirrel').click()
260 browser.getControl('Create Recipe').click()263 browser.getControl('Create Recipe').click()
@@ -575,6 +578,8 @@
575 browser = self.getViewBrowser(recipe, '+request-builds')578 browser = self.getViewBrowser(recipe, '+request-builds')
576 browser.getControl('Woody').click()579 browser.getControl('Woody').click()
577 browser.getControl('Request builds').click()580 browser.getControl('Request builds').click()
581
582 login(ANONYMOUS)
578 builds = recipe.getBuilds(True)583 builds = recipe.getBuilds(True)
579 build_distros = [584 build_distros = [
580 build.distroseries.displayname for build in builds]585 build.distroseries.displayname for build in builds]
@@ -819,18 +824,20 @@
819 build = self.makeBuild()824 build = self.makeBuild()
820 removeSecurityProxy(build).buildlog = (825 removeSecurityProxy(build).buildlog = (
821 self.factory.makeLibraryFileAlias())826 self.factory.makeLibraryFileAlias())
827 build_log_url = build.build_log_url
822 browser = self.getViewBrowser(build)828 browser = self.getViewBrowser(build)
823 link = browser.getLink('buildlog')829 link = browser.getLink('buildlog')
824 self.assertEqual(build.build_log_url, link.url)830 self.assertEqual(build_log_url, link.url)
825831
826 def test_uploadlog(self):832 def test_uploadlog(self):
827 """A link to the upload log is shown if available."""833 """A link to the upload log is shown if available."""
828 build = self.makeBuild()834 build = self.makeBuild()
829 removeSecurityProxy(build).upload_log = (835 removeSecurityProxy(build).upload_log = (
830 self.factory.makeLibraryFileAlias())836 self.factory.makeLibraryFileAlias())
837 upload_log_url = build.upload_log_url
831 browser = self.getViewBrowser(build)838 browser = self.getViewBrowser(build)
832 link = browser.getLink('uploadlog')839 link = browser.getLink('uploadlog')
833 self.assertEqual(build.upload_log_url, link.url)840 self.assertEqual(upload_log_url, link.url)
834841
835842
836class TestSourcePackageRecipeDeleteView(TestCaseForRecipe):843class TestSourcePackageRecipeDeleteView(TestCaseForRecipe):
837844
=== 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 @@
10131013
1014 <class1014 <class
1015 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">1015 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
1016 <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>1016 <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
1017 <!-- This is needed for UploadProcessor to run. The permission isn't1017 <!-- This is needed for UploadProcessor to run. The permission isn't
1018 important; launchpad.Edit isn't actually held by anybody. -->1018 important; launchpad.Edit isn't actually held by anybody. -->
1019 <require permission="launchpad.Edit" set_attributes="buildstate upload_log" />1019 <require permission="launchpad.Edit" set_attributes="buildstate upload_log" />
@@ -1027,7 +1027,7 @@
10271027
1028 <class1028 <class
1029 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">1029 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
1030 <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>1030 <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
1031 </class>1031 </class>
10321032
1033 <securedutility1033 <securedutility
@@ -1048,8 +1048,9 @@
1048 <!-- SourcePackageRecipe -->1048 <!-- SourcePackageRecipe -->
1049 <class1049 <class
1050 class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">1050 class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">
1051 <allow1051 <require permission="launchpad.View"
1052 interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"/>1052 interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"
1053 />
1053 <require1054 <require
1054 permission="launchpad.Edit"1055 permission="launchpad.Edit"
1055 set_attributes="1056 set_attributes="
10561057
=== 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 @@
84 recipe = getUtility(ISourcePackageRecipeSource).new(84 recipe = getUtility(ISourcePackageRecipeSource).new(
85 registrant=registrant, owner=owner, distroseries=[distroseries],85 registrant=registrant, owner=owner, distroseries=[distroseries],
86 name=name, description=description, builder_recipe=builder_recipe)86 name=name, description=description, builder_recipe=builder_recipe)
87 transaction.commit()
87 self.assertEquals(88 self.assertEquals(
88 (registrant, owner, set([distroseries]), name),89 (registrant, owner, set([distroseries]), name),
89 (recipe.registrant, recipe.owner, set(recipe.distroseries),90 (recipe.registrant, recipe.owner, set(recipe.distroseries),
@@ -112,6 +113,7 @@
112 # SourcePackageRecipe objects implement ISourcePackageRecipe.113 # SourcePackageRecipe objects implement ISourcePackageRecipe.
113 recipe = self.makeSourcePackageRecipeFromBuilderRecipe(114 recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
114 self.factory.makeRecipe())115 self.factory.makeRecipe())
116 transaction.commit()
115 self.assertProvides(recipe, ISourcePackageRecipe)117 self.assertProvides(recipe, ISourcePackageRecipe)
116118
117 def test_base_branch(self):119 def test_base_branch(self):
@@ -120,6 +122,7 @@
120 builder_recipe = self.factory.makeRecipe(branch)122 builder_recipe = self.factory.makeRecipe(branch)
121 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(123 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
122 builder_recipe)124 builder_recipe)
125 transaction.commit()
123 self.assertEquals(branch, sp_recipe.base_branch)126 self.assertEquals(branch, sp_recipe.base_branch)
124127
125 def test_branch_links_created(self):128 def test_branch_links_created(self):
@@ -129,6 +132,7 @@
129 builder_recipe = self.factory.makeRecipe(branch)132 builder_recipe = self.factory.makeRecipe(branch)
130 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(133 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
131 builder_recipe)134 builder_recipe)
135 transaction.commit()
132 self.assertEquals([branch], list(sp_recipe.getReferencedBranches()))136 self.assertEquals([branch], list(sp_recipe.getReferencedBranches()))
133137
134 def test_multiple_branch_links_created(self):138 def test_multiple_branch_links_created(self):
@@ -139,6 +143,7 @@
139 builder_recipe = self.factory.makeRecipe(branch1, branch2)143 builder_recipe = self.factory.makeRecipe(branch1, branch2)
140 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(144 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
141 builder_recipe)145 builder_recipe)
146 transaction.commit()
142 self.assertEquals(147 self.assertEquals(
143 sorted([branch1, branch2]),148 sorted([branch1, branch2]),
144 sorted(sp_recipe.getReferencedBranches()))149 sorted(sp_recipe.getReferencedBranches()))
@@ -465,6 +470,7 @@
465 recipe = getUtility(ISourcePackageRecipeSource).new(470 recipe = getUtility(ISourcePackageRecipeSource).new(
466 registrant=registrant, owner=owner, distroseries=[distroseries],471 registrant=registrant, owner=owner, distroseries=[distroseries],
467 name=name, description=description, builder_recipe=builder_recipe)472 name=name, description=description, builder_recipe=builder_recipe)
473 transaction.commit()
468 return recipe.builder_recipe474 return recipe.builder_recipe
469475
470 def check_base_recipe_branch(self, branch, url, revspec=None,476 def check_base_recipe_branch(self, branch, url, revspec=None,
@@ -637,14 +643,19 @@
637 return MINIMAL_RECIPE_TEXT % branch.bzr_identity643 return MINIMAL_RECIPE_TEXT % branch.bzr_identity
638644
639 def makeRecipe(self, user=None, owner=None, recipe_text=None):645 def makeRecipe(self, user=None, owner=None, recipe_text=None):
646 # rockstar 21 Jul 2010 - This function does more commits than I'd like,
647 # but it's the result of the fact that the webservice runs in a
648 # separate thread so doesn't get the database updates without those
649 # commits.
640 if user is None:650 if user is None:
641 user = self.factory.makePerson()651 user = self.factory.makePerson()
642 if owner is None:652 if owner is None:
643 owner = user653 owner = user
644 db_distroseries = self.factory.makeDistroSeries()654 db_distroseries = self.factory.makeSourcePackageRecipeDistroseries()
645 if recipe_text is None:655 if recipe_text is None:
646 recipe_text = self.makeRecipeText()656 recipe_text = self.makeRecipeText()
647 db_archive = self.factory.makeArchive(owner=owner, name="recipe-ppa")657 db_archive = self.factory.makeArchive(owner=owner, name="recipe-ppa")
658 transaction.commit()
648 launchpad = launchpadlib_for('test', user,659 launchpad = launchpadlib_for('test', user,
649 service_root="http://api.launchpad.dev:8085")660 service_root="http://api.launchpad.dev:8085")
650 login(ANONYMOUS)661 login(ANONYMOUS)
651662
=== 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 @@
2272 """See `IPerson`."""2272 """See `IPerson`."""
2273 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe2273 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
2274 builder_recipe = RecipeParser(recipe_text).parse()2274 builder_recipe = RecipeParser(recipe_text).parse()
2275 return SourcePackageRecipe.new(2275 recipe = SourcePackageRecipe.new(
2276 registrant, self, name, builder_recipe, description, distroseries,2276 registrant, self, name, builder_recipe, description, distroseries,
2277 daily_build_archive, build_daily)2277 daily_build_archive, build_daily)
2278 Store.of(recipe).flush()
2279 return recipe
22782280
2279 def getRecipe(self, name):2281 def getRecipe(self, name):
2280 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe2282 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe