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 <felix@fontein.de>

* Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/keycloak_identity_provider.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* 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 <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>

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).

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:
+            if changeset.get('mappers') is None:
-                    changeset['mappers'] = list()
+                changeset['mappers'] = list()
-                changeset['mappers'].append(new_mapper)
+            # 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

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