From 2d89fb1c152636fcd7539d1f36416c0432ad5f70 Mon Sep 17 00:00:00 2001 From: felix-grzelka Date: Sat, 30 May 2026 13:44:08 +0200 Subject: [PATCH] keycloak_user: fix email_verified is not idempotent (#11749) * fix: email_verified is not idempotent * autopep8 * fix import-before-documentation * address reviewer comments * rever formatting * revert more stuff * fix whitespace * clean up * fix diff mode * nox -Re formatters * Update plugins/modules/keycloak_user.py Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> * add deprecation warning * keycloak_default_behavior * Apply suggestions from code review Co-authored-by: Felix Fontein * Update plugins/modules/keycloak_user.py Co-authored-by: Felix Fontein * fix is_struct_included * ignore keycloak_default_behavior and fix is_struct_included * fix diff for groups * fix changed flag in check mode * nox -Re formatters * fix group diff * nox -Re formatters * fix comment logic * add todos * fix user_profile_metadata in diff * refactor diff * rm default for required_actions * update required_actions docstring * fix before_user group handling * nox -Re formatters * fix yaml indent in doc strings * use f-strings * fix tests * fix test_add_new_user * rename keycloak_default_behavior to email_verified_behavior * fix stupid * nox -Re formatters * remove typing from docstring * remove user_profile_metadata parameter * Update plugins/module_utils/_keycloak.py Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> * improve docs * precompute ignored_arguments list * nox -Re formatters * simplify diff logic * add more tests * nox -Re formatters * fix docs * clean up more * fix endstate when user does not change * finalize integrationtest * fail if group is not found * fix tests * nox -Re formatters * fix docstring * add integration tests for required_actions * fix diff logic and fail early * nox -Re formatters * fix boolean logic error * Apply suggestions from code review Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> --------- Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> Co-authored-by: Felix Fontein --- plugins/module_utils/_keycloak.py | 37 ++-- plugins/modules/keycloak_user.py | 139 +++++++++------ .../targets/keycloak_user/tasks/main.yml | 164 +++++++++++++++++- .../plugins/modules/test_keycloak_user.py | 12 +- 4 files changed, 272 insertions(+), 80 deletions(-) diff --git a/plugins/module_utils/_keycloak.py b/plugins/module_utils/_keycloak.py index bd2ce7b6e1..b5695f400f 100644 --- a/plugins/module_utils/_keycloak.py +++ b/plugins/module_utils/_keycloak.py @@ -322,41 +322,44 @@ def get_token(module_params: dict[str, t.Any]) -> dict[str, str]: return {"Authorization": f"Bearer {token}", "Content-Type": "application/json"} -def is_struct_included(struct1: object, struct2: object, exclude: Sequence[str] | None = None) -> bool: +def is_struct_included( + struct1: dict | list | bool | int | str, + struct2: dict | list | bool | int | str, + exclude: Sequence[str] | None = None, + empty_list_result: bool = True, +) -> bool: """ This function compare if the first parameter structure is included in the second. The function use every elements of struct1 and validates they are present in the struct2 structure. The two structure does not need to be equals for that function to return true. Each elements are compared recursively. :param struct1: - type: - dict for the initial call, can be dict, list, bool, int or str for recursive calls description: reference structure :param struct2: - type: - dict for the initial call, can be dict, list, bool, int or str for recursive calls description: structure to compare with first parameter. :param exclude: - type: - list description: Key to exclude from the comparison. - default: None + :param empty_list_result: + description: + Return this value, when struct1 is an empty list. :return: - type: - bool description: Return True if all element of dict 1 are present in dict 2, return false otherwise. """ if isinstance(struct1, list) and isinstance(struct2, list): if not struct1 and not struct2: return True + + if not struct1: + return empty_list_result + for item1 in struct1: if isinstance(item1, (list, dict)): for item2 in struct2: - if is_struct_included(item1, item2, exclude): + if is_struct_included(item1, item2, exclude, empty_list_result): break else: return False @@ -370,7 +373,7 @@ def is_struct_included(struct1: object, struct2: object, exclude: Sequence[str] try: for key in struct1: if not (exclude and key in exclude): - if not is_struct_included(struct1[key], struct2[key], exclude): + if not is_struct_included(struct1[key], struct2[key], exclude, empty_list_result): return False except KeyError: return False @@ -2928,7 +2931,7 @@ class KeycloakAPI: :return: Representation of the user. """ try: - user_url = URL_USER.format(url=self.baseurl, realm=realm, id=user_id) + user_url = URL_USER.format(url=self.baseurl, realm=realm, id=user_id) + "?userProfileMetadata=True" userrep = json.load(self._request(user_url, method="GET")) return userrep except Exception as e: @@ -3083,11 +3086,19 @@ class KeycloakAPI: realm_group = self.find_group_by_path(group_to_add, realm=realm) if realm_group: self.add_user_to_group(user_id=userrep["id"], group_id=realm_group["id"], realm=realm) + else: + self.module.fail_json( + msg=f"Could not update group membership for user {userrep['username']} in realm {realm}: group not found {group_to_add}" + ) for group_to_remove in groups_to_remove: realm_group = self.find_group_by_path(group_to_remove, realm=realm) if realm_group: self.remove_user_from_group(user_id=userrep["id"], group_id=realm_group["id"], realm=realm) + else: + self.module.fail_json( + msg=f"Could not update group membership for user {userrep['username']} in realm {realm}: group not found {group_to_remove}" + ) return True except Exception as e: diff --git a/plugins/modules/keycloak_user.py b/plugins/modules/keycloak_user.py index f3719c7942..93ba4df402 100644 --- a/plugins/modules/keycloak_user.py +++ b/plugins/modules/keycloak_user.py @@ -35,8 +35,9 @@ options: type: bool email_verified: description: - - Check the validity of user email. - default: false + - Set or reset the C(emailVerified) flag of the user. + - When O(email_verified_behavior=no_defaults), the default value of this option becomes C(null) and + that causes the module not to change any existing value for that attribute. type: bool aliases: - emailVerified @@ -134,8 +135,7 @@ options: default: false required_actions: description: - - RequiredActions user Auth. - default: [] + - Set or reset a user's required actions. type: list elements: str aliases: @@ -200,6 +200,19 @@ options: - If V(true), allows to remove user and recreate it. type: bool default: false + email_verified_behavior: + description: + - The O(email_verified) option used to have a default value. This caused problems when the + user expects different behavior from keycloak by default. + - The default value of this option is V(compatibility), which will ensure that the old default value + for O(email_verified) is used. + - When set to V(no_defaults), the module will not change existing values of O(email_verified) if no value is specified. + type: str + choices: + - compatibility + - no_defaults + default: compatibility + version_added: "13.1.0" extends_documentation_fragment: - community.general._keycloak - community.general._keycloak.actiongroup_keycloak @@ -380,7 +393,7 @@ def main(): last_name=dict(type="str", aliases=["lastName"]), email=dict(type="str"), enabled=dict(type="bool"), - email_verified=dict(type="bool", default=False, aliases=["emailVerified"]), + email_verified=dict(type="bool", aliases=["emailVerified"]), federation_link=dict(type="str", aliases=["federationLink"]), service_account_client_id=dict(type="str", aliases=["serviceAccountClientId"]), attributes=dict(type="list", elements="dict", options=attributes_spec), @@ -389,7 +402,7 @@ def main(): disableable_credential_types=dict( type="list", default=[], aliases=["disableableCredentialTypes"], elements="str" ), - required_actions=dict(type="list", default=[], aliases=["requiredActions"], elements="str"), + required_actions=dict(type="list", aliases=["requiredActions"], elements="str"), credentials=dict(type="list", default=[], elements="dict", options=credential_spec), federated_identities=dict(type="list", default=[], aliases=["federatedIdentities"], elements="str"), client_consents=dict( @@ -398,6 +411,7 @@ def main(): origin=dict(type="str"), state=dict(choices=["absent", "present"], default="present"), force=dict(type="bool", default=False), + email_verified_behavior=dict(type="str", choices=["compatibility", "no_defaults"], default="compatibility"), ) argument_spec.update(meta_args) @@ -427,14 +441,21 @@ def main(): username = module.params.get("username") groups = module.params.get("groups") - # Filter and map the parameters names that apply to the user - user_params = [ - x - for x in module.params - if x not in list(keycloak_argument_spec().keys()) + ["state", "realm", "force", "groups"] - and module.params.get(x) is not None + # If there is no value for email_verified, check if we should to set the old default + if module.params["email_verified"] is None and module.params["email_verified_behavior"] == "compatibility": + module.params["email_verified"] = False + + ignored_arguments = list(keycloak_argument_spec().keys()) + [ + "state", + "realm", + "force", + "groups", + "email_verified_behavior", ] + # Filter and map the parameters names that apply to the user + user_params = [x for x in module.params if x not in ignored_arguments and module.params[x] is not None] + before_user = kc.get_user_by_username(username=username, realm=realm) if before_user is None: @@ -465,58 +486,52 @@ def main(): desired_user = copy.deepcopy(before_user) desired_user.update(changeset) + if before_user: + before_groups = kc.get_user_groups(user_id=before_user["id"], realm=realm) + before_user["groups"] = before_groups + else: + before_groups = [] + result["proposed"] = changeset result["existing"] = before_user # Default values for user_created result["user_created"] = False changed = False + after_user = {} - # Cater for when it doesn't exist (an empty dict) if state == "absent": if not before_user: # Do nothing and exit - if module._diff: - result["diff"] = dict(before="", after="") - result["changed"] = False - result["end_state"] = {} - result["msg"] = "Role does not exist, doing nothing." - module.exit_json(**result) + result["msg"] = "User does not exist, doing nothing." else: # Delete user - kc.delete_user(user_id=before_user["id"], realm=realm) + if not module.check_mode: + kc.delete_user(user_id=before_user["id"], realm=realm) result["msg"] = f"User {before_user['username']} deleted" changed = True - else: - after_user = {} - if force and before_user: # If the force option is set to true + if (not before_user or force) and username is None: + module.fail_json(msg="username must be specified when creating a new user") + + if force and before_user and not module.check_mode: # If the force option is set to true # Delete the existing user kc.delete_user(user_id=before_user["id"], realm=realm) if not before_user or force: - # Process a creation - changed = True + # Create a new user + if not module.check_mode: + # Create the user + after_user = kc.create_user(userrep=desired_user, realm=realm) + # Add user ID to desired_user for group updates + desired_user["id"] = after_user["id"] + else: + after_user = desired_user - if username is None: - module.fail_json(msg="username must be specified when creating a new user") - - if module._diff: - result["diff"] = dict(before="", after=desired_user) - - if module.check_mode: - # Set user_created flag explicit for check_mode - # create_user could have failed, but we don't know for sure until we try to create the user.' - result["user_created"] = True - module.exit_json(**result) - - # Create the user - after_user = kc.create_user(userrep=desired_user, realm=realm) result["msg"] = f"User {desired_user['username']} created" - # Add user ID to new representation - desired_user["id"] = after_user["id"] - # Set user_created flag result["user_created"] = True + changed = True else: + # Update an existing user excludes = [ "access", "notBefore", @@ -527,31 +542,45 @@ def main(): "groups", "clientConsents", "federatedIdentities", - "requiredActions", ] - # Add user ID to new representation - desired_user["id"] = before_user["id"] - # Compare users if not ( - is_struct_included(desired_user, before_user, excludes) - ): # If the new user does not introduce a change to the existing user + is_struct_included(desired_user, before_user, excludes, empty_list_result=False) + ): # If the new user introduces a change to the existing user # Update the user - after_user = kc.update_user(userrep=desired_user, realm=realm) + if not module.check_mode: + after_user = kc.update_user(userrep=desired_user, realm=realm) changed = True + if not after_user: + # no change + after_user = desired_user + # set user groups - if kc.update_user_groups_membership(userrep=desired_user, groups=groups, realm=realm): - changed = True - # Get the user groups - after_user["groups"] = kc.get_user_groups(user_id=desired_user["id"], realm=realm) - result["end_state"] = after_user + if not module.check_mode: + changed |= kc.update_user_groups_membership(userrep=desired_user, groups=groups, realm=realm) + + present_groups = [g["name"] for g in groups if g["state"] == "present"] + absent_groups = [g["name"] for g in groups if g["state"] == "absent"] + + desired_user["groups"] = (set(before_groups) | set(present_groups)) - set(absent_groups) + + if module.check_mode: + # check if group meberships would have changed + changed |= not is_struct_included( + desired_user["groups"], before_user["groups"], excludes, empty_list_result=False + ) + else: + after_user["groups"] = kc.get_user_groups(user_id=desired_user["id"], realm=realm) + + if not result["msg"]: if changed: result["msg"] = f"User {desired_user['username']} updated" else: result["msg"] = f"No changes made for user {desired_user['username']}" - + result["end_state"] = after_user result["changed"] = changed + result["diff"] = dict(before=before_user, after=after_user) module.exit_json(**result) diff --git a/tests/integration/targets/keycloak_user/tasks/main.yml b/tests/integration/targets/keycloak_user/tasks/main.yml index 4f5a545b50..dd8c920d77 100644 --- a/tests/integration/targets/keycloak_user/tasks/main.yml +++ b/tests/integration/targets/keycloak_user/tasks/main.yml @@ -80,11 +80,11 @@ var: create_result - name: Assert user is created - assert: + ansible.builtin.assert: that: - create_result.changed - create_result.end_state.username == 'test' - - create_result.end_state.attributes | length == 3 + - create_result.end_state.userProfileMetadata.attributes | length == 4 - create_result.end_state.groups | length == 2 - name: Delete User @@ -108,7 +108,7 @@ var: delete_result - name: Assert user is deleted - assert: + ansible.builtin.assert: that: - delete_result.changed - delete_result.end_state | length == 0 @@ -128,7 +128,7 @@ register: plus_create_result - name: Assert plus-addressed user is created - assert: + ansible.builtin.assert: that: - plus_create_result.changed - plus_create_result.end_state.username == 'testuser+tag' @@ -149,7 +149,7 @@ register: plus_idempotent_result - name: Assert plus-addressed user is idempotent - assert: + ansible.builtin.assert: that: - plus_idempotent_result is not changed @@ -165,6 +165,158 @@ register: plus_delete_result - name: Assert plus-addressed user is deleted - assert: + ansible.builtin.assert: that: - plus_delete_result.changed + +- name: Create user again + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + state: present + email_verified_behavior: no_defaults + register: create_result + +- name: Assert user is created without email_verified + ansible.builtin.assert: + that: + - create_result.changed + - create_result.end_state.username == 'test' + - create_result.end_state.emailVerified == false + +- name: Set email_verified + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + groups: "{{ keycloak_user_groups }}" + attributes: "{{ keycloak_user_attributes }}" + email_verified: true + state: present + register: create_result + +- name: Assert email_verified is set + ansible.builtin.assert: + that: + - create_result.changed + - create_result.end_state.emailVerified == True + +- name: Leave email_verified as it is + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + state: present + email_verified_behavior: no_defaults + register: create_result + +- name: Assert email_verified is still set + ansible.builtin.assert: + that: + - not create_result.changed + - create_result.end_state.emailVerified == True + +- name: Test old email_verified behavior + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + state: present + register: create_result + +- name: Assert email_verified is reset + ansible.builtin.assert: + that: + - create_result.changed + - create_result.end_state.emailVerified == False + +- name: Test setting required_actions + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + required_actions: + - UPDATE_PASSWORD + - VERIFY_EMAIL + state: present + register: create_result + +- name: Assert required_actions are set + ansible.builtin.assert: + that: + - create_result.changed + - "'UPDATE_PASSWORD' in create_result.end_state.requiredActions" + - "'VERIFY_EMAIL' in create_result.end_state.requiredActions" + +- name: Test required_actions unchanged + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + state: present + register: create_result + +- name: Assert required_actions are still set + ansible.builtin.assert: + that: + - not create_result.changed + - "'UPDATE_PASSWORD' in create_result.end_state.requiredActions" + - "'VERIFY_EMAIL' in create_result.end_state.requiredActions" + +- name: Test required_actions reset + community.general.keycloak_user: + auth_keycloak_url: "{{ url }}" + auth_username: "{{ admin_user }}" + auth_password: "{{ admin_password }}" + auth_realm: "{{ admin_realm }}" + username: "{{ keycloak_username }}" + realm: "{{ realm }}" + first_name: Ceciestes + last_name: Untestes + email: ceciestuntestes@test.com + state: present + required_actions: [] + register: create_result + +- name: Assert required_actions are reset + ansible.builtin.assert: + that: + - create_result.changed + - create_result.end_state.requiredActions == [] diff --git a/tests/unit/plugins/modules/test_keycloak_user.py b/tests/unit/plugins/modules/test_keycloak_user.py index 57b3c8de2a..c39b0fc687 100644 --- a/tests/unit/plugins/modules/test_keycloak_user.py +++ b/tests/unit/plugins/modules/test_keycloak_user.py @@ -153,7 +153,7 @@ class TestKeycloakUser(ModuleTestCase): # Verify that the module's changed status matches what is expected self.assertIs(exec_info.exception.args[0]["changed"], changed) - def test_add_exiting_user_no_change(self): + def test_add_existing_user_no_change(self): """Add a new user""" module_args = { @@ -179,7 +179,7 @@ class TestKeycloakUser(ModuleTestCase): } ] return_value_update_user_groups_membership = [False] - return_get_user_groups = [[]] + return_get_user_groups = [[], []] return_create_user = None return_delete_user = None return_update_user = None @@ -210,7 +210,7 @@ class TestKeycloakUser(ModuleTestCase): self.assertEqual(mock_get_user_by_username.call_count, 1) self.assertEqual(mock_create_user.call_count, 0) self.assertEqual(mock_update_user_groups_membership.call_count, 1) - self.assertEqual(mock_get_user_groups.call_count, 1) + self.assertEqual(mock_get_user_groups.call_count, 2) self.assertEqual(mock_update_user.call_count, 0) self.assertEqual(mock_delete_user.call_count, 0) @@ -245,7 +245,7 @@ class TestKeycloakUser(ModuleTestCase): } ] return_value_update_user_groups_membership = [True] - return_get_user_groups = [["group1"]] + return_get_user_groups = [[], ["group1"]] return_create_user = None return_delete_user = None return_update_user = [ @@ -290,7 +290,7 @@ class TestKeycloakUser(ModuleTestCase): self.assertEqual(mock_get_user_by_username.call_count, 1) self.assertEqual(mock_create_user.call_count, 0) self.assertEqual(mock_update_user_groups_membership.call_count, 1) - self.assertEqual(mock_get_user_groups.call_count, 1) + self.assertEqual(mock_get_user_groups.call_count, 2) self.assertEqual(mock_update_user.call_count, 1) self.assertEqual(mock_delete_user.call_count, 0) @@ -354,7 +354,7 @@ class TestKeycloakUser(ModuleTestCase): self.assertEqual(mock_get_user_by_username.call_count, 1) self.assertEqual(mock_create_user.call_count, 0) self.assertEqual(mock_update_user_groups_membership.call_count, 0) - self.assertEqual(mock_get_user_groups.call_count, 0) + self.assertEqual(mock_get_user_groups.call_count, 1) self.assertEqual(mock_update_user.call_count, 0) self.assertEqual(mock_delete_user.call_count, 1)