launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/oauth-secret-sha256-4-lyf into lp:launchpad with lp:~wgrant/launchpad/oauth-secret-sha256 as a prerequisite.

Commit message:
Drop support for plaintext OAuth secrets in the DB.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-sha256-4-lyf/+merge/224786

Drop support for plaintext OAuth secrets in the DB. Pretty boring.

This also needs sampledata updates that I've omitted here to keep the diff sane.
-- 
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-sha256-4-lyf/+merge/224786
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/oauth-secret-sha256-4-lyf into lp:launchpad.

=== modified file 'lib/lp/services/oauth/model.py'
--- lib/lp/services/oauth/model.py	2014-06-27 10:37:59 +0000
+++ lib/lp/services/oauth/model.py	2014-06-27 10:37:59 +0000
@@ -44,7 +44,6 @@
 from lp.services.database.interfaces import IMasterStore
 from lp.services.database.sqlbase import SQLBase
 from lp.services.database.stormbase import StormBase
-from lp.services.features import getFeatureFlag
 from lp.services.librarian.model import LibraryFileAlias
 from lp.services.oauth.interfaces import (
     ClockSkew,
@@ -117,8 +116,7 @@
     _secret = StringCol(dbName="secret", notNull=False, default='')
 
     def __init__(self, key, secret):
-        if getFeatureFlag('auth.hash_oauth_secrets'):
-            secret = hashlib.sha256(secret).hexdigest()
+        secret = hashlib.sha256(secret).hexdigest()
         super(OAuthConsumer, self).__init__(key=key, _secret=secret)
 
     # This regular expression singles out a consumer key that
@@ -180,7 +178,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
+        return hashlib.sha256(secret).hexdigest() == self._secret
 
     def newRequestToken(self):
         """See `IOAuthConsumer`."""
@@ -242,8 +240,7 @@
     def __init__(self, consumer, permission, key, secret='', person=None,
                  date_expires=None, product=None, project=None,
                  distribution=None, sourcepackagename=None):
-        if getFeatureFlag('auth.hash_oauth_secrets'):
-            secret = hashlib.sha256(secret).hexdigest()
+        secret = hashlib.sha256(secret).hexdigest()
         super(OAuthAccessToken, self).__init__(
             consumer=consumer, permission=permission, key=key,
             _secret=secret, person=person, date_expires=date_expires,
@@ -273,7 +270,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
+        return hashlib.sha256(secret).hexdigest() == self._secret
 
     def checkNonceAndTimestamp(self, nonce, timestamp):
         """See `IOAuthAccessToken`."""
@@ -341,8 +338,7 @@
     def __init__(self, consumer, key, secret='', permission=None, person=None,
                  date_expires=None, product=None, project=None,
                  distribution=None, sourcepackagename=None):
-        if getFeatureFlag('auth.hash_oauth_secrets'):
-            secret = hashlib.sha256(secret).hexdigest()
+        secret = hashlib.sha256(secret).hexdigest()
         super(OAuthRequestToken, self).__init__(
             consumer=consumer, permission=permission, key=key,
             _secret=secret, person=person, date_expires=date_expires,
@@ -373,7 +369,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
+        return hashlib.sha256(secret).hexdigest() == self._secret
 
     def review(self, user, permission, context=None, date_expires=None):
         """See `IOAuthRequestToken`."""

=== modified file 'lib/lp/services/oauth/tests/test_tokens.py'
--- lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 10:37:59 +0000
+++ lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 10:37:59 +0000
@@ -14,13 +14,11 @@
 import hashlib
 
 import pytz
-import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.services.features.testing import FeatureFixture
-from lp.services.mail import stub
 from lp.services.oauth.interfaces import (
     IOAuthAccessToken,
     IOAuthConsumer,
@@ -107,19 +105,18 @@
 class TestRequestTokens(TestOAuth):
     """Tests for OAuth request token objects."""
 
-    secret_len = 80
-
     def test_newRequestToken(self):
         request_token, secret = self.consumer.newRequestToken()
         verifyObject(IOAuthRequestToken, request_token)
         self.assertIsInstance(secret, unicode)
-        self.assertEqual(removeSecurityProxy(request_token)._secret, secret)
+        self.assertEqual(
+            removeSecurityProxy(request_token)._secret,
+            hashlib.sha256(secret).hexdigest())
 
     def test_key_and_secret_automatically_generated(self):
         request_token, secret = self.consumer.newRequestToken()
         self.assertEqual(len(request_token.key), 20)
-        self.assertEqual(
-            len(removeSecurityProxy(request_token)._secret), self.secret_len)
+        self.assertEqual(len(removeSecurityProxy(request_token)._secret), 64)
 
     def test_date_created(self):
         request_token, _ = self.consumer.newRequestToken()
@@ -268,23 +265,6 @@
         self.assertEquals(person.oauth_request_tokens.count(), 0)
 
 
-class TestRequestTokensHashed(TestRequestTokens):
-
-    secret_len = 64
-
-    def setUp(self):
-        super(TestRequestTokensHashed, self).setUp()
-        self.useFixture(FeatureFixture({'auth.hash_oauth_secrets': 'on'}))
-
-    def test_newRequestToken(self):
-        request_token, secret = self.consumer.newRequestToken()
-        verifyObject(IOAuthRequestToken, request_token)
-        self.assertIsInstance(secret, unicode)
-        self.assertEqual(
-            removeSecurityProxy(request_token)._secret,
-            hashlib.sha256(secret).hexdigest())
-
-
 class TestAccessTokens(TestOAuth):
     """Tests for OAuth access tokens."""
 
@@ -306,14 +286,8 @@
         verifyObject(IOAuthAccessToken, access_token)
         self.assertIsInstance(access_secret, unicode)
         self.assertEqual(
-            removeSecurityProxy(access_token)._secret, access_secret)
-
-        # Make sure the security notification email went out that the new
-        # token was created.
-        transaction.commit()
-        from_addr, to_addr, msg = stub.test_emails.pop()
-        self.assertIn('OAuth token generated', msg)
-        self.assertIn('@example.com', to_addr[0])
+            removeSecurityProxy(access_token)._secret,
+            hashlib.sha256(access_secret).hexdigest())
 
     def test_access_token_inherits_data_fields_from_request_token(self):
         request_token, access_token, _ = (
@@ -411,24 +385,6 @@
         self.assertEquals(person.oauth_access_tokens.count(), 0)
 
 
-class TestAccessTokensHashed(TestAccessTokens):
-
-    def setUp(self):
-        super(TestAccessTokensHashed, self).setUp()
-        self.useFixture(FeatureFixture({'auth.hash_oauth_secrets': 'on'}))
-
-    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, access_secret = (
-            self._exchange_request_token_for_access_token())
-        verifyObject(IOAuthAccessToken, access_token)
-        self.assertIsInstance(access_secret, unicode)
-        self.assertEqual(
-            removeSecurityProxy(access_token)._secret,
-            hashlib.sha256(access_secret).hexdigest())
-
-
 class TestHelperFunctions(TestOAuth):
 
     def setUp(self):