From c70d0c33a5977ca7208fbadd876a646cd37ffb31 Mon Sep 17 00:00:00 2001 From: Mohammed Naser Date: Fri, 7 Oct 2022 17:06:12 +0000 Subject: fix(federation): allow using numerical group names When using a numerical group name, the current codebase which relies on ast.literal_eval does not account for the value being a number. Therefore, it can be parsed as a number and fail in further steps since it will not be a list. This patch adds a test to handle that use case and refactor the code that leverages ast.literal_eval to be the same everywhere so that it adds that fix everywhere. Closes-Bug: #1992186 Change-Id: I665b7e0234650ba07e0d030a2d442d6599d0888a --- keystone/federation/utils.py | 38 +++++++++++++--------- .../tests/unit/contrib/federation/test_utils.py | 18 ++++++++++ keystone/tests/unit/mapping_fixtures.py | 6 ++++ 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/keystone/federation/utils.py b/keystone/federation/utils.py index 5f53dfbb5..f19edd00c 100644 --- a/keystone/federation/utils.py +++ b/keystone/federation/utils.py @@ -562,17 +562,31 @@ class RuleProcessor(object): LOG.debug('mapped_properties: %s', mapped_properties) return mapped_properties + def _ast_literal_eval(self, value): + # This is a workaround for the fact that ast.literal_eval handles the + # case of either a string or a list of strings, but not a potential + # list of ints. + + try: + values = ast.literal_eval(value) + # NOTE(mnaser): It's possible that the group_names_list is a + # numerical value which would successfully parse + # and not raise an exception, so we forcefully + # raise is here. + if not isinstance(values, list): + raise ValueError + except (ValueError, SyntaxError): + values = [value] + + return values + def _normalize_groups(self, identity_value): # In this case, identity_value['groups'] is a string # representation of a list, and we want a real list. This is # due to the way we do direct mapping substitutions today (see # function _update_local_mapping() ) if 'name' in identity_value['groups']: - try: - group_names_list = ast.literal_eval( - identity_value['groups']) - except (ValueError, SyntaxError): - group_names_list = [identity_value['groups']] + group_names_list = self._ast_literal_eval(identity_value['groups']) def convert_json(group): if group.startswith('JSON:'): @@ -594,11 +608,8 @@ class RuleProcessor(object): "specified.") msg = msg % {'identity_value': identity_value} raise exception.ValidationError(msg) - try: - group_names_list = ast.literal_eval( - identity_value['groups']) - except (ValueError, SyntaxError): - group_names_list = [identity_value['groups']] + group_names_list = self._ast_literal_eval( + identity_value['groups']) domain = identity_value['domain'] group_dicts = [{'name': name, 'domain': domain} for name in group_names_list] @@ -699,11 +710,8 @@ class RuleProcessor(object): # group_ids parameter contains only one element, it will be # parsed as a simple string, and not a list or the # representation of a list. - try: - group_ids.update( - ast.literal_eval(identity_value['group_ids'])) - except (ValueError, SyntaxError): - group_ids.update([identity_value['group_ids']]) + group_ids.update( + self._ast_literal_eval(identity_value['group_ids'])) if 'projects' in identity_value: projects = identity_value['projects'] diff --git a/keystone/tests/unit/contrib/federation/test_utils.py b/keystone/tests/unit/contrib/federation/test_utils.py index f9153cb09..4d9f98f2d 100644 --- a/keystone/tests/unit/contrib/federation/test_utils.py +++ b/keystone/tests/unit/contrib/federation/test_utils.py @@ -764,6 +764,24 @@ class MappingRuleEngineTests(unit.BaseTestCase): self.assertEqual('ALL USERS', mapped_properties['group_names'][0]['name']) + def test_rule_engine_groups_mapping_only_one_numerical_group(self): + """Test mapping engine when groups is explicitly set. + + If the groups list has only one group, + test if the transformation is done correctly + + """ + mapping = mapping_fixtures.MAPPING_GROUPS_WITH_EMAIL + assertion = mapping_fixtures.GROUPS_ASSERTION_ONLY_ONE_NUMERICAL_GROUP + rp = mapping_utils.RuleProcessor(FAKE_MAPPING_ID, mapping['rules']) + mapped_properties = rp.process(assertion) + self.assertIsNotNone(mapped_properties) + self.assertEqual('jsmith', mapped_properties['user']['name']) + self.assertEqual('jill@example.com', + mapped_properties['user']['email']) + self.assertEqual('1234', + mapped_properties['group_names'][0]['name']) + def test_rule_engine_group_ids_mapping_whitelist(self): """Test mapping engine when group_ids is explicitly set. diff --git a/keystone/tests/unit/mapping_fixtures.py b/keystone/tests/unit/mapping_fixtures.py index 51f1526bb..5a6dbf8c3 100644 --- a/keystone/tests/unit/mapping_fixtures.py +++ b/keystone/tests/unit/mapping_fixtures.py @@ -1735,6 +1735,12 @@ GROUPS_ASSERTION_ONLY_ONE_GROUP = { 'groups': 'ALL USERS' } +GROUPS_ASSERTION_ONLY_ONE_NUMERICAL_GROUP = { + 'userEmail': 'jill@example.com', + 'UserName': 'jsmith', + 'groups': '1234' +} + GROUPS_DOMAIN_ASSERTION = { 'openstack_user': 'bwilliams', 'openstack_user_domain': 'default', -- cgit v1.2.1