From c1b2e68138e1102a68300220b90f90e0ef456171 Mon Sep 17 00:00:00 2001 From: sharkykh Date: Mon, 20 May 2019 21:03:31 +0300 Subject: [PATCH] Fix API v2 exception on incorrect API key. Fixes #5425 (#6703) * Fix API v2 exception on incorrect API key. Fixes #5425 * Update changelog * Fix authentication endpoint * Test bad auth conditions * Fix RESTful and non-RESTful error handling * Fix API pagination This reverts commit b291939f84505460065ca74cf7151accb09d323e. * Fix Tornado request logging - Fix requests getting logged twice. - Fix API v2 HTTPErrors getting logged as warnings. * Fix `write_error` for `raise HTTPError(code, message)` --- CHANGELOG.md | 1 + medusa/server/api/v2/auth.py | 6 ++-- medusa/server/api/v2/base.py | 64 ++++++++++++++++++++---------------- tests/apiv2/test_auth.py | 37 +++++++++++++++++++++ 4 files changed, 77 insertions(+), 31 deletions(-) create mode 100644 tests/apiv2/test_auth.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 471cf803d2..a397f95cb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Fixed "send to trash" option not doing anything (Python 3.6 and higher) ([#6625](https://github.com/pymedusa/Medusa/pull/6625)) - Fixed setting episodes to archived in backlog overview ([#6636](https://github.com/pymedusa/Medusa/pull/6636)) - Fixed exception in Elte-Tracker provider when no result is found ([#6680](https://github.com/pymedusa/Medusa/pull/6680)) +- Fixed exception in API v2 when an incorrect API key was provided, or none was provided ([#6703](https://github.com/pymedusa/Medusa/pull/6703)) ## 0.3.1 (2019-03-20) diff --git a/medusa/server/api/v2/auth.py b/medusa/server/api/v2/auth.py index fa9b8626b3..2bf1a900e6 100644 --- a/medusa/server/api/v2/auth.py +++ b/medusa/server/api/v2/auth.py @@ -30,9 +30,9 @@ class AuthHandler(BaseRequestHandler): #: allowed HTTP methods allowed_methods = ('POST', ) - def prepare(self): - """Prepare.""" - pass + def _check_authentication(self): + """Override authentication check for the authentication endpoint.""" + return None def http_post(self, *args, **kwargs): """Request JWT.""" diff --git a/medusa/server/api/v2/base.py b/medusa/server/api/v2/base.py index aa60e33ae9..8115363c76 100644 --- a/medusa/server/api/v2/base.py +++ b/medusa/server/api/v2/base.py @@ -25,10 +25,9 @@ from six import iteritems, string_types, text_type, viewitems from tornado.gen import coroutine -from tornado.httpclient import HTTPError from tornado.httputil import url_concat from tornado.ioloop import IOLoop -from tornado.web import RequestHandler +from tornado.web import HTTPError, RequestHandler log = BraceAdapter(logging.getLogger(__name__)) log.logger.addHandler(logging.NullHandler()) @@ -56,11 +55,8 @@ class BaseRequestHandler(RequestHandler): #: parent resource handler parent_handler = None - def prepare(self): + def _check_authentication(self): """Check if JWT or API key is provided and valid.""" - if self.request.method == 'OPTIONS': - return - api_key = self.get_argument('api_key', default=None) or self.request.headers.get('X-Api-Key') if api_key and api_key == app.API_KEY: return @@ -90,13 +86,11 @@ def async_call(self, name, *args, **kwargs): try: method = getattr(self, 'http_' + name) except AttributeError: - raise HTTPError(405, '{name} method is not allowed'.format(name=name.upper())) + raise HTTPError(405) def blocking_call(): - try: - return method(*args, **kwargs) - except Exception as error: - self._handle_request_exception(error) + result = self._check_authentication() + return method(*args, **kwargs) if result is None else result return IOLoop.current().run_in_executor(executor, blocking_call) @@ -154,17 +148,41 @@ def put(self, *args, **kwargs): if not self._finished: self.finish(content) - def write_error(self, *args, **kwargs): + def write_error(self, status_code, *args, **kwargs): """Only send traceback if app.DEVELOPER is true.""" - if app.DEVELOPER and 'exc_info' in kwargs: + response = None + exc_info = kwargs.get('exc_info', None) + + if exc_info and isinstance(exc_info[1], HTTPError): + error = exc_info[1].log_message or exc_info[1].reason + response = self.api_response(status=status_code, error=error) + elif app.DEVELOPER and exc_info: self.set_header('content-type', 'text/plain') self.set_status(500) - for line in traceback.format_exception(*kwargs['exc_info']): + for line in traceback.format_exception(*exc_info): self.write(line) - self.finish() else: response = self._internal_server_error() - self.finish(response) + + self.finish(response) + + def log_exception(self, typ, value, tb): + """ + Customize logging of uncaught exceptions. + + Only logs unhandled exceptions, as ``HTTPErrors`` are common for a RESTful API handler. + """ + if not app.WEB_LOG: + return + + if isinstance(value, HTTPError): + return + + log.error('Uncaught exception: {summary}\n{req!r}', { + 'summary': self._request_summary(), + 'req': self.request, + 'exc_info': (typ, value, tb), + }) def options(self, *args, **kwargs): """OPTIONS HTTP method.""" @@ -248,13 +266,6 @@ def create_app_handler(cls, base): return cls.create_url(base, cls.name, *(cls.identifier, cls.path_param)), cls - def _handle_request_exception(self, error): - if isinstance(error, HTTPError): - response = self.api_response(error.code, error.message) - self.finish(response) - else: - super(BaseRequestHandler, self)._handle_request_exception(error) - def _ok(self, data=None, headers=None, stream=None, content_type=None): return self.api_response(200, data=data, headers=headers, stream=stream, content_type=content_type) @@ -335,11 +346,8 @@ def _get_limit(self, default=20, maximum=1000): self._raise_bad_request_error('Invalid limit parameter') def _paginate(self, data=None, data_generator=None, sort=None): - try: - arg_page = self._get_page() - arg_limit = self._get_limit() - except HTTPError as error: - return self._bad_request(error.message) + arg_page = self._get_page() + arg_limit = self._get_limit() headers = { 'X-Pagination-Page': arg_page, diff --git a/tests/apiv2/test_auth.py b/tests/apiv2/test_auth.py new file mode 100644 index 0000000000..8c35601576 --- /dev/null +++ b/tests/apiv2/test_auth.py @@ -0,0 +1,37 @@ +# coding=utf-8 +"""Test /authentication route and authentication checks.""" +from __future__ import unicode_literals + +import json + +import pytest + + +@pytest.mark.gen_test +def test_no_api_key(http_client, create_url): + # given + expected = {'error': 'No authorization token.'} + + url = create_url('/log', api_key=None) + + # when + response = yield http_client.fetch(url, raise_error=False) + + # then + assert response.code == 401 + assert expected == json.loads(response.body) + + +@pytest.mark.gen_test +def test_bad_api_key(http_client, create_url): + # given + expected = {'error': 'No authorization token.'} + + url = create_url('/log', api_key='123') + + # when + response = yield http_client.fetch(url, raise_error=False) + + # then + assert response.code == 401 + assert expected == json.loads(response.body)