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

diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py
index 6c5066071f71efab147d30f4d4501203cfd770be..7a1d00839cd057939c3e22676e8303c42b7987da 100644 (file)
--- 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 d2502d4eb85f3091b4de8a95c29dc02a0f6a27ef..cad8c908ea30a61f7dc5209cf73651fa7b2cc583 100644 (file)
--- 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 b38d1effcd73def5401b27b44e8ac1ed4f89c06f..3ee8146ac24e8be392513aa9ea8c131d7c8ae219 100644 (file)
--- 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 852515cc0121eef8899fc4d253514170a075c6cd..4cb7e859358aaf96efd5ccd1aa089b79d41b1a79 100644 (file)
--- 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 fdb4176b854669ea14fad7a45ef1e0e19b49ff7b..033d33aac8715205caf971b45e75d5dd7b64b80a 100644 (file)
--- 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 eaca995ed3e7cd0d499b847bdcbc9bffe2335ecb..b9dbf8d77aa4b08424d16d68a984fa76df6d80ab 100644 (file)
--- 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 83324c8d5ed0eb939d9f49a23e8b169db70b1dc8..fb0e6309481e1db71a4bb4cb9ca53c9b6382ca4f 100644 (file)
--- 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 f26cd7097f651c742a39775ae9f342e7b2b6cbf0..142bdcb5f9765c104ef74542b6f562609964f486 100644 (file)
--- 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 = {}