Improve validation of openstack config requests

* In the JSON schema 'number' type restriction replaced with 'integer'.
* Extend parsing boolean values in URL query

Related-Bug: #1557462
Change-Id: Iaaafb61aaa030b001358cee00e9c6b66c306cd46

nailgun/nailgun/api/v1/validators/json_schema/openstack_config.py

@@ -19,8 +19,8 @@ _base_config = {
 }
 
 _base_properties = {
-    'cluster_id': {'type': 'number', },
+    'cluster_id': {'type': 'integer'},
-    'node_id': {'type': 'number'},
+    'node_id': {'type': 'integer'},
     'node_role': {'type': 'string'},
 }
 
@@ -28,7 +28,7 @@ OPENSTACK_CONFIG = {
     'title': 'OpenstackConfig',
     'description': 'Openstack Configuration',
     'properties': {
-        'id': {'type': 'number'},
+        'id': {'type': 'integer'},
         'configuration': {'type': 'object'},
     },
     'required': ['cluster_id', 'configuration'],
@@ -38,7 +38,7 @@ OPENSTACK_CONFIG_EXECUTE = {
     'title': 'OpenstackConfig execute',
     'description': 'Openstack Configuration filters for execute',
     'properties': {
-        'id': {'type': 'number'},
+        'id': {'type': 'integer'},
         'force': {'type': 'boolean'}
     },
     'required': ['cluster_id'],
@@ -48,12 +48,7 @@ OPENSTACK_CONFIG_QUERY = {
     'title': 'OpenstackConfig query',
     'description': 'URL query for Openstack Configuration filter',
     'properties': {
-        'is_active': {
+        'is_active': {'type': 'boolean'}
-            'type': 'number',
-            'description': "is_active is a number since GET request"
-                           " query parameters don't carry type information"
-                           " and are parsed as strings"},
-        'cluster_id': {'type': 'number'},
     },
     'required': ['cluster_id'],
 }

nailgun/nailgun/api/v1/validators/openstack_config.py

@@ -20,11 +20,11 @@ from nailgun import consts
 from nailgun.db.sqlalchemy import models
 from nailgun.errors import errors
 from nailgun import objects
+from nailgun import utils
 
 
 class OpenstackConfigValidator(BasicValidator):
 
-    int_fields = frozenset(['cluster_id', 'node_id', 'is_active'])
     exclusive_fields = frozenset(['node_id', 'node_role'])
 
     supported_configs = frozenset([
@@ -136,7 +136,7 @@ class OpenstackConfigValidator(BasicValidator):
         cls._check_exclusive_fields(data)
         cls.validate_schema(data, schema.OPENSTACK_CONFIG_QUERY)
 
-        data['is_active'] = bool(data.get('is_active', True))
+        data.setdefault('is_active', True)
         return data
 
     @classmethod
@@ -156,9 +156,21 @@ class OpenstackConfigValidator(BasicValidator):
         Schema validation doesn't perform any type conversion, so
         it is required to convert them before schema validation.
         """
-        for field in cls.int_fields:
+        for field in ['cluster_id', 'node_id']:
-            if field in data and data[field] is not None:
+            value = data.get(field, None)
-                data[field] = int(data[field])
+            if value is not None:
+                try:
+                    data[field] = int(value)
+                except ValueError:
+                    raise errors.InvalidData("Invalid '{0}' value: '{1}'"
+                                             .format(field, value))
+
+        if 'is_active' in data:
+            try:
+                data['is_active'] = utils.parse_bool(data['is_active'])
+            except ValueError:
+                raise errors.InvalidData("Invalid 'is_active' value: '{0}'"
+                                         .format(data['is_active']))
 
     @classmethod
     def _check_exclusive_fields(cls, data):

nailgun/nailgun/test/unit/test_openstack_config_handler.py

@@ -202,6 +202,16 @@ class TestOpenstackConfigHandlers(BaseIntegrationTest):
             resp.json_body['message'],
             r"Parameter '\w+' conflicts with '\w+(, \w+)*'")
 
+    def test_openstack_config_list_invalid_params(self):
+        for param in ['cluster_id', 'node_id', 'is_active']:
+            url = self._make_filter_url(**{param: 'invalidvalue'})
+            resp = self.app.get(url, headers=self.default_headers,
+                                expect_errors=True)
+            self.assertEqual(resp.status_code, 400)
+            self.assertEqual(
+                resp.json_body['message'],
+                "Invalid '{0}' value: 'invalidvalue'".format(param))
+
     def test_openstack_config_get(self):
         resp = self.app.get(
             reverse('OpenstackConfigHandler',

nailgun/nailgun/test/unit/test_utils.py

@@ -26,6 +26,7 @@ from nailgun.utils import dict_update
 from nailgun.utils import flatten
 from nailgun.utils import get_lines
 from nailgun.utils import grouper
+from nailgun.utils import parse_bool
 from nailgun.utils import text_format_safe
 from nailgun.utils import traverse
 
@@ -138,6 +139,18 @@ class TestUtils(base.BaseIntegrationTest):
 
         self.assertEqual(get_lines(mixed), ['abc', 'foo', 'bar'])
 
+    def test_parse_bool(self):
+        true_values = ['1', 't', 'T', 'TRUE', 'True', 'true']
+        false_values = ['0', 'f', 'F', 'FALSE', 'False', 'false']
+
+        for value in true_values:
+            self.assertTrue(parse_bool(value))
+        for value in false_values:
+            self.assertFalse(parse_bool(value))
+
+        self.assertRaises(ValueError, parse_bool, 'tru')
+        self.assertRaises(ValueError, parse_bool, 'fals')
+
 
 class TestTraverse(base.BaseUnitTest):
 

nailgun/nailgun/utils/__init__.py

@@ -302,3 +302,25 @@ def dict_update(target, patch, level=None):
                 target.setdefault(k, {}).update(v)
         else:
             target[k] = v
+
+
+def parse_bool(value):
+    """
+    Returns boolean value from string representation.
+
+    Function returns ``True`` for ``"1", "t", "true"`` values
+    and ``False`` for ``"0", "f", "false"``. String values are
+    case insensetive.
+    Otherwise raises ValueError.
+
+    :param str value: string representation of boolean value
+    :rtype: bool
+    :raises ValueError: if the value cannot be converted to bool
+    """
+    value = value.lower()
+    if value in ('1', 't', 'true'):
+        return True
+    elif value in ('0', 'f', 'false'):
+        return False
+    raise ValueError('Invalid value: {0}'.format(value))
+