From 2e7c7c955e4a206ca8dff64031dccec932654c45 Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Thu, 19 Jan 2017 15:09:28 -0800 Subject: [PATCH] Auth Plugins pass data back via AuthHandlerResponse All of the auth plugins now pass data back to .authenticate via the AuthHandlerResponse object instead of being passed the auth_context directly. This eliminates any direct access to the auth_context by the externally loaded auth method plugins. Change-Id: I41699f11eff44ae493f05ad23503dd0232de1609 bp: per-user-auth-plugin-reqs --- keystone/auth/controllers.py | 28 +++++++-- keystone/auth/plugins/base.py | 17 ++--- keystone/auth/plugins/external.py | 14 +++-- keystone/auth/plugins/mapped.py | 82 +++++++++++++------------ keystone/auth/plugins/oauth1.py | 12 ++-- keystone/auth/plugins/password.py | 8 ++- keystone/auth/plugins/token.py | 31 ++++++---- keystone/auth/plugins/totp.py | 8 ++- keystone/tests/unit/test_auth_plugin.py | 15 +++-- 9 files changed, 125 insertions(+), 90 deletions(-) diff --git a/keystone/auth/controllers.py b/keystone/auth/controllers.py index b879eb7d5..7a2a3431a 100644 --- a/keystone/auth/controllers.py +++ b/keystone/auth/controllers.py @@ -128,6 +128,18 @@ class AuthContext(dict): raise exception.Unauthorized(msg) return super(AuthContext, self).__setitem__(key, val) + def update(self, E=None, **F): + """Override update to prevent conflicting values.""" + # NOTE(notmorgan): This will not be nearly as performant as the + # use of the built-in "update" method on the dict, however, the + # volume of data being changed here is very minimal in most cases + # and should not see a significant impact by iterating instead of + # explicit setting of values. + update_dicts = (E or {}, F or {}) + for d in update_dicts: + for key, val in d.items(): + self[key] = val + @dependency.requires('resource_api', 'trust_api') class AuthInfo(object): @@ -546,13 +558,15 @@ class Auth(controller.V3Controller): try: external = get_auth_method('external') resp = external.authenticate(request, - auth_info, - auth_context) + auth_info) if resp and resp.status: # NOTE(notmorgan): ``external`` plugin cannot be multi-step # it is either a plain success/fail. auth_context.setdefault( 'method_names', []).insert(0, 'external') + # NOTE(notmorgan): All updates to auth_context is handled + # here in the .authenticate method. + auth_context.update(resp.response_data or {}) except exception.AuthMethodNotSupported: # This will happen there is no 'external' plugin registered @@ -573,12 +587,18 @@ class Auth(controller.V3Controller): for method_name in auth_info.get_method_names(): method = get_auth_method(method_name) resp = method.authenticate(request, - auth_info.get_method_data(method_name), - auth_context) + auth_info.get_method_data(method_name)) if resp: if resp.status: auth_context.setdefault( 'method_names', []).insert(0, method_name) + # NOTE(notmorgan): All updates to auth_context is handled + # here in the .authenticate method. If the auth attempt was + # not successful do not update the auth_context + resp_method_names = resp.response_data.pop( + 'method_names', []) + auth_context['method_names'].extend(resp_method_names) + auth_context.update(resp.response_data or {}) elif resp.response_body: auth_response['methods'].append(method_name) auth_response[method_name] = resp.response_body diff --git a/keystone/auth/plugins/base.py b/keystone/auth/plugins/base.py index 7d9859939..374a813a9 100644 --- a/keystone/auth/plugins/base.py +++ b/keystone/auth/plugins/base.py @@ -20,8 +20,8 @@ import six from keystone import exception -AuthHandlerResponse = collections.namedtuple('AuthHandlerResponse', - 'status, response_body') +AuthHandlerResponse = collections.namedtuple( + 'AuthHandlerResponse', 'status, response_body, response_data') @six.add_metaclass(abc.ABCMeta) @@ -32,7 +32,7 @@ class AuthMethodHandler(object): pass @abc.abstractmethod - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): """Authenticate user and return an authentication context. :param request: context of an authentication request @@ -40,21 +40,16 @@ class AuthMethodHandler(object): :param auth_payload: the payload content of the authentication request for a given method :type auth_payload: dict - :param auth_context: user authentication context, a dictionary shared - by all plugins. It contains "method_names" and - "extras" by default. "method_names" is a list and - "extras" is a dictionary. - :type auth_context: oslo_context.RequestContext - If successful, plugin must set ``user_id`` in ``auth_context``. + If successful, plugin must set ``user_id`` in ``response_data``. ``method_name`` is used to convey any additional authentication methods in case authentication is for re-scoping. For example, if the authentication is for re-scoping, plugin must append the previous method names into ``method_names``; NOTE: This behavior is exclusive to the re-scope type action. Also, plugin may add any additional information into ``extras``. Anything in ``extras`` will be conveyed in - the token's ``extras`` attribute. Here's an example of ``auth_context`` - on successful authentication:: + the token's ``extras`` attribute. Here's an example of + ``response_data`` on successful authentication:: { "extras": {}, diff --git a/keystone/auth/plugins/external.py b/keystone/auth/plugins/external.py index 738eb2db4..3f856bf71 100644 --- a/keystone/auth/plugins/external.py +++ b/keystone/auth/plugins/external.py @@ -30,12 +30,13 @@ CONF = keystone.conf.CONF @six.add_metaclass(abc.ABCMeta) class Base(base.AuthMethodHandler): - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload,): """Use REMOTE_USER to look up the user in the identity backend. - auth_context is an in-out variable that will be updated with the - user_id from the actual user from the REMOTE_USER env variable. + The user_id from the actual user from the REMOTE_USER env variable is + placed in the response_data. """ + response_data = {} if not request.remote_user: msg = _('No authenticated user') raise exception.Unauthorized(msg) @@ -46,13 +47,14 @@ class Base(base.AuthMethodHandler): msg = _('Unable to lookup user %s') % request.remote_user raise exception.Unauthorized(msg) - auth_context['user_id'] = user_ref['id'] + response_data['user_id'] = user_ref['id'] auth_type = (request.auth_type or '').lower() if 'kerberos' in CONF.token.bind and auth_type == 'negotiate': - auth_context['bind']['kerberos'] = user_ref['name'] + response_data.setdefault('bind', {})['kerberos'] = user_ref['name'] - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) @abc.abstractmethod def _authenticate(self, request): diff --git a/keystone/auth/plugins/mapped.py b/keystone/auth/plugins/mapped.py index 7cdbd9f79..3467d2c02 100644 --- a/keystone/auth/plugins/mapped.py +++ b/keystone/auth/plugins/mapped.py @@ -42,42 +42,39 @@ class Mapped(base.AuthMethodHandler): return token_model.KeystoneToken(token_id=token_id, token_data=response) - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): """Authenticate mapped user and set an authentication context. :param request: keystone's request context :param auth_payload: the content of the authentication for a given method - :param auth_context: user authentication context, a dictionary - shared by all plugins. - In addition to ``user_id`` in ``auth_context``, this plugin sets + In addition to ``user_id`` in ``response_data``, this plugin sets ``group_ids``, ``OS-FEDERATION:identity_provider`` and ``OS-FEDERATION:protocol`` """ if 'id' in auth_payload: token_ref = self._get_token_ref(auth_payload) - handle_scoped_token(request, - auth_context, - token_ref, - self.federation_api, - self.identity_api) + response_data = handle_scoped_token(request, + token_ref, + self.federation_api, + self.identity_api) else: - handle_unscoped_token(request, - auth_payload, - auth_context, - self.resource_api, - self.federation_api, - self.identity_api, - self.assignment_api, - self.role_api) + response_data = handle_unscoped_token(request, + auth_payload, + self.resource_api, + self.federation_api, + self.identity_api, + self.assignment_api, + self.role_api) - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) -def handle_scoped_token(request, auth_context, token_ref, - federation_api, identity_api): +def handle_scoped_token(request, token_ref, federation_api, identity_api): + response_data = {} utils.validate_expiration(token_ref) token_audit_id = token_ref.audit_id identity_provider = token_ref.federation_idp_id @@ -105,15 +102,16 @@ def handle_scoped_token(request, auth_context, token_ref, else: send_notification(taxonomy.OUTCOME_SUCCESS) - auth_context['user_id'] = user_id - auth_context['group_ids'] = group_ids - auth_context[federation_constants.IDENTITY_PROVIDER] = identity_provider - auth_context[federation_constants.PROTOCOL] = protocol + response_data['user_id'] = user_id + response_data['group_ids'] = group_ids + response_data[federation_constants.IDENTITY_PROVIDER] = identity_provider + response_data[federation_constants.PROTOCOL] = protocol + + return response_data -def handle_unscoped_token(request, auth_payload, auth_context, - resource_api, federation_api, identity_api, - assignment_api, role_api): +def handle_unscoped_token(request, auth_payload, resource_api, federation_api, + identity_api, assignment_api, role_api): def validate_shadow_mapping(shadow_projects, existing_roles, idp_domain_id, idp_id): @@ -185,18 +183,23 @@ def handle_unscoped_token(request, auth_payload, auth_context, def is_ephemeral_user(mapped_properties): return mapped_properties['user']['type'] == utils.UserType.EPHEMERAL - def build_ephemeral_user_context(auth_context, user, mapped_properties, + def build_ephemeral_user_context(user, mapped_properties, identity_provider, protocol): - auth_context['user_id'] = user['id'] - auth_context['group_ids'] = mapped_properties['group_ids'] - auth_context[federation_constants.IDENTITY_PROVIDER] = ( - identity_provider) - auth_context[federation_constants.PROTOCOL] = protocol + resp = {} + resp['user_id'] = user['id'] + resp['group_ids'] = mapped_properties['group_ids'] + resp[federation_constants.IDENTITY_PROVIDER] = identity_provider + resp[federation_constants.PROTOCOL] = protocol - def build_local_user_context(auth_context, mapped_properties): + return resp + + def build_local_user_context(mapped_properties): + resp = {} user_info = auth_plugins.UserAuthInfo.create(mapped_properties, METHOD_NAME) - auth_context['user_id'] = user_info.user_id + resp['user_id'] = user_info.user_id + + return resp assertion = extract_assertion_data(request) identity_provider = auth_payload['identity_provider'] @@ -260,11 +263,10 @@ def handle_unscoped_token(request, auth_payload, auth_context, user_id = user['id'] group_ids = mapped_properties['group_ids'] - build_ephemeral_user_context(auth_context, user, - mapped_properties, - identity_provider, protocol) + response_data = build_ephemeral_user_context( + user, mapped_properties, identity_provider, protocol) else: - build_local_user_context(auth_context, mapped_properties) + response_data = build_local_user_context(mapped_properties) except Exception: # NOTE(topol): Diaper defense to catch any exception, so we can @@ -287,6 +289,8 @@ def handle_unscoped_token(request, auth_payload, auth_context, protocol, token_id, outcome) + return response_data + def extract_assertion_data(request): assertion = dict(utils.get_assertion_params_from_env(request)) diff --git a/keystone/auth/plugins/oauth1.py b/keystone/auth/plugins/oauth1.py index 18e1d9068..ec936e070 100644 --- a/keystone/auth/plugins/oauth1.py +++ b/keystone/auth/plugins/oauth1.py @@ -25,8 +25,9 @@ from keystone.oauth1 import validator @dependency.requires('oauth_api') class OAuth(base.AuthMethodHandler): - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): """Turn a signed request with an access key into a keystone token.""" + response_data = {} oauth_headers = oauth.get_oauth_headers(request.headers) access_token_id = oauth_headers.get('oauth_token') @@ -59,8 +60,9 @@ class OAuth(base.AuthMethodHandler): if not result: msg = _('Could not validate the access token') raise exception.Unauthorized(msg) - auth_context['user_id'] = acc_token['authorizing_user_id'] - auth_context['access_token_id'] = access_token_id - auth_context['project_id'] = acc_token['project_id'] + response_data['user_id'] = acc_token['authorizing_user_id'] + response_data['access_token_id'] = access_token_id + response_data['project_id'] = acc_token['project_id'] - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) diff --git a/keystone/auth/plugins/password.py b/keystone/auth/plugins/password.py index 20e51ae09..33d2039f2 100644 --- a/keystone/auth/plugins/password.py +++ b/keystone/auth/plugins/password.py @@ -25,8 +25,9 @@ METHOD_NAME = 'password' @dependency.requires('identity_api') class Password(base.AuthMethodHandler): - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): """Try to authenticate against the identity backend.""" + response_data = {} user_info = auth_plugins.UserAuthInfo.create(auth_payload, METHOD_NAME) try: @@ -39,6 +40,7 @@ class Password(base.AuthMethodHandler): msg = _('Invalid username or password') raise exception.Unauthorized(msg) - auth_context['user_id'] = user_info.user_id + response_data['user_id'] = user_info.user_id - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) diff --git a/keystone/auth/plugins/token.py b/keystone/auth/plugins/token.py index 22fcaae4d..a4e4448d1 100644 --- a/keystone/auth/plugins/token.py +++ b/keystone/auth/plugins/token.py @@ -39,22 +39,24 @@ class Token(base.AuthMethodHandler): return token_model.KeystoneToken(token_id=token_id, token_data=response) - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): if 'id' not in auth_payload: raise exception.ValidationError(attribute='id', target='token') token_ref = self._get_token_ref(auth_payload) if token_ref.is_federated_user and self.federation_api: - mapped.handle_scoped_token( - request, auth_context, token_ref, - self.federation_api, self.identity_api) + response_data = mapped.handle_scoped_token( + request, token_ref, self.federation_api, self.identity_api) else: - token_authenticate(request, auth_context, token_ref) + response_data = token_authenticate(request, + token_ref) - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) -def token_authenticate(request, user_context, token_ref): +def token_authenticate(request, token_ref): + response_data = {} try: # Do not allow tokens used for delegation to @@ -94,17 +96,20 @@ def token_authenticate(request, user_context, token_ref): # issued prior to audit id existing, the chain is not tracked. token_audit_id = None - user_context.setdefault('expires_at', token_ref.expires) - user_context['audit_id'] = token_audit_id - user_context.setdefault('user_id', token_ref.user_id) + response_data.setdefault('expires_at', token_ref.expires) + response_data['audit_id'] = token_audit_id + response_data.setdefault('user_id', token_ref.user_id) # TODO(morganfainberg: determine if token 'extras' can be removed - # from the user_context - user_context['extras'].update(token_ref.get('extras', {})) + # from the response_data + response_data.setdefault('extras', {}).update( + token_ref.get('extras', {})) # NOTE(notmorgan): The Token auth method is *very* special and sets the # previous values to the method_names. This is because it can be used # for re-scoping and we want to maintain the values. Most # AuthMethodHandlers do no such thing and this is not required. - user_context.setdefault('method_names', []).extend(token_ref.methods) + response_data.setdefault('method_names', []).extend(token_ref.methods) + + return response_data except AssertionError as e: LOG.error(six.text_type(e)) diff --git a/keystone/auth/plugins/totp.py b/keystone/auth/plugins/totp.py index dfffee7b6..9627411da 100644 --- a/keystone/auth/plugins/totp.py +++ b/keystone/auth/plugins/totp.py @@ -68,8 +68,9 @@ def _generate_totp_passcode(secret): @dependency.requires('credential_api') class TOTP(base.AuthMethodHandler): - def authenticate(self, request, auth_payload, auth_context): + def authenticate(self, request, auth_payload): """Try to authenticate using TOTP.""" + response_data = {} user_info = plugins.TOTPUserInfo.create(auth_payload, METHOD_NAME) auth_passcode = auth_payload.get('user').get('passcode') @@ -96,6 +97,7 @@ class TOTP(base.AuthMethodHandler): msg = _('Invalid username or TOTP passcode') raise exception.Unauthorized(msg) - auth_context['user_id'] = user_info.user_id + response_data['user_id'] = user_info.user_id - return base.AuthHandlerResponse(status=True, response_body=None) + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) diff --git a/keystone/tests/unit/test_auth_plugin.py b/keystone/tests/unit/test_auth_plugin.py index 4d94e2d5c..e46f96f76 100644 --- a/keystone/tests/unit/test_auth_plugin.py +++ b/keystone/tests/unit/test_auth_plugin.py @@ -34,18 +34,21 @@ DEMO_USER_ID = uuid.uuid4().hex class SimpleChallengeResponse(base.AuthMethodHandler): - def authenticate(self, context, auth_payload, auth_context): + def authenticate(self, context, auth_payload): + response_data = {} if 'response' in auth_payload: if auth_payload['response'] != EXPECTED_RESPONSE: raise exception.Unauthorized('Wrong answer') - auth_context['user_id'] = DEMO_USER_ID - return base.AuthHandlerResponse(status=True, response_body=None) + response_data['user_id'] = DEMO_USER_ID + return base.AuthHandlerResponse(status=True, response_body=None, + response_data=response_data) else: return base.AuthHandlerResponse( status=False, response_body={ - "challenge": "What's the name of your high school?"}) + "challenge": "What's the name of your high school?"}, + response_data=None) class TestAuthPlugin(unit.SQLDriverOverrides, unit.TestCase): @@ -156,7 +159,7 @@ class TestMapped(unit.TestCase): user_id=uuid.uuid4().hex) self.api.authenticate(request, auth_info, auth_context) # make sure Mapped plugin got invoked with the correct payload - ((context, auth_payload, auth_context), + ((context, auth_payload), kwargs) = authenticate.call_args self.assertEqual(method_name, auth_payload['protocol']) @@ -182,7 +185,7 @@ class TestMapped(unit.TestCase): request = self.make_request(environ={'REMOTE_USER': 'foo@idp.com'}) self.api.authenticate(request, auth_info, auth_context) # make sure Mapped plugin got invoked with the correct payload - ((context, auth_payload, auth_context), + ((context, auth_payload), kwargs) = authenticate.call_args self.assertEqual(method_name, auth_payload['protocol'])