Skip to content

Commit

Permalink
Fix for #806
Browse files Browse the repository at this point in the history
  • Loading branch information
@forman
forman committed Feb 13, 2023
1 parent 8a403d9 commit 613a951
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 115 deletions.
9 changes: 9 additions & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
## Changes in 0.13.1 (in development)

### Fixes

* The xcube server configuration parameters `url_prefix` and
`reverse_url_prefix` can now be absolute URLs. This fixes a problem for
relative prefixes such as `"proxy/8000"` used for xcube server running
inside JupyterLab. Here, the expected returned self-referencing URL was
`https://{host}/users/{user}/proxy/8000/{path}` but we got
`http://{host}/proxy/8000/{path}`. (#806)


## Changes in 0.13.0

Expand Down
85 changes: 31 additions & 54 deletions test/server/test_config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,78 +20,55 @@
# DEALINGS IN THE SOFTWARE.

import unittest
from typing import Callable

from xcube.server.config import get_reverse_url_prefix
from xcube.server.config import get_url_prefix


class ConfigTest(unittest.TestCase):
def test_get_url_prefix(self):
self.assert_url_prefix(get_url_prefix,
key='url_prefix')

def test_get_reverse_url_prefix(self):
self.assert_url_prefix(get_reverse_url_prefix,
key='reverse_url_prefix')

def assert_url_prefix(self,
get_prefix: Callable,
key: str):
self.assertEqual('',
get_url_prefix(dict()))
get_prefix({}))
self.assertEqual('',
get_url_prefix(dict(url_prefix='')))
get_prefix({key: ''}))
self.assertEqual('',
get_url_prefix(dict(url_prefix=None)))
get_prefix({key: None}))
self.assertEqual('',
get_url_prefix(dict(url_prefix='/')))
get_prefix({key: '/'}))
self.assertEqual('',
get_url_prefix(dict(url_prefix='//')))
get_prefix({key: '//'}))

self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='api/v1')))
get_prefix({key: 'api/v1'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='/api/v1')))
get_prefix({key: '/api/v1'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='api/v1/')))
get_prefix({key: 'api/v1/'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='/api/v1/')))
get_prefix({key: '/api/v1/'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='/api/v1//')))
get_prefix({key: '/api/v1//'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='//api/v1//')))
get_prefix({key: '//api/v1//'}))
self.assertEqual('/api/v1',
get_url_prefix(dict(url_prefix='///api/v1//')))

def test_get_reverse_url_prefix(self):
self.assertEqual('',
get_reverse_url_prefix(dict()))
self.assertEqual('',
get_reverse_url_prefix(dict(reverse_url_prefix='')))
self.assertEqual('',
get_reverse_url_prefix(
dict(reverse_url_prefix=None)))
self.assertEqual('',
get_reverse_url_prefix(dict(reverse_url_prefix='/')))
self.assertEqual('',
get_reverse_url_prefix(
dict(reverse_url_prefix='//')))

self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='proxy/9192')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='/proxy/9192')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='proxy/9192/')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='/proxy/9192/')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='/proxy/9192//')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='//proxy/9192//')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='///proxy/9192//')))
get_prefix({key: '///api/v1//'}))

self.assertEqual('/api/v1',
get_reverse_url_prefix(dict(url_prefix='api/v1')))
self.assertEqual('/proxy/9192',
get_reverse_url_prefix(
dict(reverse_url_prefix='/proxy/9192',
url_prefix='/api/v1')))
self.assertEqual('https://test.com',
get_prefix({key: 'https://test.com'}))
self.assertEqual('https://test.com',
get_prefix({key: 'https://test.com/'}))
self.assertEqual('https://test.com/api',
get_prefix({key: 'https://test.com/api'}))
self.assertEqual('http://test.com/api',
get_prefix({key: 'http://test.com/api/'}))
156 changes: 114 additions & 42 deletions test/server/webservers/test_tornado.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,6 @@ def test_path_to_pattern_fails(self):
f"{cm.exception}")


# class TornadoRequestHandlerTest(unittest.TestCase):
# def test_basic_props(self):
# TornadoRequestHandler()


class TornadoApiRequestTest(unittest.TestCase):
def test_basic_props(self):
body = b'{"type": "feature", "id": 137}'
Expand All @@ -187,43 +182,6 @@ def test_basic_props(self):
self.assertEqual([],
request.get_query_args('details'))

def test_url_for_path(self):
tr = tornado.httputil.HTTPServerRequest(
method='GET',
host='localhost:8080',
uri='/datasets?details=1',
)

request = TornadoApiRequest(tr)
self.assertEqual("http://localhost:8080/collections",
request.url_for_path('collections'))
self.assertEqual("http://localhost:8080/collections?details=1",
request.url_for_path('/collections',
query='details=1'))

