Code review comment for lp:~leonardr/launchpad/anonymous-oauth

Hi Leonard,

This looks good; I have only a couple suggestions.

 review needsfixing

On Tue, 2009-12-15 at 15:13 +0000, Leonard Richardson wrote:
> === modified file 'lib/canonical/launchpad/pagetests/webservice/xx-service.txt'
> --- lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2009-08-20 04:46:48 +0000
> +++ lib/canonical/launchpad/pagetests/webservice/xx-service.txt 2009-12-15 15:13:21 +0000
> @@ -56,6 +56,39 @@
> HTTP/1.1 404 Not Found
> ...
>
> +== Anonymous requests ==
> +
> +A properly signed web service request whose OAuth token key is empty
> +is treated as an anonymous request.
> +
> + >>> root = 'http://api.launchpad.dev/beta'
> + >>> body = anon_webservice.get(root).jsonBody()
> + >>> print body['projects_collection_link']
> + http://api.launchpad.dev/beta/projects
> + >>> print body['me_link']
> + http://api.launchpad.dev/beta/people/+me
> +
> +Anonymous requests can't access certain data:
> +
> + >>> response = anon_webservice.get(body['me_link'])
> + >>> print response.getheader('status')
> + 401 Unauthorized
> + >>> print response.body
> + You need to be logged in to view this URL.
> + ...
> +
> +Anonymous requests can't change the dataset.
> +
> + >>> import simplejson
> + >>> data = simplejson.dumps({'display_name' : "This won't work"})
> + >>> response = anon_webservice.patch(root + "/~salgado",
> + ... 'application/json', data)
> + >>> print response.getheader('status')
> + 401 Unauthorized
> + >>> print response.body
> + ...
> + Unauthorized: (<Person at...>, 'displayname', 'launchpad.Edit')
> + ...
>
> == API Requests to other hosts ==
>
>
> === modified file 'lib/canonical/launchpad/testing/pages.py'
> --- lib/canonical/launchpad/testing/pages.py 2009-10-16 17:14:42 +0000
> +++ lib/canonical/launchpad/testing/pages.py 2009-12-15 15:13:21 +0000
> @@ -20,7 +20,8 @@
> from BeautifulSoup import (
> BeautifulSoup, CData, Comment, Declaration, NavigableString, PageElement,
> ProcessingInstruction, SoupStrainer, Tag)
> -from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT
> +from contrib.oauth import (
> + OAuthConsumer, OAuthRequest, OAuthSignatureMethod_PLAINTEXT, OAuthToken)
> from urlparse import urljoin
>
> from zope.app.testing.functional import HTTPCaller, SimpleCookie
> @@ -95,10 +96,15 @@
> """
> if oauth_consumer_key is not None and oauth_access_key is not None:
> login(ANONYMOUS)
> - self.consumer = getUtility(IOAuthConsumerSet).getByKey(
> - oauth_consumer_key)
> - self.access_token = self.consumer.getAccessToken(
> - oauth_access_key)
> + consumers = getUtility(IOAuthConsumerSet)
> + self.consumer = consumers.getByKey(oauth_consumer_key)
> + if self.consumer is None:

I find it really confusing that you rely on a non-registered consumer to
identify anonymous access here -- I was expecting you'd use an empty
token key for that.

To do that, though, you'll need to use the 'launchpad-library' as the
consumer key of your anon_webservice below, but when you do that we'll
lose test coverage for the auto-creation of consumers on anonymous
requests. That's no big deal as it can easily be tested by issuing a
request with a manually instantiated LaunchpadWebServiceCaller() (with
an unregistered consumer key and an empty token).

How does that sound?

> + # Set up for anonymous access
> + self.consumer = OAuthConsumer(oauth_consumer_key, '')
> + self.access_token = OAuthToken('', '')
> + else:
> + self.access_token = self.consumer.getAccessToken(
> + oauth_access_key)
> logout()
> else:
> self.consumer = None
> @@ -653,6 +659,8 @@
> 'foobar123451432', 'salgado-read-nonprivate')
> test.globs['user_webservice'] = LaunchpadWebServiceCaller(
> 'launchpad-library', 'nopriv-read-nonprivate')
> + test.globs['anon_webservice'] = LaunchpadWebServiceCaller(
> + 'anonymous-access', '')
> test.globs['setupBrowser'] = setupBrowser
> test.globs['browser'] = setupBrowser()
> test.globs['anon_browser'] = setupBrowser()
>
> === modified file 'lib/canonical/launchpad/webapp/servers.py'
> --- lib/canonical/launchpad/webapp/servers.py 2009-10-22 13:16:06 +0000
> +++ lib/canonical/launchpad/webapp/servers.py 2009-12-15 15:13:21 +0000
> @@ -1177,10 +1177,31 @@
> form = get_oauth_authorization(request)
>
> consumer_key = form.get('oauth_consumer_key')
> - consumer = getUtility(IOAuthConsumerSet).getByKey(consumer_key)
> + consumers = getUtility(IOAuthConsumerSet)
> + consumer = consumers.getByKey(consumer_key)
> + token_key = form.get('oauth_token')
> + anonymous_request = (token_key == '')
> if consumer is None:
> - raise Unauthorized('Unknown consumer (%s).' % consumer_key)
> - token_key = form.get('oauth_token')
> + if anonymous_request:
> + # This is the first time anyone has tried to make an
> + # anonymous request using this consumer
> + # name. Dynamically create the consumer.
> + consumer = consumers.new(consumer_key, '')

In general we wouldn't be able to create a consumer here because GET
requests have their transactions rolled back. However, webservice
requests always commit their transactions because they need to store
nonces, so we can rely on that to create new consumers here. It might
be worth to add a comment explaining that.

It'd also be nice to extend the extra test I suggested above to show
that the consumer is stored in the database after the request finishes.

> + else:
> + # An unknown consumer can never make a non-anonymous
> + # request, because access tokens are registered with a
> + # specific, known consumer.
> + raise Unauthorized('Unknown consumer (%s).' % consumer_key)
> + if anonymous_request:
> + # Skip the OAuth verification step and let the user access the
> + # web service as an unauthenticated user.
> + #
> + # XXX leonardr 2009-12-15 bug=496964: Ideally we'd be
> + # auto-creating a token for the anonymous user the first
> + # time, passing it through the OAuth verification step,
> + # and using it on all subsequent anonymous requests.
> + auth_utility = getUtility(IPlacelessAuthUtility)
> + return auth_utility.unauthenticatedPrincipal()
> token = consumer.getAccessToken(token_key)
> if token is None:
> raise Unauthorized('Unknown access token (%s).' % token_key)
>

--
Guilherme Salgado <email address hidden>