Merge lp:~allenap/maas/api-uri-bug-1059645-for-1.2 into lp:maas/1.2

[Gavin Panella]

Backport of lp:maas revision 1324:

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

Previously, DEFAULT_MAAS_URL was always used.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-30 15:15:30 +0000
+++ src/maasserver/api.py	2012-11-02 09:52:23 +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,
     )
@@ -967,6 +968,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."""
@@ -1841,12 +1846,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:52:23 +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 15:15:30 +0000
+++ src/maasserver/tests/test_api.py	2012-11-02 09:52:23 +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,
@@ -4192,3 +4201,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:52:23 +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-30 15:15:30 +0000
+++ src/maasserver/utils/__init__.py	2012-11-02 09:52:23 +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-30 15:15:30 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-11-02 09:52:23 +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):