From fd0d05d6f2c97b271b3bb6f13de75e4a50bf6093 Mon Sep 17 00:00:00 2001 From: Jakub Danek Date: Thu, 28 Dec 2023 09:50:01 +0100 Subject: [PATCH] Bugfix: keycloak_identity_provider does not handle mapper changes properly (#7418) * fix keycloak_identity_provider ITs to actually pass - wrong identityProviderAlias in mapper configuration * kc_identity_provider: add mapper reconfiguration regression tests * test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order * kc_identity_provider: add bugfixes for mapper reconfigurations * removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order * add changelog fragment * prevent unnecessary update_mapper calls when there is no change * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein * Update plugins/modules/keycloak_identity_provider.py Co-authored-by: Felix Fontein * kc_identity_provider: sort changeset mappers via name OR id to prevent potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein --------- Co-authored-by: Felix Fontein --- ..._provider-mapper-reconfiguration-fixes.yml | 3 + plugins/modules/keycloak_identity_provider.py | 21 +++- .../keycloak_identity_provider/tasks/main.yml | 113 +++++++++++++++++- 3 files changed, 127 insertions(+), 10 deletions(-) create mode 100644 changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml diff --git a/changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml b/changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml new file mode 100644 index 0000000000..30f3673499 --- /dev/null +++ b/changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml @@ -0,0 +1,3 @@ +bugfixes: + - keycloak_identity_provider - it was not possible to reconfigure (add, remove) ``mappers`` once they were created initially. Removal was ignored, adding new ones resulted in dropping the pre-existing unmodified mappers. Fix resolves the issue by supplying correct input to the internal update call (https://github.com/ansible-collections/community.general/pull/7418). + - keycloak_identity_provider - ``mappers`` processing was not idempotent if the mappers configuration list had not been sorted by name (in ascending order). Fix resolves the issue by sorting mappers in the desired state using the same key which is used for obtaining existing state (https://github.com/ansible-collections/community.general/pull/7418). \ No newline at end of file diff --git a/plugins/modules/keycloak_identity_provider.py b/plugins/modules/keycloak_identity_provider.py index a135b16e43..588f553e8d 100644 --- a/plugins/modules/keycloak_identity_provider.py +++ b/plugins/modules/keycloak_identity_provider.py @@ -542,10 +542,14 @@ def main(): old_mapper = dict() 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) + + if changeset.get('mappers') is None: + changeset['mappers'] = list() + # eventually this holds all desired mappers, unchanged, modified and newly added + changeset['mappers'].append(new_mapper) + + # ensure idempotency in case module.params.mappers is not sorted by name + changeset['mappers'] = sorted(changeset['mappers'], key=lambda x: x.get('id') if x.get('name') is None else x['name']) # Prepare the desired values using the existing values (non-existence results in a dict that is save to use as a basis) desired_idp = before_idp.copy() @@ -612,10 +616,17 @@ def main(): # do the update desired_idp = desired_idp.copy() updated_mappers = desired_idp.pop('mappers', []) + original_mappers = list(before_idp.get('mappers', [])) + kc.update_identity_provider(desired_idp, realm) for mapper in updated_mappers: if mapper.get('id') is not None: - kc.update_identity_provider_mapper(mapper, alias, realm) + # only update existing if there is a change + for i, orig in enumerate(original_mappers): + if mapper['id'] == orig['id']: + del original_mappers[i] + if mapper != orig: + kc.update_identity_provider_mapper(mapper, alias, realm) else: if mapper.get('identityProviderAlias') is None: mapper['identityProviderAlias'] = alias diff --git a/tests/integration/targets/keycloak_identity_provider/tasks/main.yml b/tests/integration/targets/keycloak_identity_provider/tasks/main.yml index 79ba330494..afad9740ed 100644 --- a/tests/integration/targets/keycloak_identity_provider/tasks/main.yml +++ b/tests/integration/targets/keycloak_identity_provider/tasks/main.yml @@ -35,14 +35,14 @@ syncMode: FORCE mappers: - name: "first_name" - identityProviderAlias: "oidc-idp" + identityProviderAlias: "{{ idp }}" identityProviderMapper: "oidc-user-attribute-idp-mapper" config: claim: "first_name" user.attribute: "first_name" syncMode: "INHERIT" - name: "last_name" - identityProviderAlias: "oidc-idp" + identityProviderAlias: "{{ idp }}" identityProviderMapper: "oidc-user-attribute-idp-mapper" config: claim: "last_name" @@ -84,14 +84,14 @@ syncMode: FORCE mappers: - name: "first_name" - identityProviderAlias: "oidc-idp" + identityProviderAlias: "{{ idp }}" identityProviderMapper: "oidc-user-attribute-idp-mapper" config: claim: "first_name" user.attribute: "first_name" syncMode: "INHERIT" - name: "last_name" - identityProviderAlias: "oidc-idp" + identityProviderAlias: "{{ idp }}" identityProviderMapper: "oidc-user-attribute-idp-mapper" config: claim: "last_name" @@ -109,7 +109,7 @@ that: - result is not changed -- name: Update existing identity provider (with change) +- name: Update existing identity provider (with change, no mapper change) community.general.keycloak_identity_provider: auth_keycloak_url: "{{ url }}" auth_realm: "{{ admin_realm }}" @@ -132,6 +132,109 @@ - result.existing.enabled == true - result.end_state.enabled == false +- name: Update existing identity provider (delete mapper) + community.general.keycloak_identity_provider: + auth_keycloak_url: "{{ url }}" + auth_realm: "{{ admin_realm }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + realm: "{{ realm }}" + alias: "{{ idp }}" + state: present + mappers: + - name: "first_name" + identityProviderAlias: "{{ idp }}" + identityProviderMapper: "oidc-user-attribute-idp-mapper" + config: + claim: "first_name" + user.attribute: "first_name" + syncMode: "INHERIT" + register: result + +- name: Debug + debug: + var: result + +- name: Assert identity provider updated + assert: + that: + - result is changed + - result.existing.mappers | length == 2 + - result.end_state.mappers | length == 1 + - result.end_state.mappers[0].name == "first_name" + +- name: Update existing identity provider (add mapper) + community.general.keycloak_identity_provider: + auth_keycloak_url: "{{ url }}" + auth_realm: "{{ admin_realm }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + realm: "{{ realm }}" + alias: "{{ idp }}" + state: present + mappers: + - name: "last_name" + identityProviderAlias: "{{ idp }}" + identityProviderMapper: "oidc-user-attribute-idp-mapper" + config: + claim: "last_name" + user.attribute: "last_name" + syncMode: "INHERIT" + - name: "first_name" + identityProviderAlias: "{{ idp }}" + identityProviderMapper: "oidc-user-attribute-idp-mapper" + config: + claim: "first_name" + user.attribute: "first_name" + syncMode: "INHERIT" + register: result + +- name: Debug + debug: + var: result + +- name: Assert identity provider updated + assert: + that: + - result is changed + - result.existing.mappers | length == 1 + - result.end_state.mappers | length == 2 + +- name: Update existing identity provider (no change, test mapper idempotency) + community.general.keycloak_identity_provider: + auth_keycloak_url: "{{ url }}" + auth_realm: "{{ admin_realm }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + realm: "{{ realm }}" + alias: "{{ idp }}" + state: present + mappers: + - name: "last_name" + identityProviderAlias: "{{ idp }}" + identityProviderMapper: "oidc-user-attribute-idp-mapper" + config: + claim: "last_name" + user.attribute: "last_name" + syncMode: "INHERIT" + - name: "first_name" + identityProviderAlias: "{{ idp }}" + identityProviderMapper: "oidc-user-attribute-idp-mapper" + config: + claim: "first_name" + user.attribute: "first_name" + syncMode: "INHERIT" + register: result + +- name: Debug + debug: + var: result + +- name: Assert identity provider updated + assert: + that: + - result is not changed + - name: Delete existing identity provider community.general.keycloak_identity_provider: auth_keycloak_url: "{{ url }}"