Merge lp:~allenap/maas/api-uri-bug-1059645 into lp:~maas-committers/maas/trunk

[Gavin Panella]

In the API description, make handler URIs absolute with respect to the request.

Previously, DEFAULT_MAAS_URL was always used.

I had to put a lot more work in to satisfy the review comments from jtv and jam, so I'm putting this back up for review.

The biggest change is that describe_handler now sets only a "path" field with the URI template; the describe() API function has to manufacture the "uri" field. I originally was using Django's HttpRequest.build_absolute_uri for this, but the additional tests I added showed that it was returning unexpected (and, imo, fairly useless) things, so I had to create a version that works.

These tests also highlighted that AnonFilesHandler was missing a resource_uri() method.

[John A Meinel (jameinel)]

This tests if the 'scheme' matches appropriately. Would you also want to test a nested case? (so rather than foobar.com/MAAS/1.0/api something like foo.com/bar/MAAS/1.0/api).

Otherwise it seems good to me.

[Jeroen T. Vermeulen (jtv)]

The comment “Make all API URLs absolute” in api.py seems a bit terse. Please leave a note so that some future visitor knows not to let this regress.

In test_handler_uris_are_absolute you make conditional assertions inside nested loops. That has its drawbacks: a failure stops the entire test, and you're not verifying that you're actually exercising the assertions. Better to gather your results and your expectations, and verify equality in one go.

Finally, in test_apidoc.py, why does the URL lack protocol and hostname?

Jeroen

review: Approve

[Raphaël Badin (rvb)]

Looks good to me.

[0]

272 + def make_request(
273 + self, host="example.com", port=80, script_name="", is_secure=False):

Weird formatting here.

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-30 10:58:09 +0000
+++ src/maasserver/api.py	2012-11-02 09:26:19 +0000
@@ -164,6 +164,7 @@
 from maasserver.server_address import get_maas_facing_server_address
 from maasserver.utils import (
     absolute_reverse,
+    build_absolute_uri,
     map_enum,
     strip_domain,
     )
@@ -1021,6 +1022,10 @@
 
     get = operation(idempotent=True, exported_as='get')(get_file)
 
+    @classmethod
+    def resource_uri(cls, *args, **kwargs):
+        return ('files_handler', [])
+
 
 class FilesHandler(OperationsHandler):
     """File management operations."""
