From 0bedeb449a239e79ccba3989c12ba07b3c93e363 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 8 Jul 2016 13:58:39 -0700 Subject: [PATCH] Refactor path_surgery into plugins.util so that nginx can call it --- certbot-apache/certbot_apache/configurator.py | 25 +----------- .../certbot_apache/tests/configurator_test.py | 38 ++++--------------- certbot/plugins/util.py | 30 +++++++++++++++ certbot/plugins/util_test.py | 24 ++++++++++++ 4 files changed, 64 insertions(+), 53 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 12cba34f1..74aab242e 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -18,6 +18,7 @@ from certbot import interfaces from certbot import util from certbot.plugins import common +from certbot.plugins.util import path_surgery from certbot_apache import augeas_configurator from certbot_apache import constants @@ -141,25 +142,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return os.path.join(self.config.config_dir, constants.MOD_SSL_CONF_DEST) - def _path_surgery(self, restart_cmd): - """Mitigate https://github.com/certbot/certbot/issues/1833 - - :returns: " expanded" if an expansion of the PATH occurred; - "" otherwise - """ - dirs = ("/usr/sbin", "/usr/local/bin", "/usr/local/sbin") - path = os.environ["PATH"] - added = [] - for d in dirs: - if d not in path: - path += os.pathsep + d - added.append(d) - if any(added): - logger.debug("Can't find %s, attempting PATH mitigation by adding %s", - restart_cmd, os.pathsep.join(added)) - os.environ["PATH"] = path - return " expanded" - return "" def prepare(self): """Prepare the authenticator/installer. @@ -179,10 +161,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Verify Apache is installed restart_cmd = constants.os_constant("restart_cmd")[0] if not util.exe_exists(restart_cmd): - expanded = self._path_surgery(restart_cmd) - if not util.exe_exists(restart_cmd): - logger.warn("Failed to find %s in %s PATH: %s", - restart_cmd, expanded, os.environ["PATH"]) + if not path_surgery(restart_cmd): raise errors.NoInstallationError( 'Cannot find Apache control command {0}'.format(restart_cmd)) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index d5139912e..eac16c7fe 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -49,11 +49,14 @@ class MultipleVhostsTest(util.ApacheTest): shutil.rmtree(self.config_dir) shutil.rmtree(self.work_dir) - @mock.patch("certbot_apache.configurator.util.exe_exists") - def test_prepare_no_install(self, mock_exe_exists): - mock_exe_exists.return_value = False - self.assertRaises( - errors.NoInstallationError, self.config.prepare) + @mock.patch("certbot_apache.configurator.ApacheConfigurator.init_augeas") + @mock.patch("certbot_apache.configurator.path_surgery") + def test_prepare_no_install(self, mock_surgery, _init_augeas): + silly_path = {"PATH": "/tmp/nothingness2342"} + mock_surgery.return_value = False + with mock.patch.dict('os.environ', silly_path): + self.assertRaises(errors.NoInstallationError, self.config.prepare) + self.assertEquals(mock_surgery.call_count, 1) @mock.patch("certbot_apache.augeas_configurator.AugeasConfigurator.init_augeas") def test_prepare_no_augeas(self, mock_init_augeas): @@ -86,31 +89,6 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.NotSupportedError, self.config.prepare) - @mock.patch("certbot_apache.configurator.logger.debug") - def test_path_surgery(self, mock_debug): - # pylint: disable=protected-access - all_path = {"PATH": "/usr/local/bin:/bin/:/usr/sbin/:/usr/local/sbin/"} - with mock.patch.dict('os.environ', all_path): - self.config._path_surgery("thingy") - self.assertEquals(mock_debug.call_count, 0) - self.assertEquals(os.environ["PATH"], all_path["PATH"]) - no_path = {"PATH": "/tmp/"} - with mock.patch.dict('os.environ', no_path): - self.config._path_surgery("thingy") - self.assertEquals(mock_debug.call_count, 1) - self.assertTrue("/usr/local/bin" in os.environ["PATH"]) - self.assertTrue("/tmp" in os.environ["PATH"]) - - @mock.patch("certbot_apache.configurator.ApacheConfigurator.init_augeas") - @mock.patch("certbot_apache.configurator.ApacheConfigurator._path_surgery") - @mock.patch("certbot_apache.configurator.logger.warn") - def test_no_install(self, mock_warn, mock_surgery, _init_augeas): - silly_path = {"PATH": "/tmp/nothingness2342"} - with mock.patch.dict('os.environ', silly_path): - self.assertRaises(errors.NoInstallationError, self.config.prepare) - self.assertEquals(mock_warn.call_count, 1) - self.assertEquals(mock_surgery.call_count, 1) - self.assertTrue("Failed to find" in mock_warn.call_args[0][0]) def test_add_parser_arguments(self): # pylint: disable=no-self-use from certbot_apache.configurator import ApacheConfigurator diff --git a/certbot/plugins/util.py b/certbot/plugins/util.py index 5fc98dff6..cdba88a87 100644 --- a/certbot/plugins/util.py +++ b/certbot/plugins/util.py @@ -1,15 +1,45 @@ """Plugin utilities.""" import logging +import os import socket import psutil import zope.component from certbot import interfaces +from certbot import util logger = logging.getLogger(__name__) +def path_surgery(restart_cmd): + """Attempt to perform PATH surgery to find restart_cmd + + Mitigates https://github.com/certbot/certbot/issues/1833 + + :param str restart_cmd: the command that is being searched for in the PATH + + :returns: True if the operation succeeded, False otherwise + """ + dirs = ("/usr/sbin", "/usr/local/bin", "/usr/local/sbin") + path = os.environ["PATH"] + added = [] + for d in dirs: + if d not in path: + path += os.pathsep + d + added.append(d) + + if any(added): + logger.debug("Can't find %s, attempting PATH mitigation by adding %s", + restart_cmd, os.pathsep.join(added)) + os.environ["PATH"] = path + + if util.exe_exists(restart_cmd): + return True + else: + expanded = " expanded" if any(added) else "" + logger.warn("Failed to find %s in%s PATH: %s", restart_cmd, expanded, path) + return False def already_listening(port, renewer=False): """Check if a process is already listening on the port. diff --git a/certbot/plugins/util_test.py b/certbot/plugins/util_test.py index 9bc8793c7..fa8b364d9 100644 --- a/certbot/plugins/util_test.py +++ b/certbot/plugins/util_test.py @@ -1,9 +1,33 @@ """Tests for certbot.plugins.util.""" +import os import unittest import mock import psutil +class PathSurgeryTest(unittest.TestCase): + """Tests for certbot.plugins.path_surgery.""" + + @mock.patch("certbot.plugins.util.logger.warn") + @mock.patch("certbot.plugins.util.logger.debug") + def test_path_surgery(self, mock_debug, mock_warn): + from certbot.plugins.util import path_surgery + all_path = {"PATH": "/usr/local/bin:/bin/:/usr/sbin/:/usr/local/sbin/"} + with mock.patch.dict('os.environ', all_path): + with mock.patch('certbot.util.exe_exists') as mock_exists: + mock_exists.return_value = True + self.assertEquals(path_surgery("eg"), True) + self.assertEquals(mock_debug.call_count, 0) + self.assertEquals(mock_warn.call_count, 0) + self.assertEquals(os.environ["PATH"], all_path["PATH"]) + no_path = {"PATH": "/tmp/"} + with mock.patch.dict('os.environ', no_path): + path_surgery("thingy") + self.assertEquals(mock_debug.call_count, 1) + self.assertEquals(mock_warn.call_count, 1) + self.assertTrue("Failed to find" in mock_warn.call_args[0][0]) + self.assertTrue("/usr/local/bin" in os.environ["PATH"]) + self.assertTrue("/tmp" in os.environ["PATH"]) class AlreadyListeningTest(unittest.TestCase): """Tests for certbot.plugins.already_listening."""