Refactor _fixup_perms2 to remove way-nested logic (#70701)

Change:
- Refactoring to make it harder to get wrong and easier to read.
- Generalize become_unprivileged tests and fix some that never worked
  but also never failed.

Test Plan:
- CI, new units/integration tests

Signed-off-by: Rick Elrod <rick@elrod.me>

changelogs/fragments/fixup_perms2-cleanup.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - Restructured _fixup_perms2() in ansible.plugins.action to make it more linear

lib/ansible/plugins/action/__init__.py

@@ -532,15 +532,30 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         if remote_user is None:
             remote_user = self._get_remote_user()
 
+        # Step 1: Are we on windows?
         if getattr(self._connection._shell, "_IS_WINDOWS", False):
-            # This won't work on Powershell as-is, so we'll just completely skip until
+            # This won't work on Powershell as-is, so we'll just completely
-            # we have a need for it, at which point we'll have to do something different.
+            # skip until we have a need for it, at which point we'll have to do
+            # something different.
             return remote_paths
 
-        if self._is_become_unprivileged():
+        # Step 2: If we're not becoming an unprivileged user, we are roughly
-            # Unprivileged user that's different than the ssh user.  Let's get
+        # done. Make the files +x if we're asked to, and return.
-            # to work!
+        if not self._is_become_unprivileged():
+            if execute:
+                # Can't depend on the file being transferred with execute permissions.
+                # Only need user perms because no become was used here
+                res = self._remote_chmod(remote_paths, 'u+x')
+                if res['rc'] != 0:
+                    raise AnsibleError(
+                        'Failed to set execute bit on remote files '
+                        '(rc: {0}, err: {1})'.format(
+                            res['rc'],
+                            to_native(res['stderr'])))
+            return remote_paths
 
+        # If we're still here, we have an unprivileged user that's different
+        # than the ssh user.
         become_user = self.get_become_option('become_user')
 
         # Try to use file system acls to make the files readable for sudo'd
@@ -550,85 +565,116 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             setfacl_mode = 'r-x'
         else:
             chmod_mode = 'rX'
-                # NOTE: this form fails silently on freebsd.  We currently
+            # TODO: this form fails silently on freebsd.  We currently
             # never call _fixup_perms2() with execute=False but if we
             # start to we'll have to fix this.
             setfacl_mode = 'r-X'
 
-            res = self._remote_set_user_facl(remote_paths, become_user, setfacl_mode)
+        # Step 3a: Are we able to use setfacl to add user ACLs to the file?
-            if res['rc'] != 0:
+        res = self._remote_set_user_facl(
-                # File system acls failed; let's try to use chown next
+            remote_paths,
-                # Set executable bit first as on some systems an
+            become_user,
-                # unprivileged user can use chown
+            setfacl_mode)
+
+        if res['rc'] == 0:
+            return remote_paths
+
+        # Step 3b: Set execute if we need to. We do this before anything else
+        # because some of the methods below might work but not let us set +x
+        # as part of them.
         if execute:
             res = self._remote_chmod(remote_paths, 'u+x')
             if res['rc'] != 0:
-                        raise AnsibleError('Failed to set file mode on remote temporary files (rc: {0}, err: {1})'.format(res['rc'], to_native(res['stderr'])))
+                raise AnsibleError(
+                    'Failed to set file mode on remote temporary files '
+                    '(rc: {0}, err: {1})'.format(
+                        res['rc'],
+                        to_native(res['stderr'])))
 
+        # Step 3c: File system ACLs failed above; try falling back to chown.
         res = self._remote_chown(remote_paths, become_user)
-                if res['rc'] != 0:
+        if res['rc'] == 0:
-                    # First check if we are an admin/root user. If we are
+            return remote_paths
-                    # and failed here, something weird has happened.
-                    if remote_user in self._get_admin_users():
-                        # chown failed even if remote_user is administrator/root
-                        raise AnsibleError('Failed to change ownership of the temporary files Ansible needs to create despite connecting as a privileged user. '
-                                           'Unprivileged become user would be unable to read the file.')
 
-                    # Otherwise, we're a normal user. We failed to chown the
+        # Check if we are an admin/root user. If we are and got here, it means
-                    # paths to the unprivileged user, but if we have a common
+        # we failed to chown as root and something weird has happened.
-                    # group with them, we should be able to chown it to that.
+        if remote_user in self._get_admin_users():
+            raise AnsibleError(
+                'Failed to change ownership of the temporary files Ansible '
+                'needs to create despite connecting as a privileged user. '
+                'Unprivileged become user would be unable to read the '
+                'file.')
+
+        # Step 3d: Common group
+        # Otherwise, we're a normal user. We failed to chown the paths to the
+        # unprivileged user, but if we have a common group with them, we should
+        # be able to chown it to that.
         #
-                    # Note that we have no way of knowing if this will actually
+        # Note that we have no way of knowing if this will actually work... just
-                    # work... just because chgrp exits successfully does not
+        # because chgrp exits successfully does not mean that Ansible will work.
-                    # mean that Ansible will work. We could check if the become
+        # We could check if the become user is in the group, but this would
-                    # user is in the group, but this would create an extra
+        # create an extra round trip.
-                    # round trip.
         #
         # Also note that due to the above, this can prevent the
-                    # ALLOW_WORLD_READABLE_TMPFILES logic below from ever
+        # ALLOW_WORLD_READABLE_TMPFILES logic below from ever getting called. We
-                    # getting called. We leave this up to the user to rectify
+        # leave this up to the user to rectify if they have both of these
-                    # if they have both of these features enabled.
+        # features enabled.
         group = self.get_shell_option('common_remote_group')
         if group is not None:
             res = self._remote_chgrp(remote_paths, group)
             if res['rc'] == 0:
-                            # If ALLOW_WORLD_READABLE_TMPFILES is set, we should warn the user
+                # If ALLOW_WORLD_READABLE_TMPFILES is set, we should warn the
-                            # that something might go weirdly here.
+                # user that something might go weirdly here.
                 if C.ALLOW_WORLD_READABLE_TMPFILES:
-                                display.warning('Both common_remote_group and allow_world_readable_tmpfiles are set. chgrp was successful, but there is no '
+                    display.warning(
-                                                'guarantee that Ansible will be able to read the files after this operation, particularly if '
+                        'Both common_remote_group and '
-                                                'common_remote_group was set to a group of which the unprivileged become user is not a member. In this '
+                        'allow_world_readable_tmpfiles are set. chgrp was '
-                                                'situation, allow_world_readable_tmpfiles is a no-op. See the "Risks of becoming an unprivileged user" section '
+                        'successful, but there is no guarantee that Ansible '
-                                                'of the "Understanding privilege escalation: become" user guide documentation for more information')
+                        'will be able to read the files after this operation, '
+                        'particularly if common_remote_group was set to a '
+                        'group of which the unprivileged become user is not a '
+                        'member. In this situation, '
+                        'allow_world_readable_tmpfiles is a no-op. See this '
+                        'URL for more details: '
+                        'https://docs.ansible.com/ansible/become.html'
+                        '#becoming-an-unprivileged-user')
                 if execute:
                     group_mode = 'g+rwx'
                 else:
                     group_mode = 'g+rw'
                 res = self._remote_chmod(remote_paths, group_mode)
+                if res['rc'] == 0:
+                    return remote_paths
 
-                if res['rc'] != 0:
+        # Step 4: World-readable temp directory
-                    if self.get_shell_option('world_readable_temp', C.ALLOW_WORLD_READABLE_TMPFILES):
+        if self.get_shell_option(
-                        # chown and fs acls failed -- do things this insecure
+                'world_readable_temp',
-                        # way only if the user opted in in the config file
+                C.ALLOW_WORLD_READABLE_TMPFILES):
-                        display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user. '
+            # chown and fs acls failed -- do things this insecure way only if
-                                        'This may be insecure. For information on securing this, see '
+            # the user opted in in the config file
-                                        'https://docs.ansible.com/ansible/user_guide/become.html#risks-of-becoming-an-unprivileged-user')
+            display.warning(
+                'Using world-readable permissions for temporary files Ansible '
+                'needs to create when becoming an unprivileged user. This may '
+                'be insecure. For information on securing this, see '
+                'https://docs.ansible.com/ansible/user_guide/become.html'
+                '#risks-of-becoming-an-unprivileged-user')
             res = self._remote_chmod(remote_paths, 'a+%s' % chmod_mode)
