Merge lp:~rvb/maas/oauth-retry-bug-1435767 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Delete the nonce before retrying requests.

- I've used the oauth_request.get_parameter() methods in delete_oauth_nonce instead of using the similar methods that return the Consumer and the Token object in order to minimize the number of database queries: the only thing we're interested in here is the Nonce object.

- I realize the testing done in test__get_response_deleted_nonces_across_requests is a bit fragile because it assumes a lot of stuff about the internals but that's the best I could come up with in a testing environment.

[Gavin Panella (allenap)]

Cool.

review: Approve

=== modified file 'src/maasserver/utils/tests/test_views.py'
--- src/maasserver/utils/tests/test_views.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/utils/tests/test_views.py	2015-04-02 11:03:19 +0000
@@ -34,10 +34,11 @@
 from django.http import HttpResponse
 from django.http.response import HttpResponseServerError
 from fixtures import FakeLogger
+#from maastesting.factory import factory
+from maasserver.testing.factory import factory
 from maasserver.testing.testcase import SerializationFailureTestCase
 from maasserver.utils import views
 from maasserver.utils.orm import validate_in_transaction
-from maastesting.factory import factory
 from maastesting.matchers import (
     MockCalledOnceWith,
     MockCallsMatch,
@@ -49,6 +50,8 @@
     call,
     sentinel,
 )
+from piston.authentication import initialize_server_request
+from piston.models import Nonce
 from testtools.matchers import (
     Contains,
     Equals,
@@ -57,17 +60,27 @@
     IsInstance,
     Not,
 )
+from testtools.testcase import ExpectedException
 from twisted.internet.task import Clock
 from twisted.python import log
 from twisted.web import wsgi
 
 
-def make_request():
+def make_request(env=None, oauth_env=None):
     # Return a minimal WSGIRequest.
-    return WSGIRequest({
+    if oauth_env is None:
+        oauth_env = {}
+    base_env = {
         "REQUEST_METHOD": "GET",
         "wsgi.input": wsgi._InputStream(io.BytesIO()),
-    })
+        "SERVER_NAME": "server",
+        "SERVER_PORT": 80,
+        "HTTP_AUTHORIZATION": factory.make_oauth_header(**oauth_env),
+    }
+    if env is not None:
+        base_env.update(env)
+    request = WSGIRequest(base_env)
+    return request
 
 
 class TestLogFunctions(MAASTestCase):
@@ -121,6 +134,42 @@
         self.assertEqual({}, request.COOKIES)
 
 
+class TestDeleteOAuthNonce(MAASTestCase):
+    """Tests for :py:func:`maasserver.utils.views.delete_oauth_nonce`."""
+
+    def test__deletes_nonce(self):
+        oauth_consumer_key = factory.make_string(18)
+        oauth_token = factory.make_string(18)
+        oauth_nonce = randint(0, 99999)
+        Nonce.objects.create(
+            consumer_key=oauth_consumer_key, token_key=oauth_token,
+            key=oauth_nonce)
+        oauth_env = {
+            'oauth_consumer_key': oauth_consumer_key,
+            'oauth_token': oauth_token,
+            'oauth_nonce': oauth_nonce,
+        }
+        request = make_request(oauth_env=oauth_env)
+        views.delete_oauth_nonce(request)
+        with ExpectedException(Nonce.DoesNotExist):
+            Nonce.objects.get(
+                consumer_key=oauth_consumer_key, token_key=oauth_token,
+                key=oauth_nonce)
+
+    def test__skips_missing_nonce(self):
+        oauth_consumer_key = factory.make_string(18)
+        oauth_token = factory.make_string(18)
+        oauth_nonce = randint(0, 99999)
+        oauth_env = {
+            'oauth_consumer_key': oauth_consumer_key,
+            'oauth_token': oauth_token,
+            'oauth_nonce': oauth_nonce,
+        }
+        request = make_request(oauth_env=oauth_env)
+        # No exception is raised.
+        self.assertIsNone(views.delete_oauth_nonce(request))
+
+
 class TestWebApplicationHandler(SerializationFailureTestCase):
 
     def setUp(self):
@@ -357,8 +406,44 @@
             'CONTENT_LENGTH': headers['Content-Length'],
             'HTTP_MIME_VERSION': headers['MIME-Version'],
         }
-        request = WSGIRequest(env)
+        request = make_request(env)
 
         response = handler.get_response(request)
         self.assertEqual(file_content, response.content)
         self.assertEqual(recorder, [file_content] * 3)
+
+    def test__get_response_deleted_nonces_across_requests(self):
+        handler = views.WebApplicationHandler(3)
+        user = factory.make_User()
+        token = user.userprofile.get_authorisation_tokens()[0]
+
+        recorder = []
+
+        def get_response_check_nonce(self, request):
+            _, oauth_req = initialize_server_request(request)
+            # get_or _create the Nonce object like the authentication
+            # mechanism does.
+            nonce_obj, created = Nonce.objects.get_or_create(
+                consumer_key=token.consumer.key, token_key=token.key,
+                key=oauth_req.get_parameter('oauth_nonce'))
+
+            # Record calls.
+            recorder.append(created)
+            response = HttpResponse(
+                content='',
+                status=httplib.OK,
+                mimetype=b"text/plain; charset=utf-8")
+            handler._WebApplicationHandler__retry.add(response)
+            return response
+
+        self.patch(
+            WSGIHandler, "get_response", get_response_check_nonce)
+
+        oauth_env = {
+            'oauth_consumer_key': token.consumer.key,
+            'oauth_token': token.key,
+        }
+        request = make_request(oauth_env=oauth_env)
+
+        handler.get_response(request)
+        self.assertEqual(recorder, [True] * 3, "Nonce hasn't been cleaned up!")

=== modified file 'src/maasserver/utils/views.py'
--- src/maasserver/utils/views.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/utils/views.py	2015-04-02 11:03:19 +0000
@@ -31,6 +31,8 @@
     is_serialization_failure,
     post_commit_hooks,
 )
+from piston.authentication import initialize_server_request
+from piston.models import Nonce
 from provisioningserver.utils.twisted import retries
 from twisted.internet import reactor as clock
 from twisted.python import log
@@ -56,6 +58,19 @@
         attempt, request.path, elapsed)
 
 
+def delete_oauth_nonce(request):
+    """Delete the OAuth nonce for the given request from the database.
+
+    This is to allow the exact same request to be retried.
+    """
+    oauth_server, oauth_request = initialize_server_request(request)
+    consumer_key = oauth_request.get_parameter('oauth_consumer_key')
+    token_key = oauth_request.get_parameter('oauth_token')
+    nonce = oauth_request.get_parameter('oauth_nonce')
+    Nonce.objects.filter(
+        consumer_key=consumer_key, token_key=token_key, key=nonce).delete()
+
+
 def reset_request(request):
     """Return a pristine new request object.
 
@@ -172,6 +187,7 @@
                     return response
                 # We'll retry after a brief interlude.
                 log_failed_attempt(request, attempt, elapsed, remaining, wait)
+                delete_oauth_nonce(request)
                 request = reset_request(request)
                 sleep(wait)
             else: