From c1356eeca9434ff24a0a5db799d83a7a509f430b Mon Sep 17 00:00:00 2001 From: pooja jadhav Date: Sun, 14 Jan 2018 11:48:25 +0530 Subject: [PATCH] Remove leading and trailing spaces from parameters Snapshots and backup (create/update) APIs stores 'name' and description' parameters with leading and trailing spaces in database after patches [1][2] are merged. This patch removes leading and trailing whitespaces before validation and also before storing it in db for 'name' and 'description' parameters. [1]https://review.openstack.org/#/c/520991/ [2]https://review.openstack.org/#/c/530098/ Closes-Bug: #1742907 Change-Id: Ib5c8d32e2c20cbacebd7b6f7281a44f94a70f7c0 --- cinder/api/contrib/backups.py | 1 + cinder/api/openstack/wsgi.py | 13 ++-- cinder/api/v2/snapshots.py | 2 + cinder/api/v3/backups.py | 1 + cinder/tests/unit/api/contrib/test_backups.py | 40 +++++++++++ cinder/tests/unit/api/v2/test_snapshots.py | 66 +++++++++++++++++++ cinder/tests/unit/api/v3/test_backups.py | 39 +++++++++++ 7 files changed, 156 insertions(+), 6 deletions(-) diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index ecc2b937625..acb9c460582 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -156,6 +156,7 @@ class BackupsController(wsgi.Controller): container = backup.get('container', None) volume_id = backup['volume_id'] + self.validate_name_and_description(backup, check_length=False) name = backup.get('name', None) description = backup.get('description', None) incremental = strutils.bool_from_string(backup.get( diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py index 0fae7697d57..958a9f1e34e 100644 --- a/cinder/api/openstack/wsgi.py +++ b/cinder/api/openstack/wsgi.py @@ -1250,18 +1250,19 @@ class Controller(object): raise webob.exc.HTTPBadRequest(explanation=fail_msg) @staticmethod - def validate_name_and_description(body): + def validate_name_and_description(body, check_length=True): for attribute in ['name', 'description', 'display_name', 'display_description']: value = body.get(attribute) if value is not None: if isinstance(value, six.string_types): body[attribute] = value.strip() - try: - utils.check_string_length(body[attribute], attribute, - min_length=0, max_length=255) - except exception.InvalidInput as error: - raise webob.exc.HTTPBadRequest(explanation=error.msg) + if check_length: + try: + utils.check_string_length(body[attribute], attribute, + min_length=0, max_length=255) + except exception.InvalidInput as error: + raise webob.exc.HTTPBadRequest(explanation=error.msg) @staticmethod def validate_string_length(value, entity_name, min_length=0, diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py index 084cc728658..b2f61c07ffe 100644 --- a/cinder/api/v2/snapshots.py +++ b/cinder/api/v2/snapshots.py @@ -121,6 +121,7 @@ class SnapshotsController(wsgi.Controller): force = strutils.bool_from_string(force, strict=True) LOG.info("Create snapshot from volume %s", volume_id) + self.validate_name_and_description(snapshot, check_length=False) # NOTE(thingee): v2 API allows name instead of display_name if 'name' in snapshot: snapshot['display_name'] = snapshot.pop('name') @@ -148,6 +149,7 @@ class SnapshotsController(wsgi.Controller): """Update a snapshot.""" context = req.environ['cinder.context'] snapshot_body = body['snapshot'] + self.validate_name_and_description(snapshot_body, check_length=False) if 'name' in snapshot_body: snapshot_body['display_name'] = snapshot_body.pop('name') diff --git a/cinder/api/v3/backups.py b/cinder/api/v3/backups.py index 59dbabf5015..2056372c175 100644 --- a/cinder/api/v3/backups.py +++ b/cinder/api/v3/backups.py @@ -44,6 +44,7 @@ class BackupsController(backups_v2.BackupsController): backup_update = body['backup'] + self.validate_name_and_description(backup_update, check_length=False) update_dict = {} if 'name' in backup_update: update_dict['display_name'] = backup_update.pop('name') diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 109f1bdb9ea..de23be9d327 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -515,6 +515,46 @@ class BackupsAPITestCase(test.TestCase): volume.destroy() + @ddt.data({"backup": {"description": " sample description", + "name": " test name"}}, + {"backup": {"description": "sample description ", + "name": "test "}}, + {"backup": {"description": " sample description ", + "name": " test "}}) + @mock.patch('cinder.db.service_get_all') + def test_create_backup_name_description_with_leading_trailing_spaces( + self, body, _mock_service_get_all): + _mock_service_get_all.return_value = [ + {'availability_zone': 'fake_az', 'host': 'testhost', + 'disabled': 0, 'updated_at': timeutils.utcnow(), + 'uuid': fake.BACKUP_ID}] + + volume = utils.create_volume(self.context, size=5) + body['backup']['volume_id'] = volume.id + req = webob.Request.blank('/v2/%s/backups' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(fakes.wsgi_app( + fake_auth_context=self.user_context)) + res_dict = jsonutils.loads(res.body) + + # create backup call doesn't return 'description' in response so get + # the created backup to assert name and description + req = webob.Request.blank('/v2/%s/backups/%s' % ( + fake.PROJECT_ID, res_dict['backup']['id'])) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app( + fake_auth_context=self.user_context)) + res_dict = jsonutils.loads(res.body) + + self.assertEqual(body['backup']['name'].strip(), + res_dict['backup']['name']) + self.assertEqual(body['backup']['description'].strip(), + res_dict['backup']['description']) + volume.destroy() + @mock.patch('cinder.db.service_get_all') def test_create_backup_with_metadata(self, _mock_service_get_all): _mock_service_get_all.return_value = [ diff --git a/cinder/tests/unit/api/v2/test_snapshots.py b/cinder/tests/unit/api/v2/test_snapshots.py index c5e497959e5..d9e1554267c 100644 --- a/cinder/tests/unit/api/v2/test_snapshots.py +++ b/cinder/tests/unit/api/v2/test_snapshots.py @@ -210,6 +210,24 @@ class SnapshotApiTest(test.TestCase): self.assertRaises(exception.ValidationError, self.controller.create, req, body=body) + @ddt.data({"snapshot": {"description": " sample description", + "name": " test"}}, + {"snapshot": {"description": "sample description ", + "name": "test "}}, + {"snapshot": {"description": " sample description ", + "name": " test name "}}) + def test_snapshot_create_with_leading_trailing_spaces(self, body): + volume = utils.create_volume(self.ctx) + body['snapshot']['volume_id'] = volume.id + req = fakes.HTTPRequest.blank('/v2/snapshots') + resp_dict = self.controller.create(req, body=body) + + self.assertEqual(body['snapshot']['display_name'].strip(), + resp_dict['snapshot']['name']) + self.assertEqual(body['snapshot']['description'].strip(), + resp_dict['snapshot']['description']) + db.volume_destroy(self.ctx, volume.id) + @mock.patch.object(volume.api.API, "update_snapshot", side_effect=v2_fakes.fake_snapshot_update) @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) @@ -316,6 +334,54 @@ class SnapshotApiTest(test.TestCase): self.assertRaises(exception.SnapshotNotFound, self.controller.update, req, 'not-the-uuid', body=body) + @mock.patch.object(volume.api.API, "update_snapshot", + side_effect=v2_fakes.fake_snapshot_update) + @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) + @mock.patch('cinder.db.volume_get') + @mock.patch('cinder.objects.Snapshot.get_by_id') + def test_snapshot_update_with_leading_trailing_spaces( + self, snapshot_get_by_id, volume_get, + snapshot_metadata_get, update_snapshot): + snapshot = { + 'id': UUID, + 'volume_id': fake.VOLUME_ID, + 'status': fields.SnapshotStatus.AVAILABLE, + 'created_at': "2018-01-14 00:00:00", + 'volume_size': 100, + 'display_name': 'Default name', + 'display_description': 'Default description', + 'expected_attrs': ['metadata'], + } + ctx = context.RequestContext(fake.PROJECT_ID, fake.USER_ID, True) + snapshot_obj = fake_snapshot.fake_snapshot_obj(ctx, **snapshot) + fake_volume_obj = fake_volume.fake_volume_obj(ctx) + snapshot_get_by_id.return_value = snapshot_obj + volume_get.return_value = fake_volume_obj + + updates = { + "name": " test ", + "description": " test " + } + body = {"snapshot": updates} + req = fakes.HTTPRequest.blank('/v2/snapshots/%s' % UUID) + res_dict = self.controller.update(req, UUID, body=body) + expected = { + 'snapshot': { + 'id': UUID, + 'volume_id': fake.VOLUME_ID, + 'status': fields.SnapshotStatus.AVAILABLE, + 'size': 100, + 'created_at': datetime.datetime(2018, 1, 14, 0, 0, 0, + tzinfo=pytz.utc), + 'updated_at': None, + 'name': u'test', + 'description': u'test', + 'metadata': {}, + } + } + self.assertEqual(expected, res_dict) + self.assertEqual(2, len(self.notifier.notifications)) + @mock.patch.object(volume.api.API, "delete_snapshot", side_effect=v2_fakes.fake_snapshot_update) @mock.patch('cinder.db.snapshot_metadata_get', return_value=dict()) diff --git a/cinder/tests/unit/api/v3/test_backups.py b/cinder/tests/unit/api/v3/test_backups.py index 6c54cb9f52f..acf55a7c0fa 100644 --- a/cinder/tests/unit/api/v3/test_backups.py +++ b/cinder/tests/unit/api/v3/test_backups.py @@ -15,9 +15,12 @@ """The backups V3 api.""" +import copy import ddt import mock +from oslo_serialization import jsonutils from oslo_utils import strutils +import webob from cinder.api import microversions as mv from cinder.api.openstack import api_version_request as api_version @@ -44,6 +47,8 @@ class BackupsControllerAPITestCase(test.TestCase): auth_token=True, is_admin=True) self.controller = backups.BackupsController() + self.user_context = context.RequestContext( + fake.USER_ID, fake.PROJECT_ID, auth_token=True) def _fake_update_request(self, backup_id, version=mv.BACKUP_UPDATE): req = fakes.HTTPRequest.blank('/v3/%s/backups/%s/update' % @@ -231,6 +236,40 @@ class BackupsControllerAPITestCase(test.TestCase): self.assertEqual(new_description, backup.display_description) + @ddt.data({"backup": {"description": " sample description", + "name": " test name"}}, + {"backup": {"description": "sample description ", + "name": "test "}}, + {"backup": {"description": " sample description ", + "name": " test "}}) + def test_backup_update_name_description_with_leading_trailing_spaces( + self, body): + backup = test_utils.create_backup( + self.ctxt, + status=fields.BackupStatus.AVAILABLE) + req = self._fake_update_request(fake.BACKUP_ID) + + expected_body = copy.deepcopy(body) + self.controller.update(req, + backup.id, + body=body) + backup.refresh() + + # backup update call doesn't return 'description' in response so get + # the updated backup to assert name and description + req = webob.Request.blank('/v2/%s/backups/%s' % ( + fake.PROJECT_ID, backup.id)) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app( + fake_auth_context=self.user_context)) + res_dict = jsonutils.loads(res.body) + + self.assertEqual(expected_body['backup']['name'].strip(), + res_dict['backup']['name']) + self.assertEqual(expected_body['backup']['description'].strip(), + res_dict['backup']['description']) + @ddt.data(mv.get_prior_version(mv.BACKUP_METADATA), mv.BACKUP_METADATA) def test_backup_show_with_metadata(self, version):