Skip to content
Merged
8 changes: 6 additions & 2 deletions remoteappmanager/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,12 @@ def _db_default(self):
class_path = self.file_config.accounting_class
module_path, _, cls_name = class_path.rpartition('.')
cls = getattr(importlib.import_module(module_path), cls_name)

return cls(**self.file_config.accounting_kwargs)
try:
return cls(**self.file_config.accounting_kwargs)
except Exception:
reason = 'The database is not initialised properly.'
self.log.exception(reason)
raise web.HTTPError(reason=reason)

@default("user")
def _user_default(self):
Expand Down
18 changes: 18 additions & 0 deletions remoteappmanager/handlers/base_handler.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from http.client import responses
from urllib.parse import urljoin

from tornado import web, gen
Expand Down Expand Up @@ -40,3 +41,20 @@ def render(self, template_name, **kwargs):

args.update(kwargs)
super(BaseHandler, self).render(template_name, **args)

def write_error(self, status_code, **kwargs):
"""Render error page for uncaught errors"""
status_message = responses.get(status_code, 'Unknown HTTP Error')
message = ""

# If this error was caused by an uncaught exception
# log exception message and reference number as well
exc_info = kwargs.get('exc_info')
if exc_info:
exception = exc_info[1]
ref = self.log.issue(status_message, exception)
reason = getattr(exception, 'reason', '')
message = '{} Ref.: {}'.format(reason, ref)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still open up to the chance that this code is actually presenting the user with internal information. It depends on the exception reason text, and if the exception text contains sensitive information for whatever reason, it will be shown to the user.

@kitchoi kitchoi Jul 27, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, web.HTTPError.reason is empty. If it is empty, the str(HTTPError) representation is deduced by the status code (e.g. 404 -> Not Found) . If someone raises an HTTPError with a reason attribute, that content would be displayed to the user even if the write_error was not overload. So it is entirely the web developer's choice what to put in reason.
screenshot from 2016-07-27 17 15 19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which brings us back to the reason discussion. Is it "proper" to change the HTTP reason, when it's supposed to be a standard string? What are others doing? Tornado documentation says that reason is specified only when using non standard codes, not that we can put anything in it.

I am not saying it's not possible, just if it's good practice, and from what I know of HTTP, it isn't

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w3.org says the reason phrases are only recommended, they may be replaced.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If renamingreason to something e.g.user_message makes the usage here more reasonable, then it is not the problem of the usage, it is a problem with naming.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tornado, on the other hand, seems to favour the reason usage only if you use non-standard codes.

reason (string) – Keyword-only argument. The HTTP “reason” phrase to pass in the status line along with status_code. Normally determined automatically from status_code, but can be used to use a non-standard numeric code.

the RFC says that it's for human consumption, but it doesn't say the scope of this information, or if it's good practice to change it to whatever

The individual values of the numeric status codes defined for HTTP/1.1, and an example set of corresponding Reason-Phrase's, are presented below. The reason phrases listed here are only recommendations -- they MAY be replaced by local equivalents without affecting the protocol.

It's a MAY, which means it's everybody's game. So for the technical point of view it's fine. The question is if it's an acceptable practice for us. @itziakos we need your two cents?

@kitchoi kitchoi Jul 29, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tornado, on the other hand, seems to favour the reason usage only if you use non-standard codes.

In their documentation, by 'non-standard codes', they mean 'non-standard status code' and therefore no standard reason phrase can be found (say a status code 420). In many places in their own code, they custom the 'reason' for common status codes such as 403. (See StaticFilePathHandler.validate_absolute_path)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In their documentation, by 'non-standard codes', they mean 'non-standard status code' and therefore no standard reason phrase can be found (say a status code 420).

That said, they mean to suggest using reason to support non-standard status code. I do not yet see the implication of 'don't change reason if your status code is a standard one'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, the following is the tornado's documentation we both refer to:

reason: Keyword-only argument. The HTTP "reason" phrase
to pass in the status line along with status_code. Normally
determined automatically from status_code, but can be used
to use a non-standard numeric code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After an offline discussion it looks that there is no obvious way of doing this. So this is a good solution for us and we will keep an eye for a better solution.


self.render('error.html', status_code=status_code,
status_message=status_message, message=message)
28 changes: 28 additions & 0 deletions remoteappmanager/templates/error.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{% extends "page.html" %}

{% block login_widget %}
{% endblock %}

{% block main %}

<div class="error">
{% block h1_error %}
<h1>
{{status_code}} : {{status_message}}
</h1>
{% endblock h1_error %}
{% block error_detail %}
{% if message %}
<p>
{{message}}
</p>
{% endif %}
{% if message_html %}
<p>
{{message_html | safe}}
</p>
{% endif %}
{% endblock error_detail %}
</div>

{% endblock %}
85 changes: 85 additions & 0 deletions tests/handlers/test_base_handler.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import os

from tests import utils
from tests.mocking import dummy
from tests.temp_mixin import TempMixin


class TestBaseHandler(TempMixin, utils.AsyncHTTPTestCase):
def setUp(self):
self._old_proxy_api_token = os.environ.get("PROXY_API_TOKEN", None)
os.environ["PROXY_API_TOKEN"] = "dummy_token"

def cleanup():
if self._old_proxy_api_token is not None:
os.environ["PROXY_API_TOKEN"] = self._old_proxy_api_token
else:
del os.environ["PROXY_API_TOKEN"]

self.addCleanup(cleanup)

super().setUp()

def get_app(self):
file_config = dummy.FileConfig()
file_config.accounting_class = 'this_should_fail'
file_config.accounting_kwargs = {}

app = dummy.create_application(file_config=file_config)
app.hub.verify_token.return_value = {
'pending': None,
'name': app.settings['user'],
'admin': False,
'server': app.settings['base_urlpath']}
return app

def test_home_internal_error(self):
res = self.fetch("/user/username/",
headers={
"Cookie": "jupyter-hub-token-username=foo"
}
)

self.assertEqual(res.code, 500)
self.assertIn('Internal Server Error', str(res.body))
self.assertIn(" Ref.:", str(res.body))


class TestBaseHandlerDatabaseError(TempMixin, utils.AsyncHTTPTestCase):
def setUp(self):
self._old_proxy_api_token = os.environ.get("PROXY_API_TOKEN", None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code shows up again and again in all our tests. It's becoming a liability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is always tricky to have to cleanup/recover the enviroment variables. Can you open an issue, this will need to be investigated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already there.

os.environ["PROXY_API_TOKEN"] = "dummy_token"

def cleanup():
if self._old_proxy_api_token is not None:
os.environ["PROXY_API_TOKEN"] = self._old_proxy_api_token
else:
del os.environ["PROXY_API_TOKEN"]

self.addCleanup(cleanup)

super().setUp()

def get_app(self):
file_config = dummy.FileConfig()
file_config.accounting_kwargs = {}

app = dummy.create_application(file_config=file_config)
app.hub.verify_token.return_value = {
'pending': None,
'name': app.settings['user'],
'admin': False,
'server': app.settings['base_urlpath']}
return app

def test_home_internal_error(self):
res = self.fetch("/user/username/",
headers={
"Cookie": "jupyter-hub-token-username=foo"
}
)

self.assertEqual(res.code, 500)
self.assertIn('Internal Server Error', str(res.body))
self.assertIn("The database is not initialised properly. Ref.:",
str(res.body))