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

> === modified file 'lib/canonical/launchpad/testing/pages.py'
> --- lib/canonical/launchpad/testing/pages.py 2009-12-15 14:43:33 +0000
> +++ lib/canonical/launchpad/testing/pages.py 2009-12-16 15:52:00 +0000
> @@ -99,12 +99,24 @@
> consumers = getUtility(IOAuthConsumerSet)
> self.consumer = consumers.getByKey(oauth_consumer_key)
> if self.consumer is None:
> - # Set up for anonymous access
> + # The client wants to make a request using an
> + # unrecognized consumer key. Only an anonymous request
> + # (one in which oauth_access_key is the empty string)
> + # will succeed, but we run this code in either case,
> + # so that we can verify that a non-anonymous request
> + # fails.
> self.consumer = OAuthConsumer(oauth_consumer_key, '')
> - self.access_token = OAuthToken('', '')
> + self.access_token = OAuthToken(oauth_access_key, '')
> else:
> - self.access_token = self.consumer.getAccessToken(
> - oauth_access_key)
> + if oauth_access_key == '':
> + # The client wants to make an anonymous request
> + # using a recognized consumer key.
> + self.access_token = OAuthToken(oauth_access_key, '')
> + else:
> + # The client wants to make an authorized request
> + # using a recognized consumer key.
> + self.access_token = self.consumer.getAccessToken(
> + oauth_access_key)

Although this is now making it clear that we identify anonymous requests
by the lack of an access key, I think this can be simplified a bit.
This is what I had in mind when I first commented on this change.

    if oauth_access_key == '':
        # The client wants to make an anonymous request.
        if self.consumer is None:
            # Anonymous requests don't need a registered consumer, so we
            # manually create a "fake" OauthConsumer (we call it "fake"
            # because it's not really an IOAuthConsumer as returned by
            # IOAuthConsumerSet.getByKey) to be used in the requests we make.
            self.consumer = OAuthConsumer(oauth_consumer_key, '')
        self.access_token = OAuthToken(oauth_access_key, '')
    else:
        assert self.consumer is not None, (
            "Need a registered consumer key for authenticated requests.")
        self.access_token = self.consumer.getAccessToken(
            oauth_access_key)

It's untested but I think it's equivalent to what you have.

> logout()
> else:
> self.consumer = None
> @@ -660,7 +672,7 @@
> test.globs['user_webservice'] = LaunchpadWebServiceCaller(
> 'launchpad-library', 'nopriv-read-nonprivate')
> test.globs['anon_webservice'] = LaunchpadWebServiceCaller(
> - 'anonymous-access', '')
> + 'launchpad-library', '')
> 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-12-15 14:43:33 +0000
> +++ lib/canonical/launchpad/webapp/servers.py 2009-12-16 16:42:15 +0000
> @@ -1180,12 +1180,23 @@
> consumers = getUtility(IOAuthConsumerSet)
> consumer = consumers.getByKey(consumer_key)
> token_key = form.get('oauth_token')
> - anonymous_request = (token_key == '')
> + anonymous_request = (token_key == '' or token_key is None)

Does this mean we'll treat requests which don't include an oauth_token
as anonymous? I thought the plan was to do that just for the requests
which explicitly pass an empty string as the oauth_token.

> if consumer is None:
> + if consumer_key is None:
> + # Most likely the user is trying to make a totally
> + # unauthenticated request.
> + raise Unauthorized(
> + 'Request is missing an OAuth consumer key.')
> if anonymous_request:
> # This is the first time anyone has tried to make an
> # anonymous request using this consumer
> # name. Dynamically create the consumer.
> + #
> + # In the normal website this wouldn't be possible
> + # because GET requests have their transactions rolled
> + # back. But webservice requests always have their
> + # transactions committed so that we can keep track of
> + # the OAuth nonces and prevent replay attacks.
> consumer = consumers.new(consumer_key, '')
> else:
> # An unknown consumer can never make a non-anonymous
>

--
Guilherme Salgado <email address hidden>