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

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
1=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
2--- lib/lp/code/browser/sourcepackagerecipe.py 2010-07-19 12:33:15 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-07-21 16:56:44 +0000
4@@ -20,6 +20,7 @@
5 from lazr.lifecycle.event import ObjectModifiedEvent
6 from lazr.lifecycle.snapshot import Snapshot
7 from lazr.restful.interface import use_template
8+from storm.locals import Store
9 from zope.component import getUtility
10 from zope.event import notify
11 from zope.interface import implements, Interface
12@@ -304,6 +305,7 @@
13 self.user, self.user, data['name'], recipe,
14 data['description'], data['distros'],
15 data['daily_build_archive'], data['build_daily'])
16+ Store.of(source_package_recipe).flush()
17 except ForbiddenInstruction:
18 # XXX: bug=592513 We shouldn't be hardcoding "run" here.
19 self.setFieldError(
20
21=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
22--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-19 10:51:39 +0000
23+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-21 16:56:44 +0000
24@@ -10,6 +10,7 @@
25 from datetime import datetime, timedelta
26 from textwrap import dedent
27
28+import transaction
29 from pytz import utc
30 from zope.security.interfaces import Unauthorized
31 from zope.security.proxy import removeSecurityProxy
32@@ -244,6 +245,8 @@
33 # You shouldn't be able to create a duplicate recipe owned by the same
34 # person with the same name.
35 recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)
36+ transaction.commit()
37+ recipe_name = recipe.name
38
39 product = self.factory.makeProduct(
40 name='ratatouille', displayname='Ratatouille')
41@@ -254,7 +257,7 @@
42 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
43 browser.getLink('Create packaging recipe').click()
44
45- browser.getControl(name='field.name').value = recipe.name
46+ browser.getControl(name='field.name').value = recipe_name
47 browser.getControl('Description').value = 'Make some food!'
48 browser.getControl('Secret Squirrel').click()
49 browser.getControl('Create Recipe').click()
50@@ -575,6 +578,8 @@
51 browser = self.getViewBrowser(recipe, '+request-builds')
52 browser.getControl('Woody').click()
53 browser.getControl('Request builds').click()
54+
55+ login(ANONYMOUS)
56 builds = recipe.getBuilds(True)
57 build_distros = [
58 build.distroseries.displayname for build in builds]
59@@ -819,18 +824,20 @@
60 build = self.makeBuild()
61 removeSecurityProxy(build).buildlog = (
62 self.factory.makeLibraryFileAlias())
63+ build_log_url = build.build_log_url
64 browser = self.getViewBrowser(build)
65 link = browser.getLink('buildlog')
66- self.assertEqual(build.build_log_url, link.url)
67+ self.assertEqual(build_log_url, link.url)
68
69 def test_uploadlog(self):
70 """A link to the upload log is shown if available."""
71 build = self.makeBuild()
72 removeSecurityProxy(build).upload_log = (
73 self.factory.makeLibraryFileAlias())
74+ upload_log_url = build.upload_log_url
75 browser = self.getViewBrowser(build)
76 link = browser.getLink('uploadlog')
77- self.assertEqual(build.upload_log_url, link.url)
78+ self.assertEqual(upload_log_url, link.url)
79
80
81 class TestSourcePackageRecipeDeleteView(TestCaseForRecipe):
82
83=== modified file 'lib/lp/code/configure.zcml'
84--- lib/lp/code/configure.zcml 2010-06-16 18:18:32 +0000
85+++ lib/lp/code/configure.zcml 2010-07-21 16:56:44 +0000
86@@ -1013,7 +1013,7 @@
87
88 <class
89 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
90- <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
91+ <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
92 <!-- This is needed for UploadProcessor to run. The permission isn't
93 important; launchpad.Edit isn't actually held by anybody. -->
94 <require permission="launchpad.Edit" set_attributes="buildstate upload_log" />
95@@ -1027,7 +1027,7 @@
96
97 <class
98 class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
99- <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
100+ <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
101 </class>
102
103 <securedutility
104@@ -1048,8 +1048,9 @@
105 <!-- SourcePackageRecipe -->
106 <class
107 class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">
108- <allow
109- interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"/>
110+ <require permission="launchpad.View"
111+ interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipe"
112+ />
113 <require
114 permission="launchpad.Edit"
115 set_attributes="
116
117=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
118--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-07-20 13:35:23 +0000
119+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-07-21 16:56:44 +0000
120@@ -84,6 +84,7 @@
121 recipe = getUtility(ISourcePackageRecipeSource).new(
122 registrant=registrant, owner=owner, distroseries=[distroseries],
123 name=name, description=description, builder_recipe=builder_recipe)
124+ transaction.commit()
125 self.assertEquals(
126 (registrant, owner, set([distroseries]), name),
127 (recipe.registrant, recipe.owner, set(recipe.distroseries),
128@@ -112,6 +113,7 @@
129 # SourcePackageRecipe objects implement ISourcePackageRecipe.
130 recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
131 self.factory.makeRecipe())
132+ transaction.commit()
133 self.assertProvides(recipe, ISourcePackageRecipe)
134
135 def test_base_branch(self):
136@@ -120,6 +122,7 @@
137 builder_recipe = self.factory.makeRecipe(branch)
138 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
139 builder_recipe)
140+ transaction.commit()
141 self.assertEquals(branch, sp_recipe.base_branch)
142
143 def test_branch_links_created(self):
144@@ -129,6 +132,7 @@
145 builder_recipe = self.factory.makeRecipe(branch)
146 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
147 builder_recipe)
148+ transaction.commit()
149 self.assertEquals([branch], list(sp_recipe.getReferencedBranches()))
150
151 def test_multiple_branch_links_created(self):
152@@ -139,6 +143,7 @@
153 builder_recipe = self.factory.makeRecipe(branch1, branch2)
154 sp_recipe = self.makeSourcePackageRecipeFromBuilderRecipe(
155 builder_recipe)
156+ transaction.commit()
157 self.assertEquals(
158 sorted([branch1, branch2]),
159 sorted(sp_recipe.getReferencedBranches()))
160@@ -465,6 +470,7 @@
161 recipe = getUtility(ISourcePackageRecipeSource).new(
162 registrant=registrant, owner=owner, distroseries=[distroseries],
163 name=name, description=description, builder_recipe=builder_recipe)
164+ transaction.commit()
165 return recipe.builder_recipe
166
167 def check_base_recipe_branch(self, branch, url, revspec=None,
168@@ -637,14 +643,19 @@
169 return MINIMAL_RECIPE_TEXT % branch.bzr_identity
170
171 def makeRecipe(self, user=None, owner=None, recipe_text=None):
172+ # rockstar 21 Jul 2010 - This function does more commits than I'd like,
173+ # but it's the result of the fact that the webservice runs in a
174+ # separate thread so doesn't get the database updates without those
175+ # commits.
176 if user is None:
177 user = self.factory.makePerson()
178 if owner is None:
179 owner = user
180- db_distroseries = self.factory.makeDistroSeries()
181+ db_distroseries = self.factory.makeSourcePackageRecipeDistroseries()
182 if recipe_text is None:
183 recipe_text = self.makeRecipeText()
184 db_archive = self.factory.makeArchive(owner=owner, name="recipe-ppa")
185+ transaction.commit()
186 launchpad = launchpadlib_for('test', user,
187 service_root="http://api.launchpad.dev:8085")
188 login(ANONYMOUS)
189
190=== modified file 'lib/lp/registry/model/person.py'
191--- lib/lp/registry/model/person.py 2010-07-19 15:32:21 +0000
192+++ lib/lp/registry/model/person.py 2010-07-21 16:56:44 +0000
193@@ -2272,9 +2272,11 @@
194 """See `IPerson`."""
195 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
196 builder_recipe = RecipeParser(recipe_text).parse()
197- return SourcePackageRecipe.new(
198+ recipe = SourcePackageRecipe.new(
199 registrant, self, name, builder_recipe, description, distroseries,
200 daily_build_archive, build_daily)
201+ Store.of(recipe).flush()
202+ return recipe
203
204 def getRecipe(self, name):
205 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe