From 9650c2596847263dd82127d326adb3bc22fd4f92 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 31 Oct 2023 17:10:11 -0700 Subject: [PATCH] Fix change detection on mutable values (#9829) (#9830) * handle mutable values * add unit test * add changelog entry * fix typo (cherry picked from commit c3c29afdca43cc1e17e33ba9d4e6120167f5112c) --- certbot/CHANGELOG.md | 2 + certbot/certbot/_internal/cli/helpful.py | 1 + .../_internal/tests/configuration_test.py | 9 ++- certbot/certbot/configuration.py | 65 ++++++++++++++++--- certbot/certbot/tests/util.py | 4 +- 5 files changed, 69 insertions(+), 12 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 4e82bd157..1faadc4c4 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -6,6 +6,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. diff --git a/certbot/certbot/_internal/cli/helpful.py b/certbot/certbot/_internal/cli/helpful.py index 2f3b4554c..73b4b316d 100644 --- a/certbot/certbot/_internal/cli/helpful.py +++ b/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 = [] diff --git a/certbot/certbot/_internal/tests/configuration_test.py b/certbot/certbot/_internal/tests/configuration_test.py index 8d88bb16a..8daed4fbf 100644 --- a/certbot/certbot/_internal/tests/configuration_test.py +++ b/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 diff --git a/certbot/certbot/configuration.py b/certbot/certbot/configuration.py index 4720d7ecc..3e37c7d6e 100644 --- a/certbot/certbot/configuration.py +++ b/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 diff --git a/certbot/certbot/tests/util.py b/certbot/certbot/tests/util.py index bbe2ffe9c..5d078cfbd 100644 --- a/certbot/certbot/tests/util.py +++ b/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"