Merge pull request #815 from tahoe-lafs/3422.directory-weberror

Handle exceptions raised by getChild and render_* in directory.py

Fixes: ticket:3422

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",
+                "<unknown name>",
+            ),
+        )
+        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)

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
@@ -21,7 +21,7 @@ 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,
 )
 
 
@@ -202,34 +202,40 @@ 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):
-    # return text, responsecode
-    if f.check(EmptyPathnameComponentError):
+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, "
@@ -238,9 +244,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"
@@ -260,29 +266,43 @@ 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)
+
+
+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):
@@ -480,8 +500,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 f(self, name, req)
+        except Exception as e:
+            description, status = humanize_exception(e)
+            return resource.ErrorPage(status, "Error", description)
+    return g
+
+
+def render_exception(f):
+    """
+    Decorate a ``render_*`` method with exception handling behavior to render
+    an error page reflecting the exception.
+    """
+    @wraps(f)
+    def g(self, request):
+        try:
+            return f(self, request)
+        except Exception as e:
+            description, status = humanize_exception(e)
+            return resource.ErrorPage(status, "Error", description).render(request)
+    return g

src/allmydata/web/directory.py

@@ -48,6 +48,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 +56,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 +97,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 +117,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 +212,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 +251,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 +271,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()
 
@@ -674,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
 
@@ -1458,6 +1463,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":