Fix uncaught error when parsing rest headers, document

Started this work as better docs for rest response format. But I found
406 error response was not being tested. Also there was no error for
bad Content-Type.

In rest.py fix uncaught exceptions due to invalid Accept or
Content-Type headers. If Content-type is valid but not
application/json return code 415.

Document use of accept header (was only shown in examples) and support
for q parameter. Describe using .xml and .json extensions to select
return format for testing from browser (where setting accept header is
a problem). Document 406 error code return. Document 415 error code
return and acceptable content types. Previously only doc was in
examples.

Set up tests for 406 and 415 error codes.

Author: rouilj

CHANGES.txt

@@ -63,6 +63,9 @@ Fixed:
 - fixed some issues when generating translations. Use mappings and
   named format parameters so translators can move substituted tokens
   in tranlsations.
+- in rest interface, fix uncaught exceptions when parsing invalid
+  Content-Type and Accept headers. Document response formats more
+  fully in doc/rest.txt.
 
 Features:
 - issue2550522 - Add 'filter' command to command-line

doc/rest.txt

@@ -187,6 +187,36 @@ If an explicit version is not provided, the server default is used.
 The server default is reported by querying the ``/rest/`` endpoint as
 described above.
 
+Input Formats
+=============
+
+Content-Type is allowed to be ``application/json`` or
+``application/x-www-form-urlencoded``. Any other value returns error
+code 415.
+
+Response Formats
+================
+
+The default response format is json.
+
+If you add the ``dicttoxml.py`` module you can request XML formatted
+data using the header ``Accept: application/xml`` in your
+request. Both output formats are similar in structure.
+
+The rest interface accepts the http accept header and can include
+``q`` values to specify the preferred mechanism. This is the preferred
+way to specify alternate acceptable response formats.
+
+To make testing from the browser easier, you can also append the
+extension `.json` or `.xml` to the path component of the url. This
+will force json or xml (if supported) output. If you use an extension
+it takes priority over any accept headers.
+
+The rest interface returns status 406 if you use an unrecognized
+extension.  You will also get a 406 status if none of the entries in
+the accept header are available or if the accept header is invalid.
+
+
 General Guidelines
 ==================
 
@@ -200,12 +230,8 @@ The exact details of returned data is determined by the value of the
 ``@verbose`` query parameter.  The various supported values and their
 effects are described in the following sections.
 
-The default return format is JSON. If you add the ``dicttoxml.py``
-module you can request XML formatted data using the header ``Accept:
-application/xml`` in your request. Both output formats are similar in
-structure.
-
-All output is wrapped in an envelope called ``data``.
+All output is wrapped in an envelope called ``data``. The output
+format is described in `Response Formats`_ above.
 
 When using collection endpoints (think list of issues, users ...), the
 ``data`` envelope contains metadata (e.g. total number of items) as

roundup/rest.py

@@ -223,7 +223,10 @@ def parse_accept_header(accept):
         media_params = []
         # convert vendor-specific content types into something useful (see
         # docstring)
-        typ, subtyp = media_type.split('/')
+        try:
+            typ, subtyp = media_type.split('/')
+        except ValueError:
+            raise UsageError("Invalid media type: %s"%media_type)
         # check for a + in the sub-type
         if '+' in subtyp:
             # if it exists, determine if the subtype is a vendor-specific type
@@ -246,7 +249,10 @@ def parse_accept_header(accept):
                 media_type = '{}/{}'.format(typ, extra)
         q = 1.0
         for part in parts:
-            (key, value) = part.lstrip().split("=", 1)
+            try:
+                (key, value) = part.lstrip().split("=", 1)
+            except ValueError:
+                raise UsageError("Invalid param: %s"%part.lstrip())
             key = key.strip()
             value = value.strip()
             if key == "q":
@@ -1873,7 +1879,15 @@ def dispatch(self, method, uri, input):
         # parse Accept header and get the content type
         # Acceptable types ordered with preferred one first
         # in list.
-        accept_header = parse_accept_header(headers.get('Accept'))
+        try:
+            accept_header = parse_accept_header(headers.get('Accept'))
+        except UsageError as e:
+            output = self.error_obj(406, _("Unable to parse Accept Header. %(error)s. "
+                      "Acceptable types: %(acceptable_types)s") % {
+                          'error': e.args[0],
+                          'acceptable_types': " ".join(sorted(self.__accepted_content_type.keys()))})
+            accept_header = []
+
         if not accept_header:
             accept_type = self.__default_accept_type
         else:
@@ -1913,7 +1927,12 @@ def dispatch(self, method, uri, input):
         #            header (Accept: application/json, application/xml)
         #            default (application/json)
         ext_type = os.path.splitext(urlparse(uri).path)[1][1:]
-        data_type = ext_type or accept_type or "invalid"
+
+        # headers.get('Accept') is never empty if called here.
+        # accept_type will be set to json if there is no Accept header
+        # accept_type wil be empty only if there is an Accept header
+        # with invalid values.
+        data_type = ext_type or accept_type or headers.get('Accept') or "invalid"
 
         if (ext_type):
             # strip extension so uri make sense
