launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01519
lp:~leonardr/launchpad/automatically-calculate-request-token-expire-time into lp:launchpad
Leonard Richardson has proposed merging lp:~leonardr/launchpad/automatically-calculate-request-token-expire-time into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Our use of the 'date_expires' field in IOAuthToken is a mess, but it didn't matter much up to this point, because we never really used it. I'm about to start using it, so this branch is cleanup to make it something I can use.
I plan to move the doctests in lib/canonical/launchpad/doc/oauth.txt into unit tests in the new test_oauth.py, but since this branch is subtle on its own I would rather do that in a separate branch.
The design behind this branch is salgado-approved.
Here are the problems with our current use of date_expires:
1. Request tokens always expire after REQUEST_TOKEN_VALIDITY hours. This is a site-wide constant, so there's no need to manually set the date that a request token expires--you can calculate it from REQUEST_TOKEN_VALIDITY and date_created.
2. Request tokens may pass their expiration date, but we never do anything to expired tokens, or check whether they have expired. So, effectively, request tokens never expire. A request token can be three years old and you'll still be able to review it and exchange it for an access token. This opens up our users to attacks when an intermediary acquires the key to a reviewed and forgotten request token.
3. Before a request token is exchanged for an access token, there is no way to distinguish between the date on which a request token expires, and the date on which the end-user wants the not-yet-created access token to expire. This isn't a problem now because the end-user is never given the choice of creating an access token that will expire. But I'm about to give them that choice.
I also noticed a smaller, less serious problem:
4. REQUEST_TOKEN_VALIDITY is twelve hours. That's way too long. Users will either exchange a request token for an access token within a few minutes, or forget about it altogether.
My solution to problem #4 is to reduce REQUEST_TOKEN_VALIDITY to two hours. My solution to the first three problems is a little more complex.
First, I am no longer storing the expiration date of a request token in date_expires. I am calculating it from date_created and REQUEST_TOKEN_VALIDITY. I have introduced an is_expired attribute to IOAuthToken, and separate implementations for OAuthRequestToken and OAuthAccessToken. I check token.is_expired whenever an access token is used to sign a request, and whenever an attempt is made to review a request token or exchange one for an access token.
This is the code tested by my unit tests, and this means that request tokens will in fact become unusable after REQUEST_TOKEN_VALIDITY hours. I do not do anything special to clean up expired tokens, but we weren't doing anything special before.
Second, I am taking date_expires out of IOAuthToken, and introducing it separately into the subclasses IOAuthRequestToken and IOAuthAccessToken. IOAuthRequestToken.date_expires is now the expiration date of the access token that will eventually be created from the request token--not anything to do with the request token itself. When the request token is exchanged for an access token, the IOAuthRequestToken.date_expires field is copied into IOAuthAccessToken.date_expires.
This is by direct analogy to IOAuthRequestToken.permission, which is the access level of the access token that will eventually be created from the request token--not anything to do with the request token itself. When a request token is exchanged for an access token, IOAuthRequestToken.permission becomes IOAuthAccessToken.permission.
I also split out IOAuthToken.date_created into the IOAuthRequestToken and IOAuthAccessToken, though the only difference is the description. It looked a little awkward to have the .date_created description say that .date_created is the date that a request token was created *or* the date an access token was created from some preexisting request token. It's easier to explain that separately for request and access tokens.
--
https://code.launchpad.net/~leonardr/launchpad/automatically-calculate-request-token-expire-time/+merge/38423
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/automatically-calculate-request-token-expire-time into lp:launchpad.
=== modified file 'lib/canonical/launchpad/database/oauth.py'
--- lib/canonical/launchpad/database/oauth.py 2010-10-03 15:30:06 +0000
+++ lib/canonical/launchpad/database/oauth.py 2010-10-14 14:26:09 +0000
@@ -59,7 +59,7 @@
from lp.registry.interfaces.projectgroup import IProjectGroup
# How many hours should a request token be valid for?
-REQUEST_TOKEN_VALIDITY = 12
+REQUEST_TOKEN_VALIDITY = 2
# The OAuth Core 1.0 spec (http://oauth.net/core/1.0/#nonce) says that a
# timestamp "MUST be equal or greater than the timestamp used in previous
# requests," but this is likely to cause problems if the client does request
@@ -105,10 +105,8 @@
def newRequestToken(self):
"""See `IOAuthConsumer`."""
key, secret = create_token_key_and_secret(table=OAuthRequestToken)
- date_expires = (datetime.now(pytz.timezone('UTC'))
- + timedelta(hours=REQUEST_TOKEN_VALIDITY))
return OAuthRequestToken(
- consumer=self, key=key, secret=secret, date_expires=date_expires)
+ consumer=self, key=key, secret=secret)
def getAccessToken(self, key):
"""See `IOAuthConsumer`."""
@@ -177,6 +175,11 @@
else:
return None
+ @property
+ def is_expired(self):
+ now = datetime.now(pytz.timezone('UTC'))
+ return self.date_expires is not None and self.date_expires <= now
+
def checkNonceAndTimestamp(self, nonce, timestamp):
"""See `IOAuthAccessToken`."""
timestamp = float(timestamp)
@@ -254,10 +257,19 @@
else:
return None
+ @property
+ def is_expired(self):
+ now = datetime.now(pytz.timezone('UTC'))
+ expires = self.date_created + timedelta(hours=REQUEST_TOKEN_VALIDITY)
+ return expires <= now
+
def review(self, user, permission, context=None):
"""See `IOAuthRequestToken`."""
assert not self.is_reviewed, (
"Request tokens can be reviewed only once.")
+ assert not self.is_expired, (
+ 'This request token has expired and can no longer be reviewed.'
+ )
self.date_reviewed = datetime.now(pytz.timezone('UTC'))
self.person = user
self.permission = permission
@@ -279,6 +291,11 @@
'Cannot create an access token from an unreviewed request token.')
assert self.permission != OAuthPermission.UNAUTHORIZED, (
'The user did not grant access to this consumer.')
+ assert not self.is_expired, (
+ 'This request token has expired and can no longer be exchanged '
+ 'for an access token.'
+ )
+
key, secret = create_token_key_and_secret(table=OAuthAccessToken)
access_level = AccessLevel.items[self.permission.name]
access_token = OAuthAccessToken(
=== modified file 'lib/canonical/launchpad/interfaces/oauth.py'
--- lib/canonical/launchpad/interfaces/oauth.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/oauth.py 2010-10-14 14:26:09 +0000
@@ -67,9 +67,6 @@
def newRequestToken():
"""Return a new `IOAuthRequestToken` with a random key and secret.
- Also sets the token's date_expires to `REQUEST_TOKEN_VALIDITY` hours
- from the creation date (now).
-
The other attributes of the token are supposed to be set whenever the
user logs into Launchpad and grants (or not) access to this consumer.
"""
@@ -132,11 +129,11 @@
schema=IPerson, title=_('Person'), required=False, readonly=False,
description=_('The user on whose behalf the consumer is accessing.'))
date_created = Datetime(
- title=_('Date created'), required=True, readonly=True)
- date_expires = Datetime(
- title=_('Date expires'), required=False, readonly=False,
- description=_('From this date onwards this token can not be used '
- 'by the consumer to access protected resources.'))
+ title=_('Date created'), required=True, readonly=True,
+ description=_('For a request token, the date the token was created. '
+ 'The request token will be good for a limited time '
+ 'after this date. For an access token, the date '
+ 'some request token was exchanged for this token.'))
key = TextLine(
title=_('Key'), required=True, readonly=True,
description=_('The key used to identify this token. It is included '
@@ -154,6 +151,12 @@
title=_("Distribution"), required=False, vocabulary='Distribution')
context = Attribute("FIXME")
+ is_expired = Bool(
+ title=_("Whether or not this token has expired."),
+ required=False, readonly=True,
+ description=_("A token may only be usable for a limited time, "
+ "after which it will expire."))
+
class IOAuthAccessToken(IOAuthToken):
"""A token used by a consumer to access protected resources in LP.
@@ -168,6 +171,11 @@
description=_('The level of access given to the application acting '
'on your behalf.'))
+ date_expires = Datetime(
+ title=_('Date expires'), required=False, readonly=False,
+ description=_('From this date onwards this token can not be used '
+ 'by the consumer to access protected resources.'))
+
def checkNonceAndTimestamp(nonce, timestamp):
"""Verify the nonce and timestamp.
@@ -202,6 +210,10 @@
vocabulary=OAuthPermission,
description=_('The permission you give to the application which may '
'act on your behalf.'))
+ date_expires = Datetime(
+ title=_('Date expires'), required=False, readonly=False,
+ description=_('The expiration date for the permission you give to '
+ 'the application which may act on your behalf.')
date_reviewed = Datetime(
title=_('Date reviewed'), required=True, readonly=True,
description=_('The date in which the user authorized (or not) the '
=== added file 'lib/canonical/launchpad/tests/test_oauth_tokens.py'
--- lib/canonical/launchpad/tests/test_oauth_tokens.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/test_oauth_tokens.py 2010-10-14 14:26:09 +0000
@@ -0,0 +1,47 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from datetime import (
+ datetime,
+ timedelta
+ )
+import pytz
+
+from canonical.launchpad.webapp.interfaces import OAuthPermission
+from canonical.testing.layers import DatabaseFunctionalLayer
+
+from lp.testing import (
+ TestCaseWithFactory,
+ )
+
+
+class TestRequestTokens(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ """Set up a dummy person and OAuth consumer."""
+ super(TestRequestTokens, self).setUp()
+
+ self.person = self.factory.makePerson()
+ self.consumer = self.factory.makeOAuthConsumer()
+
+ now = datetime.now(pytz.timezone('UTC'))
+ self.a_long_time_ago = now - timedelta(hours=1000)
+
+ def testExpiredRequestTokenCantBeReviewed(self):
+ """An expired request token can't be reviewed."""
+ token = self.factory.makeOAuthRequestToken(
+ date_created=self.a_long_time_ago)
+ self.assertRaises(
+ AssertionError, token.review, self.person,
+ OAuthPermission.WRITE_PUBLIC)
+
+ def testExpiredRequestTokenCantBeExchanged(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.
+ """
+ token = self.factory.makeOAuthRequestToken(
+ date_created=self.a_long_time_ago, reviewed_by=self.person)
+ self.assertRaises(AssertionError, token.createAccessToken)
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2010-09-28 07:00:56 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2010-10-14 14:26:09 +0000
@@ -8,7 +8,6 @@
__metaclass__ = type
import cgi
-from datetime import datetime
import threading
import xmlrpclib
@@ -1277,10 +1276,9 @@
token.checkNonceAndTimestamp(nonce, timestamp)
except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e:
raise Unauthorized('Invalid nonce/timestamp: %s' % e)
- now = datetime.now(pytz.timezone('UTC'))
if token.permission == OAuthPermission.UNAUTHORIZED:
raise Unauthorized('Unauthorized token (%s).' % token.key)
- elif token.date_expires is not None and token.date_expires <= now:
+ elif token.is_expired:
raise Unauthorized('Expired token (%s).' % token.key)
elif not check_oauth_signature(request, consumer, token):
raise Unauthorized('Invalid signature.')
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-10-06 18:53:53 +0000
+++ lib/lp/testing/factory.py 2010-10-14 14:26:09 +0000
@@ -79,6 +79,7 @@
EmailAddressStatus,
IEmailAddressSet,
)
+from canonical.launchpad.interfaces.oauth import IOAuthConsumerSet
from canonical.launchpad.interfaces.gpghandler import IGPGHandler
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
@@ -91,6 +92,7 @@
DEFAULT_FLAVOR,
IStoreSelector,
MAIN_STORE,
+ OAuthPermission,
)
from lp.app.enums import ServiceUsage
from lp.archiveuploader.dscfile import DSCFile
@@ -3175,6 +3177,33 @@
to_source=to_source, date_fulfilled=date_fulfilled,
status=status, diff_content=lfa))
+ # Factory methods for OAuth tokens.
+ def makeOAuthConsumer(self, key=None, secret=None):
+ if key is None:
+ key = self.getUniqueString("oauthconsumerkey")
+ if secret is None:
+ secret = ''
+ return getUtility(IOAuthConsumerSet).new(key, secret)
+
+ 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()
+ token = consumer.newRequestToken()
+
+ if reviewed_by is not None:
+ # Review the token before modifying the date_created,
+ # since the date_created can be used to simulate an
+ # expired token.
+ token.review(reviewed_by, access_level)
+
+ if date_created is not None:
+ unwrapped_token = removeSecurityProxy(token)
+ unwrapped_token.date_created = date_created
+ return token
+
# Some factory methods return simple Python types. We don't add
# security wrappers for them, as well as for objects created by
