fix(REST): issue2551383; improve errors for bad json, fix PUT docs

While adding fuzz testing for email addresses via REST
/rest/data/user/1/address, I had an error when setting the address to
the same value it currently had. Traced this to a bug in
userauditor.py. Fixed the bug. Documented in upgrading.txt.

While trying to track down issue, I realized invalid json was being
accepted without error. So I fixed the code that parses the json and
have it return an error. Also modified some tests that broke (used
invalid json, or passed body (e.g. DELETE) but shouldn't have. Add
tests for bad json to verify new code.

Fixed test that wasn't initializing the body_file in each loop, so the
test wasn't actually supplying a body.

Also realised PUT documentation was not correct. Output format isn't
quite like GET.

Fuss tests for email address also added.

Author: rouilj

CHANGES.txt

@@ -43,6 +43,12 @@ Fixed:
   incorrectly. (John Rouillard)
 - issue2551382 - invalid @verbose, @page_* values in rest uri's
   generate 409 not 400 error. (John Rouillard)
+- fix issues with rest doc and use of PUT on a property item. Response
+  is similar to use of PUT on the item, not a GET on the
+  item. Discovered while fuzz testing. (John Rouillard)
+- issue2551383 - Setting same address via REST PUT command results in
+  an error. Now the userauditor does not trigger an error if a user
+  sets the primary address to the existing value. (John Rouillard)
 
 Features:
 
@@ -71,7 +77,10 @@ Features:
   endpoint. Raw file/msg data can be retrieved using the
   /binary_content attribute and an Accept header to select the mime
   type for the data (e.g. image/png for a png file). The existing html
-  interface method still works and is supported, but is legacy.
+  interface method still works and is supported, but is legacy. (John
+  Rouillard) 
+- added fuzz testing for some code. Found issue2551382 and
+  others. (John Rouillard)
 
 2024-07-13 2.4.0
 

doc/rest.txt

@@ -1392,10 +1392,12 @@ Other Supported Methods for Items
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 The method ``PUT`` is allowed on individual items, e.g.
-``/data/issue/42`` On success it returns the same parameters as the
-respective ``GET`` method. Note that for ``PUT`` an Etag has to be
-supplied, either in the request header or as an @etag parameter. An
-example::
+``/data/issue/42`` On success it returns a data structure similar to
+the respective ``GET`` method. However it is only concerned with the
+changes that have occurred. Since it is not a full ``GET`` request, it
+doesn't include an etag or unchanged attributes. Note that for ``PUT``
+an Etag has to be supplied, either in the request header or as an
+@etag parameter. An example::
 
   curl -u admin:admin -X PUT \
      --header 'Referer: https://example.com/demo/' \
@@ -1615,7 +1617,22 @@ Other Supported Methods for fields
 
 The method ``PUT`` is allowed on a property e.g.,
 ``/data/issue/42/title``. On success it returns the same parameters as
-the respective ``GET`` method. Note that for ``PUT`` an Etag has to be
+the respective ``PUT`` method on the item. For example::
+
+  {
+    "data": {
+        "id": "42",
+        "type": "issue",
+        "link": "https://.../demo/rest/data/issue/42",
+        "attribute": {
+            "title": "this is a new title"
+        }
+    }
+  }
+
+If the new value for the title was the same as on the server, the
+attribute property would be empty since the value was not changed.
+Note that for ``PUT`` an Etag has to be
 supplied, either in the request header or as an @etag parameter.
 Example using multipart/form-data rather than json::
 
@@ -2438,3 +2455,16 @@ Rate limit tests::
 
 will show you the number of remaining requests to the REST interface
 for the user identified by password.
+
+
+Notes V2 API
+~~~~~~~~~~~~
+
+These may never be implemented but, some nits to consider.
+
+The shape of a GET and PUT/PATCH responses are different. "attributes"
+is used for GET and "attribute" is used with PATCH/PUT. A PATCH or a
+PUT can update multiple properties when used with an item endpoint.
+"attribute" kind of makes sense when used with a property endpoint
+but.... Maybe standardize on one shape so the client doesn't have to
+special case?

doc/upgrading.txt

@@ -133,6 +133,32 @@ to::
 at the top of both files. The icing macro used in other tracker
 templates was renamed to frame in this tracker template.
 
+Update userauditor.py detector (recommended)
+--------------------------------------------
+
+When using the REST interface, setting the address property of the
+user to the same value it currently has resulted in an error.
+
+If you have not changed your userauditor, you can copy one from any of
+the supplied templates in the ``detectors/userauditor.py`` file. Use
+``roundup-admin templates`` to find a list of template directories.
+
+If you have changed your userauditor from the stock version, apply the
+following diff::
+
+               raise ValueError('Email address syntax is invalid
+               "%s"'%address)
+  
+           check_main = db.user.stringFind(address=address)
+  +        # allow user to set same address via rest
+  +        if check_main:
+  +            check_main = nodeid not in check_main
+  +
+         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
+
+add the lines marked with ``+`` in the file in the location after
+check_main is assigned.
+
 More secure session cookie handling (info)
 ------------------------------------------
 

roundup/rest.py

@@ -2729,17 +2729,27 @@ class SimulateFieldStorageFromJson():
 
     '''
     def __init__(self, json_string):
-        ''' Parse the json string into an internal dict. '''
+        '''Parse the json string into an internal dict.
+
+            Because spec for rest post once exactly (POE) shows
+            posting empty content. An empty string results in an empty
+            dict, not a json parse error.
+        '''
         def raise_error_on_constant(x):
             raise ValueError("Unacceptable number: %s" % x)
+        if json_string == "":
+            self.json_dict = {}
+            self.value = None
+            return
+
         try:
             self.json_dict = json.loads(json_string,
                                     parse_constant=raise_error_on_constant)
             self.value = [self.FsValue(index, self.json_dict[index])
                           for index in self.json_dict]
-        except ValueError:
-            self.json_dict = {}
-            self.value = None
+        except (json.decoder.JSONDecodeError, ValueError) as e:
+            raise ValueError(e.args[0] + ". JSON is: " + json_string)
+
 
     class FsValue:
         '''Class that does nothing but response to a .value property '''

share/roundup/templates/classic/detectors/userauditor.py

@@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues):
             raise ValueError('Email address syntax is invalid "%s"'%address)
 
         check_main = db.user.stringFind(address=address)
+        # allow user to set same address via rest
+        if check_main:
+            check_main = nodeid not in check_main
+
         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
         check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid]
         if check_main or check_alts:

share/roundup/templates/devel/detectors/userauditor.py

@@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues):
             raise ValueError('Email address syntax is invalid "%s"'%address)
 
         check_main = db.user.stringFind(address=address)
+        # allow user to set same address via rest
+        if check_main:
+            check_main = nodeid not in check_main
+
         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
         check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid]
         if check_main or check_alts:

share/roundup/templates/jinja2/detectors/userauditor.py

@@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues):
             raise ValueError('Email address syntax is invalid "%s"'%address)
 
         check_main = db.user.stringFind(address=address)
+        # allow user to set same address via rest
+        if check_main:
+            check_main = nodeid not in check_main
+
         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
         check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid]
         if check_main or check_alts:

share/roundup/templates/minimal/detectors/userauditor.py

@@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues):
             raise ValueError('Email address syntax is invalid "%s"'%address)
 
         check_main = db.user.stringFind(address=address)
+        # allow user to set same address via rest
+        if check_main:
+            check_main = nodeid not in check_main
+
         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
         check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid]
         if check_main or check_alts:

share/roundup/templates/responsive/detectors/userauditor.py

@@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues):
             raise ValueError('Email address syntax is invalid "%s"'%address)
 
         check_main = db.user.stringFind(address=address)
+        # allow user to set same address via rest
+        if check_main:
+            check_main = nodeid not in check_main
+
         # make sure none of the alts are owned by anyone other than us (x!=nodeid)
         check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid]
         if check_main or check_alts:

test/rest_common.py

@@ -2427,6 +2427,89 @@ def testDispatchBadAccept(self):
         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 testBadJson(self):
+        '''Run some JSON we don't accept through the wringer
+        '''
+        body=b'{ "title": "Joe Doe has problems", \
+                 "nosy": [ "1", "3" ], \
+                 "assignedto": "2", \
+                 "abool": true, \
+                 "afloat": 2.3, \
+                 "anint": Infinity }'
+
+        expected={ "error":
+                   {"status": 400,
+                    "msg": ("Unacceptable number: Infinity. JSON is: "
+                            + b2s(body)),
+                   }
+        }
+
+        env = { "CONTENT_TYPE": "application/json",
+                "CONTENT_LENGTH": len(body),
+                "REQUEST_METHOD": "PUT"
+        }
+        self.server.client.env.update(env)
+        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/1",
+                            form)
+
+        self.assertEqual(json.loads(results), expected)
+
+        body=b'{ "title": "Joe Doe has problems", \
+                 nosy: [ "1", "3" ], \
+                 "assignedto": "2", \
+                 "abool": true, \
+                 "afloat": 2.3, \
+                 "anint": Infinity }'
+        self.maxDiff = None
+        expected={ "error":
+                   {"status": 400,
+                    "msg": ("Expecting property name enclosed in double "
+                            "quotes: line 1 column 53 (char 52). JSON is: "
+                            + b2s(body)),
+                   }
+        }
+
+        env = { "CONTENT_TYPE": "application/json",
+                "CONTENT_LENGTH": len(body),
+                "REQUEST_METHOD": "PUT"
+        }
+        self.server.client.env.update(env)
+        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/1",
+                            form)
+
+        self.assertEqual(json.loads(results), expected)
+
+
     def testStatsGen(self):
         # check stats being returned by put and get ops
         # using dispatch which parses the @stats query param
@@ -2835,27 +2918,20 @@ def testDispatch(self):
         etag = calculate_etag(self.db.issue.getnode("1"),
                               self.db.config['WEB_SECRET_KEY'])
         etagb = etag.strip ('"')
-        env = {"CONTENT_TYPE": "application/json",
-               "CONTENT_LEN": 0,
-               "REQUEST_METHOD": "DELETE" }
+        env = {"REQUEST_METHOD": "DELETE" }
         self.server.client.env.update(env)
         # use text/plain header and request json output by appending
         # .json to the url.
         headers={"accept": "text/plain",
-                 "content-type": env['CONTENT_TYPE'],
                  "if-match": '"%s"'%etagb,
                  "content-length": 0,
         }
         self.headers=headers
-        body_file=BytesIO(b'')  # FieldStorage needs a file
-        form = client.BinaryFieldStorage(body_file,
-                                headers=headers,
-                                environ=env)
         self.server.client.request.headers.get=self.get_header
         self.db.setCurrentUser('admin') # must be admin to delete issue
         results = self.server.dispatch('DELETE',
                             "/rest/data/issue/1.json",
-                            form)
+                            self.empty_form)
         self.assertEqual(self.server.client.response_code, 200)
         print(results)
         json_dict = json.loads(b2s(results))
@@ -3213,14 +3289,15 @@ def testMethodOverride(self):
                 "CONTENT_LENGTH": len(body),
                 "REQUEST_METHOD": "POST"
         }
-        body_file=BytesIO(body)  # FieldStorage needs a file
         self.server.client.request.headers.get=self.get_header
         for method in ( "GET", "PUT", "PATCH" ):
             headers={"accept": "application/json; version=1",
                      "content-type": env['CONTENT_TYPE'],
                      "content-length": len(body),
                      "x-http-method-override": "DElETE",
                  }
+            body_file=BytesIO(body)  # FieldStorage needs a file
+
             self.headers=headers
             form = client.BinaryFieldStorage(body_file,
                                          headers=headers,