launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Remove the secret from our OAuth interfaces, and fix tests to not assume they can retrieve the secret for an existing token. Tokens will soon be hashed, making such retrieval impossible.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-concealment/+merge/224777

Remove the secret from our OAuth interfaces, and fix tests to not assume they can retrieve the secret for an existing token. Tokens will soon be hashed, making such retrieval impossible.
-- 
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-concealment/+merge/224777
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/oauth-secret-concealment into lp:launchpad.

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2014-04-25 11:33:45 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2014-06-27 09:32:08 +0000
@@ -1208,4 +1208,4 @@
         store.invalidate()
         with StormStatementRecorder() as recorder:
             webservice.get(url)
-        self.assertThat(recorder, HasQueryCount(Equals(28)))
+        self.assertThat(recorder, HasQueryCount(Equals(26)))

=== modified file 'lib/lp/services/oauth/browser/__init__.py'
--- lib/lp/services/oauth/browser/__init__.py	2014-06-27 09:32:08 +0000
+++ lib/lp/services/oauth/browser/__init__.py	2014-06-27 09:32:08 +0000
@@ -104,8 +104,7 @@
                 ]
             return self.getJSONRepresentation(
                 permissions, token, secret=secret)
-        return u'oauth_token=%s&oauth_token_secret=%s' % (
-            token.key, token.secret)
+        return u'oauth_token=%s&oauth_token_secret=%s' % (token.key, secret)
 
 
 def token_exists_and_is_not_reviewed(form, action):

=== modified file 'lib/lp/services/oauth/interfaces.py'
--- lib/lp/services/oauth/interfaces.py	2014-06-27 09:32:08 +0000
+++ lib/lp/services/oauth/interfaces.py	2014-06-27 09:32:08 +0000
@@ -61,10 +61,6 @@
         title=_('Key'), required=True, readonly=True,
         description=_('The unique key which identifies a consumer. It is '
                       'included by the consumer in each request made.'))
