From: Mark McLoughlin Date: Wed, 12 Sep 2012 11:51:40 +0000 (+0100) Subject: Improve entity validation in volumes APIs X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=dcecb586a0578688656d5420c40c26e5a8caa942;p=openstack-build%2Fcinder-build.git 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. Change-Id: I04127972981522c1ed81903893396c4f9665bcd3 --- diff --git a/cinder/api/openstack/volume/contrib/types_extra_specs.py b/cinder/api/openstack/volume/contrib/types_extra_specs.py index e0c4d595c..50ddb234a 100644 --- a/cinder/api/openstack/volume/contrib/types_extra_specs.py +++ b/cinder/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['cinder.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['cinder.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/cinder/api/openstack/volume/contrib/types_manage.py b/cinder/api/openstack/volume/contrib/types_manage.py index a9e9fb280..3ff2efebc 100644 --- a/cinder/api/openstack/volume/contrib/types_manage.py +++ b/cinder/api/openstack/volume/contrib/types_manage.py @@ -42,13 +42,10 @@ class VolumeTypesManageController(wsgi.Controller): context = req.environ['cinder.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/cinder/api/openstack/volume/snapshots.py b/cinder/api/openstack/volume/snapshots.py index 919c01da2..192dee223 100644 --- a/cinder/api/openstack/volume/snapshots.py +++ b/cinder/api/openstack/volume/snapshots.py @@ -84,7 +84,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, ext_mgr=None): @@ -148,8 +148,8 @@ class SnapshotsController(object): """Creates a new snapshot.""" context = req.environ['cinder.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/cinder/api/openstack/volume/volumes.py b/cinder/api/openstack/volume/volumes.py index b0a4c6dfa..2c6852b5a 100644 --- a/cinder/api/openstack/volume/volumes.py +++ b/cinder/api/openstack/volume/volumes.py @@ -199,7 +199,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, ext_mgr): @@ -276,11 +276,10 @@ class VolumeController(object): @wsgi.deserializers(xml=CreateDeserializer) def create(self, req, body): """Creates a new volume.""" - context = req.environ['cinder.context'] - - if not body: + if not self.is_valid_body(body, 'volume'): raise exc.HTTPUnprocessableEntity() + context = req.environ['cinder.context'] volume = body['volume'] kwargs = {} diff --git a/cinder/tests/api/openstack/volume/contrib/test_types_extra_specs.py b/cinder/tests/api/openstack/volume/contrib/test_types_extra_specs.py index 01dd0bf82..0a613ccc0 100644 --- a/cinder/tests/api/openstack/volume/contrib/test_types_extra_specs.py +++ b/cinder/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(cinder.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(cinder.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(cinder.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(cinder.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/cinder/tests/api/openstack/volume/contrib/test_types_manage.py b/cinder/tests/api/openstack/volume/contrib/test_types_manage.py index f85bf98fd..c2ee34989 100644 --- a/cinder/tests/api/openstack/volume/contrib/test_types_manage.py +++ b/cinder/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/cinder/tests/api/openstack/volume/test_snapshots.py b/cinder/tests/api/openstack/volume/test_snapshots.py index 07d57057b..9a0e8f24f 100644 --- a/cinder/tests/api/openstack/volume/test_snapshots.py +++ b/cinder/tests/api/openstack/volume/test_snapshots.py @@ -334,3 +334,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/cinder/tests/api/openstack/volume/test_volumes.py b/cinder/tests/api/openstack/volume/test_volumes.py index 4fcf44979..ade6f8db0 100644 --- a/cinder/tests/api/openstack/volume/test_volumes.py +++ b/cinder/tests/api/openstack/volume/test_volumes.py @@ -103,14 +103,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_create_with_image_id(self): self.stubs.Set(volume_api.API, "create", fakes.stub_volume_create) self.ext_mgr.extensions = {'os-image-create': 'fake'} @@ -625,3 +617,34 @@ class TestVolumeCreateRequestXMLDeserializer(test.TestCase): }, } 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.ext_mgr = extensions.ExtensionManager() + self.ext_mgr.extensions = {} + self.controller = volumes.VolumeController(self.ext_mgr) + + 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)