Bugfix keycloak client do not report changes when there is none (#3610)

* KeycloakClientDiffBugs - Introduce test that passes.

* KeycloakClientDiffBugs - Add test to show that checking of redirect_uri's fails.

* KeycloakClientDiffBugs - (Fix1) Update so that checking of `redirectUris` no longer shows a change.

* KeycloakClientDiffBugs - Add test to show that checking of attributes's fails (sorting issue)

* KeycloakClientDiffBugs - (Fix2) Update so that checking of `attributes` no longer shows a change.

* KeycloakClientDiffBugs - Add test to show that checking of protocol_mappers's fail

* KeycloakClientDiffBugs - (Fix3) Update so that checking of `protocol_mappers` no longer shows a change when there is none.

* Introduce code fragment.

* Update the changelog to be based on the PR instead of the issue.

* Fix the readme

* Fix yaml indentation.

* Fix pep8

* Update changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml

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

* Update changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml

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

* Update plugins/modules/identity/keycloak/keycloak_client.py

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

* Remove need for .copy() after making normalise_cr not mutate the dict.

Co-authored-by: Pierre Dumuid <pierre@knowyourdata.com.au>
Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - keycloak_client - update the check mode to not show differences resulting from sorting and default values relating to the properties, ``redirectUris``, ``attributes``, and ``protocol_mappers`` (https://github.com/ansible-collections/community.general/pull/3610).

plugins/modules/identity/keycloak/keycloak_client.py

@@ -685,6 +685,36 @@ from ansible_collections.community.general.plugins.module_utils.identity.keycloa
 from ansible.module_utils.basic import AnsibleModule
 
 
+def normalise_cr(clientrep, remove_ids=False):
+    """ Re-sorts any properties where the order so that diff's is minimised, and adds default values where appropriate so that the
+    the change detection is more effective.
+
+    :param clientrep: the clientrep dict to be sanitized
+    :param remove_ids: If set to true, then the unique ID's of objects is removed to make the diff and checks for changed
+                       not alert when the ID's of objects are not usually known, (e.g. for protocol_mappers)
+    :return: normalised clientrep dict
+    """
+    # Avoid the dict passed in to be modified
+    clientrep = clientrep.copy()
+
+    if 'attributes' in clientrep:
+        clientrep['attributes'] = list(sorted(clientrep['attributes']))
+
+    if 'redirectUris' in clientrep:
+        clientrep['redirectUris'] = list(sorted(clientrep['redirectUris']))
+
+    if 'protocolMappers' in clientrep:
+        clientrep['protocolMappers'] = sorted(clientrep['protocolMappers'], key=lambda x: (x.get('name'), x.get('protocol'), x.get('protocolMapper')))
+        for mapper in clientrep['protocolMappers']:
+            if remove_ids:
+                mapper.pop('id', None)
+
+            # Set to a default value.
+            mapper['consentRequired'] = mapper.get('consentRequired', False)
+
+    return clientrep
+
+
 def sanitize_cr(clientrep):
     """ Removes probably sensitive details from a client representation.
 
@@ -697,7 +727,7 @@ def sanitize_cr(clientrep):
     if 'attributes' in result:
         if 'saml.signing.private.key' in result['attributes']:
             result['attributes']['saml.signing.private.key'] = 'no_log'
-    return result
+    return normalise_cr(result)
 
 
 def main():
@@ -865,10 +895,12 @@ def main():
 
             if module.check_mode:
                 # We can only compare the current client with the proposed updates we have
+                before_norm = normalise_cr(before_client, remove_ids=True)
+                desired_norm = normalise_cr(desired_client, remove_ids=True)
                 if module._diff:
-                    result['diff'] = dict(before=sanitize_cr(before_client),
-                                          after=sanitize_cr(desired_client))
-                result['changed'] = (before_client != desired_client)
+                    result['diff'] = dict(before=sanitize_cr(before_norm),
+                                          after=sanitize_cr(desired_norm))
+                result['changed'] = (before_norm != desired_norm)
 
                 module.exit_json(**result)
 

tests/integration/targets/keycloak_client/README.md

@@ -0,0 +1,11 @@
+The integration test can be performed as follows:
+
+```
+# 1. Start docker-compose:
+docker-compose -f tests/integration/targets/keycloak_client/docker-compose.yml stop
+docker-compose -f tests/integration/targets/keycloak_client/docker-compose.yml rm -f -v
+docker-compose -f tests/integration/targets/keycloak_client/docker-compose.yml up -d
+
+# 2. Run the integration tests:
+ansible-test integration keycloak_client --allow-unsupported -v
+```

tests/integration/targets/keycloak_client/docker-compose.yml

@@ -0,0 +1,26 @@
+version: '3.4'
+
+services:
+  postgres:
+    image: postgres:9.6
+    restart: always
+    environment:
+      POSTGRES_USER: postgres
+      POSTGRES_DB: postgres
+      POSTGRES_PASSWORD: postgres
+
+  keycloak:
+    image: jboss/keycloak:12.0.4
+    ports:
+      - 8080:8080
+
+    environment:
+      DB_VENDOR: postgres
+      DB_ADDR: postgres
+      DB_DATABASE: postgres
+      DB_USER: postgres
+      DB_SCHEMA: public
+      DB_PASSWORD: postgres
+
+      KEYCLOAK_USER: admin
+      KEYCLOAK_PASSWORD: password

tests/integration/targets/keycloak_client/tasks/main.yml

@@ -0,0 +1,59 @@
+---
+- name: Delete realm
+  community.general.keycloak_realm: "{{ auth_args | combine(call_args) }}"
+  vars:
+    call_args:
+      id: "{{ realm }}"
+      realm: "{{ realm }}"
+      state: absent
+
+- name: Create realm
+  community.general.keycloak_realm: "{{ auth_args | combine(call_args) }}"
+  vars:
+    call_args:
+      id: "{{ realm }}"
+      realm: "{{ realm }}"
+      state: present
+
+- name: Desire client
+  community.general.keycloak_client: "{{ auth_args | combine(call_args) }}"
+  vars:
+    call_args:
+      realm: "{{ realm }}"
+      client_id: "{{ client_id }}"
+      state: present
+      redirect_uris: '{{redirect_uris1}}'
+      attributes: '{{client_attributes1}}'
+      protocol_mappers: '{{protocol_mappers1}}'
+  register: desire_client_not_present
+
+- name: Desire client again with same props
+  community.general.keycloak_client: "{{ auth_args | combine(call_args) }}"
+  vars:
+    call_args:
+      realm: "{{ realm }}"
+      client_id: "{{ client_id }}"
+      state: present
+      redirect_uris: '{{redirect_uris1}}'
+      attributes: '{{client_attributes1}}'
+      protocol_mappers: '{{protocol_mappers1}}'
+  register: desire_client_when_present_and_same
+
+- name: Check client again with same props
+  community.general.keycloak_client: "{{ auth_args | combine(call_args) }}"
+  check_mode: yes
+  vars:
+    call_args:
+      realm: "{{ realm }}"
+      client_id: "{{ client_id }}"
+      state: present
+      redirect_uris: '{{redirect_uris1}}'
+      attributes: '{{client_attributes1}}'
+      protocol_mappers: '{{protocol_mappers1}}'
+  register: check_client_when_present_and_same
+
+- name: Assert changes not detected in last two tasks (desire when same, and check)
+  assert:
+    that:
+      - desire_client_when_present_and_same is not changed
+      - check_client_when_present_and_same is not changed

tests/integration/targets/keycloak_client/vars/main.yml

@@ -0,0 +1,57 @@
+---
+url: http://localhost:8080/auth
+admin_realm: master
+admin_user: admin
+admin_password: password
+realm: myrealm
+client_id: myclient
+role: myrole
+description_1: desc 1
+description_2: desc 2
+
+auth_args:
+  auth_keycloak_url: "{{ url }}"
+  auth_realm: "{{ admin_realm }}"
+  auth_username: "{{ admin_user }}"
+  auth_password: "{{ admin_password }}"
+
+redirect_uris1:
+  - "http://example.c.com/"
+  - "http://example.b.com/"
+  - "http://example.a.com/"
+
+client_attributes1: {"backchannel.logout.session.required": true, "backchannel.logout.revoke.offline.tokens": false}
+
+protocol_mappers1:
+  - name: 'email'
+    protocol: 'openid-connect'
+    protocolMapper: 'oidc-usermodel-property-mapper'
+    config:
+      "claim.name": "email"
+      "user.attribute": "email"
+      "jsonType.label": "String"
+      "id.token.claim": "true"
+      "access.token.claim": "true"
+      "userinfo.token.claim": "true"
+
+  - name: 'email_verified'
+    protocol: 'openid-connect'
+    protocolMapper: 'oidc-usermodel-property-mapper'
+    config:
+      "claim.name": "email_verified"
+      "user.attribute": "emailVerified"
+      "jsonType.label": "boolean"
+      "id.token.claim": "true"
+      "access.token.claim": "true"
+      "userinfo.token.claim": "true"
+
+  - name: 'family_name'
+    protocol: 'openid-connect'
+    protocolMapper: 'oidc-usermodel-property-mapper'
+    config:
+      "claim.name": "family_name"
+      "user.attribute": "lastName"
+      "jsonType.label": "String"
+      "id.token.claim": "true"
+      "access.token.claim": "true"
+      "userinfo.token.claim": "true"