From 2942eda8e0b2a37203e19eb9b1fe1704a91f2538 Mon Sep 17 00:00:00 2001 From: fgruenbauer Date: Mon, 12 Aug 2024 07:55:17 +0200 Subject: [PATCH] keycloak_user_federation: add mapper removal (#8695) * add unwanted mapper removal * check for mapper updates in already fetched data to remove unnecessary API calls * added mock answers and updated request count to match the added delete and fetch after_comp calls * fix sanity issues * add changelog fragment * removed automatic field numbering from format * replace filter expression with list comprehension Co-authored-by: Felix Fontein * add module name, link to issue and link to PR to changelog Co-authored-by: Felix Fontein * Use list comprehension. --------- Co-authored-by: Felix Fontein --- ...eycloak_user_federation-mapper-removal.yml | 2 + plugins/modules/keycloak_user_federation.py | 77 ++++++++++++------- .../modules/test_keycloak_user_federation.py | 54 +++++++++++-- 3 files changed, 98 insertions(+), 35 deletions(-) create mode 100644 changelogs/fragments/8695-keycloak_user_federation-mapper-removal.yml diff --git a/changelogs/fragments/8695-keycloak_user_federation-mapper-removal.yml b/changelogs/fragments/8695-keycloak_user_federation-mapper-removal.yml new file mode 100644 index 0000000000..b518d59e36 --- /dev/null +++ b/changelogs/fragments/8695-keycloak_user_federation-mapper-removal.yml @@ -0,0 +1,2 @@ +bugfixes: + - keycloak_user_federation - remove existing user federation mappers if they are not present in the federation configuration and will not be updated (https://github.com/ansible-collections/community.general/issues/7169, https://github.com/ansible-collections/community.general/pull/8695). \ No newline at end of file diff --git a/plugins/modules/keycloak_user_federation.py b/plugins/modules/keycloak_user_federation.py index f87ef936ce..00f407ec03 100644 --- a/plugins/modules/keycloak_user_federation.py +++ b/plugins/modules/keycloak_user_federation.py @@ -892,11 +892,11 @@ def main(): if cid is None: old_mapper = {} elif change.get('id') is not None: - old_mapper = kc.get_component(change['id'], realm) + old_mapper = next((before_mapper for before_mapper in before_mapper.get('mappers', []) if before_mapper["id"] == change['id']), None) if old_mapper is None: old_mapper = {} else: - found = kc.get_components(urlencode(dict(parent=cid, name=change['name'])), realm) + found = [before_mapper for before_mapper in before_comp.get('mappers', []) if before_mapper['name'] == change['name']] if len(found) > 1: module.fail_json(msg='Found multiple mappers with name `{name}`. Cannot continue.'.format(name=change['name'])) if len(found) == 1: @@ -905,10 +905,10 @@ def main(): old_mapper = {} new_mapper = old_mapper.copy() new_mapper.update(change) - if new_mapper != old_mapper: - if changeset.get('mappers') is None: - changeset['mappers'] = list() - changeset['mappers'].append(new_mapper) + # changeset contains all desired mappers: those existing, to update or to create + if changeset.get('mappers') is None: + changeset['mappers'] = list() + changeset['mappers'].append(new_mapper) # Prepare the desired values using the existing values (non-existence results in a dict that is save to use as a basis) desired_comp = before_comp.copy() @@ -931,42 +931,51 @@ def main(): # Process a creation result['changed'] = True - if module._diff: - result['diff'] = dict(before='', after=sanitize(desired_comp)) - if module.check_mode: + if module._diff: + result['diff'] = dict(before='', after=sanitize(desired_comp)) module.exit_json(**result) # create it - desired_comp = desired_comp.copy() - updated_mappers = desired_comp.pop('mappers', []) + desired_mappers = desired_comp.pop('mappers', []) after_comp = kc.create_component(desired_comp, realm) - cid = after_comp['id'] + updated_mappers = [] + # when creating a user federation, keycloak automatically creates default mappers + default_mappers = kc.get_components(urlencode(dict(parent=cid)), realm) - for mapper in updated_mappers: - found = kc.get_components(urlencode(dict(parent=cid, name=mapper['name'])), realm) + # create new mappers or update existing default mappers + for desired_mapper in desired_mappers: + found = [default_mapper for default_mapper in default_mappers if default_mapper['name'] == desired_mapper['name']] if len(found) > 1: - module.fail_json(msg='Found multiple mappers with name `{name}`. Cannot continue.'.format(name=mapper['name'])) + module.fail_json(msg='Found multiple mappers with name `{name}`. Cannot continue.'.format(name=desired_mapper['name'])) if len(found) == 1: old_mapper = found[0] else: old_mapper = {} new_mapper = old_mapper.copy() - new_mapper.update(mapper) + new_mapper.update(desired_mapper) if new_mapper.get('id') is not None: kc.update_component(new_mapper, realm) + updated_mappers.append(new_mapper) else: if new_mapper.get('parentId') is None: - new_mapper['parentId'] = after_comp['id'] - mapper = kc.create_component(new_mapper, realm) + new_mapper['parentId'] = cid + updated_mappers.append(kc.create_component(new_mapper, realm)) - after_comp['mappers'] = updated_mappers + # we remove all unwanted default mappers + # we use ids so we dont accidently remove one of the previously updated default mapper + for default_mapper in default_mappers: + if not default_mapper['id'] in [x['id'] for x in updated_mappers]: + kc.delete_component(default_mapper['id'], realm) + + after_comp['mappers'] = kc.get_components(urlencode(dict(parent=cid)), realm) + if module._diff: + result['diff'] = dict(before='', after=sanitize(after_comp)) result['end_state'] = sanitize(after_comp) - - result['msg'] = "User federation {id} has been created".format(id=after_comp['id']) + result['msg'] = "User federation {id} has been created".format(id=cid) module.exit_json(**result) else: @@ -990,22 +999,32 @@ def main(): module.exit_json(**result) # do the update - desired_comp = desired_comp.copy() - updated_mappers = desired_comp.pop('mappers', []) + desired_mappers = desired_comp.pop('mappers', []) kc.update_component(desired_comp, realm) - after_comp = kc.get_component(cid, realm) - for mapper in updated_mappers: + for before_mapper in before_comp.get('mappers', []): + # remove unwanted existing mappers that will not be updated + if not before_mapper['id'] in [x['id'] for x in desired_mappers]: + kc.delete_component(before_mapper['id'], realm) + + for mapper in desired_mappers: + if mapper in before_comp.get('mappers', []): + continue if mapper.get('id') is not None: kc.update_component(mapper, realm) else: if mapper.get('parentId') is None: mapper['parentId'] = desired_comp['id'] - mapper = kc.create_component(mapper, realm) - - after_comp['mappers'] = updated_mappers - result['end_state'] = sanitize(after_comp) + kc.create_component(mapper, realm) + after_comp = kc.get_component(cid, realm) + after_comp['mappers'] = kc.get_components(urlencode(dict(parent=cid)), realm) + after_comp_sanitized = sanitize(after_comp) + before_comp_sanitized = sanitize(before_comp) + result['end_state'] = after_comp_sanitized + if module._diff: + result['diff'] = dict(before=before_comp_sanitized, after=after_comp_sanitized) + result['changed'] = before_comp_sanitized != after_comp_sanitized result['msg'] = "User federation {id} has been updated".format(id=cid) module.exit_json(**result) diff --git a/tests/unit/plugins/modules/test_keycloak_user_federation.py b/tests/unit/plugins/modules/test_keycloak_user_federation.py index 523ef9f210..81fd65e108 100644 --- a/tests/unit/plugins/modules/test_keycloak_user_federation.py +++ b/tests/unit/plugins/modules/test_keycloak_user_federation.py @@ -144,8 +144,9 @@ class TestKeycloakUserFederation(ModuleTestCase): } } ] + # get before_comp, get default_mapper, get after_mapper return_value_components_get = [ - [], [] + [], [], [] ] changed = True @@ -159,7 +160,7 @@ class TestKeycloakUserFederation(ModuleTestCase): with self.assertRaises(AnsibleExitJson) as exec_info: self.module.main() - self.assertEqual(len(mock_get_components.mock_calls), 1) + self.assertEqual(len(mock_get_components.mock_calls), 3) self.assertEqual(len(mock_get_component.mock_calls), 0) self.assertEqual(len(mock_create_component.mock_calls), 1) self.assertEqual(len(mock_update_component.mock_calls), 0) @@ -228,6 +229,7 @@ class TestKeycloakUserFederation(ModuleTestCase): } } ], + [], [] ] return_value_component_get = [ @@ -281,7 +283,7 @@ class TestKeycloakUserFederation(ModuleTestCase): with self.assertRaises(AnsibleExitJson) as exec_info: self.module.main() - self.assertEqual(len(mock_get_components.mock_calls), 2) + self.assertEqual(len(mock_get_components.mock_calls), 3) self.assertEqual(len(mock_get_component.mock_calls), 1) self.assertEqual(len(mock_create_component.mock_calls), 0) self.assertEqual(len(mock_update_component.mock_calls), 1) @@ -344,7 +346,47 @@ class TestKeycloakUserFederation(ModuleTestCase): ] } return_value_components_get = [ - [], [] + [], + # exemplary default mapper created by keylocak + [ + { + "config": { + "always.read.value.from.ldap": "false", + "is.mandatory.in.ldap": "false", + "ldap.attribute": "mail", + "read.only": "true", + "user.model.attribute": "email" + }, + "id": "77e1763f-c51a-4286-bade-75577d64803c", + "name": "email", + "parentId": "e5f48aa3-b56b-4983-a8ad-2c7b8b5e77cb", + "providerId": "user-attribute-ldap-mapper", + "providerType": "org.keycloak.storage.ldap.mappers.LDAPStorageMapper" + }, + ], + [ + { + "id": "2dfadafd-8b34-495f-a98b-153e71a22311", + "name": "full name", + "providerId": "full-name-ldap-mapper", + "providerType": "org.keycloak.storage.ldap.mappers.LDAPStorageMapper", + "parentId": "eb691537-b73c-4cd8-b481-6031c26499d8", + "config": { + "ldap.full.name.attribute": [ + "cn" + ], + "read.only": [ + "true" + ], + "write.only": [ + "false" + ] + } + } + ] + ] + return_value_component_delete = [ + None ] return_value_component_create = [ { @@ -462,11 +504,11 @@ class TestKeycloakUserFederation(ModuleTestCase): with self.assertRaises(AnsibleExitJson) as exec_info: self.module.main() - self.assertEqual(len(mock_get_components.mock_calls), 2) + self.assertEqual(len(mock_get_components.mock_calls), 3) self.assertEqual(len(mock_get_component.mock_calls), 0) self.assertEqual(len(mock_create_component.mock_calls), 2) self.assertEqual(len(mock_update_component.mock_calls), 0) - self.assertEqual(len(mock_delete_component.mock_calls), 0) + self.assertEqual(len(mock_delete_component.mock_calls), 1) # Verify that the module's changed status matches what is expected self.assertIs(exec_info.exception.args[0]['changed'], changed)