-                        if res['rc'] != 0:
+            if res['rc'] == 0:
-                            raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], to_native(res['stderr'])))
-                    else:
-                        raise AnsibleError('Failed to set permissions on the temporary files Ansible needs to create when becoming an unprivileged user '
-                                           '(rc: %s, err: %s}). For information on working around this, see '
-                                           'https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user'
-                                           % (res['rc'], to_native(res['stderr'])))
-        elif execute:
-            # Can't depend on the file being transferred with execute permissions.
-            # Only need user perms because no become was used here
-            res = self._remote_chmod(remote_paths, 'u+x')
-            if res['rc'] != 0:
-                raise AnsibleError('Failed to set execute bit on remote files (rc: {0}, err: {1})'.format(res['rc'], to_native(res['stderr'])))
-
                 return remote_paths
+            raise AnsibleError(
+                'Failed to set file mode on remote files '
+                '(rc: {0}, err: {1})'.format(
+                    res['rc'],
+                    to_native(res['stderr'])))
+
+        raise AnsibleError(
+            'Failed to set permissions on the temporary files Ansible needs '
+            'to create when becoming an unprivileged user '
+            '(rc: %s, err: %s}). For information on working around this, see '
+            'https://docs.ansible.com/ansible/become.html'
+            '#becoming-an-unprivileged-user' % (
+                res['rc'],
+                to_native(res['stderr'])))
 
     def _remote_chmod(self, paths, mode, sudoable=False):
         '''

test/integration/targets/become_unprivileged/action_plugins/tmpdir.py

@@ -0,0 +1,14 @@
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+from ansible.plugins.action import ActionBase
+
+
+class ActionModule(ActionBase):
+
+    def run(self, tmp=None, task_vars=None):
+        result = super(ActionModule, self).run(tmp, task_vars)
+        result.update(self._execute_module('ping', task_vars=task_vars))
+        result['tmpdir'] = self._connection._shell.tmpdir
+        return result

test/integration/targets/become_unprivileged/cleanup.yml

@@ -1,8 +0,0 @@
-- name: Clean up host
-  hosts: ssh
-  gather_facts: yes
-
-   # Default, just noted here to be explicit about what is happening:
-  remote_user: root
-  roles:
-    - cleanup_become_unprivileged

test/integration/targets/become_unprivileged/cleanup_unpriv_users.yml

@@ -0,0 +1,53 @@
+- name: Clean up host and remove unprivileged users
+  hosts: ssh
+  gather_facts: yes
+  remote_user: root
+  tasks:
+    # Do this first so we can use tilde notation while the user still exists
+    - name: Delete homedirs
+      file:
+        path: '~{{ item }}'
+        state: absent
+      with_items:
+        - unpriv1
+        - unpriv2
+
+    - name: Delete users
+      user:
+        name: "{{ item }}"
+        state: absent
+        force: yes  # I think this is needed in case pipelining is used and the session remains open
+      with_items:
+        - unpriv1
+        - unpriv2
+
+    - name: Delete groups
+      group:
+        name: "{{ item }}"
+        state: absent
+      with_items:
+        - acommongroup
+        - unpriv1
+        - unpriv2
+
+    - name: Fix sudoers.d path for FreeBSD
+      set_fact:
+        sudoers_etc: /usr/local/etc
+      when: ansible_distribution == 'FreeBSD'
+
+    - name: Fix sudoers.d path for everything else
+      set_fact:
+        sudoers_etc: /etc
+      when: ansible_distribution != 'FreeBSD'
+
+    - name: Undo OpenSUSE
+      lineinfile:
+        path: "{{ sudoers_etc }}/sudoers"
+        regexp: '^### Defaults targetpw'
+        line: 'Defaults targetpw'
+        backrefs: yes
+
+    - name: Nuke custom sudoers file
+      file:
+        path: "{{ sudoers_etc }}/sudoers.d/unpriv1"
+        state: absent

test/integration/targets/become_unprivileged/common_remote_group/cleanup.yml

@@ -0,0 +1,35 @@
+- name: Cleanup (as root)
+  hosts: ssh
+  gather_facts: yes
+  remote_user: root
+  tasks:
+    - name: Remove group for unprivileged users
+      group:
+        name: commongroup
+        state: absent
+
+    - name: Check if /usr/bin/setfacl exists
+      stat:
+        path: /usr/bin/setfacl
+      register: usr_bin_setfacl
+
+    - name: Check if /bin/setfacl exists
+      stat:
+        path: /bin/setfacl
+      register: bin_setfacl
+
+    - name: Set path to setfacl
+      set_fact:
+        setfacl_path: /usr/bin/setfacl
+      when: usr_bin_setfacl.stat.exists
+
+    - name: Set path to setfacl
+      set_fact:
+        setfacl_path: /bin/setfacl
+      when: bin_setfacl.stat.exists
+
+    - name: chmod +x setfacl
+      file:
+        path: "{{ setfacl_path }}"
+        mode: a+x
+      when: setfacl_path is defined

test/integration/targets/become_unprivileged/common_remote_group/setup.yml

@@ -0,0 +1,43 @@
+- name: Prep (as root)
+  hosts: ssh
+  gather_facts: yes
+  remote_user: root
+  tasks:
+    - name: Create group for unprivileged users
+      group:
+        name: commongroup
+
+    - name: Add them to the group
+      user:
+        name: "{{ item }}"
+        groups: commongroup
+        append: yes
+      with_items:
+        - unpriv1
+        - unpriv2
+
+    - name: Check if /usr/bin/setfacl exists
+      stat:
+        path: /usr/bin/setfacl
+      register: usr_bin_setfacl
+
+    - name: Check if /bin/setfacl exists
+      stat:
+        path: /bin/setfacl
+      register: bin_setfacl
+
+    - name: Set path to setfacl
+      set_fact:
+        setfacl_path: /usr/bin/setfacl
+      when: usr_bin_setfacl.stat.exists
+
+    - name: Set path to setfacl
+      set_fact:
+        setfacl_path: /bin/setfacl
+      when: bin_setfacl.stat.exists
+
+    - name: chmod -x setfacl to disable it
+      file:
+        path: "{{ setfacl_path }}"
+        mode: a-x
+      when: setfacl_path is defined

test/integration/targets/become_unprivileged/common_remote_group/test.yml

@@ -0,0 +1,36 @@
+- name: Tests for ANSIBLE_COMMON_REMOTE_GROUP functionality
+  hosts: ssh
+  gather_facts: yes
+  remote_user: unpriv1
+
+  tasks:
+    - name: foo
+      action: tmpdir
+      register: tmpdir
+      become_user: unpriv2
+      become: yes
+
+    - name: run whoami with become
+      command: whoami
+      register: whoami
+      become_user: unpriv2
+      become: yes
+
+    - set_fact:
+        stat_cmd: stat -c '%U %G' {{ tmpdir.tmpdir }}/*
+      when: ansible_distribution not in ['MacOSX', 'FreeBSD']
+
+    - set_fact:
+        stat_cmd: stat -f '%Su %Sg' {{ tmpdir.tmpdir }}/*
+      when: ansible_distribution in ['MacOSX', 'FreeBSD']
+
+    - name: Ensure we tested the right fallback
+      shell: "{{ stat_cmd }}"
+      register: stat
+      become_user: unpriv2
+      become: yes
+
+    - assert:
+        that:
+          - whoami.stdout == "unpriv2"
+          - stat.stdout == 'unpriv1 commongroup'

test/integration/targets/become_unprivileged/inventory

@@ -1,7 +1,10 @@
 [ssh]
-ssh-pipelining    ansible_ssh_pipelining=true
+#ssh-pipelining    ansible_ssh_pipelining=true
-#ssh-no-pipelining ansible_ssh_pipelining=false
+ssh-no-pipelining ansible_ssh_pipelining=false
 [ssh:vars]
 ansible_host=localhost
 ansible_connection=ssh
 ansible_python_interpreter="{{ ansible_playbook_python }}"
+
+[all:vars]
+ansible_python_interpreter="{{ ansible_playbook_python }}"

test/integration/targets/become_unprivileged/roles/become_unprivileged/files/baz.txt

@@ -1 +0,0 @@
-hello I was copied

test/integration/targets/become_unprivileged/roles/become_unprivileged/tasks/main.yml

@@ -1,58 +0,0 @@
-- name: Run a command
-  shell: whoami
-  register: whoami
-
-# TODO: We ignore_errors here because atomic_move has some really weird edge
-# cases and gives different behavior based on whether the tmpdir we are copying
-# from is on the same partition as the target or not, among other things. There
-# is probably work to be done there to either unify the behavior if possible, or
-# if not, document/add a warning.
-#
-# In what follows, unpriv1 is remote_user and unpriv2 is become_user. Both
-# users are unprivileged.
-#
-# In particular, given a system (FreeBSD in my testing, but probably any *nix)
-# with a single partition, when we connect (as unpriv1) and become unpriv2,
-# the file ends up being unpriv1:commongroup. We can't chown it after that
-# since we are become_user, so the file remains owned by unpriv1.
-#
-# But when we have multiple partitions, os.rename() in atomic_move fails, and
-# we end up falling back to a whole new bunch of logic. In the end the file
-# ends up being creted as unpriv2 and is unpriv2:unpriv2_login_group.
-#
-# This creates a bunch of inconsistency and really should be documented better
-# but the relevant part for *this* test is that in the single-partition case,
-# we cannot chmod in the `if creating` branch of atomic_move since we do not
-# own the file. That will generate an error.
-- name: Copy a file
-  copy:
-    src: baz.txt
-    dest: ~/uh-oh
-    owner: unpriv2
-    group: notcoolenoughforroot
-    mode: 0644
-  ignore_errors: yes
-
-- name: See if the file exists
-  stat:
-    path: ~/uh-oh
-  register: uh_oh_stat
-
-#- name: Get files in /var/tmp
-#  find:
-#    paths: "/var/tmp/"
-#    patterns: 'ansible*'
-#    file_type: directory
-#  register: found
-#
-#- name: Get latest ansible tmp dir
-#  set_fact:
-#    tmpdir: "{{ found.files | sort(attribute='mtime') | last }}"
-#
-#- debug: var=tmpdir
-
-- assert:
-    that:
-      - whoami.stdout == 'unpriv2'
-      - uh_oh_stat.stat.exists
-      #- tmpdir.gr_name == 'notcoolenoughforroot'

test/integration/targets/become_unprivileged/roles/cleanup_become_unprivileged/tasks/main.yml

@@ -1,74 +0,0 @@
-# Do this first so we can use tilde notation while the user still exists
-- name: Delete homedirs
-  file:
-    path: '~{{ item }}'
-    state: absent
-  with_items:
-    - unpriv1
-    - unpriv2
-
-- name: Delete users
-  user:
-    name: "{{ item }}"
-    state: absent
-    force: yes # I think this is needed in case pipelining is used and the session remains open
-  with_items:
-    - unpriv1
-    - unpriv2
-
-- name: Delete groups
-  group:
-    name: "{{ item }}"
-    state: absent
-  with_items:
-    - notcoolenoughforroot
-    - unpriv1
-    - unpriv2
-
-- name: Fix sudoers.d path for FreeBSD
-  set_fact:
-    sudoers_etc: /usr/local/etc
-  when: ansible_distribution == 'FreeBSD'
-
-- name: Fix sudoers.d path for everything else
-  set_fact:
-    sudoers_etc: /etc
-  when: ansible_distribution != 'FreeBSD'
-
-- name: Undo OpenSUSE
-  lineinfile:
-    path: "{{ sudoers_etc }}/sudoers"
-    regexp: '^### Defaults targetpw'
-    line: 'Defaults targetpw'
-    backrefs: yes
-
-- name: Nuke custom sudoers file
-  file:
-    path: "{{ sudoers_etc }}/sudoers.d/unpriv1"
-    state: absent
-
-- name: Check if /usr/bin/setfacl exists
-  stat:
-    path: /usr/bin/setfacl
-  register: usr_bin_setfacl
-
-- name: Check if the /bin/setfacl exists
-  stat:
-    path: /bin/setfacl
-  register: bin_setfacl
-
-- name: Set path to setfacl
-  set_fact:
-    setfacl_path: /usr/bin/setfacl
-  when: usr_bin_setfacl.stat.exists
-
-- name: Set path to setfacl
-  set_fact:
-    setfacl_path: /bin/setfacl
-  when: bin_setfacl.stat.exists
-
-- name: chmod +x setfacl
-  file:
-    path: "{{ setfacl_path }}"
-    mode: +x
-  when: setfacl_path is defined

test/integration/targets/become_unprivileged/roles/setup_become_unprivileged/tasks/main.yml

@@ -1,128 +0,0 @@
----
-####################################################################
-# NOTE! Any destructive changes you make here... Undo them in
-# cleanup_become_unprivileged so that they don't affect other tests.
-####################################################################
-
-- name: Create groups for unprivileged users
-  group:
-    name: "{{ item }}"
-  with_items:
-    - notcoolenoughforroot
-    - unpriv1
-    - unpriv2
-
-# MacOS requires unencrypted password
-- name: Set password for unpriv1 (MacOSX)
-  set_fact:
-    password: 'iWishIWereCoolEnoughForRoot!'
-  when: ansible_distribution == 'MacOSX'
-
-- name: Set password for unpriv1 (everything else)
-  set_fact:
-    password: $6$CRuKRUfAoVwibjUI$1IEOISMFAE/a0VG73K9QsD0uruXNPLNkZ6xWg4Sk3kZIXwv6.YJLECzfNjn6pu8ay6XlVcj2dUvycLetL5Lgx1
-  when: ansible_distribution != 'MacOSX'
-
-# This user is special. It gets a password so we can sudo as it
-# (we set the sudo password in runme.sh) and it gets wheel so it can
-# `become` unpriv2 without an overly complex sudoers file.
-- name: Create first unprivileged user
-  user:
-    name: unpriv1
-    groups: unpriv1,notcoolenoughforroot
-    append: yes
-    password: "{{ password }}"
-
-- name: Create second unprivileged user
-  user:
-    name: unpriv2
-    groups: unpriv2,notcoolenoughforroot
-    append: yes
-
-- name: Create .ssh for unpriv1
-  file:
-    path: ~unpriv1/.ssh
-    state: directory
-    owner: unpriv1
-    group: unpriv1
-    mode: 0700
-
-- name: Set authorized key for unpriv1
-  copy:
-    src: ~root/.ssh/authorized_keys
-    dest: ~unpriv1/.ssh/authorized_keys
-    remote_src: yes
-    owner: unpriv1
-    group: unpriv1
-    mode: 0600
-
-# Without this we get:
-# "Failed to connect to the host via ssh: "System is booting up. Unprivileged
-# users are not permitted to log in yet. Please come back later."
-- name: Nuke /run/nologin
-  file:
-    path: /run/nologin
-    state: absent
-
-- name: Fix sudoers.d path for FreeBSD
-  set_fact:
-    sudoers_etc: /usr/local/etc
-  when: ansible_distribution == 'FreeBSD'
-
-- name: Fix sudoers.d path for everything else
-  set_fact:
-    sudoers_etc: /etc
-  when: sudoers_etc is not defined
-
-- name: Chown group for bsd and osx
-  set_fact:
-    chowngroup: wheel
-  when: ansible_distribution in ('FreeBSD', 'MacOSX')
-
-- name: Chown group for everything else
-  set_fact:
-    chowngroup: root
-  when: chowngroup is not defined
-
-- name: Make it so unpriv1 can sudo (Chapter 1)
-  copy:
-    dest: "{{ sudoers_etc }}/sudoers.d/unpriv1"
-    content: unpriv1 ALL=(ALL) ALL
-    owner: root
-    group: "{{ chowngroup }}"
-    mode: 0644
-
-# OpenSUSE has a weird sudo default here and requires the root pw
-# instead of the user pw. Undo that setting, we can clean it up later.
-- name: Make it so unpriv1 can sudo (Chapter 2 - The Return Of the OpenSUSE)
-  lineinfile:
-    dest: "{{ sudoers_etc }}/sudoers"
-    regexp: '^Defaults targetpw'
-    line: '### Defaults targetpw'
-    backrefs: yes
-
-- name: Check if /usr/bin/setfacl exists
-  stat:
-    path: /usr/bin/setfacl
-  register: usr_bin_setfacl
-
-- name: Check if the /bin/setfacl exists
-  stat:
-    path: /bin/setfacl
-  register: bin_setfacl
-
-- name: Set path to setfacl
-  set_fact:
-    setfacl_path: /usr/bin/setfacl
-  when: usr_bin_setfacl.stat.exists
-
-- name: Set path to setfacl
-  set_fact:
-    setfacl_path: /bin/setfacl
-  when: bin_setfacl.stat.exists
-
-- name: chmod -x setfacl
-  file:
-    path: "{{ setfacl_path }}"
-    mode: -x
-  when: setfacl_path is defined

test/integration/targets/become_unprivileged/runme.sh

@@ -1,14 +1,35 @@
 #!/usr/bin/env bash
 
-set -ux
+set -eux
 
-ansible-playbook setup.yml -i inventory -v "$@"
+begin_sandwich() {
+    ansible-playbook setup_unpriv_users.yml -i inventory -v "$@"
+}
 
-export ANSIBLE_KEEP_REMOTE_FILES=True
+end_sandwich() {
-export ANSIBLE_COMMON_REMOTE_GROUP=notcoolenoughforroot
+    unset ANSIBLE_KEEP_REMOTE_FILES
-export ANSIBLE_BECOME_PASS='iWishIWereCoolEnoughForRoot!'
+    unset ANSIBLE_COMMON_REMOTE_GROUP
+    unset ANSIBLE_BECOME_PASS
 
-ansible-playbook test.yml -i inventory -v "$@"
+    # Do a few cleanup tasks (nuke users, groups, and homedirs, undo config changes)
+    ansible-playbook cleanup_unpriv_users.yml -i inventory -v "$@"
 
-# Do a few cleanup tasks (nuke users, groups, and homedirs, undo config changes)
+    # We do these last since they do things like remove groups and will error
-ansible-playbook cleanup.yml -i inventory -v "$@"
+    # if there are still users in them.
+    for pb in */cleanup.yml; do
+        ansible-playbook "$pb" -i inventory -v "$@"
+    done
+}
+
+trap "end_sandwich \"\$@\"" EXIT
+
+# Common group tests
+begin_sandwich "$@"
+  ansible-playbook common_remote_group/setup.yml -i inventory -v "$@"
+  export ANSIBLE_KEEP_REMOTE_FILES=True
+  export ANSIBLE_COMMON_REMOTE_GROUP=commongroup
+  export ANSIBLE_BECOME_PASS='iWishIWereCoolEnoughForRoot!'
+  ANSIBLE_ACTION_PLUGINS="$(pwd)/action_plugins"
+  export ANSIBLE_ACTION_PLUGINS
+  ansible-playbook common_remote_group/test.yml -i inventory -v "$@"
+end_sandwich "$@"

test/integration/targets/become_unprivileged/setup.yml

@@ -1,8 +0,0 @@
-- name: Set up host
-  hosts: ssh
-  gather_facts: yes
-
-   # Default, just noted here to be explicit about what is happening:
-  remote_user: root
-  roles:
-    - setup_become_unprivileged

test/integration/targets/become_unprivileged/setup_unpriv_users.yml

@@ -0,0 +1,109 @@
+####################################################################
+# NOTE! Any destructive changes you make here... Undo them in
+# cleanup_become_unprivileged so that they don't affect other tests.
+####################################################################
+- name: Set up host and create unprivileged users
+  hosts: ssh
+  gather_facts: yes
+  remote_user: root
+  tasks:
+    - name: Create groups for unprivileged users
+      group:
+        name: "{{ item }}"
+      with_items:
+        - unpriv1
+        - unpriv2
+
+    # MacOS requires unencrypted password
+    - name: Set password for unpriv1 (MacOSX)
+      set_fact:
+        password: 'iWishIWereCoolEnoughForRoot!'
+      when: ansible_distribution == 'MacOSX'
+
+    - name: Set password for unpriv1 (everything else)
+      set_fact:
+        password: $6$CRuKRUfAoVwibjUI$1IEOISMFAE/a0VG73K9QsD0uruXNPLNkZ6xWg4Sk3kZIXwv6.YJLECzfNjn6pu8ay6XlVcj2dUvycLetL5Lgx1
+      when: ansible_distribution != 'MacOSX'
+
+    # This user is special. It gets a password so we can sudo as it
+    # (we set the sudo password in runme.sh) and it gets wheel so it can
+    # `become` unpriv2 without an overly complex sudoers file.
+    - name: Create first unprivileged user
+      user:
+        name: unpriv1
+        group: unpriv1
+        password: "{{ password }}"
+
+    - name: Create second unprivileged user
+      user:
+        name: unpriv2
+        group: unpriv2
+
+    - name: Special case group add for macOS
+      user:
+        name: unpriv1
+        groups: com.apple.access_ssh
+        append: yes
+      when: ansible_distribution == 'MacOSX'
+
+    - name: Create .ssh for unpriv1
+      file:
+        path: ~unpriv1/.ssh
+        state: directory
+        owner: unpriv1
+        group: unpriv1
+        mode: 0700
+
+    - name: Set authorized key for unpriv1
+      copy:
+        src: ~root/.ssh/authorized_keys
+        dest: ~unpriv1/.ssh/authorized_keys
+        remote_src: yes
+        owner: unpriv1
+        group: unpriv1
+        mode: 0600
+
+    # Without this we get:
+    # "Failed to connect to the host via ssh: "System is booting up. Unprivileged
+    # users are not permitted to log in yet. Please come back later."
+    - name: Nuke /run/nologin
+      file:
+        path: /run/nologin
+        state: absent
+
+    - name: Fix sudoers.d path for FreeBSD
+      set_fact:
+        sudoers_etc: /usr/local/etc
+      when: ansible_distribution == 'FreeBSD'
+
+    - name: Fix sudoers.d path for everything else
+      set_fact:
+        sudoers_etc: /etc
+      when: sudoers_etc is not defined
+
+    - name: Set chown group for bsd and osx
+      set_fact:
+        chowngroup: wheel
+      when: ansible_distribution in ('FreeBSD', 'MacOSX')
+
+    - name: Chown group for everything else
+      set_fact:
+        chowngroup: root
+      when: chowngroup is not defined
+
+    - name: Make it so unpriv1 can sudo (Chapter 1)
+      copy:
+        dest: "{{ sudoers_etc }}/sudoers.d/unpriv1"
+        content: unpriv1 ALL=(ALL) ALL
+        owner: root
+        group: "{{ chowngroup }}"
+        mode: 0644
+
+    # OpenSUSE has a weird sudo default here and requires the root pw
+    # instead of the user pw. Undo that setting, we can clean it up later.
+    - name: Make it so unpriv1 can sudo (Chapter 2 - The Return Of the OpenSUSE)
+      lineinfile:
+        dest: "{{ sudoers_etc }}/sudoers"
+        regexp: '^Defaults targetpw'
+        line: '### Defaults targetpw'
+        backrefs: yes

test/integration/targets/become_unprivileged/test.yml

@@ -1,8 +0,0 @@
-- name: Run the test
-  hosts: ssh
-  gather_facts: yes
-  remote_user: unpriv1
-  become: yes
-  become_user: unpriv2
-  roles:
-    - become_unprivileged

test/units/plugins/action/test_action.py

@@ -316,6 +316,189 @@ class TestActionBase(unittest.TestCase):
         action_base._low_level_execute_command.return_value = dict(rc=1, stdout='some stuff here', stderr='No space left on device')
         self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
 
+    def test_action_base__fixup_perms2(self):
+        mock_task = MagicMock()
+        mock_connection = MagicMock()
+        play_context = PlayContext()
+        action_base = DerivedActionBase(
+            task=mock_task,
+            connection=mock_connection,
+            play_context=play_context,
+            loader=None,
+            templar=None,
+            shared_loader_obj=None,
+        )
+        action_base._low_level_execute_command = MagicMock()
+        remote_paths = ['/tmp/foo/bar.txt', '/tmp/baz.txt']
+        remote_user = 'remoteuser1'
+
+        def runWithNoExpectation(execute=False):
+            return action_base._fixup_perms2(
+                remote_paths,
+                remote_user=remote_user,
+                execute=execute)
+
+        def assertSuccess(execute=False):
+            self.assertEqual(runWithNoExpectation(execute), remote_paths)
+
+        def assertThrowRegex(regex, execute=False):
+            self.assertRaisesRegexp(
+                AnsibleError,
+                regex,
+                action_base._fixup_perms2,
+                remote_paths,
+                remote_user=remote_user,
+                execute=execute)
+
+        def get_shell_option_for_arg(args_kv, default):
+            '''A helper for get_shell_option. Returns a function that, if
+            called with ``option`` that exists in args_kv, will return the
+            value, else will return ``default`` for every other given arg'''
+            def _helper(option, *args, **kwargs):
+                return args_kv.get(option, default)
+            return _helper
+
+        # Step 1: On windows, we just return remote_paths
+        action_base._connection._shell._IS_WINDOWS = True
+        assertSuccess(execute=False)
+        assertSuccess(execute=True)
+
+        # But if we're not on windows....we have more work to do.
+        action_base._connection._shell._IS_WINDOWS = False
+
+        # Step 2: We're /not/ becoming an unprivileged user
+        action_base._remote_chmod = MagicMock()
+        action_base._is_become_unprivileged = MagicMock()
+        action_base._is_become_unprivileged.return_value = False
+        # Two subcases:
+        #   - _remote_chmod rc is 0
+        #   - _remote-chmod rc is not 0, something failed
+        action_base._remote_chmod.return_value = {
+            'rc': 0,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertSuccess(execute=True)
+
+        # When execute=False, we just get the list back. But add it here for
+        # completion. chmod is never called.
+        assertSuccess()
+
+        action_base._remote_chmod.return_value = {
+            'rc': 1,
+            'stdout': 'some stuff here',
+            'stderr': 'and here',
+        }
+        assertThrowRegex(
+            'Failed to set execute bit on remote files',
+            execute=True)
+
+        # Step 3: we are becoming unprivileged
+        action_base._is_become_unprivileged.return_value = True
+
+        # Step 3a: setfacl
+        action_base._remote_set_user_facl = MagicMock()
+        action_base._remote_set_user_facl.return_value = {
+            'rc': 0,
+            'stdout': '',
+            'stderr': '',
+        }
+        assertSuccess()
+
+        # Step 3b: chmod +x if we need to
+        # To get here, setfacl failed, so mock it as such.
+        action_base._remote_set_user_facl.return_value = {
+            'rc': 1,
+            'stdout': '',
+            'stderr': '',
+        }
+        action_base._remote_chmod.return_value = {
+            'rc': 0,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertSuccess(execute=True)
+        action_base._remote_chmod.return_value = {
+            'rc': 1,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertThrowRegex(
+            'Failed to set file mode on remote temporary file',
+            execute=True)
+
+        # Step 3c: chown
+        action_base._remote_chown = MagicMock()
+        action_base._remote_chown.return_value = {
+            'rc': 0,
+            'stdout': '',
+            'stderr': '',
+        }
+        assertSuccess()
+        action_base._remote_chown.return_value = {
+            'rc': 1,
+            'stdout': '',
+            'stderr': '',
+        }
+        remote_user = 'root'
+        action_base._get_admin_users = MagicMock()
+        action_base._get_admin_users.return_value = ['root']
+        assertThrowRegex('user would be unable to read the file.')
+        remote_user = 'remoteuser1'
+
+        # Step 3d: Common group
+
+        get_shell_option = action_base.get_shell_option
+        action_base.get_shell_option = MagicMock()
+        action_base.get_shell_option.side_effect = get_shell_option_for_arg(
+            {
+                'common_remote_group': 'commongroup',
+            },
+            None)
+        action_base._remote_chgrp = MagicMock()
+        action_base._remote_chgrp.return_value = {
+            'rc': 0,
+            'stdout': '',
+            'stderr': '',
+        }
+        # TODO: Add test to assert warning is shown if
+        # ALLOW_WORLD_READABLE_TMPFILES is set in this case.
+        action_base._remote_chmod.return_value = {
+            'rc': 0,
+            'stdout': '',
+            'stderr': '',
+        }
+        assertSuccess()
+        action_base._remote_chgrp.assert_called_once_with(
+            remote_paths,
+            'commongroup')
+
+        # Step 4: world-readable tmpdir
+        action_base.get_shell_option.side_effect = get_shell_option_for_arg(
+            {
+                'world_readable_temp': True,
+                'common_remote_group': None,
+            },
+            None)
+        action_base._remote_chmod.return_value = {
+            'rc': 0,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertSuccess()
+        action_base._remote_chmod.return_value = {
+            'rc': 1,
+            'stdout': 'some stuff here',
+            'stderr': '',
+        }
+        assertThrowRegex('Failed to set file mode on remote files')
+
+        # Otherwise if we make it here in this state, we hit the catch-all
+        action_base.get_shell_option.side_effect = get_shell_option_for_arg(
+            {},
+            None)
+        assertThrowRegex('on the temporary files Ansible needs to create')
+
     def test_action_base__remove_tmp_path(self):
         # create our fake task
         mock_task = MagicMock()