From 757a8ddae7c5ac1a8acd500cda9b1f7505fc4963 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 8 Jul 2016 00:37:52 -0700
Subject: [PATCH] Fixes & tests

---
 certbot-apache/certbot_apache/configurator.py | 20 +++++++++-----
 .../certbot_apache/tests/configurator_test.py | 26 +++++++++++++++++++
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py
index 329e62135..12cba34f1 100644
--- a/certbot-apache/certbot_apache/configurator.py
+++ b/certbot-apache/certbot_apache/configurator.py
@@ -141,9 +141,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         return os.path.join(self.config.config_dir,
                             constants.MOD_SSL_CONF_DEST)
 
-    def _path_surgery(self):
-        """Mitigate https://github.com/certbot/certbot/issues/1833"""
-        dirs = ("/usr/sbin/", "/usr/local/bin/", "/usr/local/sbin/")
+    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:
@@ -151,9 +155,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 path += os.pathsep + d
                 added.append(d)
         if any(added):
-            logger.debug("Can't find %s, attempting PATH mitigation by adding %s"
+            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.
@@ -173,10 +179,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Verify Apache is installed
         restart_cmd = constants.os_constant("restart_cmd")[0]
         if not util.exe_exists(restart_cmd):
-            self._path_surgery()
+            expanded = self._path_surgery(restart_cmd)
             if not util.exe_exists(restart_cmd):
-                logger.warn("Failed to find %s in expanded PATH: %s",
-                            restart_cmd, os.environ["PATH"])
+                logger.warn("Failed to find %s in %s PATH: %s",
+                            restart_cmd, expanded, os.environ["PATH"])
                 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 9a034c3e0..d5139912e 100644
--- a/certbot-apache/certbot_apache/tests/configurator_test.py
+++ b/certbot-apache/certbot_apache/tests/configurator_test.py
@@ -86,6 +86,32 @@ 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
         # Weak test..