diff --git a/jupyterhub/apihandlers/proxy.py b/jupyterhub/apihandlers/proxy.py index a95e137c..d30b9fe1 100644 --- a/jupyterhub/apihandlers/proxy.py +++ b/jupyterhub/apihandlers/proxy.py @@ -9,26 +9,9 @@ from urllib.parse import urlparse from tornado import gen, web from .. import orm -from ..proxy import RouteSpec from ..utils import admin_only from .base import APIHandler -class _RouteSpecJSONEncoder(json.JSONEncoder): - """JSON encoder that handles routespecs""" - def encode(self, obj): - if isinstance(obj, RouteSpec): - obj = obj.host + obj.path - return super().encode(obj) - - def iterencode(self, obj, _one_shot=True): - if isinstance(obj, dict): - obj = { - key.host + key.path if isinstance(key, RouteSpec) else key : value - for key, value in obj.items() - } - return super().iterencode(obj) - -_json = _RouteSpecJSONEncoder() class ProxyAPIHandler(APIHandler): @@ -41,7 +24,7 @@ class ProxyAPIHandler(APIHandler): but without clients needing to maintain separate """ routes = yield self.proxy.get_all_routes() - self.write(_json.encode(routes)) + self.write(json.dumps(routes)) @admin_only @gen.coroutine diff --git a/jupyterhub/objects.py b/jupyterhub/objects.py index f6951659..75a8391d 100644 --- a/jupyterhub/objects.py +++ b/jupyterhub/objects.py @@ -12,6 +12,7 @@ from traitlets import ( HasTraits, Instance, Integer, Unicode, default, observe, ) +from .traitlets import URLPrefix from . import orm from .utils import ( url_path_join, can_connect, wait_for_server, @@ -30,7 +31,7 @@ class Server(HasTraits): connect_ip = Unicode() proto = Unicode('http') port = Integer() - base_url = Unicode('/') + base_url = URLPrefix('/') cookie_name = Unicode('') @property diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py index fd791a99..1ee79293 100644 --- a/jupyterhub/proxy.py +++ b/jupyterhub/proxy.py @@ -1,4 +1,11 @@ -"""API for JupyterHub's proxy.""" +"""API for JupyterHub's proxy. + +Route Specification: + +- A routespec is a URI (excluding scheme), e.g. + 'host.name/path/' for host-based routing or '/path/' for default routing. +- Route paths should be normalized to always start and end with `/` +""" # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. @@ -8,7 +15,7 @@ import json import os from subprocess import Popen import time -from urllib.parse import quote +from urllib.parse import quote, urlparse from tornado import gen from tornado.httpclient import AsyncHTTPClient, HTTPRequest @@ -28,54 +35,6 @@ from . import utils from .utils import url_path_join -class RouteSpec(namedtuple('RouteSpec', ['path', 'host'])): - """Object wrapping a proxy route specification - - Attributes: - - path (str): The URL path prefix for this route. Required. - host (str): The hostname used for routing when host-based routing is enabled. - - """ - def __new__(cls, path, *, host=''): - """Create a route specification. - - Arguments: - - path (str): The path prefix. Leading and trailing slashes will be added - if missing. Required. - host (str): The hostname used for routing when host-based routing is enabled. - Optional, keyword-only. - """ - # give host a default value - if isinstance(path, cls) and host == '': - # RouteSpec(routespec) makes a copy - path, host = path - - # ensure leading/trailing slashes - if not path.startswith('/'): - path = '/' + path - if not path.endswith('/'): - path = path + '/' - return super().__new__(cls, path, host) - - @classmethod - def as_routespec(cls, routespec): - """Ensure a RouteSpec or str is a RouteSpec - - Allows string arguments to be accepted anywhere RouteSpecs are accepted. - """ - if isinstance(routespec, str): - return cls(routespec) - else: - return routespec - - def __repr__(self): - if not self.host: - return '%s(path=%r)' % (self.__class__.__name__, self.path) - else: - return super().__repr__() - class Proxy(LoggingConfigurable): """Base class for configurable proxies that JupyterHub can use.""" @@ -106,14 +65,32 @@ class Proxy(LoggingConfigurable): Will be called during teardown if should_start is True. """ + + def validate_routespec(self, routespec): + """Validate a routespec + + - Checks host value vs host-based routing. + - Ensures trailing slash on path. + """ + # check host routing + host_route = not routespec.startswith('/') + if host_route and not self.host_routing: + raise ValueError("Cannot add host-based route %r, not using host-routing" % routespec) + if self.host_routing and not host_route: + raise ValueError("Cannot add route without host %r, using host-routing" % routespec) + # add trailing slash + if not routespec.endswith('/'): + return routespec + '/' + else: + return routespec @gen.coroutine def add_route(self, routespec, target, data): """Add a route to the proxy. Args: - routespec (RouteSpec or str): A specification for which this route will be matched. - If a string, should be treated as RouteSpec(routespec). + routespec (str): A URI (excluding scheme) for which this route will be matched, + e.g. host.name/path/ target (str): A URL that will be the target of this route. data (dict): A JSONable dict that will be associated with this route, and will be returned when retrieving information about this route. @@ -131,13 +108,31 @@ class Proxy(LoggingConfigurable): """Delete a route with a given routespec if it exists.""" pass + @gen.coroutine + def get_all_routes(self): + """Fetch and return all the routes associated by JupyterHub from the + proxy. + + Should return a dictionary of routes, where the keys are + routespecs and each value is a dict of the form:: + + { + 'routespec': the route specification + 'target': the target host for this route + 'data': the attached data dict for this route (as specified in add_route) + } + that would be returned by + `get_route(routespec)`. + """ + pass + @gen.coroutine def get_route(self, routespec): """Return the route info for a given routespec. Args: - routespec (RouteSpec or str): The route specification that was used to add this route. - If a string, should be treated as RouteSpec(routespec). + routespec (str): A URI that was used to add this route, + e.g. `host.tld/path/` Returns: result (dict): with the following keys: @@ -148,21 +143,10 @@ class Proxy(LoggingConfigurable): None: if there are no routes matching the given routespec """ # default implementation relies on get_all_routes - routespec = RouteSpec.as_routespec(routespec) + routespec = self.validate_routespec(routespec) routes = yield self.get_all_routes() return routes.get(routespec) - @gen.coroutine - def get_all_routes(self): - """Fetch and return all the routes associated by JupyterHub from the - proxy. - - Should return a dictionary of routes, where the keys are - routespecs and each value is the dict that would be returned by - `get_route(routespec)`. - """ - pass - # Most basic implementers must only implement above methods @gen.coroutine @@ -416,39 +400,41 @@ class ConfigurableHTTPProxy(Proxy): ) yield self.start() yield self.restore_routes() - + def _routespec_to_chp_path(self, routespec): - """Turn a RouteSpec into a CHP API path""" - path = routespec.path - if routespec.host: - if not self.host_routing: - raise RuntimeError("Adding route with a host") - path = '/' + url_path_join(routespec.host, path) + """Turn a routespec into a CHP API path + + For host-based routing, CHP uses the host as the first path segment. + """ + path = self.validate_routespec(routespec) + # CHP always wants to start with / + if not path.startswith('/'): + path = path + '/' + # BUG: CHP doesn't seem to like trailing slashes on some endpoints (DELETE) if path != '/' and path.endswith('/'): path = path.rstrip('/') return path def _routespec_from_chp_path(self, chp_path): - """Turn a CHP route into a RouteSpec + """Turn a CHP route into a route spec In the JSON API, CHP route keys are unescaped, - so re-escape them to raw URLs. + so re-escape them to raw URLs and ensure slashes are in the right places. """ # chp stores routes in unescaped form. # restore escaped-form we created it with. - path = quote(chp_path, safe='@/') - host = '' + routespec = quote(chp_path, safe='@/') if self.host_routing: - host, *rest = path.lstrip('/').split('/', 1) - path = '/' + ''.join(rest) - return RouteSpec(path, host=host) - + # host routes don't start with / + routespec = routespec.lstrip('/') + # all routes should end with / + if not routespec.endswith('/'): + routespec = routespec + '/' + return routespec def api_request(self, path, method='GET', body=None, client=None): """Make an authenticated API request of the proxy.""" client = client or AsyncHTTPClient() - if isinstance(path, RouteSpec): - path = self._routespec_to_chp_path(path) url = url_path_join(self.api_url, 'api/routes', path) if isinstance(body, dict): @@ -464,22 +450,20 @@ class ConfigurableHTTPProxy(Proxy): return client.fetch(req) def add_route(self, routespec, target, data=None): - # ensure RouteSpec object - routespec = RouteSpec.as_routespec(routespec) body = data or {} body['target'] = target - return self.api_request(routespec, + path = self._routespec_to_chp_path(routespec) + return self.api_request(path, method='POST', body=body, ) def delete_route(self, routespec): - routespec = RouteSpec.as_routespec(routespec) - return self.api_request(routespec, method='DELETE') + path = self._routespec_to_chp_path(routespec) + return self.api_request(path, method='DELETE') def _reformat_routespec(self, routespec, chp_data): """Reformat CHP data format to JupyterHub's proxy API.""" - # ensure RouteSpec object target = chp_data.pop('target') return { 'routespec': routespec, diff --git a/jupyterhub/services/service.py b/jupyterhub/services/service.py index 4ce05325..a30f2532 100644 --- a/jupyterhub/services/service.py +++ b/jupyterhub/services/service.py @@ -52,7 +52,6 @@ from traitlets.config import LoggingConfigurable from .. import orm from ..objects import Server -from ..proxy import RouteSpec from ..traitlets import Command from ..spawner import LocalProcessSpawner, set_user_setuid from ..utils import url_path_join @@ -249,9 +248,9 @@ class Service(LoggingConfigurable): if not self.server: return '' if self.domain: - return RouteSpec(path=self.server.base_url, host=self.domain) + return self.domain + self.server.base_url else: - return RouteSpec(self.server.base_url) + return self.server.base_url def __repr__(self): return "<{cls}(name={name}{managed})>".format( diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index f0cb3fce..a393b330 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -413,9 +413,8 @@ def test_spawn(app, io_loop): status = io_loop.run_sync(app_user.spawner.poll) assert status is None - assert user.server.base_url == ujoin(app.base_url, 'user/%s' % name) + assert user.server.base_url == ujoin(app.base_url, 'user/%s' % name) + '/' url = public_url(app, user) - print(url) r = requests.get(url) assert r.status_code == 200 assert r.text == user.server.base_url diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 72f4b34e..237ac450 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -97,7 +97,7 @@ def test_spawn_redirect(app, io_loop): r.raise_for_status() print(urlparse(r.url)) path = urlparse(r.url).path - assert path == ujoin(app.base_url, 'user/%s' % name) + assert path == ujoin(app.base_url, 'user/%s/' % name) # should have started server status = io_loop.run_sync(u.spawner.poll) @@ -108,7 +108,7 @@ def test_spawn_redirect(app, io_loop): r.raise_for_status() print(urlparse(r.url)) path = urlparse(r.url).path - assert path == ujoin(app.base_url, '/user/%s' % name) + assert path == ujoin(app.base_url, '/user/%s/' % name) def test_spawn_page(app): with mock.patch.dict(app.users.settings, {'spawner_class': FormSpawner}): diff --git a/jupyterhub/tests/test_proxy.py b/jupyterhub/tests/test_proxy.py index 0a977ce3..b270d738 100644 --- a/jupyterhub/tests/test_proxy.py +++ b/jupyterhub/tests/test_proxy.py @@ -15,40 +15,6 @@ from .mocking import MockHub from .test_api import api_request from ..utils import wait_for_http_server, url_path_join as ujoin -from jupyterhub.proxy import RouteSpec - -def test_routespec(): - with pytest.raises(TypeError): - RouteSpec() - - spec = RouteSpec('/test/') - assert spec.host == '' - assert spec.path == '/test/' - - assert 'path=%r' % spec.path in repr(spec) - assert 'host' not in repr(spec) - - spec = RouteSpec('test2', host='myhost') - assert spec.path == '/test2/' - assert spec.host == 'myhost' - - assert 'path=%r' % spec.path in repr(spec) - assert 'host=%r' % spec.host in repr(spec) - - copyspec = RouteSpec(spec) - assert copyspec.path == '/test2/' - assert copyspec.host == 'myhost' - assert copyspec == spec - -def test_as_routespec(): - spec = RouteSpec('/test', host='myhost') - as_spec = RouteSpec.as_routespec(spec) - assert as_spec is spec - - spec2 = RouteSpec.as_routespec('/path') - assert isinstance(spec2, RouteSpec) - assert spec2.path == '/path/' - def test_external_proxy(request, io_loop): @@ -98,7 +64,7 @@ def test_external_proxy(request, io_loop): # test if api service has a root route '/' routes = io_loop.run_sync(app.proxy.get_all_routes) - assert list(routes.keys()) == [RouteSpec('/')] + assert list(routes.keys()) == ['/'] # add user to the db and start a single user server name = 'river' @@ -109,12 +75,13 @@ def test_external_proxy(request, io_loop): routes = io_loop.run_sync(app.proxy.get_all_routes) # sets the desired path result - user_path = ujoin(app.base_url, 'user/river') + user_path = ujoin(app.base_url, 'user/river') + '/' + print(app.base_url, user_path) host = '' if app.subdomain_host: host = '%s.%s' % (name, urlparse(app.subdomain_host).hostname) - user_spec = RouteSpec(user_path, host=host) - assert sorted(routes.keys()) == [RouteSpec('/'), user_spec] + user_spec = host + user_path + assert sorted(routes.keys()) == ['/', user_spec] # teardown the proxy and start a new one in the same place proxy.terminate() @@ -123,7 +90,7 @@ def test_external_proxy(request, io_loop): routes = io_loop.run_sync(app.proxy.get_all_routes) - assert list(routes.keys()) == [RouteSpec('/')] + assert list(routes.keys()) == ['/'] # poke the server to update the proxy r = api_request(app, 'proxy', method='post') @@ -131,7 +98,7 @@ def test_external_proxy(request, io_loop): # check that the routes are correct routes = io_loop.run_sync(app.proxy.get_all_routes) - assert sorted(routes.keys()) == [RouteSpec('/'), user_spec] + assert sorted(routes.keys()) == ['/', user_spec] # teardown the proxy, and start a new one with different auth and port proxy.terminate() @@ -170,7 +137,7 @@ def test_external_proxy(request, io_loop): # check that the routes are correct routes = io_loop.run_sync(app.proxy.get_all_routes) - assert sorted(routes.keys()) == [RouteSpec('/'), user_spec] + assert sorted(routes.keys()) == ['/', user_spec] @pytest.mark.parametrize("username, endpoints", [ @@ -207,39 +174,53 @@ def test_check_routes(app, io_loop, username, endpoints): # check that before and after state are the same assert before == after +from contextlib import contextmanager -@pytest.mark.now @pytest.mark.gen_test -@pytest.mark.parametrize("route_str", [ - '/has%20space/foo', - 'trailing/slash/', +@pytest.mark.parametrize("routespec", [ + '/has%20space/foo/', + '/missing-trailing/slash', '/has/@/', - 'has/' + quote('üñîçø∂é'), + '/has/' + quote('üñîçø∂é'), + 'host.name/path/', + 'other.host/path/no/slash', ]) -@pytest.mark.parametrize("as_str", [ - True, - False, -]) -def test_add_get_delete(app, route_str, as_str): - if app.subdomain_host and as_str: - # can't use simple str args when host is not empty - pytest.skip() - routespec = RouteSpec(route_str, host=urlparse(app.subdomain_host).hostname or '') +def test_add_get_delete(app, routespec): + arg = routespec + if not routespec.endswith('/'): + routespec = routespec + '/' + + # host-routes when not host-routing raises an error + # and vice versa + expect_value_error = bool(app.subdomain_host) ^ (not routespec.startswith('/')) + @contextmanager + def context(): + if expect_value_error: + with pytest.raises(ValueError): + yield + else: + yield + proxy = app.proxy - arg = route_str if as_str else routespec target = 'https://localhost:1234' - yield proxy.add_route(arg, target=target) + with context(): + yield proxy.add_route(arg, target=target) routes = yield proxy.get_all_routes() - assert routespec in routes.keys() - route = yield proxy.get_route(arg) - assert route == { - 'target': target, - 'routespec': routespec, - 'data': route.get('data'), - } - yield proxy.delete_route(arg) - route = yield proxy.get_route(arg) - assert route is None + if not expect_value_error: + assert routespec in routes.keys() + with context(): + route = yield proxy.get_route(arg) + assert route == { + 'target': target, + 'routespec': routespec, + 'data': route.get('data'), + } + with context(): + yield proxy.delete_route(arg) + with context(): + route = yield proxy.get_route(arg) + assert route is None + @pytest.mark.parametrize("test_data", [None, 'notjson', json.dumps([])]) def test_proxy_patch_bad_request_data(app, test_data): diff --git a/jupyterhub/user.py b/jupyterhub/user.py index 4394ffb7..915fd452 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -15,7 +15,6 @@ from . import orm from .objects import Server from traitlets import HasTraits, Any, Dict, observe, default from .spawner import LocalProcessSpawner -from .proxy import RouteSpec class UserDict(dict): """Like defaultdict, but for users @@ -120,7 +119,7 @@ class User(HasTraits): self.allow_named_servers = self.settings.get('allow_named_servers', False) self.base_url = url_path_join( - self.settings.get('base_url', '/'), 'user', self.escaped_name) + self.settings.get('base_url', '/'), 'user', self.escaped_name) + '/' self.spawner = self.spawner_class( user=self, @@ -171,9 +170,9 @@ class User(HasTraits): @property def proxy_spec(self): if self.settings.get('subdomain_host'): - return RouteSpec(path=self.base_url, host=self.domain) + return self.domain + self.base_url else: - return RouteSpec(path=self.base_url) + return self.base_url @property def domain(self): @@ -223,7 +222,7 @@ class User(HasTraits): server_name = options['server_name'] else: server_name = default_server_name(self) - base_url = url_path_join(self.base_url, server_name) + base_url = url_path_join(self.base_url, server_name) + '/' else: server_name = '' base_url = self.base_url