@@ -1935,12 +1940,23 @@
     Returns a JSON object describing the whole MAAS API.
     """
     from maasserver import urls_api as urlconf
+    resources = [
+        describe_resource(resource)
+        for resource in find_api_resources(urlconf)
+        ]
+    # Make all URIs absolute. Clients - maas-cli in particular - expect that
+    # all handler URIs are absolute, not just paths. The handler URIs returned
+    # by describe_resource() are relative paths.
+    absolute = partial(build_absolute_uri, request)
+    for resource in resources:
+        for handler_type in "anon", "auth":
+            handler = resource[handler_type]
+            if handler is not None:
+                handler["uri"] = absolute(handler["path"])
+    # Package it all up.
     description = {
         "doc": "MAAS API",
-        "resources": [
-            describe_resource(resource)
-            for resource in find_api_resources(urlconf)
-            ],
+        "resources": resources,
         }
     # For backward compatibility, add "handlers" as an alias for all not-None
     # anon and auth handlers in "resources".

=== modified file 'src/maasserver/apidoc.py'
--- src/maasserver/apidoc.py	2012-10-06 22:42:45 +0000
+++ src/maasserver/apidoc.py	2012-11-02 09:26:19 +0000
@@ -19,9 +19,7 @@
 
 from inspect import getdoc
 from itertools import izip_longest
-from urlparse import urljoin
 
-from django.conf import settings
 from django.core.urlresolvers import (
     get_resolver,
     RegexURLPattern,
@@ -137,11 +135,8 @@
     if isinstance(handler, BaseHandler):
         handler = type(handler)
 
-    uri_template = generate_doc(handler).resource_uri_template
-    if uri_template is None:
-        uri_template = settings.DEFAULT_MAAS_URL
-    else:
-        uri_template = urljoin(settings.DEFAULT_MAAS_URL, uri_template)
+    path = generate_doc(handler).resource_uri_template
+    path = "" if path is None else path
 
     resource_uri = getattr(handler, "resource_uri", lambda: ())
     view_name, uri_params, uri_kw = merge(resource_uri(), (None, (), {}))
@@ -154,7 +149,7 @@
         "doc": getdoc(handler),
         "name": handler.__name__,
         "params": uri_params,
-        "uri": uri_template,
+        "path": path,
         }
 
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-30 10:58:09 +0000
+++ src/maasserver/tests/test_api.py	2012-11-02 09:26:19 +0000
@@ -27,10 +27,12 @@
 import httplib
 from itertools import izip
 import json
+from operator import itemgetter
 import os
 import random
 import shutil
 import sys
+from urlparse import urlparse
 
 from apiclient.maas_client import MAASClient
 from celery.app import app_or_default
@@ -38,9 +40,11 @@
 from django.contrib.auth.models import AnonymousUser
 from django.core.urlresolvers import reverse
 from django.http import QueryDict
+from django.test.client import RequestFactory
 from fixtures import Fixture
 from maasserver import api
 from maasserver.api import (
+    describe,
     DISPLAYED_NODEGROUP_FIELDS,
     extract_constraints,
     extract_oauth_key,
@@ -130,10 +134,15 @@
 from provisioningserver.pxe import tftppath
 from provisioningserver.testing.boot_images import make_boot_image_params
 from testresources import FixtureResource
+from testscenarios import multiply_scenarios
 from testtools.matchers import (
+    AfterPreprocessing,
     AllMatch,
     Contains,
     Equals,
+    Is,
+    MatchesAll,
+    MatchesAny,
     MatchesListwise,
     MatchesStructure,
     StartsWith,
@@ -4349,3 +4358,49 @@
         self.assertSetEqual(
             {"doc", "handlers", "resources"}, set(description))
         self.assertIsInstance(description["handlers"], list)
+
+
+class TestDescribeAbsoluteURIs(AnonAPITestCase):
+    """Tests for the `describe` view's URI manipulation."""
+
+    scenarios_schemes = (
+        ("http", dict(scheme="http")),
+        ("https", dict(scheme="https")),
+        )
+
+    scenarios_paths = (
+        ("script-at-root", dict(script_name="", path_info="")),
+        ("script-below-root-1", dict(script_name="/foo/bar", path_info="")),
+        ("script-below-root-2", dict(script_name="/foo", path_info="/bar")),
+        )
+
+    scenarios = multiply_scenarios(
+        scenarios_schemes, scenarios_paths)
+
+    def test_handler_uris_are_absolute(self):
+        server = factory.make_name("server").lower()
+        extra = {
+            "PATH_INFO": self.path_info,
+            "SCRIPT_NAME": self.script_name,
+            "SERVER_NAME": server,
+            "wsgi.url_scheme": self.scheme,
+            }
+        request = RequestFactory().get(
+            "/%s/describe" % factory.make_name("path"), **extra)
+        response = describe(request)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        description = json.loads(response.content)
+        expected_uri = AfterPreprocessing(
+            urlparse, MatchesStructure(
+                scheme=Equals(self.scheme), hostname=Equals(server),
+                # The path is always the script name followed by "api/"
+                # because all API calls are within the "api" tree.
+                path=StartsWith(self.script_name + "/api/")))
+        expected_handler = MatchesAny(
+            Is(None), AfterPreprocessing(itemgetter("uri"), expected_uri))
+        expected_resource = MatchesAll(
+            AfterPreprocessing(itemgetter("anon"), expected_handler),
+            AfterPreprocessing(itemgetter("auth"), expected_handler))
+        resources = description["resources"]
+        self.assertNotEqual([], resources)
+        self.assertThat(resources, AllMatch(expected_resource))

=== modified file 'src/maasserver/tests/test_apidoc.py'
--- src/maasserver/tests/test_apidoc.py	2012-10-08 10:23:16 +0000
+++ src/maasserver/tests/test_apidoc.py	2012-11-02 09:26:19 +0000
@@ -203,7 +203,7 @@
         observed = describe_handler(ExampleHandler)
         # The description contains several entries.
         self.assertSetEqual(
-            {"actions", "doc", "name", "params", "uri"},
+            {"actions", "doc", "name", "params", "path"},
             set(observed))
         self.assertEqual(ExampleHandler.__doc__, observed["doc"])
         self.assertEqual(ExampleHandler.__name__, observed["name"])
@@ -231,11 +231,11 @@
             }
         self.assertSetEqual(expected_actions, observed_actions)
         self.assertSetEqual({"system_id"}, set(description["params"]))
