Merge lp:~leonardr/launchpad/oauth-doctest-to-unit-test into lp:launchpad

[Leonard Richardson]

This branch makes one substantive change: when a request token is converted into an access token, the request token's .date_expires becomes the access token's .date_expires, just as the request token's .context becomes the access token's .context and the request token's .permission becomes the access token's .access_level. This is a follow-up to my automatically-calculate-request-token-expire-time branch, which freed up IOAuthRequestToken.date_expires for just this purpose.

The vast majority of this branch consists of converting the oauth.txt doctests into unit tests. I converted all the doctests except for the ones dealing with OAuth nonces.

I also added a new unit test (test_access_token_inherits_context_and_expiration) to test the new code.

[Henning Eggers (henninge)]

Hi Leonard!
Thanks for the new unit tests! That is really cool stuff. I don't have much to
complain, I'd just like you to split up the tests a bit more. Having long test
methods with lots of asserts turns a unit test into a doc test again. Please
see my comments.

Cheers,
Henning

Am 18.10.2010 15:05, schrieb Leonard Richardson:
> === modified file 'lib/canonical/launchpad/database/oauth.py'

I have to trust you on this one, no formal glitches, though.

> === modified file 'lib/canonical/launchpad/doc/oauth.txt'
> --- lib/canonical/launchpad/doc/oauth.txt 2010-10-18 13:04:51 +0000
> +++ lib/canonical/launchpad/doc/oauth.txt 2010-10-18 13:04:52 +0000

[...]

> += OAuth =
> +
> +Most of the OAuth doctests have been converted into unit tests and
> +moved to test_oauth_tokens.py
> +

Are you sure this is necessary? Do think many people will be searching for
tests here and not look far a unittest? This is a common structure that should
not need to be explained.

The fact that the tests were *moved* is transitory but has no meaning for this
doctest.

> +== Nonces and timestamps ==
>
> A nonce is a random string, generated by the client for each request.
>

[...]

