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
1=== modified file 'lib/canonical/launchpad/browser/librarian.py'
2--- lib/canonical/launchpad/browser/librarian.py 2010-11-08 12:52:43 +0000
3+++ lib/canonical/launchpad/browser/librarian.py 2010-12-09 14:21:53 +0000
4@@ -85,11 +85,11 @@
5
6 If the file is public, it will redirect to the files http url.
7
8- Otherwise if the feature flag publicrestrictedlibrarian is set to 'on' this
9- will allocate a token and redirect to the aliases private url.
10+ Otherwise if the feature flag publicrestrictedlibrarian is set to 'on'
11+ this will allocate a token and redirect to the aliases private url.
12
13 Otherwise it will proxy the file in the appserver.
14-
15+
16 Once we no longer have any proxy code at all it should be possible to
17 consolidate this with LibraryFileAliasView.
18
19@@ -98,7 +98,7 @@
20 we have to take special care about their origin.
21 SafeStreamOrRedirectLibraryFileAliasView is used when we do not trust the
22 content, otherwise StreamOrRedirectLibraryFileAliasView. We are working
23- to remove both of these views entirely, but some transition will be
24+ to remove both of these views entirely, but some transition will be
25 needed.
26
27 The context provides a file-like interface - it can be opened and closed
28@@ -161,7 +161,7 @@
29
30 def browserDefault(self, request):
31 """Decides how to deliver the file.
32-
33+
34 The options are:
35 - redirect to the contexts http url
36 - redirect to a time limited secure url
37@@ -182,7 +182,7 @@
38 # Public file, just point the client at the right place.
39 return RedirectionView(self.context.http_url, self.request), ()
40 if getFeatureFlag(u'publicrestrictedlibrarian') != 'on':
41- # Restricted file and we have not enabled the public
42+ # Restricted file and we have not enabled the public
43 # restricted librarian yet :- deliver inline.
44 self._when_streaming()
45 return self, ()
46@@ -203,12 +203,12 @@
47 """Hook for SafeStreamOrRedirectLibraryFileAliasView."""
48
49
50-
51 class SafeStreamOrRedirectLibraryFileAliasView(MixedFileAliasView):
52 """A view for Librarian files that sets the content disposition header."""
53
54 def _when_streaming(self):
55- super(SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()
56+ super(
57+ SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()
58 self.request.response.setHeader(
59 'Content-Disposition', 'attachment')
60
61@@ -257,18 +257,17 @@
62 class ProxiedLibraryFileAlias:
63 """A `LibraryFileAlias` decorator for use in URL generation.
64
65- The URL's output by this decorator will always point at the webapp. This is
66- useful when:
67- - we are proxying files via the webapp (as we do at the moment)
68- - when the webapp has to be contacted to get access to a file (the case
69- for restricted files in the future)
70+ The URL's output by this decorator will always point at the webapp. This
71+ is useful when:
72+ - the webapp has to be contacted to get access to a file (required for
73+ restricted files).
74 - files might change from public to private and thus not work even if the
75 user has access to the once its private, unless they go via the webapp.
76
77 This should be used anywhere we are outputting URL's to LibraryFileAliases
78 other than directly in rendered pages. For rendered pages, using a
79 LibraryFileAlias directly is OK as at that point the status of the file
80- is know.
81+ is known.
82
83 Overrides `ILibraryFileAlias.http_url` to always point to the webapp URL,
84 even when called from the webservice domain.
85
86=== modified file 'lib/canonical/launchpad/rest/bytestorage.py'
87--- lib/canonical/launchpad/rest/bytestorage.py 2010-09-03 20:28:36 +0000
88+++ lib/canonical/launchpad/rest/bytestorage.py 2010-12-09 14:21:53 +0000
89@@ -6,7 +6,6 @@
90 __metaclass__ = type
91 __all__ = [
92 'LibraryBackedByteStorage',
93- 'RestrictedLibraryBackedByteStorage',
94 ]
95
96
97@@ -16,7 +15,6 @@
98 from zope.component import getUtility
99 from zope.interface import implements
100
101-from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
102 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
103 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
104
105@@ -75,29 +73,3 @@
106 def deleteStored(self):
107 """See `IByteStorage`."""
108 setattr(self.entry, self.field.__name__, None)
109-
110-
111-class RestrictedLibraryBackedByteStorage(LibraryBackedByteStorage):
112- """See `IByteStorage`.
113-
114- This variant of LibraryBackedByteStorage provides an alias_url
115- which points to a StreamOrRedirectLibraryFileAliasView for
116- restricted Librarian files.
117- """
118-
119- @property
120- def alias_url(self):
121- """See `IByteStorage`."""
122- if self.file_alias.restricted:
123- # XXX Abel Deuring 2010-09-03, bug=629804
124- # This is a bad hack: We can't give ordinary users API access to
125- # restricted files at present. But we can allow access to
126- # some machines in the data center (and should do that).
127- # We need here an HTTP URL, not an HTTPS URL, so we don't
128- # use getUrl(). Since http_url uses not an official domain
129- # name, all we do is expose an unaccessible HTTP URL also
130- # users outside of the data center, instead of the HTTPS URL
131- # returned by getURL().
132- return self.file_alias.http_url
133- else:
134- return self.file_alias.getURL()
135
136=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
137--- lib/canonical/launchpad/zcml/webservice.zcml 2010-11-09 14:35:44 +0000
138+++ lib/canonical/launchpad/zcml/webservice.zcml 2010-12-09 14:21:53 +0000
139@@ -43,19 +43,6 @@
140 <allow interface='lazr.restful.interfaces.IByteStorage' />
141 </class>
142
143- <!-- Registration for the class that manages an entry's RestrictedBytes
144- fields. -->
145- <adapter
146- for="lazr.restful.interfaces.IEntry
147- lp.services.fields.IRestrictedBytes"
148- provides="lazr.restful.interfaces.IByteStorage"
149- factory="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage"
150- />
151-
152- <class class="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage">
153- <allow interface='lazr.restful.interfaces.IByteStorage' />
154- </class>
155-
156 <!-- WebService uses the default LaunchpadRootNavigation -->
157 <view
158 for="canonical.launchpad.interfaces.launchpad.IWebServiceApplication"
159
160=== modified file 'lib/lp/bugs/interfaces/bugattachment.py'
161--- lib/lp/bugs/interfaces/bugattachment.py 2010-11-02 19:54:59 +0000
162+++ lib/lp/bugs/interfaces/bugattachment.py 2010-12-09 14:21:53 +0000
163@@ -39,7 +39,7 @@
164 from canonical.launchpad import _
165 from canonical.launchpad.interfaces.launchpad import IHasBug
166 from canonical.launchpad.interfaces.message import IMessage
167-from lp.services.fields import RestrictedBytes, Title
168+from lp.services.fields import Title
169
170
171 class BugAttachmentType(DBEnumeratedType):
172@@ -115,7 +115,7 @@
173 libraryfile = Bytes(title=_("The attachment content."),
174 required=True)
175 data = exported(
176- RestrictedBytes(title=_("The attachment content."),
177+ Bytes(title=_("The attachment content."),
178 required=True,
179 readonly=True))
180 message = exported(
181
182=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
183--- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-10-22 13:58:41 +0000
184+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2010-12-09 14:21:53 +0000
185@@ -1360,46 +1360,6 @@
186 ---
187
188
189-Attachments of private bugs
190-~~~~~~~~~~~~~~~~~~~~~~~~~~~
191-
192-Attachments of private bugs are served by the app server itself.
193-
194- >>> print webservice.patch(
195- ... bug_one['self_link'], 'application/json', dumps(dict(private=True)))
196- HTTP/1.1 209 Content Returned...
197-
198- >>> response = webservice.named_post(
199- ... bug_one['self_link'], 'addAttachment',
200- ... data="67890", filename="more-numbers.txt", content_type='foo/bar',
201- ... comment="The numbers you asked for.")
202- >>> attachments = webservice.get(
203- ... bug_one['attachments_collection_link']).jsonBody()
204- >>> attachment = attachments['entries'][0]
205- >>> pprint_entry(attachment)
206- bug_link:...
207- data_link: u'http://api.launchpad.dev/beta/bugs/1/+attachment/.../data'
208- ...
209-
210-XXX Abel Deuring 2010-09-03, bug=629804: Please note that the http URLs in this
211-example are part of a temporary hack. Please see the comment in
212-canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage.
213-alias_url for details.
214-
215- >>> response = webservice.get(attachment['data_link'])
216- >>> print response
217- HTTP/1.1 303 See Other
218- Status: 303 See Other
219- ...
220- Location: http://localhost:58005/94/more-numbers.txt
221- ...
222-
223- >>> # Let's make the bug public again for later tests.
224- >>> print webservice.patch(
225- ... bug_one['self_link'], 'application/json', dumps(dict(private=False)))
226- HTTP/1.1 209 Content Returned...
227-
228-
229 Searching for bugs
230 ------------------
231
232@@ -2133,7 +2093,7 @@
233 next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'
234 resource_type_link: u'http://.../#bug_activity-page-resource'
235 start: 0
236- total_size: 26
237+ total_size: 23
238 ...
239
240 >>> bug_nine_activity = webservice.get(
241
242=== modified file 'lib/lp/services/fields/__init__.py'
243--- lib/lp/services/fields/__init__.py 2010-10-23 16:45:43 +0000
244+++ lib/lp/services/fields/__init__.py 2010-12-09 14:21:53 +0000
245@@ -22,7 +22,6 @@
246 'ILocationField',
247 'INoneableTextLine',
248 'IPasswordField',
249- 'IRestrictedBytes',
250 'IStrippedTextLine',
251 'ISummary',
252 'ITag',
253@@ -46,7 +45,6 @@
254 'ProductBugTracker',
255 'ProductNameField',
256 'PublicPersonChoice',
257- 'RestrictedBytes',
258 'SearchTag',
259 'StrippedTextLine',
260 'Summary',
261@@ -229,10 +227,6 @@
262 """
263
264
265-class IRestrictedBytes(IBytes):
266- """A marker interface used for restricted LibraryFileAlias fields."""
267-
268-
269 class StrippedTextLine(TextLine):
270 implements(IStrippedTextLine)
271
272@@ -842,8 +836,3 @@
273 else:
274 # The vocabulary prevents the revealing of private team names.
275 raise PrivateTeamNotAllowed(value)
276-
277-
278-class RestrictedBytes(Bytes):
279- """A field for restricted LibraryFileAlias records."""
280- implements(IRestrictedBytes)