Launchpad itself

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

  1. recipe-oopses
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: 10997
Proposed branch: lp:~rockstar/launchpad/recipe-oopses
Merge into: lp:launchpad
Diff against target: 312 lines (+111/-46)
7 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+24/-5)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+49/-5)
lib/lp/code/errors.py (+32/-0)
lib/lp/code/interfaces/sourcepackagerecipe.py (+0/-31)
lib/lp/code/model/sourcepackagerecipe.py (+2/-1)
lib/lp/code/model/sourcepackagerecipedata.py (+1/-2)
lib/lp/code/model/tests/test_sourcepackagerecipe.py (+3/-2)
To merge this branch: bzr merge lp:~rockstar/launchpad/recipe-oopses
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+27313@code.launchpad.net

Description of the change

This bug fixes bug #586966 - The run command is not allowed in our use of bzr-builder, and so we should catch that case and notify the user that what they're doing just ain't cool.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

ForbiddenInstruction is being imported from
lp.code.interfaces.sourcepakagerecipe. Can we put that in lp.code.errors
instead?

> except ForbiddenInstruction:
> self.setFieldError(
> 'recipe_text',
> 'The bzr-builder instruction "run" is not permitted here.')

Can we get the instruction out of the forbidden instruction exception? That
makes more sense than hard coding it. This may cause unnecessary confusion if
they have a different forbidden instruction.

Also, you should do the check in the validate method, not in the action.

review: Needs Fixing
Revision history for this message
Tim Penhey (thumper) wrote :

Approved conditional on fixing the formatting on the XXX comments :)