> === modified file 'lib/canonical/launchpad/tests/test_oauth_tokens.py'
> --- lib/canonical/launchpad/tests/test_oauth_tokens.py 2010-10-18 13:04:51 +0000
> +++ lib/canonical/launchpad/tests/test_oauth_tokens.py 2010-10-18 13:04:52 +0000
> @@ -1,35 +1,242 @@
> # Copyright 2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> +"""OAuth is a mechanism for allowing a user's desktop or a third-party
> +website to access Launchpad on a user's behalf. These applications
> +are identified by a unique key and are stored as OAuthConsumers. The
> +OAuth specification is defined in <http://oauth.net/core/1.0/>.
> +"""
> +
> from datetime import (
> datetime,
> timedelta
> )
> import pytz
>
> -from canonical.launchpad.webapp.interfaces import OAuthPermission
> +from zope.component import getUtility
> +from zope.proxy import sameProxiedObjects
> +from zope.security.interfaces import Unauthorized
> +
> +from canonical.launchpad.ftests import (
> + login_person,
> + logout,
> + )
> +from canonical.launchpad.webapp.interfaces import (
> + AccessLevel,
> + OAuthPermission,
> + )
> +from canonical.launchpad.webapp.testing import verifyObject
> from canonical.testing.layers import DatabaseFunctionalLayer

Wrong import ordering. Run the utilities/format-imports script on your branch,
please.

>
> +from canonical.launchpad.interfaces.oauth import (
> + IOAuthConsumer,
> + IOAuthConsumerSet,
> + IOAuthRequestToken,
> + IOAuthRequestTokenSet,
> + )
> +
> from lp.testing import (
> TestCaseWithFactory,
> + oauth_access_token_for
> )
>
>
> -class TestRequestTokens(TestCaseWithFactory):
> +class TestOAuth(TestCaseWithFactory):

Using TestCase base classes is a bit tricky. This one is safe because it does
not define any "test_" methods but usually it is a better idea to define
common functionality for test cases in a Mixin. Although in that case the
Mixin must not co...

review: Approve (code)

[Leonard Richardson (leonardr)]

I've split up the unit tests; please take another look and see if there's more you'd like me to do.

[Henning Eggers (henninge)]

The split-up looks good. I would have left the comments in the methods. Also, the convention for naming test methods is "test_methodName_condition_or_expected_result", at least where a specific method is being tested. Have a look if that fits for any of your cases.

Cheers, Henning

=== modified file 'lib/canonical/launchpad/database/oauth.py'
--- lib/canonical/launchpad/database/oauth.py	2010-10-19 18:51:49 +0000
+++ lib/canonical/launchpad/database/oauth.py	2010-10-19 18:51:50 +0000
@@ -310,7 +310,7 @@
         expires = self.date_created + timedelta(hours=REQUEST_TOKEN_VALIDITY)
         return expires <= now
 
-    def review(self, user, permission, context=None):
+    def review(self, user, permission, context=None, date_expires=None):
         """See `IOAuthRequestToken`."""
         if self.is_reviewed:
             raise AssertionError(
@@ -320,6 +320,7 @@
                 'This request token has expired and can no longer be '
                 'reviewed.')
         self.date_reviewed = datetime.now(pytz.timezone('UTC'))
+        self.date_expires = date_expires
         self.person = user
         self.permission = permission
         if IProduct.providedBy(context):
@@ -352,7 +353,8 @@
         access_level = AccessLevel.items[self.permission.name]
         access_token = OAuthAccessToken(
             consumer=self.consumer, person=self.person, key=key,
-            secret=secret, permission=access_level, product=self.product,
+            secret=secret, permission=access_level,
+            date_expires=self.date_expires, product=self.product,
             project=self.project, distribution=self.distribution,
             sourcepackagename=self.sourcepackagename)
         self.destroySelf()

=== modified file 'lib/canonical/launchpad/doc/oauth.txt'
--- lib/canonical/launchpad/doc/oauth.txt	2010-10-19 18:51:49 +0000
+++ lib/canonical/launchpad/doc/oauth.txt	2010-10-19 18:51:50 +0000
@@ -1,334 +1,9 @@
-=====
-OAuth
-=====
-
-This is a mechanism for allowing a third party application to access
-Launchpad on a user's behalf.  These applications are identified by a
-unique key and are stored as OAuthConsumers.  The OAuth specification is
-defined in <http://oauth.net/core/1.0/>.
-
-These applications (also called consumers) are managed by the
-OAuthConsumerSet utility.
-
-    >>> from canonical.launchpad.webapp.testing import verifyObject
-    >>> from canonical.launchpad.webapp.interfaces import (
-    ...     AccessLevel, OAuthPermission)
-    >>> from canonical.launchpad.interfaces import (
-    ...     IOAuthAccessToken, IOAuthConsumer, IOAuthConsumerSet,
-    ...     IOAuthNonce, IOAuthRequestToken, IPersonSet)
-    >>> consumer_set = getUtility(IOAuthConsumerSet)
-    >>> verifyObject(IOAuthConsumerSet, consumer_set)
-    True
-
-    >>> consumer = consumer_set.new(key='asdfg')
-    >>> verifyObject(IOAuthConsumer, consumer)
-    True
-
-    >>> consumer_set.getByKey('asdfg') == consumer
-    True
-
-    >>> print consumer_set.getByKey('gfdsa')
-    None
-
-As mentioned above, the keys are unique, so we can't create a second
-Consumer with the same key.
-
-    >>> consumer_set.new(key='asdfg')
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-Desktop consumers
-=================
-
-In a web context, each application is represented by a unique consumer
-key. But a typical user sitting at a typical desktop (or other
-personal computer), using multiple desktop applications that integrate
-with Launchpad, is represented by a single consumer key. The user's
-session as a whole is a single "consumer", and the consumer key is
-expected to contain structured information: the type of system
-(usually the operating system plus the word "desktop") and a string
-that the end-user would recognize as identifying their computer.
-
-    >>> desktop_key = consumer_set.new(
-    ...     "System-wide: Ubuntu desktop (hostname)")
-    >>> desktop_key.is_integrated_desktop
-    True
-    >>> print desktop_key.integrated_desktop_type
-    Ubuntu desktop
-    >>> print desktop_key.integrated_desktop_name
-    hostname
-
-    >>> desktop_key = consumer_set.new(
-    ...     "System-wide: Android phone (My Phone)")
-    >>> desktop_key.is_integrated_desktop
-    True
-    >>> print desktop_key.integrated_desktop_type
-    Android phone
-    >>> print desktop_key.integrated_desktop_name
-    My Phone
-
-A normal OAuth consumer does not have this information.
-
-    >>> ordinary_key = consumer_set.new("Not a desktop at all.")
-    >>> ordinary_key.is_integrated_desktop
-    False
-    >>> print ordinary_key.integrated_desktop_type
-    None
-    >>> print ordinary_key.integrated_desktop_name
-    None
-
-Request tokens
-==============
-
-When a consumer wants to access protected resources on Launchpad, it
-must first ask for an OAuthRequestToken, which is then used when the
-consumer sends the user to the Launchpad authorization page.
-
-
-Creating request tokens
------------------------
-
-The request tokens are created using IOAuthConsumer.newRequestToken().
-
-    # XXX EdwinGrubbs 2008-10-03 bug=277756
-    # Tests could be simplified with helper methods for creating tokens
-    # in different states.
-    >>> request_token = consumer.newRequestToken()
-    >>> verifyObject(IOAuthRequestToken, request_token)
-    True
-
-The token's key and secret have a length of 20 and 80 respectively.
-
-    >>> len(request_token.key)
-    20
-    >>> len(request_token.secret)
-    80
-
-Newly created tokens have no context associated with.
-
-    >>> print request_token.context
-    None
-
-Initially, a token does not have a person or permission associated with it as
-the consumer doesn't know the user's identity on Launchpad.
-
-    >>> print request_token.person
-    None
-    >>> print request_token.permission
-    None
-    >>> print request_token.date_reviewed
-    None
-
-Once the user reviews (approve/decline) the consumer's request, the
-token is considered used and can only be exchanged for an access token
-(when the access is granted by the user).
-
-    >>> salgado = getUtility(IPersonSet).getByName('salgado')
-    >>> request_token.review(salgado, OAuthPermission.WRITE_PUBLIC)
-    >>> from canonical.launchpad.ftests import syncUpdate
-    >>> syncUpdate(request_token)
-
-    >>> from datetime import datetime, timedelta
-    >>> import pytz
-    >>> print request_token.person.name
-    salgado
-    >>> request_token.permission
-    <DBItem OAuthPermission.WRITE_PUBLIC...
-    >>> request_token.date_reviewed <= datetime.now(pytz.timezone('UTC'))
-    True
-    >>> request_token.is_reviewed
-    True
-
-When reviewing a token, we can also change the context associated with
-it, which means the consumer using that token will only have access
-to things linked to that context (Product, ProjectGroup, Distribution,
-DistroSourcePackage).
-
-    >>> from lp.registry.interfaces.distribution import IDistributionSet
-    >>> from lp.registry.interfaces.product import IProductSet
-    >>> from lp.registry.interfaces.projectgroup import IProjectGroupSet
-
-    >>> firefox = getUtility(IProductSet)['firefox']
-    >>> request_token2 = consumer.newRequestToken()
-    >>> request_token2.review(
-    ...     salgado, OAuthPermission.WRITE_PRIVATE, context=firefox)
-    >>> print request_token2.context.title
-    Mozilla Firefox
-
-    >>> mozilla = getUtility(IProjectGroupSet)['mozilla']
-    >>> request_token2 = consumer.newRequestToken()
-    >>> request_token2.review(
-    ...     salgado, OAuthPermission.WRITE_PRIVATE, context=mozilla)
-    >>> print request_token2.context.title
-    The Mozilla Project
-
-    >>> ubuntu = getUtility(IDistributionSet)['ubuntu']
-    >>> evolution = ubuntu.getSourcePackage('evolution')
-    >>> request_token2 = consumer.newRequestToken()
-    >>> request_token2.review(
-    ...     salgado, OAuthPermission.WRITE_PRIVATE, context=evolution)
-
-    >>> from canonical.encoding import ascii_smash
-    >>> print ascii_smash(request_token2.context.title)
-    evolution package in Ubuntu
-
-
-Retrieving request tokens
--------------------------
-
-Any consumer can retrieve its request tokens as long as it knows their
-keys.
-
-    >>> consumer.getRequestToken(request_token.key) == request_token
-    True
-
-If there is no token with the given key, or the existing token is
-associated with another consumer, getRequestToken() will return None.
-
-    >>> print consumer.getRequestToken('zzzzzzzz')
-    None
-    >>> consumer2 = consumer_set.new(key='foobar')
-    >>> print consumer2.getRequestToken(request_token.key)
-    None
-
-We also have OAuthRequestTokenSet.getByKey(), which allows us to get a
-request token with the given key regardless of the consumer associated
-with it.
-
-    >>> from canonical.launchpad.interfaces.oauth import IOAuthRequestTokenSet
-    >>> token_set = getUtility(IOAuthRequestTokenSet)
-    >>> token_set.getByKey(request_token.key) == request_token
-    True
-
-    >>> request_token2 = consumer2.newRequestToken()
-    >>> token_set.getByKey(request_token2.key) == request_token2
-    True
-
-    >>> print token_set.getByKey('zzzzzzzzz')
-    None
-
-
-Exchanging request tokens for access tokens
--------------------------------------------
-
-Once a request token has been reviewed it may be exchanged for an access
-token. That may happen only if the user actually granted some sort of
-permission to the consumer when reviewing the request.
-
-The access token's permission will be the same as the request token's
-one, but it is an item of AccessLevel rather than OAuthPermission
-because the former doesn't have an UNAUTHORIZED item (which doesn't
-make sense in access tokens).
-
-    >>> request_token.is_reviewed
-    True
-    >>> request_token.permission
-    <DBItem OAuthPermission.WRITE_PUBLIC...
-    >>> access_token = request_token.createAccessToken()
-    >>> verifyObject(IOAuthAccessToken, access_token)
-    True
-    >>> access_token.permission
-    <DBItem AccessLevel.WRITE_PUBLIC...
-
-After the access token is generated, the request token is deleted.
-
-    >>> print consumer.getRequestToken(request_token.key)
-    None
-
-By default, access tokens don't expire.
-
-    >>> print access_token.date_expires
-    None
-
-Access tokens will also inherit the context from the request token.
-
-    >>> request_token2 = consumer.newRequestToken()
-    >>> request_token2.review(
-    ...     salgado, OAuthPermission.WRITE_PRIVATE, context=firefox)
-    >>> access_token2 = request_token2.createAccessToken()
-    >>> print access_token2.context.title
-    Mozilla Firefox
-
-If the request token hasn't been reviewed yet, it can't be used to
-create an access token.
-
-    >>> request_token = consumer.newRequestToken()
-    >>> request_token.is_reviewed
-    False
-    >>> access_token = request_token.createAccessToken()
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-The same holds true for request tokens that have UNAUTHORIZED as their
-permission.
-
-    >>> request_token.review(salgado, OAuthPermission.UNAUTHORIZED)
-    >>> request_token.is_reviewed
-    True
-    >>> access_token = request_token.createAccessToken()
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-
-Access tokens
-=============
-
-As shown above, access tokens can be created from any reviewed (and
-authorized) request tokens. These tokens are then stored by the consumer
-and included in all further requests made on behalf of the same user, so
-we need a way to retrieve an access token from any consumer.
-
-    >>> consumer.getAccessToken(access_token.key) == access_token
-    True
-
-An access token can only be changed by the person associated with it.
-
-    >>> access_token.permission = OAuthPermission.WRITE_PUBLIC
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
-    >>> login_person(access_token.person)
-    >>> access_token.permission = AccessLevel.WRITE_PUBLIC
-
-From any given person it's possible to retrieve his non-expired access
-tokens.
-
-    >>> access_token.person.oauth_access_tokens.count()
-    4
-    >>> access_token.date_expires = (
-    ...     datetime.now(pytz.timezone('UTC')) - timedelta(hours=1))
-    >>> syncUpdate(access_token)
-    >>> access_token.person.oauth_access_tokens.count()
-    3
-
-It's also possible to retrieve the user's non-expired request tokens.
-
-    >>> unclaimed_request_token = consumer.newRequestToken()
-    >>> unclaimed_request_token.review(salgado, OAuthPermission.WRITE_PUBLIC)
-    >>> salgado.oauth_request_tokens.count()
-    5
-    >>> salgado.oauth_request_tokens[0].date_expires = (
-    ...     datetime.now(pytz.timezone('UTC')) - timedelta(hours=1))
-    >>> syncUpdate(unclaimed_request_token)
-    >>> salgado.oauth_request_tokens.count()
-    4
-
-A user has edit permission over his own access tokens, he can expire them.
-
-    >>> api_user = factory.makePerson()
-    >>> login_person(api_user)
-    >>> api_request_token = consumer.newRequestToken()
-    >>> api_request_token.review(api_user, OAuthPermission.WRITE_PUBLIC)
-    >>> api_access_token = api_request_token.createAccessToken()
-    >>> api_access_token.date_expires = (
-    ...     datetime.now(pytz.timezone('UTC')) - timedelta(hours=1))
-
-
-Nonces and timestamps
-=====================
+= OAuth =
+
+Most of the OAuth doctests have been converted into unit tests and
+moved to test_oauth_tokens.py
+
+== Nonces and timestamps ==
 
 A nonce is a random string, generated by the client for each request.
 
@@ -469,57 +144,3 @@
     Traceback (most recent call last):
     ...
     TimestampOrderingError: ...
-
-
-Helper methods
-==============
-
-The oauth_access_token_for() helper function makes it easy to get an
-access token for any user, consumer key, permission, and context.
-
-If the user already has an access token that does what you need,
-oauth_access_token_for() returns the existing token.
-
-    >>> from lp.testing import oauth_access_token_for
-    >>> existing_token = salgado.oauth_access_tokens[0]
-    >>> token = oauth_access_token_for(
-    ...     existing_token.consumer.key, existing_token.person,
-    ...     existing_token.permission, existing_token.context)
-
-    >>> from zope.proxy import sameProxiedObjects
-    >>> sameProxiedObjects(token, existing_token)
-    True
-
-If the user does not already have an access token that matches your
-requirements, oauth_access_token_for() creates a request token and
-automatically authorizes it. Here, we create a brand new token for a
-never-before-seen consumer.
-
-    >>> new_consumer = 'new consumer key to test oauth_access_token_for'
-    >>> token = oauth_access_token_for(
-    ...     new_consumer, salgado, 'WRITE_PRIVATE', firefox)
-
-    >>> print token.consumer.key
-    new consumer key to test oauth_access_token_for
-
-    >>> print token.person.name
-    salgado
-
-    >>> token.permission
-    <DBItem AccessLevel.WRITE_PRIVATE...>
-
-    >>> print token.context.name
-    firefox
-
-    >>> print token.date_expires
-    None
-
-You can use the token identifying one of Launchpad's OAuth permission
-levels instead of the constant itself, but if you specify a
-nonexistent permission you'll get an error.
-
-    >>> oauth_access_token_for(
-    ...     new_consumer, salgado, 'NO_SUCH_PERMISSION', firefox)
-    Traceback (most recent call last):
-    ...
-    KeyError: 'NO_SUCH_PERMISSION'

=== modified file 'lib/canonical/launchpad/tests/test_oauth_tokens.py'
--- lib/canonical/launchpad/tests/test_oauth_tokens.py	2010-10-19 18:51:49 +0000
+++ lib/canonical/launchpad/tests/test_oauth_tokens.py	2010-10-19 18:51:50 +0000
@@ -1,35 +1,234 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+"""OAuth is a mechanism for allowing a user's desktop or a third-party
+website to access Launchpad on a user's behalf.  These applications
+are identified by a unique key and are stored as OAuthConsumers.  The
+OAuth specification is defined in <http://oauth.net/core/1.0/>.
+"""
+
 from datetime import (
     datetime,
-    timedelta
+    timedelta,
     )
+
 import pytz
+from zope.component import getUtility
+from zope.proxy import sameProxiedObjects
+from zope.security.interfaces import Unauthorized
 
-from canonical.launchpad.webapp.interfaces import OAuthPermission
+from canonical.launchpad.ftests import (
+    login_person,
+    logout,
+    )
+from canonical.launchpad.interfaces.oauth import (
+    IOAuthAccessToken,
+    IOAuthConsumer,
+    IOAuthConsumerSet,
+    IOAuthRequestToken,
+    IOAuthRequestTokenSet,
+    )
+from canonical.launchpad.webapp.interfaces import (
+    AccessLevel,
+    OAuthPermission,
+    )
+from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing.layers import DatabaseFunctionalLayer
-
 from lp.testing import (
+    oauth_access_token_for,
     TestCaseWithFactory,
     )
 
 
-class TestRequestTokens(TestCaseWithFactory):
+class TestOAuth(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        """Set up a dummy person and OAuth consumer."""
-        super(TestRequestTokens, self).setUp()
+        """Set up some convenient data objects and timestamps."""
+        super(TestOAuth, self).setUp()
 
         self.person = self.factory.makePerson()
         self.consumer = self.factory.makeOAuthConsumer()
 
         now = datetime.now(pytz.timezone('UTC'))
+        self.in_a_while = now + timedelta(hours=1)
         self.a_long_time_ago = now - timedelta(hours=1000)
 
-    def testExpiredRequestTokenCantBeReviewed(self):
+
+class TestConsumerSet(TestOAuth):
+    """Tests of the utility that manages OAuth consumers."""
+
+    def setUp(self):
+        super(TestConsumerSet, self).setUp()
+        self.consumers = getUtility(IOAuthConsumerSet)
+
+    def test_interface(self):
+        verifyObject(IOAuthConsumerSet, self.consumers)
+
+    def test_new(self):
+        consumer = self.consumers.new(
+            self.factory.getUniqueString("oauthconsumerkey"))
+        verifyObject(IOAuthConsumer, consumer)
+
+    def test_new_wont_create_duplicate_consumer(self):
+        self.assertRaises(
+            AssertionError, self.consumers.new, key=self.consumer.key)
+
+    def test_getByKey(self):
+        self.assertEqual(
+            self.consumers.getByKey(self.consumer.key), self.consumer)
+
+    def test_getByKey_returns_none_for_nonexistent_consumer(self):
+        # There is no consumer called "oauthconsumerkey-nonexistent".
+        nonexistent_key = self.factory.getUniqueString(
+            "oauthconsumerkey-nonexistent")
+        self.assertEqual(self.consumers.getByKey(nonexistent_key), None)
+
+
+class TestRequestTokenSet(TestOAuth):
+    """Test the set of request tokens."""
+
+    def setUp(self):
+        """Set up a reference to the token list."""
+        super(TestRequestTokenSet, self).setUp()
+        self.tokens = getUtility(IOAuthRequestTokenSet)
+
+    def test_getByKey(self):
+        token = self.consumer.newRequestToken()
+        self.assertEquals(token, self.tokens.getByKey(token.key))
+
+    def test_getByKey_returns_none_for_unused_key(self):
+        self.assertEquals(None, self.tokens.getByKey("no-such-token"))
+
+
+class TestRequestTokens(TestOAuth):
+    """Tests for OAuth request token objects."""
+
+    def test_newRequestToken(self):
+        request_token = self.consumer.newRequestToken()
+        verifyObject(IOAuthRequestToken, request_token)
+
+    def test_key_and_secret_automatically_generated(self):
+        request_token = self.consumer.newRequestToken()
+        self.assertEqual(len(request_token.key), 20)
+        self.assertEqual(len(request_token.secret), 80)
+
+    def test_date_created(self):
+        request_token = self.consumer.newRequestToken()
+        now = datetime.now(pytz.timezone('UTC'))
+        self.assertTrue(request_token.date_created <= now)
+
+    def test_new_token_is_not_reviewed(self):
+        request_token = self.consumer.newRequestToken()
+        self.assertFalse(request_token.is_reviewed)
+        self.assertEqual(None, request_token.person)
+        self.assertEqual(None, request_token.date_reviewed)
+
+        # An unreviewed token has no associated permission, expiration
+        # date, or context.
+        self.assertEqual(None, request_token.permission)
+        self.assertEqual(None, request_token.date_expires)
+        self.assertEqual(None, request_token.context)
+
+    def test_getRequestToken(self):
+        token_1 = self.consumer.newRequestToken()
+        token_2 = self.consumer.getRequestToken(token_1.key)
+        self.assertEqual(token_1, token_2)
+
+    def test_getRequestToken_for_wrong_consumer_returns_none(self):
+        token_1 = self.consumer.newRequestToken()
+        consumer_2 = self.factory.makeOAuthConsumer()
+        self.assertEquals(
+            None, consumer_2.getRequestToken(token_1.key))
+
+    def test_getRequestToken_for_nonexistent_key_returns_none(self):
+        self.assertEquals(
+            None, self.consumer.getRequestToken("no-such-token"))
+
+    def test_token_review(self):
+        request_token = self.consumer.newRequestToken()
+
+        request_token.review(self.person, OAuthPermission.WRITE_PUBLIC)
+        now = datetime.now(pytz.timezone('UTC'))
+
+        self.assertTrue(request_token.is_reviewed)
+        self.assertEquals(request_token.person, self.person)
+        self.assertEquals(request_token.permission,
+                          OAuthPermission.WRITE_PUBLIC)
+
+        self.assertTrue(request_token.date_created <= now)
+
+        # By default, reviewing a token does not set a context or
+        # expiration date.
+        self.assertEquals(request_token.context, None)
+        self.assertEquals(request_token.date_expires, None)
+
+    def test_token_review_as_unauthorized(self):
+        request_token = self.consumer.newRequestToken()
+        request_token.review(self.person, OAuthPermission.UNAUTHORIZED)
+
+        # This token has been reviewed, but it may not be used for any
+        # purpose.
+        self.assertTrue(request_token.is_reviewed)
+        self.assertEquals(request_token.permission,
+                          OAuthPermission.UNAUTHORIZED)
+
+    def test_review_with_expiration_date(self):
+        # A request token may be associated with an expiration date
+        # upon review.
+        request_token = self.consumer.newRequestToken()
+        request_token.review(
+            self.person, OAuthPermission.WRITE_PUBLIC,
+            date_expires=self.in_a_while)
+        self.assertEquals(request_token.date_expires, self.in_a_while)
+
+    def test_review_with_expiration_date_in_the_past(self):
+        # The expiration date, like the permission and context, is
+        # associated with the eventual access token. It has nothing to
+        # do with how long the *request* token will remain
+        # valid.
+        #
+        # Setting a request token's date_expires to a date in the past
+        # is not a good idea, but it won't expire the request token.
+        request_token = self.consumer.newRequestToken()
+        request_token.review(
+            self.person, OAuthPermission.WRITE_PUBLIC,
+            date_expires=self.a_long_time_ago)
+        self.assertEquals(request_token.date_expires, self.a_long_time_ago)
+        self.assertFalse(request_token.is_expired)
+
+    def _reviewed_token_for_context(self, context_factory):
+        """Create and review a request token with a given context."""
+        token = self.consumer.newRequestToken()
+        name = self.factory.getUniqueString('context')
+        context = context_factory(name)
+        token.review(
+            self.person, OAuthPermission.WRITE_PRIVATE, context=context)
+        return token, name
+
+    def test_review_with_product_context(self):
+        # When reviewing a request token, the context may be set to a
+        # product.
+        token, name = self._reviewed_token_for_context(
+            self.factory.makeProduct)
+        self.assertEquals(token.context.name, name)
+
+    def test_review_with_project_context(self):
+        # When reviewing a request token, the context may be set to a
+        # project.
+        token, name = self._reviewed_token_for_context(
+            self.factory.makeProject)
+        self.assertEquals(token.context.name, name)
+
+    def test_review_with_distrosourcepackage_context(self):
+        # When reviewing a request token, the context may be set to a
+        # distribution source package.
+        token, name = self._reviewed_token_for_context(
+            self.factory.makeDistributionSourcePackage)
+        self.assertEquals(token.context.name, name)
+
+    def test_expired_request_token_cant_be_reviewed(self):
         """An expired request token can't be reviewed."""
         token = self.factory.makeOAuthRequestToken(
             date_created=self.a_long_time_ago)
