launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/314507-oauth into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #314507 OAUTH server ignores ignores first element in header (rather than realm key)
  https://bugs.launchpad.net/bugs/314507


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.
-- 
https://code.launchpad.net/~mbp/launchpad/314507-oauth/+merge/44188
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/314507-oauth into lp:launchpad.

=== 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:49:55 +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:49:55 +0000
@@ -243,15 +243,20 @@
     @staticmethod
     def _split_header(header):
         params = {}
+        header = header.lstrip()
+        if not header.lstrip().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