Merge lp:~allenap/maas/break-on-invalid-oauth into lp:~maas-committers/maas/trunk

[Gavin Panella]

Reject invalid OAuth requests instead of silently downgrading them.

Previously no mention would be made of invalid OAuth credentials, and requests would be silently downgraded to anonymous, with puzzling results.

[Julian Edwards (julian-edwards)]

Perfick.

review: Approve

[MAAS Lander (maas-lander)]

Attempt to merge into lp:maas failed due to conflicts:

text conflict in src/maasserver/tests/test_api.py

=== modified file 'src/maasserver/api_auth.py'
--- src/maasserver/api_auth.py	2012-04-16 10:00:51 +0000
+++ src/maasserver/api_auth.py	2012-10-08 11:20:28 +0000
@@ -14,22 +14,53 @@
     'api_auth',
     ]
 
-from piston.authentication import OAuthAuthentication
+from maasserver.exceptions import Unauthorized
+from oauth import oauth
+from piston.authentication import (
+    OAuthAuthentication,
+    send_oauth_error,
+    )
 from piston.utils import rc
 
 
+class OAuthUnauthorized(Unauthorized):
+    """Unauthorized error for OAuth signed requests with invalid tokens."""
+
+    def __init__(self, error):
+        super(OAuthUnauthorized, self).__init__()
+        self.error = error
+
+    def make_http_response(self):
+        return send_oauth_error(self.error)
+
+
 class MAASAPIAuthentication(OAuthAuthentication):
-    """A piston authentication class that uses the currently logged-in user
-    if there is one, and defaults to piston's OAuthAuthentication if not.
+    """Use the currently logged-in user; resort to OAuth if there isn't one.
 
+    There may be a user already logged-in via another mechanism, like a
+    familiar in-browser user/pass challenge.
     """
 
     def is_authenticated(self, request):
         if request.user.is_authenticated():
             return request.user
-        else:
-            return super(
-                MAASAPIAuthentication, self).is_authenticated(request)
+
+        # The following is much the same as is_authenticated from Piston's
+        # OAuthAuthentication, with the difference that an OAuth request that
+        # does not validate is rejected instead of being silently downgraded.
+        if self.is_valid_request(request):
+            try:
+                consumer, token, parameters = self.validate_token(request)
+            except oauth.OAuthError as error:
+                raise OAuthUnauthorized(error)
+
+            if consumer and token:
+                request.user = token.user
+                request.consumer = consumer
+                request.throttle_extra = token.consumer.id
+                return True
+
+        return False
 
     def challenge(self):
         # Beware: this returns 401: Unauthorized, not 403: Forbidden

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-08 10:32:48 +0000
+++ src/maasserver/tests/test_api.py	2012-10-08 11:20:28 +0000
@@ -225,6 +225,23 @@
         self.assertEqual([data_value], results.getlist(key))
 
 
+class TestAuthentication(APIv10TestMixin, TestCase):
+    """Tests for `maasserver.api_auth`."""
+
+    def test_invalid_oauth_request(self):
+        # An OAuth-signed request that does not validate is an error.
+        user = factory.make_user()
+        client = OAuthAuthenticatedClient(user)
+        get_auth_tokens(user).delete()  # Delete the user's API keys.
+        response = client.post(self.get_uri('nodes/'), {'op': 'start'})
+        observed = response.status_code, response.content
+        expected = (
+            Equals(httplib.UNAUTHORIZED),
+            Contains("Invalid access token:"),
+            )
+        self.assertThat(observed, MatchesListwise(expected))
+
+
 class TestStoreNodeParameters(TestCase):
     """Tests for `store_node_power_parameters`."""