Launchpad itself

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

314507-oauth
Merge into devel

Proposed by Martin Pool on 2010-12-20

Status:

Merged

Approved by:

Robert Collins on 2010-12-20

Approved revision:

no longer in the source branch.

Merged at revision:

12127

Proposed branch:

lp://staging/~mbp/launchpad/314507-oauth

Merge into:

lp://staging/launchpad

Diff against target:

72 lines (+35/-3)

2 files modified

lib/canonical/launchpad/webapp/tests/test_authentication.py (+27/-0)
lib/contrib/oauth.py (+8/-3)

To merge this branch:

bzr merge lp://staging/~mbp/launchpad/314507-oauth

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Collins (community)		2010-12-20	Approve on 2010-12-20
Review via email: mp+44188@code.staging.launchpad.net

Commit message

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

Description of the change

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.

Revision history for this message

Robert Collins (lifeless) wrote on 2010-12-20:

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

review: Needs Information

Revision history for this message

Martin Pool (mbp) wrote on 2010-12-20:

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

Revision history for this message

Robert Collins (lifeless) wrote on 2010-12-20:

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?

Revision history for this message

Martin Pool (mbp) wrote on 2010-12-20:

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.

Revision history for this message

Robert Collins (lifeless) on 2010-12-20:

review: Approve

Revision history for this message

Martin Pool (mbp) wrote on 2010-12-21:

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

--
Martin

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2011-01-03:

> 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

Revision history for this message

Martin Pool (mbp) wrote on 2011-01-04:

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.

Revision history for this message

Robert Collins (lifeless) wrote on 2011-01-04:

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.