Improve entity validation in volumes APIs

Fixes bug #1048565

Use the new Controller.is_valid_body() helper to validate the entity
body in various volumes related POST/PUT handlers and return 422
as appropriate.

(Cherry picks commit dcecb586 from Cinder and adds similar fixes for
the volumes bits in the compute API)

Change-Id: I04127972981522c1ed81903893396c4f9665bcd3

12 files changed, 242 insertions, 89 deletions

diff --git a/nova/api/openstack/compute/contrib/volumes.py b/nova/api/openstack/compute/contrib/volumes.py
index 7194f64e3..9940e3050 100644
--- a/nova/api/openstack/compute/contrib/volumes.py
+++ b/nova/api/openstack/compute/contrib/volumes.py
@@ -160,7 +160,7 @@ class CreateDeserializer(CommonDeserializer):
         return {'body': {'volume': volume}}
 
 
-class VolumeController(object):
+class VolumeController(wsgi.Controller):
     """The Volumes API controller for the OpenStack API."""
 
     def __init__(self):
@@ -221,7 +221,7 @@ class VolumeController(object):
         context = req.environ['nova.context']
         authorize(context)
 
-        if not body:
+        if not self.is_valid_body(body, 'volume'):
             raise exc.HTTPUnprocessableEntity()
 
         vol = body['volume']
@@ -323,7 +323,7 @@ class VolumeAttachmentsTemplate(xmlutil.TemplateBuilder):
         return xmlutil.MasterTemplate(root, 1)
 
 
