Refactor path_surgery into plugins.util so that nginx can call it

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))
 

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

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.

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."""