launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/describe-anon-handlers into lp:maas with lp:~allenap/maas/walk-django-resolver as a prerequisite.

Commit message:
Describe anonymous fallback handlers in the API resource descriptions.

Previously there was no distinction made between anonymous and non-anonymous handlers, and no mention of how they related was included in the API description document.

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/describe-anon-handlers/+merge/128366
-- 
https://code.launchpad.net/~allenap/maas/describe-anon-handlers/+merge/128366
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/describe-anon-handlers into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-06 22:52:22 +0000
+++ src/maasserver/api.py	2012-10-06 22:52:22 +0000
@@ -1729,8 +1729,15 @@
             for resource in find_api_resources(urlconf)
             ],
         }
-    # For backward compatibility, add "handlers" as an alias for "resources".
-    description["handlers"] = description["resources"]
+    # For backward compatibility, add "handlers" as an alias for all not-None
+    # anon and auth handlers in "resources".
+    description["handlers"] = []
+    description["handlers"].extend(
+        resource["anon"] for resource in description["resources"]
+        if resource["anon"] is not None)
+    description["handlers"].extend(
+        resource["auth"] for resource in description["resources"]
+        if resource["auth"] is not None)
     return HttpResponse(
         json.dumps(description),
         content_type="application/json")

=== modified file 'src/maasserver/apidoc.py'
--- src/maasserver/apidoc.py	2012-10-06 22:52:22 +0000
+++ src/maasserver/apidoc.py	2012-10-06 22:52:22 +0000
@@ -27,6 +27,7 @@
     RegexURLPattern,
     RegexURLResolver,
     )
+from piston.authentication import NoAuthentication
 from piston.doc import generate_doc
 from piston.handler import BaseHandler
 from piston.resource import Resource
@@ -162,4 +163,18 @@
 
     :type resource: :class:`OperationsResource` instance.
     """
-    return describe_handler(resource.handler)
+    authenticate = not any(
+        isinstance(auth, NoAuthentication)
+        for auth in resource.authentication)
+    if authenticate:
+        if resource.anonymous is None:
+            anon = None
+            auth = describe_handler(resource.handler)
+        else:
+            anon = describe_handler(resource.anonymous)
+            auth = describe_handler(resource.handler)
+    else:
+        anon = describe_handler(resource.handler)
+        auth = None
+    name = anon["name"] if auth is None else auth["name"]
+    return {"anon": anon, "auth": auth, "name": name}

=== modified file 'src/maasserver/tests/test_apidoc.py'
--- src/maasserver/tests/test_apidoc.py	2012-10-06 22:52:22 +0000
+++ src/maasserver/tests/test_apidoc.py	2012-10-06 22:52:22 +0000
@@ -34,6 +34,7 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
+from mock import sentinel
 from piston.doc import HandlerDocumentation
 from piston.handler import BaseHandler
 from piston.resource import Resource
@@ -158,6 +159,12 @@
         return ("megadeth_view", ["vic", "rattlehead"])
 
 
+class RattleheadsHandler(OperationsHandler):
+    """Cover band, maybe."""
+
+    create = read = delete = update = None
+
+
 class TestDescribingAPI(TestCase):
     """Tests for functions that describe a Piston API."""
 
@@ -225,10 +232,40 @@
             "http://example.com/api/1.0/nodes/{system_id}/";,
             description["uri"])
 
-    def test_describe_resource(self):
-        # describe_resource() returns a description of a resource. Right now
-        # it is just the description of the resource's handler class.
+    def test_describe_resource_anonymous_resource(self):
+        # When the resource does not require authentication, any configured
+        # fallback is ignored, and only the resource's handler is described.
+        # The resource name comes from this handler.
+        self.patch(MegadethHandler, "anonymous", RattleheadsHandler)
         resource = OperationsResource(MegadethHandler)
-        self.assertEqual(
-            describe_handler(MegadethHandler),
-            describe_resource(resource))
+        expected = {
+            "anon": describe_handler(MegadethHandler),
+            "auth": None,
+            "name": "MegadethHandler",
+            }
+        self.assertEqual(expected, describe_resource(resource))
+
+    def test_describe_resource_authenticated_resource(self):
+        # When the resource requires authentication, but has no fallback
+        # anonymous handler, the first is described. The resource name comes
+        # from this handler.
+        resource = OperationsResource(MegadethHandler, sentinel.auth)
+        expected = {
+            "anon": None,
+            "auth": describe_handler(MegadethHandler),
+            "name": "MegadethHandler",
+            }
+        self.assertEqual(expected, describe_resource(resource))
+
+    def test_describe_resource_authenticated_resource_with_fallback(self):
+        # When the resource requires authentication, but has a fallback
+        # anonymous handler, both are described. The resource name is taken
+        # from the authenticated handler.
+        self.patch(MegadethHandler, "anonymous", RattleheadsHandler)
+        resource = OperationsResource(MegadethHandler, sentinel.auth)
+        expected = {
+            "anon": describe_handler(RattleheadsHandler),
+            "auth": describe_handler(MegadethHandler),
+            "name": "MegadethHandler",
+            }
+        self.assertEqual(expected, describe_resource(resource))