Merge lp:~leonardr/launchpad/no-cookie-vary-header into lp:launchpad

Proposed by Leonard Richardson on 2010-04-05
Status: Merged
Approved by: Aaron Bentley on 2010-04-05
Approved revision: 10618
Merge reported by: Leonard Richardson
Merged at revision: not available
Proposed branch: lp:~leonardr/launchpad/no-cookie-vary-header
Merge into: lp:launchpad
Diff against target: 215 lines (+135/-24) (has conflicts)
5 files modified
lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt (+91/-19)
lib/canonical/launchpad/pagetests/webservice/xx-service.txt (+22/-0)
lib/canonical/launchpad/webapp/servers.py (+20/-2)
lib/canonical/launchpad/webapp/tests/test_servers.py (+1/-2)
versions.cfg (+1/-1)
Text conflict in lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt
Contents conflict in lib/lp/bugs/scripts/checkwatches/core.py
Path conflict: lib/lp/bugs/scripts/checkwatches/core.py / lib/lp/bugs/scripts/checkwatches/core.py
Contents conflict in lib/lp/bugs/scripts/checkwatches/tests/test_core.py
Path conflict: lib/lp/bugs/scripts/checkwatches/tests/test_core.py / lib/lp/bugs/scripts/checkwatches/tests/test_core.py
Contents conflict in lib/lp/registry/model/projectgroup.py
Path conflict: lib/lp/registry/model/projectgroup.py / lib/lp/registry/model/projectgroup.py
To merge this branch: bzr merge lp:~leonardr/launchpad/no-cookie-vary-header
Reviewer Review Type Date Requested Status
Aaron Bentley (community) 2010-04-05 Approve on 2010-04-06
Gary Poster (community) Approve on 2010-04-06
Review via email: mp+22812@code.launchpad.net

Description of the change

