openstack
/
cinder
Code Issues Proposed changes

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
This commit is contained in:
PranaliDeore 2015-05-11 04:00:01 -07:00
parent 611608fdc9
commit a1bb185a1f
8 changed files with 150 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
cinder/api/contrib/backups.py
View File

@ -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)

20
cinder/api/openstack/wsgi.py
View File

@ -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."""

11
cinder/api/v2/snapshots.py
View File

@ -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:

24
cinder/api/v2/volumes.py
View File

@ -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]
# 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']
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.pop('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)

24
cinder/tests/unit/api/contrib/test_backups.py
View File

@ -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)

41
cinder/tests/unit/api/openstack/test_wsgi.py
View File

@ -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'])

16
cinder/tests/unit/api/v2/test_snapshots.py
View File

@ -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):

50
cinder/tests/unit/api/v2/test_volumes.py
View File

@ -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 = {}