Merge lp:~lifeless/launchpad/private-librarian into lp:launchpad/db-devel
- private-librarian
- Merge into db-devel
Status: | Merged |
---|---|
Approved by: | Stuart Bishop |
Approved revision: | no longer in the source branch. |
Merged at revision: | 9757 |
Proposed branch: | lp:~lifeless/launchpad/private-librarian |
Merge into: | lp:launchpad/db-devel |
Diff against target: |
1731 lines (+804/-148) 29 files modified
database/schema/launchpad_session.sql (+16/-0) lib/canonical/database/sqlbase.py (+10/-2) lib/canonical/launchpad/browser/librarian.py (+88/-12) lib/canonical/launchpad/database/ftests/test_timelimitedtoken.py (+29/-0) lib/canonical/launchpad/database/librarian.py (+69/-1) lib/canonical/launchpad/doc/librarian.txt (+175/-24) lib/canonical/launchpad/ftests/test_libraryfilealias.py (+1/-6) lib/canonical/launchpad/interfaces/librarian.py (+5/-1) lib/canonical/launchpad/interfaces/lpstorm.py (+0/-1) lib/canonical/launchpad/scripts/garbo.py (+47/-5) lib/canonical/launchpad/scripts/tests/test_garbo.py (+23/-2) lib/canonical/launchpad/utilities/looptuner.py (+4/-0) lib/canonical/launchpad/webapp/session.py (+2/-1) lib/canonical/launchpad/zcml/librarian.zcml (+5/-0) lib/canonical/librarian/client.py (+37/-9) lib/canonical/librarian/db.py (+33/-5) lib/canonical/librarian/ftests/test_db.py (+14/-12) lib/canonical/librarian/ftests/test_storage.py (+3/-2) lib/canonical/librarian/ftests/test_web.py (+157/-25) lib/canonical/librarian/interfaces.py (+13/-2) lib/canonical/librarian/storage.py (+2/-2) lib/canonical/librarian/web.py (+37/-9) lib/canonical/testing/layers.py (+6/-4) lib/lp/bugs/browser/bugattachment.py (+1/-16) lib/lp/bugs/browser/configure.zcml (+0/-4) lib/lp/bugs/browser/tests/test_bugattachment_file_access.py (+17/-2) lib/lp/scripts/utilities/importfascist.py (+1/-0) lib/lp/services/features/webapp.py (+8/-0) lib/lp/testing/fakelibrarian.py (+1/-1) |
To merge this branch: | bzr merge lp:~lifeless/launchpad/private-librarian |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stuart Bishop (community) | Approve | ||
Graham Binns (community) | release-critical | Approve | |
Launchpad code reviewers | Pending | ||
Review via email:
|
Commit message
Introduce a public restricted librarian uses a time limited token approach.
Description of the change
The basic idea is to have an https librarian that uses an access token for a time limited period, rather than proxying on the appservers which is terrible in several ways that aren't all that relevant except to say its hard to improve and incompatible with our peformance goals.
So in this model, we hand out a token when someone (including wget) accesses a private attachment on launchpad, and issue a temporary redirect (over ssl) to https:/
The token goes in the session DB, the garbo cleans that up, and we all are happy happy happy.
Oh, and the librarian rejects requests without a token for private files.
We can't use OAuth because then the OAuth token would be attackable by content in the private librarian.
RT 41202 contains the request for wildcard DNS keys.
The remaining work to make this fully reviewable is to:
- profit
Folk wanting to test or collaborate on this branch should:
- dropdb session_dev
- dropdb session_ftest
- pull the branch
- run 'make schema' which will create the timelimitedtoken table in your session dbs.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stuart Bishop (stub) wrote : | # |
So one thing we will need to do is raise an exception if we try to generate a secure url as an anonymous user. This is a safety net, as I don't think we are doing this at the moment. It will stop accidentally ending up with stale URLs in the Squid caches.
I think I understand why the session store was (ab)used here - it runs in autocommit mode and we rollback the database connections on GET requests to ensure they are idempotent. I still don't like it but can't really offer a better solution with our existing infrastructure except perhaps using memcached. Certainly session.sql should not be modified though, with all the new stuff going into launchpad_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Collins (lifeless) wrote : | # |
I used the session DB because this stuff is transient : memcached might be an answer too but the token *system* has to be available or private file downloads will fail, and memcached is currently (AIUI) a 'when its there use it' system - which is fine. Replicating the tokens isn't terribly useful given we're just going to delete them again, but all of that is a bit beside the point : yes, we do need to commit changes in get requests :).
Will move the definitions to launchpad_
We could store the expiry time, but I figured we'd make the lifetime a config option and thus work from the creation point rather than expiry.
I figured we might want reporting on stuff in the table - thats why I used url, token, rather than a single column. Easy to change if you have a strong opinion any which way - I don't.
I can't answer the url/path segment question yet : once its fully hooked up it should be obvious what is easiest. The current schema is what was easiest based on the attributes of the objects I am using (LibraryFileAlias etc).
We could make the librarian offer a redirect *back* to launchpad for expired tokens, rather than a go-away message - adding the source canonical_url to the table would permit that trivially.
Re: exceptions for anonymous users calling TimeLimitedToke
-Rob
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stuart Bishop (stub) wrote : | # |
On Wed, Jul 28, 2010 at 5:08 AM, Robert Collins
<email address hidden> wrote:
> I used the session DB because this stuff is transient : memcached might be an answer too but the token *system* has to be available or private file downloads will fail, and memcached is currently (AIUI) a 'when its there use it' system - which is fine. Replicating the tokens isn't terribly useful given we're just going to delete them again, but all of that is a bit beside the point : yes, we do need to commit changes in get requests :).
Right. We should consider promoting memcached to an always-on service
as we have tripped over a few cases where this would be desirable.
That is a separate task though. Stuffing this in the session database
seems the best fit for now.
> Will move the definitions to launchpad_
Ta.
> We could store the expiry time, but I figured we'd make the lifetime a config option and thus work from the creation point rather than expiry.
>
> I figured we might want reporting on stuff in the table - thats why I used url, token, rather than a single column. Easy to change if you have a strong opinion any which way - I don't.
>
> I can't answer the url/path segment question yet : once its fully hooked up it should be obvious what is easiest. The current schema is what was easiest based on the attributes of the objects I am using (LibraryFileAlias etc).
I suspect the host and port is just noise and we only want the path.
We have a complex environment, and relying on the URLs seems fragile
so I personally would feel more secure if we just stored the path.
> We could make the librarian offer a redirect *back* to launchpad for expired tokens, rather than a go-away message - adding the source canonical_url to the table would permit that trivially.
That sounds like a good idea.
> Re: exceptions for anonymous users calling TimeLimitedToke
If we offer the redirect back on expired tokens, there is no need for
this. My concern was expired tokens getting stuck in caches and Squid
caching Anonymous connections was the first I thought off.
After going over bug lists with Gary last night, this will solve a lot
of outstanding Librarian issues.
--
Stuart Bishop <email address hidden>
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stuart Bishop (stub) wrote : | # |
So files served from the same domain can steal content from files served from the same domain. We don't care for public information, but this isn't cool for private.
One way around this is to serve every file from a unique domain. Changing the URL structure from https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Graham Binns (gmb) : | # |
Preview Diff
1 | === modified file 'database/schema/launchpad_session.sql' |
2 | --- database/schema/launchpad_session.sql 2009-06-24 21:17:33 +0000 |
3 | +++ database/schema/launchpad_session.sql 2010-09-07 04:07:54 +0000 |
4 | @@ -14,3 +14,19 @@ |
5 | GRANT EXECUTE ON FUNCTION |
6 | set_session_pkg_data(text, text, text, bytea) TO session; |
7 | |
8 | +CREATE TABLE TimeLimitedToken ( |
9 | + path text NOT NULL, |
10 | + token text NOT NULL, |
11 | + created timestamp without time zone NOT NULL DEFAULT CURRENT_TIMESTAMP, |
12 | + constraint timelimitedtoken_pky primary key (path, token) |
13 | + ) WITHOUT OIDS; |
14 | +COMMENT ON TABLE TimeLimitedToken IS 'stores tokens for granting access to a single path in the librarian for a short while. The garbo takes care of cleanups, and we should only have a few thousand at a time. Tokens are handed out just-in-time on the appserver, when a client attempts to dereference a private thing which we do not want to deliver in-line. OAuth tokens cannot be used for the launchpadlibrarian content because they would then be attackable. See lib.canonical.database.librarian for the python class.'; |
15 | +-- Give the garbo an efficient selection to cleanup |
16 | +CREATE INDEX timelimitedtoken_created ON TimeLimitedToken(created); |
17 | +-- Give the librarian an efficient lookup |
18 | +CREATE INDEX timelimitedtoken_path_token ON TimeLimitedToken(path, token); |
19 | + |
20 | +-- Let the session user access file access tokens. |
21 | +GRANT SELECT, INSERT, UPDATE, DELETE ON TimeLimitedToken TO session; |
22 | +-- And the garbo needs to run on it too. |
23 | +GRANT SELECT, DELETE ON TimeLimitedToken TO session; |
24 | |
25 | === modified file 'lib/canonical/database/sqlbase.py' |
26 | --- lib/canonical/database/sqlbase.py 2010-08-24 10:45:57 +0000 |
27 | +++ lib/canonical/database/sqlbase.py 2010-09-07 04:07:54 +0000 |
28 | @@ -27,11 +27,12 @@ |
29 | 'RandomiseOrderDescriptor', |
30 | 'reset_store', |
31 | 'rollback', |
32 | + 'session_store', |
33 | 'SQLBase', |
34 | 'sqlvalues', |
35 | 'StupidCache', |
36 | 'ZopelessTransactionManager', |
37 | -] |
38 | + ] |
39 | |
40 | |
41 | from datetime import datetime |
42 | @@ -578,7 +579,7 @@ |
43 | |
44 | """ |
45 | if not isinstance(x, basestring): |
46 | - raise TypeError, 'Not a string (%s)' % type(x) |
47 | + raise TypeError('Not a string (%s)' % type(x)) |
48 | return quote(x).replace('%', r'\\%').replace('_', r'\\_') |
49 | |
50 | |
51 | @@ -693,6 +694,7 @@ |
52 | clause = clause.replace('?', '%s') % sqlvalues(*parameters) |
53 | return clause |
54 | |
55 | + |
56 | def flush_database_updates(): |
57 | """Flushes all pending database updates. |
58 | |
59 | @@ -833,6 +835,7 @@ |
60 | DEPRECATED - use of this class is deprecated in favour of using |
61 | Store.execute(). |
62 | """ |
63 | + |
64 | def __init__(self): |
65 | self._connection = _get_sqlobject_store()._connection |
66 | self._result = None |
67 | @@ -861,3 +864,8 @@ |
68 | if self._result is not None: |
69 | self._result.close() |
70 | self._result = None |
71 | + |
72 | + |
73 | +def session_store(): |
74 | + """Return a store connected to the session DB.""" |
75 | + return getUtility(IZStorm).get('session', 'launchpad-session:') |
76 | |
77 | === modified file 'lib/canonical/launchpad/browser/librarian.py' |
78 | --- lib/canonical/launchpad/browser/librarian.py 2010-09-03 07:53:11 +0000 |
79 | +++ lib/canonical/launchpad/browser/librarian.py 2010-09-07 04:07:54 +0000 |
80 | @@ -11,7 +11,9 @@ |
81 | 'LibraryFileAliasMD5View', |
82 | 'LibraryFileAliasView', |
83 | 'ProxiedLibraryFileAlias', |
84 | + 'SafeStreamOrRedirectLibraryFileAliasView', |
85 | 'StreamOrRedirectLibraryFileAliasView', |
86 | + 'RedirectPerhapsWithTokenLibraryFileAliasView', |
87 | ] |
88 | |
89 | import os |
90 | @@ -44,6 +46,7 @@ |
91 | filechunks, |
92 | guess_librarian_encoding, |
93 | ) |
94 | +from lp.services.features import getFeatureFlag |
95 | |
96 | |
97 | class LibraryFileAliasView(LaunchpadView): |
98 | @@ -77,11 +80,29 @@ |
99 | return '%s %s' % (self.context.content.md5, self.context.filename) |
100 | |
101 | |
102 | -class StreamOrRedirectLibraryFileAliasView(LaunchpadView): |
103 | +class MixedFileAliasView(LaunchpadView): |
104 | """Stream or redirects to `ILibraryFileAlias`. |
105 | |
106 | - It streams the contents of restricted library files or redirects |
107 | - to public ones. |
108 | + If the file is public, it will redirect to the files http url. |
109 | + |
110 | + Otherwise if the feature flag publicrestrictedlibrarian is set to 'on' this |
111 | + will allocate a token and redirect to the aliases private url. |
112 | + |
113 | + Otherwise it will proxy the file in the appserver. |
114 | + |
115 | + Once we no longer have any proxy code at all it should be possible to |
116 | + consolidate this with LibraryFileAliasView. |
117 | + |
118 | + Note that streaming restricted files is a security concern - they show up |
119 | + in the launchpad.net domain rather than launchpadlibrarian.net and thus |
120 | + we have to take special care about their origin. |
121 | + SafeStreamOrRedirectLibraryFileAliasView is used when we do not trust the |
122 | + content, otherwise StreamOrRedirectLibraryFileAliasView. We are working |
123 | + to remove both of these views entirely, but some transition will be |
124 | + needed. |
125 | + |
126 | + The context provides a file-like interface - it can be opened and closed |
127 | + and read from. |
128 | """ |
129 | implements(IBrowserPublisher) |
130 | |
131 | @@ -91,6 +112,8 @@ |
132 | # the shell environment changes. Download the library file |
133 | # content into a local temporary file. Finally, restore original |
134 | # proxy-settings and refresh the urllib2 opener. |
135 | + # XXX: This is not threadsafe, so two calls at once will collide and |
136 | + # can then corrupt the variable. bug=395960 |
137 | original_proxy = os.getenv('http_proxy') |
138 | try: |
139 | if original_proxy is not None: |
140 | @@ -137,25 +160,65 @@ |
141 | return tmp_file |
142 | |
143 | def browserDefault(self, request): |
144 | - """Decides whether to redirect or stream the file content. |
145 | + """Decides how to deliver the file. |
146 | + |
147 | + The options are: |
148 | + - redirect to the contexts http url |
149 | + - redirect to a time limited secure url |
150 | + - stream the file content. |
151 | |
152 | Only restricted file contents are streamed, finishing the traversal |
153 | chain with this view. If the context file is public return the |
154 | appropriate `RedirectionView` for its HTTP url. |
155 | """ |
156 | + # Perhaps we should give a 404 at this point rather than asserting? |
157 | + # If someone has a page open with an attachment link, then someone |
158 | + # else deletes the attachment, this is a normal situation, not an |
159 | + # error. -- RBC 20100726. |
160 | assert not self.context.deleted, ( |
161 | - "StreamOrRedirectLibraryFileAliasView can not operate on " |
162 | - "deleted librarian files, since their URL is undefined.") |
163 | - |
164 | - if self.context.restricted: |
165 | + "MixedFileAliasView can not operate on deleted librarian files, " |
166 | + "since their URL is undefined.") |
167 | + if not self.context.restricted: |
168 | + # Public file, just point the client at the right place. |
169 | + return RedirectionView(self.context.http_url, self.request), () |
170 | + if getFeatureFlag(u'publicrestrictedlibrarian') != 'on': |
171 | + # Restricted file and we have not enabled the public |
172 | + # restricted librarian yet :- deliver inline. |
173 | + self._when_streaming() |
174 | return self, () |
175 | - |
176 | - return RedirectionView(self.context.http_url, self.request), () |
177 | + # Avoids a circular import seen in |
178 | + # scripts/ftests/librarianformatter.txt |
179 | + from canonical.launchpad.database.librarian import TimeLimitedToken |
180 | + # Allocate a token for the file to be accessed for a limited time and |
181 | + # redirect the client to the file. |
182 | + token = TimeLimitedToken.allocate(self.context.private_url) |
183 | + final_url = self.context.private_url + '?token=%s' % token |
184 | + return RedirectionView(final_url, self.request), () |
185 | |
186 | def publishTraverse(self, request, name): |
187 | - """See `IBrowserPublisher`.""" |
188 | + """See `IBrowserPublisher` - can't traverse below a file.""" |
189 | raise NotFound(name, self.context) |
190 | |
191 | + def _when_streaming(self): |
192 | + """Hook for SafeStreamOrRedirectLibraryFileAliasView.""" |
193 | + |
194 | + |
195 | + |
196 | +class SafeStreamOrRedirectLibraryFileAliasView(MixedFileAliasView): |
197 | + """A view for Librarian files that sets the content disposition header.""" |
198 | + |
199 | + def _when_streaming(self): |
200 | + super(SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming() |
201 | + self.request.response.setHeader( |
202 | + 'Content-Disposition', 'attachment') |
203 | + |
204 | + |
205 | +# The eventual name of MixedFileAliasView once the proxy code |
206 | +# is ripped out. |
207 | +RedirectPerhapsWithTokenLibraryFileAliasView = MixedFileAliasView |
208 | +# The name for the old behaviour being removed: |
209 | +StreamOrRedirectLibraryFileAliasView = MixedFileAliasView |
210 | + |
211 | |
212 | class DeletedProxiedLibraryFileAlias(NotFound): |
213 | """Raised when a deleted `ProxiedLibraryFileAlias` is accessed.""" |
214 | @@ -192,7 +255,20 @@ |
215 | |
216 | |
217 | class ProxiedLibraryFileAlias: |
218 | - """A `LibraryFileAlias` proxied via webapp. |
219 | + """A `LibraryFileAlias` decorator for use in URL generation. |
220 | + |
221 | + The URL's output by this decorator will always point at the webapp. This is |
222 | + useful when: |
223 | + - we are proxying files via the webapp (as we do at the moment) |
224 | + - when the webapp has to be contacted to get access to a file (the case |
225 | + for restricted files in the future) |
226 | + - files might change from public to private and thus not work even if the |
227 | + user has access to the once its private, unless they go via the webapp. |
228 | + |
229 | + This should be used anywhere we are outputting URL's to LibraryFileAliases |
230 | + other than directly in rendered pages. For rendered pages, using a |
231 | + LibraryFileAlias directly is OK as at that point the status of the file |
232 | + is know. |
233 | |
234 | Overrides `ILibraryFileAlias.http_url` to always point to the webapp URL, |
235 | even when called from the webservice domain. |
236 | |
237 | === added file 'lib/canonical/launchpad/database/ftests/test_timelimitedtoken.py' |
238 | --- lib/canonical/launchpad/database/ftests/test_timelimitedtoken.py 1970-01-01 00:00:00 +0000 |
239 | +++ lib/canonical/launchpad/database/ftests/test_timelimitedtoken.py 2010-09-07 04:07:54 +0000 |
240 | @@ -0,0 +1,29 @@ |
241 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
242 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
243 | + |
244 | +"""Tests for TimeLimitedToken.""" |
245 | + |
246 | +__metaclass__ = type |
247 | + |
248 | +import testtools |
249 | +import transaction |
250 | + |
251 | +from canonical.database.sqlbase import session_store |
252 | +from canonical.launchpad.database.librarian import TimeLimitedToken |
253 | +from canonical.testing import LaunchpadFunctionalLayer |
254 | + |
255 | + |
256 | +class TestLibraryFileAlias(testtools.TestCase): |
257 | + |
258 | + layer = LaunchpadFunctionalLayer |
259 | + |
260 | + def test_allocate(self): |
261 | + store = session_store() |
262 | + store.find(TimeLimitedToken).remove() |
263 | + token1 = TimeLimitedToken.allocate('foo://') |
264 | + token2 = TimeLimitedToken.allocate('foo://') |
265 | + # We must get unique tokens |
266 | + self.assertNotEqual(token1, token2) |
267 | + # They must be bytestrings (as a surrogate for valid url fragment') |
268 | + self.assertIsInstance(token1, str) |
269 | + self.assertIsInstance(token2, str) |
270 | |
271 | === modified file 'lib/canonical/launchpad/database/librarian.py' |
272 | --- lib/canonical/launchpad/database/librarian.py 2010-08-20 20:31:18 +0000 |
273 | +++ lib/canonical/launchpad/database/librarian.py 2010-09-07 04:07:54 +0000 |
274 | @@ -10,12 +10,16 @@ |
275 | 'LibraryFileAliasSet', |
276 | 'LibraryFileContent', |
277 | 'LibraryFileDownloadCount', |
278 | + 'TimeLimitedToken', |
279 | ] |
280 | |
281 | from datetime import ( |
282 | datetime, |
283 | timedelta, |
284 | ) |
285 | +from hashlib import md5 |
286 | +import random |
287 | +from urlparse import urlparse |
288 | |
289 | from lazr.delegates import delegates |
290 | import pytz |
291 | @@ -26,6 +30,7 @@ |
292 | SQLRelatedJoin, |
293 | StringCol, |
294 | ) |
295 | +import storm.base |
296 | from storm.locals import ( |
297 | Date, |
298 | Desc, |
299 | @@ -48,7 +53,10 @@ |
300 | UTC_NOW, |
301 | ) |
302 | from canonical.database.datetimecol import UtcDateTimeCol |
303 | -from canonical.database.sqlbase import SQLBase |
304 | +from canonical.database.sqlbase import ( |
305 | + session_store, |
306 | + SQLBase, |
307 | + ) |
308 | from canonical.launchpad.interfaces import ( |
309 | ILibraryFileAlias, |
310 | ILibraryFileAliasSet, |
311 | @@ -127,8 +135,15 @@ |
312 | return url |
313 | return url.replace('http', 'https', 1) |
314 | |
315 | + @property |
316 | + def private_url(self): |
317 | + """See ILibraryFileAlias.https_url""" |
318 | + return self.client.getURLForAlias(self.id, secure=True) |
319 | + |
320 | def getURL(self): |
321 | """See ILibraryFileAlias.getURL""" |
322 | + if self.restricted: |
323 | + return self.private_url |
324 | if config.librarian.use_https: |
325 | return self.https_url |
326 | else: |
327 | @@ -285,3 +300,56 @@ |
328 | count = Int(allow_none=False) |
329 | country_id = Int(name='country', allow_none=True) |
330 | country = Reference(country_id, 'Country.id') |
331 | + |
332 | + |
333 | +class TimeLimitedToken(storm.base.Storm): |
334 | + """A time limited access token for accessing a private file.""" |
335 | + |
336 | + __storm_table__ = 'TimeLimitedToken' |
337 | + |
338 | + created = UtcDateTimeCol(notNull=True, default=UTC_NOW) |
339 | + path = StringCol(notNull=True) |
340 | + token = StringCol(notNull=True) |
341 | + __storm_primary__ = ("path", "token") |
342 | + |
343 | + def __init__(self, path, token, created=None): |
344 | + """Create a TimeLimitedToken.""" |
345 | + if created is not None: |
346 | + self.created = created |
347 | + self.path = path |
348 | + self.token = token |
349 | + |
350 | + @staticmethod |
351 | + def allocate(url): |
352 | + """Allocate a token for url path in the librarian. |
353 | + |
354 | + :param url: A url bytestring. e.g. |
355 | + https://i123.restricted.launchpad-librarian.net/123/foo.txt |
356 | + Note that the token is generated for 123/foo.txt |
357 | + :return: A url fragment token ready to be attached to the url. |
358 | + e.g. 'a%20token' |
359 | + """ |
360 | + # We use random.random to get a string which varies reasonably, and we |
361 | + # hash it to distribute it widely and get a easily copy and pastable |
362 | + # single string (nice for debugging). The randomness is not a key |
363 | + # factor here: as long as tokens are not guessable, they are hidden by |
364 | + # https, not exposed directly in the API (tokens will be allocated by |
365 | + # the appropriate objects), not by direct access to the |
366 | + # TimeLimitedToken class. |
367 | + baseline = str(random.random()) |
368 | + hashed = md5(baseline).hexdigest() |
369 | + token = hashed |
370 | + store = session_store() |
371 | + path = TimeLimitedToken.url_to_token_path(url) |
372 | + store.add(TimeLimitedToken(path, token)) |
373 | + # The session isn't part of the main transaction model, and in fact it |
374 | + # has autocommit on. The commit here is belts and bracers: after |
375 | + # allocation the external librarian must be able to serve the file |
376 | + # immediately. |
377 | + store.commit() |
378 | + return token |
379 | + |
380 | + @staticmethod |
381 | + def url_to_token_path(url): |
382 | + """Return the token path used for authorising access to url.""" |
383 | + return urlparse(url)[2] |
384 | |
385 | === modified file 'lib/canonical/launchpad/doc/librarian.txt' |
386 | --- lib/canonical/launchpad/doc/librarian.txt 2010-09-04 05:01:17 +0000 |
387 | +++ lib/canonical/launchpad/doc/librarian.txt 2010-09-07 04:07:54 +0000 |
388 | @@ -1,5 +1,31 @@ |
389 | = Librarian Access = |
390 | |
391 | +The librarian is a file storage service for launchpad. Conceptually similar to |
392 | +other file storage API's like S3, it is used to store binary or large content - |
393 | +bug attachments, package builds, images and so on. |
394 | + |
395 | +Content in the librarian can be exposed at different urls. To expose some |
396 | +content use a LibraryFileAlias. Private content is supported as well - for that |
397 | +tokens are added to permit access for a limited time by a client - each time a |
398 | +client attempts to dereference a private LibraryFileAlias a token is emitted. |
399 | + |
400 | += Deployment notes = |
401 | + |
402 | +(These may seem a bit out of place - they are, but they need to be written down |
403 | +somewhere, and the deployment choices inform the implementation choices). |
404 | + |
405 | +The basics are simple: The librarian talks to clients. However restricted file |
406 | +access makes things a little more complex. As the librarian itself doesn't do |
407 | +SSL processing, and we want restricted files to be kept confidential the |
408 | +librarian will need a hint from the SSL front end that SSL was in fact used. |
409 | +The semi standard header Front-End-Https can be used for this if we filter it |
410 | +in incoming requests from clients. |
411 | + |
412 | +== setUp == |
413 | + |
414 | + >>> from canonical.database.sqlbase import session_store |
415 | + >>> from canonical.launchpad.database.librarian import TimeLimitedToken |
416 | + |
417 | == High Level == |
418 | |
419 | >>> from canonical.launchpad.interfaces import ILibraryFileAliasSet |
420 | @@ -58,7 +84,9 @@ |
421 | ... ) is not None |
422 | True |
423 | |
424 | -Librarian also serves the same file through https. |
425 | +Librarian also serves the same file through https, we use this for branding |
426 | +and similar inline-presented objects which would trigger security warnings on |
427 | +https pages otherwise. |
428 | |
429 | >>> re.search( |
430 | ... r'^https://localhost:58000/\d+/text.txt$', alias.https_url |
431 | @@ -83,7 +111,7 @@ |
432 | ... """) |
433 | >>> config.push('test', test_data) |
434 | >>> re.search( |
435 | - ... r'^https://localhost:58000/\d+/text.txt$', alias.getURL() |
436 | + ... r'^https://localhost:58000/\d+/text.txt$', alias.https_url |
437 | ... ) is not None |
438 | True |
439 | |
440 | @@ -183,6 +211,15 @@ |
441 | >>> re.search(r'^http://localhost:58000/\d+/text.txt$', url) is not None |
442 | True |
443 | |
444 | +When secure=True, the returned url has the id as part of the domain name and |
445 | +the protocol is https: |
446 | + |
447 | + >>> expected = 'https://i%d.restricted.localhost:58000/%d/text.txt' % ( |
448 | + ... aid, aid) |
449 | + >>> expected == client.getURLForAlias(aid, secure=True) |
450 | + True |
451 | + |
452 | + |
453 | Librarian reads are logged in the request timeline. |
454 | |
455 | >>> from canonical.lazr.utils import get_current_browser_request |
456 | @@ -465,10 +502,9 @@ |
457 | True |
458 | |
459 | |
460 | -== StreamOrRedirectLibraryFileAliasView == |
461 | +== File views setup == |
462 | |
463 | -This special view allows call sites to stream restricted `LibraryFileAlias` |
464 | -contents directly from launchpad instances. |
465 | +We need some files to test different ways of accessing them. |
466 | |
467 | >>> filename = 'public.txt' |
468 | >>> content = 'PUBLIC' |
469 | @@ -482,11 +518,141 @@ |
470 | ... filename, len(content), StringIO(content), 'text/plain', |
471 | ... NEVER_EXPIRES, restricted=True) |
472 | |
473 | + # Create a new LibraryFileAlias not referencing any LibraryFileContent |
474 | + # record. Such records are considered as being deleted. |
475 | + >>> from canonical.launchpad.database import LibraryFileAlias |
476 | + >>> from canonical.launchpad.webapp.interfaces import ( |
477 | + ... IStoreSelector, MAIN_STORE, MASTER_FLAVOR) |
478 | + |
479 | + >>> store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR) |
480 | + >>> deleted_file = LibraryFileAlias( |
481 | + ... content=None, filename='deleted.txt', |
482 | + ... mimetype='text/plain') |
483 | + >>> ignore = store.add(deleted_file) |
484 | + |
485 | Commit the just-created files. |
486 | |
487 | >>> from canonical.database.sqlbase import commit |
488 | >>> commit() |
489 | |
490 | + >>> deleted_file = getUtility(ILibraryFileAliasSet)[deleted_file.id] |
491 | + >>> print deleted_file.deleted |
492 | + True |
493 | + |
494 | +Clear out existing tokens. |
495 | + |
496 | + >>> _ = session_store().find(TimeLimitedToken).remove() |
497 | + |
498 | +== RedirectPerhapsWithTokenLibraryFileAliasView == |
499 | + |
500 | +This is the eventual name of StreamOrRedirectLibraryFileAliasView once we have |
501 | +migrated over to the token based approach. For now the code for the two things |
502 | +is interleaved: zope's registration system is too static to permit doing it |
503 | +cleanly with separate classes. |
504 | + |
505 | +To enable the behaviour we want to test while its controlled by a feature flag, |
506 | +we must enable a feature flag for it: |
507 | + |
508 | + >>> from lp.services.features.model import FeatureFlag, getFeatureStore |
509 | + >>> ignore = getFeatureStore().add(FeatureFlag( |
510 | + ... scope=u'default', flag=u'publicrestrictedlibrarian', value=u'on', |
511 | + ... priority=1)) |
512 | + >>> transaction.commit() |
513 | + |
514 | +This special view allows granting access to restricted `LibraryFileAlias` |
515 | +objects on the launchpadlibrarian. All requests get redirected, restricted |
516 | +files have an access token allocated. |
517 | + |
518 | +This view implements `IBrowserPublisher` which allows us to use it in |
519 | +traversals. |
520 | + |
521 | + >>> from canonical.launchpad.browser.librarian import ( |
522 | + ... RedirectPerhapsWithTokenLibraryFileAliasView) |
523 | + |
524 | + >>> empty_request = LaunchpadTestRequest() |
525 | + |
526 | +XXX bug=631884 we have to establish the flags object manually for testing. |
527 | + |
528 | + >>> from lp.services.features.webapp import ScopesFromRequest |
529 | + >>> from lp.services.features.flags import FeatureController |
530 | + >>> from lp.services.features import per_thread |
531 | + >>> per_thread.features = FeatureController( |
532 | + ... ScopesFromRequest(empty_request).lookup) |
533 | + |
534 | + >>> restricted_view = RedirectPerhapsWithTokenLibraryFileAliasView( |
535 | + ... restricted_file, empty_request) |
536 | + |
537 | + >>> from zope.publisher.interfaces.browser import IBrowserPublisher |
538 | + >>> verifyObject(IBrowserPublisher, restricted_view) |
539 | + True |
540 | + |
541 | +When the context file is a restricted `LibraryFileAlias`, traversal causes an |
542 | +access token to be allocated and a redirection to https on a unique domain to |
543 | +be issued. |
544 | + |
545 | + >>> view, extra = restricted_view.browserDefault(empty_request) |
546 | + >>> print view |
547 | + <...RedirectionView...> |
548 | + >>> view.target |
549 | + 'https://...restricted.txt?token=...' |
550 | + >>> private_path = TimeLimitedToken.url_to_token_path( |
551 | + ... restricted_file.private_url) |
552 | + >>> view.target.endswith(session_store().find(TimeLimitedToken, |
553 | + ... path=private_path).any().token) |
554 | + True |
555 | + |
556 | +The domain for the target starts with the id of the LibraryFileAlias. |
557 | + |
558 | + >>> view.target.startswith('https://i%d.restricted.' % restricted_file.id) |
559 | + True |
560 | + |
561 | +Otherwise, no token is allocated and a redirection is issued to the http url. |
562 | + |
563 | + >>> public_view = RedirectPerhapsWithTokenLibraryFileAliasView( |
564 | + ... public_file, empty_request) |
565 | + >>> verifyObject(IBrowserPublisher, public_view) |
566 | + True |
567 | + >>> view, extra = public_view.browserDefault(empty_request) |
568 | + >>> print view |
569 | + <...RedirectionView ...> |
570 | + >>> view.target == public_file.http_url |
571 | + True |
572 | + >>> view.target |
573 | + 'http://.../public.txt' |
574 | + |
575 | +Deleted files cannot be handled by the view and it raises an appropriate |
576 | +AssertionError when it gets traversed. |
577 | + |
578 | + >>> deleted_view = RedirectPerhapsWithTokenLibraryFileAliasView( |
579 | + ... deleted_file, empty_request) |
580 | + |
581 | + >>> view, extra = deleted_view.browserDefault(empty_request) |
582 | + Traceback (most recent call last): |
583 | + ... |
584 | + AssertionError: MixedFileAliasView can not operate on deleted librarian |
585 | + files, since their URL is undefined. |
586 | + |
587 | + |
588 | +As RedirectPerhapsWithTokenLibraryFileAliasView is |
589 | +StreamOrRedirectLibraryFileAliasView influence by a feature flag, until the |
590 | +proxy code is removed, we must remove the feature to test the old behaviour. |
591 | + |
592 | + >>> ignore = getFeatureStore().find(FeatureFlag, |
593 | + ... FeatureFlag.flag==u'publicrestrictedlibrarian').remove() |
594 | + >>> transaction.commit() |
595 | + |
596 | +Because the flags code aggressively caches, we have to do a small dance to |
597 | +convince it a new request has started too. |
598 | + |
599 | + >>> empty_request = LaunchpadTestRequest() |
600 | + >>> per_thread.features = FeatureController( |
601 | + ... ScopesFromRequest(empty_request).lookup) |
602 | + |
603 | +== StreamOrRedirectLibraryFileAliasView == |
604 | + |
605 | +This special view allows call sites to stream restricted `LibraryFileAlias` |
606 | +contents directly from launchpad instances. |
607 | + |
608 | This view implements `IBrowserPublisher` which allows us to use it in |
609 | traversals. |
610 | |
611 | @@ -505,6 +671,8 @@ |
612 | traversal chain is finished with the view object itself. |
613 | |
614 | >>> view, extra = restricted_view.browserDefault(empty_request) |
615 | + >>> if view != restricted_view: |
616 | + ... print view |
617 | >>> view == restricted_view |
618 | True |
619 | |
620 | @@ -586,31 +754,14 @@ |
621 | Files in this condition cannot be handled by the view and it raises an |
622 | appropriate AssertionError when it gets traversed. |
623 | |
624 | - # Create a new LibraryFileAlias not referencing any LibraryFileContent |
625 | - # record. Such records are considered as being deleted. |
626 | - >>> from canonical.launchpad.database import LibraryFileAlias |
627 | - >>> from canonical.launchpad.webapp.interfaces import ( |
628 | - ... IStoreSelector, MAIN_STORE, MASTER_FLAVOR) |
629 | - |
630 | - >>> store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR) |
631 | - >>> deleted_file = LibraryFileAlias( |
632 | - ... content=None, filename='deleted.txt', |
633 | - ... mimetype='text/plain') |
634 | - >>> ignore = store.add(deleted_file) |
635 | - >>> store.commit() |
636 | - |
637 | - >>> deleted_file = getUtility(ILibraryFileAliasSet)[deleted_file.id] |
638 | - >>> print deleted_file.deleted |
639 | - True |
640 | - |
641 | >>> deleted_view = StreamOrRedirectLibraryFileAliasView( |
642 | ... deleted_file, empty_request) |
643 | |
644 | >>> view, extra = deleted_view.browserDefault(empty_request) |
645 | Traceback (most recent call last): |
646 | ... |
647 | - AssertionError: StreamOrRedirectLibraryFileAliasView can not |
648 | - operate on deleted librarian files, since their URL is undefined. |
649 | + AssertionError: MixedFileAliasView can not operate on deleted librarian |
650 | + files, since their URL is undefined. |
651 | |
652 | |
653 | == LibraryFileAliasMD5View == |
654 | |
655 | === modified file 'lib/canonical/launchpad/ftests/test_libraryfilealias.py' |
656 | --- lib/canonical/launchpad/ftests/test_libraryfilealias.py 2010-08-20 20:31:18 +0000 |
657 | +++ lib/canonical/launchpad/ftests/test_libraryfilealias.py 2010-09-07 04:07:54 +0000 |
658 | @@ -6,8 +6,8 @@ |
659 | __metaclass__ = type |
660 | |
661 | from cStringIO import StringIO |
662 | + |
663 | import unittest |
664 | - |
665 | import transaction |
666 | from zope.component import getUtility |
667 | |
668 | @@ -45,8 +45,3 @@ |
669 | # the remaining content. If it's reset, the file will be auto-opened |
670 | # and its whole content will be returned. |
671 | self.assertEquals(self.text_content, self.file_alias.read()) |
672 | - |
673 | - |
674 | -def test_suite(): |
675 | - return unittest.TestLoader().loadTestsFromName(__name__) |
676 | - |
677 | |
678 | === modified file 'lib/canonical/launchpad/interfaces/librarian.py' |
679 | --- lib/canonical/launchpad/interfaces/librarian.py 2010-08-20 20:31:18 +0000 |
680 | +++ lib/canonical/launchpad/interfaces/librarian.py 2010-09-07 04:07:54 +0000 |
681 | @@ -80,13 +80,17 @@ |
682 | # byte strings. |
683 | http_url = Attribute(_("The http URL to this file")) |
684 | https_url = Attribute(_("The https URL to this file")) |
685 | + private_url = Attribute(_("The secure URL to this file (private files)")) |
686 | |
687 | def getURL(): |
688 | """Return this file's http or https URL. |
689 | |
690 | + If the file is a restricted file, the private_url will be returned, |
691 | + which is on https and uses unique domains per file alias. |
692 | + |
693 | The generated URL will be https if the use_https config variable is |
694 | set, in order to prevent warnings about insecure objects from |
695 | - happening in some browsers. |
696 | + happening in some browsers (this is used for, e.g., branding). |
697 | |
698 | If config.launchpad.virtual_host.use_https is set, then return the |
699 | https URL. Otherwise return the http URL. |
700 | |
701 | === modified file 'lib/canonical/launchpad/interfaces/lpstorm.py' |
702 | --- lib/canonical/launchpad/interfaces/lpstorm.py 2009-06-25 05:30:52 +0000 |
703 | +++ lib/canonical/launchpad/interfaces/lpstorm.py 2010-09-07 04:07:54 +0000 |
704 | @@ -32,4 +32,3 @@ |
705 | |
706 | class IMasterObject(IDBObject): |
707 | """A Storm database object associated with its master Store.""" |
708 | - |
709 | |
710 | === modified file 'lib/canonical/launchpad/scripts/garbo.py' |
711 | --- lib/canonical/launchpad/scripts/garbo.py 2010-08-20 20:31:18 +0000 |
712 | +++ lib/canonical/launchpad/scripts/garbo.py 2010-09-07 04:07:54 +0000 |
713 | @@ -29,10 +29,14 @@ |
714 | from canonical.database.constants import THIRTY_DAYS_AGO |
715 | from canonical.database.sqlbase import ( |
716 | cursor, |
717 | + session_store, |
718 | sqlvalues, |
719 | ) |
720 | from canonical.launchpad.database.emailaddress import EmailAddress |
721 | -from canonical.launchpad.database.librarian import LibraryFileAlias |
722 | +from canonical.launchpad.database.librarian import ( |
723 | + LibraryFileAlias, |
724 | + TimeLimitedToken, |
725 | + ) |
726 | from canonical.launchpad.database.oauth import OAuthNonce |
727 | from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce |
728 | from canonical.launchpad.interfaces import IMasterStore |
729 | @@ -685,6 +689,43 @@ |
730 | transaction.commit() |
731 | |
732 | |
733 | +class OldTimeLimitedTokenDeleter(TunableLoop): |
734 | + """Delete expired url access tokens from the session DB.""" |
735 | + |
736 | + maximum_chunk_size = 24*60*60 # 24 hours in seconds. |
737 | + |
738 | + def __init__(self, log, abort_time=None): |
739 | + super(OldTimeLimitedTokenDeleter, self).__init__(log, abort_time) |
740 | + self.store = session_store() |
741 | + self._update_oldest() |
742 | + |
743 | + def _update_oldest(self): |
744 | + self.oldest_age = self.store.execute(""" |
745 | + SELECT COALESCE(EXTRACT(EPOCH FROM |
746 | + CURRENT_TIMESTAMP AT TIME ZONE 'UTC' |
747 | + - MIN(created)), 0) |
748 | + FROM TimeLimitedToken |
749 | + """).get_one()[0] |
750 | + |
751 | + def isDone(self): |
752 | + return self.oldest_age <= ONE_DAY_IN_SECONDS |
753 | + |
754 | + def __call__(self, chunk_size): |
755 | + self.oldest_age = max( |
756 | + ONE_DAY_IN_SECONDS, self.oldest_age - chunk_size) |
757 | + |
758 | + self.log.debug( |
759 | + "Removed TimeLimitedToken rows older than %d seconds" |
760 | + % self.oldest_age) |
761 | + self.store.find( |
762 | + TimeLimitedToken, |
763 | + TimeLimitedToken.created < SQL( |
764 | + "CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval '%d seconds'" |
765 | + % ONE_DAY_IN_SECONDS)).remove() |
766 | + transaction.commit() |
767 | + self._update_oldest() |
768 | + |
769 | + |
770 | class SuggestiveTemplatesCacheUpdater(TunableLoop): |
771 | """Refresh the SuggestivePOTemplate cache. |
772 | |
773 | @@ -806,13 +847,14 @@ |
774 | class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector): |
775 | script_name = 'garbo-daily' |
776 | tunable_loops = [ |
777 | + BranchJobPruner, |
778 | + BugNotificationPruner, |
779 | + BugWatchActivityPruner, |
780 | CodeImportResultPruner, |
781 | - RevisionAuthorEmailLinker, |
782 | HWSubmissionEmailLinker, |
783 | - BugNotificationPruner, |
784 | - BranchJobPruner, |
785 | - BugWatchActivityPruner, |
786 | ObsoleteBugAttachmentDeleter, |
787 | + OldTimeLimitedTokenDeleter, |
788 | + RevisionAuthorEmailLinker, |
789 | SuggestiveTemplatesCacheUpdater, |
790 | ] |
791 | experimental_tunable_loops = [ |
792 | |
793 | === modified file 'lib/canonical/launchpad/scripts/tests/test_garbo.py' |
794 | --- lib/canonical/launchpad/scripts/tests/test_garbo.py 2010-08-20 20:31:18 +0000 |
795 | +++ lib/canonical/launchpad/scripts/tests/test_garbo.py 2010-09-07 04:07:54 +0000 |
796 | @@ -23,11 +23,12 @@ |
797 | from zope.security.proxy import removeSecurityProxy |
798 | |
799 | from canonical.config import config |
800 | +from canonical.database import sqlbase |
801 | from canonical.database.constants import ( |
802 | THIRTY_DAYS_AGO, |
803 | UTC_NOW, |
804 | ) |
805 | -from canonical.database.sqlbase import quote |
806 | +from canonical.launchpad.database.librarian import TimeLimitedToken |
807 | from canonical.launchpad.database.message import Message |
808 | from canonical.launchpad.database.oauth import OAuthNonce |
809 | from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce |
810 | @@ -561,6 +562,26 @@ |
811 | LaunchpadZopelessLayer.switchDbUser('testadmin') |
812 | self.assertEqual(bug.attachments.count(), 0) |
813 | |
814 | + def test_TimeLimitedTokenPruner(self): |
815 | + # Ensure there are no tokens |
816 | + store = sqlbase.session_store() |
817 | + map(store.remove, store.find(TimeLimitedToken)) |
818 | + store.flush() |
819 | + self.assertEqual(0, len(list(store.find(TimeLimitedToken, |
820 | + path="sample path")))) |
821 | + # One to clean and one to keep |
822 | + store.add(TimeLimitedToken(path="sample path", token="foo", |
823 | + created=datetime(2008, 01, 01, tzinfo=UTC))) |
824 | + store.add(TimeLimitedToken(path="sample path", token="bar")), |
825 | + store.commit() |
826 | + self.assertEqual(2, len(list(store.find(TimeLimitedToken, |
827 | + path="sample path")))) |
828 | + self.runDaily() |
829 | + self.assertEqual(0, len(list(store.find(TimeLimitedToken, |
830 | + path="sample path", token="foo")))) |
831 | + self.assertEqual(1, len(list(store.find(TimeLimitedToken, |
832 | + path="sample path", token="bar")))) |
833 | + |
834 | def test_CacheSuggestivePOTemplates(self): |
835 | LaunchpadZopelessLayer.switchDbUser('testadmin') |
836 | template = self.factory.makePOTemplate() |
837 | @@ -571,6 +592,6 @@ |
838 | SELECT count(*) |
839 | FROM SuggestivePOTemplate |
840 | WHERE potemplate = %s |
841 | - """ % quote(template.id)).get_one() |
842 | + """ % sqlbase.quote(template.id)).get_one() |
843 | |
844 | self.assertEqual(1, count) |
845 | |
846 | === modified file 'lib/canonical/launchpad/utilities/looptuner.py' |
847 | --- lib/canonical/launchpad/utilities/looptuner.py 2010-08-20 20:31:18 +0000 |
848 | +++ lib/canonical/launchpad/utilities/looptuner.py 2010-09-07 04:07:54 +0000 |
849 | @@ -314,6 +314,10 @@ |
850 | self.log = log |
851 | self.abort_time = abort_time |
852 | |
853 | + def isDone(self): |
854 | + """Return True when the TunableLoop is complete.""" |
855 | + raise NotImplementedError(self.isDone) |
856 | + |
857 | def run(self): |
858 | assert self.maximum_chunk_size is not None, ( |
859 | "Did not override maximum_chunk_size.") |
860 | |
861 | === modified file 'lib/canonical/launchpad/webapp/session.py' |
862 | --- lib/canonical/launchpad/webapp/session.py 2010-08-20 20:31:18 +0000 |
863 | +++ lib/canonical/launchpad/webapp/session.py 2010-09-07 04:07:54 +0000 |
864 | @@ -13,6 +13,7 @@ |
865 | from zope.session.http import CookieClientIdManager |
866 | |
867 | from canonical.config import config |
868 | +from canonical.database.sqlbase import session_store |
869 | |
870 | |
871 | SECONDS = 1 |
872 | @@ -77,7 +78,7 @@ |
873 | # Secret is looked up here rather than in __init__, because |
874 | # we can't be sure the database connections are setup at that point. |
875 | if self._secret is None: |
876 | - store = getUtility(IZStorm).get('session', 'launchpad-session:') |
877 | + store = session_store() |
878 | result = store.execute("SELECT secret FROM secret") |
879 | self._secret = result.get_one()[0] |
880 | return self._secret |
881 | |
882 | === modified file 'lib/canonical/launchpad/zcml/librarian.zcml' |
883 | --- lib/canonical/launchpad/zcml/librarian.zcml 2010-07-06 16:12:46 +0000 |
884 | +++ lib/canonical/launchpad/zcml/librarian.zcml 2010-09-07 04:07:54 +0000 |
885 | @@ -55,6 +55,11 @@ |
886 | <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" /> |
887 | </class> |
888 | |
889 | + <class class="canonical.launchpad.browser.librarian.SafeStreamOrRedirectLibraryFileAliasView"> |
890 | + <allow attributes="__call__" /> |
891 | + <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" /> |
892 | + </class> |
893 | + |
894 | <adapter |
895 | factory="canonical.launchpad.database.librarian.LibraryFileAliasWithParent" |
896 | provides="canonical.launchpad.interfaces.librarian.ILibraryFileAliasWithParent" |
897 | |
898 | === modified file 'lib/canonical/librarian/client.py' |
899 | --- lib/canonical/librarian/client.py 2010-09-04 05:01:17 +0000 |
900 | +++ lib/canonical/librarian/client.py 2010-09-07 04:07:54 +0000 |
901 | @@ -23,7 +23,11 @@ |
902 | |
903 | from select import select |
904 | from socket import SOCK_STREAM, AF_INET |
905 | -from urlparse import urljoin |
906 | +from urlparse import ( |
907 | + urlparse, |
908 | + urljoin, |
909 | + urlunparse, |
910 | + ) |
911 | |
912 | from storm.store import Store |
913 | from zope.interface import implements |
914 | @@ -291,16 +295,17 @@ |
915 | # raise DownloadFailed, 'Incomplete response' |
916 | # return paths |
917 | |
918 | - def _getPathForAlias(self, aliasID): |
919 | + def _getPathForAlias(self, aliasID, secure=False): |
920 | """Returns the path inside the librarian to talk about the given |
921 | alias. |
922 | |
923 | :param aliasID: A unique ID for the alias |
924 | - |
925 | + :param secure: Controls the behaviour when looking up restricted |
926 | + files. If False restricted files are only permitted when |
927 | + self.restricted is True. See getURLForAlias. |
928 | :returns: String path, url-escaped. Unicode is UTF-8 encoded before |
929 | url-escaping, as described in section 2.2.5 of RFC 2718. |
930 | None if the file has been deleted. |
931 | - |
932 | :raises: DownloadFailed if the alias is invalid |
933 | """ |
934 | from canonical.launchpad.database import LibraryFileAlias |
935 | @@ -311,7 +316,7 @@ |
936 | lfa = LibraryFileAlias.get(aliasID) |
937 | except SQLObjectNotFound: |
938 | raise DownloadFailed('Alias %d not found' % aliasID) |
939 | - if self.restricted != lfa.restricted: |
940 | + if not secure and self.restricted != lfa.restricted: |
941 | raise DownloadFailed( |
942 | 'Alias %d cannot be downloaded from this client.' % aliasID) |
943 | if lfa.deleted: |
944 | @@ -319,18 +324,41 @@ |
945 | return get_libraryfilealias_download_path( |
946 | aliasID, lfa.filename.encode('utf-8')) |
947 | |
948 | - def getURLForAlias(self, aliasID): |
949 | + def getURLForAlias(self, aliasID, secure=False): |
950 | """Returns the url for talking to the librarian about the given |
951 | alias. |
952 | |
953 | :param aliasID: A unique ID for the alias |
954 | - |
955 | + :param secure: If true generate https urls on unique domains for |
956 | + security. |
957 | :returns: String URL, or None if the file has expired and been deleted. |
958 | """ |
959 | - path = self._getPathForAlias(aliasID) |
960 | + # Note that the path is the same for both secure and insecure URLs - |
961 | + # this is deliberate: the server doesn't need to know about the original |
962 | + # Host the client provides, testing is easier as we don't need wildcard |
963 | + # https environments on dev machines. |
964 | + path = self._getPathForAlias(aliasID, secure=secure) |
965 | if path is None: |
966 | return None |
967 | - base = self.download_url |
968 | + if not secure: |
969 | + base = self.download_url |
970 | + else: |
971 | + # Secure url generation is the same for both restricted and |
972 | + # unrestricted files aliases : it is used to give web clients (not |
973 | + # appservers) a url to use to access a file which is either |
974 | + # restricted (and so they will also need a TimeLimitedToken) or |
975 | + # is suspected hostile (and so it should be isolated on its own |
976 | + # domain). Note that only the former is currently used in LP. |
977 | + # The algorithm is: |
978 | + # parse the url |
979 | + download_url = config.librarian.download_url |
980 | + parsed = list(urlparse(download_url)) |
981 | + # Force the scheme to https |
982 | + parsed[0] = 'https' |
983 | + # Insert the alias id (which is a unique key, thus unique) in the |
984 | + # netloc |
985 | + parsed[1] = ('i%d.restricted.' % aliasID) + parsed[1] |
986 | + base = urlunparse(parsed) |
987 | return urljoin(base, path) |
988 | |
989 | def _getURLForDownload(self, aliasID): |
990 | |
991 | === modified file 'lib/canonical/librarian/db.py' |
992 | --- lib/canonical/librarian/db.py 2009-11-24 15:36:44 +0000 |
993 | +++ lib/canonical/librarian/db.py 2010-09-07 04:07:54 +0000 |
994 | @@ -13,12 +13,19 @@ |
995 | from psycopg2.extensions import TransactionRollbackError |
996 | from sqlobject.sqlbuilder import AND |
997 | from storm.exceptions import DisconnectionError, IntegrityError |
998 | +from storm.expr import SQL |
999 | import transaction |
1000 | from twisted.python.util import mergeFunctionMetadata |
1001 | |
1002 | -from canonical.database.sqlbase import reset_store |
1003 | +from canonical.database.sqlbase import ( |
1004 | + reset_store, |
1005 | + session_store, |
1006 | + ) |
1007 | from canonical.launchpad.database.librarian import ( |
1008 | - LibraryFileContent, LibraryFileAlias) |
1009 | + LibraryFileAlias, |
1010 | + LibraryFileContent, |
1011 | + TimeLimitedToken, |
1012 | + ) |
1013 | |
1014 | |
1015 | RETRY_ATTEMPTS = 3 |
1016 | @@ -98,19 +105,40 @@ |
1017 | def lookupBySHA1(self, digest): |
1018 | return [fc.id for fc in LibraryFileContent.selectBy(sha1=digest)] |
1019 | |
1020 | - def getAlias(self, aliasid): |
1021 | + def getAlias(self, aliasid, token, path): |
1022 | """Returns a LibraryFileAlias, or raises LookupError. |
1023 | |
1024 | A LookupError is raised if no record with the given ID exists |
1025 | or if not related LibraryFileContent exists. |
1026 | + |
1027 | + :param token: The token for the file. If None no token is present. |
1028 | + When a token is supplied, it is looked up with path. |
1029 | + :param path: The path the request is for, unused unless a token |
1030 | + is supplied; when supplied it must match the token. The |
1031 | + value of path is expected to be that from a twisted request.args |
1032 | + e.g. /foo/bar. |
1033 | """ |
1034 | + restricted = self.restricted |
1035 | + if token and path: |
1036 | + # with a token and a path we may be able to serve restricted files |
1037 | + # on the public port. |
1038 | + store = session_store() |
1039 | + token_found = store.find(TimeLimitedToken, |
1040 | + SQL("age(created) < interval '1 day'"), |
1041 | + TimeLimitedToken.token==token, |
1042 | + TimeLimitedToken.path==path).is_empty() |
1043 | + store.reset() |
1044 | + if token_found: |
1045 | + raise LookupError("Token stale/pruned/path mismatch") |
1046 | + else: |
1047 | + restricted = True |
1048 | alias = LibraryFileAlias.selectOne(AND( |
1049 | LibraryFileAlias.q.id==aliasid, |
1050 | LibraryFileAlias.q.contentID==LibraryFileContent.q.id, |
1051 | - LibraryFileAlias.q.restricted==self.restricted, |
1052 | + LibraryFileAlias.q.restricted==restricted, |
1053 | )) |
1054 | if alias is None: |
1055 | - raise LookupError |
1056 | + raise LookupError("No file alias with LibraryFileContent") |
1057 | return alias |
1058 | |
1059 | def getAliases(self, fileid): |
1060 | |
1061 | === modified file 'lib/canonical/librarian/ftests/test_db.py' |
1062 | --- lib/canonical/librarian/ftests/test_db.py 2009-11-24 16:54:45 +0000 |
1063 | +++ lib/canonical/librarian/ftests/test_db.py 2010-09-07 04:07:54 +0000 |
1064 | @@ -39,7 +39,7 @@ |
1065 | sorted(library.lookupBySHA1('deadbeef'))) |
1066 | |
1067 | aliasID = library.addAlias(fileID, 'file1', 'text/unknown') |
1068 | - alias = library.getAlias(aliasID) |
1069 | + alias = library.getAlias(aliasID, None, '/') |
1070 | self.assertEqual('file1', alias.filename) |
1071 | self.assertEqual('text/unknown', alias.mimetype) |
1072 | |
1073 | @@ -65,42 +65,44 @@ |
1074 | # Library.getAlias() returns the LibrarayFileAlias for a given |
1075 | # LibrarayFileAlias ID. |
1076 | library = db.Library(restricted=False) |
1077 | - alias = library.getAlias(1) |
1078 | + alias = library.getAlias(1, None, '/') |
1079 | self.assertEqual(1, alias.id) |
1080 | |
1081 | def test_getAlias_no_such_record(self): |
1082 | # Library.getAlias() raises a LookupError, if no record with |
1083 | # the given ID exists. |
1084 | library = db.Library(restricted=False) |
1085 | - self.assertRaises(LookupError, library.getAlias, -1) |
1086 | + self.assertRaises(LookupError, library.getAlias, -1, None, '/') |
1087 | |
1088 | def test_getAlias_content_is_null(self): |
1089 | # Library.getAlias() raises a LookupError, if no content |
1090 | # record for the given alias exists. |
1091 | library = db.Library(restricted=False) |
1092 | - alias = library.getAlias(1) |
1093 | + alias = library.getAlias(1, None, '/') |
1094 | alias.content = None |
1095 | - self.assertRaises(LookupError, library.getAlias, 1) |
1096 | + self.assertRaises(LookupError, library.getAlias, 1, None, '/') |
1097 | |
1098 | def test_getAlias_content_is_none(self): |
1099 | # Library.getAlias() raises a LookupError, if the matching |
1100 | # record does not reference any LibraryFileContent record. |
1101 | library = db.Library(restricted=False) |
1102 | - alias = library.getAlias(1) |
1103 | + alias = library.getAlias(1, None, '/') |
1104 | alias.content = None |
1105 | - self.assertRaises(LookupError, library.getAlias, 1) |
1106 | + self.assertRaises(LookupError, library.getAlias, 1, None, '/') |
1107 | |
1108 | def test_getAlias_content_wrong_library(self): |
1109 | # Library.getAlias() raises a LookupError, if a restricted |
1110 | # library looks up a unrestricted LibraryFileAlias and |
1111 | # vice versa. |
1112 | restricted_library = db.Library(restricted=True) |
1113 | - self.assertRaises(LookupError, restricted_library.getAlias, 1) |
1114 | + self.assertRaises( |
1115 | + LookupError, restricted_library.getAlias, 1, None, '/') |
1116 | |
1117 | unrestricted_library = db.Library(restricted=False) |
1118 | - alias = unrestricted_library.getAlias(1) |
1119 | + alias = unrestricted_library.getAlias(1, None, '/') |
1120 | alias.restricted = True |
1121 | - self.assertRaises(LookupError, unrestricted_library.getAlias, 1) |
1122 | + self.assertRaises( |
1123 | + LookupError, unrestricted_library.getAlias, 1, None, '/') |
1124 | |
1125 | def test_getAliases(self): |
1126 | # Library.getAliases() returns a sequence |
1127 | @@ -119,7 +121,7 @@ |
1128 | # Library.getAliases() does not return records which do not |
1129 | # reference any LibraryFileContent record. |
1130 | library = db.Library(restricted=False) |
1131 | - alias = library.getAlias(1) |
1132 | + alias = library.getAlias(1, None, '/') |
1133 | alias.content = None |
1134 | aliases = library.getAliases(1) |
1135 | expected_aliases = [ |
1136 | @@ -132,7 +134,7 @@ |
1137 | # LibrarayFileAlias records when called from a unrestricted |
1138 | # library and vice versa. |
1139 | unrestricted_library = db.Library(restricted=False) |
1140 | - alias = unrestricted_library.getAlias(1) |
1141 | + alias = unrestricted_library.getAlias(1, None, '/') |
1142 | alias.restricted = True |
1143 | |
1144 | aliases = unrestricted_library.getAliases(1) |
1145 | |
1146 | === modified file 'lib/canonical/librarian/ftests/test_storage.py' |
1147 | --- lib/canonical/librarian/ftests/test_storage.py 2010-02-09 00:17:40 +0000 |
1148 | +++ lib/canonical/librarian/ftests/test_storage.py 2010-09-07 04:07:54 +0000 |
1149 | @@ -71,7 +71,7 @@ |
1150 | fileid, aliasid = newfile.store() |
1151 | |
1152 | # Check that its alias has the right mimetype |
1153 | - fa = self.storage.getFileAlias(aliasid) |
1154 | + fa = self.storage.getFileAlias(aliasid, None, '/') |
1155 | self.assertEqual('text/unknown', fa.mimetype) |
1156 | |
1157 | # Re-add the same file, with the same name and mimetype... |
1158 | @@ -81,7 +81,8 @@ |
1159 | fileid2, aliasid2 = newfile2.store() |
1160 | |
1161 | # Verify that we didn't get back the same alias ID |
1162 | - self.assertNotEqual(fa.id, self.storage.getFileAlias(aliasid2).id) |
1163 | + self.assertNotEqual(fa.id, |
1164 | + self.storage.getFileAlias(aliasid2, None, '/').id) |
1165 | |
1166 | def test_clientProvidedDuplicateIDs(self): |
1167 | # This test checks the new behaviour specified by LibrarianTransactions |
1168 | |
1169 | === modified file 'lib/canonical/librarian/ftests/test_web.py' |
1170 | --- lib/canonical/librarian/ftests/test_web.py 2009-11-24 15:36:44 +0000 |
1171 | +++ lib/canonical/librarian/ftests/test_web.py 2010-09-07 04:07:54 +0000 |
1172 | @@ -3,17 +3,28 @@ |
1173 | |
1174 | from cStringIO import StringIO |
1175 | from datetime import datetime |
1176 | +import httplib |
1177 | import unittest |
1178 | from urllib2 import urlopen, HTTPError |
1179 | +from urlparse import urlparse |
1180 | |
1181 | import pytz |
1182 | - |
1183 | +from storm.expr import SQL |
1184 | import transaction |
1185 | from zope.component import getUtility |
1186 | |
1187 | from canonical.config import config |
1188 | -from canonical.database.sqlbase import flush_database_updates, cursor |
1189 | -from canonical.librarian.client import LibrarianClient |
1190 | +from canonical.database.sqlbase import ( |
1191 | + cursor, |
1192 | + flush_database_updates, |
1193 | + session_store, |
1194 | + ) |
1195 | +from canonical.launchpad.database.librarian import TimeLimitedToken |
1196 | +from canonical.librarian.client import ( |
1197 | + get_libraryfilealias_download_path, |
1198 | + LibrarianClient, |
1199 | + RestrictedLibrarianClient, |
1200 | + ) |
1201 | from canonical.librarian.interfaces import DownloadFailed |
1202 | from canonical.launchpad.database import LibraryFileAlias |
1203 | from canonical.launchpad.interfaces import IMasterStore |
1204 | @@ -57,12 +68,7 @@ |
1205 | # librarian allow access to files that don't exist in the DB |
1206 | # and spitting them out with an 'unknown' mime-type |
1207 | # -- StuartBishop) |
1208 | - try: |
1209 | - urlopen(url) |
1210 | - self.fail('Should have raised a 404') |
1211 | - except HTTPError, x: |
1212 | - self.failUnlessEqual(x.code, 404) |
1213 | - |
1214 | + self.require404(url) |
1215 | self.commit() |
1216 | |
1217 | # Make sure we can download it using the API |
1218 | @@ -148,11 +154,7 @@ |
1219 | config.librarian.download_port, |
1220 | aid, filename |
1221 | ) |
1222 | - try: |
1223 | - urlopen(self._makeURL(aid, 'different.txt')) |
1224 | - self.fail('404 not raised') |
1225 | - except HTTPError, x: |
1226 | - self.failUnlessEqual(x.code, 404) |
1227 | + self.require404(self._makeURL(aid, 'different.txt')) |
1228 | |
1229 | def _makeURL(self, aliasID, filename): |
1230 | host = config.librarian.download_host |
1231 | @@ -172,18 +174,9 @@ |
1232 | self.failUnlessEqual(url, self._makeURL(aid, filename)) |
1233 | |
1234 | # Change the aliasid and assert we get a 404 |
1235 | - try: |
1236 | - urlopen(self._makeURL(aid+1, filename)) |
1237 | - self.fail('404 not raised') |
1238 | - except HTTPError, x: |
1239 | - self.failUnlessEqual(x.code, 404) |
1240 | - |
1241 | + self.require404(self._makeURL(aid+1, filename)) |
1242 | # Change the filename and assert we get a 404 |
1243 | - try: |
1244 | - urlopen(self._makeURL(aid, 'different.txt')) |
1245 | - self.fail('404 not raised') |
1246 | - except HTTPError, x: |
1247 | - self.failUnlessEqual(x.code, 404) |
1248 | + self.require404(self._makeURL(aid, 'different.txt')) |
1249 | |
1250 | def test_duplicateuploads(self): |
1251 | client = LibrarianClient() |
1252 | @@ -237,6 +230,145 @@ |
1253 | self.failUnlessEqual( |
1254 | last_modified_header, 'Tue, 30 Jan 2001 13:45:59 GMT') |
1255 | |
1256 | + def get_restricted_file_and_public_url(self): |
1257 | + # Use a regular LibrarianClient to ensure we speak to the nonrestricted |
1258 | + # port on the librarian which is where secured restricted files are |
1259 | + # served from. |
1260 | + client = LibrarianClient() |
1261 | + fileAlias = client.addFile('sample', 12, StringIO('a'*12), |
1262 | + contentType='text/plain') |
1263 | + # Note: We're deliberately using the wrong url here: we should be |
1264 | + # passing secure=True to getURLForAlias, but to use the returned URL we |
1265 | + # would need a wildcard DNS facility patched into urlopen; instead |
1266 | + # we use the *deliberate* choice of having the path of secure and insecure |
1267 | + # urls be the same, so that we can test it: the server code doesn't need |
1268 | + # to know about the fancy wildcard domains. |
1269 | + url = client.getURLForAlias(fileAlias) |
1270 | + # Now that we have a url which talks to the public librarian, make the |
1271 | + # file restricted. |
1272 | + IMasterStore(LibraryFileAlias).find(LibraryFileAlias, |
1273 | + LibraryFileAlias.id==fileAlias).set( |
1274 | + LibraryFileAlias.restricted==True) |
1275 | + self.commit() |
1276 | + return fileAlias, url |
1277 | + |
1278 | + def test_restricted_subdomain_must_match_file_alias(self): |
1279 | + # IFF there is a .restricted. in the host, then the library file alias |
1280 | + # in the subdomain must match that in the path. |
1281 | + client = LibrarianClient() |
1282 | + fileAlias = client.addFile('sample', 12, StringIO('a'*12), |
1283 | + contentType='text/plain') |
1284 | + fileAlias2 = client.addFile('sample', 12, StringIO('b'*12), |
1285 | + contentType='text/plain') |
1286 | + self.commit() |
1287 | + url = client.getURLForAlias(fileAlias) |
1288 | + download_host = urlparse(config.librarian.download_url)[1] |
1289 | + if ':' in download_host: |
1290 | + download_host = download_host[:download_host.find(':')] |
1291 | + template_host = 'i%%d.restricted.%s' % download_host |
1292 | + path = get_libraryfilealias_download_path(fileAlias, 'sample') |
1293 | + # The basic URL must work. |
1294 | + urlopen(url) |
1295 | + # Use the network level protocol because DNS resolution won't work |
1296 | + # here (no wildcard support) |
1297 | + connection = httplib.HTTPConnection( |
1298 | + config.librarian.download_host, |
1299 | + config.librarian.download_port) |
1300 | + # A valid subdomain based URL must work. |
1301 | + good_host = template_host % fileAlias |
1302 | + connection.request("GET", path, headers={'Host': good_host}) |
1303 | + response = connection.getresponse() |
1304 | + response.read() |
1305 | + self.assertEqual(200, response.status) |
1306 | + # A subdomain based URL trying to put fileAlias into the restricted |
1307 | + # domain of fileAlias2 must not work. |
1308 | + hostile_host = template_host % fileAlias2 |
1309 | + connection.request("GET", path, headers={'Host': hostile_host}) |
1310 | + response = connection.getresponse() |
1311 | + response.read() |
1312 | + self.assertEqual(404, response.status) |
1313 | + # A subdomain which matches the LFA but is nested under one that |
1314 | + # doesn't is also treated as hostile. |
1315 | + nested_host = 'i%d.restricted.i%d.restricted.%s' % ( |
1316 | + fileAlias, fileAlias2, download_host) |
1317 | + connection.request("GET", path, headers={'Host': nested_host}) |
1318 | + response = connection.getresponse() |
1319 | + response.read() |
1320 | + self.assertEqual(404, response.status) |
1321 | + |
1322 | + def test_restricted_no_token(self): |
1323 | + fileAlias, url = self.get_restricted_file_and_public_url() |
1324 | + # The file should not be able to be opened - we haven't allocated a |
1325 | + # token. When the token is wrong or stale a 404 is given (to avoid |
1326 | + # disclosure about what content we hold. Alternatively a 401 could be |
1327 | + # given (as long as we give a 401 when the file is missing as well - |
1328 | + # but that requires some more complex changes in the deployment |
1329 | + # infrastructure to permit more backend knowledge of the frontend |
1330 | + # request. |
1331 | + self.require404(url) |
1332 | + |
1333 | + def test_restricted_made_up_token(self): |
1334 | + fileAlias, url = self.get_restricted_file_and_public_url() |
1335 | + # The file should not be able to be opened - the token supplied |
1336 | + # is not one we issued. |
1337 | + self.require404(url + '?token=haxx0r') |
1338 | + |
1339 | + def test_restricted_with_token(self): |
1340 | + fileAlias, url = self.get_restricted_file_and_public_url() |
1341 | + # We have the base url for a restricted file; grant access to it |
1342 | + # for a short time. |
1343 | + token = TimeLimitedToken.allocate(url) |
1344 | + url = url + "?token=%s" % token |
1345 | + # Now we should be able to access the file. |
1346 | + fileObj = urlopen(url) |
1347 | + try: |
1348 | + self.assertEqual("a"*12, fileObj.read()) |
1349 | + finally: |
1350 | + fileObj.close() |
1351 | + |
1352 | + def test_restricted_with_expired_token(self): |
1353 | + fileAlias, url = self.get_restricted_file_and_public_url() |
1354 | + # We have the base url for a restricted file; grant access to it |
1355 | + # for a short time. |
1356 | + token = TimeLimitedToken.allocate(url) |
1357 | + # But time has passed |
1358 | + store = session_store() |
1359 | + tokens = store.find(TimeLimitedToken, TimeLimitedToken.token==token) |
1360 | + tokens.set( |
1361 | + TimeLimitedToken.created==SQL("created - interval '1 week'")) |
1362 | + url = url + "?token=%s" % token |
1363 | + # Now, as per test_restricted_no_token we should get a 404. |
1364 | + self.require404(url) |
1365 | + |
1366 | + def test_restricted_file_headers(self): |
1367 | + fileAlias, url = self.get_restricted_file_and_public_url() |
1368 | + token = TimeLimitedToken.allocate(url) |
1369 | + url = url + "?token=%s" % token |
1370 | + # Change the date_created to a known value for testing. |
1371 | + file_alias = IMasterStore(LibraryFileAlias).get( |
1372 | + LibraryFileAlias, fileAlias) |
1373 | + file_alias.date_created = datetime( |
1374 | + 2001, 01, 30, 13, 45, 59, tzinfo=pytz.utc) |
1375 | + # Commit the update. |
1376 | + self.commit() |
1377 | + # Fetch the file via HTTP, recording the interesting headers |
1378 | + result = urlopen(url) |
1379 | + last_modified_header = result.info()['Last-Modified'] |
1380 | + cache_control_header = result.info()['Cache-Control'] |
1381 | + # No caching for restricted files. |
1382 | + self.failUnlessEqual(cache_control_header, 'max-age=0, private') |
1383 | + # And we should have a correct Last-Modified header too. |
1384 | + self.failUnlessEqual( |
1385 | + last_modified_header, 'Tue, 30 Jan 2001 13:45:59 GMT') |
1386 | + # Perhaps we should also set Expires to the Last-Modified. |
1387 | + |
1388 | + def require404(self, url): |
1389 | + try: |
1390 | + urlopen(url) |
1391 | + self.fail('404 not raised') |
1392 | + except HTTPError, e: |
1393 | + self.failUnlessEqual(e.code, 404) |
1394 | + |
1395 | |
1396 | class LibrarianZopelessWebTestCase(LibrarianWebTestCase): |
1397 | layer = LaunchpadZopelessLayer |
1398 | |
1399 | === modified file 'lib/canonical/librarian/interfaces.py' |
1400 | --- lib/canonical/librarian/interfaces.py 2010-08-17 21:05:47 +0000 |
1401 | +++ lib/canonical/librarian/interfaces.py 2010-09-07 04:07:54 +0000 |
1402 | @@ -89,8 +89,19 @@ |
1403 | class IFileDownloadClient(Interface): |
1404 | """Download API for the Librarian client.""" |
1405 | |
1406 | - def getURLForAlias(aliasID): |
1407 | - """Returns the URL to the given file""" |
1408 | + def getURLForAlias(aliasID, secure=False): |
1409 | + """Returns the URL to the given file. |
1410 | + |
1411 | + :param aliasID: The LibraryFileAlias for the file to get. A DB lookup |
1412 | + will be done for this - if many are to be calculated, eagar loading |
1413 | + is recommended. |
1414 | + :param secure: If False, generate an http url on the main librarian |
1415 | + domain. |
1416 | + If True, generate an https url on a subdomain |
1417 | + i$aliasID.restricted.$main_librarian_domain. |
1418 | + Note that when a secure URL is generated, a TimeLimitedToken must |
1419 | + separately be obtained and combined with the URL to use it. |
1420 | + """ |
1421 | |
1422 | def getFileByAlias(aliasID, timeout=LIBRARIAN_SERVER_DEFAULT_TIMEOUT): |
1423 | """Returns a file-like object to read the file contents from. |
1424 | |
1425 | === modified file 'lib/canonical/librarian/storage.py' |
1426 | --- lib/canonical/librarian/storage.py 2010-08-16 18:50:40 +0000 |
1427 | +++ lib/canonical/librarian/storage.py 2010-09-07 04:07:54 +0000 |
1428 | @@ -72,8 +72,8 @@ |
1429 | def startAddFile(self, filename, size): |
1430 | return LibraryFileUpload(self, filename, size) |
1431 | |
1432 | - def getFileAlias(self, aliasid): |
1433 | - return self.library.getAlias(aliasid) |
1434 | + def getFileAlias(self, aliasid, token, path): |
1435 | + return self.library.getAlias(aliasid, token, path) |
1436 | |
1437 | |
1438 | class LibraryFileUpload(object): |
1439 | |
1440 | === modified file 'lib/canonical/librarian/web.py' |
1441 | --- lib/canonical/librarian/web.py 2010-08-17 16:33:33 +0000 |
1442 | +++ lib/canonical/librarian/web.py 2010-09-07 04:07:54 +0000 |
1443 | @@ -5,10 +5,12 @@ |
1444 | |
1445 | from datetime import datetime |
1446 | import time |
1447 | +from urlparse import urlparse |
1448 | |
1449 | from twisted.web import resource, static, util, server, proxy |
1450 | from twisted.internet.threads import deferToThread |
1451 | |
1452 | +from canonical.config import config |
1453 | from canonical.librarian.client import url_path_quote |
1454 | from canonical.librarian.db import read_transaction, write_transaction |
1455 | from canonical.librarian.utils import guess_librarian_encoding |
1456 | @@ -62,11 +64,11 @@ |
1457 | self.upstreamPort = upstreamPort |
1458 | |
1459 | def getChild(self, filename, request): |
1460 | - |
1461 | # If we still have another component of the path, then we have |
1462 | # an old URL that encodes the content ID. We want to keep supporting |
1463 | # these, so we just ignore the content id that is currently in |
1464 | - # self.aliasID and extract the real one from the URL. |
1465 | + # self.aliasID and extract the real one from the URL. Note that |
1466 | + # tokens do not work with the old URL style: they are URL specific. |
1467 | if len(request.postpath) == 1: |
1468 | try: |
1469 | self.aliasID = int(filename) |
1470 | @@ -74,7 +76,27 @@ |
1471 | return fourOhFour |
1472 | filename = request.postpath[0] |
1473 | |
1474 | - deferred = deferToThread(self._getFileAlias, self.aliasID) |
1475 | + # IFF the request has a .restricted. subdomain, ensure there is a |
1476 | + # alias id in the right most subdomain, and that it matches |
1477 | + # self.aliasIDd, And that the host precisely matches what we generate |
1478 | + # (specifically to stop people putting a good prefix to the left of an |
1479 | + # attacking one). |
1480 | + hostname = request.getRequestHostname() |
1481 | + if '.restricted.' in hostname: |
1482 | + # Configs can change without warning: evaluate every time. |
1483 | + download_url = config.librarian.download_url |
1484 | + parsed = list(urlparse(download_url)) |
1485 | + netloc = parsed[1] |
1486 | + # Strip port if present |
1487 | + if netloc.find(':') > -1: |
1488 | + netloc = netloc[:netloc.find(':')] |
1489 | + expected_hostname = 'i%d.restricted.%s' % (self.aliasID, netloc) |
1490 | + if expected_hostname != hostname: |
1491 | + return fourOhFour |
1492 | + |
1493 | + token = request.args.get('token', [None])[0] |
1494 | + path = request.path |
1495 | + deferred = deferToThread(self._getFileAlias, self.aliasID, token, path) |
1496 | deferred.addCallback( |
1497 | self._cb_getFileAlias, filename, request |
1498 | ) |
1499 | @@ -82,12 +104,12 @@ |
1500 | return util.DeferredResource(deferred) |
1501 | |
1502 | @write_transaction |
1503 | - def _getFileAlias(self, aliasID): |
1504 | + def _getFileAlias(self, aliasID, token, path): |
1505 | try: |
1506 | - alias = self.storage.getFileAlias(aliasID) |
1507 | + alias = self.storage.getFileAlias(aliasID, token, path) |
1508 | alias.updateLastAccessed() |
1509 | return (alias.contentID, alias.filename, |
1510 | - alias.mimetype, alias.date_created) |
1511 | + alias.mimetype, alias.date_created, alias.restricted) |
1512 | except LookupError: |
1513 | raise NotFound |
1514 | |
1515 | @@ -96,7 +118,7 @@ |
1516 | return fourOhFour |
1517 | |
1518 | def _cb_getFileAlias( |
1519 | - self, (dbcontentID, dbfilename, mimetype, date_created), |
1520 | + self, (dbcontentID, dbfilename, mimetype, date_created, restricted), |
1521 | filename, request |
1522 | ): |
1523 | # Return a 404 if the filename in the URL is incorrect. This offers |
1524 | @@ -105,8 +127,14 @@ |
1525 | if dbfilename.encode('utf-8') != filename: |
1526 | return fourOhFour |
1527 | |
1528 | - # Set our caching headers. Librarian files can be cached forever. |
1529 | - request.setHeader('Cache-Control', 'max-age=31536000, public') |
1530 | + if not restricted: |
1531 | + # Set our caching headers. Librarian files can be cached forever. |
1532 | + request.setHeader('Cache-Control', 'max-age=31536000, public') |
1533 | + else: |
1534 | + # Restricted files require revalidation every time. For now, |
1535 | + # until the deployment details are completely reviewed, the |
1536 | + # simplest, most cautious approach is taken: no caching permited. |
1537 | + request.setHeader('Cache-Control', 'max-age=0, private') |
1538 | |
1539 | if self.storage.hasFile(dbcontentID) or self.upstreamHost is None: |
1540 | # XXX: Brad Crittenden 2007-12-05 bug=174204: When encodings are |
1541 | |
1542 | === modified file 'lib/canonical/testing/layers.py' |
1543 | --- lib/canonical/testing/layers.py 2010-05-06 15:51:34 +0000 |
1544 | +++ lib/canonical/testing/layers.py 2010-09-07 04:07:54 +0000 |
1545 | @@ -96,7 +96,11 @@ |
1546 | from canonical.config import CanonicalConfig, config, dbconfig |
1547 | from canonical.database.revision import ( |
1548 | confirm_dbrevision, confirm_dbrevision_on_startup) |
1549 | -from canonical.database.sqlbase import cursor, ZopelessTransactionManager |
1550 | +from canonical.database.sqlbase import ( |
1551 | + cursor, |
1552 | + session_store, |
1553 | + ZopelessTransactionManager, |
1554 | + ) |
1555 | from canonical.launchpad.interfaces import IMailBox, IOpenLaunchBag |
1556 | from lp.testing import ANONYMOUS, login, logout, is_logged_in |
1557 | import lp.services.mail.stub |
1558 | @@ -193,9 +197,7 @@ |
1559 | # as soon as SQLBase._connection is accessed |
1560 | r = main_store.execute('SELECT count(*) FROM LaunchpadDatabaseRevision') |
1561 | assert r.get_one()[0] > 0, 'Storm is not talking to the database' |
1562 | - |
1563 | - session_store = getUtility(IZStorm).get('session', 'launchpad-session:') |
1564 | - assert session_store is not None, 'Failed to reconnect' |
1565 | + assert session_store() is not None, 'Failed to reconnect' |
1566 | |
1567 | |
1568 | def wait_children(seconds=120): |
1569 | |
1570 | === modified file 'lib/lp/bugs/browser/bugattachment.py' |
1571 | --- lib/lp/bugs/browser/bugattachment.py 2010-08-20 20:31:18 +0000 |
1572 | +++ lib/lp/bugs/browser/bugattachment.py 2010-09-07 04:07:54 +0000 |
1573 | @@ -10,7 +10,6 @@ |
1574 | 'BugAttachmentSetNavigation', |
1575 | 'BugAttachmentEditView', |
1576 | 'BugAttachmentURL', |
1577 | - 'SafeStreamOrRedirectLibraryFileAliasView', |
1578 | ] |
1579 | |
1580 | from cStringIO import StringIO |
1581 | @@ -23,6 +22,7 @@ |
1582 | FileNavigationMixin, |
1583 | ProxiedLibraryFileAlias, |
1584 | StreamOrRedirectLibraryFileAliasView, |
1585 | + SafeStreamOrRedirectLibraryFileAliasView, |
1586 | ) |
1587 | from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet |
1588 | from canonical.launchpad.webapp import ( |
1589 | @@ -242,21 +242,6 @@ |
1590 | return self.context.type == BugAttachmentType.PATCH |
1591 | |
1592 | |
1593 | -class SafeStreamOrRedirectLibraryFileAliasView( |
1594 | - StreamOrRedirectLibraryFileAliasView): |
1595 | - """A view for Librarian files that sets the content disposion header.""" |
1596 | - |
1597 | - def __call__(self): |
1598 | - """Stream the content of the context `ILibraryFileAlias`. |
1599 | - |
1600 | - Set the content disposition header to the safe value "attachment". |
1601 | - """ |
1602 | - self.request.response.setHeader( |
1603 | - 'Content-Disposition', 'attachment') |
1604 | - return super( |
1605 | - SafeStreamOrRedirectLibraryFileAliasView, self).__call__() |
1606 | - |
1607 | - |
1608 | class BugAttachmentFileNavigation(Navigation, FileNavigationMixin): |
1609 | """Traversal to +files/${filename}.""" |
1610 | |
1611 | |
1612 | === modified file 'lib/lp/bugs/browser/configure.zcml' |
1613 | --- lib/lp/bugs/browser/configure.zcml 2010-08-03 13:37:55 +0000 |
1614 | +++ lib/lp/bugs/browser/configure.zcml 2010-09-07 04:07:54 +0000 |
1615 | @@ -1159,9 +1159,5 @@ |
1616 | classes=" |
1617 | BugWatchSetNavigation"/> |
1618 | </facet> |
1619 | - <class class="lp.bugs.browser.bugattachment.SafeStreamOrRedirectLibraryFileAliasView"> |
1620 | - <allow attributes="__call__" /> |
1621 | - <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" /> |
1622 | - </class> |
1623 | |
1624 | </configure> |
1625 | |
1626 | === modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py' |
1627 | --- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-08-20 20:31:18 +0000 |
1628 | +++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-09-07 04:07:54 +0000 |
1629 | @@ -15,6 +15,7 @@ |
1630 | |
1631 | from canonical.launchpad.browser.librarian import ( |
1632 | StreamOrRedirectLibraryFileAliasView, |
1633 | + SafeStreamOrRedirectLibraryFileAliasView, |
1634 | ) |
1635 | from canonical.launchpad.interfaces import ILaunchBag |
1636 | from canonical.launchpad.interfaces.librarian import ( |
1637 | @@ -25,12 +26,13 @@ |
1638 | from canonical.testing import LaunchpadFunctionalLayer |
1639 | from lp.bugs.browser.bugattachment import ( |
1640 | BugAttachmentFileNavigation, |
1641 | - SafeStreamOrRedirectLibraryFileAliasView, |
1642 | ) |
1643 | from lp.testing import ( |
1644 | login_person, |
1645 | TestCaseWithFactory, |
1646 | ) |
1647 | +import lp.services.features |
1648 | +from lp.services.features.flags import NullFeatureController |
1649 | |
1650 | |
1651 | class TestAccessToBugAttachmentFiles(TestCaseWithFactory): |
1652 | @@ -80,7 +82,10 @@ |
1653 | self.assertIsNot(None, mo) |
1654 | |
1655 | def test_access_to_restricted_file(self): |
1656 | - # Requests of restricted files are handled by ProxiedLibraryFileAlias. |
1657 | + # Requests of restricted files are handled by ProxiedLibraryFileAlias |
1658 | + # until we enable the publicrestrictedlibrarian (at which point |
1659 | + # this test should check the view like |
1660 | + # test_access_to_unrestricted_file. |
1661 | lfa_with_parent = getMultiAdapter( |
1662 | (self.bugattachment.libraryfile, self.bugattachment), |
1663 | ILibraryFileAliasWithParent) |
1664 | @@ -91,6 +96,11 @@ |
1665 | request.setTraversalStack(['foo.txt']) |
1666 | navigation = BugAttachmentFileNavigation(self.bugattachment, request) |
1667 | view = navigation.publishTraverse(request, '+files') |
1668 | + # XXX Ensure the feature will be off - everything is off with |
1669 | + # NullFeatureController. bug=631884 |
1670 | + lp.services.features.per_thread.features = NullFeatureController() |
1671 | + self.addCleanup( |
1672 | + setattr, lp.services.features.per_thread, 'features', None) |
1673 | next_view, traversal_path = view.browserDefault(request) |
1674 | self.assertEqual(view, next_view) |
1675 | file_ = next_view() |
1676 | @@ -130,6 +140,11 @@ |
1677 | request.setTraversalStack(['foo.txt']) |
1678 | navigation = BugAttachmentFileNavigation(self.bugattachment, request) |
1679 | view = navigation.publishTraverse(request, '+files') |
1680 | + # XXX Ensure the feature will be off - everything is off with |
1681 | + # NullFeatureController. bug=631884 |
1682 | + lp.services.features.per_thread.features = NullFeatureController() |
1683 | + self.addCleanup( |
1684 | + setattr, lp.services.features.per_thread, 'features', None) |
1685 | next_view, traversal_path = view.browserDefault(request) |
1686 | self.assertIsInstance( |
1687 | next_view, SafeStreamOrRedirectLibraryFileAliasView) |
1688 | |
1689 | === modified file 'lib/lp/scripts/utilities/importfascist.py' |
1690 | --- lib/lp/scripts/utilities/importfascist.py 2010-08-20 20:31:18 +0000 |
1691 | +++ lib/lp/scripts/utilities/importfascist.py 2010-09-07 04:07:54 +0000 |
1692 | @@ -31,6 +31,7 @@ |
1693 | lp.codehosting.inmemory |
1694 | canonical.launchpad.browser.branchlisting |
1695 | lp.code.browser.branchlisting |
1696 | + canonical.launchpad.browser.librarian |
1697 | canonical.launchpad.feed.branch |
1698 | lp.code.feed.branch |
1699 | canonical.launchpad.interfaces.person |
1700 | |
1701 | === modified file 'lib/lp/services/features/webapp.py' |
1702 | --- lib/lp/services/features/webapp.py 2010-08-20 20:31:18 +0000 |
1703 | +++ lib/lp/services/features/webapp.py 2010-09-07 04:07:54 +0000 |
1704 | @@ -19,6 +19,14 @@ |
1705 | self._request = request |
1706 | |
1707 | def lookup(self, scope_name): |
1708 | + """Determine if scope_name applies to this request. |
1709 | + |
1710 | + Currently supports the following scopes: |
1711 | + - default |
1712 | + - is_edge/is_lpnet etc (thunks through to the config) |
1713 | + """ |
1714 | + if scope_name == 'default': |
1715 | + return True |
1716 | parts = scope_name.split('.') |
1717 | if len(parts) == 2: |
1718 | if parts[0] == 'server': |
1719 | |
1720 | === modified file 'lib/lp/testing/fakelibrarian.py' |
1721 | --- lib/lp/testing/fakelibrarian.py 2010-09-03 13:08:42 +0000 |
1722 | +++ lib/lp/testing/fakelibrarian.py 2010-09-07 04:07:54 +0000 |
1723 | @@ -150,7 +150,7 @@ |
1724 | """See `IFileUploadClient`.""" |
1725 | return NotImplementedError() |
1726 | |
1727 | - def getURLForAlias(self, aliasID): |
1728 | + def getURLForAlias(self, aliasID, secure=False): |
1729 | """See `IFileDownloadClient`.""" |
1730 | alias = self.aliases.get(aliasID) |
1731 | path = get_libraryfilealias_download_path(aliasID, alias.filename) |
The idea seems good.
I don't see why the table should exist in the session database. It should exist in the main database. Putting it in the session database just doubles the number of database connections the Librarian and garbage collector need to maintain, and makes things a maintenance pain as that database has to have schema changes and database permissions applied manually to the primary and fail over databases. This table will get comparatively little traffic, so scalability issues are a non-issue.
I believe the blessed import location for the Storm class is from storm.locals.
Should we be storing the expiry timestamp? I'm wondering if the length of time a token is valid for should be a function of the file size, or if it will be different for different parts of Launchpad.
The index on (url, token) is redundant, as it is created implicitly by the primary key constraint.
Do we actually need to store url and token separately, or just a single url column that includes the query string?
Do we want to store the url, or just the path segment? Storing just the path segment would avoid ambiguities between host and host:port, and might future proof things a bit if our SSL front ends get distributed around the world on different domain names.
Are their any magic headers the Librarian can emit so if a launchpadlibrar ian.net URL is bookmarked, the original launchpad.net URL is bookmarked instead? Not terribly important, but if we can do this we will need to store the source URL too as the Librarian has no way of reconstructing it.