From: Rajesh Tailor Date: Fri, 29 May 2015 10:26:22 +0000 (-0700) Subject: Validate bool value using strutils.bool_from_string method X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=24fd26fc98acf1fbc07f6922ace5de14296d8f0e;p=openstack-build%2Fcinder-build.git Validate bool value using strutils.bool_from_string method bool value is validated with different logic scattered all over the places 1. isinstance of six.string_type or bool 2. is_valid_boolstr method Made changes to use strutils.bool_from_string method to validate bool value at all places as this method does all things expected above. Changes are not made in v1 api as it is going to be deprecated in Liberty release. Example error message: Invalid value for force_host_copy: Unrecognized value 'xyz', acceptable values are: '0', '1', 'f', 'false', 'n', 'no', 'off', 'on', 't', 'true', 'y', 'yes' ApiImpact: 400 BadRequest response will be returned in case invalid force value is provided during volume upload to image, earlier it was ignored. Closes-Bug: 1460575 Change-Id: Ie64d429f5860a950cda9b363a65c5282f128f10b --- diff --git a/cinder/api/contrib/admin_actions.py b/cinder/api/contrib/admin_actions.py index 3ef3511c5..f11b1bd7e 100644 --- a/cinder/api/contrib/admin_actions.py +++ b/cinder/api/contrib/admin_actions.py @@ -15,7 +15,6 @@ from oslo_log import log as logging import oslo_messaging as messaging from oslo_utils import strutils -import six import webob from webob import exc @@ -222,17 +221,13 @@ class VolumeAdminController(AdminController): host = params['host'] except KeyError: raise exc.HTTPBadRequest(explanation=_("Must specify 'host'")) - force_host_copy = params.get('force_host_copy', False) - if isinstance(force_host_copy, six.string_types): - try: - force_host_copy = strutils.bool_from_string(force_host_copy, - strict=True) - except ValueError: - raise exc.HTTPBadRequest( - explanation=_("Bad value for 'force_host_copy'")) - elif not isinstance(force_host_copy, bool): - raise exc.HTTPBadRequest( - explanation=_("'force_host_copy' not string or bool")) + force_host_copy = params.get('force_host_copy', 'False') + try: + force_host_copy = strutils.bool_from_string(force_host_copy, + strict=True) + except ValueError as e: + msg = (_("Invalid value for force_host_copy: '%s'") % e.message) + raise exc.HTTPBadRequest(explanation=msg) self.volume_api.migrate_volume(context, volume, host, force_host_copy) return webob.Response(status_int=202) diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py index d6c88309d..b99f9e205 100644 --- a/cinder/api/contrib/volume_actions.py +++ b/cinder/api/contrib/volume_actions.py @@ -265,15 +265,11 @@ class VolumeActionsController(wsgi.Controller): msg = _("No image_name was specified in request.") raise webob.exc.HTTPBadRequest(explanation=msg) - force = params.get('force', False) - if isinstance(force, six.string_types): - try: - force = strutils.bool_from_string(force, strict=False) - except ValueError: - msg = _("Bad value for 'force' parameter.") - raise webob.exc.HTTPBadRequest(explanation=msg) - elif not isinstance(force, bool): - msg = _("'force' is not string or bool.") + force = params.get('force', 'False') + try: + force = strutils.bool_from_string(force, strict=True) + except ValueError as error: + msg = _("Invalid value for 'force': '%s'") % error.message raise webob.exc.HTTPBadRequest(explanation=msg) try: @@ -337,16 +333,11 @@ class VolumeActionsController(wsgi.Controller): msg = _("Must specify readonly in request.") raise webob.exc.HTTPBadRequest(explanation=msg) - if isinstance(readonly_flag, six.string_types): - try: - readonly_flag = strutils.bool_from_string(readonly_flag, - strict=True) - except ValueError: - msg = _("Bad value for 'readonly'") - raise webob.exc.HTTPBadRequest(explanation=msg) - - elif not isinstance(readonly_flag, bool): - msg = _("'readonly' not string or bool") + try: + readonly_flag = strutils.bool_from_string(readonly_flag, + strict=True) + except ValueError as error: + msg = _("Invalid value for 'readonly': '%s'") % error.message raise webob.exc.HTTPBadRequest(explanation=msg) self.volume_api.update_readonly_flag(context, volume, readonly_flag) @@ -382,16 +373,11 @@ class VolumeActionsController(wsgi.Controller): msg = _("Must specify bootable in request.") raise webob.exc.HTTPBadRequest(explanation=msg) - if isinstance(bootable, six.string_types): - try: - bootable = strutils.bool_from_string(bootable, - strict=True) - except ValueError: - msg = _("Bad value for 'bootable'") - raise webob.exc.HTTPBadRequest(explanation=msg) - - elif not isinstance(bootable, bool): - msg = _("'bootable' not string or bool") + try: + bootable = strutils.bool_from_string(bootable, + strict=True) + except ValueError as error: + msg = _("Invalid value for 'bootable': '%s'") % error.message raise webob.exc.HTTPBadRequest(explanation=msg) update_dict = {'bootable': bootable} diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py index d3976629d..c31797714 100644 --- a/cinder/api/v2/snapshots.py +++ b/cinder/api/v2/snapshots.py @@ -196,11 +196,13 @@ class SnapshotsController(wsgi.Controller): snapshot['display_name'] = snapshot.get('name') del snapshot['name'] - if not utils.is_valid_boolstr(force): - msg = _("Invalid value '%s' for force. ") % force + try: + force = strutils.bool_from_string(force, strict=True) + except ValueError as error: + msg = _("Invalid value for 'force': '%s'") % error.message raise exception.InvalidParameterValue(err=msg) - if strutils.bool_from_string(force): + if force: new_snapshot = self.volume_api.create_snapshot_force( context, volume, diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index 0d6034c15..bc6a892d9 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -841,7 +841,7 @@ class AdminActionsTest(test.TestCase): db.snapshot_create(ctx, {'volume_id': volume['id']}) self._migrate_volume_exec(ctx, volume, host, expected_status) - def test_migrate_volume_bad_force_host_copy1(self): + def test_migrate_volume_bad_force_host_copy(self): expected_status = 400 host = 'test2' ctx = context.RequestContext('admin', 'fake', True) @@ -849,14 +849,6 @@ class AdminActionsTest(test.TestCase): self._migrate_volume_exec(ctx, volume, host, expected_status, force_host_copy='foo') - def test_migrate_volume_bad_force_host_copy2(self): - expected_status = 400 - host = 'test2' - ctx = context.RequestContext('admin', 'fake', True) - volume = self._migrate_volume_prep() - self._migrate_volume_exec(ctx, volume, host, expected_status, - force_host_copy=1) - def _migrate_volume_comp_exec(self, ctx, volume, new_volume, error, expected_status, expected_id, no_body=False): req = webob.Request.blank('/v2/fake/volumes/%s/action' % volume['id'])