Mercurial > p > roundup > code
changeset 6554:576d630fc908
Fix error status for invalid props
Some places were raising AttributeError which results in a 405 (bad
method) not a 400. Replace with UsageError or KeyError.
Make rest.py::transitive_props run aginst a prop that has no
transitive elements as well. So it will verify that assignedto exists
even though it has no period like assignedto.name would.
These should check properties in @fields and @sort.
Also validate fields that are used as search params:
?assignedto=1
If the search prop was mispelled or incorrect, the search element was
ignored as though it had not been specified. Now it returns a 400 to
notify sender they sent an incorrect filter.
Also remove unused statements that were originally for finding invalid
props before we supported transitive props.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sat, 11 Dec 2021 21:41:49 -0500 |
| parents | 75da037d1c54 |
| children | 34cbd0e633d2 |
| files | roundup/rest.py test/rest_common.py |
| diffstat | 2 files changed, 73 insertions(+), 5 deletions(-) [+] |
line wrap: on
line diff
--- a/roundup/rest.py Sat Dec 11 21:26:46 2021 -0500 +++ b/roundup/rest.py Sat Dec 11 21:41:49 2021 -0500 @@ -563,13 +563,13 @@ for pn in p.split('.'): # Tried to dereference a non-Link property if cn is None: - raise AttributeError("Unknown: %s" % p) + raise UsageError("Property %(base)s can not be dereferenced in %(p)s." % { "base": p[:-(len(pn)+1)], "p": p}) cls = self.db.getclass(cn) # This raises a KeyError for unknown prop: try: prop = cls.getprops(protected=True)[pn] except KeyError: - raise AttributeError("Unknown: %s" % p) + raise KeyError("Unknown property: %s" % p) if isinstance(prop, hyperdb.Multilink): raise UsageError( 'Multilink Traversal not allowed: %s' % p) @@ -582,6 +582,13 @@ cn = prop.classname except AttributeError: cn = None + else: + cls = self.db.getclass(cn) + # This raises a KeyError for unknown prop: + try: + prop = cls.getprops(protected=True)[pn] + except KeyError: + raise KeyError("Unknown property: %s" % pn) checked_props.append (p) return checked_props @@ -802,11 +809,9 @@ f = value.split(",") if len(f) == 1: f = value.split(":") - allprops = class_obj.getprops(protected=True) display_props.update(self.transitive_props(class_name, f)) elif key == "@sort": f = value.split(",") - allprops = class_obj.getprops(protected=True) for p in f: if not p: raise UsageError("Empty property " @@ -845,6 +850,8 @@ except KeyError: raise UsageError("Field %s is not valid for %s class." % (p, class_name)) + # Call this for the side effect of validating the key + _ = self.transitive_props(class_name, [ key ]) # We drop properties without search permission silently # This reflects the current behavior of other roundup # interfaces @@ -1024,7 +1031,6 @@ f = value.split(",") if len(f) == 1: f = value.split(":") - allprops = class_obj.getprops(protected=True) props.update(self.transitive_props(class_name, f)) elif key == "@protected": # allow client to request read only
--- a/test/rest_common.py Sat Dec 11 21:26:46 2021 -0500 +++ b/test/rest_common.py Sat Dec 11 21:41:49 2021 -0500 @@ -391,6 +391,68 @@ results = self.server.get_collection('issue', form) self.assertDictEqual(expected, results) + def testGetBadTransitive(self): + """ + Mess up the names of various properties and make sure we get a 400 + and a somewhat useful error message. + """ + base_path = self.db.config['TRACKER_WEB'] + 'rest/data/' + #self.maxDiff=None + self.create_sampledata() + self.db.issue.set('2', status=self.db.status.lookup('closed')) + self.db.issue.set('3', status=self.db.status.lookup('chatting')) + expected = [ + {'error': {'msg': KeyError('Unknown property: assignedto.isse',), + 'status': 400}}, + {'error': {'msg': KeyError('Unknown property: stat',), + 'status': 400}}, + {'error': {'msg': KeyError('Unknown property: status.nam',), + 'status': 400}}, + ] + + ## test invalid transitive property in @fields + form = cgi.FieldStorage() + form.list = [ + cgi.MiniFieldStorage('status.name', 'o'), + cgi.MiniFieldStorage('@fields', 'status,assignedto.isse'), + cgi.MiniFieldStorage('@sort', 'status.name'), + ] + results = self.server.get_collection('issue', form) + self.assertEqual(self.dummy_client.response_code, 400) + self.assertEqual(repr(expected[0]['error']['msg']), + repr(results['error']['msg'])) + self.assertEqual(expected[0]['error']['status'], + results['error']['status']) + + ## test invalid property in @fields + form = cgi.FieldStorage() + form.list = [ + cgi.MiniFieldStorage('status.name', 'o'), + cgi.MiniFieldStorage('@fields', 'stat,assignedto.isuse'), + cgi.MiniFieldStorage('@sort', 'status.name'), + ] + results = self.server.get_collection('issue', form) + self.assertEqual(self.dummy_client.response_code, 400) + self.assertEqual(repr(expected[1]['error']['msg']), + repr(results['error']['msg'])) + self.assertEqual(expected[1]['error']['status'], + results['error']['status']) + + ## test invalid transitive property in filter TODO + form = cgi.FieldStorage() + form.list = [ + cgi.MiniFieldStorage('status.nam', 'o'), + cgi.MiniFieldStorage('@fields', 'status,assignedto.isuse'), + cgi.MiniFieldStorage('@sort', 'status.name'), + ] + results = self.server.get_collection('issue', form) + # is currently 403 not 400 + self.assertEqual(self.dummy_client.response_code, 400) + self.assertEqual(repr(expected[2]['error']['msg']), + repr(results['error']['msg'])) + self.assertEqual(expected[2]['error']['status'], + results['error']['status']) + def testGetExactMatch(self): """ Retrieve all issues with an exact title """