From 059bb2250b5fe63f84f17558c0ff9425247e3303 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 18 Sep 2020 14:10:09 -0400 Subject: [PATCH 1/6] Add a BadRequest resource to help with BAD REQUEST --- src/allmydata/web/common.py | 3 ++- src/allmydata/web/common_py3.py | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 4a2bc2c3c..03db8463b 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -21,7 +21,8 @@ from allmydata.util.encodingutil import to_bytes, quote_output # Originally part of this module, so still part of its API: from .common_py3 import ( # noqa: F401 - get_arg, abbreviate_time, MultiFormatResource, WebError + get_arg, abbreviate_time, MultiFormatResource, WebError, + BadRequest, ) diff --git a/src/allmydata/web/common_py3.py b/src/allmydata/web/common_py3.py index 06751a8e8..f532e9000 100644 --- a/src/allmydata/web/common_py3.py +++ b/src/allmydata/web/common_py3.py @@ -23,6 +23,15 @@ class WebError(Exception): self.code = code +class BadRequest(resource.ErrorPage): + """ + ``BadRequest`` is a specialiation of ``ErrorPage`` which returns the HTTP + response code **BAD REQUEST**. + """ + def __init__(self, message=""): + resource.ErrorPage.__init__(self, http.BAD_REQUEST, "Bad Request", message) + + def get_arg(ctx_or_req, argname, default=None, multiple=False): """Extract an argument from either the query args (req.args) or the form body fields (req.fields). If multiple=False, this returns a single value From e648965fb6d8c46d8145df3c188a0947390b8435 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 18 Sep 2020 14:49:19 -0400 Subject: [PATCH 2/6] Add helpers to implement the desired exception behavior and use them --- src/allmydata/web/common.py | 100 ++++++++++++++++++++++++--------- src/allmydata/web/directory.py | 13 ++++- 2 files changed, 85 insertions(+), 28 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 03db8463b..ac748c237 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -204,33 +204,52 @@ def should_create_intermediate_directories(req): t not in ("delete", "rename", "rename-form", "check")) def humanize_failure(f): - # return text, responsecode - if f.check(EmptyPathnameComponentError): + """ + Create an human-oriented description of a failure along with some HTTP + metadata. + + :param Failure f: The failure to describe. + + :return (bytes, int): A tuple of some prose and an HTTP code describing + the failure. + """ + return humanize_exception(f.value) + + +def humanize_exception(exc): + """ + Like ``humanize_failure`` but for an exception. + + :param Exception exc: The exception to describe. + + :return: See ``humanize_failure``. + """ + if isinstance(exc, EmptyPathnameComponentError): return ("The webapi does not allow empty pathname components, " "i.e. a double slash", http.BAD_REQUEST) - if f.check(ExistingChildError): + if isinstance(exc, ExistingChildError): return ("There was already a child by that name, and you asked me " "to not replace it.", http.CONFLICT) - if f.check(NoSuchChildError): - quoted_name = quote_output(f.value.args[0], encoding="utf-8", quotemarks=False) + if isinstance(exc, NoSuchChildError): + quoted_name = quote_output(exc.args[0], encoding="utf-8", quotemarks=False) return ("No such child: %s" % quoted_name, http.NOT_FOUND) - if f.check(NotEnoughSharesError): + if isinstance(exc, NotEnoughSharesError): t = ("NotEnoughSharesError: This indicates that some " "servers were unavailable, or that shares have been " "lost to server departure, hard drive failure, or disk " "corruption. You should perform a filecheck on " "this object to learn more.\n\nThe full error message is:\n" - "%s") % str(f.value) + "%s") % str(exc) return (t, http.GONE) - if f.check(NoSharesError): + if isinstance(exc, NoSharesError): t = ("NoSharesError: no shares could be found. " "Zero shares usually indicates a corrupt URI, or that " "no servers were connected, but it might also indicate " "severe corruption. You should perform a filecheck on " "this object to learn more.\n\nThe full error message is:\n" - "%s") % str(f.value) + "%s") % str(exc) return (t, http.GONE) - if f.check(UnrecoverableFileError): + if isinstance(exc, UnrecoverableFileError): t = ("UnrecoverableFileError: the directory (or mutable file) could " "not be retrieved, because there were insufficient good shares. " "This might indicate that no servers were connected, " @@ -239,9 +258,9 @@ def humanize_failure(f): "failure, or disk corruption. You should perform a filecheck on " "this object to learn more.") return (t, http.GONE) - if f.check(MustNotBeUnknownRWError): - quoted_name = quote_output(f.value.args[1], encoding="utf-8") - immutable = f.value.args[2] + if isinstance(exc, MustNotBeUnknownRWError): + quoted_name = quote_output(exc.args[1], encoding="utf-8") + immutable = exc.args[2] if immutable: t = ("MustNotBeUnknownRWError: an operation to add a child named " "%s to a directory was given an unknown cap in a write slot.\n" @@ -261,29 +280,30 @@ def humanize_failure(f): "writecap in the write slot if desired, would also work in this " "case.") % quoted_name return (t, http.BAD_REQUEST) - if f.check(MustBeDeepImmutableError): - quoted_name = quote_output(f.value.args[1], encoding="utf-8") + if isinstance(exc, MustBeDeepImmutableError): + quoted_name = quote_output(exc.args[1], encoding="utf-8") t = ("MustBeDeepImmutableError: a cap passed to this operation for " "the child named %s, needed to be immutable but was not. Either " "the cap is being added to an immutable directory, or it was " "originally retrieved from an immutable directory as an unknown " "cap.") % quoted_name return (t, http.BAD_REQUEST) - if f.check(MustBeReadonlyError): - quoted_name = quote_output(f.value.args[1], encoding="utf-8") + if isinstance(exc, MustBeReadonlyError): + quoted_name = quote_output(exc.args[1], encoding="utf-8") t = ("MustBeReadonlyError: a cap passed to this operation for " "the child named '%s', needed to be read-only but was not. " "The cap is being passed in a read slot (ro_uri), or was retrieved " "from a read slot as an unknown cap.") % quoted_name return (t, http.BAD_REQUEST) - if f.check(blacklist.FileProhibited): - t = "Access Prohibited: %s" % quote_output(f.value.reason, encoding="utf-8", quotemarks=False) + if isinstance(exc, blacklist.FileProhibited): + t = "Access Prohibited: %s" % quote_output(exc.reason, encoding="utf-8", quotemarks=False) return (t, http.FORBIDDEN) - if f.check(WebError): - return (f.value.text, f.value.code) - if f.check(FileTooLargeError): - return (f.getTraceback(), http.REQUEST_ENTITY_TOO_LARGE) - return (str(f), None) + if isinstance(exc, WebError): + return (exc.text, exc.code) + if isinstance(exc, FileTooLargeError): + return ("FileTooLargeError: %s" % (exc,), http.REQUEST_ENTITY_TOO_LARGE) + return (str(exc), None) + class MyExceptionHandler(appserver.DefaultExceptionHandler, object): def simple(self, ctx, text, code=http.BAD_REQUEST): @@ -481,8 +501,38 @@ class TokenOnlyWebApi(resource.Resource, object): req.setResponseCode(e.code) return json.dumps({"error": e.text}) except Exception as e: - message, code = humanize_failure(Failure()) + message, code = humanize_exception(e) req.setResponseCode(500 if code is None else code) return json.dumps({"error": message}) else: raise WebError("'%s' invalid type for 't' arg" % (t,), http.BAD_REQUEST) + + +def exception_to_child(f): + """ + Decorate ``getChild`` method with exception handling behavior to render an + error page reflecting the exception. + """ + @wraps(f) + def g(self, name, req): + try: + return m(self, name, req) + except Exception as e: + description, status = humanize_exception(e) + return ErrorPage(status, "Error", description) + return g + + +def render_exception(m): + """ + Decorate a ``render_*`` method with exception handling behavior to render + an error page reflecting the exception. + """ + @wraps(m) + def g(self, request): + try: + return m(self, request) + except Exception as e: + description, status = humanize_exception(e) + return ErrorPage(status, "Error", description).render(request) + return g diff --git a/src/allmydata/web/directory.py b/src/allmydata/web/directory.py index cabcd023e..3ad935bab 100644 --- a/src/allmydata/web/directory.py +++ b/src/allmydata/web/directory.py @@ -2,6 +2,7 @@ import json import urllib from datetime import timedelta +from functools import wraps from zope.interface import implementer from twisted.internet import defer @@ -48,6 +49,7 @@ from allmydata.web.common import ( parse_replace_arg, should_create_intermediate_directories, humanize_failure, + humanize_exception, convert_children_json, get_format, get_mutable_type, @@ -55,6 +57,8 @@ from allmydata.web.common import ( render_time, MultiFormatResource, SlotsSequenceElement, + exception_to_child, + render_exception, ) from allmydata.web.filenode import ReplaceMeMixin, \ FileNodeHandler, PlaceHolderNodeHandler @@ -94,6 +98,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): self.name = name self._operations = client.get_web_service().get_operations() + @exception_to_child def getChild(self, name, req): """ Dynamically create a child for the given request and name @@ -113,9 +118,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): # we will follow suit. for segment in req.prepath: if not segment: - raise EmptyPathnameComponentError( - u"The webapi does not allow empty pathname components", - ) + raise EmptyPathnameComponentError() d = self.node.get(name) d.addBoth(self._got_child, req, name) @@ -210,6 +213,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): d.addCallback(lambda res: self.node.get_uri()) return d + @render_exception def render_GET(self, req): # This is where all of the directory-related ?t=* code goes. t = get_arg(req, "t", "").strip() @@ -248,6 +252,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): raise WebError("GET directory: bad t=%s" % t) + @render_exception def render_PUT(self, req): t = get_arg(req, "t", "").strip() replace = parse_replace_arg(get_arg(req, "replace", "true")) @@ -267,6 +272,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): raise WebError("PUT to a directory") + @render_exception def render_POST(self, req): t = get_arg(req, "t", "").strip() @@ -1458,6 +1464,7 @@ class UnknownNodeHandler(Resource, object): self.parentnode = parentnode self.name = name + @render_exception def render_GET(self, req): t = get_arg(req, "t", "").strip() if t == "info": From 8f3a32a22c08e213b376d911c87efced46ba63d0 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 18 Sep 2020 14:49:39 -0400 Subject: [PATCH 3/6] news fragment --- newsfragments/3422.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3422.minor diff --git a/newsfragments/3422.minor b/newsfragments/3422.minor new file mode 100644 index 000000000..e69de29bb From 97872118a59eeb5e488570ff599c95ea0eb8aef3 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 18 Sep 2020 14:50:45 -0400 Subject: [PATCH 4/6] derived function below --- src/allmydata/web/common.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index ac748c237..e6e818e7a 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -203,19 +203,6 @@ def should_create_intermediate_directories(req): return bool(req.method in ("PUT", "POST") and t not in ("delete", "rename", "rename-form", "check")) -def humanize_failure(f): - """ - Create an human-oriented description of a failure along with some HTTP - metadata. - - :param Failure f: The failure to describe. - - :return (bytes, int): A tuple of some prose and an HTTP code describing - the failure. - """ - return humanize_exception(f.value) - - def humanize_exception(exc): """ Like ``humanize_failure`` but for an exception. @@ -305,6 +292,19 @@ def humanize_exception(exc): return (str(exc), None) +def humanize_failure(f): + """ + Create an human-oriented description of a failure along with some HTTP + metadata. + + :param Failure f: The failure to describe. + + :return (bytes, int): A tuple of some prose and an HTTP code describing + the failure. + """ + return humanize_exception(f.value) + + class MyExceptionHandler(appserver.DefaultExceptionHandler, object): def simple(self, ctx, text, code=http.BAD_REQUEST): req = IRequest(ctx) From b200d2043093b6746cc5a4951b5a5eb271faa498 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 18 Sep 2020 15:01:53 -0400 Subject: [PATCH 5/6] minor cleanups/rearranging --- src/allmydata/web/common.py | 15 +++++++-------- src/allmydata/web/common_py3.py | 9 --------- src/allmydata/web/directory.py | 3 +-- 3 files changed, 8 insertions(+), 19 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index e6e818e7a..6035e2884 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -1,10 +1,10 @@ import time import json +from functools import wraps from twisted.web import http, server, resource, template from twisted.python import log -from twisted.python.failure import Failure from nevow import loaders, appserver from nevow.rend import Page from nevow.inevow import IRequest @@ -22,7 +22,6 @@ from allmydata.util.encodingutil import to_bytes, quote_output # Originally part of this module, so still part of its API: from .common_py3 import ( # noqa: F401 get_arg, abbreviate_time, MultiFormatResource, WebError, - BadRequest, ) @@ -516,23 +515,23 @@ def exception_to_child(f): @wraps(f) def g(self, name, req): try: - return m(self, name, req) + return f(self, name, req) except Exception as e: description, status = humanize_exception(e) - return ErrorPage(status, "Error", description) + return resource.ErrorPage(status, "Error", description) return g -def render_exception(m): +def render_exception(f): """ Decorate a ``render_*`` method with exception handling behavior to render an error page reflecting the exception. """ - @wraps(m) + @wraps(f) def g(self, request): try: - return m(self, request) + return f(self, request) except Exception as e: description, status = humanize_exception(e) - return ErrorPage(status, "Error", description).render(request) + return resource.ErrorPage(status, "Error", description).render(request) return g diff --git a/src/allmydata/web/common_py3.py b/src/allmydata/web/common_py3.py index f532e9000..06751a8e8 100644 --- a/src/allmydata/web/common_py3.py +++ b/src/allmydata/web/common_py3.py @@ -23,15 +23,6 @@ class WebError(Exception): self.code = code -class BadRequest(resource.ErrorPage): - """ - ``BadRequest`` is a specialiation of ``ErrorPage`` which returns the HTTP - response code **BAD REQUEST**. - """ - def __init__(self, message=""): - resource.ErrorPage.__init__(self, http.BAD_REQUEST, "Bad Request", message) - - def get_arg(ctx_or_req, argname, default=None, multiple=False): """Extract an argument from either the query args (req.args) or the form body fields (req.fields). If multiple=False, this returns a single value diff --git a/src/allmydata/web/directory.py b/src/allmydata/web/directory.py index 3ad935bab..4d7aa1bd1 100644 --- a/src/allmydata/web/directory.py +++ b/src/allmydata/web/directory.py @@ -2,7 +2,6 @@ import json import urllib from datetime import timedelta -from functools import wraps from zope.interface import implementer from twisted.internet import defer @@ -680,7 +679,7 @@ class DirectoryAsHTML(Element): try: children = yield self.node.list() except Exception as e: - text, code = humanize_failure(Failure(e)) + text, code = humanize_exception(e) children = None self.dirnode_children_error = text From 0e139114f760bc0aa0788e2f492ca8b313ff1440 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 21 Sep 2020 14:07:11 -0400 Subject: [PATCH 6/6] add a limited amount of missing test coverage for humanize_exception --- src/allmydata/test/web/test_web.py | 40 +++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/web/test_web.py b/src/allmydata/test/web/test_web.py index 11c5a8c5c..8189542c7 100644 --- a/src/allmydata/test/web/test_web.py +++ b/src/allmydata/test/web/test_web.py @@ -59,7 +59,11 @@ from .common import ( unknown_immcap, ) -from allmydata.interfaces import IMutableFileNode, SDMF_VERSION, MDMF_VERSION +from allmydata.interfaces import ( + IMutableFileNode, SDMF_VERSION, MDMF_VERSION, + FileTooLargeError, + MustBeReadonlyError, +) from allmydata.mutable import servermap, publish, retrieve from .. import common_util as testutil from ..common_py3 import TimezoneMixin @@ -67,6 +71,10 @@ from ..common_web import ( do_http, Error, ) +from ...web.common import ( + humanize_exception, +) + from allmydata.client import _Client, SecretHolder # create a fake uploader/downloader, and a couple of fake dirnodes, then @@ -4790,3 +4798,33 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, testutil.ReallyEqualMixi # doesn't reveal anything. This addresses #1720. d.addCallback(lambda e: self.assertEquals(str(e), "404 Not Found")) return d + + +class HumanizeExceptionTests(TrialTestCase): + """ + Tests for ``humanize_exception``. + """ + def test_mustbereadonly(self): + """ + ``humanize_exception`` describes ``MustBeReadonlyError``. + """ + text, code = humanize_exception( + MustBeReadonlyError( + "URI:DIR2 directory writecap used in a read-only context", + "", + ), + ) + self.assertIn("MustBeReadonlyError", text) + self.assertEqual(code, http.BAD_REQUEST) + + def test_filetoolarge(self): + """ + ``humanize_exception`` describes ``FileTooLargeError``. + """ + text, code = humanize_exception( + FileTooLargeError( + "This file is too large to be uploaded (data_size).", + ), + ) + self.assertIn("FileTooLargeError", text) + self.assertEqual(code, http.REQUEST_ENTITY_TOO_LARGE)