From 2b1bbc342ed1ea4bdf3ed9a265e6787b65ba914b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Sat, 5 Oct 2019 19:18:53 +0200 Subject: [PATCH 1/4] fix: Call bash only when necessary --- salt/modules/cmdmod.py | 8 ++++- tests/unit/modules/test_cmdmod.py | 60 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 723eddc67b04..d69b627092cd 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -418,7 +418,13 @@ def _get_stripped(cmd): # Ensure environment is correct for a newly logged-in user by running # the command under bash as a login shell - cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd)) + try: + user_shell = __salt__['user.info'](runas)['shell'] + if re.search('bash$', user_shell): + cmd = '{shell} -l -c {cmd}'.format(shell=user_shell, + cmd=_cmd_quote(cmd)) + except KeyError: + pass # Ensure the login is simulated correctly (note: su runs sh, not bash, # which causes the environment to be initialised incorrectly, which is diff --git a/tests/unit/modules/test_cmdmod.py b/tests/unit/modules/test_cmdmod.py index 8da672dd229c..c1add3c52110 100644 --- a/tests/unit/modules/test_cmdmod.py +++ b/tests/unit/modules/test_cmdmod.py @@ -329,6 +329,66 @@ def test_os_environment_remains_intact(self): if not salt.utils.platform.is_darwin(): getpwnam_mock.assert_called_with('foobar') + @skipIf(salt.utils.platform.is_windows(), 'Do not run on Windows') + def test_shell_properly_handled_on_macOS(self): + ''' + cmd.run should invoke a new bash login only + when bash is the default shell for the selected user + ''' + class _CommandHandler: + ''' + Class for capture cmd + ''' + def __init__(self): + self.clear() + + def get(self): + return self._cmd + + def set(self, cmd): + self._cmd = cmd + + def clear(self): + self._cmd = None + + cmd_handler = _CommandHandler() + + def mock_proc(__cmd__, **kwargs): + cmd_handler.set(' '.join(__cmd__)) + return MagicMock(return_value=MockTimedProc(stdout=None, stderr=None)) + + with patch('pwd.getpwnam') as getpwnam_mock: + with patch('salt.utils.timed_subprocess.TimedProc', mock_proc): + with patch('salt.utils.platform.is_darwin', MagicMock(return_value=True)): + + # User default shell is '/usr/local/bin/bash' + user_default_shell = '/usr/local/bin/bash' + with patch.dict(cmdmod.__salt__, + {'user.info': MagicMock(return_value={'shell': user_default_shell})}): + + cmd_handler.clear() + cmdmod._run('ls', + cwd=tempfile.gettempdir(), + runas='foobar', + use_vt=False) + + self.assertRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), + "cmd invokes right bash session on macOS") + + # User default shell is '/bin/zsh' + user_default_shell = '/bin/zsh' + with patch.dict(cmdmod.__salt__, + {'user.info': MagicMock(return_value={'shell': user_default_shell})}): + + cmd_handler.clear() + cmdmod._run('ls', + cwd=tempfile.gettempdir(), + runas='foobar', + use_vt=False) + + self.assertNotRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), + "cmd does not invoke user shell on macOS") + def test_run_cwd_doesnt_exist_issue_7154(self): ''' cmd.run should fail and raise From e1bac5c9f001cb5c1ea20cd7c076c59d4675857d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Sat, 5 Oct 2019 19:25:47 +0200 Subject: [PATCH 2/4] test: Improve test test_shell_properly_handled_on_macOS --- tests/unit/modules/test_cmdmod.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/modules/test_cmdmod.py b/tests/unit/modules/test_cmdmod.py index c1add3c52110..f3068afbd5ef 100644 --- a/tests/unit/modules/test_cmdmod.py +++ b/tests/unit/modules/test_cmdmod.py @@ -386,7 +386,7 @@ def mock_proc(__cmd__, **kwargs): runas='foobar', use_vt=False) - self.assertNotRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), + self.assertNotRegex(cmd_handler.get(), "bash -l -c", "cmd does not invoke user shell on macOS") def test_run_cwd_doesnt_exist_issue_7154(self): From 0a6b7cff87d52c5fcd9d10c6f7b5fb0cc4d199d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Tue, 8 Oct 2019 18:20:18 +0200 Subject: [PATCH 3/4] feat: skip test if platform is not macOS Skip test test_shell_properly_handled_on_macOS if platform is not macOS --- tests/unit/modules/test_cmdmod.py | 59 +++++++++++++++---------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/tests/unit/modules/test_cmdmod.py b/tests/unit/modules/test_cmdmod.py index f3068afbd5ef..3004381898dd 100644 --- a/tests/unit/modules/test_cmdmod.py +++ b/tests/unit/modules/test_cmdmod.py @@ -329,7 +329,7 @@ def test_os_environment_remains_intact(self): if not salt.utils.platform.is_darwin(): getpwnam_mock.assert_called_with('foobar') - @skipIf(salt.utils.platform.is_windows(), 'Do not run on Windows') + @skipIf(not salt.utils.platform.is_darwin(), 'applicable to macOS only') def test_shell_properly_handled_on_macOS(self): ''' cmd.run should invoke a new bash login only @@ -359,35 +359,34 @@ def mock_proc(__cmd__, **kwargs): with patch('pwd.getpwnam') as getpwnam_mock: with patch('salt.utils.timed_subprocess.TimedProc', mock_proc): - with patch('salt.utils.platform.is_darwin', MagicMock(return_value=True)): - - # User default shell is '/usr/local/bin/bash' - user_default_shell = '/usr/local/bin/bash' - with patch.dict(cmdmod.__salt__, - {'user.info': MagicMock(return_value={'shell': user_default_shell})}): - - cmd_handler.clear() - cmdmod._run('ls', - cwd=tempfile.gettempdir(), - runas='foobar', - use_vt=False) - - self.assertRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), - "cmd invokes right bash session on macOS") - - # User default shell is '/bin/zsh' - user_default_shell = '/bin/zsh' - with patch.dict(cmdmod.__salt__, - {'user.info': MagicMock(return_value={'shell': user_default_shell})}): - - cmd_handler.clear() - cmdmod._run('ls', - cwd=tempfile.gettempdir(), - runas='foobar', - use_vt=False) - - self.assertNotRegex(cmd_handler.get(), "bash -l -c", - "cmd does not invoke user shell on macOS") + + # User default shell is '/usr/local/bin/bash' + user_default_shell = '/usr/local/bin/bash' + with patch.dict(cmdmod.__salt__, + {'user.info': MagicMock(return_value={'shell': user_default_shell})}): + + cmd_handler.clear() + cmdmod._run('ls', + cwd=tempfile.gettempdir(), + runas='foobar', + use_vt=False) + + self.assertRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), + "cmd invokes right bash session on macOS") + + # User default shell is '/bin/zsh' + user_default_shell = '/bin/zsh' + with patch.dict(cmdmod.__salt__, + {'user.info': MagicMock(return_value={'shell': user_default_shell})}): + + cmd_handler.clear() + cmdmod._run('ls', + cwd=tempfile.gettempdir(), + runas='foobar', + use_vt=False) + + self.assertNotRegex(cmd_handler.get(), "bash -l -c", + "cmd does not invoke user shell on macOS") def test_run_cwd_doesnt_exist_issue_7154(self): ''' From 28877b6cb867bee42f475931086f8503dcdcfa1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Wed, 25 Dec 2019 09:52:20 +0100 Subject: [PATCH 4/4] bugfix: Solve pylint old-style class warning --- tests/unit/modules/test_cmdmod.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/tests/unit/modules/test_cmdmod.py b/tests/unit/modules/test_cmdmod.py index 3004381898dd..c589ba8567ff 100644 --- a/tests/unit/modules/test_cmdmod.py +++ b/tests/unit/modules/test_cmdmod.py @@ -335,26 +335,20 @@ def test_shell_properly_handled_on_macOS(self): cmd.run should invoke a new bash login only when bash is the default shell for the selected user ''' - class _CommandHandler: + class _CommandHandler(object): ''' - Class for capture cmd + Class for capturing cmd ''' def __init__(self): - self.clear() - - def get(self): - return self._cmd - - def set(self, cmd): - self._cmd = cmd + self.cmd = None def clear(self): - self._cmd = None + self.cmd = None cmd_handler = _CommandHandler() def mock_proc(__cmd__, **kwargs): - cmd_handler.set(' '.join(__cmd__)) + cmd_handler.cmd = ' '.join(__cmd__) return MagicMock(return_value=MockTimedProc(stdout=None, stderr=None)) with patch('pwd.getpwnam') as getpwnam_mock: @@ -371,7 +365,7 @@ def mock_proc(__cmd__, **kwargs): runas='foobar', use_vt=False) - self.assertRegex(cmd_handler.get(), "{} -l -c".format(user_default_shell), + self.assertRegex(cmd_handler.cmd, "{} -l -c".format(user_default_shell), "cmd invokes right bash session on macOS") # User default shell is '/bin/zsh' @@ -385,7 +379,7 @@ def mock_proc(__cmd__, **kwargs): runas='foobar', use_vt=False) - self.assertNotRegex(cmd_handler.get(), "bash -l -c", + self.assertNotRegex(cmd_handler.cmd, "bash -l -c", "cmd does not invoke user shell on macOS") def test_run_cwd_doesnt_exist_issue_7154(self):