@@ -37,7 +236,97 @@
             AssertionError, token.review, self.person,
             OAuthPermission.WRITE_PUBLIC)
 
-    def testExpiredRequestTokenCantBeExchanged(self):
+    def test_get_request_tokens_for_person(self):
+        """It's possible to get a person's request tokens."""
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_request_tokens.count(), 0)
+        for i in range(0,3):
+            self.factory.makeOAuthRequestToken(reviewed_by=person)
+        self.assertEquals(person.oauth_request_tokens.count(), 3)
+
+    def test_expired_request_token_disappears_from_list(self):
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_request_tokens.count(), 0)
+        request_token = self.factory.makeOAuthRequestToken(reviewed_by=person)
+        self.assertEquals(person.oauth_request_tokens.count(), 1)
+
+        login_person(person)
+        request_token.date_expires = self.a_long_time_ago
+        logout()
+
+        self.assertEquals(person.oauth_request_tokens.count(), 0)
+
+
+class TestAccessTokens(TestOAuth):
+    """Tests for OAuth access tokens."""
+
+    def _exchange_request_token_for_access_token(self):
+        # Use this method instead of factory.makeOAuthAccessToken() to
+        # a) to show how a request token is exchanged for an access
+        # token, b) acquire a reference to the request token that was
+        # used to create the access token.
+        request_token = self.consumer.newRequestToken()
+        request_token.review(self.person, OAuthPermission.WRITE_PRIVATE)
+        access_token = request_token.createAccessToken()
+        return request_token, access_token
+
+    def test_exchange_request_token_for_access_token(self):
+        # Make sure the basic exchange of request token for access
+        # token works.
+        request_token, access_token = (
+            self._exchange_request_token_for_access_token())
+        verifyObject(IOAuthAccessToken, access_token)
+
+    def test_access_token_inherits_data_fields_from_request_token(self):
+        request_token, access_token = (
+            self._exchange_request_token_for_access_token())
+
+        self.assertEquals(request_token.consumer, access_token.consumer)
+
+        # An access token inherits its permission from the request
+        # token that created it. But an access token's .permission is
+        # an AccessLevel object, not an OAuthPermission. The only real
+        # difference is that there's no AccessLevel corresponding to
+        # OAuthPermission.UNAUTHORIZED.
+        self.assertEquals(
+            access_token.permission, AccessLevel.WRITE_PRIVATE)
+
+        self.assertEquals(None, access_token.context)
+        self.assertEquals(None, access_token.date_expires)
+
+    def test_access_token_field_inheritance(self):
+        # Make sure that specific fields like context and expiration
+        # date are passed down from request token to access token.
+        context = self.factory.makeProduct()
+        request_token = self.consumer.newRequestToken()
+        request_token.review(
+            self.person, OAuthPermission.WRITE_PRIVATE,
+            context=context, date_expires=self.in_a_while)
+
+        access_token = request_token.createAccessToken()
+        self.assertEquals(request_token.context, access_token.context)
+        self.assertEquals(
+            request_token.date_expires, access_token.date_expires)
+
+    def test_request_token_disappears_when_exchanged(self):
+        request_token, access_token = (
+            self._exchange_request_token_for_access_token())
+        self.assertEquals(
+            None, self.consumer.getRequestToken(request_token.key))
+
+    def test_cant_exchange_unreviewed_request_token(self):
+        # An unreviewed request token cannot be exchanged for an access token.
+        token = self.consumer.newRequestToken()
+        self.assertRaises(AssertionError, token.createAccessToken)
+
+    def test_cant_exchange_unauthorized_request_token(self):
+        # A request token associated with the UNAUTHORIZED
+        # OAuthPermission cannot be exchanged for an access token.
+        token = self.consumer.newRequestToken()
+        token.review(self.person, OAuthPermission.UNAUTHORIZED)
+        self.assertRaises(AssertionError, token.createAccessToken)
+
+    def test_expired_request_token_cant_be_exchanged(self):
         """An expired request token can't be exchanged for an access token.
 
         This can only happen if the token was reviewed before it expired.
@@ -45,3 +334,80 @@
         token = self.factory.makeOAuthRequestToken(
             date_created=self.a_long_time_ago, reviewed_by=self.person)
         self.assertRaises(AssertionError, token.createAccessToken)
+
+    def test_write_permission(self):
+        """An access token can only be modified by its creator."""
+        access_token = self.factory.makeOAuthAccessToken()
+        def try_to_set():
+            access_token.permission = AccessLevel.WRITE_PUBLIC
+        self.assertRaises(Unauthorized, try_to_set)
+
+        login_person(access_token.person)
+        try_to_set()
+        logout()
+
+    def test_get_access_tokens_for_person(self):
+        """It's possible to get a person's access tokens."""
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_access_tokens.count(), 0)
+        for i in range(0,3):
+            self.factory.makeOAuthAccessToken(self.consumer, person)
+        self.assertEquals(person.oauth_access_tokens.count(), 3)
+
+    def test_expired_access_token_disappears_from_list(self):
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_access_tokens.count(), 0)
+        access_token = self.factory.makeOAuthAccessToken(
+            self.consumer, person)
+        self.assertEquals(person.oauth_access_tokens.count(), 1)
+
+        login_person(access_token.person)
+        access_token.date_expires = self.a_long_time_ago
+        logout()
+        self.assertEquals(person.oauth_access_tokens.count(), 0)
+
+
+class TestHelperFunctions(TestOAuth):
+
+    def setUp(self):
+        super(TestHelperFunctions, self).setUp()
+        self.context = self.factory.makeProduct()
+
+    def test_oauth_access_token_for_creates_nonexistent_token(self):
+        # If there's no token for user/consumer key/permission/context,
+        # one is created.
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_access_tokens.count(), 0)
+        access_token = oauth_access_token_for(
+            self.consumer.key, person, OAuthPermission.WRITE_PUBLIC,
+            self.context)
+        self.assertEquals(person.oauth_access_tokens.count(), 1)
+
+    def test_oauth_access_token_for_retrieves_existing_token(self):
+        # If there's already a token for a
+        # user/consumer key/permission/context, it's retrieved.
+        person = self.factory.makePerson()
+        self.assertEquals(person.oauth_access_tokens.count(), 0)
+        access_token = oauth_access_token_for(
+            self.consumer.key, person, OAuthPermission.WRITE_PUBLIC,
+            self.context)
+        self.assertEquals(person.oauth_access_tokens.count(), 1)
+
+        access_token_2 = oauth_access_token_for(
+            access_token.consumer.key, access_token.person,
+            access_token.permission, access_token.context)
+        self.assertEquals(person.oauth_access_tokens.count(), 1)
+        self.assertTrue(sameProxiedObjects(access_token, access_token_2))
+
+    def test_oauth_access_token_string_permission(self):
+        """You can pass in a string instead of an OAuthPermission."""
+        access_token = oauth_access_token_for(
+            self.consumer.key, self.person, 'WRITE_PUBLIC')
+        self.assertEqual(access_token.permission, AccessLevel.WRITE_PUBLIC)
+
+    def test_oauth_access_token_string_with_nonexistent_permission(self):
+        # NO_SUCH_PERMISSION doesn't correspond to any OAuthPermission
+        # object.
+        self.assertRaises(
+            KeyError, oauth_access_token_for, self.consumer.key,
+            self.person, 'NO_SUCH_PERMISSION')

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-10-19 18:51:49 +0000
+++ lib/lp/testing/factory.py	2010-10-19 18:51:50 +0000
@@ -3186,9 +3186,9 @@
             secret = ''
         return getUtility(IOAuthConsumerSet).new(key, secret)
 
-    def makeOAuthRequestToken(
-        self, consumer=None, date_created=None, reviewed_by=None,
-        access_level=OAuthPermission.READ_PUBLIC):
+    def makeOAuthRequestToken(self, consumer=None, date_created=None,
+                              reviewed_by=None,
+                              access_level=OAuthPermission.READ_PUBLIC):
         """Create a (possibly reviewed) OAuth request token."""
         if consumer is None:
             consumer = self.makeOAuthConsumer()
@@ -3205,6 +3205,15 @@
             unwrapped_token.date_created = date_created
         return token
 
+    def makeOAuthAccessToken(self, consumer=None, owner=None,
+                             access_level=OAuthPermission.READ_PUBLIC):
+        """Create an OAuth access token."""
+        if owner is None:
+            owner = self.makePerson()
+        request_token = self.makeOAuthRequestToken(
+            consumer, reviewed_by=owner, access_level=access_level)
+        return request_token.createAccessToken()
+
 
 # Some factory methods return simple Python types. We don't add
 # security wrappers for them, as well as for objects created by