Avoid to delete both webroot_map and webroot_path (#7095)

* Always restore webroot_path in renewal config.

* Add unit tests to ensure correct behavior

* Add changelog

* Add certbot as modified package

CHANGELOG.md

@@ -16,12 +16,14 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Renewal parameter `webroot_path` is always saved, avoiding some regressions
+  when `webroot` authenticator plugin is invoked with no challenge to perform.
 
 Despite us having broken lockstep, we are continuing to release new versions of
 all Certbot components during releases for the time being, however, the only
 package with changes other than its version number was:
 
+* certbot
 * certbot-dns-rfc2136
 
 More details about these changes can be found on our GitHub repo.

certbot/plugins/webroot_test.py

@@ -295,6 +295,19 @@ class WebrootActionTest(unittest.TestCase):
         self.assertEqual(
             config.webroot_map[self.achall.domain], self.path)
 
+    def test_webroot_map_partial_without_perform(self):
+        # This test acknowledges the fact that webroot_map content will be partial if webroot
+        # plugin perform method is not invoked (corner case when all auths are already valid).
+        # To not be a problem, the webroot_path must always been conserved during renew.
+        # This condition is challenged by:
+        # certbot.tests.renewal_tests::RenewalTest::test_webroot_params_conservation
+        # See https://github.com/certbot/certbot/pull/7095 for details.
+        other_webroot_path = tempfile.mkdtemp()
+        args = self.parser.parse_args("-w {0} -d {1} -w {2} -d bar".format(
+            self.path, self.achall.domain, other_webroot_path).split())
+        self.assertEqual(args.webroot_map, {self.achall.domain: self.path})
+        self.assertEqual(args.webroot_path, [self.path, other_webroot_path])
+
     def _get_config_after_perform(self, config):
         from certbot.plugins.webroot import Authenticator
         auth = Authenticator(config, "webroot")

certbot/renewal.py

@@ -106,11 +106,11 @@ def _restore_webroot_config(config, renewalparams):
     restoring logic is not able to correctly parse it from the serialized
     form.
     """
-    if "webroot_map" in renewalparams:
-        if not cli.set_by_cli("webroot_map"):
-            config.webroot_map = renewalparams["webroot_map"]
-    elif "webroot_path" in renewalparams:
-        logger.debug("Ancient renewal conf file without webroot-map, restoring webroot-path")
+    if "webroot_map" in renewalparams and not cli.set_by_cli("webroot_map"):
+        config.webroot_map = renewalparams["webroot_map"]
+    # To understand why webroot_path and webroot_map processing are not mutually exclusive,
+    # see https://github.com/certbot/certbot/pull/7095
+    if "webroot_path" in renewalparams and not cli.set_by_cli("webroot_path"):
         wp = renewalparams["webroot_path"]
         if isinstance(wp, six.string_types):  # prior to 0.1.0, webroot_path was a string
             wp = [wp]

certbot/tests/renewal_test.py

@@ -28,6 +28,29 @@ class RenewalTest(test_util.ConfigTestCase):
         renewal._restore_webroot_config(config, renewalparams)
         self.assertEqual(config.webroot_path, ['/var/www/'])
 
+    @mock.patch('certbot.renewal.cli.set_by_cli')
+    def test_webroot_params_conservation(self, mock_set_by_cli):
+        # For more details about why this test is important, see:
+        # certbot.plugins.webroot_test::WebrootActionTest::test_webroot_map_partial_without_perform
+        from certbot import renewal
+        mock_set_by_cli.return_value = False
+
+        renewalparams = {
+            'webroot_map': {'test.example.com': '/var/www/test'},
+            'webroot_path': ['/var/www/test', '/var/www/other'],
+        }
+        renewal._restore_webroot_config(self.config, renewalparams)  # pylint: disable=protected-access
+        self.assertEqual(self.config.webroot_map, {'test.example.com': '/var/www/test'})
+        self.assertEqual(self.config.webroot_path, ['/var/www/test', '/var/www/other'])
+
+        renewalparams = {
+            'webroot_map': {},
+            'webroot_path': '/var/www/test',
+        }
+        renewal._restore_webroot_config(self.config, renewalparams)  # pylint: disable=protected-access
+        self.assertEqual(self.config.webroot_map, {})
+        self.assertEqual(self.config.webroot_path, ['/var/www/test'])
+
 
 class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):
     """Tests for certbot.renewal.restore_required_config_elements."""
@@ -89,5 +112,6 @@ class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):
         self.assertRaises(
             errors.Error, self._call, self.config, renewalparams)
 
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover