Launchpad itself

Merge lp:~adeuring/launchpad/security-guarded-test-object-factory-1 into lp:launchpad

  1. security-guarded-test-object-factory-1
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 11208
Proposed branch: lp:~adeuring/launchpad/security-guarded-test-object-factory-1
Merge into: lp:launchpad
Diff against target: 852 lines (+251/-48)
20 files modified
lib/lp/code/browser/tests/test_branchlisting.py (+4/-2)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+12/-5)
lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py (+4/-1)
lib/lp/code/model/tests/test_branch.py (+4/-1)
lib/lp/code/model/tests/test_linkedbranch.py (+7/-2)
lib/lp/code/model/tests/test_sourcepackagerecipebuild.py (+5/-2)
lib/lp/registry/browser/tests/distroseries-views.txt (+1/-1)
lib/lp/registry/browser/tests/milestone-views.txt (+5/-1)
lib/lp/registry/browser/tests/productseries-views.txt (+6/-2)
lib/lp/registry/stories/webservice/xx-project-registry.txt (+4/-2)
lib/lp/registry/tests/test_distroseries.py (+15/-6)
lib/lp/registry/tests/test_sourcepackage.py (+9/-2)
lib/lp/soyuz/browser/tests/test_distrosourcepackagerelease.py (+7/-1)
lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py (+11/-5)
lib/lp/soyuz/tests/test_publishing.py (+9/-3)
lib/lp/testing/factory.py (+82/-7)
lib/lp/testing/tests/test_factory.py (+50/-0)
lib/lp/translations/browser/tests/test_breadcrumbs.py (+3/-1)
lib/lp/translations/doc/translations-export-to-branch.txt (+5/-1)
lib/lp/translations/stories/buildfarm/xx-build-summary.txt (+8/-3)
To merge this branch: bzr merge lp:~adeuring/launchpad/security-guarded-test-object-factory-1
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+30497@code.launchpad.net

Description of the change

While working together with Brian on another branch during the LP epic last week, I was bitten by the fact that we have still a number of methods in LaunchpadObjectFactory which return unproxied objects. While we fixed this problem for the one or two methods we needed in that branch, a number of other methods still remained that return unproxied objects.

Some time ago, Gavin noticed the same problem, IIRC for makePerson(); he also filed a bug that other methods still returned unproxied objects, and Salgado finally fixed that bug.

However, new methods obviously returning unproxied objects creeped into LaunchpadObjectFactory in the meantime, thus reintroducing the old problem. So I thought that it might be reasonable to ensure that all objects returned by LPObjectFactory methods are either of a simple Python type or are security proxied.

The implementation is quite simple: I renamed LaunchpadObjectFactory to _LaunchpadObjectFactory and wrote a new class LaunchpadObjectFactory with basically one method, __getattr__(). This method returns a the "real" factory method, wrapped in a function guarded_method() which ensures that the object returned by the _LaunchpadObjectFactory.makeWhatever() matches the conditions described above.

We have at present ca 20 methods which return, at least sometimes, unproxied objects. Fixing all these methods in one branch would result in a huge diff, so I fixed only two methods in this branch, makeDistroRelease() and makeProductSeries().

My plan is to let guarded_method() raise an exception if it detects an unproxied object, once all existing factory methods return properly proxied objects. For now, it just print a warning to stderr.

The largest part of the diff is test fixes. I took the simple route: Just to remove the security proxy so that the affected tests pass again. I did not bother to check if this is reasonable in a given test -- doing so would simply have needed far too much time. Instead, I added a function remove_security_proxy_and_shout_at_engineer() which just calls removeSecurityProxy() and print a warning to stderr. This will make the tests quite noisy (as do the warnings issued guarded_method() about unproxied objects)...

Pre-/mid-imp discussions with Jeroen and Gary.

affected tests: Many, see the diff.

I also removed some lint in the files I touched, but some messages remain:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/tests/test_branchlisting.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py
  lib/lp/code/model/tests/test_branch.py
  lib/lp/code/model/tests/test_linkedbranch.py
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/code/model/tests/test_sourcepackagerecipebuild.py
  lib/lp/registry/browser/tests/distroseries-views.txt
  lib/lp/registry/browser/tests/milestone-views.txt
  lib/lp/registry/browser/tests/productseries-views.txt
  lib/lp/registry/stories/webservice/xx-project-registry.txt
  lib/lp/registry/tests/test_distroseries.py
  lib/lp/registry/tests/test_sourcepackage.py
  lib/lp/soyuz/browser/tests/test_distrosourcepackagerelease.py
  lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py
  lib/lp/soyuz/tests/test_publishing.py
  lib/lp/testing/factory.py
  lib/lp/translations/browser/tests/test_breadcrumbs.py
  lib/lp/translations/doc/translations-export-to-branch.txt
  lib/lp/translations/stories/buildfarm/xx-build-summary.txt

./lib/lp/registry/browser/tests/productseries-views.txt
      83: want exceeds 78 characters.
./lib/lp/registry/stories/webservice/xx-project-registry.txt
     570: source exceeds 78 characters.
    1254: 'IRepresentationCache' imported but unused
./lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py
     119: E302 expected 2 blank lines, found 1
./lib/lp/translations/browser/tests/test_breadcrumbs.py
      27: E501 line too long (81 characters)
      37: E501 line too long (86 characters)
      45: E501 line too long (81 characters)
      56: E501 line too long (86 characters)
      64: E501 line too long (81 characters)
      72: E501 line too long (82 characters)
      81: E501 line too long (82 characters)
      88: E501 line too long (81 characters)
     138: E301 expected 1 blank line, found 0
     152: E501 line too long (85 characters)
      27: Line exceeds 78 characters.
      37: Line exceeds 78 characters.
      45: Line exceeds 78 characters.
      56: Line exceeds 78 characters.
      64: Line exceeds 78 characters.
      72: Line exceeds 78 characters.
      81: Line exceeds 78 characters.
      88: Line exceeds 78 characters.
     116: Line exceeds 78 characters.
     133: Line exceeds 78 characters.
     152: Line exceeds 78 characters.
