From 92209228f6faf01b6b35da0ab3e5b6c1408c1bee Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 21 Sep 2017 12:01:20 +0200 Subject: [PATCH 1/4] raise 403 on APIHandler failed login instead of redirecting to human login page, which can have side effects --- notebook/base/handlers.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index 3a0eb815a..76828df04 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -450,6 +450,16 @@ class APIHandler(IPythonHandler): raise web.HTTPError(404) return super(APIHandler, self).prepare() + def get_current_user(self): + """Raise 403 on API handlers instead of redirecting to human login page""" + # preserve _user_cache so we don't raise more than once + if hasattr(self, '_user_cache'): + return self._user_cache + self._user_cache = user = super(APIHandler, self).get_current_user() + if user is None: + raise web.HTTPError(403) + return user + @property def content_security_policy(self): csp = '; '.join([ From ba353e20f76580e90e8d48f59b086bca3717d48f Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 21 Sep 2017 12:03:21 +0200 Subject: [PATCH 2/4] use .write_error on APIHandler instead of `@json_errors` for JSON error messages this is the standard tornado way to do it, and catches errors at the `prepare` stage, which method decorators do not --- notebook/base/handlers.py | 50 ++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index 76828df04..4aba4775e 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -10,6 +10,8 @@ import os import re import sys import traceback +import types +import warnings try: # py3 from http.client import responses @@ -450,6 +452,24 @@ class APIHandler(IPythonHandler): raise web.HTTPError(404) return super(APIHandler, self).prepare() + def write_error(self, status_code, **kwargs): + """APIHandler errors are JSON, not human pages""" + self.set_header('Content-Type', 'application/json') + message = responses.get(status_code, 'Unknown HTTP Error') + reply = { + 'message': message, + } + exc_info = kwargs.get('exc_info') + if exc_info: + e = exc_info[1] + if isinstance(e, HTTPError): + reply['message'] = e.log_message or message + else: + reply['message'] = 'Unhandled error' + reply['traceback'] = ''.join(traceback.format_exception(*exc_info)) + self.log.warning(reply['message']) + self.finish(json.dumps(reply)) + def get_current_user(self): """Raise 403 on API handlers instead of redirecting to human login page""" # preserve _user_cache so we don't raise more than once @@ -557,32 +577,14 @@ def json_errors(method): 2. Create and return a JSON body with a message field describing the error in a human readable form. """ + warnings.warn('@json_errors is deprecated. Use APIHandler', + DeprecationWarning, + stacklevel=2, + ) @functools.wraps(method) - @gen.coroutine def wrapper(self, *args, **kwargs): - try: - result = yield gen.maybe_future(method(self, *args, **kwargs)) - except web.HTTPError as e: - self.set_header('Content-Type', 'application/json') - status = e.status_code - message = e.log_message - self.log.warning(message) - self.set_status(e.status_code) - reply = dict(message=message, reason=e.reason) - self.finish(json.dumps(reply)) - except Exception: - self.set_header('Content-Type', 'application/json') - self.log.error("Unhandled error in API request", exc_info=True) - status = 500 - message = "Unknown server error" - t, value, tb = sys.exc_info() - self.set_status(status) - tb_text = ''.join(traceback.format_exception(t, value, tb)) - reply = dict(message=message, reason=None, traceback=tb_text) - self.finish(json.dumps(reply)) - else: - # FIXME: can use regular return in generators in py3 - raise gen.Return(result) + self.write_error = types.MethodType(APIHandler.write_error, self) + return method(self, *args, **kwargs) return wrapper From 962c5ccd807fc4c7f43612cfc17bea0e44318a59 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 21 Sep 2017 12:53:51 +0200 Subject: [PATCH 3/4] stop using `@json_handlers` --- notebook/base/handlers.py | 1 - notebook/services/api/handlers.py | 3 +-- notebook/services/config/handlers.py | 5 +---- notebook/services/contents/handlers.py | 12 +----------- notebook/services/kernels/handlers.py | 7 +------ notebook/services/kernelspecs/handlers.py | 4 +--- notebook/services/nbconvert/handlers.py | 3 +-- notebook/services/security/handlers.py | 3 +-- notebook/services/sessions/handlers.py | 7 +------ notebook/terminal/api_handlers.py | 6 +----- 10 files changed, 9 insertions(+), 42 deletions(-) diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index 4aba4775e..df5007bf6 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -655,7 +655,6 @@ class FileFindHandler(IPythonHandler, web.StaticFileHandler): class APIVersionHandler(APIHandler): - @json_errors def get(self): # not authenticated, so give as few info as possible self.finish(json.dumps({"version":notebook.__version__})) diff --git a/notebook/services/api/handlers.py b/notebook/services/api/handlers.py index 6847348aa..b76457b50 100644 --- a/notebook/services/api/handlers.py +++ b/notebook/services/api/handlers.py @@ -8,7 +8,7 @@ import json from tornado import gen, web -from ...base.handlers import IPythonHandler, APIHandler, json_errors +from ...base.handlers import IPythonHandler, APIHandler from notebook._tz import utcfromtimestamp, isoformat import os @@ -28,7 +28,6 @@ class APIStatusHandler(APIHandler): _track_activity = False - @json_errors @web.authenticated @gen.coroutine def get(self): diff --git a/notebook/services/config/handlers.py b/notebook/services/config/handlers.py index 7a49068a0..76c1bd3e5 100644 --- a/notebook/services/config/handlers.py +++ b/notebook/services/config/handlers.py @@ -9,24 +9,21 @@ import errno from tornado import web from ipython_genutils.py3compat import PY3 -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler class ConfigHandler(APIHandler): - @json_errors @web.authenticated def get(self, section_name): self.set_header("Content-Type", 'application/json') self.finish(json.dumps(self.config_manager.get(section_name))) - @json_errors @web.authenticated def put(self, section_name): data = self.get_json_body() # Will raise 400 if content is not valid JSON self.config_manager.set(section_name, data) self.set_status(204) - @json_errors @web.authenticated def patch(self, section_name): new_data = self.get_json_body() diff --git a/notebook/services/contents/handlers.py b/notebook/services/contents/handlers.py index a063aec40..25dcaf213 100644 --- a/notebook/services/contents/handlers.py +++ b/notebook/services/contents/handlers.py @@ -14,7 +14,7 @@ from notebook.utils import url_path_join, url_escape from jupyter_client.jsonutil import date_default from notebook.base.handlers import ( - IPythonHandler, APIHandler, json_errors, path_regex, + IPythonHandler, APIHandler, path_regex, ) @@ -87,7 +87,6 @@ class ContentsHandler(APIHandler): self.set_header('Content-Type', 'application/json') self.finish(json.dumps(model, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def get(self, path=''): @@ -115,7 +114,6 @@ class ContentsHandler(APIHandler): validate_model(model, expect_content=content) self._finish_model(model, location=False) - @json_errors @web.authenticated @gen.coroutine def patch(self, path=''): @@ -168,7 +166,6 @@ class ContentsHandler(APIHandler): validate_model(model, expect_content=False) self._finish_model(model) - @json_errors @web.authenticated @gen.coroutine def post(self, path=''): @@ -204,7 +201,6 @@ class ContentsHandler(APIHandler): else: yield self._new_untitled(path) - @json_errors @web.authenticated @gen.coroutine def put(self, path=''): @@ -230,7 +226,6 @@ class ContentsHandler(APIHandler): else: yield gen.maybe_future(self._new_untitled(path)) - @json_errors @web.authenticated @gen.coroutine def delete(self, path=''): @@ -244,7 +239,6 @@ class ContentsHandler(APIHandler): class CheckpointsHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def get(self, path=''): @@ -254,7 +248,6 @@ class CheckpointsHandler(APIHandler): data = json.dumps(checkpoints, default=date_default) self.finish(data) - @json_errors @web.authenticated @gen.coroutine def post(self, path=''): @@ -271,7 +264,6 @@ class CheckpointsHandler(APIHandler): class ModifyCheckpointsHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def post(self, path, checkpoint_id): @@ -281,7 +273,6 @@ class ModifyCheckpointsHandler(APIHandler): self.set_status(204) self.finish() - @json_errors @web.authenticated @gen.coroutine def delete(self, path, checkpoint_id): @@ -310,7 +301,6 @@ class NotebooksRedirectHandler(IPythonHandler): class TrustNotebooksHandler(IPythonHandler): """ Handles trust/signing of notebooks """ - @json_errors @web.authenticated @gen.coroutine def post(self,path=''): diff --git a/notebook/services/kernels/handlers.py b/notebook/services/kernels/handlers.py index 8ba8cd038..d42107703 100644 --- a/notebook/services/kernels/handlers.py +++ b/notebook/services/kernels/handlers.py @@ -18,14 +18,13 @@ from jupyter_client.jsonutil import date_default from ipython_genutils.py3compat import cast_unicode from notebook.utils import url_path_join, url_escape -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler from ...base.zmqhandlers import AuthenticatedZMQStreamHandler, deserialize_binary_message from jupyter_client import protocol_version as client_protocol_version class MainKernelHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def get(self): @@ -33,7 +32,6 @@ class MainKernelHandler(APIHandler): kernels = yield gen.maybe_future(km.list_kernels()) self.finish(json.dumps(kernels, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def post(self): @@ -56,7 +54,6 @@ class MainKernelHandler(APIHandler): class KernelHandler(APIHandler): - @json_errors @web.authenticated def get(self, kernel_id): km = self.kernel_manager @@ -64,7 +61,6 @@ class KernelHandler(APIHandler): model = km.kernel_model(kernel_id) self.finish(json.dumps(model, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def delete(self, kernel_id): @@ -76,7 +72,6 @@ class KernelHandler(APIHandler): class KernelActionHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def post(self, kernel_id, action): diff --git a/notebook/services/kernelspecs/handlers.py b/notebook/services/kernelspecs/handlers.py index 8779ab88a..7de1e2b09 100644 --- a/notebook/services/kernelspecs/handlers.py +++ b/notebook/services/kernelspecs/handlers.py @@ -13,7 +13,7 @@ pjoin = os.path.join from tornado import web -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler from ...utils import url_path_join, url_unescape def kernelspec_model(handler, name): @@ -45,7 +45,6 @@ def kernelspec_model(handler, name): class MainKernelSpecHandler(APIHandler): - @json_errors @web.authenticated def get(self): ksm = self.kernel_spec_manager @@ -66,7 +65,6 @@ class MainKernelSpecHandler(APIHandler): class KernelSpecHandler(APIHandler): - @json_errors @web.authenticated def get(self, kernel_name): try: diff --git a/notebook/services/nbconvert/handlers.py b/notebook/services/nbconvert/handlers.py index 83a0ee8bd..7e27783ef 100644 --- a/notebook/services/nbconvert/handlers.py +++ b/notebook/services/nbconvert/handlers.py @@ -2,11 +2,10 @@ import json from tornado import web -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler class NbconvertRootHandler(APIHandler): - @json_errors @web.authenticated def get(self): try: diff --git a/notebook/services/security/handlers.py b/notebook/services/security/handlers.py index d8f38feff..82a00d234 100644 --- a/notebook/services/security/handlers.py +++ b/notebook/services/security/handlers.py @@ -5,7 +5,7 @@ from tornado import web -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler from . import csp_report_uri class CSPReportHandler(APIHandler): @@ -21,7 +21,6 @@ class CSPReportHandler(APIHandler): # don't check XSRF for CSP reports return - @json_errors @web.authenticated def post(self): '''Log a content security policy violation report''' diff --git a/notebook/services/sessions/handlers.py b/notebook/services/sessions/handlers.py index 2b3b677b0..9075756e6 100644 --- a/notebook/services/sessions/handlers.py +++ b/notebook/services/sessions/handlers.py @@ -11,7 +11,7 @@ import os from tornado import gen, web -from ...base.handlers import APIHandler, json_errors +from ...base.handlers import APIHandler from jupyter_client.jsonutil import date_default from notebook.utils import url_path_join from jupyter_client.kernelspec import NoSuchKernel @@ -19,7 +19,6 @@ from jupyter_client.kernelspec import NoSuchKernel class SessionRootHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def get(self): @@ -28,7 +27,6 @@ class SessionRootHandler(APIHandler): sessions = yield gen.maybe_future(sm.list_sessions()) self.finish(json.dumps(sessions, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def post(self): @@ -90,7 +88,6 @@ class SessionRootHandler(APIHandler): class SessionHandler(APIHandler): - @json_errors @web.authenticated @gen.coroutine def get(self, session_id): @@ -99,7 +96,6 @@ class SessionHandler(APIHandler): model = yield gen.maybe_future(sm.get_session(session_id=session_id)) self.finish(json.dumps(model, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def patch(self, session_id): @@ -153,7 +149,6 @@ class SessionHandler(APIHandler): ) self.finish(json.dumps(model, default=date_default)) - @json_errors @web.authenticated @gen.coroutine def delete(self, session_id): diff --git a/notebook/terminal/api_handlers.py b/notebook/terminal/api_handlers.py index ed2d8ce3e..059cc9165 100644 --- a/notebook/terminal/api_handlers.py +++ b/notebook/terminal/api_handlers.py @@ -1,17 +1,15 @@ import json from tornado import web, gen -from ..base.handlers import APIHandler, json_errors +from ..base.handlers import APIHandler from ..utils import url_path_join class TerminalRootHandler(APIHandler): - @json_errors @web.authenticated def get(self): tm = self.terminal_manager terms = [{'name': name} for name in tm.terminals] self.finish(json.dumps(terms)) - @json_errors @web.authenticated def post(self): """POST /terminals creates a new terminal and redirects to it""" @@ -22,7 +20,6 @@ class TerminalRootHandler(APIHandler): class TerminalHandler(APIHandler): SUPPORTED_METHODS = ('GET', 'DELETE') - @json_errors @web.authenticated def get(self, name): tm = self.terminal_manager @@ -31,7 +28,6 @@ class TerminalHandler(APIHandler): else: raise web.HTTPError(404, "Terminal not found: %r" % name) - @json_errors @web.authenticated @gen.coroutine def delete(self, name): From 4467dc9f127dea60cc6a9ba57caf9f54f070b270 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 21 Sep 2017 12:54:36 +0200 Subject: [PATCH 4/4] specify version for deprecation --- notebook/base/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index df5007bf6..894cfc8b1 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -577,7 +577,7 @@ def json_errors(method): 2. Create and return a JSON body with a message field describing the error in a human readable form. """ - warnings.warn('@json_errors is deprecated. Use APIHandler', + warnings.warn('@json_errors is deprecated in notebook 5.2.0. Subclass APIHandler instead.', DeprecationWarning, stacklevel=2, )