diff --git a/cinder/api/common.py b/cinder/api/common.py index a2904ab316a..b78b1b82f10 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -68,22 +68,6 @@ ATTRIBUTE_CONVERTERS = {'name~': 'display_name~', METADATA_TYPES = enum.Enum('METADATA_TYPES', 'user image') -# Regex that matches alphanumeric characters, periods, hyphens, -# colons and underscores: -# ^ assert position at start of the string -# [\w\.\-\:\_] match expression -# $ assert position at end of the string -VALID_KEY_NAME_REGEX = re.compile(r"^[\w\.\-\:\_]+$", re.UNICODE) - - -def validate_key_names(key_names_list): - """Validate each item of the list to match key name regex.""" - for key_name in key_names_list: - if not VALID_KEY_NAME_REGEX.match(key_name): - return False - return True - - def get_pagination_params(params, max_limit=None): """Return marker, limit, offset tuple from request. diff --git a/cinder/api/contrib/types_extra_specs.py b/cinder/api/contrib/types_extra_specs.py index ee5ba0989a8..0b172f91fc0 100644 --- a/cinder/api/contrib/types_extra_specs.py +++ b/cinder/api/contrib/types_extra_specs.py @@ -21,16 +21,16 @@ from oslo_log import versionutils from six.moves import http_client import webob -from cinder.api import common from cinder.api import extensions from cinder.api.openstack import wsgi +from cinder.api.schemas import types_extra_specs +from cinder.api import validation from cinder import context as ctxt from cinder import db from cinder import exception from cinder.i18n import _ from cinder.policies import type_extra_specs as policy from cinder import rpc -from cinder import utils from cinder.volume import volume_types LOG = logging.getLogger(__name__) @@ -86,17 +86,14 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): versionutils.report_deprecated_feature(LOG, msg) return - def create(self, req, type_id, body=None): + @validation.schema(types_extra_specs.create) + def create(self, req, type_id, body): context = req.environ['cinder.context'] context.authorize(policy.CREATE_POLICY) self._allow_update(context, type_id) - self.assert_valid_body(body, 'extra_specs') - self._check_type(context, type_id) specs = body['extra_specs'] - self._check_key_names(specs.keys()) - utils.validate_dictionary_string_length(specs) db.volume_type_extra_specs_update_or_create(context, type_id, @@ -111,23 +108,16 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): notifier_info) return body - def update(self, req, type_id, id, body=None): + @validation.schema(types_extra_specs.update) + def update(self, req, type_id, id, body): context = req.environ['cinder.context'] context.authorize(policy.UPDATE_POLICY) self._allow_update(context, type_id) - if not body: - expl = _('Request body empty') - raise webob.exc.HTTPBadRequest(explanation=expl) self._check_type(context, type_id) if id not in body: expl = _('Request body and URI mismatch') raise webob.exc.HTTPBadRequest(explanation=expl) - if len(body) > 1: - expl = _('Request body contains too many items') - raise webob.exc.HTTPBadRequest(explanation=expl) - self._check_key_names(body.keys()) - utils.validate_dictionary_string_length(body) db.volume_type_extra_specs_update_or_create(context, type_id, @@ -177,13 +167,6 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): 'volume_type_extra_specs.delete', notifier_info) - def _check_key_names(self, keys): - if not common.validate_key_names(keys): - expl = _('Key names can only contain alphanumeric characters, ' - 'underscores, periods, colons and hyphens.') - - raise webob.exc.HTTPBadRequest(explanation=expl) - class Types_extra_specs(extensions.ExtensionDescriptor): """Type extra specs support.""" diff --git a/cinder/api/schemas/types_extra_specs.py b/cinder/api/schemas/types_extra_specs.py new file mode 100644 index 00000000000..aa24fd37bd9 --- /dev/null +++ b/cinder/api/schemas/types_extra_specs.py @@ -0,0 +1,39 @@ +# Copyright (C) 2018 NTT DATA +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +""" +Schema for V3 types_extra_specs API. + +""" + +import copy + +from cinder.api.validation import parameter_types + +create = { + 'type': 'object', + 'properties': { + 'extra_specs': parameter_types.extra_specs_with_no_spaces_key + }, + 'required': ['extra_specs'], + 'additionalProperties': False, +} + + +update = copy.deepcopy(parameter_types.extra_specs_with_no_spaces_key) +update.update({ + 'minProperties': 1, + 'maxProperties': 1 +}) diff --git a/cinder/api/validation/parameter_types.py b/cinder/api/validation/parameter_types.py index 9b9d8fb3139..fc44e6b7be4 100644 --- a/cinder/api/validation/parameter_types.py +++ b/cinder/api/validation/parameter_types.py @@ -153,6 +153,17 @@ extra_specs = { } +extra_specs_with_no_spaces_key = { + 'type': 'object', + 'patternProperties': { + '^[a-zA-Z0-9-_:.]{1,255}$': { + 'type': ['string', 'null'], 'minLength': 0, 'maxLength': 255 + } + }, + 'additionalProperties': False +} + + group_snapshot_status = { 'type': 'string', 'format': 'group_snapshot_status' } diff --git a/cinder/tests/unit/api/contrib/test_types_extra_specs.py b/cinder/tests/unit/api/contrib/test_types_extra_specs.py index b0b35fb951b..89f4dfb34cb 100644 --- a/cinder/tests/unit/api/contrib/test_types_extra_specs.py +++ b/cinder/tests/unit/api/contrib/test_types_extra_specs.py @@ -15,6 +15,7 @@ # License for the specific language governing permissions and limitations # under the License. +import ddt import mock from oslo_config import cfg from oslo_utils import timeutils @@ -63,6 +64,7 @@ def fake_volume_type_extra_specs(): return specs +@ddt.ddt class VolumeTypesExtraSpecsTest(test.TestCase): def setUp(self): @@ -130,8 +132,7 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertRaises(exception.VolumeTypeExtraSpecsNotFound, self.controller.delete, req, fake.VOLUME_ID, 'key6') - @mock.patch('cinder.utils.check_string_length') - def test_create(self, mock_check): + def test_create(self): self.mock_object(cinder.db, 'volume_type_extra_specs_update_or_create', return_create_volume_type_extra_specs) @@ -139,17 +140,15 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank(self.api_path) - res_dict = self.controller.create(req, fake.VOLUME_ID, body) + res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertIn('created_at', self.notifier.notifications[0]['payload']) self.assertIn('updated_at', self.notifier.notifications[0]['payload']) - self.assertTrue(mock_check.called) self.assertEqual('value1', res_dict['extra_specs']['key1']) @mock.patch.object(cinder.db, 'volume_type_extra_specs_update_or_create') - @mock.patch('cinder.utils.check_string_length') def test_create_key_allowed_chars( - self, mock_check, volume_type_extra_specs_update_or_create): + self, volume_type_extra_specs_update_or_create): mock_return_value = {"key1": "value1", "key2": "value2", "key3": "value3", @@ -163,16 +162,14 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank(self.api_path) - res_dict = self.controller.create(req, fake.VOLUME_ID, body) + res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) - self.assertTrue(mock_check.called) self.assertEqual('value1', res_dict['extra_specs']['other_alphanum.-_:']) @mock.patch.object(cinder.db, 'volume_type_extra_specs_update_or_create') - @mock.patch('cinder.utils.check_string_length') def test_create_too_many_keys_allowed_chars( - self, mock_check, volume_type_extra_specs_update_or_create): + self, volume_type_extra_specs_update_or_create): mock_return_value = {"key1": "value1", "key2": "value2", "key3": "value3", @@ -188,9 +185,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank(self.api_path) - res_dict = self.controller.create(req, fake.VOLUME_ID, body) + res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) - self.assertTrue(mock_check.called) self.assertEqual('value1', res_dict['extra_specs']['other_alphanum.-_:']) self.assertEqual('value2', @@ -198,8 +194,7 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual('value3', res_dict['extra_specs']['other3_alphanum.-_:']) - @mock.patch('cinder.utils.check_string_length') - def test_update_item(self, mock_check): + def test_update_item(self): self.mock_object(cinder.db, 'volume_type_extra_specs_update_or_create', return_create_volume_type_extra_specs) @@ -207,11 +202,11 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank(self.api_path + '/key1') - res_dict = self.controller.update(req, fake.VOLUME_ID, 'key1', body) + res_dict = self.controller.update(req, fake.VOLUME_ID, 'key1', + body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertIn('created_at', self.notifier.notifications[0]['payload']) self.assertIn('updated_at', self.notifier.notifications[0]['payload']) - self.assertTrue(mock_check.called) self.assertEqual('value1', res_dict['key1']) @@ -222,8 +217,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): body = {"key1": "value1", "key2": "value2"} req = fakes.HTTPRequest.blank(self.api_path + '/key1') - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, - req, fake.VOLUME_ID, 'key1', body) + self.assertRaises(exception.ValidationError, self.controller.update, + req, fake.VOLUME_ID, 'key1', body=body) def test_update_item_body_uri_mismatch(self): self.mock_object(cinder.db, @@ -233,15 +228,16 @@ class VolumeTypesExtraSpecsTest(test.TestCase): req = fakes.HTTPRequest.blank(self.api_path + '/bad') self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, - req, fake.VOLUME_ID, 'bad', body) + req, fake.VOLUME_ID, 'bad', body=body) def _extra_specs_empty_update(self, body): req = fakes.HTTPRequest.blank('/v2/%s/types/%s/extra_specs' % ( fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) req.method = 'POST' - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.update, req, fake.VOLUME_ID, body) + self.assertRaises(exception.ValidationError, + self.controller.update, req, fake.VOLUME_ID, + body=body) def test_update_no_body(self): self._extra_specs_empty_update(body=None) @@ -254,8 +250,9 @@ class VolumeTypesExtraSpecsTest(test.TestCase): fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) req.method = 'POST' - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.create, req, fake.VOLUME_ID, body) + self.assertRaises(exception.ValidationError, + self.controller.create, req, fake.VOLUME_ID, + body=body) def test_create_no_body(self): self._extra_specs_create_bad_body(body=None) @@ -295,10 +292,23 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create, req, - fake.VOLUME_ID, body) + fake.VOLUME_ID, body=body) # Again but with conf set to allow modification CONF.set_default('allow_inuse_volume_type_modification', True) - res_dict = self.controller.create(req, fake.VOLUME_ID, body) + res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual({'extra_specs': {'key1': 'value1'}}, res_dict) + + @ddt.data({'extra_specs': {'a' * 256: 'a'}}, + {'extra_specs': {'a': 'a' * 256}}, + {'extra_specs': {'': 'a'}}, + {'extra_specs': {' ': 'a'}}) + def test_create_with_invalid_extra_specs(self, body): + req = fakes.HTTPRequest.blank('/v2/%s/types/%s/extra_specs' % ( + fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) + req.method = 'POST' + + self.assertRaises(exception.ValidationError, + self.controller.create, req, fake.VOLUME_ID, + body=body)