From 8904e625ce1ddc98e9c3425c78c83813919ec9e5 Mon Sep 17 00:00:00 2001
From: Alex Willmer <alex@moreati.org.uk>
Date: Tue, 21 Sep 2021 01:24:30 +0100
Subject: [PATCH] module_utils: Consolidate set_owner_if_different &
 set_group_if_different

This merges implementations of `AnsibleModule.set_owner_if_different()`
& `AnsibleModule.set_group_if_different()` into a single method. The
benefits are
- reduction in code duplication
- makes possible halving the number of calls to `os.lchown()` when the
  copy module sets both owner and group.

There should be no outward change in behavior. The two original methods
become thin wrappers around the new one.

This PR is related to #75741. If both are merged, then the second
benefit above can be realised - by replacing calls in
`copy.chown_recursive()`.

Open questions:

- Should this new method be public (without a leading underscore)?
- Is there a better name?
---
 .../fragments/75742-chown-if-different.yml    |  6 ++
 lib/ansible/module_utils/basic.py             | 78 ++++++++-----------
 2 files changed, 39 insertions(+), 45 deletions(-)
 create mode 100644 changelogs/fragments/75742-chown-if-different.yml

diff --git a/changelogs/fragments/75742-chown-if-different.yml b/changelogs/fragments/75742-chown-if-different.yml
new file mode 100644
index 00000000000..c04405861be
--- /dev/null
+++ b/changelogs/fragments/75742-chown-if-different.yml
@@ -0,0 +1,6 @@
+minor_changes:
+  - >
+    module_utils - Consolidate the implemenations of
+    `AnsibleModule.set_owner_if_different()` &
+    `AnsibleModule.set_group_if_different()`. Provide wrappers to maintain
+    compatibility.
diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index 41ae6288c55..c1ff58908bb 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -722,9 +722,8 @@ class AnsibleModule(object):
             changed = True
         return changed
 
-    def set_owner_if_different(self, path, owner, changed, diff=None, expand=True):
-
-        if owner is None:
+    def _chown_if_different(self, path, owner, group, changed, diff=None, expand=True):
+        if owner is None and group is None:
             return changed
 
         b_path = to_bytes(path, errors='surrogate_or_strict')
@@ -736,15 +735,26 @@ class AnsibleModule(object):
 
         orig_uid, orig_gid = self.user_and_group(b_path, expand)
         try:
-            uid = int(owner)
+            uid = -1 if owner is None else int(owner)
         except ValueError:
             try:
                 uid = pwd.getpwnam(owner).pw_uid
             except KeyError:
                 path = to_text(b_path)
                 self.fail_json(path=path, msg='chown failed: failed to look up user %s' % owner)
+        try:
+            gid = -1 if group is None else int(group)
+        except ValueError:
+            try:
+                gid = grp.getgrnam(group).gr_gid
+            except KeyError:
+                path = to_text(b_path)
+                self.fail_json(path=path, msg='chgrp failed: failed to look up group %s' % group)
+
+        if orig_uid == uid and orig_gid == gid:
+            return changed
 
-        if orig_uid != uid:
+        if uid not in (-1, orig_uid):
             if diff is not None:
                 if 'before' not in diff:
                     diff['before'] = {}
@@ -752,40 +762,9 @@ class AnsibleModule(object):
                 if 'after' not in diff:
                     diff['after'] = {}
                 diff['after']['owner'] = uid
-
-            if self.check_mode:
-                return True
-            try:
-                os.lchown(b_path, uid, -1)
-            except (IOError, OSError) as e:
-                path = to_text(b_path)
-                self.fail_json(path=path, msg='chown failed: %s' % (to_text(e)))
             changed = True
-        return changed
-
-    def set_group_if_different(self, path, group, changed, diff=None, expand=True):
-
-        if group is None:
-            return changed
 
-        b_path = to_bytes(path, errors='surrogate_or_strict')
-        if expand:
-            b_path = os.path.expanduser(os.path.expandvars(b_path))
-
-        if self.check_file_absent_if_check_mode(b_path):
-            return True
-
-        orig_uid, orig_gid = self.user_and_group(b_path, expand)
-        try:
-            gid = int(group)
-        except ValueError:
-            try:
-                gid = grp.getgrnam(group).gr_gid
-            except KeyError:
-                path = to_text(b_path)
-                self.fail_json(path=path, msg='chgrp failed: failed to look up group %s' % group)
-
-        if orig_gid != gid:
+        if gid not in (-1, orig_gid):
             if diff is not None:
                 if 'before' not in diff:
                     diff['before'] = {}
@@ -793,17 +772,26 @@ class AnsibleModule(object):
                 if 'after' not in diff:
                     diff['after'] = {}
                 diff['after']['group'] = gid
-
-            if self.check_mode:
-                return True
-            try:
-                os.lchown(b_path, -1, gid)
-            except OSError:
-                path = to_text(b_path)
-                self.fail_json(path=path, msg='chgrp failed')
             changed = True
+
+        if self.check_mode:
+            return changed
+
+        try:
+            os.lchown(b_path, uid, gid)
+        except (IOError, OSError) as e:
+            path = to_text(b_path)
+            op = 'chown' if uid != -1 else 'chgrp'
+            self.fail_json(path=path, msg='%s failed: %s' % (op, to_text(e)))
+
         return changed
 
+    def set_owner_if_different(self, path, owner, changed, diff=None, expand=True):
+        return self._chown_if_different(path, owner, None, changed, diff=None, expand=True)
+
+    def set_group_if_different(self, path, group, changed, diff=None, expand=True):
+        return self._chown_if_different(path, None, group, changed, diff=None, expand=True)
+
     def set_mode_if_different(self, path, mode, changed, diff=None, expand=True):
 
         if mode is None: