From 1b8d0589ce79cb141326d68c12c9f2a37951e938 Mon Sep 17 00:00:00 2001 From: Lance Bragstad Date: Wed, 14 Feb 2018 16:00:57 +0000 Subject: [PATCH] Remove needs_persistence property from token providers Since the sql token storage mechanism was removed in Rocky, we no longer need hooks in the token Manager to determine if a token needs to be retrieved from or written to a backend somewhere. Instead, token providers will need to handle storage requirements if they need them. This will result in a cleaner token provider interface. Change-Id: Icc095987d41e9c08de2f34dc657b08b98bd944e4 --- .../tests/unit/token/test_fernet_provider.py | 3 - keystone/token/provider.py | 56 ---------- keystone/token/providers/base.py | 10 -- keystone/token/providers/common.py | 105 ++++++------------ keystone/token/providers/fernet/core.py | 4 - ...en-provider-refactor-a3a64146807daf36.yaml | 7 ++ 6 files changed, 43 insertions(+), 142 deletions(-) create mode 100644 releasenotes/notes/token-provider-refactor-a3a64146807daf36.yaml diff --git a/keystone/tests/unit/token/test_fernet_provider.py b/keystone/tests/unit/token/test_fernet_provider.py index da7e820230..b0183264d1 100644 --- a/keystone/tests/unit/token/test_fernet_provider.py +++ b/keystone/tests/unit/token/test_fernet_provider.py @@ -54,9 +54,6 @@ class TestFernetTokenProvider(unit.TestCase): def test_supports_bind_authentication_returns_false(self): self.assertFalse(self.provider._supports_bind_authentication) - def test_needs_persistence_returns_false(self): - self.assertFalse(self.provider.needs_persistence()) - def test_invalid_v3_token_raises_token_not_found(self): token_id = uuid.uuid4().hex e = self.assertRaises( diff --git a/keystone/token/provider.py b/keystone/token/provider.py index 905eaa058a..9a58b7d478 100644 --- a/keystone/token/provider.py +++ b/keystone/token/provider.py @@ -15,11 +15,9 @@ """Token provider interface.""" import datetime -import sys from oslo_log import log from oslo_utils import timeutils -import six from keystone.common import cache from keystone.common import manager @@ -62,7 +60,6 @@ class Manager(manager.Manager): V3 = V3 VERSIONS = VERSIONS - _persistence_manager = None def __init__(self): super(Manager, self).__init__(CONF.token.provider) @@ -99,35 +96,6 @@ class Manager(manager.Manager): notifications.register_event_callback(event, resource_type, callback_fns) - @property - def _needs_persistence(self): - return self.driver.needs_persistence() - - @property - def _persistence(self): - # NOTE(morganfainberg): This should not be handled via __init__ to - # avoid dependency injection oddities circular dependencies (where - # the provider manager requires the token persistence manager, which - # requires the token provider manager). - if self._persistence_manager is None: - self._persistence_manager = self._token_persistence_manager - return self._persistence_manager - - def _create_token(self, token_id, token_data): - try: - if isinstance(token_data['expires'], six.string_types): - token_data['expires'] = timeutils.normalize_time( - timeutils.parse_isotime(token_data['expires'])) - self._persistence.create_token(token_id, token_data) - except Exception: - exc_info = sys.exc_info() - # an identical token may have been created already. - # if so, return the token_data as it is also identical - try: - self._persistence.get_token(token_id) - except exception.TokenNotFound: - six.reraise(*exc_info) - def check_revocation_v3(self, token): try: token_data = token['token'] @@ -144,15 +112,6 @@ class Manager(manager.Manager): raise exception.TokenNotFound(_('No token in the request')) try: - # NOTE(lbragstad): Only go to persistent storage if we have a token - # to fetch from the backend (the driver persists the token). - # Otherwise the information about the token must be in the token - # id. - if self._needs_persistence: - token_ref = self._persistence.get_token(token_id) - # Overload the token_id variable to be a token reference - # instead. - token_id = token_ref token_ref = self._validate_token(token_id) self._is_valid_token(token_ref, window_seconds=window_seconds) return token_ref @@ -207,18 +166,6 @@ class Manager(manager.Manager): app_cred_id=app_cred_id, include_catalog=include_catalog, parent_audit_id=parent_audit_id) - if self._needs_persistence: - data = dict(key=token_id, - id=token_id, - expires=token_data['token']['expires_at'], - user=token_data['token']['user'], - tenant=token_data['token'].get('project'), - is_domain=is_domain, - token_data=token_data, - trust_id=trust['id'] if trust else None, - token_version=self.V3) - self._create_token(token_id, data) - if CONF.token.cache_on_issue: # NOTE(amakarov): here and above TOKENS_REGION is to be passed # to serve as required positional "self" argument. It's ignored, @@ -255,9 +202,6 @@ class Manager(manager.Manager): else: PROVIDERS.revoke_api.revoke_by_audit_id(token_ref.audit_id) - if CONF.token.revoke_by_id and self._needs_persistence: - self._persistence.delete_token(token_id=token_id) - # FIXME(morganfainberg): Does this cache actually need to be # invalidated? We maintain a cached revocation list, which should be # consulted before accepting a token as valid. For now we will diff --git a/keystone/token/providers/base.py b/keystone/token/providers/base.py index c3ecdfa4c4..5f62afb23b 100644 --- a/keystone/token/providers/base.py +++ b/keystone/token/providers/base.py @@ -23,16 +23,6 @@ from keystone import exception class Provider(object): """Interface description for a Token provider.""" - @abc.abstractmethod - def needs_persistence(self): - """Determine if the token should be persisted. - - If the token provider requires that the token be persisted to a - backend this should return True, otherwise return False. - - """ - raise exception.NotImplemented() # pragma: no cover - @abc.abstractmethod def get_token_version(self, token_data): """Return the version of the given token data. diff --git a/keystone/token/providers/common.py b/keystone/token/providers/common.py index 64f14dd583..9a0b7445d3 100644 --- a/keystone/token/providers/common.py +++ b/keystone/token/providers/common.py @@ -641,78 +641,45 @@ class BaseProvider(provider_api.ProviderAPIMixin, base.Provider): return token_ref def validate_token(self, token_id): - if self.needs_persistence(): - token_ref = token_id - token_data = token_ref.get('token_data') - user_id = token_ref['user_id'] - methods = token_data['token']['methods'] - bind = token_data['token'].get('bind') - issued_at = token_data['token']['issued_at'] - expires_at = token_data['token']['expires_at'] - audit_ids = token_data['token'].get('audit_ids') - system = token_data['token'].get('system', {}).get('all') - if system: - system = 'all' - domain_id = token_data['token'].get('domain', {}).get('id') - project_id = token_data['token'].get('project', {}).get('id') - access_token = None - if token_data['token'].get('OS-OAUTH1'): - access_token = { - 'id': token_data['token'].get('OS-OAUTH1', {}).get( - 'access_token_id' - ), - 'consumer_id': token_data['token'].get( - 'OS-OAUTH1', {} - ).get('consumer_id') - } - trust_ref = None - trust_id = token_ref.get('trust_id') - if trust_id: - trust_ref = PROVIDERS.trust_api.get_trust(trust_id) - token_dict = None - if token_data['token']['user'].get( - federation_constants.FEDERATION): - token_dict = {'user': token_ref['user']} - else: - try: - (user_id, methods, audit_ids, system, domain_id, - project_id, trust_id, federated_info, access_token_id, - issued_at, expires_at) = ( - self.token_formatter.validate_token(token_id)) - except exception.ValidationError as e: - raise exception.TokenNotFound(e) + try: + (user_id, methods, audit_ids, system, domain_id, + project_id, trust_id, federated_info, access_token_id, + issued_at, expires_at) = ( + self.token_formatter.validate_token(token_id)) + except exception.ValidationError as e: + raise exception.TokenNotFound(e) - bind = None - token_dict = None - trust_ref = None - if federated_info: - # NOTE(lbragstad): We need to rebuild information about the - # federated token as well as the federated token roles. This is - # because when we validate a non-persistent token, we don't - # have a token reference to pull the federated token - # information out of. As a result, we have to extract it from - # the token itself and rebuild the federated context. These - # private methods currently live in the - # keystone.token.providers.fernet.Provider() class. - token_dict = self._rebuild_federated_info( - federated_info, user_id + bind = None + token_dict = None + trust_ref = None + if federated_info: + # NOTE(lbragstad): We need to rebuild information about the + # federated token as well as the federated token roles. This is + # because when we validate a non-persistent token, we don't + # have a token reference to pull the federated token + # information out of. As a result, we have to extract it from + # the token itself and rebuild the federated context. These + # private methods currently live in the + # keystone.token.providers.fernet.Provider() class. + token_dict = self._rebuild_federated_info( + federated_info, user_id + ) + if project_id or domain_id: + self._rebuild_federated_token_roles( + token_dict, + federated_info, + user_id, + project_id, + domain_id ) - if project_id or domain_id: - self._rebuild_federated_token_roles( - token_dict, - federated_info, - user_id, - project_id, - domain_id - ) - if trust_id: - trust_ref = PROVIDERS.trust_api.get_trust(trust_id) + if trust_id: + trust_ref = PROVIDERS.trust_api.get_trust(trust_id) - access_token = None - if access_token_id: - access_token = PROVIDERS.oauth_api.get_access_token( - access_token_id - ) + access_token = None + if access_token_id: + access_token = PROVIDERS.oauth_api.get_access_token( + access_token_id + ) return self.v3_token_data_helper.get_token_data( user_id, diff --git a/keystone/token/providers/fernet/core.py b/keystone/token/providers/fernet/core.py index 33964cfc03..e47c959ce8 100644 --- a/keystone/token/providers/fernet/core.py +++ b/keystone/token/providers/fernet/core.py @@ -44,10 +44,6 @@ class Provider(common.BaseProvider): self.token_formatter = tf.TokenFormatter() - def needs_persistence(self): - """Should the token be written to a backend.""" - return False - def issue_token(self, *args, **kwargs): token_id, token_data = super(Provider, self).issue_token( *args, **kwargs) diff --git a/releasenotes/notes/token-provider-refactor-a3a64146807daf36.yaml b/releasenotes/notes/token-provider-refactor-a3a64146807daf36.yaml new file mode 100644 index 0000000000..6180b1a7bc --- /dev/null +++ b/releasenotes/notes/token-provider-refactor-a3a64146807daf36.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - | + The token provider API has removed the ``needs_persistence`` property from + the abstract interface. Token providers are expected to handle persistence + requirement if needed. This will require out-of-tree token providers to + remove the unused property and handle token storage.