-class VolumeAttachmentController(object):
+class VolumeAttachmentController(wsgi.Controller):
     """The volume attachment API controller for the OpenStack API.
 
     A child resource of the server.  Note that we use the volume id
@@ -381,7 +381,7 @@ class VolumeAttachmentController(object):
         context = req.environ['nova.context']
         authorize(context)
 
-        if not body:
+        if not self.is_valid_body(body, 'volumeAttachment'):
             raise exc.HTTPUnprocessableEntity()
 
         volume_id = body['volumeAttachment']['volumeId']
@@ -525,7 +525,7 @@ class SnapshotsTemplate(xmlutil.TemplateBuilder):
         return xmlutil.MasterTemplate(root, 1)
 
 
-class SnapshotController(object):
+class SnapshotController(wsgi.Controller):
     """The Volumes API controller for the OpenStack API."""
 
     def __init__(self):
@@ -585,8 +585,8 @@ class SnapshotController(object):
         context = req.environ['nova.context']
         authorize(context)
 
-        if not body:
-            return exc.HTTPUnprocessableEntity()
+        if not self.is_valid_body(body, 'snapshot'):
+            raise exc.HTTPUnprocessableEntity()
 
         snapshot = body['snapshot']
         volume_id = snapshot['volume_id']
diff --git a/nova/api/openstack/compute/contrib/volumetypes.py b/nova/api/openstack/compute/contrib/volumetypes.py
index 2711c45d7..036e3ff42 100644
--- a/nova/api/openstack/compute/contrib/volumetypes.py
+++ b/nova/api/openstack/compute/contrib/volumetypes.py
@@ -53,7 +53,7 @@ class VolumeTypesTemplate(xmlutil.TemplateBuilder):
         return xmlutil.MasterTemplate(root, 1)
 
 
-class VolumeTypesController(object):
+class VolumeTypesController(wsgi.Controller):
     """ The volume types API controller for the OpenStack API """
 
     @wsgi.serializers(xml=VolumeTypesTemplate)
@@ -69,13 +69,10 @@ class VolumeTypesController(object):
         context = req.environ['nova.context']
         authorize(context)
 
-        if not body or body == "":
-            raise exc.HTTPUnprocessableEntity()
-
-        vol_type = body.get('volume_type', None)
-        if vol_type is None or vol_type == "":
+        if not self.is_valid_body(body, 'volume_type'):
             raise exc.HTTPUnprocessableEntity()
 
+        vol_type = body['volume_type']
         name = vol_type.get('name', None)
         specs = vol_type.get('extra_specs', {})
 
diff --git a/nova/api/openstack/volume/contrib/types_extra_specs.py b/nova/api/openstack/volume/contrib/types_extra_specs.py
index d70e24538..2e993ad8a 100644
--- a/nova/api/openstack/volume/contrib/types_extra_specs.py
+++ b/nova/api/openstack/volume/contrib/types_extra_specs.py
@@ -50,7 +50,7 @@ class VolumeTypeExtraSpecTemplate(xmlutil.TemplateBuilder):
         return xmlutil.MasterTemplate(root, 1)
 
 
-class VolumeTypeExtraSpecsController(object):
+class VolumeTypeExtraSpecsController(wsgi.Controller):
     """ The volume type extra specs API controller for the OpenStack API """
 
     def _get_extra_specs(self, context, type_id):
@@ -60,11 +60,6 @@ class VolumeTypeExtraSpecsController(object):
             specs_dict[key] = value
         return dict(extra_specs=specs_dict)
 
-    def _check_body(self, body):
-        if not body:
-            expl = _('No Request Body')
-            raise webob.exc.HTTPBadRequest(explanation=expl)
-
     def _check_type(self, context, type_id):
         try:
             volume_types.get_volume_type(context, type_id)
@@ -83,12 +78,13 @@ class VolumeTypeExtraSpecsController(object):
     def create(self, req, type_id, body=None):
         context = req.environ['nova.context']
         authorize(context)
+
+        if not self.is_valid_body(body, 'extra_specs'):
+            raise webob.exc.HTTPUnprocessableEntity()
+
         self._check_type(context, type_id)
-        self._check_body(body)
-        specs = body.get('extra_specs')
-        if not isinstance(specs, dict):
-            expl = _('Malformed extra specs')
-            raise webob.exc.HTTPBadRequest(explanation=expl)
+
+        specs = body['extra_specs']
         db.volume_type_extra_specs_update_or_create(context,
                                                     type_id,
                                                     specs)
@@ -98,8 +94,9 @@ class VolumeTypeExtraSpecsController(object):
     def update(self, req, type_id, id, body=None):
         context = req.environ['nova.context']
         authorize(context)
+        if not body:
+            raise webob.exc.HTTPUnprocessableEntity()
         self._check_type(context, type_id)
-        self._check_body(body)
         if not id in body:
             expl = _('Request body and URI mismatch')
             raise webob.exc.HTTPBadRequest(explanation=expl)
diff --git a/nova/api/openstack/volume/contrib/types_manage.py b/nova/api/openstack/volume/contrib/types_manage.py
index ff21174c3..e68093ce8 100644
--- a/nova/api/openstack/volume/contrib/types_manage.py
+++ b/nova/api/openstack/volume/contrib/types_manage.py
@@ -42,13 +42,10 @@ class VolumeTypesManageController(wsgi.Controller):
         context = req.environ['nova.context']
         authorize(context)
 
-        if not body or body == "":
-            raise webob.exc.HTTPUnprocessableEntity()
-
-        vol_type = body.get('volume_type', None)
-        if vol_type is None or vol_type == "":
+        if not self.is_valid_body(body, 'volume_type'):
             raise webob.exc.HTTPUnprocessableEntity()
 
+        vol_type = body['volume_type']
         name = vol_type.get('name', None)
         specs = vol_type.get('extra_specs', {})
 
diff --git a/nova/api/openstack/volume/snapshots.py b/nova/api/openstack/volume/snapshots.py
index 755398369..fa070e2e3 100644
--- a/nova/api/openstack/volume/snapshots.py
+++ b/nova/api/openstack/volume/snapshots.py
@@ -85,7 +85,7 @@ class SnapshotsTemplate(xmlutil.TemplateBuilder):
         return xmlutil.MasterTemplate(root, 1)
 
 
-class SnapshotsController(object):
+class SnapshotsController(wsgi.Controller):
     """The Volumes API controller for the OpenStack API."""
 
     def __init__(self):
@@ -145,8 +145,8 @@ class SnapshotsController(object):
         """Creates a new snapshot."""
         context = req.environ['nova.context']
 
-        if not body:
-            return exc.HTTPUnprocessableEntity()
+        if not self.is_valid_body(body, 'snapshot'):
+            raise exc.HTTPUnprocessableEntity()
 
         snapshot = body['snapshot']
         volume_id = snapshot['volume_id']
diff --git a/nova/api/openstack/volume/volumes.py b/nova/api/openstack/volume/volumes.py
index e83030945..f69ef0a21 100644
--- a/nova/api/openstack/volume/volumes.py
+++ b/nova/api/openstack/volume/volumes.py
@@ -192,7 +192,7 @@ class CreateDeserializer(CommonDeserializer):
         return {'body': {'volume': volume}}
 
 
-class VolumeController(object):
+class VolumeController(wsgi.Controller):
     """The Volumes API controller for the OpenStack API."""
 
     def __init__(self):
@@ -253,11 +253,10 @@ class VolumeController(object):
     @wsgi.deserializers(xml=CreateDeserializer)
     def create(self, req, body):
         """Creates a new volume."""
-        context = req.environ['nova.context']
-
-        if not body:
+        if not self.is_valid_body(body, 'volume'):
             raise exc.HTTPUnprocessableEntity()
 
+        context = req.environ['nova.context']
         volume = body['volume']
 
         kwargs = {}
diff --git a/nova/tests/api/openstack/compute/contrib/test_volume_types.py b/nova/tests/api/openstack/compute/contrib/test_volume_types.py
index 4ad6297b8..af88cf601 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volume_types.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volume_types.py
@@ -151,16 +151,6 @@ class VolumeTypesApiTest(test.TestCase):
         self.assertEqual(1, len(res_dict))
         self.assertEqual('vol_type_1', res_dict['volume_type']['name'])
 
-    def test_create_empty_body(self):
-        self.stubs.Set(volume_types, 'create',
-                       return_volume_types_create)
-        self.stubs.Set(volume_types, 'get_volume_type_by_name',
-                       return_volume_types_get_by_name)
-
-        req = fakes.HTTPRequest.blank('/v2/fake/os-volume-types')
-        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
-                          self.controller.create, req, '')
-
 
 class VolumeTypesSerializerTest(test.TestCase):
     def _verify_volume_type(self, vtype, tree):
@@ -204,3 +194,31 @@ class VolumeTypesSerializerTest(test.TestCase):
         tree = etree.fromstring(text)
 
         self._verify_volume_type(vtype, tree)
+
+
+class VolumeTypesUnprocessableEntityTestCase(test.TestCase):
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(VolumeTypesUnprocessableEntityTestCase, self).setUp()
+        self.controller = volumetypes.VolumeTypesController()
+
+    def _unprocessable_volume_type_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/os-volume-types')
+        req.method = 'POST'
+
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.create, req, body)
+
+    def test_create_volume_type_no_body(self):
+        self._unprocessable_volume_type_create(body=None)
+
+    def test_create_volume_type_missing_volume_type(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_volume_type_create(body=body)
+
+    def test_create_volume_type_malformed_entity(self):
+        body = {'volume_type': 'string'}
+        self._unprocessable_volume_type_create(body=body)
diff --git a/nova/tests/api/openstack/compute/contrib/test_volumes.py b/nova/tests/api/openstack/compute/contrib/test_volumes.py
index 108ec8964..7ed04f1ad 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volumes.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volumes.py
@@ -175,15 +175,6 @@ class VolumeApiTest(test.TestCase):
         self.assertEqual(resp_dict['volume']['availabilityZone'],
                          vol['availability_zone'])
 
-    def test_volume_create_no_body(self):
-        req = webob.Request.blank('/v2/fake/os-volumes')
-        req.method = 'POST'
-        req.body = jsonutils.dumps({})
-        req.headers['content-type'] = 'application/json'
-
-        resp = req.get_response(fakes.wsgi_app())
-        self.assertEqual(resp.status_int, 422)
-
     def test_volume_index(self):
         req = webob.Request.blank('/v2/fake/os-volumes')
         resp = req.get_response(fakes.wsgi_app())
@@ -566,3 +557,64 @@ class TestVolumeCreateRequestXMLDeserializer(test.TestCase):
         }
         self.maxDiff = None
         self.assertEquals(request['body'], expected)
+
+
+class CommonUnprocessableEntityTestCase(object):
+
+    resource = None
+    entity_name = None
+    controller_cls = None
+    kwargs = {}
+
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(CommonUnprocessableEntityTestCase, self).setUp()
+        self.controller = self.controller_cls()
+
+    def _unprocessable_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/' + self.resource)
+        req.method = 'POST'
+
+        kwargs = self.kwargs.copy()
+        kwargs['body'] = body
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.create, req, **kwargs)
+
+    def test_create_no_body(self):
+        self._unprocessable_create(body=None)
+
+    def test_create_missing_volume(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_create(body=body)
+
+    def test_create_malformed_entity(self):
+        body = {self.entity_name: 'string'}
+        self._unprocessable_create(body=body)
+
+
+class UnprocessableVolumeTestCase(CommonUnprocessableEntityTestCase,
+                                  test.TestCase):
+
+    resource = 'os-volumes'
+    entity_name = 'volume'
+    controller_cls = volumes.VolumeController
+
+
+class UnprocessableAttachmentTestCase(CommonUnprocessableEntityTestCase,
+                                      test.TestCase):
+
+    resource = 'servers/' + FAKE_UUID + '/os-volume_attachments'
+    entity_name = 'volumeAttachment'
+    controller_cls = volumes.VolumeAttachmentController
+    kwargs = {'server_id': FAKE_UUID}
+
+
+class UnprocessableSnapshotTestCase(CommonUnprocessableEntityTestCase,
+                                    test.TestCase):
+
+    resource = 'os-snapshots'
+    entity_name = 'snapshot'
+    controller_cls = volumes.SnapshotController
diff --git a/nova/tests/api/openstack/volume/contrib/test_types_extra_specs.py b/nova/tests/api/openstack/volume/contrib/test_types_extra_specs.py
index c25ec8c01..edc127b8a 100644
--- a/nova/tests/api/openstack/volume/contrib/test_types_extra_specs.py
+++ b/nova/tests/api/openstack/volume/contrib/test_types_extra_specs.py
@@ -118,15 +118,6 @@ class VolumeTypesExtraSpecsTest(test.TestCase):
 
         self.assertEqual('value1', res_dict['extra_specs']['key1'])
 
-    def test_create_empty_body(self):
-        self.stubs.Set(nova.db,
-                       'volume_type_extra_specs_update_or_create',
-                       return_create_volume_type_extra_specs)
-
-        req = fakes.HTTPRequest.blank(self.api_path)
-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create,
-                          req, 1, '')
-
     def test_update_item(self):
         self.stubs.Set(nova.db,
                        'volume_type_extra_specs_update_or_create',
@@ -138,15 +129,6 @@ class VolumeTypesExtraSpecsTest(test.TestCase):
 
         self.assertEqual('value1', res_dict['key1'])
 
-    def test_update_item_empty_body(self):
-        self.stubs.Set(nova.db,
-                       'volume_type_extra_specs_update_or_create',
-                       return_create_volume_type_extra_specs)
-
-        req = fakes.HTTPRequest.blank(self.api_path + '/key1')
-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
-                          req, 1, 'key1', '')
-
     def test_update_item_too_many_keys(self):
         self.stubs.Set(nova.db,
                        'volume_type_extra_specs_update_or_create',
@@ -200,3 +182,45 @@ class VolumeTypeExtraSpecsSerializerTest(test.TestCase):
         self.assertEqual('key1', tree.tag)
         self.assertEqual('value1', tree.text)
         self.assertEqual(0, len(tree))
+
+
+class VolumeTypeExtraSpecsUnprocessableEntityTestCase(test.TestCase):
+
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(VolumeTypeExtraSpecsUnprocessableEntityTestCase, self).setUp()
+        self.controller = types_extra_specs.VolumeTypeExtraSpecsController()
+
+    def _unprocessable_extra_specs_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/types/1/extra_specs')
+        req.method = 'POST'
+
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.create, req, '1', body)
+
+    def test_create_no_body(self):
+        self._unprocessable_extra_specs_create(body=None)
+
+    def test_create_missing_volume(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_extra_specs_create(body=body)
+
+    def test_create_malformed_entity(self):
+        body = {'extra_specs': 'string'}
+        self._unprocessable_extra_specs_create(body=body)
+
+    def _unprocessable_extra_specs_update(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/types/1/extra_specs')
+        req.method = 'POST'
+
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.update, req, '1', body)
+
+    def test_update_no_body(self):
+        self._unprocessable_extra_specs_update(body=None)
+
+    def test_update_empty_body(self):
+        self._unprocessable_extra_specs_update(body={})
diff --git a/nova/tests/api/openstack/volume/contrib/test_types_manage.py b/nova/tests/api/openstack/volume/contrib/test_types_manage.py
index 70c89d896..f69038144 100644
--- a/nova/tests/api/openstack/volume/contrib/test_types_manage.py
+++ b/nova/tests/api/openstack/volume/contrib/test_types_manage.py
@@ -92,12 +92,31 @@ class VolumeTypesManageApiTest(test.TestCase):
         self.assertEqual(1, len(res_dict))
         self.assertEqual('vol_type_1', res_dict['volume_type']['name'])
 
-    def test_create_empty_body(self):
-        self.stubs.Set(volume_types, 'create',
-                       return_volume_types_create)
-        self.stubs.Set(volume_types, 'get_volume_type_by_name',
-                       return_volume_types_get_by_name)
 
-        req = fakes.HTTPRequest.blank('/v1/fake/types')
+class VolumeTypesUnprocessableEntityTestCase(test.TestCase):
+
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(VolumeTypesUnprocessableEntityTestCase, self).setUp()
+        self.controller = types_manage.VolumeTypesManageController()
+
+    def _unprocessable_volume_type_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/types')
+        req.method = 'POST'
+
         self.assertRaises(webob.exc.HTTPUnprocessableEntity,
-                          self.controller._create, req, '')
+                          self.controller._create, req, body)
+
+    def test_create_no_body(self):
+        self._unprocessable_volume_type_create(body=None)
+
+    def test_create_missing_volume(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_volume_type_create(body=body)
+
+    def test_create_malformed_entity(self):
+        body = {'volume_type': 'string'}
+        self._unprocessable_volume_type_create(body=body)
diff --git a/nova/tests/api/openstack/volume/test_snapshots.py b/nova/tests/api/openstack/volume/test_snapshots.py
index c65182cb7..66f8cddc3 100644
--- a/nova/tests/api/openstack/volume/test_snapshots.py
+++ b/nova/tests/api/openstack/volume/test_snapshots.py
@@ -254,3 +254,32 @@ class SnapshotSerializerTest(test.TestCase):
         self.assertEqual(len(raw_snapshots), len(tree))
         for idx, child in enumerate(tree):
             self._verify_snapshot(raw_snapshots[idx], child)
+
+
+class SnapshotsUnprocessableEntityTestCase(test.TestCase):
+
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(SnapshotsUnprocessableEntityTestCase, self).setUp()
+        self.controller = snapshots.SnapshotsController()
+
+    def _unprocessable_snapshot_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/snapshots')
+        req.method = 'POST'
+
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.create, req, body)
+
+    def test_create_no_body(self):
+        self._unprocessable_snapshot_create(body=None)
+
+    def test_create_missing_snapshot(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_snapshot_create(body=body)
+
+    def test_create_malformed_entity(self):
+        body = {'snapshot': 'string'}
+        self._unprocessable_snapshot_create(body=body)
diff --git a/nova/tests/api/openstack/volume/test_volumes.py b/nova/tests/api/openstack/volume/test_volumes.py
index 46a772fbd..0b9872a5a 100644
--- a/nova/tests/api/openstack/volume/test_volumes.py
+++ b/nova/tests/api/openstack/volume/test_volumes.py
@@ -83,14 +83,6 @@ class VolumeApiTest(test.TestCase):
                           req,
                           body)
 
-    def test_volume_create_no_body(self):
-        body = {}
-        req = fakes.HTTPRequest.blank('/v1/volumes')
-        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
-                          self.controller.create,
-                          req,
-                          body)
-
     def test_volume_list(self):
         self.stubs.Set(volume_api.API, 'get_all',
                      fakes.stub_volume_get_all_by_project)
@@ -470,3 +462,32 @@ class TestVolumeCreateRequestXMLDeserializer(test.TestCase):
         }
         self.maxDiff = None
         self.assertEquals(request['body'], expected)
+
+
+class VolumesUnprocessableEntityTestCase(test.TestCase):
+
+    """
+    Tests of places we throw 422 Unprocessable Entity from
+    """
+
+    def setUp(self):
+        super(VolumesUnprocessableEntityTestCase, self).setUp()
+        self.controller = volumes.VolumeController()
+
+    def _unprocessable_volume_create(self, body):
+        req = fakes.HTTPRequest.blank('/v2/fake/volumes')
+        req.method = 'POST'
+
+        self.assertRaises(webob.exc.HTTPUnprocessableEntity,
+                          self.controller.create, req, body)
+
+    def test_create_no_body(self):
+        self._unprocessable_volume_create(body=None)
+
+    def test_create_missing_volume(self):
+        body = {'foo': {'a': 'b'}}
+        self._unprocessable_volume_create(body=body)
+
+    def test_create_malformed_entity(self):
+        body = {'volume': 'string'}
+        self._unprocessable_volume_create(body=body)