Fix change detection on mutable values (#9829)

* handle mutable values

* add unit test

* add changelog entry

* fix typo

certbot/CHANGELOG.md

@@ -14,6 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
+* Fixed a bug introduced in version 2.7.0 that caused interactively entered
+  webroot plugin values to not be saved for renewal.
 * Fixed a bug introduced in version 2.7.0 of our Lexicon based DNS plugins that
   caused them to fail to find the DNS zone that needs to be modified in some
   cases.

certbot/certbot/_internal/cli/helpful.py

@@ -165,6 +165,7 @@ class HelpfulArgumentParser:
     def remove_config_file_domains_for_renewal(self, config: NamespaceConfig) -> None:
         """Make "certbot renew" safe if domains are set in cli.ini."""
         # Works around https://github.com/certbot/certbot/issues/4096
+        assert config.argument_sources is not None
         if (config.argument_sources['domains'] == ArgumentSource.CONFIG_FILE and
                 self.verb == "renew"):
             config.domains = []

certbot/certbot/_internal/tests/configuration_test.py

@@ -165,17 +165,22 @@ class NamespaceConfigTest(test_util.ConfigTestCase):
 
     def test_set_by_user_exception(self):
         from certbot.configuration import NamespaceConfig
-        
+
         # a newly created NamespaceConfig has no argument sources dict, so an
         # exception is raised
         config = NamespaceConfig(self.config.namespace)
         with pytest.raises(RuntimeError):
             config.set_by_user('whatever')
-        
+
         # now set an argument sources dict
         config.set_argument_sources({})
         assert not config.set_by_user('whatever')
 
+    def test_set_by_user_mutables(self):
+        assert not self.config.set_by_user('domains')
+        self.config.domains.append('example.org')
+        assert self.config.set_by_user('domains')
+
 
 if __name__ == '__main__':
     sys.exit(pytest.main(sys.argv[1:] + [__file__]))  # pragma: no cover

certbot/certbot/configuration.py

@@ -66,7 +66,8 @@ class NamespaceConfig:
         self.namespace: argparse.Namespace
         # Avoid recursion loop because of the delegation defined in __setattr__
         object.__setattr__(self, 'namespace', namespace)
-        object.__setattr__(self, 'argument_sources', None)
+        object.__setattr__(self, '_argument_sources', None)
+        object.__setattr__(self, '_previously_accessed_mutables', {})
 
         self.namespace.config_dir = os.path.abspath(self.namespace.config_dir)
         self.namespace.work_dir = os.path.abspath(self.namespace.work_dir)
@@ -90,7 +91,7 @@ class NamespaceConfig:
         """
 
         # Avoid recursion loop because of the delegation defined in __setattr__
-        object.__setattr__(self, 'argument_sources', argument_sources)
+        object.__setattr__(self, '_argument_sources', argument_sources)
 
 
     def set_by_user(self, var: str) -> bool:
@@ -145,15 +146,48 @@ class NamespaceConfig:
         """
         If an argument_sources dict was set, overwrites an argument's source to
         be ArgumentSource.RUNTIME. Used when certbot sets an argument's values
-        at runtime.
+        at runtime. This also clears the modified value from
+        _previously_accessed_mutables since it is no longer needed.
         """
-        if self.argument_sources is not None:
-            self.argument_sources[name] = ArgumentSource.RUNTIME
+        if self._argument_sources is not None:
+            self._argument_sources[name] = ArgumentSource.RUNTIME
+            if name in self._previously_accessed_mutables:
+                del self._previously_accessed_mutables[name]
+
+    @property
+    def argument_sources(self) -> Optional[Dict[str, ArgumentSource]]:
+        """Returns _argument_sources after handling any changes to accessed mutable values."""
+        # We keep values in _previously_accessed_mutables until we've detected a modification to try
+        # to provide up-to-date information when argument_sources is accessed. Once a mutable object
+        # has been accessed, it can be modified at any time if a reference to it was kept somewhere
+        # else.
+
+        # We copy _previously_accessed_mutables because _mark_runtime_override modifies it.
+        for name, prev_value in self._previously_accessed_mutables.copy().items():
+            current_value = getattr(self.namespace, name)
+            if current_value != prev_value:
+                self._mark_runtime_override(name)
+        return self._argument_sources
 
     # Delegate any attribute not explicitly defined to the underlying namespace object.
+    #
+    # If any mutable namespace attributes are explicitly defined in the future, you'll probably want
+    # to take an approach like the one used in __getattr__ and the argument_sources property.
 
     def __getattr__(self, name: str) -> Any:
-        return getattr(self.namespace, name)
+        arg_sources = self.argument_sources
+        value = getattr(self.namespace, name)
+        if arg_sources is not None:
+            # If the requested attribute was already modified at runtime, we don't need to track any
+            # future changes.
+            if name not in arg_sources or arg_sources[name] != ArgumentSource.RUNTIME:
+                # If name is already in _previously_accessed_mutables, we don't need to make a copy
+                # of it again. If its value was changed, this would have been caught while preparing
+                # the return value of the property self.argument_sources accessed earlier in this
+                # function.
+                if name not in self._previously_accessed_mutables and not _is_immutable(value):
+                    self._previously_accessed_mutables[name] = copy.deepcopy(value)
+        return value
 
     def __setattr__(self, name: str, value: Any) -> None:
         self._mark_runtime_override(name)
@@ -425,9 +459,10 @@ class NamespaceConfig:
         # Work around https://bugs.python.org/issue1515 for py26 tests :( :(
         new_ns = copy.deepcopy(self.namespace)
         new_config = type(self)(new_ns)
-        if self.set_argument_sources is not None:
-            new_sources = copy.deepcopy(self.argument_sources)
-            new_config.set_argument_sources(new_sources)
+        # Avoid recursion loop because of the delegation defined in __setattr__
+        object.__setattr__(new_config, '_argument_sources', copy.deepcopy(self.argument_sources))
+        object.__setattr__(new_config, '_previously_accessed_mutables',
+                           copy.deepcopy(self._previously_accessed_mutables))
         return new_config
 
 
@@ -450,3 +485,15 @@ def _check_config_sanity(config: NamespaceConfig) -> None:
         for domain in config.namespace.domains:
             # This may be redundant, but let's be paranoid
             util.enforce_domain_sanity(domain)
+
+
+def _is_immutable(value: Any) -> bool:
+    """Is value of an immutable type?"""
+    if isinstance(value, tuple):
+        # tuples are only immutable if all contained values are immutable.
+        return all(_is_immutable(subvalue) for subvalue in value)
+    for immutable_type in (int, float, complex, str, bytes, bool, frozenset,):
+        if isinstance(value, immutable_type):
+            return True
+    # The last case we consider here is None which is also immutable.
+    return value is None

certbot/certbot/tests/util.py

@@ -1,6 +1,7 @@
 """Test utilities."""
 import atexit
 from contextlib import ExitStack
+import copy
 from importlib import reload as reload_module
 import io
 import logging
@@ -403,7 +404,8 @@ class ConfigTestCase(TempDirTestCase):
     def setUp(self) -> None:
         super().setUp()
         self.config = configuration.NamespaceConfig(
-            mock.MagicMock(**constants.CLI_DEFAULTS),
+            # We make a copy here so any mutable values from CLI_DEFAULTS do not get modified.
+            mock.MagicMock(**copy.deepcopy(constants.CLI_DEFAULTS)),
         )
         self.config.set_argument_sources({})
         self.config.namespace.verb = "certonly"