This branch stops Launchpad from including 'Cookie' in its Vary header when serving web service requests. Although not technically incorrect (the Launchpad web service doesn't use cookies at all), it's not necessary to mention this header, and mentioning it triggers an httplib2 bug (http://code.google.com/p/httplib2/issues/detail?id=94) which manifests in launchpadlib as bug 521927.

To post a comment you must log in.
Aaron Bentley (abentley) wrote :

I think there are supposed to be two blank lines, not one, before a new header, as on line 8 of the patch. Otherwise, good.

review: Approve
Leonard Richardson (leonardr) wrote :

I decided to use my new powers to write launchpadlib tests for this in Launchpad, and discovered that removing Cookie doesn't solve the problem: we also need to remove Authorization, because every OAuth nonce is different.

I gave this some thought and decided that this is fine with our existing web service, but depending on the performance improvements we make in the future, we might want to change launchpadlib to keep a different cache for every authorization token. This would degrade client-side performance but it would make it impossible for less-privileged tokens to accidentally get cached data obtained by more privileged tokens.

I don't know how much this matters, and it certainly wouldn't affect a malicious program, because a malicious program can just scour the .launchpadlib directory for a more-privileged token and use that token instead.

10619. By Leonard Richardson on 2010-04-05

Fixed spacing.

10620. By Leonard Richardson on 2010-04-05

Merged in the launchpadlib pagetest branch.

10621. By Leonard Richardson on 2010-04-05

Added test, removed Authorization from Vary so the test would work.

10622. By Leonard Richardson on 2010-04-06

Merge trunk.

10623. By Leonard Richardson on 2010-04-06

Fixed test.

Gary Poster (gary) wrote :

The launchpadlib test is awesome. Yay.

The potential security issues are concerning, but they do seem fine now.

IRC discussion:

[10:23am] gary_poster: leonardr: xx-service.txt looks like it would fail now. Your code says self.response.setHeader('Vary', 'Accept') but your test says
[10:23am] gary_poster: 83+ >>> response = webservice.get(
[10:23am] gary_poster: 84+ ... 'http://bugs.launchpad.dev/api/devel/')
[10:23am] gary_poster: 85+ >>> print response.getheader('Vary')
[10:23am] gary_poster: 86+ Authorization, Accept
[10:23am] leonardr: gary: thanks, don't know how i missed that

review: Approve
Aaron Bentley (abentley) wrote :

I hearby reapprove this with the new authorization non-variance.

review: Approve
10624. By Leonard Richardson on 2010-04-06

Fixed test again.

10625. By Leonard Richardson on 2010-04-06

Fixed test failure.

10626. By Leonard Richardson on 2010-04-06

Updated versions.cfg to seek a new version of launchpadlib.

10627. By Leonard Richardson on 2010-04-27

Merge with trunk.

10628. By Leonard Richardson on 2010-04-27

Merged from trunk and resolved conflict.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt'
2--- lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt 2010-04-23 18:53:53 +0000
3+++ lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt 2010-04-27 15:30:57 +0000
4@@ -1,5 +1,6 @@
5+*******************************
6 Using launchpadlib in pagetests
7-===============================
8+*******************************
9
10 As an alternative to crafting HTTP requests with the 'webservice'
11 object, you can write pagetests using launchpadlib.
12@@ -50,21 +51,92 @@
13 >>> launchpad = Launchpad(credentials, 'http://api.launchpad.dev/')
14 >>> print launchpad.me.name
15 no-priv
16-
17- >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
18- ... 'http://api.launchpad.dev/')
19-
20-The Launchpad object for the anonymous user can be used to access
21-public information.
22-
23- >>> apache_results = lp_anon.project_groups.search(text="Apache")
24- >>> print apache_results[0].name
25- apache
26-
27-But trying to access information that requires a logged in user
28-results in an error.
29-
30- >>> print lp_anon.me.name
31- Traceback (most recent call last):
32- ...
33- HTTPError: HTTP Error 401: Unauthorized...
34+<<<<<<< TREE
35+
36+ >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
37+ ... 'http://api.launchpad.dev/')
38+
39+The Launchpad object for the anonymous user can be used to access
40+public information.
41+
42+ >>> apache_results = lp_anon.project_groups.search(text="Apache")
43+ >>> print apache_results[0].name
44+ apache
45+
46+But trying to access information that requires a logged in user
47+results in an error.
48+
49+ >>> print lp_anon.me.name
50+ Traceback (most recent call last):
51+ ...
52+ HTTPError: HTTP Error 401: Unauthorized...
53+=======
54+
55+Anonymous access
56+================
57+
58+ >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
59+ ... 'http://api.launchpad.dev/')
60+
61+The Launchpad object for the anonymous user can be used to access
62+public information.
63+
64+ >>> apache_results = lp_anon.project_groups.search(text="Apache")
65+ >>> print apache_results[0].name
66+ apache
67+
68+But trying to access information that requires a logged in user
69+results in an error.
70+
71+ >>> print lp_anon.me.name
72+ Traceback (most recent call last):
73+ ...
74+ HTTPError: HTTP Error 401: Unauthorized...
75+
76+Caching
77+=======
78+
79+Let's make sure Launchpad serves caching-related headers that make
80+launchpadlib work correctly. First, we set up a temporary directory to
81+store the cache.
82+
83+ >>> import tempfile
84+ >>> cache = tempfile.mkdtemp()
85+
86+Then we make it possible to view the HTTP traffic between launchpadlib
87+and Launchpad.
88+
89+ >>> import httplib2
90+ >>> old_debug_level = httplib2.debuglevel
91+ >>> httplib2.debuglevel = 1
92+
93+Now create a Launchpad object and observe how it populates the cache.
94+
95+ >>> launchpad = Launchpad(
96+ ... credentials, 'http://api.launchpad.dev/', cache=cache)
97+ send: 'GET /1.0/ ...accept: application/vnd.sun.wadl+xml...'
98+ reply: 'HTTP/1.0 200 Ok\n'
99+ ...
100+ send: 'GET /1.0/ ...accept: application/json...'
101+ reply: 'HTTP/1.0 200 Ok\n'
102+ ...
103+
104+Create a second Launchpad object, and the cached documents will be
105+used to make conditional HTTP requests. The conditions will succeed,
106+and launchpadlib will not have to download the documents again.
107+
108+ >>> launchpad = Launchpad(
109+ ... credentials, 'http://api.launchpad.dev/', cache=cache)
110+ send: 'GET /1.0/ ...accept: application/vnd.sun.wadl+xml...'
111+ reply: 'HTTP/1.0 304 Not Modified\n'
112+ ...
113+ send: 'GET /1.0/ ...accept: application/json...'
114+ reply: 'HTTP/1.0 304 Not Modified\n'
115+ ...
116+
117+Cleanup.
118+
119+ >>> import shutil
120+ >>> shutil.rmtree(cache)
121+ >>> httplib2.debuglevel = old_debug_level
122+>>>>>>> MERGE-SOURCE
123
124=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-service.txt'
125--- lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2010-02-16 17:05:07 +0000
126+++ lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2010-04-27 15:30:57 +0000
127@@ -209,3 +209,25 @@
128 HTTP/1.1 401 Unauthorized
129 ...
130 Request is missing an OAuth consumer key.
131+
132+
133+The 'Vary' Header
134+=================
135+
136+Launchpad's web service sets the Vary header differently from other
137+parts of Launchpad.
138+
139+ >>> browser.open("http://launchpad.dev/")
140+ >>> print browser.headers['Vary']
141+ Cookie, Authorization
142+
143+ >>> response = webservice.get(
144+ ... 'http://bugs.launchpad.dev/api/devel/')
145+ >>> print response.getheader('Vary')
146+ Accept
147+
148+The web service's Vary header does not mention the 'Cookie' header,
149+because the web service doesn't use cookies. It doesn't mention the
150+'Authorization' header, because every web service request has a
151+distinct 'Authorization' header. It does mention the 'Accept' header,
152+because the web service does use content negotiation.
153
154=== modified file 'lib/canonical/launchpad/webapp/servers.py'
155--- lib/canonical/launchpad/webapp/servers.py 2010-03-19 20:02:25 +0000
156+++ lib/canonical/launchpad/webapp/servers.py 2010-04-27 15:30:57 +0000
157@@ -1267,8 +1267,26 @@
158 def __init__(self, body_instream, environ, response=None):
159 super(WebServiceClientRequest, self).__init__(
160 body_instream, environ, response)
161- # Web service requests use content negotiation.
162- self.response.setHeader('Vary', 'Cookie, Authorization, Accept')
163+ # Web service requests use content negotiation, so we put
164+ # 'Accept' in the Vary header. They don't use cookies, so
165+ # there's no point in putting 'Cookie' in the Vary header, and
166+ # putting 'Authorization' in the Vary header totally destroys
167+ # caching because every web service request contains a
168+ # distinct OAuth nonce in its Authorization header.
169+ #
170+ # Because 'Authorization' is not in the Vary header, a client
171+ # that reuses a single cache for different OAuth credentials
172+ # could conceivably leak private information to an
173+ # unprivileged user via the cache. This won't happen for the
174+ # web service root resource because the service root is the
175+ # same for everybody. It won't happen for entry resources
176+ # because if two users have a different representation of an
177+ # entry, the ETag will also be different and a conditional
178+ # request will fail.
179+ #
180+ # Once lazr.restful starts setting caching directives other
181+ # than ETag, we may have to revisit this.
182+ self.response.setHeader('Vary', 'Accept')
183
184 def getRootURL(self, rootsite):
185 """See IBasicLaunchpadRequest."""
186
187=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
188--- lib/canonical/launchpad/webapp/tests/test_servers.py 2010-03-16 13:36:29 +0000
189+++ lib/canonical/launchpad/webapp/tests/test_servers.py 2010-04-27 15:30:57 +0000
190@@ -315,8 +315,7 @@
191 def test_response_should_vary_based_on_content_type(self):
192 request = WebServiceClientRequest(StringIO.StringIO(''), {})
193 self.assertEquals(
194- request.response.getHeader('Vary'),
195- 'Cookie, Authorization, Accept')
196+ request.response.getHeader('Vary'), 'Accept')
197
198
199 class TestBasicLaunchpadRequest(TestCase):
200
201=== renamed file 'lib/lp/bugs/scripts/checkwatches/core.py' => 'lib/lp/bugs/scripts/checkwatches/core.py.THIS'
202=== renamed file 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py' => 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py.THIS'
203=== renamed file 'lib/lp/registry/model/projectgroup.py' => 'lib/lp/registry/model/projectgroup.py.THIS'
204=== modified file 'versions.cfg'
205--- versions.cfg 2010-04-22 04:56:27 +0000
206+++ versions.cfg 2010-04-27 15:30:57 +0000
207@@ -21,7 +21,7 @@
208 grokcore.component = 1.6
209 httplib2 = 0.6.0
210 ipython = 0.9.1
211-launchpadlib = 1.5.8
212+launchpadlib = 1.6.0
213 lazr.authentication = 0.1.1
214 lazr.batchnavigator = 1.1
215 lazr.config = 1.1.3