From ca5a2b291a1cd03ed5492b5129e13603c69fb21a Mon Sep 17 00:00:00 2001 From: Pierre Dumuid Date: Sat, 30 Oct 2021 16:48:31 +1030 Subject: [PATCH] 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 * Update changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml Co-authored-by: Felix Fontein * Update plugins/modules/identity/keycloak/keycloak_client.py Co-authored-by: Felix Fontein * Remove need for .copy() after making normalise_cr not mutate the dict. Co-authored-by: Pierre Dumuid Co-authored-by: Felix Fontein --- ...keycloak-client-diff-bugs-when-sorting.yml | 2 + .../identity/keycloak/keycloak_client.py | 40 +++++++++++-- .../targets/keycloak_client/README.md | 11 ++++ .../keycloak_client/docker-compose.yml | 26 ++++++++ .../targets/keycloak_client/tasks/main.yml | 59 +++++++++++++++++++ .../targets/keycloak_client/vars/main.yml | 57 ++++++++++++++++++ 6 files changed, 191 insertions(+), 4 deletions(-) create mode 100644 changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml create mode 100644 tests/integration/targets/keycloak_client/README.md create mode 100644 tests/integration/targets/keycloak_client/docker-compose.yml create mode 100644 tests/integration/targets/keycloak_client/tasks/main.yml create mode 100644 tests/integration/targets/keycloak_client/vars/main.yml diff --git a/changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml b/changelogs/fragments/3610-fix-keycloak-client-diff-bugs-when-sorting.yml new file mode 100644 index 0000000000..ebbd6015d4 --- /dev/null +++ b/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). diff --git a/plugins/modules/identity/keycloak/keycloak_client.py b/plugins/modules/identity/keycloak/keycloak_client.py index 4309918f79..82cdab8b6c 100644 --- a/plugins/modules/identity/keycloak/keycloak_client.py +++ b/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) diff --git a/tests/integration/targets/keycloak_client/README.md b/tests/integration/targets/keycloak_client/README.md new file mode 100644 index 0000000000..06c2a4b414 --- /dev/null +++ b/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 +``` diff --git a/tests/integration/targets/keycloak_client/docker-compose.yml b/tests/integration/targets/keycloak_client/docker-compose.yml new file mode 100644 index 0000000000..d14a331e48 --- /dev/null +++ b/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 diff --git a/tests/integration/targets/keycloak_client/tasks/main.yml b/tests/integration/targets/keycloak_client/tasks/main.yml new file mode 100644 index 0000000000..322fc3e2f4 --- /dev/null +++ b/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 diff --git a/tests/integration/targets/keycloak_client/vars/main.yml b/tests/integration/targets/keycloak_client/vars/main.yml new file mode 100644 index 0000000000..0b1555e4bb --- /dev/null +++ b/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"