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: rouilj

roundup/rest.py

@@ -563,13 +563,13 @@ def transitive_props (self, class_name, props):
                 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 @@ def transitive_props (self, class_name, props):
                         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 @@ def get_collection(self, class_name, input):
                 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 @@ def get_collection(self, class_name, input):
                 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 @@ def get_element(self, class_name, item_id, input):
                 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

test/rest_common.py

@@ -391,6 +391,68 @@ def testGetTransitive(self):
         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
         """