From 5c70aef2dacf801ccc147be4450e5985f545a855 Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Fri, 12 Oct 2018 10:30:04 -0700 Subject: [PATCH] Make collection_key and member_key raise if unset Instead of populating with __UNUSED__ or other silly string, make direct use of "collection_key" or "member_key" raise a ValueError if they are unset and referenced. Change-Id: Idf4f4df9d933317fff96a474cdf23d758ebdfa8c Partial-Bug: #1776504 --- keystone/api/_shared/EC2_S3_Resource.py | 3 -- keystone/api/auth.py | 3 -- keystone/api/ec2tokens.py | 3 -- keystone/api/users.py | 3 -- keystone/common/rbac_enforcer/enforcer.py | 10 ++++++- keystone/server/flask/common.py | 27 ++++++++++-------- .../tests/unit/server/test_keystone_flask.py | 28 +++++++++++++++++++ 7 files changed, 52 insertions(+), 25 deletions(-) diff --git a/keystone/api/_shared/EC2_S3_Resource.py b/keystone/api/_shared/EC2_S3_Resource.py index 36494b99e3..f7d642d868 100644 --- a/keystone/api/_shared/EC2_S3_Resource.py +++ b/keystone/api/_shared/EC2_S3_Resource.py @@ -31,9 +31,6 @@ CRED_TYPE_EC2 = 'ec2' class ResourceBase(ks_flask.ResourceBase): - collection_key = '__UNUSED__' - member_key = '__UNUSED__' - def get(self): # SPECIAL CASE: GET is not allowed, raise METHOD_NOT_ALLOWED raise exceptions.MethodNotAllowed(valid_methods=['POST']) diff --git a/keystone/api/auth.py b/keystone/api/auth.py index 568cccee2c..da63ed9f85 100644 --- a/keystone/api/auth.py +++ b/keystone/api/auth.py @@ -101,9 +101,6 @@ def _get_sso_origin_host(): class _AuthFederationWebSSOBase(ks_flask.ResourceBase): - collection_key = '__UNUSED__' - member_key = '__UNUSED__' - @staticmethod def _render_template_response(host, token_id): with open(CONF.federation.sso_callback_template) as template: diff --git a/keystone/api/ec2tokens.py b/keystone/api/ec2tokens.py index f3b23ab3b7..d10b429b9c 100644 --- a/keystone/api/ec2tokens.py +++ b/keystone/api/ec2tokens.py @@ -30,9 +30,6 @@ CRED_TYPE_EC2 = 'ec2' class EC2TokensResource(EC2_S3_Resource.ResourceBase): - collection_key = '__UNUSED__' - member_key = '__UNUSED__' - @staticmethod def _check_signature(creds_ref, credentials): signer = ec2_utils.Ec2Signer(creds_ref['secret']) diff --git a/keystone/api/users.py b/keystone/api/users.py index fed08a3c5a..bd5a125364 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -193,9 +193,6 @@ class UserResource(ks_flask.ResourceBase): class UserChangePasswordResource(ks_flask.ResourceBase): - collection_key = '__UNUSED__' - member_key = '__UNUSED__' - @ks_flask.unenforced_api def get(self, user_id): # Special case, GET is not allowed. diff --git a/keystone/common/rbac_enforcer/enforcer.py b/keystone/common/rbac_enforcer/enforcer.py index 032147d13b..71fbba02ae 100644 --- a/keystone/common/rbac_enforcer/enforcer.py +++ b/keystone/common/rbac_enforcer/enforcer.py @@ -168,7 +168,15 @@ class RBACEnforcer(object): # here. resource = flask.current_app.view_functions[ flask.request.endpoint].view_class - member_name = getattr(resource, 'member_key', None) + try: + member_name = getattr(resource, 'member_key', None) + except ValueError: + # NOTE(morgan): In the case that the ResourceBase keystone + # class is used, we raise a value error when member_key + # has not been set on the class. This is perfectly + # normal and acceptable. Set member_name to None as though + # it wasn't set. + member_name = None func = getattr( resource, 'get_member_from_driver', None) if member_name is not None and callable(func): diff --git a/keystone/server/flask/common.py b/keystone/server/flask/common.py index 520c17f44e..f48e90c403 100644 --- a/keystone/server/flask/common.py +++ b/keystone/server/flask/common.py @@ -570,10 +570,22 @@ class APIBase(object): return inst -class ResourceBase(flask_restful.Resource): +class _AttributeRaisesError(object): + # NOTE(morgan): This is a special case class that exists to effectively + # create a @classproperty style function. We use __get__ to raise the + # exception. - collection_key = None - member_key = None + def __init__(self, name): + self.__msg = 'PROGRAMMING ERROR: `self.{name}` is not set.'.format( + name=name) + + def __get__(self, instance, owner): + raise ValueError(self.__msg) + + +class ResourceBase(flask_restful.Resource): + collection_key = _AttributeRaisesError(name='collection_key') + member_key = _AttributeRaisesError(name='member_key') _public_parameters = frozenset([]) # NOTE(morgan): This must match the string on the API the resource is # registered to. @@ -582,15 +594,6 @@ class ResourceBase(flask_restful.Resource): method_decorators = [] - def __init__(self): - super(ResourceBase, self).__init__() - if self.collection_key is None: - raise ValueError('PROGRAMMING ERROR: `self.collection_key` ' - 'cannot be `None`.') - if self.member_key is None: - raise ValueError('PROGRAMMING ERROR: `self.member_key` cannot ' - 'be `None`.') - @staticmethod def _assign_unique_id(ref): ref = ref.copy() diff --git a/keystone/tests/unit/server/test_keystone_flask.py b/keystone/tests/unit/server/test_keystone_flask.py index 94a7a00d8b..147c76199e 100644 --- a/keystone/tests/unit/server/test_keystone_flask.py +++ b/keystone/tests/unit/server/test_keystone_flask.py @@ -696,3 +696,31 @@ class TestKeystoneFlaskCommon(rest.RestfulTestCase): headers={'Content-Type': 'unrecognized/content-type'}): # No exception should be raised, everything is happy. json_body.json_body_before_request() + + def test_resource_collection_key_raises_exception_if_unset(self): + class TestResource(flask_common.ResourceBase): + """A Test Resource.""" + + class TestResourceWithKey(flask_common.ResourceBase): + collection_key = uuid.uuid4().hex + + r = TestResource() + self.assertRaises(ValueError, getattr, r, 'collection_key') + + r = TestResourceWithKey() + self.assertEqual( + TestResourceWithKey.collection_key, r.collection_key) + + def test_resource_member_key_raises_exception_if_unset(self): + class TestResource(flask_common.ResourceBase): + """A Test Resource.""" + + class TestResourceWithKey(flask_common.ResourceBase): + member_key = uuid.uuid4().hex + + r = TestResource() + self.assertRaises(ValueError, getattr, r, 'member_key') + + r = TestResourceWithKey() + self.assertEqual( + TestResourceWithKey.member_key, r.member_key)