Launchpad itself

Merge lp:~adeuring/launchpad/librarian-filealiases into lp:launchpad

  1. librarian-filealiases
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 12041
Proposed branch: lp:~adeuring/launchpad/librarian-filealiases
Merge into: lp:launchpad
Diff against target: 280 lines (+16/-109)
6 files modified
lib/canonical/launchpad/browser/librarian.py (+13/-14)
lib/canonical/launchpad/rest/bytestorage.py (+0/-28)
lib/canonical/launchpad/zcml/webservice.zcml (+0/-13)
lib/lp/bugs/interfaces/bugattachment.py (+2/-2)
lib/lp/bugs/stories/webservice/xx-bug.txt (+1/-41)
lib/lp/services/fields/__init__.py (+0/-11)
To merge this branch: bzr merge lp:~adeuring/launchpad/librarian-filealiases
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+42942@code.launchpad.net

Commit message

[bug=620458,629804] [r=gmb][ui=none][bug=629804] Back out hack to serve restricted files directly from the internal restricted librarian port.

Description of the change

This branch removes a temporary workaround which we needed to give certain webservice clients access to restricted Librarian files.

The core of the workaround was to point webservice clients to an HTTP (not HHTPS) URL having a non-public hostname. This was necessary because we wanted to avoid serving the data via app servers, as done in loder variants of ProxiedLibraryFileAlias. The proper fix, access to restricted files via time-limited tokens, was not available at that time, but the Apport retracers (which are located in our DC and which can resolve the non-public hostname) needed access to the restricted files.

The workaround was implemented by the class RestrictedLibraryBackedByteStorage, together with a marker interface IRestriedBytes. These classes are now gone, together with some ZCML stuff. I also deleted a test for accessing restricted files. This test lived in longer page test file; removing it (the test, not the file...) changed the number of activities of test bug #1, which in turn required a minor change of at the end of the file.

And there is a bit of lint cleanup.

There are not special tests for this branch; the full test suite passed an EC2 run.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) :
review: Approve (code)
Revision history for this message
Robert Collins (lifeless) wrote :

This is great stuff Abel. Once its landed, QA'd and deployed, I wonder
if you'd have the time to follow on and remove the proxying support
completely? It would be good to have the code simplifications that
that should bring.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py 2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/browser/librarian.py 2010-12-09 14:21:53 +0000
@@ -85,11 +85,11 @@
8585
86 If the file is public, it will redirect to the files http url.86 If the file is public, it will redirect to the files http url.
8787
88 Otherwise if the feature flag publicrestrictedlibrarian is set to 'on' this88 Otherwise if the feature flag publicrestrictedlibrarian is set to 'on'
89 will allocate a token and redirect to the aliases private url.89 this will allocate a token and redirect to the aliases private url.
9090
91 Otherwise it will proxy the file in the appserver.91 Otherwise it will proxy the file in the appserver.
92 92
93 Once we no longer have any proxy code at all it should be possible to93 Once we no longer have any proxy code at all it should be possible to
94 consolidate this with LibraryFileAliasView.94 consolidate this with LibraryFileAliasView.
9595
@@ -98,7 +98,7 @@
98 we have to take special care about their origin.98 we have to take special care about their origin.
99 SafeStreamOrRedirectLibraryFileAliasView is used when we do not trust the99 SafeStreamOrRedirectLibraryFileAliasView is used when we do not trust the
100 content, otherwise StreamOrRedirectLibraryFileAliasView. We are working100 content, otherwise StreamOrRedirectLibraryFileAliasView. We are working
101 to remove both of these views entirely, but some transition will be 101 to remove both of these views entirely, but some transition will be
102 needed.102 needed.
103103
104 The context provides a file-like interface - it can be opened and closed104 The context provides a file-like interface - it can be opened and closed
@@ -161,7 +161,7 @@
161161
162 def browserDefault(self, request):162 def browserDefault(self, request):
163 """Decides how to deliver the file.163 """Decides how to deliver the file.
164 164
165 The options are:165 The options are:
166 - redirect to the contexts http url166 - redirect to the contexts http url
167 - redirect to a time limited secure url167 - redirect to a time limited secure url
@@ -182,7 +182,7 @@
182 # Public file, just point the client at the right place.182 # Public file, just point the client at the right place.
183 return RedirectionView(self.context.http_url, self.request), ()183 return RedirectionView(self.context.http_url, self.request), ()
184 if getFeatureFlag(u'publicrestrictedlibrarian') != 'on':184 if getFeatureFlag(u'publicrestrictedlibrarian') != 'on':
185 # Restricted file and we have not enabled the public 185 # Restricted file and we have not enabled the public
186 # restricted librarian yet :- deliver inline.186 # restricted librarian yet :- deliver inline.
187 self._when_streaming()187 self._when_streaming()
188 return self, ()188 return self, ()
@@ -203,12 +203,12 @@
203 """Hook for SafeStreamOrRedirectLibraryFileAliasView."""203 """Hook for SafeStreamOrRedirectLibraryFileAliasView."""
204204
205205
206
207class SafeStreamOrRedirectLibraryFileAliasView(MixedFileAliasView):206class SafeStreamOrRedirectLibraryFileAliasView(MixedFileAliasView):
208 """A view for Librarian files that sets the content disposition header."""207 """A view for Librarian files that sets the content disposition header."""
209208
210 def _when_streaming(self):209 def _when_streaming(self):
211 super(SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()210 super(
211 SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()
212 self.request.response.setHeader(212 self.request.response.setHeader(
213 'Content-Disposition', 'attachment')213 'Content-Disposition', 'attachment')
214214
@@ -257,18 +257,17 @@
257class ProxiedLibraryFileAlias:257class ProxiedLibraryFileAlias:
258 """A `LibraryFileAlias` decorator for use in URL generation.258 """A `LibraryFileAlias` decorator for use in URL generation.
259259
260 The URL's output by this decorator will always point at the webapp. This is260 The URL's output by this decorator will always point at the webapp. This
261 useful when:261 is useful when:
262 - we are proxying files via the webapp (as we do at the moment)262 - the webapp has to be contacted to get access to a file (required for
263 - when the webapp has to be contacted to get access to a file (the case263 restricted files).
264 for restricted files in the future)
265 - files might change from public to private and thus not work even if the264 - files might change from public to private and thus not work even if the
266 user has access to the once its private, unless they go via the webapp.265 user has access to the once its private, unless they go via the webapp.
267266
268 This should be used anywhere we are outputting URL's to LibraryFileAliases267 This should be used anywhere we are outputting URL's to LibraryFileAliases
269 other than directly in rendered pages. For rendered pages, using a268 other than directly in rendered pages. For rendered pages, using a
270 LibraryFileAlias directly is OK as at that point the status of the file269 LibraryFileAlias directly is OK as at that point the status of the file
271 is know.270 is known.
272271
273 Overrides `ILibraryFileAlias.http_url` to always point to the webapp URL,272 Overrides `ILibraryFileAlias.http_url` to always point to the webapp URL,
274 even when called from the webservice domain.273 even when called from the webservice domain.
275274
=== modified file 'lib/canonical/launchpad/rest/bytestorage.py'
--- lib/canonical/launchpad/rest/bytestorage.py 2010-09-03 20:28:36 +0000
+++ lib/canonical/launchpad/rest/bytestorage.py 2010-12-09 14:21:53 +0000
@@ -6,7 +6,6 @@
6__metaclass__ = type6__metaclass__ = type
7__all__ = [7__all__ = [
8 'LibraryBackedByteStorage',8 'LibraryBackedByteStorage',
9 'RestrictedLibraryBackedByteStorage',
10]9]
1110
1211
@@ -16,7 +15,6 @@
16from zope.component import getUtility15from zope.component import getUtility
17from zope.interface import implements16from zope.interface import implements
1817
19from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
20from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet18from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
21from canonical.launchpad.webapp.interfaces import ICanonicalUrlData19from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
2220
@@ -75,29 +73,3 @@
75 def deleteStored(self):73 def deleteStored(self):
76 """See `IByteStorage`."""74 """See `IByteStorage`."""
77 setattr(self.entry, self.field.__name__, None)75 setattr(self.entry, self.field.__name__, None)
78
79
80class RestrictedLibraryBackedByteStorage(LibraryBackedByteStorage):
81 """See `IByteStorage`.
82
83 This variant of LibraryBackedByteStorage provides an alias_url
84 which points to a StreamOrRedirectLibraryFileAliasView for
85 restricted Librarian files.
86 """
87
88 @property
89 def alias_url(self):
90 """See `IByteStorage`."""
91 if self.file_alias.restricted:
92 # XXX Abel Deuring 2010-09-03, bug=629804
93 # This is a bad hack: We can't give ordinary users API access to
94 # restricted files at present. But we can allow access to
95 # some machines in the data center (and should do that).
96 # We need here an HTTP URL, not an HTTPS URL, so we don't
97 # use getUrl(). Since http_url uses not an official domain
98 # name, all we do is expose an unaccessible HTTP URL also
99 # users outside of the data center, instead of the HTTPS URL
100 # returned by getURL().
101 return self.file_alias.http_url
102 else:
103 return self.file_alias.getURL()
10476
=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
--- lib/canonical/launchpad/zcml/webservice.zcml 2010-11-09 14:35:44 +0000
+++ lib/canonical/launchpad/zcml/webservice.zcml 2010-12-09 14:21:53 +0000
@@ -43,19 +43,6 @@
43 <allow interface='lazr.restful.interfaces.IByteStorage' />43 <allow interface='lazr.restful.interfaces.IByteStorage' />
44 </class>44 </class>
4545
46 <!-- Registration for the class that manages an entry's RestrictedBytes
47 fields. -->
48 <adapter
49 for="lazr.restful.interfaces.IEntry
50 lp.services.fields.IRestrictedBytes"
51 provides="lazr.restful.interfaces.IByteStorage"
52 factory="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage"
53 />
54
55 <class class="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage">
56 <allow interface='lazr.restful.interfaces.IByteStorage' />
57 </class>
58
59 <!-- WebService uses the default LaunchpadRootNavigation -->46 <!-- WebService uses the default LaunchpadRootNavigation -->
60 <view47 <view
61 for="canonical.launchpad.interfaces.launchpad.IWebServiceApplication"48 for="canonical.launchpad.interfaces.launchpad.IWebServiceApplication"
6249
=== modified file 'lib/lp/bugs/interfaces/bugattachment.py'
--- lib/lp/bugs/interfaces/bugattachment.py 2010-11-02 19:54:59 +0000
+++ lib/lp/bugs/interfaces/bugattachment.py 2010-12-09 14:21:53 +0000
@@ -39,7 +39,7 @@
39from canonical.launchpad import _39from canonical.launchpad import _
40from canonical.launchpad.interfaces.launchpad import IHasBug40from canonical.launchpad.interfaces.launchpad import IHasBug
41from canonical.launchpad.interfaces.message import IMessage41from canonical.launchpad.interfaces.message import IMessage
42from lp.services.fields import RestrictedBytes, Title42from lp.services.fields import Title
4343
4444
45class BugAttachmentType(DBEnumeratedType):45class BugAttachmentType(DBEnumeratedType):
@@ -115,7 +115,7 @@
115 libraryfile = Bytes(title=_("The attachment content."),115 libraryfile = Bytes(title=_("The attachment content."),
116 required=True)116 required=True)
117 data = exported(117 data = exported(
118 RestrictedBytes(title=_("The attachment content."),118 Bytes(title=_("The attachment content."),
119 required=True,119 required=True,
120 readonly=True))120 readonly=True))
121 message = exported(121 message = exported(
122122
=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-10-22 13:58:41 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2010-12-09 14:21:53 +0000
@@ -1360,46 +1360,6 @@
1360 ---1360 ---
13611361
13621362
1363Attachments of private bugs
1364~~~~~~~~~~~~~~~~~~~~~~~~~~~
1365
1366Attachments of private bugs are served by the app server itself.
1367
1368 >>> print webservice.patch(
1369 ... bug_one['self_link'], 'application/json', dumps(dict(private=True)))
1370 HTTP/1.1 209 Content Returned...
1371
1372 >>> response = webservice.named_post(
1373 ... bug_one['self_link'], 'addAttachment',
1374 ... data="67890", filename="more-numbers.txt", content_type='foo/bar',
1375 ... comment="The numbers you asked for.")
1376 >>> attachments = webservice.get(
1377 ... bug_one['attachments_collection_link']).jsonBody()
1378 >>> attachment = attachments['entries'][0]
1379 >>> pprint_entry(attachment)
1380 bug_link:...
1381 data_link: u'http://api.launchpad.dev/beta/bugs/1/+attachment/.../data'
1382 ...
1383
1384XXX Abel Deuring 2010-09-03, bug=629804: Please note that the http URLs in this
1385example are part of a temporary hack. Please see the comment in
1386canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage.
1387alias_url for details.
1388
1389 >>> response = webservice.get(attachment['data_link'])
1390 >>> print response
1391 HTTP/1.1 303 See Other
1392 Status: 303 See Other
1393 ...
1394 Location: http://localhost:58005/94/more-numbers.txt
1395 ...
1396
1397 >>> # Let's make the bug public again for later tests.
1398 >>> print webservice.patch(
1399 ... bug_one['self_link'], 'application/json', dumps(dict(private=False)))
1400 HTTP/1.1 209 Content Returned...
1401
1402
1403Searching for bugs1363Searching for bugs
1404------------------1364------------------
14051365
@@ -2133,7 +2093,7 @@
2133 next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'2093 next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'
2134 resource_type_link: u'http://.../#bug_activity-page-resource'2094 resource_type_link: u'http://.../#bug_activity-page-resource'
2135 start: 02095 start: 0
2136 total_size: 262096 total_size: 23
2137 ...2097 ...
21382098
2139 >>> bug_nine_activity = webservice.get(2099 >>> bug_nine_activity = webservice.get(
21402100
=== modified file 'lib/lp/services/fields/__init__.py'
--- lib/lp/services/fields/__init__.py 2010-10-23 16:45:43 +0000
+++ lib/lp/services/fields/__init__.py 2010-12-09 14:21:53 +0000
@@ -22,7 +22,6 @@
22 'ILocationField',22 'ILocationField',
23 'INoneableTextLine',23 'INoneableTextLine',
24 'IPasswordField',24 'IPasswordField',
25 'IRestrictedBytes',
26 'IStrippedTextLine',25 'IStrippedTextLine',
27 'ISummary',26 'ISummary',
28 'ITag',27 'ITag',
@@ -46,7 +45,6 @@
46 'ProductBugTracker',45 'ProductBugTracker',
47 'ProductNameField',46 'ProductNameField',
48 'PublicPersonChoice',47 'PublicPersonChoice',
49 'RestrictedBytes',
50 'SearchTag',48 'SearchTag',
51 'StrippedTextLine',49 'StrippedTextLine',
52 'Summary',50 'Summary',
@@ -229,10 +227,6 @@
229 """227 """
230228
231229
232class IRestrictedBytes(IBytes):
233 """A marker interface used for restricted LibraryFileAlias fields."""
234
235
236class StrippedTextLine(TextLine):230class StrippedTextLine(TextLine):
237 implements(IStrippedTextLine)231 implements(IStrippedTextLine)
238232
@@ -842,8 +836,3 @@
842 else:836 else:
843 # The vocabulary prevents the revealing of private team names.837 # The vocabulary prevents the revealing of private team names.
844 raise PrivateTeamNotAllowed(value)838 raise PrivateTeamNotAllowed(value)
845
846
847class RestrictedBytes(Bytes):
848 """A field for restricted LibraryFileAlias records."""
849 implements(IRestrictedBytes)