@@ -1956,6 +1975,10 @@ def dispatch(self, method, uri, input):
                     input = SimulateFieldStorageFromJson(b2s(input.value))
                 except ValueError as msg:
                     output = self.error_obj(400, msg)
+            else:
+                    output = self.error_obj(415,
+                                "Unable to process input of type %s" %
+                                            content_type_header)
 
         # check for pretty print
         try:

test/rest_common.py

@@ -1321,6 +1321,204 @@ def testDispatchPost(self):
                           ['assignedto']['link'],
            "http://tracker.example/cgi-bin/roundup.cgi/bugs/rest/data/user/2")
 
+    def testDispatchBadContent(self):
+        """
+        runthrough rest dispatch() with bad content_type patterns.
+        """
+
+        # simulate: /rest/data/issue
+        body=b'{ "title": "Joe Doe has problems", \
+                 "nosy": [ "1", "3" ], \
+                 "assignedto": "2", \
+                 "abool": true, \
+                 "afloat": 2.3, \
+                 "anint": 567890 \
+        }'
+        env = { "CONTENT_TYPE": "application/jzot",
+                "CONTENT_LENGTH": len(body),
+                "REQUEST_METHOD": "POST"
+        }
+
+        headers={"accept": "application/json; version=1",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 415)
+        json_dict = json.loads(b2s(results))
+        self.assertEqual(json_dict['error']['msg'],
+                         "Unable to process input of type application/jzot")
+
+        # Test GET as well. I am not sure if this should pass or not.
+        # Arguably GET doesn't use any form/json input but....
+        results = self.server.dispatch('GET',
+                            "/rest/data/issue",
+                            form)
+        print(results)
+        self.assertEqual(self.server.client.response_code, 415)
+
+
+
+    def testDispatchBadAccept(self):
+        # simulate: /rest/data/issue expect failure unknown accept settings
+        body=b'{ "title": "Joe Doe has problems", \
+                 "nosy": [ "1", "3" ], \
+                 "assignedto": "2", \
+                 "abool": true, \
+                 "afloat": 2.3, \
+                 "anint": 567890 \
+        }'
+        env = { "CONTENT_TYPE": "application/json",
+                "CONTENT_LENGTH": len(body),
+                "REQUEST_METHOD": "POST"
+        }
+
+        headers={"accept": "application/zot; version=1; q=0.5",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 406)
+        self.assertIn(b"Requested content type 'application/zot; version=1; q=0.5' is not available.\nAcceptable types: */*, application/json,", results)
+
+        # simulate: /rest/data/issue works, multiple acceptable output, one
+        # is valid
+        env = { "CONTENT_TYPE": "application/json",
+                "CONTENT_LENGTH": len(body),
+                "REQUEST_METHOD": "POST"
+        }
+
+        headers={"accept": "application/zot; version=1; q=0.75, "
+                           "application/json; version=1; q=0.5",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 201)
+        json_dict = json.loads(b2s(results))
+        # ERROR this should be 1. What's happening is that the code
+        # for handling 406 error code runs through everything and creates
+        # the item. Then it throws a 406 after the work is done when it
+        # realizes it can't respond as requested. So the 406 post above
+        # creates issue 1 and this one creates issue 2.
+        self.assertEqual(json_dict['data']['id'], "2")
+
+
+        # test 3 accept is empty. This triggers */* so passes
+        headers={"accept": "",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 201)
+        json_dict = json.loads(b2s(results))
+        # This is one more than above. Will need to be fixed
+        # When error above is fixed.
+        self.assertEqual(json_dict['data']['id'], "3")
+
+        # test 4 accept is random junk.
+        headers={"accept": "Xyzzy I am not a mime, type;",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 406)
+        json_dict = json.loads(b2s(results))
+        self.assertIn('Unable to parse Accept Header. Invalid media type: Xyzzy I am not a mime. Acceptable types: */* application/json', json_dict['error']['msg'])
+
+        # test 5 accept mimetype is ok, param is not
+        headers={"accept": "*/*; foo",
+                 "content-type": env['CONTENT_TYPE'],
+                 "content-length": env['CONTENT_LENGTH'],
+        }
+
+        self.headers=headers
+        # we need to generate a FieldStorage the looks like
+        #  FieldStorage(None, None, 'string') rather than
+        #  FieldStorage(None, None, [])
+        body_file=BytesIO(body)  # FieldStorage needs a file
+        form = client.BinaryFieldStorage(body_file,
+                                headers=headers,
+                                environ=env)
+        self.server.client.request.headers.get=self.get_header
+        results = self.server.dispatch(env["REQUEST_METHOD"],
+                            "/rest/data/issue",
+                            form)
+
+        print(results)
+        self.assertEqual(self.server.client.response_code, 406)
+        json_dict = json.loads(b2s(results))
+        self.assertIn('Unable to parse Accept Header. Invalid param: foo. Acceptable types: */* application/json', json_dict['error']['msg'])
 
     def testStatsGen(self):
         # check stats being returned by put and get ops