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 }}"