launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/break-on-invalid-oauth into lp:maas with lp:~allenap/maas/describe-anon-handlers as a prerequisite.

Commit message:
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.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1058316 in MAAS: "maas-cli reports unknown operation if you're logged in with the wrong credentials"
  https://bugs.launchpad.net/maas/+bug/1058316

For more details, see:
https://code.launchpad.net/~allenap/maas/break-on-invalid-oauth/+merge/128377
-- 
https://code.launchpad.net/~allenap/maas/break-on-invalid-oauth/+merge/128377
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/break-on-invalid-oauth into lp:maas.

=== 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-07 09:54:23 +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-07 09:54:23 +0000
+++ src/maasserver/tests/test_api.py	2012-10-07 09:54:23 +0000
@@ -221,6 +221,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 MultipleUsersScenarios:
     """A mixin that uses testscenarios to repeat a testcase as different
     users.