Merge lp:~mbp/launchpad/314507-oauth into lp:launchpad

[Martin Pool]

[r=lifeless][ui=none][bug=314507] more correct parsing of OAuth http header

This is a fix for <https://bugs.launchpad.net/launchpad/+bug/314507>. The bug is that Launchpad's API server assumes there is a 'realm' parameter as the first part of the Authentication header, even though <http://tools.ietf.org/html/rfc5849> doesn't say it must be first or indeed present at all.

The bug is in contrib/oauth.py. Perhaps the fix should be sent upstream but there is no copyright statement on this file.

I added unit tests in what seems like the most reasonable place, given that oauth.py itself doesn't have a test case.

I've manually tested this locally by running

  curl -k -v -H 'Authorization: OAuth oauth_consumer_key="just+testing",realm="blah"' https://api.launchpad.dev/beta/

Against production launchpad, this is treated as anonymous/unauthenticated because lp doesn't see the header. On my instance it says "Unknown consumer (just+testing)." which I think is as it should be.

[Robert Collins (lifeless)]

I think this is fine but I'm struggling to understand why the old behaviour occured.

review: Needs Information

[Martin Pool (mbp)]

On 20 December 2010 16:54, Robert Collins <email address hidden> wrote:
> Review: Needs Information
> I think this is fine but I'm struggling to understand why the old behaviour occured.

Sure:

The spec says, basically, that OAuth requests have a header like

    Authorization: OAuth realm="Photos",
        oauth_consumer_key="dpf43f3p2l4k3l03",
        oauth_signature_method="HMAC-SHA1",
        oauth_timestamp="137131200",
        oauth_nonce="wIjqoS",
        oauth_callback="http%3A%2F%2Fprinter.example.com%2Fready",
        oauth_signature="74KNZJeDHnMBp0EMJ9ZHt%2FXKycU%3D"

where the order of the key/value parameters is arbitrary (before
signing they are put in a canonical ordering). The realm attribute is
optional, and ignored when calculating the request. You can see that
the old code assumed there would be a realm parameter and it would be
first. Neither is justified by the RFC. Assuming the string "OAuth
realm" will occur before the first comma is just wrong.

--
Martin

[Robert Collins (lifeless)]

Thats not what I was asking about :) - I got that from the bug and MP cover.

What I mean is
"- if param.find('OAuth realm') > -1:
- continue"

That looks like it would not trigger if the first element is anything
other than realm. So *how* was it triggering?

[Martin Pool (mbp)]

It doesn't trigger. The interesting thing is that the bug doesn't cause data to be skipped, just misfiled.

If we have let's say

   headers = OAuthRequest._split_header(
       'OAuth oauth_consumer_key="justtesting"')

then it splits on commas, strips white space, splits on equals, dequotes, and ends up with

   { 'OAuth oauth_consumer_key': 'justtesting' }

and therefore the upper-level code sees no entry under 'oauth_consumer_key'.

The problem is that it seems 'OAuth ' as part of the first key, rather than the true structure which is the word 'OAuth' _followed by_ a k/v dict.

[Robert Collins (lifeless)]

review: Approve

[Martin Pool (mbp)]

Launchpad's pqm rejects my requests, so perhaps someone else would be
kind enough to submit this for me?

--
Martin

[Francis J. Lacoste (flacoste)]

> The bug is in contrib/oauth.py. Perhaps the fix should be sent upstream but there is
> no copyright statement on this file.

That's interesting because I thought we had removed contrib/oauth.py. We already include it as the proper PyPI module in buildout. (versions.cfg has oatuh = 1.0)

It seems that the code wasn't updated to match :-/

Anyway, upstream is here: http://pypi.python.org/pypi/oauth/1.0.1

[Martin Pool (mbp)]

It looks like Maverick's python-oauth already has an equivalent fix, so if there are no other local changes in the contrib copy, perhaps we should just delete it.

[Robert Collins (lifeless)]

On Tue, Jan 4, 2011 at 1:04 PM, Martin Pool <email address hidden> wrote:
> It looks like Maverick's python-oauth already has an equivalent fix, so if there are no other local changes in the contrib copy, perhaps we should just delete it.

We run on lucid; we need a fix there, or a package in the ppa, or the
egg we're using via buildout to have the fix.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_authentication.py'
--- lib/canonical/launchpad/webapp/tests/test_authentication.py	2010-10-04 19:50:45 +0000
+++ lib/canonical/launchpad/webapp/tests/test_authentication.py	2010-12-20 03:57:16 +0000
@@ -10,6 +10,8 @@
 
 from zope.app.security.principalregistry import UnauthenticatedPrincipal
 
+from contrib.oauth import OAuthRequest
+
 from canonical.config import config
 from canonical.launchpad.ftests import login
 from canonical.launchpad.testing.systemdocs import (
@@ -54,6 +56,31 @@
         self.failUnless(isinstance(principal, UnauthenticatedPrincipal))
 
 
+class TestOAuthParsing(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_split_oauth(self):
+        # OAuth headers are parsed correctly: see bug 314507.
+        # This was really a bug in the underlying contrib/oauth.py module, but
+        # it has no standalone test case.
+        #
+        # Note that the 'realm' parameter is not returned, because it's not
+        # included in the OAuth calculations.
+        headers = OAuthRequest._split_header(
+            'OAuth realm="foo", oauth_consumer_key="justtesting"')
+        self.assertEquals(headers,
+            {'oauth_consumer_key': 'justtesting'})
+        headers = OAuthRequest._split_header(
+            'OAuth oauth_consumer_key="justtesting"')
+        self.assertEquals(headers,
+            {'oauth_consumer_key': 'justtesting'})
+        headers = OAuthRequest._split_header(
+            'OAuth oauth_consumer_key="justtesting", realm="realm"')
+        self.assertEquals(headers,
+            {'oauth_consumer_key': 'justtesting'})
+
+
 def test_suite():
     suite = unittest.TestLoader().loadTestsFromName(__name__)
     suite.addTest(LayeredDocFileSuite(

=== modified file 'lib/contrib/oauth.py'
--- lib/contrib/oauth.py	2008-03-24 13:12:41 +0000
+++ lib/contrib/oauth.py	2010-12-20 03:57:16 +0000
@@ -243,15 +243,20 @@
     @staticmethod
     def _split_header(header):
         params = {}
+        header = header.lstrip()
+        if not header.startswith('OAuth '):
+            raise ValueError("not an OAuth header: %r" % header)
+        header = header[6:]
         parts = header.split(',')
         for param in parts:
-            # ignore realm parameter
-            if param.find('OAuth realm') > -1:
-                continue
             # remove whitespace
             param = param.strip()
             # split key-value
             param_parts = param.split('=', 1)
+            if param_parts[0] == 'realm':
+                # Realm header is not an OAuth parameter according to rfc5849
+                # section 3.4.1.3.1.
+                continue
             # remove quotes and unescape the value
             params[param_parts[0]] = urllib.unquote(param_parts[1].strip('\"'))
         return params