request = TornadoApiRequest(tr,
url_prefix='/api/v1')
self.assertEqual(
"http://localhost:8080/api/v1/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)

request = TornadoApiRequest(tr,
url_prefix='/api/v1',
reverse_url_prefix='/proxy/9999')
self.assertEqual(
"http://localhost:8080/api/v1/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual(
"http://localhost:8080/proxy/9999/collections?details=1",
request.url_for_path('/collections',
query='details=1',
reverse=True)
)

def test_get_query_args(self):
tr = tornado.httputil.HTTPServerRequest(
method='GET',
Expand Down Expand Up @@ -285,6 +243,120 @@ def test_invalid_json(self):
f'{cm.exception}')


class TornadoApiRequestUrlTest(unittest.TestCase):
tr = tornado.httputil.HTTPServerRequest(
method='GET',
host='localhost:8080',
uri='/datasets?details=1',
)

def test_prefixes_not_given(self):
request = TornadoApiRequest(self.tr)
self.assertEqual("http://localhost:8080/collections",
request.url_for_path('collections'))
self.assertEqual("http://localhost:8080/collections?details=1",
request.url_for_path('/collections',
query='details=1'))
self.assertEqual("http://localhost:8080",
request.base_url)
self.assertEqual("http://localhost:8080",
request.reverse_base_url)

def test_rel_url_prefix_given(self):
request = TornadoApiRequest(self.tr,
url_prefix='/api/v1')
self.assertEqual(
"http://localhost:8080/api/v1/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual("http://localhost:8080/api/v1",
request.base_url)
self.assertEqual("http://localhost:8080/api/v1",
request.reverse_base_url)

def test_rel_url_prefix_and_rel_reverse_url_prefix_given(self):
request = TornadoApiRequest(self.tr,
url_prefix='/api/v1',
reverse_url_prefix='/proxy/9999')
self.assertEqual(
"http://localhost:8080/api/v1/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual(
"http://localhost:8080/proxy/9999/collections?details=1",
request.url_for_path('/collections',
query='details=1',
reverse=True)
)
self.assertEqual("http://localhost:8080/api/v1",
request.base_url)
self.assertEqual("http://localhost:8080/proxy/9999",
request.reverse_base_url)

def test_abs_url_prefix_given(self):
request = TornadoApiRequest(self.tr,
url_prefix='https://test.com')
self.assertEqual(
"https://test.com/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual(
"https://test.com/collections?details=1",
request.url_for_path('/collections',
query='details=1',
reverse=True)
)
self.assertEqual("https://test.com",
request.base_url)
self.assertEqual("https://test.com",
request.reverse_base_url)

def test_abs_url_prefix_and_rel_reverse_url_prefix_given(self):
request = TornadoApiRequest(self.tr,
url_prefix='https://test.com/api/v1',
reverse_url_prefix='/proxy/9999')
self.assertEqual(
"https://test.com/api/v1/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual(
"http://localhost:8080/proxy/9999/collections?details=1",
request.url_for_path('/collections',
query='details=1',
reverse=True)
)
self.assertEqual("https://test.com/api/v1",
request.base_url)
self.assertEqual("http://localhost:8080/proxy/9999",
request.reverse_base_url)

def test_abs_reverse_url_prefix_given(self):
request = TornadoApiRequest(self.tr,
url_prefix='',
reverse_url_prefix='https://test.com/api')
self.assertEqual(
"http://localhost:8080/collections?details=1",
request.url_for_path('/collections',
query='details=1')
)
self.assertEqual(
"https://test.com/api/collections?details=1",
request.url_for_path('/collections',
query='details=1',
reverse=True)
)
self.assertEqual("http://localhost:8080",
request.base_url)
self.assertEqual("https://test.com/api",
request.reverse_base_url)




class TornadoRequestHandlerTest(unittest.TestCase):

# noinspection PyMethodMayBeStatic
Expand Down
11 changes: 8 additions & 3 deletions xcube/server/config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,18 @@ def _sanitize_url_prefix(url_prefix: Optional[str]) -> str:
"""
if not url_prefix:
return ''

while url_prefix.startswith('//'):
url_prefix = url_prefix[1:]
while url_prefix.endswith('/'):
url_prefix = url_prefix[:-1]

if url_prefix == '':
return ''
elif url_prefix.startswith('/'):

if url_prefix.startswith('/') \
or url_prefix.startswith('http://') \
or url_prefix.startswith('https://'):
return url_prefix
else:
return '/' + url_prefix

return '/' + url_prefix
32 changes: 16 additions & 16 deletions xcube/server/webservers/tornado.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,27 +426,27 @@ def url_for_path(self,
path: str,
query: Optional[str] = None,
reverse: bool = False) -> str:
"""Get the reverse URL for given *path* and *query*."""

# TODO (forman): in some cases, e.g.,
# when running a tornado server (such as xcube server)
# next to a remote Jupyter Server,
# the URL reported by this implementation is wrong.
# For example with reverse prefix "/proxy/8000", we expect
# https://{host}/users/{user}/proxy/8000/{path}
# but we get
# http://{host}/proxy/8000/{path}
# Also note, that protocol degraded from HTTPS to HTTP!
#
protocol = self._request.protocol
host = self._request.host
prefix = self._reverse_url_prefix if reverse else self._url_prefix
"""Get the URL for given *path* and *query*.
If the *reverse* flag is set, the configuration parameter
``reverse_url_prefix``, if provided, is used to construct the URL,
otherwise only ``url_prefix``, if provided, is used.
"""
prefix = self._url_prefix
if reverse:
prefix = self._reverse_url_prefix or prefix
uri = ""
if path:
uri = path if path.startswith("/") else "/" + path
if query:
uri += "?" + query
return f"{protocol}://{host}{prefix}{uri}"
if "://" in prefix:
# Absolute prefix
return f"{prefix}{uri}"
else:
# Relative prefix
protocol = self._request.protocol
host = self._request.host
return f"{protocol}://{host}{prefix}{uri}"

@property
def url(self) -> str:
Expand Down

0 comments on commit 613a951

Please sign in to comment.