From 76ca8c184bed7aa706ac6ef1010c3f4ebf08f7f0 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Wed, 12 Sep 2012 12:51:01 +0100 Subject: [PATCH] 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 --- nova/api/openstack/compute/contrib/volumes.py | 14 ++-- .../openstack/compute/contrib/volumetypes.py | 9 +-- .../volume/contrib/types_extra_specs.py | 21 +++--- .../openstack/volume/contrib/types_manage.py | 7 +- nova/api/openstack/volume/snapshots.py | 6 +- nova/api/openstack/volume/volumes.py | 7 +- .../compute/contrib/test_volume_types.py | 38 +++++++--- .../openstack/compute/contrib/test_volumes.py | 70 ++++++++++++++++--- .../volume/contrib/test_types_extra_specs.py | 60 +++++++++++----- .../volume/contrib/test_types_manage.py | 33 +++++++-- .../api/openstack/volume/test_snapshots.py | 29 ++++++++ .../api/openstack/volume/test_volumes.py | 37 +++++++--- 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 7194f64e354..9940e3050e1 100644 --- a/nova/api/openstack/compute/contrib/volumes.py +++ b/nova/api/openstack/compute/contrib/volumes.py @@ -160,7 +160,7 @@ def default(self, string): 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 @@ def create(self, req, body): 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 @@ def construct(self): 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 @@ def create(self, req, server_id, body): 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 @@ def construct(self): 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 @@ def create(self, req, body): 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 2711c45d71a..036e3ff4276 100644 --- a/nova/api/openstack/compute/contrib/volumetypes.py +++ b/nova/api/openstack/compute/contrib/volumetypes.py @@ -53,7 +53,7 @@ def construct(self): 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 @@ def create(self, req, body): 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 d70e2453894..2e993ad8a1f 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 @@ def extraspec_sel(obj, do_raise=False): 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 @@ def _get_extra_specs(self, context, type_id): 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 @@ def index(self, req, type_id): 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 @@ def create(self, req, type_id, body=None): 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 ff21174c3b8..e68093ce8f5 100644 --- a/nova/api/openstack/volume/contrib/types_manage.py +++ b/nova/api/openstack/volume/contrib/types_manage.py @@ -42,13 +42,10 @@ def _create(self, req, body): 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 75539836977..fa070e2e369 100644 --- a/nova/api/openstack/volume/snapshots.py +++ b/nova/api/openstack/volume/snapshots.py @@ -85,7 +85,7 @@ def construct(self): 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 @@ def create(self, req, body): """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 e8303094565..f69ef0a2143 100644 --- a/nova/api/openstack/volume/volumes.py +++ b/nova/api/openstack/volume/volumes.py @@ -192,7 +192,7 @@ def default(self, string): 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 @@ def _items(self, req, entity_maker): @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 4ad6297b8af..af88cf60164 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 @@ def test_create(self): 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 @@ def test_voltype_serializer(self): 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 108ec89646f..7ed04f1ad0c 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 @@ def test_volume_create(self): 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 @@ def test_full_volume(self): } 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 c25ec8c01c8..edc127b8a98 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 @@ def test_create(self): 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 @@ def test_update_item(self): 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 @@ def test_update_show_serializer(self): 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 70c89d8969b..f6903814429 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 @@ def test_create(self): 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 c65182cb733..66f8cddc39b 100644 --- a/nova/tests/api/openstack/volume/test_snapshots.py +++ b/nova/tests/api/openstack/volume/test_snapshots.py @@ -254,3 +254,32 @@ def test_snapshot_index_detail_serializer(self): 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 46a772fbd95..0b9872a5a8f 100644 --- a/nova/tests/api/openstack/volume/test_volumes.py +++ b/nova/tests/api/openstack/volume/test_volumes.py @@ -83,14 +83,6 @@ def test_volume_creation_fails_with_bad_size(self): 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 @@ def test_full_volume(self): } 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)