Merge lp:~lukasz-czyzykowski/rnr-server/fix-duplicate-consumer into lp:rnr-server

[Łukasz Czyżykowski]

[r=noodels][bug=706035] Workaround for Bug:706035 to enable access to api when there is more than one oauth consumer with the same consumer_key.

Overview
========
This branch works around the problem of having duplicate oauth consumers in the database with the same consumer_key.

Details
=======
In principle the situation when there is more than one oauth consumer with the same consumer_key should never happen. But because it had happened on our servers the workaround proposed here is to:

- not fail miserably, when trying to use api using oauth consumer key when such bad situation had happened,

- clear the situation, so future access will be quicker and saner.

Testing
=======
Follow `README` instructions to setup environment and then issue `make test`.

[Michael Nelson (michael.nelson)]

09:36 <@noodles> lukasz: with your branch, the code change looks fine, but I think there should be a comment in the code along the lines of "There should not be more than one, but so that we don't error..."...
09:36 < lukasz> k
09:36 <@noodles> and, I wonder if it would be worth logging the deletion when it occurs?
09:36 <@noodles> (so that we can eventually track it down?)
09:37 < lukasz> I wondered about that
09:37 < lukasz> or just leave alone
09:37 < lukasz> the code can cope with that
09:38 < lukasz> noodles: the deletion was achuni's idea
09:38 <@noodles> Hrm... are we certain that the duplicates are always for the same user? (based on the consumer_key, they have to be?)
09:38 < lukasz> noodles: no, they can't be fore the same user (that's a unique thing for Consumers)
09:39 * noodles tries to re-read that statement
09:39 < lukasz> but, same person can have two django users
09:39 < lukasz> in my opinion this can only happen if you switch open_id providers
09:40 <@noodles> Ah - between staging/production?
09:40 < lukasz> but I'm not sure
09:40 < lukasz> yes
09:40 <@noodles> Which would fit, time-wise too.
09:41 <@noodles> OK, lets put that knowledge in the comment with the code, so that if we come back to it in 6months, we can understand why we did it?
09:41 < lukasz> maybe just switch that to log such situation and leave it alone
09:41 <@noodles> (although, you've linked to the bug too)
09:41 < lukasz> ok
09:41 <@noodles> +1 for logging and leaving (so we can always investigate)
09:42 <@noodles> if our assumptions turn out to be false...
09:43 < lukasz> ok

[lp:~lukasz-czyzykowski/rnr-server/fix-duplicate-consumer]

153. By Łukasz Czyżykowski on 2011-03-17

Moved logic for finding Consumer object based on the consumer_key to
Consumer class itself, so that other parts of the code can also use it.

154. By Łukasz Czyżykowski on 2011-03-17

Changed buildout from depending on distribute to setuptools because of
current distribute not working too well.

[Michael Nelson (michael.nelson)]

Great, thanks lukasz.

review: Approve

=== modified file 'Makefile'
--- Makefile	2011-02-23 11:12:49 +0000
+++ Makefile	2011-03-17 09:22:24 +0000
@@ -6,7 +6,7 @@
 	bin/buildout
 
 bootstrap:
-	$(PYTHON) bootstrap.py --distribute
+	$(PYTHON) bootstrap.py
 
 port=8000
 run:

=== modified file 'src/reviewsapp/models/oauthtoken.py'
--- src/reviewsapp/models/oauthtoken.py	2010-12-02 18:45:33 +0000
+++ src/reviewsapp/models/oauthtoken.py	2011-03-17 09:22:24 +0000
@@ -31,6 +31,7 @@
 import struct
 import string
 from functools import partial
+from logging import getLogger
 
 from oauth.oauth import OAuthToken, OAuthDataStore, OAuthConsumer
 
@@ -97,6 +98,30 @@
     created_at = models.DateTimeField(auto_now_add=True)
     updated_at = models.DateTimeField(auto_now=True)
 
+    @classmethod
+    def lookup_consumer(self, consumer_key):
+        # lukasz: To my best knowledge the query below can return more than one
+        # entry only if the OpenID provider was switched for already deployed
+        # code (between staging/production SSO). That would mean, that the same
+        # user on SSO will get second Django user, and second Consumer object,
+        # but with exactly the same consumer_key.
+        consumers = Consumer.objects.filter(key=consumer_key)
+        # If more than one Consumer object is found we choose the one created
+        # as the last one keeping up with the assumption of switching the
+        # OpenID providers.
+        consumers = consumers.order_by('-created_at')
+        consumers_count = consumers.count()
+
+        if consumers_count == 0:
+            return None
+        elif consumers_count > 1:
+            getLogger(__name__).info(
+                "consumer_key=%s has %d entries in the database",
+                consumer_key, consumers_count
+            )
+        consumer = consumers[0]
+        return consumer
+
     def __unicode__(self):
         return self.key
 
@@ -119,7 +144,7 @@
         """
         Create new nonce object linked to given consumer and token
         """
-        consumer = Consumer.objects.get(key=consumer_key)
+        consumer = Consumer.lookup_consumer(consumer_key)
         token = consumer.token_set.get(token=token_value)
         return consumer.nonce_set.create(token=token, nonce=nonce)
 
@@ -157,11 +182,10 @@
 
         :returns: OAuthConsumer object
         """
-        try:
-            consumer = Consumer.objects.get(key=consumer_key)
+        consumer = Consumer.lookup_consumer(consumer_key)
+        if consumer is not None:
             return consumer.oauth_consumer()
-        except Consumer.DoesNotExist:
-            return None
+
 
     def lookup_nonce(self, consumer, token, nonce):
         """

=== modified file 'src/reviewsapp/tests/test_auth.py'
--- src/reviewsapp/tests/test_auth.py	2011-02-23 10:20:41 +0000
+++ src/reviewsapp/tests/test_auth.py	2011-03-17 09:22:24 +0000
@@ -38,6 +38,7 @@
 from reviewsapp.tests.factory import TestCaseWithFactory
 from reviewsapp.tests.helpers import mock_data_for_account
 
+
 class MockRequest:
     def __init__(self, method='GET'):
         self.META = {'QUERY_STRING': ''}
@@ -253,6 +254,30 @@
         user = User.objects.get(id=user.id)
         self.assertEqual('Hazel Jennet', user.get_full_name())
 
+    @patch('reviewsapp.utilities.WebServices.get_data_for_account')
+    def test_auth_succeedes_when_more_than_one_consumer(self, mock_get_data):
+        """
+        Check that auth works even when there are two consumers for one token
+
+        Bug: #706035
+        
+        """
+        user1 = self.factory.makeUser()
+        user2 = self.factory.makeUser()
+
+        token1, consumer1 = self.factory.makeOAuthTokenAndConsumer(user=user1)
+        token2, consumer2 = self.factory.makeOAuthTokenAndConsumer(user=user2)
+        consumer2.key = consumer1.key
+        consumer2.save()
+
+        request = MockRequest()
+        request.META.update(
+            self.header_from_token(request, token2, consumer2))
+        mock_get_data.return_value = mock_data_for_account(consumer2, token2)
+
+        response = self.get_resource(request)
+        self.assertEquals(response.status_code, 200)
+
     def make_request_with_token(self, save=True, method='GET'):
         token, consumer = self.factory.makeOAuthTokenAndConsumer(save=save)
         request = MockRequest(method=method)