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

[Leonard Richardson]

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.

[Aaron Bentley (abentley)]

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)]

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.

[lp:~leonardr/launchpad/no-cookie-vary-header]

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)]

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)]

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

review: Approve

[lp:~leonardr/launchpad/no-cookie-vary-header]

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.

=== modified file 'lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt'
--- lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt	2010-04-23 18:53:53 +0000
+++ lib/canonical/launchpad/pagetests/webservice/launchpadlib.txt	2010-04-27 15:30:57 +0000
@@ -1,5 +1,6 @@
+*******************************
 Using launchpadlib in pagetests
-===============================
+*******************************
 
 As an alternative to crafting HTTP requests with the 'webservice'
 object, you can write pagetests using launchpadlib.
@@ -50,21 +51,92 @@
     >>> launchpad = Launchpad(credentials, 'http://api.launchpad.dev/')
     >>> print launchpad.me.name
     no-priv
-
-    >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
-    ...                                       'http://api.launchpad.dev/')
-
-The Launchpad object for the anonymous user can be used to access
-public information.
-
-    >>> apache_results = lp_anon.project_groups.search(text="Apache")
-    >>> print apache_results[0].name
-    apache
-
-But trying to access information that requires a logged in user
-results in an error.
-
-    >>> print lp_anon.me.name
-    Traceback (most recent call last):
-      ...
-    HTTPError: HTTP Error 401: Unauthorized...
+<<<<<<< TREE
+
+    >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
+    ...                                       'http://api.launchpad.dev/')
+
+The Launchpad object for the anonymous user can be used to access
+public information.
+
+    >>> apache_results = lp_anon.project_groups.search(text="Apache")
+    >>> print apache_results[0].name
+    apache
+
+But trying to access information that requires a logged in user
+results in an error.
+
+    >>> print lp_anon.me.name
+    Traceback (most recent call last):
+      ...
+    HTTPError: HTTP Error 401: Unauthorized...
+=======
+
+Anonymous access
+================
+
+    >>> lp_anon = Launchpad.login_anonymously('launchpadlib test',
+    ...                                       'http://api.launchpad.dev/')
+
+The Launchpad object for the anonymous user can be used to access
+public information.
+
+    >>> apache_results = lp_anon.project_groups.search(text="Apache")
+    >>> print apache_results[0].name
+    apache
+
+But trying to access information that requires a logged in user
+results in an error.
+
+    >>> print lp_anon.me.name
+    Traceback (most recent call last):
+      ...
+    HTTPError: HTTP Error 401: Unauthorized...
+
+Caching
+=======
+
+Let's make sure Launchpad serves caching-related headers that make
+launchpadlib work correctly. First, we set up a temporary directory to
+store the cache.
+
+    >>> import tempfile
+    >>> cache = tempfile.mkdtemp()
+
+Then we make it possible to view the HTTP traffic between launchpadlib
+and Launchpad.
+
+    >>> import httplib2
+    >>> old_debug_level = httplib2.debuglevel
+    >>> httplib2.debuglevel = 1
+
+Now create a Launchpad object and observe how it populates the cache.
+
+    >>> launchpad = Launchpad(
+    ...     credentials, 'http://api.launchpad.dev/', cache=cache)
+    send: 'GET /1.0/ ...accept: application/vnd.sun.wadl+xml...'
+    reply: 'HTTP/1.0 200 Ok\n'
+    ...
+    send: 'GET /1.0/ ...accept: application/json...'
+    reply: 'HTTP/1.0 200 Ok\n'
+    ...
+
+Create a second Launchpad object, and the cached documents will be
+used to make conditional HTTP requests. The conditions will succeed,
+and launchpadlib will not have to download the documents again.
+
+    >>> launchpad = Launchpad(
+    ...     credentials, 'http://api.launchpad.dev/', cache=cache)
+    send: 'GET /1.0/ ...accept: application/vnd.sun.wadl+xml...'
+    reply: 'HTTP/1.0 304 Not Modified\n'
+    ...
+    send: 'GET /1.0/ ...accept: application/json...'
+    reply: 'HTTP/1.0 304 Not Modified\n'
+    ...
+
+Cleanup.
+
+    >>> import shutil
+    >>> shutil.rmtree(cache)
+    >>> httplib2.debuglevel = old_debug_level
+>>>>>>> MERGE-SOURCE
 
=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-service.txt'
--- lib/canonical/launchpad/pagetests/webservice/xx-service.txt	2010-02-16 17:05:07 +0000
+++ lib/canonical/launchpad/pagetests/webservice/xx-service.txt	2010-04-27 15:30:57 +0000
@@ -209,3 +209,25 @@
     HTTP/1.1 401 Unauthorized
     ...
     Request is missing an OAuth consumer key.
+
+
+The 'Vary' Header
+=================
+
+Launchpad's web service sets the Vary header differently from other
+parts of Launchpad.
+
+    >>> browser.open("http://launchpad.dev/")
+    >>> print browser.headers['Vary']
+    Cookie, Authorization
+
+    >>> response = webservice.get(
+    ...     'http://bugs.launchpad.dev/api/devel/')
+    >>> print response.getheader('Vary')
+    Accept
+
+The web service's Vary header does not mention the 'Cookie' header,
+because the web service doesn't use cookies. It doesn't mention the
+'Authorization' header, because every web service request has a
+distinct 'Authorization' header. It does mention the 'Accept' header,
+because the web service does use content negotiation.
 
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2010-03-19 20:02:25 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2010-04-27 15:30:57 +0000
@@ -1267,8 +1267,26 @@
     def __init__(self, body_instream, environ, response=None):
         super(WebServiceClientRequest, self).__init__(
             body_instream, environ, response)
-        # Web service requests use content negotiation.
-        self.response.setHeader('Vary', 'Cookie, Authorization, Accept')
+        # Web service requests use content negotiation, so we put
+        # 'Accept' in the Vary header. They don't use cookies, so
+        # there's no point in putting 'Cookie' in the Vary header, and
+        # putting 'Authorization' in the Vary header totally destroys
+        # caching because every web service request contains a
+        # distinct OAuth nonce in its Authorization header.
+        #
+        # Because 'Authorization' is not in the Vary header, a client
+        # that reuses a single cache for different OAuth credentials
+        # could conceivably leak private information to an
+        # unprivileged user via the cache. This won't happen for the
+        # web service root resource because the service root is the
+        # same for everybody. It won't happen for entry resources
+        # because if two users have a different representation of an
+        # entry, the ETag will also be different and a conditional
+        # request will fail.
+        #
+        # Once lazr.restful starts setting caching directives other
+        # than ETag, we may have to revisit this.
+        self.response.setHeader('Vary', 'Accept')
 
     def getRootURL(self, rootsite):
         """See IBasicLaunchpadRequest."""
 
=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
--- lib/canonical/launchpad/webapp/tests/test_servers.py	2010-03-16 13:36:29 +0000
+++ lib/canonical/launchpad/webapp/tests/test_servers.py	2010-04-27 15:30:57 +0000
@@ -315,8 +315,7 @@
     def test_response_should_vary_based_on_content_type(self):
         request = WebServiceClientRequest(StringIO.StringIO(''), {})
         self.assertEquals(
-            request.response.getHeader('Vary'),
-            'Cookie, Authorization, Accept')
+            request.response.getHeader('Vary'), 'Accept')
 
 
 class TestBasicLaunchpadRequest(TestCase):
 
=== renamed file 'lib/lp/bugs/scripts/checkwatches/core.py' => 'lib/lp/bugs/scripts/checkwatches/core.py.THIS'
=== renamed file 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py' => 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py.THIS'
=== renamed file 'lib/lp/registry/model/projectgroup.py' => 'lib/lp/registry/model/projectgroup.py.THIS'
=== modified file 'versions.cfg'
--- versions.cfg	2010-04-22 04:56:27 +0000
+++ versions.cfg	2010-04-27 15:30:57 +0000
@@ -21,7 +21,7 @@
 grokcore.component = 1.6
 httplib2 = 0.6.0
 ipython = 0.9.1
-launchpadlib = 1.5.8
+launchpadlib = 1.6.0
 lazr.authentication = 0.1.1
 lazr.batchnavigator = 1.1
 lazr.config = 1.1.3