-    secret = TextLine(
-        title=_('Secret'), required=False, readonly=False,
-        description=_('The secret which, if not empty, should be used by the '
-                      'consumer to sign its requests.'))
 
     is_integrated_desktop = Attribute(
         """This attribute is true if the consumer corresponds to a
@@ -155,10 +151,6 @@
         title=_('Key'), required=True, readonly=True,
         description=_('The key used to identify this token.  It is included '
                       'by the consumer in each request.'))
-    secret = TextLine(
-        title=_('Secret'), required=True, readonly=True,
-        description=_('The secret associated with this token.  It is used '
-                      'by the consumer to sign its requests.'))
     product = Choice(title=_('Project'), required=False, vocabulary='Product')
     project = Choice(
         title=_('Project'), required=False, vocabulary='ProjectGroup')

=== modified file 'lib/lp/services/oauth/model.py'
--- lib/lp/services/oauth/model.py	2014-06-27 09:32:08 +0000
+++ lib/lp/services/oauth/model.py	2014-06-27 09:32:08 +0000
@@ -112,7 +112,7 @@
     date_created = UtcDateTimeCol(default=UTC_NOW, notNull=True)
     disabled = BoolCol(notNull=True, default=False)
     key = StringCol(notNull=True)
-    secret = StringCol(notNull=False, default='')
+    _secret = StringCol(dbName="secret", notNull=False, default='')
 
     # This regular expression singles out a consumer key that
     # represents any and all apps running on a specific computer. The
@@ -173,13 +173,13 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self.secret
+        return secret == self._secret
 
     def newRequestToken(self):
         """See `IOAuthConsumer`."""
         key, secret = create_token_key_and_secret(table=OAuthRequestToken)
         return (
-            OAuthRequestToken(consumer=self, key=key, secret=secret), secret)
+            OAuthRequestToken(consumer=self, key=key, _secret=secret), secret)
 
     def getAccessToken(self, key):
         """See `IOAuthConsumer`."""
@@ -198,7 +198,7 @@
         """See `IOAuthConsumerSet`."""
         assert self.getByKey(key) is None, (
             "The key '%s' is already in use by another consumer." % key)
-        return OAuthConsumer(key=key, secret=secret)
+        return OAuthConsumer(key=key, _secret=secret)
 
     def getByKey(self, key):
         """See `IOAuthConsumerSet`."""
@@ -216,7 +216,7 @@
     date_created = UtcDateTimeCol(default=UTC_NOW, notNull=True)
     date_expires = UtcDateTimeCol(notNull=False, default=None)
     key = StringCol(notNull=True)
-    secret = StringCol(notNull=False, default='')
+    _secret = StringCol(dbName="secret", notNull=False, default='')
 
     permission = EnumCol(enum=AccessLevel, notNull=True)
 
@@ -255,7 +255,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self.secret
+        return secret == self._secret
 
     def checkNonceAndTimestamp(self, nonce, timestamp):
         """See `IOAuthAccessToken`."""
@@ -303,7 +303,7 @@
     date_created = UtcDateTimeCol(default=UTC_NOW, notNull=True)
     date_expires = UtcDateTimeCol(notNull=False, default=None)
     key = StringCol(notNull=True)
-    secret = StringCol(notNull=False, default='')
+    _secret = StringCol(dbName="secret", notNull=False, default='')
 
     permission = EnumCol(enum=OAuthPermission, notNull=False, default=None)
     date_reviewed = UtcDateTimeCol(default=None, notNull=False)
@@ -344,7 +344,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self.secret
+        return secret == self._secret
 
     def review(self, user, permission, context=None, date_expires=None):
         """See `IOAuthRequestToken`."""
@@ -389,7 +389,7 @@
         access_level = AccessLevel.items[self.permission.name]
         access_token = OAuthAccessToken(
             consumer=self.consumer, person=self.person, key=key,
-            secret=secret, permission=access_level,
+            _secret=secret, permission=access_level,
             date_expires=self.date_expires, product=self.product,
             project=self.project, distribution=self.distribution,
             sourcepackagename=self.sourcepackagename)

=== modified file 'lib/lp/services/oauth/tests/test_tokens.py'
--- lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 09:32:08 +0000
+++ lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 09:32:08 +0000
@@ -16,6 +16,7 @@
 import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 
 from lp.services.mail import stub
 from lp.services.oauth.interfaces import (
@@ -108,12 +109,12 @@
         request_token, secret = self.consumer.newRequestToken()
         verifyObject(IOAuthRequestToken, request_token)
         self.assertIsInstance(secret, unicode)
-        self.assertEqual(request_token.secret, secret)
+        self.assertEqual(removeSecurityProxy(request_token)._secret, secret)
 
     def test_key_and_secret_automatically_generated(self):
         request_token, secret = self.consumer.newRequestToken()
         self.assertEqual(len(request_token.key), 20)
-        self.assertEqual(len(request_token.secret), 80)
+        self.assertEqual(len(removeSecurityProxy(request_token)._secret), 80)
 
     def test_date_created(self):
         request_token, _ = self.consumer.newRequestToken()
@@ -282,7 +283,8 @@
             self._exchange_request_token_for_access_token())
         verifyObject(IOAuthAccessToken, access_token)
         self.assertIsInstance(access_secret, unicode)
-        self.assertEqual(access_token.secret, access_secret)
+        self.assertEqual(
+            removeSecurityProxy(access_token)._secret, access_secret)
 
         # Make sure the security notification email went out that the new
         # token was created.

=== modified file 'lib/lp/soyuz/tests/test_livefs.py'
--- lib/lp/soyuz/tests/test_livefs.py	2014-06-18 18:29:13 +0000
+++ lib/lp/soyuz/tests/test_livefs.py	2014-06-27 09:32:08 +0000
@@ -659,4 +659,4 @@
         store.invalidate()
         with StormStatementRecorder() as recorder:
             self.webservice.get(url)
-        self.assertThat(recorder, HasQueryCount(Equals(21)))
+        self.assertThat(recorder, HasQueryCount(Equals(19)))

=== modified file 'lib/lp/testing/pages.py'
--- lib/lp/testing/pages.py	2014-06-27 09:32:08 +0000
+++ lib/lp/testing/pages.py	2014-06-27 09:32:08 +0000
@@ -68,6 +68,12 @@
     stop,
     )
 
+SAMPLEDATA_ACCESS_SECRETS = {
+    'salgado-read-nonprivate': 'secret',
+    'salgado-change-anything': 'test',
+    'nopriv-read-nonprivate': 'mystery',
+    }
+
 
 class UnstickyCookieHTTPCaller(HTTPCaller):
     """HTTPCaller subclass that do not carry cookies across requests.
@@ -113,8 +119,8 @@
     """A class for making calls to Launchpad web services."""
 
     def __init__(self, oauth_consumer_key=None, oauth_access_key=None,
-                 handle_errors=True, domain='api.launchpad.dev',
-                 protocol='http'):
+                 oauth_access_secret=None, handle_errors=True,
+                 domain='api.launchpad.dev', protocol='http'):
         """Create a LaunchpadWebServiceCaller.
         :param oauth_consumer_key: The OAuth consumer key to use.
         :param oauth_access_key: The OAuth access key to use for the request.
@@ -124,39 +130,17 @@
         Other parameters are passed to the HTTPCaller used to make the calls.
         """
         if oauth_consumer_key is not None and oauth_access_key is not None:
-            login(ANONYMOUS)
-            consumers = getUtility(IOAuthConsumerSet)
-            self.consumer = consumers.getByKey(oauth_consumer_key)
-            if oauth_access_key == '':
-                # The client wants to make an anonymous request.
-                self.access_token = OAuthToken(oauth_access_key, '')
-                if self.consumer is None:
-                    # The client is trying to make an anonymous
-                    # request with a previously unknown consumer. This
-                    # is fine: we manually create a "fake"
-                    # OAuthConsumer (it's "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, '')
-            else:
-                if self.consumer is None:
-                    # Requests using this caller will be rejected by
-                    # the server, but we have a test that verifies
-                    # such requests _are_ rejected, so we'll create a
-                    # fake OAuthConsumer object.
-                    self.consumer = OAuthConsumer(oauth_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)
+            self.consumer = OAuthConsumer(oauth_consumer_key, '')
+            if oauth_access_secret is None:
+                oauth_access_secret = SAMPLEDATA_ACCESS_SECRETS.get(
+                    oauth_access_key, '')
+            self.access_token = OAuthToken(
+                oauth_access_key, oauth_access_secret)
+            # This shouldn't be here, but many old tests expect it.
             logout()
         else:
             self.consumer = None
             self.access_token = None
-
         self.handle_errors = handle_errors
         WebServiceCaller.__init__(self, handle_errors, domain, protocol)
 
@@ -717,9 +701,10 @@
         consumer = oacs.new(consumer_key)
     request_token, _ = consumer.newRequestToken()
     request_token.review(person, permission, context)
-    access_token, _ = request_token.createAccessToken()
+    access_token, access_secret = request_token.createAccessToken()
     logout()
-    service = LaunchpadWebServiceCaller(consumer_key, access_token.key)
+    service = LaunchpadWebServiceCaller(
+        consumer_key, access_token.key, access_secret)
     service.user = person
     return service