From a1bb185a1f0311c36fd590d48531ded6ccccc5c6 Mon Sep 17 00:00:00 2001 From: PranaliDeore Date: Mon, 11 May 2015 04:00:01 -0700 Subject: [PATCH] Validate name and description string If you pass name or description parameters with more than 255 characters to create and update apis of volume and snapshot and create api of backup, then it returns 500 error code. Added new method validate_name_and_description() in cinder.api.openstack.wsgi.Controllera to validate string limit and returned 400 if limit exceeds and also removing leading or trailing whitespaces and string containing only whitespaces. APIImpact 1. For all above APIs 400 response will be returned. 2. Earlier it was possible to pass only whitespaces or leading-trailing spaces to 'name' parameter. Now it will raise 400 error if only whitespaces are passed and will remove leading-trailing spaces if present in other cases. Closes-Bug: 1454244 Change-Id: Iaf7159e816f69fd776a09828c3bc1d27fc9fdcdb --- cinder/api/contrib/backups.py | 1 + cinder/api/openstack/wsgi.py | 20 ++++++++ cinder/api/v2/snapshots.py | 11 ++-- cinder/api/v2/volumes.py | 22 ++++---- cinder/tests/unit/api/contrib/test_backups.py | 24 +++++++-- cinder/tests/unit/api/openstack/test_wsgi.py | 41 +++++++++++++++ cinder/tests/unit/api/v2/test_snapshots.py | 16 ++++-- cinder/tests/unit/api/v2/test_volumes.py | 50 +++++++++++++++---- 8 files changed, 149 insertions(+), 36 deletions(-) diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index 6c5066071..7a1d00839 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -252,6 +252,7 @@ class BackupsController(wsgi.Controller): msg = _("Incorrect request body format") raise exc.HTTPBadRequest(explanation=msg) container = backup.get('container', None) + self.validate_name_and_description(backup) name = backup.get('name', None) description = backup.get('description', None) incremental = backup.get('incremental', False) diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py index d2502d4eb..cad8c908e 100644 --- a/cinder/api/openstack/wsgi.py +++ b/cinder/api/openstack/wsgi.py @@ -1220,6 +1220,26 @@ class Controller(object): explanation=_("Missing required element '%s' in " "request body.") % entity_name) + @staticmethod + def validate_name_and_description(body): + name = body.get('name') + if name is not None: + if isinstance(name, six.string_types): + body['name'] = name.strip() + try: + utils.check_string_length(body['name'], 'Name', + min_length=1, max_length=255) + except exception.InvalidInput as error: + raise webob.exc.HTTPBadRequest(explanation=error.msg) + + description = body.get('description') + if description is not None: + try: + utils.check_string_length(description, 'Description', + min_length=0, max_length=255) + except exception.InvalidInput as error: + raise webob.exc.HTTPBadRequest(explanation=error.msg) + class Fault(webob.exc.HTTPException): """Wrap webob.exc.HTTPException to provide API friendly response.""" diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py index b38d1effc..3ee8146ac 100644 --- a/cinder/api/v2/snapshots.py +++ b/cinder/api/v2/snapshots.py @@ -185,11 +185,11 @@ class SnapshotsController(wsgi.Controller): force = snapshot.get('force', False) msg = _LI("Create snapshot from volume %s") LOG.info(msg, volume_id, context=context) + self.validate_name_and_description(snapshot) # NOTE(thingee): v2 API allows name instead of display_name if 'name' in snapshot: - snapshot['display_name'] = snapshot.get('name') - del snapshot['name'] + snapshot['display_name'] = snapshot.pop('name') try: force = strutils.bool_from_string(force, strict=True) @@ -240,17 +240,16 @@ class SnapshotsController(wsgi.Controller): 'display_name', 'display_description', ) + self.validate_name_and_description(snapshot) # NOTE(thingee): v2 API allows name instead of display_name if 'name' in snapshot: - snapshot['display_name'] = snapshot['name'] - del snapshot['name'] + snapshot['display_name'] = snapshot.pop('name') # NOTE(thingee): v2 API allows description instead of # display_description if 'description' in snapshot: - snapshot['display_description'] = snapshot['description'] - del snapshot['description'] + snapshot['display_description'] = snapshot.pop('description') for key in valid_update_keys: if key in snapshot: diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index 852515cc0..4cb7e8593 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -326,17 +326,16 @@ class VolumeController(wsgi.Controller): volume = body['volume'] kwargs = {} + self.validate_name_and_description(volume) # NOTE(thingee): v2 API allows name instead of display_name - if volume.get('name'): - volume['display_name'] = volume.get('name') - del volume['name'] + if 'name' in volume: + volume['display_name'] = volume.pop('name') # NOTE(thingee): v2 API allows description instead of # display_description - if volume.get('description'): - volume['display_description'] = volume.get('description') - del volume['description'] + if 'description' in volume: + volume['display_description'] = volume.pop('description') if 'image_id' in volume: volume['imageRef'] = volume.get('image_id') @@ -471,15 +470,16 @@ class VolumeController(wsgi.Controller): if key in volume: update_dict[key] = volume[key] + self.validate_name_and_description(update_dict) + # NOTE(thingee): v2 API allows name instead of display_name if 'name' in update_dict: - update_dict['display_name'] = update_dict['name'] - del update_dict['name'] + update_dict['display_name'] = update_dict.pop('name') - # NOTE(thingee): v2 API allows name instead of display_name + # NOTE(thingee): v2 API allows description instead of + # display_description if 'description' in update_dict: - update_dict['display_description'] = update_dict['description'] - del update_dict['description'] + update_dict['display_description'] = update_dict.pop('description') try: volume = self.volume_api.get(context, id, viewable_admin_meta=True) diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index fdb4176b8..033d33aac 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -378,7 +378,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id1) @mock.patch('cinder.db.service_get_all_by_topic') - def test_create_backup_json(self, _mock_service_get_all_by_topic): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_create_backup_json(self, mock_validate, + _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ {'availability_zone': "fake_az", 'host': 'test_host', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -403,6 +406,7 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(res.status_int, 202) self.assertIn('id', res_dict['backup']) self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(mock_validate.called) db.volume_destroy(context.get_admin_context(), volume_id) @@ -470,7 +474,10 @@ class BackupsAPITestCase(test.TestCase): db.volume_destroy(context.get_admin_context(), volume_id) @mock.patch('cinder.db.service_get_all_by_topic') - def test_create_backup_snapshot_json(self, _mock_service_get_all_by_topic): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_create_backup_snapshot_json(self, mock_validate, + _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ {'availability_zone': "fake_az", 'host': 'test_host', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -495,11 +502,15 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(res.status_int, 202) self.assertIn('id', res_dict['backup']) self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(mock_validate.called) db.volume_destroy(context.get_admin_context(), volume_id) @mock.patch('cinder.db.service_get_all_by_topic') - def test_create_backup_xml(self, _mock_service_get_all_by_topic): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_create_backup_xml(self, mock_validate, + _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ {'availability_zone': "fake_az", 'host': 'test_host', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -520,11 +531,15 @@ class BackupsAPITestCase(test.TestCase): backup = dom.getElementsByTagName('backup') self.assertTrue(backup.item(0).hasAttribute('id')) self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(mock_validate.called) db.volume_destroy(context.get_admin_context(), volume_id) @mock.patch('cinder.db.service_get_all_by_topic') - def test_create_backup_delta(self, _mock_service_get_all_by_topic): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_create_backup_delta(self, mock_validate, + _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ {'availability_zone': "fake_az", 'host': 'test_host', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -550,6 +565,7 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertIn('id', res_dict['backup']) self.assertTrue(_mock_service_get_all_by_topic.called) + self.assertTrue(mock_validate.called) db.backup_destroy(context.get_admin_context(), backup_id) db.volume_destroy(context.get_admin_context(), volume_id) diff --git a/cinder/tests/unit/api/openstack/test_wsgi.py b/cinder/tests/unit/api/openstack/test_wsgi.py index eaca995ed..b9dbf8d77 100644 --- a/cinder/tests/unit/api/openstack/test_wsgi.py +++ b/cinder/tests/unit/api/openstack/test_wsgi.py @@ -983,3 +983,44 @@ class ValidBodyTest(test.TestCase): wsgi.Resource(controller=None) body = {'foo': 'bar'} self.assertFalse(self.controller.is_valid_body(body, 'foo')) + + def test_validate_name_and_description_with_name_too_long(self): + body = {'name': 'a' * 256} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.validate_name_and_description, + body) + + def test_validate_name_and_description_with_desc_too_long(self): + body = {'description': 'a' * 256} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.validate_name_and_description, + body) + + def test_validate_name_and_description_with_name_as_int(self): + body = {'name': 1234} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.validate_name_and_description, + body) + + def test_validate_name_and_description_with_desc_as_int(self): + body = {'description': 1234} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.validate_name_and_description, + body) + + def test_validate_name_and_description_with_name_zero_length(self): + body = {'name': ""} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.validate_name_and_description, + body) + + def test_validate_name_and_description_with_desc_zero_length(self): + body = {'description': ""} + self.controller.validate_name_and_description(body) + self.assertEqual('', body['description']) + + def test_validate_name_and_description_with_name_contains_whitespaces( + self): + body = {'name': 'a' * 255 + " "} + self.controller.validate_name_and_description(body) + self.assertEqual('a' * 255, body['name']) diff --git a/cinder/tests/unit/api/v2/test_snapshots.py b/cinder/tests/unit/api/v2/test_snapshots.py index 83324c8d5..fb0e63094 100644 --- a/cinder/tests/unit/api/v2/test_snapshots.py +++ b/cinder/tests/unit/api/v2/test_snapshots.py @@ -90,7 +90,9 @@ class SnapshotApiTest(test.TestCase): self.stubs.Set(db, 'snapshot_get_all', stubs.stub_snapshot_get_all) - def test_snapshot_create(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_snapshot_create(self, mock_validate): self.stubs.Set(volume.api.API, "create_snapshot", stub_snapshot_create) self.stubs.Set(volume.api.API, 'get', stubs.stub_volume_get) snapshot_name = 'Snapshot Test Name' @@ -107,10 +109,10 @@ class SnapshotApiTest(test.TestCase): resp_dict = self.controller.create(req, body) self.assertIn('snapshot', resp_dict) - self.assertEqual(snapshot_name, - resp_dict['snapshot']['name']) + self.assertEqual(snapshot_name, resp_dict['snapshot']['name']) self.assertEqual(snapshot_description, resp_dict['snapshot']['description']) + self.assertTrue(mock_validate.called) def test_snapshot_create_force(self): self.stubs.Set(volume.api.API, "create_snapshot_force", @@ -166,8 +168,11 @@ class SnapshotApiTest(test.TestCase): @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) @mock.patch('cinder.objects.Volume.get_by_id') @mock.patch('cinder.objects.Snapshot.get_by_id') - def test_snapshot_update(self, snapshot_get_by_id, volume_get_by_id, - snapshot_metadata_get, update_snapshot): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_snapshot_update( + self, mock_validate, snapshot_get_by_id, volume_get_by_id, + snapshot_metadata_get, update_snapshot): snapshot = { 'id': UUID, 'volume_id': 1, @@ -202,6 +207,7 @@ class SnapshotApiTest(test.TestCase): } } self.assertEqual(expected, res_dict) + self.assertTrue(mock_validate.called) self.assertEqual(2, len(self.notifier.notifications)) def test_snapshot_update_missing_body(self): diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index f26cd7097..142bdcb5f 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -61,7 +61,9 @@ class VolumeApiTest(test.TestCase): stubs.stub_service_get_all_by_topic) self.maxDiff = None - def test_volume_create(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_create(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -71,8 +73,11 @@ class VolumeApiTest(test.TestCase): res_dict = self.controller.create(req, body) ex = self._expected_vol_from_controller() self.assertEqual(ex, res_dict) + self.assertTrue(mock_validate.called) - def test_volume_create_with_type(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_create_with_type(self, mock_validate): vol_type = db.volume_type_create( context.get_admin_context(), dict(name=CONF.default_volume_type, extra_specs={}) @@ -108,6 +113,7 @@ class VolumeApiTest(test.TestCase): volume_type={'name': vol_type})]) req = fakes.HTTPRequest.blank('/v2/volumes/detail') res_dict = self.controller.detail(req) + self.assertTrue(mock_validate.called) def _vol_in_request_body(self, size=stubs.DEFAULT_VOL_SIZE, @@ -351,7 +357,9 @@ class VolumeApiTest(test.TestCase): self.controller.create, req, body) - def test_volume_create_with_image_ref(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_create_with_image_ref(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -364,6 +372,7 @@ class VolumeApiTest(test.TestCase): req = fakes.HTTPRequest.blank('/v2/volumes') res_dict = self.controller.create(req, body) self.assertEqual(ex, res_dict) + self.assertTrue(mock_validate.called) def test_volume_create_with_image_ref_is_integer(self): self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -401,7 +410,9 @@ class VolumeApiTest(test.TestCase): req, body) - def test_volume_create_with_image_id(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_create_with_image_id(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -414,6 +425,7 @@ class VolumeApiTest(test.TestCase): req = fakes.HTTPRequest.blank('/v2/volumes') res_dict = self.controller.create(req, body) self.assertEqual(ex, res_dict) + self.assertTrue(mock_validate.called) def test_volume_create_with_image_id_is_integer(self): self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -451,7 +463,9 @@ class VolumeApiTest(test.TestCase): req, body) - def test_volume_create_with_image_name(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_create_with_image_name(self, mock_validate): self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) self.stubs.Set(fake_image._FakeImageService, @@ -467,6 +481,7 @@ class VolumeApiTest(test.TestCase): req = fakes.HTTPRequest.blank('/v2/volumes') res_dict = self.controller.create(req, body) self.assertEqual(ex, res_dict) + self.assertTrue(mock_validate.called) def test_volume_create_with_image_name_has_multiple(self): self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) @@ -504,7 +519,9 @@ class VolumeApiTest(test.TestCase): req, body) - def test_volume_update(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_update(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) @@ -520,8 +537,11 @@ class VolumeApiTest(test.TestCase): metadata={'attached_mode': 'rw', 'readonly': 'False'}) self.assertEqual(expected, res_dict) self.assertEqual(2, len(self.notifier.notifications)) + self.assertTrue(mock_validate.called) - def test_volume_update_deprecation(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_update_deprecation(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) @@ -539,8 +559,11 @@ class VolumeApiTest(test.TestCase): metadata={'attached_mode': 'rw', 'readonly': 'False'}) self.assertEqual(expected, res_dict) self.assertEqual(2, len(self.notifier.notifications)) + self.assertTrue(mock_validate.called) - def test_volume_update_deprecation_key_priority(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_update_deprecation_key_priority(self, mock_validate): """Test current update keys have priority over deprecated keys.""" self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) @@ -561,8 +584,11 @@ class VolumeApiTest(test.TestCase): metadata={'attached_mode': 'rw', 'readonly': 'False'}) self.assertEqual(expected, res_dict) self.assertEqual(2, len(self.notifier.notifications)) + self.assertTrue(mock_validate.called) - def test_volume_update_metadata(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_update_metadata(self, mock_validate): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) @@ -579,8 +605,11 @@ class VolumeApiTest(test.TestCase): 'qos_max_iops': 2000}) self.assertEqual(expected, res_dict) self.assertEqual(2, len(self.notifier.notifications)) + self.assertTrue(mock_validate.called) - def test_volume_update_with_admin_metadata(self): + @mock.patch( + 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') + def test_volume_update_with_admin_metadata(self, mock_validate): self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) volume = stubs.stub_volume("1") @@ -621,6 +650,7 @@ class VolumeApiTest(test.TestCase): metadata={'key': 'value', 'readonly': 'True'}) self.assertEqual(expected, res_dict) self.assertEqual(2, len(self.notifier.notifications)) + self.assertTrue(mock_validate.called) def test_update_empty_body(self): body = {} -- 2.45.2