-        # The URI is a URI Template <http://tools.ietf.org/html/rfc6570>, the
+        # The path is a URI Template <http://tools.ietf.org/html/rfc6570>, the
         # components of which correspond to the parameters declared.
         self.assertEqual(
-            "http://example.com/api/1.0/nodes/{system_id}/",
-            description["uri"])
+            "/api/1.0/nodes/{system_id}/",
+            description["path"])
 
     def test_describe_resource_anonymous_resource(self):
         # When the resource does not require authentication, any configured

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2012-10-25 12:17:28 +0000
+++ src/maasserver/utils/__init__.py	2012-11-02 09:26:19 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = [
     'absolute_reverse',
+    'build_absolute_uri',
     'get_db_state',
     'ignore_unused',
     'map_enum',
@@ -85,6 +86,23 @@
     return url
 
 
+def build_absolute_uri(request, path):
+    """Given a full app-relative path, returns an absolute URI.
+
+    The path ordinarily starts with a forward-slash... imagine that you've
+    magically chroot'ed to your Django application; the path is the absolute
+    path to the view within that environment.
+
+    The URI returned uses the request to figure out how to make an absolute
+    URL. This means that the URI returned will use the same IP address or
+    alias that the request came in on.
+    """
+    script_name = request.META["SCRIPT_NAME"]
+    return "%s://%s%s%s" % (
+        "https" if request.is_secure() else "http",
+        request.get_host(), script_name, path)
+
+
 def strip_domain(hostname):
     """Return `hostname` with the domain part removed."""
     return hostname.split('.', 1)[0]

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2012-10-25 12:17:28 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-11-02 09:26:19 +0000
@@ -16,11 +16,13 @@
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from django.http import HttpRequest
 from maasserver.enum import NODE_STATUS_CHOICES
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase as DjangoTestCase
 from maasserver.utils import (
     absolute_reverse,
+    build_absolute_uri,
     get_db_state,
     map_enum,
     strip_domain,
@@ -107,6 +109,67 @@
         self.assertEqual(status, get_db_state(node, 'status'))
 
 
+class TestBuildAbsoluteURI(TestCase):
+    """Tests for `build_absolute_uri`."""
+
+    def make_request(self, host="example.com", port=80, script_name="",
+                     is_secure=False):
+        """Return a :class:`HttpRequest` with the given parameters."""
+        request = HttpRequest()
+        request.META["SERVER_NAME"] = host
+        request.META["SERVER_PORT"] = port
+        request.META["SCRIPT_NAME"] = script_name
+        request.is_secure = lambda: is_secure
+        return request
+
+    def test_simple(self):
+        request = self.make_request()
+        self.assertEqual(
+            "http://example.com/fred",
+            build_absolute_uri(request, "/fred"))
+
+    def test_different_port(self):
+        request = self.make_request(port=1234)
+        self.assertEqual(
+            "http://example.com:1234/fred",
+            build_absolute_uri(request, "/fred"))
+
+    def test_script_name_is_prefixed(self):
+        # The script name is always prefixed to the given path.
+        request = self.make_request(script_name="/foo/bar")
+        self.assertEqual(
+            "http://example.com/foo/bar/fred",
+            build_absolute_uri(request, "/fred"))
+
+    def test_secure(self):
+        request = self.make_request(port=443, is_secure=True)
+        self.assertEqual(
+            "https://example.com/fred",
+            build_absolute_uri(request, "/fred"))
+
+    def test_different_port_and_secure(self):
+        request = self.make_request(port=9443, is_secure=True)
+        self.assertEqual(
+            "https://example.com:9443/fred",
+            build_absolute_uri(request, "/fred"))
+
+    def test_no_leading_forward_slash(self):
+        # No attempt is made to ensure that the given path is separated from
+        # the to-be-prefixed path.
+        request = self.make_request(script_name="/foo")
+        self.assertEqual(
+            "http://example.com/foobar",
+            build_absolute_uri(request, "bar"))
+
+    def test_preserve_two_leading_slashes(self):
+        # Whilst this shouldn't ordinarily happen, two leading slashes in the
+        # path should be preserved, and not treated specially.
+        request = self.make_request(script_name="//foo")
+        self.assertEqual(
+            "http://example.com//foo/fred",
+            build_absolute_uri(request, "/fred"))
+
+
 class TestStripDomain(TestCase):
 
     def test_strip_domain(self):