We should have name, date and bug.

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-06-09 15:32:32 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
4@@ -39,6 +39,7 @@
5 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
6 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
7 from lp.buildmaster.interfaces.buildbase import BuildStatus
8+from lp.code.errors import ForbiddenInstruction
9 from lp.code.interfaces.sourcepackagerecipe import (
10 ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
11 from lp.code.interfaces.sourcepackagerecipebuild import (
12@@ -352,9 +353,18 @@
13 def request_action(self, action, data):
14 parser = RecipeParser(data['recipe_text'])
15 recipe = parser.parse()
16- source_package_recipe = getUtility(ISourcePackageRecipeSource).new(
17- self.user, self.user, data['distros'],
18- data['name'], recipe, data['description'])
19+ try:
20+ source_package_recipe = getUtility(
21+ ISourcePackageRecipeSource).new(
22+ self.user, self.user, data['distros'],
23+ data['name'], recipe, data['description'])
24+ except ForbiddenInstruction:
25+ # XXX: bug=592513 We shouldn't be hardcoding "run" here.
26+ self.setFieldError(
27+ 'recipe_text',
28+ 'The bzr-builder instruction "run" is not permitted here.')
29+ return
30+
31 self.next_url = canonical_url(source_package_recipe)
32
33 def validate(self, data):
34@@ -402,8 +412,17 @@
35 parser = RecipeParser(recipe_text)
36 recipe = parser.parse()
37 if self.context.builder_recipe != recipe:
38- self.context.builder_recipe = recipe
39- changed = True
40+ try:
41+ self.context.builder_recipe = recipe
42+ changed = True
43+ except ForbiddenInstruction:
44+ # XXX: bug=592513 We shouldn't be hardcoding "run" here.
45+ self.setFieldError(
46+ 'recipe_text',
47+ 'The bzr-builder instruction "run" is not permitted here.'
48+ )
49+ return
50+
51
52 distros = data.pop('distros')
53 if distros != self.context.distroseries:
54
55=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
56--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-09 15:32:32 +0000
57+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
58@@ -88,7 +88,6 @@
59 name='ratatouille', displayname='Ratatouille')
60 branch = self.factory.makeBranch(
61 owner=self.chef, product=product, name='veggies')
62- self.factory.makeSourcePackage()
63
64 # A new recipe can be created from the branch page.
65 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
66@@ -120,6 +119,31 @@
67 self.assertTextMatchesExpressionIgnoreWhitespace(
68 pattern, main_text)
69
70+ def test_create_recipe_forbidden_instruction(self):
71+ # We don't allow the "run" instruction in our recipes. Make sure this
72+ # is communicated to the user properly.
73+ product = self.factory.makeProduct(
74+ name='ratatouille', displayname='Ratatouille')
75+ branch = self.factory.makeBranch(
76+ owner=self.chef, product=product, name='veggies')
77+
78+ # A new recipe can be created from the branch page.
79+ browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
80+ browser.getLink('Create packaging recipe').click()
81+
82+ browser.getControl(name='field.name').value = 'daily'
83+ browser.getControl('Description').value = 'Make some food!'
84+ browser.getControl('Secret Squirrel').click()
85+
86+ browser.getControl('Recipe text').value = (
87+ browser.getControl('Recipe text').value + 'run cat /etc/passwd')
88+
89+ browser.getControl('Create Recipe').click()
90+
91+ self.assertEqual(
92+ extract_text(find_tags_by_class(browser.contents, 'message')[1]),
93+ 'The bzr-builder instruction "run" is not permitted here.')
94+
95 def test_create_new_recipe_empty_name(self):
96 # Leave off the name and make sure that the widgets validate before
97 # the content validates.
98@@ -127,7 +151,6 @@
99 name='ratatouille', displayname='Ratatouille')
100 branch = self.factory.makeBranch(
101 owner=self.chef, product=product, name='veggies')
102- self.factory.makeSourcePackage()
103
104 # A new recipe can be created from the branch page.
105 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
106@@ -148,7 +171,6 @@
107 name='ratatouille', displayname='Ratatouille')
108 branch = self.factory.makeBranch(
109 owner=self.chef, product=product, name='veggies')
110- self.factory.makeSourcePackage()
111
112 # A new recipe can be created from the branch page.
113 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
114@@ -172,7 +194,6 @@
115 name='ratatouille', displayname='Ratatouille')
116 branch = self.factory.makeBranch(
117 owner=self.chef, product=product, name='veggies')
118- self.factory.makeSourcePackage(sourcepackagename='ratatouille')
119
120 # A new recipe can be created from the branch page.
121 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
122@@ -241,6 +262,29 @@
123 self.assertTextMatchesExpressionIgnoreWhitespace(
124 pattern, main_text)
125
126+ def test_edit_recipe_forbidden_instruction(self):
127+ self.factory.makeDistroSeries(
128+ displayname='Mumbly Midget', name='mumbly',
129+ distribution=self.ppa.distribution)
130+ product = self.factory.makeProduct(
131+ name='ratatouille', displayname='Ratatouille')
132+ veggie_branch = self.factory.makeBranch(
133+ owner=self.chef, product=product, name='veggies')
134+ recipe = self.factory.makeSourcePackageRecipe(
135+ owner=self.chef, registrant=self.chef,
136+ name=u'things', description=u'This is a recipe',
137+ distroseries=self.squirrel, branches=[veggie_branch])
138+
139+ browser = self.getUserBrowser(canonical_url(recipe), user=self.chef)
140+ browser.getLink('Edit recipe').click()
141+ browser.getControl('Recipe text').value = (
142+ browser.getControl('Recipe text').value + 'run cat /etc/passwd')
143+ browser.getControl('Update Recipe').click()
144+
145+ self.assertEqual(
146+ extract_text(find_tags_by_class(browser.contents, 'message')[1]),
147+ 'The bzr-builder instruction "run" is not permitted here.')
148+
149 def test_edit_recipe_already_exists(self):
150 self.factory.makeDistroSeries(
151 displayname='Mumbly Midget', name='mumbly',
152@@ -255,7 +299,7 @@
153 owner=self.chef, registrant=self.chef,
154 name=u'things', description=u'This is a recipe',
155 distroseries=self.squirrel, branches=[veggie_branch])
156- recipe_fings = self.factory.makeSourcePackageRecipe(
157+ self.factory.makeSourcePackageRecipe(
158 owner=self.chef, registrant=self.chef,
159 name=u'fings', description=u'This is a recipe',
160 distroseries=self.squirrel, branches=[veggie_branch])
161
162=== modified file 'lib/lp/code/errors.py'
163--- lib/lp/code/errors.py 2010-05-26 17:16:01 +0000
164+++ lib/lp/code/errors.py 2010-06-11 19:07:30 +0000
165@@ -12,8 +12,11 @@
166 'CodeImportAlreadyRunning',
167 'CodeImportNotInReviewedState',
168 'ClaimReviewFailed',
169+ 'ForbiddenInstruction',
170 'InvalidBranchMergeProposal',
171 'ReviewNotPending',
172+ 'TooManyBuilds',
173+ 'TooNewRecipeFormat',
174 'UnknownBranchTypeError',
175 'UserHasExistingReview',
176 'UserNotBranchReviewer',
177@@ -91,3 +94,32 @@
178 """Raised when the user requests an import that is already running."""
179
180 webservice_error(400)
181+
182+
183+class ForbiddenInstruction(Exception):
184+ """A forbidden instruction was found in the recipe."""
185+
186+ def __init__(self, instruction_name):
187+ super(ForbiddenInstruction, self).__init__()
188+ self.instruction_name = instruction_name
189+
190+
191+class TooNewRecipeFormat(Exception):
192+ """The format of the recipe supplied was too new."""
193+
194+ def __init__(self, supplied_format, newest_supported):
195+ super(TooNewRecipeFormat, self).__init__()
196+ self.supplied_format = supplied_format
197+ self.newest_supported = newest_supported
198+
199+
200+class TooManyBuilds(Exception):
201+ """A build was requested that exceeded the quota."""
202+
203+ def __init__(self, recipe, distroseries):
204+ self.recipe = recipe
205+ self.distroseries = distroseries
206+ msg = (
207+ 'You have exceeded your quota for recipe %s for distroseries %s'
208+ % (self.recipe, distroseries))
209+ Exception.__init__(self, msg)
210
211=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
212--- lib/lp/code/interfaces/sourcepackagerecipe.py 2010-06-04 16:14:54 +0000
213+++ lib/lp/code/interfaces/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
214@@ -10,13 +10,10 @@
215
216
217 __all__ = [
218- 'ForbiddenInstruction',
219 'ISourcePackageRecipe',
220 'ISourcePackageRecipeData',
221 'ISourcePackageRecipeSource',
222 'MINIMAL_RECIPE_TEXT',
223- 'TooManyBuilds',
224- 'TooNewRecipeFormat',
225 ]
226
227
228@@ -47,34 +44,6 @@
229 %s
230 ''')
231
232-class ForbiddenInstruction(Exception):
233- """A forbidden instruction was found in the recipe."""
234-
235- def __init__(self, instruction_name):
236- super(ForbiddenInstruction, self).__init__()
237- self.instruction_name = instruction_name
238-
239-
240-class TooManyBuilds(Exception):
241- """A build was requested that exceeded the quota."""
242-
243- def __init__(self, recipe, distroseries):
244- self.recipe = recipe
245- self.distroseries = distroseries
246- msg = (
247- 'You have exceeded your quota for recipe %s for distroseries %s'
248- % (self.recipe, distroseries))
249- Exception.__init__(self, msg)
250-
251-
252-class TooNewRecipeFormat(Exception):
253- """The format of the recipe supplied was too new."""
254-
255- def __init__(self, supplied_format, newest_supported):
256- super(TooNewRecipeFormat, self).__init__()
257- self.supplied_format = supplied_format
258- self.newest_supported = newest_supported
259-
260
261 class ISourcePackageRecipeData(Interface):
262 """A recipe as database data, not text."""
263
264=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
265--- lib/lp/code/model/sourcepackagerecipe.py 2010-06-10 09:55:32 +0000
266+++ lib/lp/code/model/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
267@@ -23,9 +23,10 @@
268 from canonical.database.datetimecol import UtcDateTimeCol
269 from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
270
271+from lp.code.errors import TooManyBuilds
272 from lp.code.interfaces.sourcepackagerecipe import (
273 ISourcePackageRecipe, ISourcePackageRecipeSource,
274- ISourcePackageRecipeData, TooManyBuilds)
275+ ISourcePackageRecipeData)
276 from lp.code.interfaces.sourcepackagerecipebuild import (
277 ISourcePackageRecipeBuildSource)
278 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
279
280=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
281--- lib/lp/code/model/sourcepackagerecipedata.py 2010-04-19 05:04:00 +0000
282+++ lib/lp/code/model/sourcepackagerecipedata.py 2010-06-11 19:07:30 +0000
283@@ -27,11 +27,10 @@
284 from canonical.database.enumcol import EnumCol
285 from canonical.launchpad.interfaces.lpstorm import IStore
286
287+from lp.code.errors import ForbiddenInstruction, TooNewRecipeFormat
288 from lp.code.model.branch import Branch
289 from lp.code.interfaces.branch import NoSuchBranch
290 from lp.code.interfaces.branchlookup import IBranchLookup
291-from lp.code.interfaces.sourcepackagerecipe import (
292- ForbiddenInstruction, TooNewRecipeFormat)
293
294
295 class InstructionType(DBEnumeratedType):
296
297=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
298--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-06-10 09:55:32 +0000
299+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
300@@ -29,9 +29,10 @@
301 InvalidPocketForPPA)
302 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
303 from lp.buildmaster.model.buildqueue import BuildQueue
304+from lp.code.errors import (
305+ ForbiddenInstruction, TooManyBuilds, TooNewRecipeFormat)
306 from lp.code.interfaces.sourcepackagerecipe import (
307- ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource,
308- TooManyBuilds, TooNewRecipeFormat, MINIMAL_RECIPE_TEXT)
309+ ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
310 from lp.code.interfaces.sourcepackagerecipebuild import (
311 ISourcePackageRecipeBuild, ISourcePackageRecipeBuildJob)
312 from lp.code.model.sourcepackagerecipebuild import (