From dddc4ad4f160d32699078bf59f541016b24cb085 Mon Sep 17 00:00:00 2001 From: Jakub Kadlčík Date: Mar 05 2019 12:23:31 +0000 Subject: [frontend] rework error handlers to fix #531 There are multiple issues with our current implementation of error handling. First, `apiv3_ns` defines error handlers on application level, not blueprint level. This causes the #531 issue and results in showing API errors even for exceptions, that happened in UI. Also, `coprs_ns` defines error handler for 404, but only for the case, that some object is not found. It doesn't cover the case when a whole API endpoint is not found. We can't add it there, though. According to the Flask documentation http://flask.pocoo.org/docs/1.0/blueprints/#error-handlers it is not possible to add handlers for 404 and 405 on blueprint level. That's why it is currently implemented in `misc.py`. That basically means, that our error handlers are inconsistently spread within the application and we need to have some of them for `apiv3_ns` and `coprs_ns` blueprints and also a conditioned handler for 404 on application level. I have had multiple moments of clarity, thinking that I finally understand how the code works, but I didn't. And then again and gain, so I decided to rather have all handlers implemented the same way. --- diff --git a/frontend/coprs_frontend/coprs/__init__.py b/frontend/coprs_frontend/coprs/__init__.py index 62af04d..d41c549 100644 --- a/frontend/coprs_frontend/coprs/__init__.py +++ b/frontend/coprs_frontend/coprs/__init__.py @@ -74,6 +74,7 @@ from coprs.views.webhooks_ns import webhooks_ns from coprs.views.webhooks_ns import webhooks_general +from coprs.exceptions import ObjectNotFound, AccessRestricted, BadRequest, CoprHttpException from .context_processors import include_banner, inject_fedmenu, counter_processor setup_log() @@ -94,6 +95,42 @@ app.register_blueprint(webhooks_ns) app.add_url_rule("/", "coprs_ns.coprs_show", coprs_general.coprs_show) + +def get_error_handler(): + # http://flask.pocoo.org/docs/1.0/blueprints/#error-handlers + if flask.request.path.startswith('/api_3/'): + return apiv3_ns.APIErrorHandler() + return coprs_ns.UIErrorHandler() + + +@app.errorhandler(404) +@app.errorhandler(ObjectNotFound) +def handle_404(error): + error_handler = get_error_handler() + return error_handler.handle_404(error) + + +@app.errorhandler(403) +@app.errorhandler(AccessRestricted) +def handle_403(error): + error_handler = get_error_handler() + return error_handler.handle_403(error) + + +@app.errorhandler(400) +@app.errorhandler(BadRequest) +def handle_400(error): + error_handler = get_error_handler() + return error_handler.handle_400(error) + + +@app.errorhandler(500) +@app.errorhandler(CoprHttpException) +def handle_500(error): + error_handler = get_error_handler() + return error_handler.handle_500(error) + + app.jinja_env.trim_blocks = True app.jinja_env.lstrip_blocks = True diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py index 64d0824..ca9c9ec 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py @@ -6,7 +6,7 @@ import inspect from functools import wraps from werkzeug.datastructures import ImmutableMultiDict from coprs import app -from coprs.exceptions import CoprHttpException +from coprs.exceptions import CoprHttpException, ObjectNotFound from coprs.logic.complex_logic import ComplexLogic @@ -20,25 +20,35 @@ PUT = ["POST", "PUT"] DELETE = ["POST", "DELETE"] -@app.errorhandler(CoprHttpException) -def handle_copr_exception(error): - return handle_api_error(error.message, error.code) +class APIErrorHandler(object): + def handle_404(self, error): + if isinstance(error, ObjectNotFound): + return self.respond(str(error), 404) + return self.respond("Such API endpoint doesn't exist", 404) + def handle_403(self, error): + return self.handle_xxx(error) -@app.errorhandler(404) -def page_not_found(error): - return handle_api_error("Such API endpoint doesn't exist", 404) + def handle_400(self, error): + return self.handle_xxx(error) + def handle_500(self, error): + return self.respond("Request wasn't successful, there is probably a bug in the API code.", 500) -@app.errorhandler(500) -def page_not_found(error): - return handle_api_error("Request wasn't successful, there is probably a bug in the API code.", 500) + def handle_xxx(self, error): + return self.respond(self.message(error), error.code) + def respond(self, message, code): + response = flask.jsonify(error=message) + response.status_code = code + return response -def handle_api_error(message, code): - response = flask.jsonify(error=message) - response.status_code = code - return response + def message(self, error): + if isinstance(error, CoprHttpException): + return error.message + if hasattr(error, "description"): + return error.description + return str(error) def query_params(): diff --git a/frontend/coprs_frontend/coprs/views/coprs_ns/__init__.py b/frontend/coprs_frontend/coprs/views/coprs_ns/__init__.py index f9bf00f..6e2d8bf 100644 --- a/frontend/coprs_frontend/coprs/views/coprs_ns/__init__.py +++ b/frontend/coprs_frontend/coprs/views/coprs_ns/__init__.py @@ -3,26 +3,27 @@ import flask from coprs.views.misc import page_not_found, access_restricted, bad_request_handler, server_error_handler -from coprs.exceptions import ObjectNotFound, AccessRestricted, BadRequest, CoprHttpException +from coprs.exceptions import CoprHttpException coprs_ns = flask.Blueprint("coprs_ns", __name__, url_prefix="/coprs") -@coprs_ns.errorhandler(ObjectNotFound) -def handle_404(error): - return page_not_found(error.message) +class UIErrorHandler(object): + def handle_404(self, error): + return page_not_found(self.message(error)) + def handle_403(self, error): + return access_restricted(self.message(error)) -@coprs_ns.errorhandler(AccessRestricted) -def handle_403(error): - return access_restricted(error.message) + def handle_400(self, error): + return bad_request_handler(self.message(error)) + def handle_500(self, error): + return server_error_handler(self.message(error)) -@coprs_ns.errorhandler(BadRequest) -def handle_400(error): - return bad_request_handler(error.message) - - -@coprs_ns.errorhandler(CoprHttpException) -def handle_500(error): - return server_error_handler(error.message) + def message(self, error): + if isinstance(error, CoprHttpException): + return error.message + if hasattr(error, "description"): + return error.description + return str(error) diff --git a/frontend/coprs_frontend/coprs/views/misc.py b/frontend/coprs_frontend/coprs/views/misc.py index 6f18828..7de8441 100644 --- a/frontend/coprs_frontend/coprs/views/misc.py +++ b/frontend/coprs_frontend/coprs/views/misc.py @@ -79,12 +79,10 @@ def lookup_current_user(): models.User.username == username).first() -@app.errorhandler(404) def page_not_found(message): return flask.render_template("404.html", message=message), 404 -@app.errorhandler(403) def access_restricted(message): return flask.render_template("403.html", message=message), 403 @@ -103,9 +101,6 @@ def generic_error(message, code=500, title=None): server_error_handler = partial(generic_error, code=500, title="Internal Server Error") bad_request_handler = partial(generic_error, code=400, title="Bad Request") -app.errorhandler(500)(server_error_handler) -app.errorhandler(400)(bad_request_handler) - misc = flask.Blueprint("misc", __name__) diff --git a/frontend/coprs_frontend/tests/coprs_test_case.py b/frontend/coprs_frontend/tests/coprs_test_case.py index ff96f34..0e67eba 100644 --- a/frontend/coprs_frontend/tests/coprs_test_case.py +++ b/frontend/coprs_frontend/tests/coprs_test_case.py @@ -479,6 +479,11 @@ class CoprsTestCase(object): } ) + def post_api3_with_auth(self, url, content, user): + headers = {"Authorization": self._get_auth_string(user.api_login, user.api_token), + "Content-Type": "application/json"} + return self.tc.post(url, data=json.dumps(content), headers=headers) + class TransactionDecorator(object): diff --git a/frontend/coprs_frontend/tests/test_exceptions.py b/frontend/coprs_frontend/tests/test_exceptions.py new file mode 100644 index 0000000..cb2839f --- /dev/null +++ b/frontend/coprs_frontend/tests/test_exceptions.py @@ -0,0 +1,48 @@ +import json +import pytest +from tests.coprs_test_case import CoprsTestCase + + +class TestExceptionHandling(CoprsTestCase): + def test_json_only_for_api(self): + r1 = self.tc.get("/nonexisting/endpoint/") + assert r1.status_code == 404 + + with pytest.raises(json.JSONDecodeError): + json.loads(r1.data) + + r2 = self.tc.get("/api_3/nonexisting/endpoint/") + assert r2.status_code == 404 + data = json.loads(r2.data) + assert "error" in data + + def test_both_nonexisting_page_and_object(self): + r1 = self.tc.get("/nonexisting/endpoint/") + assert r1.status_code == 404 + + r2 = self.tc.get("/coprs/nonexisting/nonexisting/") + assert r2.status_code == 404 + + def test_both_nonexisting_page_and_object_api(self): + r1 = self.tc.get("/api_3/nonexisting/endpoint/") + assert r1.status_code == 404 + d1 = json.loads(r1.data) + assert "API endpoint" in d1["error"] + + r2 = self.tc.get("/api_3/project?ownername=nonexisting&projectname=nonexisting") + assert r2.status_code == 404 + d2 = json.loads(r2.data) + assert "Project nonexisting/nonexisting does not exist" in d2["error"] + + def test_api_401(self): + r1 = self.tc.post("api_3/project/add/someone") + assert r1.status_code == 401 + data = json.loads(r1.data) + assert "Login invalid/expired" in data["error"] + + def test_api_403(self, f_users, f_coprs, f_mock_chroots, f_users_api, f_db): + request_data = {"chroots": None, "description": "Changed description"} + r1 = self.post_api3_with_auth("api_3/project/edit/user2/foocopr", request_data, self.u3) + assert r1.status_code == 403 + data = json.loads(r1.data) + assert "Only owners and admins may update their projects" in data["error"]