./lib/lp/translations/doc/translations-export-to-branch.txt
       0: narrative uses a moin header.
      15: want exceeds 78 characters.
     273: narrative uses a moin header.
     301: narrative uses a moin header.
     242: Could not compile:
             finally:
./lib/lp/translations/stories/buildfarm/xx-build-summary.txt
       0: narrative uses a moin header.
       5: narrative uses a moin header.
      64: narrative uses a moin header.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Thanks for doing this. We discussed this over IRL chat and came up with many little improvements: chiefly renaming _LaunchpadObjectFactory to BareLaunchpadObjectFactory, moving some code out of a nested function, fixing some corner cases, and adding a test for the proxy-checking function since it turned out to be a lot less trivial than originally thought.

This is often the case when engineers say a change will be trivial.

Jeroen

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
2--- lib/lp/code/browser/tests/test_branchlisting.py 2010-05-28 09:54:45 +0000
3+++ lib/lp/code/browser/tests/test_branchlisting.py 2010-07-22 13:01:10 +0000
4@@ -32,12 +32,14 @@
5 from lp.testing import (
6 BrowserTestCase, TestCase, TestCaseWithFactory, login_person,
7 person_logged_in, time_counter)
8+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
9 from lp.testing.views import create_initialized_view
10 from canonical.launchpad.testing.pages import extract_text, find_tag_by_id
11 from canonical.launchpad.webapp import canonical_url
12 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
13 from canonical.testing.layers import DatabaseFunctionalLayer
14
15+
16 class TestListingToSortOrder(TestCase):
17 """Tests for the BranchSet._listingSortToOrderBy static method.
18
19@@ -60,6 +62,7 @@
20
21 def assertSortsEqual(self, sort_one, sort_two):
22 """Assert that one list of sort specs is equal to another."""
23+
24 def sort_data(sort):
25 return sort.suffix, sort.expr
26 self.assertEqual(map(sort_data, sort_one), map(sort_data, sort_two))
27@@ -352,7 +355,7 @@
28 # series on the main site, not the code site.
29 branch = self.factory.makeProductBranch()
30 series = self.factory.makeProductSeries(product=branch.product)
31- series.branch = branch
32+ remove_security_proxy_and_shout_at_engineer(series).branch = branch
33 browser = self.getUserBrowser(
34 canonical_url(branch.product, rootsite='code'))
35 link = browser.getLink(re.compile('^' + series.name + '$'))
36@@ -402,4 +405,3 @@
37
38 def test_suite():
39 return unittest.TestLoader().loadTestsFromName(__name__)
40-
41
42=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
43--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-21 21:16:54 +0000
44+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-22 13:01:10 +0000
45@@ -29,6 +29,7 @@
46 from lp.registry.interfaces.pocket import PackagePublishingPocket
47 from lp.soyuz.model.processor import ProcessorFamily
48 from lp.testing import ANONYMOUS, BrowserTestCase, login, logout
49+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
50
51
52 class TestCaseForRecipe(BrowserTestCase):
53@@ -45,7 +46,9 @@
54 self.squirrel = self.factory.makeDistroSeries(
55 displayname='Secret Squirrel', name='secret', version='100.04',
56 distribution=self.ppa.distribution)
57- self.squirrel.nominatedarchindep = self.squirrel.newArch(
58+ naked_squirrel = remove_security_proxy_and_shout_at_engineer(
59+ self.squirrel)
60+ naked_squirrel.nominatedarchindep = self.squirrel.newArch(
61 'i386', ProcessorFamily.get(1), False, self.chef,
62 supports_virtualized=True)
63
64@@ -494,7 +497,7 @@
65 def makeBuildJob(self, recipe):
66 """Return a build associated with a buildjob."""
67 build = self.factory.makeSourcePackageRecipeBuild(
68- recipe=recipe, distroseries=self.squirrel, archive=self.ppa )
69+ recipe=recipe, distroseries=self.squirrel, archive=self.ppa)
70 self.factory.makeSourcePackageRecipeBuildJob(recipe_build=build)
71 return build
72
73@@ -527,6 +530,7 @@
74 self.assertEqual(
75 set([build1, build2, build3, build4, build5, build6]),
76 set(view.builds))
77+
78 def set_day(build, day):
79 removeSecurityProxy(build).datebuilt = datetime(
80 2010, 03, day, tzinfo=utc)
81@@ -569,7 +573,8 @@
82 woody = self.factory.makeDistroSeries(
83 name='woody', displayname='Woody',
84 distribution=self.ppa.distribution)
85- woody.nominatedarchindep = woody.newArch(
86+ naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
87+ naked_woody.nominatedarchindep = woody.newArch(
88 'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
89 supports_virtualized=True)
90
91@@ -603,7 +608,8 @@
92 woody = self.factory.makeDistroSeries(
93 name='woody', displayname='Woody',
94 distribution=self.ppa.distribution)
95- woody.nominatedarchindep = woody.newArch(
96+ naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
97+ naked_woody.nominatedarchindep = woody.newArch(
98 'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
99 supports_virtualized=True)
100
101@@ -624,7 +630,8 @@
102 woody = self.factory.makeDistroSeries(
103 name='woody', displayname='Woody',
104 distribution=self.ppa.distribution)
105- woody.nominatedarchindep = woody.newArch(
106+ naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
107+ naked_woody.nominatedarchindep = woody.newArch(
108 'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
109 supports_virtualized=True)
110
111
112=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py'
113--- lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py 2010-07-19 09:22:06 +0000
114+++ lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py 2010-07-22 13:01:10 +0000
115@@ -18,6 +18,7 @@
116 from lp.buildmaster.interfaces.buildbase import BuildStatus
117 from lp.soyuz.model.processor import ProcessorFamily
118 from lp.testing import ANONYMOUS, BrowserTestCase, login, logout
119+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
120
121
122 class TestSourcePackageRecipeBuild(BrowserTestCase):
123@@ -36,7 +37,9 @@
124 self.squirrel = self.factory.makeDistroSeries(
125 displayname='Secret Squirrel', name='secret', version='100.04',
126 distribution=self.ppa.distribution)
127- self.squirrel.nominatedarchindep = self.squirrel.newArch(
128+ naked_squirrel = remove_security_proxy_and_shout_at_engineer(
129+ self.squirrel)
130+ naked_squirrel.nominatedarchindep = self.squirrel.newArch(
131 'i386', ProcessorFamily.get(1), False, self.chef,
132 supports_virtualized=True)
133
134
135=== modified file 'lib/lp/code/model/tests/test_branch.py'
136--- lib/lp/code/model/tests/test_branch.py 2010-07-17 23:15:30 +0000
137+++ lib/lp/code/model/tests/test_branch.py 2010-07-22 13:01:10 +0000
138@@ -537,7 +537,7 @@
139 jobs = list(getUtility(IBranchUpgradeJobSource).iterReady())
140 self.assertEqual(
141 jobs,
142- [job,])
143+ [job, ])
144
145 def test_requestUpgrade_no_upgrade_needed(self):
146 # If a branch doesn't need to be upgraded, requestUpgrade raises an
147@@ -635,6 +635,7 @@
148 branch = self.factory.makeProductBranch(
149 product=fooix, owner=eric, name='trunk')
150 linked_branch = ICanHasLinkedBranch(future)
151+ login_person(fooix.owner)
152 linked_branch.setBranch(branch)
153 self.assertEqual(
154 [linked_branch],
155@@ -827,6 +828,7 @@
156 product = branch.product
157 series = self.factory.makeProductSeries(product=product)
158 linked_branch = ICanHasLinkedBranch(series)
159+ login_person(series.owner)
160 linked_branch.setBranch(branch)
161 self.assertBzrIdentity(branch, linked_branch.bzr_path)
162
163@@ -852,6 +854,7 @@
164 removeSecurityProxy(branch.product))
165 series_link = ICanHasLinkedBranch(series)
166 product_link.setBranch(branch)
167+ login_person(series.owner)
168 series_link.setBranch(branch)
169 self.assertBzrIdentity(branch, product_link.bzr_path)
170
171
172=== modified file 'lib/lp/code/model/tests/test_linkedbranch.py'
173--- lib/lp/code/model/tests/test_linkedbranch.py 2010-04-16 15:06:55 +0000
174+++ lib/lp/code/model/tests/test_linkedbranch.py 2010-07-22 13:01:10 +0000
175@@ -18,6 +18,7 @@
176 from lp.registry.interfaces.distroseries import NoSuchDistroSeries
177 from lp.registry.interfaces.pocket import PackagePublishingPocket
178 from lp.testing import run_with_login, TestCaseWithFactory
179+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
180
181
182 class TestProductSeriesLinkedBranch(TestCaseWithFactory):
183@@ -27,7 +28,9 @@
184 def test_branch(self):
185 # The linked branch of a product series is its branch attribute.
186 product_series = self.factory.makeProductSeries()
187- product_series.branch = self.factory.makeProductBranch(
188+ naked_product_series = remove_security_proxy_and_shout_at_engineer(
189+ product_series)
190+ naked_product_series.branch = self.factory.makeProductBranch(
191 product=product_series.product)
192 self.assertEqual(
193 product_series.branch, ICanHasLinkedBranch(product_series).branch)
194@@ -35,9 +38,11 @@
195 def test_setBranch(self):
196 # setBranch sets the linked branch of the product series.
197 product_series = self.factory.makeProductSeries()
198+ naked_product_series = remove_security_proxy_and_shout_at_engineer(
199+ product_series)
200 branch = self.factory.makeProductBranch(
201 product=product_series.product)
202- ICanHasLinkedBranch(product_series).setBranch(branch)
203+ ICanHasLinkedBranch(naked_product_series).setBranch(branch)
204 self.assertEqual(branch, product_series.branch)
205
206 def test_bzr_path(self):
207
208=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
209--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2010-07-20 12:12:46 +0000
210+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2010-07-22 13:01:10 +0000
211@@ -37,6 +37,7 @@
212 from lp.soyuz.model.processor import ProcessorFamily
213 from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
214 from lp.testing import ANONYMOUS, login, person_logged_in, TestCaseWithFactory
215+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
216 from lp.testing.fakemethod import FakeMethod
217 from lp.testing.mail_helpers import pop_notifications
218
219@@ -53,7 +54,9 @@
220 distroseries_i386 = distroseries.newArch(
221 'i386', ProcessorFamily.get(1), False, person,
222 supports_virtualized=True)
223- distroseries.nominatedarchindep = distroseries_i386
224+ naked_distroseries = remove_security_proxy_and_shout_at_engineer(
225+ distroseries)
226+ naked_distroseries.nominatedarchindep = distroseries_i386
227
228 return getUtility(ISourcePackageRecipeBuildSource).new(
229 distroseries=distroseries,
230@@ -320,7 +323,7 @@
231 removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
232 IStore(build).flush()
233 build.notify()
234- (message,) = pop_notifications()
235+ (message, ) = pop_notifications()
236 requester = build.requester
237 requester_address = format_address(
238 requester.displayname, requester.preferredemail.email)
239
240=== modified file 'lib/lp/registry/browser/tests/distroseries-views.txt'
241--- lib/lp/registry/browser/tests/distroseries-views.txt 2010-05-24 22:04:19 +0000
242+++ lib/lp/registry/browser/tests/distroseries-views.txt 2010-07-22 13:01:10 +0000
243@@ -196,7 +196,7 @@
244 >>> [field.__name__ for field in view.form_fields]
245 ['displayname', 'title', 'summary', 'description', 'status']
246
247- >>> print view.widgets.get('status')._getFormValue()
248+ >>> print view.widgets.get('status')._getFormValue().title
249 Active Development
250
251
252
253=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
254--- lib/lp/registry/browser/tests/milestone-views.txt 2010-06-30 19:37:09 +0000
255+++ lib/lp/registry/browser/tests/milestone-views.txt 2010-07-22 13:01:10 +0000
256@@ -579,9 +579,13 @@
257 The driver of a series that belongs to an `IDerivativeDistribution` is a
258 release manager and can create milestones.
259
260+ >>> from lp.testing.factory import (
261+ ... remove_security_proxy_and_shout_at_engineer)
262 >>> distroseries = factory.makeDistroRelease(name='pumpkin')
263 >>> driver = factory.makePerson(name='a-driver')
264- >>> distroseries.driver = driver
265+ >>> naked_distroseries = remove_security_proxy_and_shout_at_engineer(
266+ ... distroseries)
267+ >>> naked_distroseries.driver = driver
268 >>> login_person(driver)
269
270 >>> form = {
271
272=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
273--- lib/lp/registry/browser/tests/productseries-views.txt 2010-06-13 02:01:25 +0000
274+++ lib/lp/registry/browser/tests/productseries-views.txt 2010-07-22 13:01:10 +0000
275@@ -255,6 +255,8 @@
276
277 >>> from datetime import datetime
278 >>> from pytz import UTC
279+ >>> from lp.testing.factory import (
280+ ... remove_security_proxy_and_shout_at_engineer)
281
282 >>> product = factory.makeProduct(name="field", displayname='Field')
283 >>> productseries = factory.makeProductSeries(
284@@ -263,7 +265,9 @@
285
286 # Hack the creation date for testing purposes.
287 >>> test_date = datetime(2009, 05, 01, 19, 34, 24, tzinfo=UTC)
288- >>> productseries.datecreated = test_date
289+ >>> naked_productseries = remove_security_proxy_and_shout_at_engineer(
290+ ... productseries)
291+ >>> naked_productseries.datecreated = test_date
292
293 Users without edit permission cannot access the view.
294
295@@ -470,7 +474,7 @@
296
297 The series status is set to obsolete and the releasefileglob was set to None.
298
299- >>> print productseries.status
300+ >>> print productseries.status.title
301 Obsolete
302 >>> print productseries.releasefileglob
303 None
304
305=== modified file 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
306--- lib/lp/registry/stories/webservice/xx-project-registry.txt 2010-06-14 18:32:58 +0000
307+++ lib/lp/registry/stories/webservice/xx-project-registry.txt 2010-07-22 13:01:10 +0000
308@@ -847,15 +847,17 @@
309 The entry for a project series is available at its canonical URL on the
310 virtual host.
311
312+ >>> from zope.security.proxy import removeSecurityProxy
313 >>> login('test@canonical.com')
314 >>> babadoo_owner = factory.makePerson(name='babadoo-owner')
315 >>> babadoo = factory.makeProduct(name='babadoo', owner=babadoo_owner)
316 >>> foobadoo = factory.makeProductSeries(
317 ... product=babadoo, name='foobadoo', owner=babadoo_owner)
318- >>> foobadoo.summary = u'Foobadoo support for Babadoo'
319+ >>> removeSecurityProxy(foobadoo).summary = (
320+ ... u'Foobadoo support for Babadoo')
321 >>> fooey = factory.makeAnyBranch(
322 ... product=babadoo, name='fooey', owner=babadoo_owner)
323- >>> foobadoo.branch = fooey
324+ >>> removeSecurityProxy(foobadoo).branch = fooey
325 >>> logout()
326
327 >>> babadoo_foobadoo = webservice.get('/babadoo/foobadoo').jsonBody()
328
329=== modified file 'lib/lp/registry/tests/test_distroseries.py'
330--- lib/lp/registry/tests/test_distroseries.py 2010-06-23 23:23:28 +0000
331+++ lib/lp/registry/tests/test_distroseries.py 2010-07-22 13:01:10 +0000
332@@ -24,6 +24,7 @@
333 active_publishing_status, PackagePublishingStatus)
334 from lp.soyuz.model.processor import ProcessorFamilySet
335 from lp.testing import TestCase, TestCaseWithFactory
336+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
337 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
338 from lp.translations.interfaces.translations import (
339 TranslationsBranchImportMode)
340@@ -285,7 +286,9 @@
341 self.linkPackage('hot')
342 self.makeSeriesPackage('cold')
343 product_series = self.linkPackage('cold')
344- product_series.product.bugtraker = self.factory.makeBugTracker()
345+ naked_product_series = remove_security_proxy_and_shout_at_engineer(
346+ product_series)
347+ naked_product_series.product.bugtraker = self.factory.makeBugTracker()
348 packagings = self.series.getPrioritizedlPackagings()
349 names = [packaging.sourcepackagename.name for packaging in packagings]
350 expected = [u'hot', u'cold', u'linked']
351@@ -296,7 +299,9 @@
352 self.linkPackage('translatable')
353 self.makeSeriesPackage('withbranch')
354 product_series = self.linkPackage('withbranch')
355- product_series.branch = self.factory.makeBranch()
356+ naked_product_series = remove_security_proxy_and_shout_at_engineer(
357+ product_series)
358+ naked_product_series.branch = self.factory.makeBranch()
359 packagings = self.series.getPrioritizedlPackagings()
360 names = [packaging.sourcepackagename.name for packaging in packagings]
361 expected = [u'translatable', u'linked', u'withbranch']
362@@ -308,8 +313,10 @@
363 self.linkPackage('translatable')
364 self.makeSeriesPackage('importabletranslatable')
365 product_series = self.linkPackage('importabletranslatable')
366- product_series.branch = self.factory.makeBranch()
367- product_series.translations_autoimport_mode = (
368+ naked_product_series = remove_security_proxy_and_shout_at_engineer(
369+ product_series)
370+ naked_product_series.branch = self.factory.makeBranch()
371+ naked_product_series.translations_autoimport_mode = (
372 TranslationsBranchImportMode.IMPORT_TEMPLATES)
373 packagings = self.series.getPrioritizedlPackagings()
374 names = [packaging.sourcepackagename.name for packaging in packagings]
375@@ -343,7 +350,9 @@
376
377 new_distroseries = (
378 self.factory.makeDistroRelease(name=u"sampleseries"))
379- new_distroseries.hide_all_translations = False
380+ naked_new_distroseries = remove_security_proxy_and_shout_at_engineer(
381+ new_distroseries)
382+ naked_new_distroseries.hide_all_translations = False
383 transaction.commit()
384 translatables = self._get_translatables()
385 self.failUnlessEqual(
386@@ -365,7 +374,7 @@
387 translatables,
388 self._ref_translatables(u"sampleseries")))
389
390- new_distroseries.hide_all_translations = True
391+ naked_new_distroseries.hide_all_translations = True
392 transaction.commit()
393 translatables = self._get_translatables()
394 self.failUnlessEqual(
395
396=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
397--- lib/lp/registry/tests/test_sourcepackage.py 2010-02-24 03:06:54 +0000
398+++ lib/lp/registry/tests/test_sourcepackage.py 2010-07-22 13:01:10 +0000
399@@ -22,6 +22,7 @@
400 from lp.code.interfaces.seriessourcepackagebranch import (
401 IMakeOfficialBranchLinks)
402 from lp.testing import TestCaseWithFactory
403+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
404 from lp.testing.views import create_initialized_view
405 from canonical.testing.layers import DatabaseFunctionalLayer
406
407@@ -252,11 +253,17 @@
408
409 self.obsolete_productseries = self.factory.makeProductSeries(
410 name='obsolete', product=self.product)
411- self.obsolete_productseries.status = SeriesStatus.OBSOLETE
412+ naked_obsolete_productseries = (
413+ remove_security_proxy_and_shout_at_engineer(
414+ self.obsolete_productseries))
415+ naked_obsolete_productseries.status = SeriesStatus.OBSOLETE
416
417 self.dev_productseries = self.factory.makeProductSeries(
418 name='current', product=self.product)
419- self.dev_productseries.status = SeriesStatus.DEVELOPMENT
420+ naked_dev_productseries = (
421+ remove_security_proxy_and_shout_at_engineer(
422+ self.dev_productseries))
423+ naked_dev_productseries.status = SeriesStatus.DEVELOPMENT
424
425 self.distribution = self.factory.makeDistribution(
426 name='youbuntu', displayname='Youbuntu', owner=self.owner)
427
428=== modified file 'lib/lp/soyuz/browser/tests/test_distrosourcepackagerelease.py'
429--- lib/lp/soyuz/browser/tests/test_distrosourcepackagerelease.py 2010-07-20 12:06:36 +0000
430+++ lib/lp/soyuz/browser/tests/test_distrosourcepackagerelease.py 2010-07-22 13:01:10 +0000
431@@ -14,6 +14,7 @@
432 DistributionSourcePackageRelease)
433 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
434 from lp.testing import TestCaseWithFactory
435+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
436 from lp.testing.views import create_initialized_view
437
438
439@@ -27,7 +28,12 @@
440 # The package must be published for the page to render.
441 stp = SoyuzTestPublisher()
442 distroseries = stp.setUpDefaultDistroSeries()
443- distro = distroseries.distribution
444+ naked_distroseries = remove_security_proxy_and_shout_at_engineer(
445+ distroseries)
446+ # XXX Abel Deuring, 2010-07-21, bug 608240. This is scary. But
447+ # if we use distroseries.distribution instead,
448+ # test_spr_files_deleted() and test_spr_files_one() fail.
449+ distro = naked_distroseries.distribution
450 source_package_release = stp.getPubSource().sourcepackagerelease
451 self.dspr = DistributionSourcePackageRelease(
452 distro, source_package_release)
453
454=== modified file 'lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py'
455--- lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py 2010-07-20 12:06:36 +0000
456+++ lib/lp/soyuz/browser/tests/test_sourcepackagerelease.py 2010-07-22 13:01:10 +0000
457@@ -16,6 +16,7 @@
458 from canonical.testing import (
459 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
460 from lp.testing import TestCaseWithFactory
461+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
462 from lp.testing.views import create_initialized_view
463
464
465@@ -64,20 +65,23 @@
466
467 def test_highlighted_copyright_is_None(self):
468 expected = ''
469- self.source_package_release.copyright = None
470+ remove_security_proxy_and_shout_at_engineer(
471+ self.source_package_release).copyright = None
472 view = create_initialized_view(
473 self.source_package_release, '+copyright')
474 self.assertEqual(expected, view.highlighted_copyright)
475
476 def test_highlighted_copyright_no_matches(self):
477 expected = 'nothing to see and/or do.'
478- self.source_package_release.copyright = expected
479+ remove_security_proxy_and_shout_at_engineer(
480+ self.source_package_release).copyright = expected
481 view = create_initialized_view(
482 self.source_package_release, '+copyright')
483 self.assertEqual(expected, view.highlighted_copyright)
484
485 def test_highlighted_copyright_match_url(self):
486- self.source_package_release.copyright = (
487+ remove_security_proxy_and_shout_at_engineer(
488+ self.source_package_release).copyright = (
489 'Downloaded from https://upstream.dom/fnord/no/ and')
490 expected = (
491 'Downloaded from '
492@@ -88,7 +92,8 @@
493 self.assertEqual(expected, view.highlighted_copyright)
494
495 def test_highlighted_copyright_match_path(self):
496- self.source_package_release.copyright = (
497+ remove_security_proxy_and_shout_at_engineer(
498+ self.source_package_release).copyright = (
499 'See /usr/share/common-licenses/GPL')
500 expected = (
501 'See '
502@@ -98,7 +103,8 @@
503 self.assertEqual(expected, view.highlighted_copyright)
504
505 def test_highlighted_copyright_match_multiple(self):
506- self.source_package_release.copyright = (
507+ remove_security_proxy_and_shout_at_engineer(
508+ self.source_package_release).copyright = (
509 'See /usr/share/common-licenses/GPL or https://osi.org/mit')
510 expected = (
511 'See '
512
513=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
514--- lib/lp/soyuz/tests/test_publishing.py 2010-07-21 14:14:54 +0000
515+++ lib/lp/soyuz/tests/test_publishing.py 2010-07-22 13:01:10 +0000
516@@ -44,7 +44,8 @@
517 from lp.soyuz.interfaces.queue import PackageUploadStatus
518 from canonical.launchpad.scripts import FakeLogger
519 from lp.testing import TestCaseWithFactory
520-from lp.testing.factory import LaunchpadObjectFactory
521+from lp.testing.factory import (
522+ LaunchpadObjectFactory, remove_security_proxy_and_shout_at_engineer)
523 from lp.testing.fakemethod import FakeMethod
524
525
526@@ -146,7 +147,10 @@
527 PackageUploadStatus.DONE: 'setDone',
528 PackageUploadStatus.ACCEPTED: 'setAccepted',
529 }
530- method = getattr(package_upload, status_to_method[upload_status])
531+ naked_package_upload = remove_security_proxy_and_shout_at_engineer(
532+ package_upload)
533+ method = getattr(
534+ naked_package_upload, status_to_method[upload_status])
535 method()
536
537 return package_upload
538@@ -222,7 +226,9 @@
539 changes_file_name=changes_file_name,
540 changes_file_content=changes_file_content,
541 upload_status=upload_status)
542- package_upload.addSource(spr)
543+ naked_package_upload = remove_security_proxy_and_shout_at_engineer(
544+ package_upload)
545+ naked_package_upload.addSource(spr)
546
547 if filename is None:
548 filename = "%s_%s.dsc" % (sourcename, version)
549
550=== modified file 'lib/lp/testing/factory.py'
551--- lib/lp/testing/factory.py 2010-07-21 14:41:26 +0000
552+++ lib/lp/testing/factory.py 2010-07-22 13:01:10 +0000
553@@ -13,8 +13,10 @@
554 __metaclass__ = type
555 __all__ = [
556 'GPGSigningContext',
557+ 'is_security_proxied_or_harmless',
558 'LaunchpadObjectFactory',
559 'ObjectFactory',
560+ 'remove_security_proxy_and_shout_at_engineer',
561 ]
562
563 from contextlib import nested
564@@ -25,18 +27,22 @@
565 from email.mime.text import MIMEText
566 from email.mime.multipart import MIMEMultipart
567 from itertools import count
568+from operator import isSequenceType
569 import os.path
570 from random import randint
571 from StringIO import StringIO
572+import sys
573 from textwrap import dedent
574 from threading import local
575+from types import InstanceType
576
577 import pytz
578
579 from twisted.python.util import mergeFunctionMetadata
580
581 from zope.component import ComponentLookupError, getUtility
582-from zope.security.proxy import removeSecurityProxy
583+from zope.security.proxy import (
584+ builtin_isinstance, Proxy, ProxyFactory, removeSecurityProxy)
585
586 from canonical.autodecorate import AutoDecorate
587 from canonical.config import config
588@@ -325,7 +331,7 @@
589 branch_id, rcstype, url, cvs_root, cvs_module)
590
591
592-class LaunchpadObjectFactory(ObjectFactory):
593+class BareLaunchpadObjectFactory(ObjectFactory):
594 """Factory methods for creating Launchpad objects.
595
596 All the factory methods should be callable with no parameters.
597@@ -744,8 +750,8 @@
598 # We don't want to login() as the person used to create the product,
599 # so we remove the security proxy before creating the series.
600 naked_product = removeSecurityProxy(product)
601- return naked_product.newSeries(owner=owner, name=name,
602- summary=summary)
603+ return ProxyFactory(
604+ naked_product.newSeries(owner=owner, name=name, summary=summary))
605
606 def makeProject(self, name=None, displayname=None, title=None,
607 homepageurl=None, summary=None, owner=None,
608@@ -1671,7 +1677,7 @@
609 description=self.getUniqueString(),
610 parent_series=parent_series, owner=distribution.owner)
611 series.status = status
612- return series
613+ return ProxyFactory(series)
614
615 # Most people think of distro releases as distro series.
616 makeDistroSeries = makeDistroRelease
617@@ -1780,7 +1786,7 @@
618
619 def makeRecipeText(self, *branches):
620 if len(branches) == 0:
621- branches = (self.makeAnyBranch(),)
622+ branches = (self.makeAnyBranch(), )
623 base_branch = branches[0]
624 other_branches = branches[1:]
625 text = MINIMAL_RECIPE_TEXT % base_branch.bzr_identity
626@@ -1943,7 +1949,7 @@
627 productseries = self.makeProductSeries(owner=owner)
628 # Make it use Translations, otherwise there's little point
629 # to us creating a template for it.
630- productseries.product.official_rosetta = True
631+ removeSecurityProxy(productseries).product.official_rosetta = True
632 templateset = getUtility(IPOTemplateSet)
633 subset = templateset.getSubset(
634 distroseries, sourcepackagename, productseries)
635@@ -2718,3 +2724,72 @@
636 new_uuid = getUtility(ITemporaryStorageManager).new(blob, expires)
637
638 return getUtility(ITemporaryStorageManager).fetch(new_uuid)
639+
640+
641+# Some factory methods return simple Python types. We don't add
642+# security wrappers for them.
643+unwrapped_types = (
644+ DSCFile, InstanceType, Message, datetime, int, str, unicode)
645+
646+def is_security_proxied_or_harmless(obj):
647+ """Check that the given object is security wrapped or a harmless object."""
648+ if obj is None:
649+ return True
650+ if builtin_isinstance(obj, Proxy):
651+ return True
652+ if type(obj) in unwrapped_types:
653+ return True
654+ if isSequenceType(obj):
655+ for element in obj:
656+ if not is_security_proxied_or_harmless(element):
657+ return False
658+ return True
659+ return False
660+
661+
662+class LaunchpadObjectFactory:
663+ """A wrapper around `BareLaunchpadObjectFactory`.
664+
665+ Ensure that each object created by a `BareLaunchpadObjectFactory` method
666+ is either of a simple Python type or is security proxied.
667+
668+ A warning message is printed to stderr if a factory method creates
669+ an object without a security proxy.
670+
671+ Whereever you see such a warning: fix it!
672+ """
673+ def __init__(self):
674+ self._factory = BareLaunchpadObjectFactory()
675+
676+ def __getattr__(self, name):
677+ attr = getattr(self._factory, name)
678+ if callable(attr):
679+
680+ def guarded_method(*args, **kw):
681+ result = attr(*args, **kw)
682+ if not is_security_proxied_or_harmless(result):
683+ message = (
684+ "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
685+ "unproxied object." % name)
686+ print >>sys.stderr, message
687+ return result
688+ return guarded_method
689+ else:
690+ return attr
691+
692+
693+def remove_security_proxy_and_shout_at_engineer(obj):
694+ """Remove an object's security proxy and print a warning.
695+
696+ A number of LaunchpadObjectFactory methods returned objects without
697+ a security proxy. This is now no longer possible, but a number of
698+ tests rely on unrestricted access to object attributes.
699+
700+ This function should only be used in legacy tests which fail because
701+ they expect unproxied objects.
702+ """
703+ print >>sys.stderr, (
704+ "\nWarning: called removeSecurityProxy() for %r without a check if "
705+ "this reasonable. Look for a call of "
706+ "remove_security_proxy_and_shout_at_engineer(some_object)." % obj)
707+ return removeSecurityProxy(obj)
708
709=== modified file 'lib/lp/testing/tests/test_factory.py'
710--- lib/lp/testing/tests/test_factory.py 2010-07-17 18:40:02 +0000
711+++ lib/lp/testing/tests/test_factory.py 2010-07-22 13:01:10 +0000
712@@ -8,11 +8,13 @@
713 import unittest
714
715 from zope.component import getUtility
716+from zope.security.proxy import removeSecurityProxy
717
718 from canonical.launchpad.webapp.interfaces import ILaunchBag
719 from canonical.testing.layers import DatabaseFunctionalLayer
720 from lp.code.enums import CodeImportReviewStatus
721 from lp.testing import TestCaseWithFactory
722+from lp.testing.factory import is_security_proxied_or_harmless
723
724
725 class TestFactory(TestCaseWithFactory):
726@@ -39,6 +41,54 @@
727 self.assertIsNot(None, person)
728 self.assertEqual(person, current_person)
729
730+ def test_is_security_proxied_or_harmless__none(self):
731+ # is_security_proxied_or_harmless() considers the None object
732+ # to be a harmless object.
733+ self.assertTrue(is_security_proxied_or_harmless(None))
734+
735+ def test_is_security_proxied_or_harmless__int(self):
736+ # is_security_proxied_or_harmless() considers integers
737+ # to be harmless.
738+ self.assertTrue(is_security_proxied_or_harmless(1))
739+
740+ def test_is_security_proxied_or_harmless__string(self):
741+ # is_security_proxied_or_harmless() considers strings
742+ # to be harmless.
743+ self.assertTrue(is_security_proxied_or_harmless('abc'))
744+
745+ def test_is_security_proxied_or_harmless__unicode(self):
746+ # is_security_proxied_or_harmless() considers unicode objects
747+ # to be harmless.
748+ self.assertTrue(is_security_proxied_or_harmless(u'abc'))
749+
750+ def test_is_security_proxied_or_harmless__proxied_object(self):
751+ # is_security_proxied_or_harmless() treats security proxied
752+ # objects as harmless.
753+ proxied_person = self.factory.makePerson()
754+ self.assertTrue(is_security_proxied_or_harmless(proxied_person))
755+
756+ def test_is_security_proxied_or_harmless__unproxied_object(self):
757+ # is_security_proxied_or_harmless() treats security proxied
758+ # objects as harmless.
759+ unproxied_person = removeSecurityProxy(self.factory.makePerson())
760+ self.assertFalse(is_security_proxied_or_harmless(unproxied_person))
761+
762+ def test_is_security_proxied_or_harmless__sequence_harmless_content(self):
763+ # is_security_proxied_or_harmless() checks all elements
764+ # of a sequence. If all elements are harmless, so is the
765+ # sequence.
766+ proxied_person = self.factory.makePerson()
767+ self.assertTrue(
768+ is_security_proxied_or_harmless([1, '2', proxied_person]))
769+
770+ def test_is_security_proxied_or_harmless__sequence_harmful_content(self):
771+ # is_security_proxied_or_harmless() checks all elements
772+ # of a sequence. If at least elements are harmful, so is the
773+ # sequence.
774+ unproxied_person = removeSecurityProxy(self.factory.makePerson())
775+ self.assertFalse(
776+ is_security_proxied_or_harmless([1, '2', unproxied_person]))
777+
778
779 def test_suite():
780 return unittest.TestLoader().loadTestsFromName(__name__)
781
782=== modified file 'lib/lp/translations/browser/tests/test_breadcrumbs.py'
783--- lib/lp/translations/browser/tests/test_breadcrumbs.py 2010-07-19 15:31:57 +0000
784+++ lib/lp/translations/browser/tests/test_breadcrumbs.py 2010-07-22 13:01:10 +0000
785@@ -9,6 +9,7 @@
786
787 from lp.services.worlddata.interfaces.language import ILanguageSet
788 from lp.testing.breadcrumbs import BaseBreadcrumbTestCase
789+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
790 from lp.translations.interfaces.distroserieslanguage import (
791 IDistroSeriesLanguageSet)
792 from lp.translations.interfaces.productserieslanguage import (
793@@ -109,7 +110,8 @@
794 name='crumb-tester', displayname="Crumb Tester")
795 series = self.factory.makeDistroRelease(
796 name="test", version="1.0", distribution=distribution)
797- series.hide_all_translations = False
798+ naked_series = remove_security_proxy_and_shout_at_engineer(series)
799+ naked_series.hide_all_translations = False
800 serieslanguage = getUtility(IDistroSeriesLanguageSet).getDummy(
801 series, self.language)
802
803
804=== modified file 'lib/lp/translations/doc/translations-export-to-branch.txt'
805--- lib/lp/translations/doc/translations-export-to-branch.txt 2010-06-16 07:22:57 +0000
806+++ lib/lp/translations/doc/translations-export-to-branch.txt 2010-07-22 13:01:10 +0000
807@@ -220,8 +220,12 @@
808 >>> from email import message_from_string
809 >>> from lp.services.mail import stub
810 >>> from lp.codehosting.vfs import get_rw_server
811+ >>> from lp.testing.factory import (
812+ ... remove_security_proxy_and_shout_at_engineer)
813 >>> productseries = factory.makeProductSeries()
814- >>> productseries.translations_branch = factory.makeBranch()
815+ >>> naked_productseries = remove_security_proxy_and_shout_at_engineer(
816+ ... productseries)
817+ >>> naked_productseries.translations_branch = factory.makeBranch()
818 >>> template = factory.makePOTemplate(productseries=productseries)
819 >>> potmsgset = factory.makePOTMsgSet(template)
820 >>> pofile = removeSecurityProxy(
821
822=== modified file 'lib/lp/translations/stories/buildfarm/xx-build-summary.txt'
823--- lib/lp/translations/stories/buildfarm/xx-build-summary.txt 2010-03-16 11:09:46 +0000
824+++ lib/lp/translations/stories/buildfarm/xx-build-summary.txt 2010-07-22 13:01:10 +0000
825@@ -14,6 +14,8 @@
826 ... ILibraryFileAliasSet)
827 >>> from canonical.launchpad.scripts.logger import QuietFakeLogger
828 >>> from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
829+ >>> from lp.testing.factory import (
830+ ... remove_security_proxy_and_shout_at_engineer)
831 >>> from lp.testing.fakemethod import FakeMethod
832 >>> from lp.translations.interfaces.translations import (
833 ... TranslationsBranchImportMode)
834@@ -29,12 +31,15 @@
835
836 >>> productseries = factory.makeProductSeries(owner=owner)
837 >>> product = productseries.product
838- >>> product.official_rosetta = True
839+ >>> naked_product = remove_security_proxy_and_shout_at_engineer(product)
840+ >>> naked_product.official_rosetta = True
841 >>> branch = factory.makeProductBranch(product=product, owner=owner)
842 >>> branch_url = branch.unique_name
843
844- >>> productseries.branch = factory.makeBranch()
845- >>> productseries.translations_autoimport_mode = (
846+ >>> naked_productseries = remove_security_proxy_and_shout_at_engineer(
847+ ... productseries)
848+ >>> naked_productseries.branch = factory.makeBranch()
849+ >>> naked_productseries.translations_autoimport_mode = (
850 ... TranslationsBranchImportMode.IMPORT_TEMPLATES)
851 >>> specific_job = factory.makeTranslationTemplatesBuildJob(branch=branch)
852 >>> buildqueue = getUtility(IBuildQueueSet).getByJob(specific_job.job)