From fc8d7422a83ef3a5926388dfa9f14d946e06a8eb Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Thu, 31 Dec 2015 19:57:50 +0800 Subject: [PATCH] Remove redundant check after enforcing schema validation check_immutable_params() turn out to be redundant since schema validation that is enforce has the constraint that no additional properties is allowed, any call with immutable params will be caught by json schema. The patch remove the check from controller and the relevant testcases since the restful testcase will never get to the call of the API actually. The patch also remove the method check_immutable_params and relevant testcases since it's not used anywhere. Partially implements: bp schema-validation-extent Change-Id: I017c14e4d1229951e389a9e8d48384b54e197dd3 --- keystone/common/controller.py | 32 +------------- keystone/federation/controllers.py | 14 +----- keystone/tests/unit/test_v3_controller.py | 54 ----------------------- tox.ini | 1 - 4 files changed, 4 insertions(+), 97 deletions(-) delete mode 100644 keystone/tests/unit/test_v3_controller.py diff --git a/keystone/common/controller.py b/keystone/common/controller.py index 32d1574c6c..d1234e3a0d 100644 --- a/keystone/common/controller.py +++ b/keystone/common/controller.py @@ -425,8 +425,6 @@ class V3Controller(wsgi.Application): Class parameters: - * `_mutable_parameters` - set of parameters that can be changed by users. - Usually used by cls.check_immutable_params() * `_public_parameters` - set of parameters that are exposed to the user. Usually used by cls.filter_params() @@ -811,38 +809,12 @@ class V3Controller(wsgi.Application): utils.flatten_dict(policy_dict)) LOG.debug('RBAC: Authorization granted') - @classmethod - def check_immutable_params(cls, ref): - """Raise exception when disallowed parameter is in ref. - - Check whether the ref dictionary representing a request has only - mutable parameters included. If not, raise an exception. This method - checks only root-level keys from a ref dictionary. - - :param ref: a dictionary representing deserialized request to be - stored - :raises: :class:`keystone.exception.ImmutableAttributeError` - - """ - ref_keys = set(ref.keys()) - blocked_keys = ref_keys.difference(cls._mutable_parameters) - - if not blocked_keys: - # No immutable parameters changed - return - - exception_args = {'target': cls.__name__, - 'attributes': ', '.join(blocked_keys)} - raise exception.ImmutableAttributeError(**exception_args) - @classmethod def filter_params(cls, ref): """Remove unspecified parameters from the dictionary. - This function removes unspecified parameters from the dictionary. See - check_immutable_parameters for corresponding function that raises - exceptions. This method checks only root-level keys from a ref - dictionary. + This function removes unspecified parameters from the dictionary. + This method checks only root-level keys from a ref dictionary. :param ref: a dictionary representing deserialized response to be serialized diff --git a/keystone/federation/controllers.py b/keystone/federation/controllers.py index 612486d813..56bddddd55 100644 --- a/keystone/federation/controllers.py +++ b/keystone/federation/controllers.py @@ -55,7 +55,6 @@ class IdentityProvider(_ControllerBase): collection_name = 'identity_providers' member_name = 'identity_provider' - _mutable_parameters = frozenset(['description', 'enabled', 'remote_ids']) _public_parameters = frozenset(['id', 'enabled', 'description', 'remote_ids', 'links' ]) @@ -95,7 +94,6 @@ class IdentityProvider(_ControllerBase): def create_identity_provider(self, context, idp_id, identity_provider): identity_provider = self._normalize_dict(identity_provider) identity_provider.setdefault('enabled', False) - IdentityProvider.check_immutable_params(identity_provider) idp_ref = self.federation_api.create_idp(idp_id, identity_provider) response = IdentityProvider.wrap_member(context, idp_ref) return wsgi.render_response(body=response, status=('201', 'Created')) @@ -119,7 +117,6 @@ class IdentityProvider(_ControllerBase): @validation.validated(schema.identity_provider_update, 'identity_provider') def update_identity_provider(self, context, idp_id, identity_provider): identity_provider = self._normalize_dict(identity_provider) - IdentityProvider.check_immutable_params(identity_provider) idp_ref = self.federation_api.update_idp(idp_id, identity_provider) return IdentityProvider.wrap_member(context, idp_ref) @@ -128,8 +125,8 @@ class IdentityProvider(_ControllerBase): class FederationProtocol(_ControllerBase): """A federation protocol representation. - See IdentityProvider docstring for explanation on _mutable_parameters - and _public_parameters class attributes. + See keystone.common.controller.V3Controller docstring for explanation + on _public_parameters class attributes. """ @@ -137,7 +134,6 @@ class FederationProtocol(_ControllerBase): member_name = 'protocol' _public_parameters = frozenset(['id', 'mapping_id', 'links']) - _mutable_parameters = frozenset(['mapping_id']) @classmethod def _add_self_referential_link(cls, context, ref): @@ -184,7 +180,6 @@ class FederationProtocol(_ControllerBase): @validation.validated(schema.federation_protocol_schema, 'protocol') def create_protocol(self, context, idp_id, protocol_id, protocol): ref = self._normalize_dict(protocol) - FederationProtocol.check_immutable_params(ref) ref = self.federation_api.create_protocol(idp_id, protocol_id, ref) response = FederationProtocol.wrap_member(context, ref) return wsgi.render_response(body=response, status=('201', 'Created')) @@ -193,7 +188,6 @@ class FederationProtocol(_ControllerBase): @validation.validated(schema.federation_protocol_schema, 'protocol') def update_protocol(self, context, idp_id, protocol_id, protocol): ref = self._normalize_dict(protocol) - FederationProtocol.check_immutable_params(ref) ref = self.federation_api.update_protocol(idp_id, protocol_id, protocol) return FederationProtocol.wrap_member(context, ref) @@ -466,8 +460,6 @@ class ServiceProvider(_ControllerBase): collection_name = 'service_providers' member_name = 'service_provider' - _mutable_parameters = frozenset(['auth_url', 'description', 'enabled', - 'relay_state_prefix', 'sp_url']) _public_parameters = frozenset(['auth_url', 'id', 'enabled', 'description', 'links', 'relay_state_prefix', 'sp_url']) @@ -478,7 +470,6 @@ class ServiceProvider(_ControllerBase): service_provider.setdefault('enabled', False) service_provider.setdefault('relay_state_prefix', CONF.saml.relay_state_prefix) - ServiceProvider.check_immutable_params(service_provider) sp_ref = self.federation_api.create_sp(sp_id, service_provider) response = ServiceProvider.wrap_member(context, sp_ref) return wsgi.render_response(body=response, status=('201', 'Created')) @@ -502,7 +493,6 @@ class ServiceProvider(_ControllerBase): @validation.validated(schema.service_provider_update, 'service_provider') def update_service_provider(self, context, sp_id, service_provider): service_provider = self._normalize_dict(service_provider) - ServiceProvider.check_immutable_params(service_provider) sp_ref = self.federation_api.update_sp(sp_id, service_provider) return ServiceProvider.wrap_member(context, sp_ref) diff --git a/keystone/tests/unit/test_v3_controller.py b/keystone/tests/unit/test_v3_controller.py deleted file mode 100644 index 97ed5c5309..0000000000 --- a/keystone/tests/unit/test_v3_controller.py +++ /dev/null @@ -1,54 +0,0 @@ -# Copyright 2014 CERN. -# -# 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. - -import uuid - -import six -from six.moves import range -from testtools import matchers - -from keystone.common import controller -from keystone import exception -from keystone.tests import unit - - -class V3ControllerTestCase(unit.TestCase): - """Tests for the V3Controller class.""" - - def setUp(self): - super(V3ControllerTestCase, self).setUp() - - class ControllerUnderTest(controller.V3Controller): - _mutable_parameters = frozenset(['hello', 'world']) - - self.api = ControllerUnderTest() - - def test_check_immutable_params(self): - """Pass valid parameters to the method and expect no failure.""" - ref = { - 'hello': uuid.uuid4().hex, - 'world': uuid.uuid4().hex - } - self.api.check_immutable_params(ref) - - def test_check_immutable_params_fail(self): - """Pass invalid parameter to the method and expect failure.""" - ref = {uuid.uuid4().hex: uuid.uuid4().hex for _ in range(3)} - - ex = self.assertRaises(exception.ImmutableAttributeError, - self.api.check_immutable_params, ref) - ex_msg = six.text_type(ex) - self.assertThat(ex_msg, matchers.Contains(self.api.__class__.__name__)) - for key in ref.keys(): - self.assertThat(ex_msg, matchers.Contains(key)) diff --git a/tox.ini b/tox.ini index b8d0af6f3e..a8a63cc28f 100644 --- a/tox.ini +++ b/tox.ini @@ -67,7 +67,6 @@ commands = keystone/tests/unit/test_token_provider.py \ keystone/tests/unit/test_url_middleware.py \ keystone/tests/unit/test_v2_controller.py \ - keystone/tests/unit/test_v3_controller.py \ keystone/tests/unit/test_validation.py \ keystone/tests/unit/test_wsgi.py \ keystone/tests/unit/token/test_pki_provider.py \