become - stop using play context in more places (#62373)

* become - stop using play context in more places - ci_complete

* Fix up review points

changelogs/fragments/become-pass-precedence.yaml

@@ -0,0 +1,3 @@
+bugfixes:
+- become - Fix various plugins that still used play_context to get the become password instead of through the plugin - https://github.com/ansible/ansible/issues/62367
+- runas - Fix the ``runas`` ``become_pass`` variable fallback from ``ansible_runas_runas`` to ``ansible_runas_pass``

lib/ansible/plugins/action/__init__.py

@@ -175,6 +175,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         final_environment = dict()
         self._compute_environment_string(final_environment)
 
+        become_kwargs = {}
+        if self._connection.become:
+            become_kwargs['become'] = True
+            become_kwargs['become_method'] = self._connection.become.name
+            become_kwargs['become_user'] = self._connection.become.get_option('become_user',
+                                                                              playcontext=self._play_context)
+            become_kwargs['become_password'] = self._connection.become.get_option('become_pass',
+                                                                                  playcontext=self._play_context)
+            become_kwargs['become_flags'] = self._connection.become.get_option('become_flags',
+                                                                               playcontext=self._play_context)
+
         # modify_module will exit early if interpreter discovery is required; re-run after if necessary
         for dummy in (1, 2):
             try:
@@ -182,12 +193,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                                                                             task_vars=task_vars,
                                                                             module_compression=self._play_context.module_compression,
                                                                             async_timeout=self._task.async_val,
-                                                                            become=self._play_context.become,
-                                                                            become_method=self._play_context.become_method,
-                                                                            become_user=self._play_context.become_user,
-                                                                            become_password=self._play_context.become_pass,
-                                                                            become_flags=self._play_context.become_flags,
-                                                                            environment=final_environment)
+                                                                            environment=final_environment,
+                                                                            **become_kwargs)
                 break
             except InterpreterDiscoveryRequiredError as idre:
                 self._discovered_interpreter = AnsibleUnsafeText(discover_interpreter(
@@ -260,7 +267,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             module_style == "new",                     # old style modules do not support pipelining
             not C.DEFAULT_KEEP_REMOTE_FILES,           # user wants remote files
             not wrap_async or self._connection.always_pipeline_modules,  # async does not normally support pipelining unless it does (eg winrm)
-            self._play_context.become_method != 'su',  # su does not work with pipelining,
+            (self._connection.become.name if self._connection.become else '') != 'su',  # su does not work with pipelining,
             # FIXME: we might need to make become_method exclusion a configurable list
         ]:
             if not condition:
@@ -298,7 +305,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         '''
         # if we don't use become then we know we aren't switching to a
         # different unprivileged user
-        if not self._play_context.become:
+        if not self._connection.become:
             return False
 
         # if we use become and the user is not an admin (or same user) then
@@ -634,7 +641,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             become_user = self.get_become_option('become_user')
             if getattr(self._connection, '_remote_is_local', False):
                 pass
-            elif sudoable and self._play_context.become and become_user:
+            elif sudoable and self._connection.become and become_user:
                 expand_path = '~%s' % become_user
             else:
                 # use remote user instead, if none set default to current user

lib/ansible/plugins/action/fetch.py

@@ -72,7 +72,7 @@ class ActionModule(ActionBase):
             source = self._remote_expand_user(source)
 
             remote_checksum = None
-            if not self._play_context.become:
+            if not self._connection.become:
                 # calculate checksum for the remote file, don't bother if using become as slurp will be used
                 # Force remote_checksum to follow symlinks because fetch always follows symlinks
                 remote_checksum = self._remote_checksum(source, all_vars=task_vars, follow=True)

lib/ansible/plugins/action/win_updates.py

@@ -8,6 +8,7 @@ from ansible.module_utils._text import to_text
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.parsing.yaml.objects import AnsibleUnicode
 from ansible.plugins.action import ActionBase
+from ansible.plugins.loader import become_loader
 from ansible.utils.display import Display
 
 display = Display()
@@ -103,27 +104,19 @@ class ActionModule(ActionBase):
 
     def _execute_module_with_become(self, module_name, module_args, task_vars,
                                     wrap_async, use_task):
-        orig_become = self._play_context.become
-        orig_become_method = self._play_context.become_method
-        orig_become_user = self._play_context.become_user
-
-        if not use_task:
-            if orig_become is None or orig_become is False:
-                self._play_context.become = True
-            if orig_become_method != 'runas':
-                self._play_context.become_method = 'runas'
-            if orig_become_user is None or orig_become_user == 'root':
-                self._play_context.become_user = 'SYSTEM'
-
+        orig_become = self._connection.become
         try:
+            if not use_task and orig_become is None:
+                become = become_loader.get('runas')
+                become.set_options(direct={'become_user': 'SYSTEM', 'become_pass': None})
+                self._connection.set_become_plugin(become)
+
             module_res = self._execute_module(module_name=module_name,
                                               module_args=module_args,
                                               task_vars=task_vars,
                                               wrap_async=wrap_async)
         finally:
-            self._play_context.become = orig_become
-            self._play_context.become_method = orig_become_method
-            self._play_context.become_user = orig_become_user
+            self._connection.set_become_plugin(orig_become)
 
         return module_res
 

lib/ansible/plugins/become/__init__.py

@@ -40,6 +40,19 @@ class BecomeBase(AnsiblePlugin):
         self._id = ''
         self.success = ''
 
+    def get_option(self, option, hostvars=None, playcontext=None):
+        """ Overrides the base get_option to provide a fallback to playcontext vars in case a 3rd party plugin did not
+        implement the base become options required in Ansible. """
+        # TODO: add deprecation warning for ValueError in devel that removes the playcontext fallback
+        try:
+            return super(BecomeBase, self).get_option(option, hostvars=hostvars)
+        except KeyError:
+            pc_fallback = ['become_user', 'become_pass', 'become_flags', 'become_exe']
+            if option not in pc_fallback:
+                raise
+
+            return getattr(playcontext, option, None)
+
     def expect_prompt(self):
         """This function assists connection plugins in determining if they need to wait for
         a prompt. Both a prompt and a password are required.

lib/ansible/plugins/become/runas.py

@@ -48,7 +48,7 @@ DOCUMENTATION = """
             vars:
               - name: ansible_become_password
               - name: ansible_become_pass
-              - name: ansible_runas_runas
+              - name: ansible_runas_pass
             env:
               - name: ANSIBLE_BECOME_PASS
               - name: ANSIBLE_RUNAS_PASS

lib/ansible/plugins/connection/docker.py

@@ -249,7 +249,8 @@ class Connection(ConnectionBase):
                 selector.close()
 
             if not self.become.check_success(become_output):
-                p.stdin.write(to_bytes(self._play_context.become_pass, errors='surrogate_or_strict') + b'\n')
+                become_pass = self.become.get_option('become_pass', playcontext=self._play_context)
+                p.stdin.write(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n')
             fcntl.fcntl(p.stdout, fcntl.F_SETFL, fcntl.fcntl(p.stdout, fcntl.F_GETFL) & ~os.O_NONBLOCK)
             fcntl.fcntl(p.stderr, fcntl.F_SETFL, fcntl.fcntl(p.stderr, fcntl.F_GETFL) & ~os.O_NONBLOCK)
 

lib/ansible/plugins/connection/local.py

@@ -118,7 +118,8 @@ class Connection(ConnectionBase):
                 selector.close()
 
             if not self.become.check_success(become_output):
-                p.stdin.write(to_bytes(self._play_context.become_pass, errors='surrogate_or_strict') + b'\n')
+                become_pass = self.become.get_option('become_pass', playcontext=self._play_context)
+                p.stdin.write(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n')
             fcntl.fcntl(p.stdout, fcntl.F_SETFL, fcntl.fcntl(p.stdout, fcntl.F_GETFL) & ~os.O_NONBLOCK)
             fcntl.fcntl(p.stderr, fcntl.F_SETFL, fcntl.fcntl(p.stderr, fcntl.F_GETFL) & ~os.O_NONBLOCK)
 

lib/ansible/plugins/connection/paramiko_ssh.py

@@ -415,7 +415,9 @@ class Connection(ConnectionBase):
                     display.debug("chunk is: %s" % chunk)
                     if not chunk:
                         if b'unknown user' in become_output:
-                            raise AnsibleError('user %s does not exist' % self._play_context.become_user)
+                            n_become_user = to_native(self.become.get_option('become_user',
+                                                                             playcontext=self._play_context))
+                            raise AnsibleError('user %s does not exist' % n_become_user)
                         else:
                             break
                             # raise AnsibleError('ssh connection closed waiting for password prompt')
@@ -432,8 +434,9 @@ class Connection(ConnectionBase):
                             break
 
                 if passprompt:
-                    if self._play_context.become and self._play_context.become_pass:
-                        chan.sendall(to_bytes(self._play_context.become_pass) + b'\n')
+                    if self.become:
+                        become_pass = self.become.get_option('become_pass', playcontext=self._play_context)
+                        chan.sendall(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n')
                     else:
                         raise AnsibleError("A password is required but none was supplied")
                 else:

lib/ansible/plugins/connection/psrp.py

@@ -577,10 +577,6 @@ if ($bytes_read -gt 0) {
         self._connected = False
 
     def _build_kwargs(self):
-        self._become_method = self._play_context.become_method
-        self._become_user = self._play_context.become_user
-        self._become_pass = self._play_context.become_pass
-
         self._psrp_host = self.get_option('remote_addr')
         self._psrp_user = self.get_option('remote_user')
         self._psrp_pass = self._play_context.password

lib/ansible/plugins/connection/ssh.py

@@ -837,7 +837,7 @@ class Connection(ConnectionBase):
                 # wait for a password prompt.
                 state = states.index('awaiting_prompt')
                 display.debug(u'Initial state: %s: %s' % (states[state], to_text(prompt)))
-            elif self._play_context.become and self.become.success:
+            elif self.become and self.become.success:
                 # We're requesting escalation without a password, so we have to
                 # detect success/failure before sending any initial data.
                 state = states.index('awaiting_escalation')
@@ -947,7 +947,8 @@ class Connection(ConnectionBase):
                 if states[state] == 'awaiting_prompt':
                     if self._flags['become_prompt']:
                         display.debug(u'Sending become_password in response to prompt')
-                        stdin.write(to_bytes(self._play_context.become_pass) + b'\n')
+                        become_pass = self.become.get_option('become_pass', playcontext=self._play_context)
+                        stdin.write(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n')
                         # On python3 stdin is a BufferedWriter, and we don't have a guarantee
                         # that the write will happen without a flush
                         stdin.flush()
@@ -968,19 +969,19 @@ class Connection(ConnectionBase):
                         display.vvv(u'Escalation failed')
                         self._terminate_process(p)
                         self._flags['become_error'] = False
-                        raise AnsibleError('Incorrect %s password' % self._play_context.become_method)
+                        raise AnsibleError('Incorrect %s password' % self.become.name)
                     elif self._flags['become_nopasswd_error']:
                         display.vvv(u'Escalation requires password')
                         self._terminate_process(p)
                         self._flags['become_nopasswd_error'] = False
-                        raise AnsibleError('Missing %s password' % self._play_context.become_method)
+                        raise AnsibleError('Missing %s password' % self.become.name)
                     elif self._flags['become_prompt']:
                         # This shouldn't happen, because we should see the "Sorry,
                         # try again" message first.
                         display.vvv(u'Escalation prompt repeated')
                         self._terminate_process(p)
                         self._flags['become_prompt'] = False
-                        raise AnsibleError('Incorrect %s password' % self._play_context.become_method)
+                        raise AnsibleError('Incorrect %s password' % self.become.name)
 
                 # Once we're sure that the privilege escalation prompt, if any, has
                 # been dealt with, we can send any initial data and start waiting

test/integration/targets/win_become/tasks/main.yml

@@ -143,6 +143,15 @@
     - '"LogonUser failed" not in become_invalid_pass.msg'
     - '"Win32ErrorCode 1326 - 0x0000052E)" not in become_invalid_pass.msg'
 
+  - name: test become password precedence
+    win_whoami:
+    become: yes
+    become_method: runas
+    become_user: '{{ become_test_username }}'
+    vars:
+      ansible_become_pass: broken
+      ansible_runas_pass: '{{ gen_pw }}'  # should have a higher precedence than ansible_become_pass
+
   - name: test become + async
     vars: *become_vars
     win_command: whoami

test/units/plugins/action/test_win_updates.py

@@ -6,13 +6,30 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
+import os
 import pytest
 
 from units.compat.mock import patch, MagicMock
 from ansible.plugins.action.win_updates import ActionModule
+from ansible.plugins.become.runas import BecomeModule
 from ansible.playbook.task import Task
 
 
+@pytest.fixture()
+def test_win_updates():
+    task = MagicMock(Task)
+    task.args = {}
+
+    connection = MagicMock()
+    connection.module_implementation_preferences = ('.ps1', '.exe', '')
+
+    play_context = MagicMock()
+    play_context.check_mode = False
+
+    plugin = ActionModule(task, connection, play_context, loader=None, templar=None, shared_loader_obj=None)
+    return plugin
+
+
 class TestWinUpdatesActionPlugin(object):
 
     INVALID_OPTIONS = (
@@ -58,58 +75,46 @@ class TestWinUpdatesActionPlugin(object):
         assert res['failed']
         assert expected in res['msg']
 
-    BECOME_OPTIONS = (
-        (False, False, "sudo", "root", True, "runas", "SYSTEM"),
-        (False, True, "sudo", "root", True, "runas", "SYSTEM"),
-        (False, False, "runas", "root", True, "runas", "SYSTEM"),
-        (False, False, "sudo", "user", True, "runas", "user"),
-        (False, None, "sudo", None, True, "runas", "SYSTEM"),
+    def test_exec_with_become(self, test_win_updates):
+        test_become = os.urandom(8)
 
-        # use scheduled task, we shouldn't change anything
-        (True, False, "sudo", None, False, "sudo", None),
-        (True, True, "runas", "SYSTEM", True, "runas", "SYSTEM"),
-    )
+        set_become_mock = MagicMock()
+        test_win_updates._connection.become = test_become
+        test_win_updates._connection.set_become_plugin = set_become_mock
 
-    # pylint bug: https://github.com/PyCQA/pylint/issues/511
-    # pylint: disable=undefined-variable
-    @pytest.mark.parametrize('use_task, o_b, o_bmethod, o_buser, e_b, e_bmethod, e_buser',
-                             ((u, ob, obm, obu, eb, ebm, ebu)
-                              for u, ob, obm, obu, eb, ebm, ebu in BECOME_OPTIONS))
-    def test_module_exec_with_become(self, use_task, o_b, o_bmethod, o_buser,
-                                     e_b, e_bmethod, e_buser):
-        def mock_execute_module(self, **kwargs):
-            pc = self._play_context
-            return {"become": pc.become, "become_method": pc.become_method,
-                    "become_user": pc.become_user}
+        with patch('ansible.plugins.action.ActionBase._execute_module', new=MagicMock()):
+            test_win_updates._execute_module_with_become('win_updates', {}, {}, True, False)
 
-        task = MagicMock(Task)
-        task.args = {}
+        # Asserts we don't override the become plugin.
+        assert set_become_mock.call_count == 1
+        assert set_become_mock.mock_calls[0][1][0] == test_become
 
-        connection = MagicMock()
-        connection.module_implementation_preferences = ('.ps1', '.exe', '')
+    def test_exec_with_become_no_plugin_set(self, test_win_updates):
+        set_become_mock = MagicMock()
+        test_win_updates._connection.become = None
+        test_win_updates._connection.set_become_plugin = set_become_mock
 
-        play_context = MagicMock()
-        play_context.check_mode = False
-        play_context.become = o_b
-        play_context.become_method = o_bmethod
-        play_context.become_user = o_buser
+        with patch('ansible.plugins.action.ActionBase._execute_module', new=MagicMock()):
+            test_win_updates._execute_module_with_become('win_updates', {}, {}, True, False)
 
-        plugin = ActionModule(task, connection, play_context, loader=None,
-                              templar=None, shared_loader_obj=None)
-        with patch('ansible.plugins.action.ActionBase._execute_module',
-                   new=mock_execute_module):
-            actual = plugin._execute_module_with_become('win_updates', {}, {},
-                                                        True, use_task)
-
-        # always make sure we reset back to the defaults
-        assert play_context.become == o_b
-        assert play_context.become_method == o_bmethod
-        assert play_context.become_user == o_buser
-
-        # verify what was set when _execute_module was called
-        assert actual['become'] == e_b
-        assert actual['become_method'] == e_bmethod
-        assert actual['become_user'] == e_buser
+        assert set_become_mock.call_count == 2
+        assert isinstance(set_become_mock.mock_calls[0][1][0], BecomeModule)
+        assert set_become_mock.mock_calls[0][1][0].name == 'runas'
+        assert set_become_mock.mock_calls[0][1][0].get_option('become_user') == 'SYSTEM'
+        assert set_become_mock.mock_calls[0][1][0].get_option('become_flags') == ''
+        assert set_become_mock.mock_calls[0][1][0].get_option('become_pass') is None
+        assert set_become_mock.mock_calls[1][1] == (None,)
+
+    def test_exec_with_become_no_plugin_set_use_task(self, test_win_updates):
+        set_become_mock = MagicMock()
+        test_win_updates._connection.become = None
+        test_win_updates._connection.set_become_plugin = set_become_mock
+
+        with patch('ansible.plugins.action.ActionBase._execute_module', new=MagicMock()):
+            test_win_updates._execute_module_with_become('win_updates', {}, {}, True, True)
+
+        assert set_become_mock.call_count == 1
+        assert set_become_mock.mock_calls[0][1][0] is None
 
     def test_module_exec_async_result(self, monkeypatch):
         return_val = {