From 556dadba6d2646e104d04d4b7dcdda7a7d18306a Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 14 Feb 2023 10:08:02 -0500 Subject: [PATCH] user - fix comparing existing group names to group IDs (#79981) --- .../79981-user-fix-groups-comparison.yml | 2 + lib/ansible/modules/user.py | 26 +++++++----- .../targets/user/tasks/test_local.yml | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 changelogs/fragments/79981-user-fix-groups-comparison.yml diff --git a/changelogs/fragments/79981-user-fix-groups-comparison.yml b/changelogs/fragments/79981-user-fix-groups-comparison.yml new file mode 100644 index 00000000000..95c6726d6c1 --- /dev/null +++ b/changelogs/fragments/79981-user-fix-groups-comparison.yml @@ -0,0 +1,2 @@ +bugfixes: + - user - fix comparing group IDs to existing group names so groups are not always updated (https://github.com/ansible/ansible/issues/79956). diff --git a/lib/ansible/modules/user.py b/lib/ansible/modules/user.py index 022797a8499..a8199628d25 100644 --- a/lib/ansible/modules/user.py +++ b/lib/ansible/modules/user.py @@ -855,7 +855,7 @@ class User(object): if current_groups and not self.append: groups_need_mod = True else: - groups = self.get_groups_set(remove_existing=False) + groups = self.get_groups_set(remove_existing=False, names_only=True) group_diff = set(current_groups).symmetric_difference(groups) if group_diff: @@ -996,16 +996,22 @@ class User(object): except (ValueError, KeyError): return list(grp.getgrnam(group)) - def get_groups_set(self, remove_existing=True): + def get_groups_set(self, remove_existing=True, names_only=False): if self.groups is None: return None info = self.user_info() groups = set(x.strip() for x in self.groups.split(',') if x) + group_names = set() for g in groups.copy(): if not self.group_exists(g): self.module.fail_json(msg="Group %s does not exist" % (g)) - if info and remove_existing and self.group_info(g)[2] == info[3]: + group_info = self.group_info(g) + if info and remove_existing and group_info[2] == info[3]: groups.remove(g) + elif names_only: + group_names.add(group_info[0]) + if names_only: + return group_names return groups def user_group_membership(self, exclude_primary=True): @@ -1511,7 +1517,7 @@ class FreeBsdUser(User): if self.groups is not None: current_groups = self.user_group_membership() - groups = self.get_groups_set() + groups = self.get_groups_set(names_only=True) group_diff = set(current_groups).symmetric_difference(groups) groups_need_mod = False @@ -1705,7 +1711,7 @@ class OpenBSDUser(User): if current_groups and not self.append: groups_need_mod = True else: - groups = self.get_groups_set() + groups = self.get_groups_set(names_only=True) group_diff = set(current_groups).symmetric_difference(groups) if group_diff: @@ -1881,7 +1887,7 @@ class NetBSDUser(User): if current_groups and not self.append: groups_need_mod = True else: - groups = self.get_groups_set() + groups = self.get_groups_set(names_only=True) group_diff = set(current_groups).symmetric_difference(groups) if group_diff: @@ -2115,7 +2121,7 @@ class SunOS(User): if self.groups is not None: current_groups = self.user_group_membership() - groups = self.get_groups_set() + groups = self.get_groups_set(names_only=True) group_diff = set(current_groups).symmetric_difference(groups) groups_need_mod = False @@ -2392,7 +2398,7 @@ class DarwinUser(User): current = set(self._list_user_groups()) if self.groups is not None: - target = set(self.groups.split(',')) + target = self.get_groups_set(names_only=True) else: target = set([]) @@ -2676,7 +2682,7 @@ class AIX(User): if current_groups and not self.append: groups_need_mod = True else: - groups = self.get_groups_set() + groups = self.get_groups_set(names_only=True) group_diff = set(current_groups).symmetric_difference(groups) if group_diff: @@ -2874,7 +2880,7 @@ class HPUX(User): if current_groups and not self.append: groups_need_mod = True else: - groups = self.get_groups_set(remove_existing=False) + groups = self.get_groups_set(remove_existing=False, names_only=True) group_diff = set(current_groups).symmetric_difference(groups) if group_diff: diff --git a/test/integration/targets/user/tasks/test_local.yml b/test/integration/targets/user/tasks/test_local.yml index 67c24a210cb..217d4769019 100644 --- a/test/integration/targets/user/tasks/test_local.yml +++ b/test/integration/targets/user/tasks/test_local.yml @@ -86,9 +86,11 @@ - testgroup3 - testgroup4 - testgroup5 + - testgroup6 - local_ansibulluser tags: - user_test_local_mode + register: test_groups - name: Create local_ansibulluser with groups user: @@ -113,6 +115,18 @@ tags: - user_test_local_mode +- name: Append groups for local_ansibulluser (again) + user: + name: local_ansibulluser + state: present + local: yes + groups: ['testgroup3', 'testgroup4'] + append: yes + register: local_user_test_4_again + ignore_errors: yes + tags: + - user_test_local_mode + - name: Test append without groups for local_ansibulluser user: name: local_ansibulluser @@ -133,6 +147,28 @@ tags: - user_test_local_mode +- name: Append groups for local_ansibulluser using group id + user: + name: local_ansibulluser + state: present + append: yes + groups: "{{ test_groups.results[5]['gid'] }}" + register: local_user_test_7 + ignore_errors: yes + tags: + - user_test_local_mode + +- name: Append groups for local_ansibulluser using gid (again) + user: + name: local_ansibulluser + state: present + append: yes + groups: "{{ test_groups.results[5]['gid'] }}" + register: local_user_test_7_again + ignore_errors: yes + tags: + - user_test_local_mode + # If we don't re-assign, then "Set user expiration" will # fail. - name: Re-assign named group for local_ansibulluser @@ -164,6 +200,7 @@ - testgroup3 - testgroup4 - testgroup5 + - testgroup6 - local_ansibulluser tags: - user_test_local_mode @@ -175,7 +212,10 @@ - local_user_test_2 is not changed - local_user_test_3 is changed - local_user_test_4 is changed + - local_user_test_4_again is not changed - local_user_test_6 is changed + - local_user_test_7 is changed + - local_user_test_7_again is not changed - local_user_test_remove_1 is changed - local_user_test_remove_2 is not changed tags: