diff --git a/certbot-compatibility-test/certbot_compatibility_test/configurators/apache/common.py b/certbot-compatibility-test/certbot_compatibility_test/configurators/apache/common.py index 5c7ee1e0f..ca8a02b18 100644 --- a/certbot-compatibility-test/certbot_compatibility_test/configurators/apache/common.py +++ b/certbot-compatibility-test/certbot_compatibility_test/configurators/apache/common.py @@ -58,7 +58,7 @@ class Proxy(configurators_common.Proxy): getattr(entrypoint.ENTRYPOINT.OS_DEFAULTS, k)) self._configurator = entrypoint.ENTRYPOINT( - config=configuration.NamespaceConfig(self.le_config, {}), + config=configuration.NamespaceConfig(self.le_config), name="apache") self._configurator.prepare() diff --git a/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py b/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py index ffb46af87..6f2b9c1ae 100644 --- a/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py +++ b/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py @@ -44,7 +44,7 @@ class Proxy(configurators_common.Proxy): for k in constants.CLI_DEFAULTS: setattr(self.le_config, "nginx_" + k, constants.os_constant(k)) - conf = configuration.NamespaceConfig(self.le_config, {}) + conf = configuration.NamespaceConfig(self.le_config) self._configurator = configurator.NginxConfigurator(config=conf, name="nginx") self._configurator.prepare() diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index 4943dbbfb..e0dadaaf8 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -10,7 +10,6 @@ from typing import Optional from typing import Type import certbot -from certbot.configuration import NamespaceConfig from certbot._internal import constants from certbot._internal.cli.cli_constants import ARGPARSE_PARAMS_TO_REMOVE from certbot._internal.cli.cli_constants import cli_command @@ -21,6 +20,7 @@ from certbot._internal.cli.cli_constants import HELP_AND_VERSION_USAGE from certbot._internal.cli.cli_constants import SHORT_USAGE from certbot._internal.cli.cli_constants import VAR_MODIFIERS from certbot._internal.cli.cli_constants import ZERO_ARG_ACTIONS +from certbot._internal.cli.cli_utils import _Default from certbot._internal.cli.cli_utils import _DeployHookAction from certbot._internal.cli.cli_utils import _DomainsAction from certbot._internal.cli.cli_utils import _EncodeReasonAction @@ -54,19 +54,19 @@ logger = logging.getLogger(__name__) helpful_parser: Optional[HelpfulArgumentParser] = None -def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[str] - ) -> NamespaceConfig: +def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[str], + detect_defaults: bool = False) -> argparse.Namespace: """Returns parsed command line arguments. :param .PluginsRegistry plugins: available plugins :param list args: command line arguments with the program name removed :returns: parsed command line arguments - :rtype: configuration.NamespaceConfig + :rtype: argparse.Namespace """ - helpful = HelpfulArgumentParser(args, plugins) + helpful = HelpfulArgumentParser(args, plugins, detect_defaults) _add_all_groups(helpful) # --help is automatically provided by argparse @@ -471,16 +471,95 @@ def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[st # parser (--help should display plugin-specific options last) _plugins_parsing(helpful, plugins) - global helpful_parser # pylint: disable=global-statement - helpful_parser = helpful + if not detect_defaults: + global helpful_parser # pylint: disable=global-statement + helpful_parser = helpful return helpful.parse_args() +def set_by_cli(var: str) -> bool: + """ + Return True if a particular config variable has been set by the user + (CLI or config file) including if the user explicitly set it to the + default. Returns False if the variable was assigned a default value. + """ + # We should probably never actually hit this code. But if we do, + # a deprecated option has logically never been set by the CLI. + if var in DEPRECATED_OPTIONS: + return False + + detector = set_by_cli.detector # type: ignore + if detector is None and helpful_parser is not None: + # Setup on first run: `detector` is a weird version of config in which + # the default value of every attribute is wrangled to be boolean-false + plugins = plugins_disco.PluginsRegistry.find_all() + # reconstructed_args == sys.argv[1:], or whatever was passed to main() + reconstructed_args = helpful_parser.args + [helpful_parser.verb] + + detector = set_by_cli.detector = prepare_and_parse_args( # type: ignore + plugins, reconstructed_args, detect_defaults=True) + # propagate plugin requests: eg --standalone modifies config.authenticator + detector.authenticator, detector.installer = ( + plugin_selection.cli_plugin_requests(detector)) + + if not isinstance(getattr(detector, var), _Default): + logger.debug("Var %s=%s (set by user).", var, getattr(detector, var)) + return True + + for modifier in VAR_MODIFIERS.get(var, []): + if set_by_cli(modifier): + logger.debug("Var %s=%s (set by user).", + var, VAR_MODIFIERS.get(var, [])) + return True + + return False + + +# static housekeeping var +# functions attributed are not supported by mypy +# https://github.com/python/mypy/issues/2087 +set_by_cli.detector = None # type: ignore + + +def has_default_value(option: str, value: Any) -> bool: + """Does option have the default value? + + If the default value of option is not known, False is returned. + + :param str option: configuration variable being considered + :param value: value of the configuration variable named option + + :returns: True if option has the default value, otherwise, False + :rtype: bool + + """ + if helpful_parser is not None: + return (option in helpful_parser.defaults and + helpful_parser.defaults[option] == value) + return False + + +def option_was_set(option: str, value: Any) -> bool: + """Was option set by the user or does it differ from the default? + + :param str option: configuration variable being considered + :param value: value of the configuration variable named option + + :returns: True if the option was set, otherwise, False + :rtype: bool + + """ + # If an option is deprecated, it was effectively not set by the user. + if option in DEPRECATED_OPTIONS: + return False + return set_by_cli(option) or not has_default_value(option, value) + + def argparse_type(variable: Any) -> Type: """Return our argparse type function for a config variable (default: str)""" # pylint: disable=protected-access if helpful_parser is not None: - for action in helpful_parser.actions: + for action in helpful_parser.parser._actions: if action.type is not None and action.dest == variable: return action.type return str diff --git a/certbot/certbot/_internal/cli/cli_utils.py b/certbot/certbot/_internal/cli/cli_utils.py index 9d9c309da..5f3267eb0 100644 --- a/certbot/certbot/_internal/cli/cli_utils.py +++ b/certbot/certbot/_internal/cli/cli_utils.py @@ -22,6 +22,22 @@ if TYPE_CHECKING: from certbot._internal.cli import helpful +class _Default: + """A class to use as a default to detect if a value is set by a user""" + + def __bool__(self) -> bool: + return False + + def __eq__(self, other: Any) -> bool: + return isinstance(other, _Default) + + def __hash__(self) -> int: + return id(_Default) + + def __nonzero__(self) -> bool: + return self.__bool__() + + def read_file(filename: str, mode: str = "rb") -> Tuple[str, Any]: """Returns the given file's contents. diff --git a/certbot/certbot/_internal/cli/helpful.py b/certbot/certbot/_internal/cli/helpful.py index 92ba83bea..0dd72e53a 100644 --- a/certbot/certbot/_internal/cli/helpful.py +++ b/certbot/certbot/_internal/cli/helpful.py @@ -1,6 +1,7 @@ """Certbot command line argument parser""" import argparse +import copy import functools import glob import sys @@ -9,7 +10,6 @@ from typing import Dict from typing import Iterable from typing import List from typing import Optional -from typing import Tuple from typing import Union import configargparse @@ -19,9 +19,13 @@ from certbot import errors from certbot import util from certbot._internal import constants from certbot._internal import hooks +from certbot._internal.cli.cli_constants import ARGPARSE_PARAMS_TO_REMOVE from certbot._internal.cli.cli_constants import COMMAND_OVERVIEW +from certbot._internal.cli.cli_constants import EXIT_ACTIONS from certbot._internal.cli.cli_constants import HELP_AND_VERSION_USAGE from certbot._internal.cli.cli_constants import SHORT_USAGE +from certbot._internal.cli.cli_constants import ZERO_ARG_ACTIONS +from certbot._internal.cli.cli_utils import _Default from certbot._internal.cli.cli_utils import add_domains from certbot._internal.cli.cli_utils import CustomHelpFormatter from certbot._internal.cli.cli_utils import flag_default @@ -31,8 +35,6 @@ from certbot._internal.cli.verb_help import VERB_HELP_MAP from certbot._internal.display import obj as display_obj from certbot._internal.plugins import disco from certbot.compat import os -from certbot.configuration import ArgumentSource -from certbot.configuration import NamespaceConfig class HelpfulArgumentParser: @@ -43,7 +45,8 @@ class HelpfulArgumentParser: 'certbot --help security' for security options. """ - def __init__(self, args: List[str], plugins: Iterable[str]) -> None: + def __init__(self, args: List[str], plugins: Iterable[str], + detect_defaults: bool = False) -> None: from certbot._internal import main self.VERBS = { "auth": main.certonly, @@ -69,8 +72,6 @@ class HelpfulArgumentParser: # Get notification function for printing self.notify = display_obj.NoninteractiveDisplay(sys.stdout).notification - self.actions: List[configargparse.Action] = [] - # List of topics for which additional help can be provided HELP_TOPICS: List[Optional[str]] = ["all", "security", "paths", "automation", "testing"] HELP_TOPICS += list(self.VERBS) + self.COMMANDS_TOPICS + ["manage"] @@ -78,6 +79,7 @@ class HelpfulArgumentParser: plugin_names: List[Optional[str]] = list(plugins) self.help_topics: List[Optional[str]] = HELP_TOPICS + plugin_names + [None] + self.detect_defaults = detect_defaults self.args = args if self.args and self.args[0] == 'help': @@ -98,6 +100,8 @@ class HelpfulArgumentParser: # elements are added by .add_group() self.groups: Dict[str, argparse._ArgumentGroup] = {} + # elements are added by .parse_args() + self.defaults: Dict[str, Any] = {} self.parser = configargparse.ArgParser( prog="certbot", @@ -162,114 +166,75 @@ class HelpfulArgumentParser: return usage - def remove_config_file_domains_for_renewal(self, config: NamespaceConfig) -> None: + def remove_config_file_domains_for_renewal(self, parsed_args: argparse.Namespace) -> None: """Make "certbot renew" safe if domains are set in cli.ini.""" # Works around https://github.com/certbot/certbot/issues/4096 - if (config.argument_sources['domains'] == ArgumentSource.CONFIG_FILE and - self.verb == "renew"): - config.domains = [] + if self.verb == "renew": + for source, flags in self.parser._source_to_settings.items(): # pylint: disable=protected-access + if source.startswith("config_file") and "domains" in flags: + parsed_args.domains = _Default() if self.detect_defaults else [] - def _build_sources_dict(self) -> Dict[str, ArgumentSource]: - # ConfigArgparse's get_source_to_settings_dict doesn't actually create - # default entries for each argument with a default value, omitting many - # args we'd otherwise care about. So in general, unless an argument was - # specified in a config file/environment variable/command line arg, - # consider it as having a "default" value - result = { action.dest: ArgumentSource.DEFAULT for action in self.actions } - - source_to_settings_dict: Dict[str, Dict[str, Tuple[configargparse.Action, str]]] - source_to_settings_dict = self.parser.get_source_to_settings_dict() - - # We'll process the sources dict in order of each source's "priority", - # i.e. the order in which ConfigArgparse ultimately sets argument - # values: - # 1. defaults (`result` already has everything marked as such) - # 2. config files - # 3. env vars (shouldn't be any) - # 4. command line - def update_result(settings_dict: Dict[str, Tuple[configargparse.Action, str]], - source: ArgumentSource) -> None: - actions = [action for _, (action, _) in settings_dict.items()] - result.update({ action.dest: source for action in actions}) - - # config file sources look like "config_file|" - for source_key in source_to_settings_dict: - if source_key.startswith('config_file'): - update_result(source_to_settings_dict[source_key], ArgumentSource.CONFIG_FILE) - - update_result(source_to_settings_dict.get('env_var', {}), ArgumentSource.ENV_VAR) - - # The command line settings dict is weird, so handle it separately - if 'command_line' in source_to_settings_dict: - settings_dict: Dict[str, Tuple[None, List[str]]] - settings_dict = source_to_settings_dict['command_line'] # type: ignore - (_, args) = settings_dict[''] - args = [arg for arg in args if arg.startswith('-')] - for arg in args: - # find the action corresponding to this arg - for action in self.actions: - if arg in action.option_strings: - result[action.dest] = ArgumentSource.COMMAND_LINE - continue - - return result - - def parse_args(self) -> NamespaceConfig: + def parse_args(self) -> argparse.Namespace: """Parses command line arguments and returns the result. :returns: parsed command line arguments - :rtype: configuration.NamespaceConfig + :rtype: argparse.Namespace """ parsed_args = self.parser.parse_args(self.args) parsed_args.func = self.VERBS[self.verb] parsed_args.verb = self.verb - config = NamespaceConfig(parsed_args, self._build_sources_dict()) - self.remove_config_file_domains_for_renewal(config) + self.remove_config_file_domains_for_renewal(parsed_args) + + if self.detect_defaults: + return parsed_args + + self.defaults = {key: copy.deepcopy(self.parser.get_default(key)) + for key in vars(parsed_args)} # Do any post-parsing homework here if self.verb == "renew": - if config.force_interactive: + if parsed_args.force_interactive: raise errors.Error( "{0} cannot be used with renew".format( constants.FORCE_INTERACTIVE_FLAG)) - config.noninteractive_mode = True + parsed_args.noninteractive_mode = True - if config.force_interactive and config.noninteractive_mode: + if parsed_args.force_interactive and parsed_args.noninteractive_mode: raise errors.Error( "Flag for non-interactive mode and {0} conflict".format( constants.FORCE_INTERACTIVE_FLAG)) - if config.staging or config.dry_run: - self.set_test_server(config) + if parsed_args.staging or parsed_args.dry_run: + self.set_test_server(parsed_args) - if config.csr: - self.handle_csr(config) + if parsed_args.csr: + self.handle_csr(parsed_args) - if config.must_staple and not config.staple: - config.staple = True + if parsed_args.must_staple: + parsed_args.staple = True - if config.validate_hooks: - hooks.validate_hooks(config) + if parsed_args.validate_hooks: + hooks.validate_hooks(parsed_args) - if config.allow_subset_of_names: - if any(util.is_wildcard_domain(d) for d in config.domains): + if parsed_args.allow_subset_of_names: + if any(util.is_wildcard_domain(d) for d in parsed_args.domains): raise errors.Error("Using --allow-subset-of-names with a" " wildcard domain is not supported.") - if config.hsts and config.auto_hsts: + if parsed_args.hsts and parsed_args.auto_hsts: raise errors.Error( "Parameters --hsts and --auto-hsts cannot be used simultaneously.") - if isinstance(config.key_type, list) and len(config.key_type) > 1: + if isinstance(parsed_args.key_type, list) and len(parsed_args.key_type) > 1: raise errors.Error( "Only *one* --key-type type may be provided at this time.") - return config + return parsed_args - def set_test_server(self, config: NamespaceConfig) -> None: + def set_test_server(self, parsed_args: argparse.Namespace) -> None: """We have --staging/--dry-run; perform sanity check and set config.server""" # Flag combinations should produce these results: @@ -281,51 +246,51 @@ class HelpfulArgumentParser: default_servers = (flag_default("server"), constants.STAGING_URI) - if config.staging and config.server not in default_servers: + if parsed_args.staging and parsed_args.server not in default_servers: raise errors.Error("--server value conflicts with --staging") - if config.server == flag_default("server"): - config.server = constants.STAGING_URI + if parsed_args.server in default_servers: + parsed_args.server = constants.STAGING_URI - if config.dry_run: + if parsed_args.dry_run: if self.verb not in ["certonly", "renew"]: raise errors.Error("--dry-run currently only works with the " "'certonly' or 'renew' subcommands (%r)" % self.verb) - config.break_my_certs = config.staging = True - if glob.glob(os.path.join(config.config_dir, constants.ACCOUNTS_DIR, "*")): + parsed_args.break_my_certs = parsed_args.staging = True + if glob.glob(os.path.join(parsed_args.config_dir, constants.ACCOUNTS_DIR, "*")): # The user has a prod account, but might not have a staging # one; we don't want to start trying to perform interactive registration - config.tos = True - config.register_unsafely_without_email = True + parsed_args.tos = True + parsed_args.register_unsafely_without_email = True - def handle_csr(self, config: NamespaceConfig) -> None: + def handle_csr(self, parsed_args: argparse.Namespace) -> None: """Process a --csr flag.""" - if config.verb != "certonly": + if parsed_args.verb != "certonly": raise errors.Error("Currently, a CSR file may only be specified " "when obtaining a new or replacement " "via the certonly command. Please try the " "certonly command instead.") - if config.allow_subset_of_names: + if parsed_args.allow_subset_of_names: raise errors.Error("--allow-subset-of-names cannot be used with --csr") - csrfile, contents = config.csr[0:2] + csrfile, contents = parsed_args.csr[0:2] typ, csr, domains = crypto_util.import_csr_file(csrfile, contents) # This is not necessary for webroot to work, however, - # obtain_certificate_from_csr requires config.domains to be set + # obtain_certificate_from_csr requires parsed_args.domains to be set for domain in domains: - add_domains(config, domain) + add_domains(parsed_args, domain) if not domains: # TODO: add CN to domains instead: raise errors.Error( "Unfortunately, your CSR %s needs to have a SubjectAltName for every domain" - % config.csr[0]) + % parsed_args.csr[0]) - config.actual_csr = (csr, typ) + parsed_args.actual_csr = (csr, typ) csr_domains = {d.lower() for d in domains} - config_domains = set(config.domains) + config_domains = set(parsed_args.domains) if csr_domains != config_domains: raise errors.ConfigurationError( "Inconsistent domain requests:\nFrom the CSR: {0}\nFrom command line/config: {1}" @@ -391,10 +356,6 @@ class HelpfulArgumentParser: :param dict **kwargs: various argparse settings for this argument """ - self.actions.append(self._add(topics, *args, **kwargs)) - - def _add(self, topics: Optional[Union[List[Optional[str]], str]], *args: Any, - **kwargs: Any) -> configargparse.Action: action = kwargs.get("action") if action is util.DeprecatedArgumentAction: # If the argument is deprecated through @@ -405,7 +366,8 @@ class HelpfulArgumentParser: # handling default detection since these actions aren't needed and # can cause bugs like # https://github.com/certbot/certbot/issues/8495. - return self.parser.add_argument(*args, **kwargs) + self.parser.add_argument(*args, **kwargs) + return if isinstance(topics, list): # if this flag can be listed in multiple sections, try to pick the one @@ -414,15 +376,40 @@ class HelpfulArgumentParser: else: topic = topics # there's only one + if self.detect_defaults: + kwargs = self.modify_kwargs_for_default_detection(**kwargs) + if not isinstance(topic, bool) and self.visible_topics[topic]: if topic in self.groups: group = self.groups[topic] - return group.add_argument(*args, **kwargs) + group.add_argument(*args, **kwargs) else: - return self.parser.add_argument(*args, **kwargs) + self.parser.add_argument(*args, **kwargs) else: kwargs["help"] = argparse.SUPPRESS - return self.parser.add_argument(*args, **kwargs) + self.parser.add_argument(*args, **kwargs) + + def modify_kwargs_for_default_detection(self, **kwargs: Any) -> Dict[str, Any]: + """Modify an arg so we can check if it was set by the user. + + Changes the parameters given to argparse when adding an argument + so we can properly detect if the value was set by the user. + + :param dict kwargs: various argparse settings for this argument + + :returns: a modified versions of kwargs + :rtype: dict + + """ + action = kwargs.get("action", None) + if action not in EXIT_ACTIONS: + kwargs["action"] = ("store_true" if action in ZERO_ARG_ACTIONS else + "store") + kwargs["default"] = _Default() + for param in ARGPARSE_PARAMS_TO_REMOVE: + kwargs.pop(param, None) + + return kwargs def add_deprecated_argument(self, argument_name: str, num_args: int) -> None: """Adds a deprecated argument with the name argument_name. diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index e2da44cca..e60ba4f11 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -615,16 +615,15 @@ class Client: for path in cert_path, chain_path, fullchain_path: util.make_or_verify_dir(os.path.dirname(path), 0o755, self.config.strict_permissions) - cert_file, abs_cert_path = _open_pem_file(self.config, 'cert_path', cert_path) + cert_file, abs_cert_path = _open_pem_file('cert_path', cert_path) try: cert_file.write(cert_pem) finally: cert_file.close() - chain_file, abs_chain_path = _open_pem_file(self.config, 'chain_path', chain_path) - fullchain_file, abs_fullchain_path = _open_pem_file( - self.config, 'fullchain_path', fullchain_path) + chain_file, abs_chain_path = _open_pem_file('chain_path', chain_path) + fullchain_file, abs_fullchain_path = _open_pem_file('fullchain_path', fullchain_path) _save_chain(chain_pem, chain_file) _save_chain(cert_pem + chain_pem, fullchain_file) @@ -848,8 +847,7 @@ def rollback(default_installer: str, checkpoints: int, installer.restart() -def _open_pem_file(config: configuration.NamespaceConfig, - cli_arg_path: str, pem_path: str) -> Tuple[IO, str]: +def _open_pem_file(cli_arg_path: str, pem_path: str) -> Tuple[IO, str]: """Open a pem file. If cli_arg_path was set by the client, open that. @@ -861,7 +859,7 @@ def _open_pem_file(config: configuration.NamespaceConfig, :returns: a tuple of file object and its absolute file path """ - if config.set_by_user(cli_arg_path): + if cli.set_by_cli(cli_arg_path): return util.safe_open(pem_path, chmod=0o644, mode="wb"),\ os.path.abspath(pem_path) uniq = util.unique_file(pem_path, 0o644, "wb") diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 422d69a61..880850db0 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -168,7 +168,7 @@ def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig, # If both --key-type and --cert-name are provided, we consider the user's intent to # be unambiguous: to change the key type of this lineage. - is_confirmed_via_cli = config.set_by_user("key_type") and config.set_by_user("certname") + is_confirmed_via_cli = cli.set_by_cli("key_type") and cli.set_by_cli("certname") # Failing that, we interactively prompt the user to confirm the change. if is_confirmed_via_cli or display_util.yesno( @@ -181,7 +181,7 @@ def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig, # If --key-type was set on the CLI but the user did not confirm the key type change using # one of the two above methods, their intent is ambiguous. Error out. - if config.set_by_user("key_type"): + if cli.set_by_cli("key_type"): raise errors.Error( 'Are you trying to change the key type of the certificate named ' f'{cert.lineagename} from {cur_key_type} to {new_key_type}? Please provide ' @@ -1126,13 +1126,13 @@ def _populate_from_certname(config: configuration.NamespaceConfig) -> configurat if not lineage: return config if not config.key_path: - config.key_path = lineage.key_path + config.namespace.key_path = lineage.key_path if not config.cert_path: - config.cert_path = lineage.cert_path + config.namespace.cert_path = lineage.cert_path if not config.chain_path: - config.chain_path = lineage.chain_path + config.namespace.chain_path = lineage.chain_path if not config.fullchain_path: - config.fullchain_path = lineage.fullchain_path + config.namespace.fullchain_path = lineage.fullchain_path return config @@ -1364,7 +1364,7 @@ def revoke(config: configuration.NamespaceConfig, storage.renewal_file_for_certname(config, config.certname), config) config.cert_path = lineage.cert_path # --server takes priority over lineage.server - if lineage.server and not config.set_by_user("server"): + if lineage.server and not cli.set_by_cli("server"): config.server = lineage.server elif not config.cert_path or (config.cert_path and config.certname): # intentionally not supporting --cert-path & --cert-name together, @@ -1843,7 +1843,8 @@ def main(cli_args: Optional[List[str]] = None) -> Optional[Union[str, int]]: misc.prepare_virtual_console() # note: arg parser internally handles --help (and exits afterwards) - config = cli.prepare_and_parse_args(plugins, cli_args) + args = cli.prepare_and_parse_args(plugins, cli_args) + config = configuration.NamespaceConfig(args) # On windows, shell without administrative right cannot create symlinks required by certbot. # So we check the rights before continuing. diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py index ae8542974..2b329dfd8 100644 --- a/certbot/certbot/_internal/renewal.py +++ b/certbot/certbot/_internal/renewal.py @@ -127,11 +127,11 @@ def _restore_webroot_config(config: configuration.NamespaceConfig, restoring logic is not able to correctly parse it from the serialized form. """ - if "webroot_map" in renewalparams and not config.set_by_user("webroot_map"): + 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 config.set_by_user("webroot_path"): + if "webroot_path" in renewalparams and not cli.set_by_cli("webroot_path"): wp = renewalparams["webroot_path"] if isinstance(wp, str): # prior to 0.1.0, webroot_path was a string wp = [wp] @@ -170,7 +170,7 @@ def _restore_plugin_configs(config: configuration.NamespaceConfig, for plugin_prefix in set(plugin_prefixes): plugin_prefix = plugin_prefix.replace('-', '_') for config_item, config_value in renewalparams.items(): - if config_item.startswith(plugin_prefix + "_") and not config.set_by_user(config_item): + if config_item.startswith(plugin_prefix + "_") and not cli.set_by_cli(config_item): # Values None, True, and False need to be treated specially, # As their types aren't handled correctly by configobj if config_value in ("None", "True", "False"): @@ -199,9 +199,9 @@ def restore_required_config_elements(config: configuration.NamespaceConfig, zip(INT_CONFIG_ITEMS, itertools.repeat(_restore_int)), zip(STR_CONFIG_ITEMS, itertools.repeat(_restore_str))) for item_name, restore_func in required_items: - if item_name in renewalparams and not config.set_by_user(item_name): + if item_name in renewalparams and not cli.set_by_cli(item_name): value = restore_func(item_name, renewalparams[item_name]) - setattr(config, item_name, value) + setattr(config.namespace, item_name, value) def _remove_deprecated_config_elements(renewalparams: Mapping[str, Any]) -> Dict[str, Any]: @@ -339,7 +339,7 @@ def _avoid_reuse_key_conflicts(config: configuration.NamespaceConfig, --new-key is also set. """ # If --no-reuse-key is set, no conflict - if config.set_by_user("reuse_key") and not config.reuse_key: + if cli.set_by_cli("reuse_key") and not config.reuse_key: return # If reuse_key is not set on the lineage and --reuse-key is not diff --git a/certbot/certbot/_internal/storage.py b/certbot/certbot/_internal/storage.py index 7ddc5e929..086d3882f 100644 --- a/certbot/certbot/_internal/storage.py +++ b/certbot/certbot/_internal/storage.py @@ -32,6 +32,7 @@ from certbot import errors from certbot import interfaces from certbot import ocsp from certbot import util +from certbot._internal import cli from certbot._internal import constants from certbot._internal import error_handler from certbot._internal.plugins import disco as plugins_disco @@ -216,7 +217,7 @@ def update_configuration(lineagename: str, archive_dir: str, target: Mapping[str os.unlink(temp_filename) # Save only the config items that are relevant to renewal - values = relevant_values(cli_config) + values = relevant_values(vars(cli_config.namespace)) write_renewal_config(config_filename, temp_filename, archive_dir, target, values) filesystem.replace(temp_filename, config_filename) @@ -282,23 +283,22 @@ def _relevant(namespaces: Iterable[str], option: str) -> bool: any(option.startswith(namespace) for namespace in namespaces)) -def relevant_values(config: configuration.NamespaceConfig) -> Dict[str, Any]: +def relevant_values(all_values: Mapping[str, Any]) -> Dict[str, Any]: """Return a new dict containing only items relevant for renewal. - :param .NamespaceConfig config: parsed command line + :param dict all_values: The original values. :returns: A new dictionary containing items that can be used in renewal. :rtype dict: """ - all_values = config.to_dict() plugins = plugins_disco.PluginsRegistry.find_all() namespaces = [plugins_common.dest_namespace(plugin) for plugin in plugins] rv = { option: value for option, value in all_values.items() - if _relevant(namespaces, option) and config.set_by_user(option) + if _relevant(namespaces, option) and cli.option_was_set(option, value) } # We always save the server value to help with forward compatibility # and behavioral consistency when versions of Certbot with different @@ -1121,7 +1121,7 @@ class RenewableCert(interfaces.RenewableCert): config_file.close() # Save only the config items that are relevant to renewal - values = relevant_values(cli_config) + values = relevant_values(vars(cli_config.namespace)) new_config = write_renewal_config(config_filename, config_filename, archive, target, values) diff --git a/certbot/certbot/_internal/tests/cert_manager_test.py b/certbot/certbot/_internal/tests/cert_manager_test.py index b50bfa909..c8483efa7 100644 --- a/certbot/certbot/_internal/tests/cert_manager_test.py +++ b/certbot/certbot/_internal/tests/cert_manager_test.py @@ -236,7 +236,7 @@ class CertificatesTest(BaseCertManagerTest): work_dir=os.path.join(empty_tempdir, "work"), logs_dir=os.path.join(empty_tempdir, "logs"), quiet=False - ), {}) + )) filesystem.makedirs(empty_config.renewal_configs_dir) self._certificates(empty_config) diff --git a/certbot/certbot/_internal/tests/cli_test.py b/certbot/certbot/_internal/tests/cli_test.py index 446b2252f..16e1f7d75 100644 --- a/certbot/certbot/_internal/tests/cli_test.py +++ b/certbot/certbot/_internal/tests/cli_test.py @@ -5,7 +5,6 @@ from importlib import reload as reload_module import io import sys import tempfile -from typing import Any, List import unittest from unittest import mock @@ -13,10 +12,8 @@ import pytest from acme import challenges from certbot import errors -from certbot.configuration import ArgumentSource, NamespaceConfig from certbot._internal import cli from certbot._internal import constants -from certbot._internal.cli.cli_utils import flag_default from certbot._internal.plugins import disco from certbot.compat import filesystem from certbot.compat import os @@ -72,31 +69,25 @@ class FlagDefaultTest(unittest.TestCase): assert cli.flag_default('logs_dir') == 'C:\\Certbot\\log' -def assert_set_by_user_with_value(namespace, attr: str, value: Any): - assert getattr(namespace, attr) == value - assert namespace.set_by_user(attr) - - -def assert_value_and_source(namespace, attr: str, value: Any, source: ArgumentSource): - assert getattr(namespace, attr) == value - assert namespace.argument_sources[attr] == source - - class ParseTest(unittest.TestCase): '''Test the cli args entrypoint''' - @staticmethod - def _unmocked_parse(args: List[str]) -> NamespaceConfig: - """Get result of cli.prepare_and_parse_args.""" - return cli.prepare_and_parse_args(PLUGINS, args) + + def setUp(self): + reload_module(cli) @staticmethod - def parse(args: List[str]) -> NamespaceConfig: + def _unmocked_parse(*args, **kwargs): + """Get result of cli.prepare_and_parse_args.""" + return cli.prepare_and_parse_args(PLUGINS, *args, **kwargs) + + @staticmethod + def parse(*args, **kwargs): """Mocks certbot._internal.display.obj.get_display and calls _unmocked_parse.""" with test_util.patch_display_util(): - return ParseTest._unmocked_parse(args) + return ParseTest._unmocked_parse(*args, **kwargs) - def _help_output(self, args: List[str]): + def _help_output(self, args): "Run a command, and return the output string for scrutiny" output = io.StringIO() @@ -109,7 +100,7 @@ class ParseTest(unittest.TestCase): mock_get_utility().notification.side_effect = write_msg with mock.patch('certbot._internal.main.sys.stderr'): with pytest.raises(SystemExit): - self._unmocked_parse(args) + self._unmocked_parse(args, output) return output.getvalue() @@ -126,19 +117,18 @@ class ParseTest(unittest.TestCase): mock_flag_default.side_effect = shim namespace = self.parse(["certonly"]) - assert_value_and_source(namespace, 'domains', [], ArgumentSource.DEFAULT) + assert namespace.domains == [] with open(tmp_config.name, 'w') as file_h: file_h.write("domains = example.com") namespace = self.parse(["certonly"]) - assert_value_and_source(namespace, 'domains', ["example.com"], ArgumentSource.CONFIG_FILE) + assert namespace.domains == ["example.com"] namespace = self.parse(["renew"]) - assert_value_and_source(namespace, 'domains', [], ArgumentSource.RUNTIME) + assert namespace.domains == [] def test_no_args(self): namespace = self.parse([]) for d in ('config_dir', 'logs_dir', 'work_dir'): assert getattr(namespace, d) == cli.flag_default(d) - assert not namespace.set_by_user(d) def test_install_abspath(self): cert = 'cert' @@ -236,35 +226,35 @@ class ParseTest(unittest.TestCase): def test_parse_domains(self): short_args = ['-d', 'example.com'] namespace = self.parse(short_args) - assert_set_by_user_with_value(namespace, 'domains', ['example.com']) + assert namespace.domains == ['example.com'] short_args = ['-d', 'trailing.period.com.'] namespace = self.parse(short_args) - assert_set_by_user_with_value(namespace, 'domains', ['trailing.period.com']) + assert namespace.domains == ['trailing.period.com'] short_args = ['-d', 'example.com,another.net,third.org,example.com'] namespace = self.parse(short_args) - assert_set_by_user_with_value(namespace, 'domains', - ['example.com', 'another.net', 'third.org']) + assert namespace.domains == ['example.com', 'another.net', + 'third.org'] long_args = ['--domains', 'example.com'] namespace = self.parse(long_args) - assert_set_by_user_with_value(namespace, 'domains', ['example.com']) + assert namespace.domains == ['example.com'] long_args = ['--domains', 'trailing.period.com.'] namespace = self.parse(long_args) - assert_set_by_user_with_value(namespace, 'domains', ['trailing.period.com']) + assert namespace.domains == ['trailing.period.com'] long_args = ['--domains', 'example.com,another.net,example.com'] namespace = self.parse(long_args) - assert_set_by_user_with_value(namespace, 'domains', ['example.com', 'another.net']) + assert namespace.domains == ['example.com', 'another.net'] def test_preferred_challenges(self): short_args = ['--preferred-challenges', 'http, dns'] namespace = self.parse(short_args) expected = [challenges.HTTP01.typ, challenges.DNS01.typ] - assert_set_by_user_with_value(namespace, 'pref_challs', expected) + assert namespace.pref_challs == expected short_args = ['--preferred-challenges', 'jumping-over-the-moon'] # argparse.ArgumentError makes argparse print more information @@ -275,17 +265,13 @@ class ParseTest(unittest.TestCase): def test_server_flag(self): namespace = self.parse('--server example.com'.split()) - assert_set_by_user_with_value(namespace, 'server', 'example.com') + assert namespace.server == 'example.com' def test_must_staple_flag(self): - namespace = self.parse(['--must-staple']) - assert_set_by_user_with_value(namespace, 'must_staple', True) - assert_value_and_source(namespace, 'staple', True, ArgumentSource.RUNTIME) - - def test_must_staple_and_staple_ocsp_flags(self): - namespace = self.parse(['--must-staple', '--staple-ocsp']) - assert_set_by_user_with_value(namespace, 'must_staple', True) - assert_set_by_user_with_value(namespace, 'staple', True) + short_args = ['--must-staple'] + namespace = self.parse(short_args) + assert namespace.must_staple is True + assert namespace.staple is True def _check_server_conflict_message(self, parser_args, conflicting_args): try: @@ -301,24 +287,24 @@ class ParseTest(unittest.TestCase): def test_staging_flag(self): short_args = ['--staging'] namespace = self.parse(short_args) - assert_set_by_user_with_value(namespace, 'staging', True) - assert_set_by_user_with_value(namespace, 'server', constants.STAGING_URI) + assert namespace.staging is True + assert namespace.server == constants.STAGING_URI short_args += '--server example.com'.split() self._check_server_conflict_message(short_args, '--staging') def _assert_dry_run_flag_worked(self, namespace, existing_account): - assert_set_by_user_with_value(namespace, 'dry_run', True) - assert_value_and_source(namespace, 'break_my_certs', True, ArgumentSource.RUNTIME) - assert_value_and_source(namespace, 'staging', True, ArgumentSource.RUNTIME) - assert_value_and_source(namespace, 'server', constants.STAGING_URI, ArgumentSource.RUNTIME) + assert namespace.dry_run is True + assert namespace.break_my_certs is True + assert namespace.staging is True + assert namespace.server == constants.STAGING_URI if existing_account: - assert_value_and_source(namespace, 'tos', True, ArgumentSource.RUNTIME) - assert_value_and_source(namespace, 'register_unsafely_without_email', True, ArgumentSource.RUNTIME) + assert namespace.tos is True + assert namespace.register_unsafely_without_email is True else: - assert_value_and_source(namespace, 'tos', False, ArgumentSource.DEFAULT) - assert_value_and_source(namespace, 'register_unsafely_without_email', False, ArgumentSource.DEFAULT) + assert namespace.tos is False + assert namespace.register_unsafely_without_email is False def test_dry_run_flag(self): config_dir = tempfile.mkdtemp() @@ -344,73 +330,51 @@ class ParseTest(unittest.TestCase): short_args += ['certonly'] # `--dry-run --server example.com` should emit example.com - config = self.parse(short_args + ['--server', 'example.com']) - assert_set_by_user_with_value(config, 'server', 'example.com') + assert self.parse(short_args + ['--server', 'example.com']).server == \ + 'example.com' # `--dry-run --server STAGING_URI` should emit STAGING_URI - config = self.parse(short_args + ['--server', constants.STAGING_URI]) - assert_set_by_user_with_value(config, 'server', constants.STAGING_URI) + assert self.parse(short_args + ['--server', constants.STAGING_URI]).server == \ + constants.STAGING_URI # `--dry-run --server LIVE` should emit STAGING_URI - config = self.parse(short_args + ['--server', cli.flag_default("server")]) - assert_value_and_source(config, 'server', constants.STAGING_URI, ArgumentSource.RUNTIME) + assert self.parse(short_args + ['--server', cli.flag_default("server")]).server == \ + constants.STAGING_URI # `--dry-run --server example.com --staging` should emit an error conflicts = ['--staging'] self._check_server_conflict_message(short_args + ['--server', 'example.com', '--staging'], conflicts) - def test_user_set_rsa_key_size(self): + def test_option_was_set(self): key_size_option = 'rsa_key_size' key_size_value = cli.flag_default(key_size_option) - config = self.parse('--rsa-key-size {0}'.format(key_size_value).split()) + self.parse('--rsa-key-size {0}'.format(key_size_value).split()) - assert config.set_by_user(key_size_option) + assert cli.option_was_set(key_size_option, key_size_value) is True + assert cli.option_was_set('no_verify_ssl', True) is True config_dir_option = 'config_dir' - assert not config.set_by_user( - config_dir_option) - assert not config.set_by_user('authenticator') + assert not cli.option_was_set( + config_dir_option, cli.flag_default(config_dir_option)) + assert not cli.option_was_set( + 'authenticator', cli.flag_default('authenticator')) - def test_user_set_installer_and_authenticator(self): - config = self.parse('--apache') - assert config.set_by_user('installer') - assert config.set_by_user('authenticator') - - config = self.parse('--installer webroot') - assert config.set_by_user('installer') - assert not config.set_by_user('authenticator') - - def test_user_set_ecdsa_key_option(self): + def test_ecdsa_key_option(self): elliptic_curve_option = 'elliptic_curve' elliptic_curve_option_value = cli.flag_default(elliptic_curve_option) - config = self.parse('--elliptic-curve {0}'.format(elliptic_curve_option_value).split()) - assert config.set_by_user(elliptic_curve_option) + self.parse('--elliptic-curve {0}'.format(elliptic_curve_option_value).split()) + assert cli.option_was_set(elliptic_curve_option, elliptic_curve_option_value) is True - def test_user_set_invalid_key_type(self): + def test_invalid_key_type(self): key_type_option = 'key_type' key_type_value = cli.flag_default(key_type_option) - config = self.parse('--key-type {0}'.format(key_type_value).split()) - assert config.set_by_user(key_type_option) + self.parse('--key-type {0}'.format(key_type_value).split()) + assert cli.option_was_set(key_type_option, key_type_value) is True with pytest.raises(SystemExit): self.parse("--key-type foo") - @mock.patch('certbot._internal.hooks.validate_hooks') - def test_user_set_deploy_hook(self, unused_mock_validate_hooks): - args = 'renew --deploy-hook foo'.split() - plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) - assert config.set_by_user('renew_hook') - - @mock.patch('certbot._internal.plugins.webroot._validate_webroot') - def test_user_set_webroot_map(self, mock_validate_webroot): - args = 'renew -w /var/www/html -d example.com'.split() - mock_validate_webroot.return_value = '/var/www/html' - plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) - assert config.set_by_user('webroot_map') - def test_encode_revocation_reason(self): for reason, code in constants.REVOCATION_REASONS.items(): namespace = self.parse(['--reason', reason]) @@ -435,15 +399,15 @@ class ParseTest(unittest.TestCase): namespace = self.parse(["--renew-hook", value, "--deploy-hook", value, "--disable-hook-validation"]) - assert_set_by_user_with_value(namespace, 'deploy_hook', value) - assert_set_by_user_with_value(namespace, 'renew_hook', value) + assert namespace.deploy_hook == value + assert namespace.renew_hook == value def test_deploy_hook_sets_renew_hook(self): value = "foo" namespace = self.parse( ["--deploy-hook", value, "--disable-hook-validation"]) - assert_set_by_user_with_value(namespace, 'deploy_hook', value) - assert_set_by_user_with_value(namespace, 'renew_hook', value) + assert namespace.deploy_hook == value + assert namespace.renew_hook == value def test_renew_hook_conflict(self): with mock.patch("certbot._internal.cli.sys.stderr"): @@ -455,15 +419,15 @@ class ParseTest(unittest.TestCase): namespace = self.parse(["--deploy-hook", value, "--renew-hook", value, "--disable-hook-validation"]) - assert_set_by_user_with_value(namespace, 'deploy_hook', value) - assert_set_by_user_with_value(namespace, 'renew_hook', value) + assert namespace.deploy_hook == value + assert namespace.renew_hook == value def test_renew_hook_does_not_set_renew_hook(self): value = "foo" namespace = self.parse( ["--renew-hook", value, "--disable-hook-validation"]) assert namespace.deploy_hook is None - assert_set_by_user_with_value(namespace, 'renew_hook', value) + assert namespace.renew_hook == value def test_max_log_backups_error(self): with mock.patch('certbot._internal.cli.sys.stderr'): @@ -475,40 +439,37 @@ class ParseTest(unittest.TestCase): def test_max_log_backups_success(self): value = "42" namespace = self.parse(["--max-log-backups", value]) - assert_set_by_user_with_value(namespace, 'max_log_backups', int(value)) + assert namespace.max_log_backups == int(value) def test_unchanging_defaults(self): namespace = self.parse([]) - assert_value_and_source(namespace, 'domains', [], ArgumentSource.DEFAULT) - assert_value_and_source(namespace, 'pref_challs', [], ArgumentSource.DEFAULT) + assert namespace.domains == [] + assert namespace.pref_challs == [] namespace.pref_challs = [challenges.HTTP01.typ] namespace.domains = ['example.com'] namespace = self.parse([]) - assert_value_and_source(namespace, 'domains', [], ArgumentSource.DEFAULT) - assert_value_and_source(namespace, 'pref_challs', [], ArgumentSource.DEFAULT) + assert namespace.domains == [] + assert namespace.pref_challs == [] def test_no_directory_hooks_set(self): - namespace = self.parse(["--no-directory-hooks"]) - assert_set_by_user_with_value(namespace, 'directory_hooks', False) + assert not self.parse(["--no-directory-hooks"]).directory_hooks def test_no_directory_hooks_unset(self): - namespace = self.parse([]) - assert_value_and_source(namespace, 'directory_hooks', True, ArgumentSource.DEFAULT) + assert self.parse([]).directory_hooks is True def test_delete_after_revoke(self): namespace = self.parse(["--delete-after-revoke"]) - assert_set_by_user_with_value(namespace, 'delete_after_revoke', True) + assert namespace.delete_after_revoke is True def test_delete_after_revoke_default(self): namespace = self.parse([]) assert namespace.delete_after_revoke is None - assert not namespace.set_by_user('delete_after_revoke') def test_no_delete_after_revoke(self): namespace = self.parse(["--no-delete-after-revoke"]) - assert_set_by_user_with_value(namespace, 'delete_after_revoke', False) + assert namespace.delete_after_revoke is False def test_allow_subset_with_wildcard(self): with pytest.raises(errors.Error): @@ -519,38 +480,50 @@ class ParseTest(unittest.TestCase): for topic in ['all', 'plugins', 'dns-route53']: assert 'certbot-route53:auth' not in self._help_output([help_flag, topic]) - def test_parse_args_hosts_and_auto_hosts(self): - with pytest.raises(errors.Error): - self.parse(['--hsts', '--auto-hsts']) - def test_parse_with_multiple_argument_sources(self): - DEFAULT_VALUE = flag_default('server') - CONFIG_FILE_VALUE = 'configfile.biz' - COMMAND_LINE_VALUE = 'commandline.edu' +class DefaultTest(unittest.TestCase): + """Tests for certbot._internal.cli._Default.""" - # check that the default is set - namespace = self.parse(['certonly']) - assert_value_and_source(namespace, 'server', DEFAULT_VALUE, ArgumentSource.DEFAULT) - with tempfile.NamedTemporaryFile() as tmp_config: - tmp_config.close() # close now because of compatibility issues on Windows - with open(tmp_config.name, 'w') as file_h: - file_h.write(f'server = {CONFIG_FILE_VALUE}') + def setUp(self): + # pylint: disable=protected-access + self.default1 = cli._Default() + self.default2 = cli._Default() - # first, just provide a value from a config file - namespace = self.parse([ - 'certonly', - '-c', tmp_config.name, - ]) - assert_value_and_source(namespace, 'server', CONFIG_FILE_VALUE, ArgumentSource.CONFIG_FILE) + def test_boolean(self): + assert bool(self.default1) is False + assert bool(self.default2) is False - # now provide config file + command line values - namespace = self.parse([ - 'certonly', - '-c', tmp_config.name, - '--server', COMMAND_LINE_VALUE, - ]) - assert_value_and_source(namespace, 'server', COMMAND_LINE_VALUE, ArgumentSource.COMMAND_LINE) + def test_equality(self): + assert self.default1 == self.default2 + + def test_hash(self): + assert hash(self.default1) == hash(self.default2) + + +class SetByCliTest(unittest.TestCase): + """Tests for certbot.set_by_cli and related functions.""" + + + def setUp(self): + reload_module(cli) + + def test_deploy_hook(self): + assert _call_set_by_cli( + 'renew_hook', '--deploy-hook foo'.split(), 'renew') + + def test_webroot_map(self): + args = '-w /var/www/html -d example.com'.split() + verb = 'renew' + assert _call_set_by_cli('webroot_map', args, verb) is True + + +def _call_set_by_cli(var, args, verb): + with mock.patch('certbot._internal.cli.helpful_parser') as mock_parser: + with test_util.patch_display_util(): + mock_parser.args = args + mock_parser.verb = verb + return cli.set_by_cli(var) if __name__ == '__main__': diff --git a/certbot/certbot/_internal/tests/configuration_test.py b/certbot/certbot/_internal/tests/configuration_test.py index 2ddbd7d1c..9482aec2f 100644 --- a/certbot/certbot/_internal/tests/configuration_test.py +++ b/certbot/certbot/_internal/tests/configuration_test.py @@ -7,9 +7,7 @@ import warnings import pytest from certbot import errors -from certbot._internal import cli from certbot._internal import constants -from certbot._internal.plugins import disco from certbot.compat import misc from certbot.compat import os from certbot.tests import util as test_util @@ -26,10 +24,10 @@ class NamespaceConfigTest(test_util.ConfigTestCase): self.config.http01_port = 4321 def test_init_same_ports(self): - self.config.https_port = 4321 + self.config.namespace.https_port = 4321 from certbot.configuration import NamespaceConfig with pytest.raises(errors.Error): - NamespaceConfig(self.config.namespace, {}) + NamespaceConfig(self.config.namespace) def test_proxy_getattr(self): assert self.config.foo == 'bar' @@ -39,7 +37,7 @@ class NamespaceConfigTest(test_util.ConfigTestCase): assert ['acme-server.org:443', 'new'] == \ self.config.server_path.split(os.path.sep) - self.config.server = ('http://user:pass@acme.server:443' + self.config.namespace.server = ('http://user:pass@acme.server:443' '/p/a/t/h;parameters?query#fragment') assert ['user:pass@acme.server:443', 'p', 'a', 't', 'h'] == \ self.config.server_path.split(os.path.sep) @@ -87,7 +85,7 @@ class NamespaceConfigTest(test_util.ConfigTestCase): mock_namespace.work_dir = work_base mock_namespace.logs_dir = logs_base mock_namespace.server = server - config = NamespaceConfig(mock_namespace, {}) + config = NamespaceConfig(mock_namespace) assert os.path.isabs(config.config_dir) assert config.config_dir == \ @@ -132,7 +130,7 @@ class NamespaceConfigTest(test_util.ConfigTestCase): mock_namespace.config_dir = config_base mock_namespace.work_dir = work_base mock_namespace.logs_dir = logs_base - config = NamespaceConfig(mock_namespace, {}) + config = NamespaceConfig(mock_namespace) assert os.path.isabs(config.default_archive_dir) assert os.path.isabs(config.live_dir) diff --git a/certbot/certbot/_internal/tests/conftest.py b/certbot/certbot/_internal/tests/conftest.py new file mode 100644 index 000000000..64ae64983 --- /dev/null +++ b/certbot/certbot/_internal/tests/conftest.py @@ -0,0 +1,8 @@ +import pytest + +from certbot._internal import cli + + +@pytest.fixture(autouse=True) +def reset_cli_global(): + cli.set_by_cli.detector = None diff --git a/certbot/certbot/_internal/tests/helpful_test.py b/certbot/certbot/_internal/tests/helpful_test.py index 2da2d6cd3..0c928d961 100644 --- a/certbot/certbot/_internal/tests/helpful_test.py +++ b/certbot/certbot/_internal/tests/helpful_test.py @@ -1,8 +1,12 @@ """Tests for certbot.helpful_parser""" import sys +from unittest import mock import pytest +from certbot import errors +from certbot._internal import constants +from certbot._internal.cli import _DomainsAction from certbot._internal.cli import HelpfulArgumentParser @@ -73,7 +77,7 @@ class TestAdd: arg_parser.add(None, "--hello-world") parsed_args = arg_parser.parser.parse_args(['--hello-world', 'Hello World!']) - assert parsed_args.hello_world == 'Hello World!' + assert parsed_args.hello_world is 'Hello World!' assert not hasattr(parsed_args, 'potato') def test_add_expected_argument(self): @@ -113,5 +117,92 @@ class TestAddGroup: assert arg_parser.groups["certonly"] is False +class TestParseArgsErrors: + '''Tests for errors that should be met for some cases in parse_args method + in HelpfulArgumentParser''' + def test_parse_args_renew_force_interactive(self): + arg_parser = HelpfulArgumentParser(['renew', '--force-interactive'], + {}) + arg_parser.add( + None, constants.FORCE_INTERACTIVE_FLAG, action="store_true") + + with pytest.raises(errors.Error): + arg_parser.parse_args() + + def test_parse_args_non_interactive_and_force_interactive(self): + arg_parser = HelpfulArgumentParser(['--force-interactive', + '--non-interactive'], {}) + arg_parser.add( + None, constants.FORCE_INTERACTIVE_FLAG, action="store_true") + arg_parser.add( + None, "--non-interactive", dest="noninteractive_mode", + action="store_true" + ) + + with pytest.raises(errors.Error): + arg_parser.parse_args() + + def test_parse_args_subset_names_wildcard_domain(self): + arg_parser = HelpfulArgumentParser(['--domain', + '*.example.com,potato.example.com', + '--allow-subset-of-names'], {}) + # The following arguments are added because they have to be defined + # in order for arg_parser to run completely. They are not used for the + # test. + arg_parser.add( + None, constants.FORCE_INTERACTIVE_FLAG, action="store_true") + arg_parser.add( + None, "--non-interactive", dest="noninteractive_mode", + action="store_true") + arg_parser.add( + None, "--staging" + ) + arg_parser.add(None, "--dry-run") + arg_parser.add(None, "--csr") + arg_parser.add(None, "--must-staple") + arg_parser.add(None, "--validate-hooks") + + arg_parser.add(None, "-d", "--domain", dest="domains", + metavar="DOMAIN", action=_DomainsAction) + arg_parser.add(None, "--allow-subset-of-names") + # with self.assertRaises(errors.Error): + # arg_parser.parse_args() + + def test_parse_args_hosts_and_auto_hosts(self): + arg_parser = HelpfulArgumentParser(['--hsts', '--auto-hsts'], {}) + + arg_parser.add( + None, "--hsts", action="store_true", dest="hsts") + arg_parser.add( + None, "--auto-hsts", action="store_true", dest="auto_hsts") + # The following arguments are added because they have to be defined + # in order for arg_parser to run completely. They are not used for the + # test. + arg_parser.add( + None, constants.FORCE_INTERACTIVE_FLAG, action="store_true") + arg_parser.add( + None, "--non-interactive", dest="noninteractive_mode", + action="store_true") + arg_parser.add(None, "--staging") + arg_parser.add(None, "--dry-run") + arg_parser.add(None, "--csr") + arg_parser.add(None, "--must-staple") + arg_parser.add(None, "--validate-hooks") + arg_parser.add(None, "--allow-subset-of-names") + with pytest.raises(errors.Error): + arg_parser.parse_args() + + +class TestAddDeprecatedArgument: + """Tests for add_deprecated_argument method of HelpfulArgumentParser""" + + @mock.patch.object(HelpfulArgumentParser, "modify_kwargs_for_default_detection") + def test_no_default_detection_modifications(self, mock_modify): + arg_parser = HelpfulArgumentParser(["run"], {}, detect_defaults=True) + arg_parser.add_deprecated_argument("--foo", 0) + arg_parser.parse_args() + mock_modify.assert_not_called() + + if __name__ == '__main__': sys.exit(pytest.main(sys.argv[1:] + [__file__])) # pragma: no cover diff --git a/certbot/certbot/_internal/tests/main_test.py b/certbot/certbot/_internal/tests/main_test.py index c67958fcd..7f90ee25c 100644 --- a/certbot/certbot/_internal/tests/main_test.py +++ b/certbot/certbot/_internal/tests/main_test.py @@ -21,6 +21,7 @@ import pytest import pytz from acme.messages import Error as acme_error +from certbot import configuration from certbot import crypto_util from certbot import errors from certbot import interfaces @@ -85,10 +86,9 @@ class TestHandleCerts(unittest.TestCase): assert mock_handle_migration.called @mock.patch("certbot._internal.main.display_util.yesno") - def test_handle_unexpected_key_type_migration(self, mock_yesno): + @mock.patch("certbot._internal.main.cli.set_by_cli") + def test_handle_unexpected_key_type_migration(self, mock_set, mock_yesno): config = mock.Mock() - mock_set = mock.Mock() - config.set_by_user = mock_set cert = mock.Mock() # If the key types do not differ, it should be a no-op. @@ -163,7 +163,8 @@ class RunTest(test_util.ConfigTestCase): def _call(self): args = '-a webroot -i null -d {0}'.format(self.domain).split() plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, args)) from certbot._internal.main import run run(config, plugins) @@ -230,7 +231,8 @@ class CertonlyTest(unittest.TestCase): def _call(self, args): plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, args)) with mock.patch('certbot._internal.main._init_le_client') as mock_init: with mock.patch('certbot._internal.main._suggest_donation_if_appropriate'): @@ -431,11 +433,19 @@ class RevokeTest(test_util.TempDirTestCase): args = 'revoke --cert-path={0} ' args = args.format(self.tmp_cert_path).split() plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, args)) from certbot._internal.main import revoke revoke(config, plugins) + def _mock_set_by_cli(self, mocked: mock.MagicMock, key: str, value: bool) -> None: + def set_by_cli(k: str) -> bool: + if key == k: + return value + return mock.DEFAULT + mocked.side_effect = set_by_cli + @mock.patch('certbot._internal.main._delete_if_appropriate') @mock.patch('certbot._internal.main.client.acme_client') def test_revoke_with_reason(self, mock_acme_client, @@ -457,9 +467,11 @@ class RevokeTest(test_util.TempDirTestCase): @mock.patch('certbot._internal.storage.RenewableCert') @mock.patch('certbot._internal.storage.renewal_file_for_certname') @mock.patch('certbot._internal.client.acme_from_config_key') - def test_revoke_by_certname(self, mock_acme_from_config, + @mock.patch('certbot._internal.cli.set_by_cli') + def test_revoke_by_certname(self, mock_set_by_cli, mock_acme_from_config, unused_mock_renewal_file_for_certname, mock_cert, mock_delete_if_appropriate): + self._mock_set_by_cli(mock_set_by_cli, "server", False) mock_acme_from_config.return_value = self.mock_acme_client mock_cert.return_value = mock.MagicMock(cert_path=self.tmp_cert_path, server="https://acme.example") @@ -474,10 +486,12 @@ class RevokeTest(test_util.TempDirTestCase): @mock.patch('certbot._internal.storage.RenewableCert') @mock.patch('certbot._internal.storage.renewal_file_for_certname') @mock.patch('certbot._internal.client.acme_from_config_key') - def test_revoke_by_certname_and_server(self, mock_acme_from_config, + @mock.patch('certbot._internal.cli.set_by_cli') + def test_revoke_by_certname_and_server(self, mock_set_by_cli, mock_acme_from_config, unused_mock_renewal_file_for_certname, mock_cert, mock_delete_if_appropriate): """Revoking with --server should use the server from the CLI""" + self._mock_set_by_cli(mock_set_by_cli, "server", True) mock_cert.return_value = mock.MagicMock(cert_path=self.tmp_cert_path, server="https://acme.example") args = 'revoke --cert-name=example.com --server https://other.example'.split() @@ -491,7 +505,8 @@ class RevokeTest(test_util.TempDirTestCase): @mock.patch('certbot._internal.storage.RenewableCert') @mock.patch('certbot._internal.storage.renewal_file_for_certname') @mock.patch('certbot._internal.client.acme_from_config_key') - def test_revoke_by_certname_empty_server(self, mock_acme_from_config, + @mock.patch('certbot._internal.cli.set_by_cli') + def test_revoke_by_certname_empty_server(self, mock_set_by_cli, mock_acme_from_config, unused_mock_renewal_file_for_certname, mock_cert, mock_delete_if_appropriate): """Revoking with --cert-name where the lineage server is empty shouldn't crash """ @@ -584,7 +599,8 @@ class ReconfigureTest(test_util.TempDirTestCase): def _call(self, passed_args): full_args = passed_args + ['--config-dir', self.config_dir] plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, full_args) + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, full_args)) from certbot._internal.main import reconfigure reconfigure(config, plugins) @@ -600,12 +616,6 @@ class ReconfigureTest(test_util.TempDirTestCase): @mock.patch('certbot._internal.cert_manager.get_certnames') def test_asks_for_certname(self, mock_cert_manager): - named_mock = mock.Mock() - named_mock.name = 'nginx' - - self.mocks['pick_installer'].return_value = named_mock - self.mocks['pick_auth'].return_value = named_mock - self.mocks['find_init'].return_value = named_mock mock_cert_manager.return_value = ['example.com'] self._call('--nginx'.split()) assert mock_cert_manager.call_count == 1 @@ -764,7 +774,7 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): mock_match_and_check_overlaps, mock_renewal_file_for_certname): # pylint: disable = unused-argument config = self.config - config.noninteractive_mode = True + config.namespace.noninteractive_mode = True config.cert_path = "/some/reasonable/path" config.certname = "" mock_cert_path_to_lineage.return_value = "example.com" @@ -783,7 +793,7 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): mock_cert_path_to_lineage, mock_full_archive_dir, mock_match_and_check_overlaps, mock_renewal_file_for_certname): config = self.config - config.delete_after_revoke = True + config.namespace.delete_after_revoke = True config.cert_path = "/some/reasonable/path" config.certname = "" mock_cert_path_to_lineage.return_value = "example.com" @@ -1922,7 +1932,8 @@ class EnhanceTest(test_util.ConfigTestCase): def _call(self, args): plugins = disco.PluginsRegistry.find_all() - config = cli.prepare_and_parse_args(plugins, args) + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, args)) with mock.patch('certbot._internal.cert_manager.get_certnames') as mock_certs: mock_certs.return_value = ['example.com'] diff --git a/certbot/certbot/_internal/tests/plugins/selection_test.py b/certbot/certbot/_internal/tests/plugins/selection_test.py index fa325dbac..ee776d1e8 100644 --- a/certbot/certbot/_internal/tests/plugins/selection_test.py +++ b/certbot/certbot/_internal/tests/plugins/selection_test.py @@ -222,7 +222,8 @@ class TestChooseConfiguratorPlugins(unittest.TestCase): def _parseArgs(self, args): from certbot import configuration from certbot._internal import cli - return cli.prepare_and_parse_args(self.plugins, args.split()) + return configuration.NamespaceConfig( + cli.prepare_and_parse_args(self.plugins, args.split())) def setUp(self): self.plugins = PluginsRegistry({ diff --git a/certbot/certbot/_internal/tests/renewal_test.py b/certbot/certbot/_internal/tests/renewal_test.py index 1b9605b16..f11b01603 100644 --- a/certbot/certbot/_internal/tests/renewal_test.py +++ b/certbot/certbot/_internal/tests/renewal_test.py @@ -14,15 +14,15 @@ import certbot.tests.util as test_util class RenewalTest(test_util.ConfigTestCase): - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_ancient_webroot_renewal_conf(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.cli.set_by_cli') + def test_ancient_webroot_renewal_conf(self, mock_set_by_cli): + mock_set_by_cli.return_value = False rc_path = test_util.make_lineage( self.config.config_dir, 'sample-renewal-ancient.conf') self.config.account = None self.config.email = None self.config.webroot_path = None - config = configuration.NamespaceConfig(self.config, {}) + config = configuration.NamespaceConfig(self.config) lineage = storage.RenewableCert(rc_path, config) renewalparams = lineage.configuration['renewalparams'] # pylint: disable=protected-access @@ -30,13 +30,13 @@ class RenewalTest(test_util.ConfigTestCase): renewal._restore_webroot_config(config, renewalparams) assert config.webroot_path == ['/var/www/'] - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_webroot_params_conservation(self, mock_set_by_user): + @mock.patch('certbot._internal.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._internal.plugins.webroot_test:: # WebrootActionTest::test_webroot_map_partial_without_perform from certbot._internal import renewal - mock_set_by_user.return_value = False + mock_set_by_cli.return_value = False renewalparams = { 'webroot_map': {'test.example.com': '/var/www/test'}, @@ -59,7 +59,7 @@ class RenewalTest(test_util.ConfigTestCase): self.config.elliptic_curve = 'INVALID_VALUE' self.config.reuse_key = True self.config.dry_run = True - config = configuration.NamespaceConfig(self.config, {}) + config = configuration.NamespaceConfig(self.config) rc_path = test_util.make_lineage( self.config.config_dir, 'sample-renewal.conf') @@ -81,7 +81,7 @@ class RenewalTest(test_util.ConfigTestCase): self.config.reuse_key = True self.config.dry_run = True self.config.key_type = 'ecdsa' - config = configuration.NamespaceConfig(self.config, {}) + config = configuration.NamespaceConfig(self.config) rc_path = test_util.make_lineage( self.config.config_dir, @@ -100,15 +100,15 @@ class RenewalTest(test_util.ConfigTestCase): assert self.config.elliptic_curve == 'secp256r1' - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_new_key(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_new_key(self, mock_set_by_cli): + mock_set_by_cli.return_value = False # When renewing with both reuse_key and new_key, the key should be regenerated, # the key type, key parameters and reuse_key should be kept. self.config.reuse_key = True self.config.new_key = True self.config.dry_run = True - config = configuration.NamespaceConfig(self.config, {}) + config = configuration.NamespaceConfig(self.config) rc_path = test_util.make_lineage( self.config.config_dir, 'sample-renewal.conf') @@ -129,9 +129,9 @@ class RenewalTest(test_util.ConfigTestCase): le_client.obtain_certificate.assert_called_with(mock.ANY, None) @mock.patch('certbot._internal.renewal.hooks.renew_hook') - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_reuse_key_conflicts(self, mock_set_by_user, unused_mock_renew_hook): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_reuse_key_conflicts(self, mock_set_by_cli, unused_mock_renew_hook): + mock_set_by_cli.return_value = False # When renewing with reuse_key and a conflicting key parameter (size, curve) # an error should be raised ... @@ -140,7 +140,7 @@ class RenewalTest(test_util.ConfigTestCase): self.config.rsa_key_size = 4096 self.config.dry_run = True - config = configuration.NamespaceConfig(self.config, {}) + config = configuration.NamespaceConfig(self.config) rc_path = test_util.make_lineage( self.config.config_dir, 'sample-renewal.conf') @@ -156,15 +156,15 @@ class RenewalTest(test_util.ConfigTestCase): renewal.renew_cert(self.config, None, le_client, lineage) # ... unless --no-reuse-key is set - mock_set_by_user.side_effect = lambda var: var == "reuse_key" + mock_set_by_cli.side_effect = lambda var: var == "reuse_key" self.config.reuse_key = False renewal.renew_cert(self.config, None, le_client, lineage) @test_util.patch_display_util() - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_remove_deprecated_config_elements(self, mock_set_by_user, unused_mock_get_utility): - mock_set_by_user.return_value = False - config = configuration.NamespaceConfig(self.config, {}) + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_remove_deprecated_config_elements(self, mock_set_by_cli, unused_mock_get_utility): + mock_set_by_cli.return_value = False + config = configuration.NamespaceConfig(self.config) config.certname = "sample-renewal-deprecated-option" rc_path = test_util.make_lineage( @@ -177,9 +177,9 @@ class RenewalTest(test_util.ConfigTestCase): # value in the renewal conf file assert isinstance(lineage_config.manual_public_ip_logging_ok, mock.MagicMock) - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_absent_key_type_restored(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_absent_key_type_restored(self, mock_set_by_cli): + mock_set_by_cli.return_value = False rc_path = test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False) @@ -196,60 +196,60 @@ class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): from certbot._internal.renewal import restore_required_config_elements return restore_required_config_elements(*args, **kwargs) - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_allow_subset_of_names_success(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_allow_subset_of_names_success(self, mock_set_by_cli): + mock_set_by_cli.return_value = False self._call(self.config, {'allow_subset_of_names': 'True'}) assert self.config.allow_subset_of_names is True - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_allow_subset_of_names_failure(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_allow_subset_of_names_failure(self, mock_set_by_cli): + mock_set_by_cli.return_value = False renewalparams = {'allow_subset_of_names': 'maybe'} with pytest.raises(errors.Error): self._call(self.config, renewalparams) - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_pref_challs_list(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_pref_challs_list(self, mock_set_by_cli): + mock_set_by_cli.return_value = False renewalparams = {'pref_challs': 'http-01, dns'.split(',')} self._call(self.config, renewalparams) expected = [challenges.HTTP01.typ, challenges.DNS01.typ] assert self.config.pref_challs == expected - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_pref_challs_str(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_pref_challs_str(self, mock_set_by_cli): + mock_set_by_cli.return_value = False renewalparams = {'pref_challs': 'dns'} self._call(self.config, renewalparams) expected = [challenges.DNS01.typ] assert self.config.pref_challs == expected - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_pref_challs_failure(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_pref_challs_failure(self, mock_set_by_cli): + mock_set_by_cli.return_value = False renewalparams = {'pref_challs': 'finding-a-shrubbery'} with pytest.raises(errors.Error): self._call(self.config, renewalparams) - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_must_staple_success(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_must_staple_success(self, mock_set_by_cli): + mock_set_by_cli.return_value = False self._call(self.config, {'must_staple': 'True'}) assert self.config.must_staple is True - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_must_staple_failure(self, mock_set_by_user): - mock_set_by_user.return_value = False + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_must_staple_failure(self, mock_set_by_cli): + mock_set_by_cli.return_value = False renewalparams = {'must_staple': 'maybe'} with pytest.raises(errors.Error): self._call(self.config, renewalparams) - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - def test_ancient_server_renewal_conf(self, mock_set_by_user): + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_ancient_server_renewal_conf(self, mock_set_by_cli): from certbot._internal import constants self.config.server = None - mock_set_by_user.return_value = False + mock_set_by_cli.return_value = False self._call(self.config, {'server': constants.V1_URI}) assert self.config.server == constants.CLI_DEFAULTS['server'] diff --git a/certbot/certbot/_internal/tests/storage_test.py b/certbot/certbot/_internal/tests/storage_test.py index 3b4075478..34ea95246 100644 --- a/certbot/certbot/_internal/tests/storage_test.py +++ b/certbot/certbot/_internal/tests/storage_test.py @@ -12,7 +12,6 @@ import pytest import pytz import certbot -from certbot import configuration from certbot import errors from certbot._internal.storage import ALL_FOUR from certbot.compat import filesystem @@ -40,24 +39,23 @@ class RelevantValuesTest(unittest.TestCase): def setUp(self): self.values = {"server": "example.org", "key_type": "rsa"} - self.mock_config = mock.MagicMock() - self.mock_config.set_by_user = mock.MagicMock() - def _call(self, values): + def _call(self, *args, **kwargs): from certbot._internal.storage import relevant_values - self.mock_config.to_dict.return_value = values - return relevant_values(self.mock_config) + return relevant_values(*args, **kwargs) + @mock.patch("certbot._internal.cli.option_was_set") @mock.patch("certbot._internal.plugins.disco.PluginsRegistry.find_all") - def test_namespace(self, mock_find_all): + def test_namespace(self, mock_find_all, mock_option_was_set): mock_find_all.return_value = ["certbot-foo:bar"] - self.mock_config.set_by_user.return_value = True + mock_option_was_set.return_value = True self.values["certbot_foo:bar_baz"] = 42 assert self._call(self.values.copy()) == self.values - def test_option_set(self): - self.mock_config.set_by_user.return_value = True + @mock.patch("certbot._internal.cli.option_was_set") + def test_option_set(self, mock_option_was_set): + mock_option_was_set.return_value = True self.values["allow_subset_of_names"] = True self.values["authenticator"] = "apache" @@ -67,46 +65,26 @@ class RelevantValuesTest(unittest.TestCase): assert self._call(self.values) == expected_relevant_values - def test_option_unset(self): - self.mock_config.set_by_user.return_value = False + @mock.patch("certbot._internal.cli.option_was_set") + def test_option_unset(self, mock_option_was_set): + mock_option_was_set.return_value = False expected_relevant_values = self.values.copy() self.values["rsa_key_size"] = 2048 assert self._call(self.values) == expected_relevant_values - def test_deprecated_item(self): - deprected_option = 'manual_public_ip_logging_ok' - self.mock_config.set_by_user = lambda v: False if v == deprected_option else True + @mock.patch("certbot._internal.cli.set_by_cli") + def test_deprecated_item(self, unused_mock_set_by_cli): # deprecated items should never be relevant to store expected_relevant_values = self.values.copy() - self.values[deprected_option] = None + self.values["manual_public_ip_logging_ok"] = None assert self._call(self.values) == expected_relevant_values - self.values[deprected_option] = True + self.values["manual_public_ip_logging_ok"] = True assert self._call(self.values) == expected_relevant_values - self.values[deprected_option] = False + self.values["manual_public_ip_logging_ok"] = False assert self._call(self.values) == expected_relevant_values - def test_with_real_parser(self): - from certbot._internal.storage import relevant_values - from certbot._internal.plugins import disco - from certbot._internal import cli - from certbot._internal import constants - - PLUGINS = disco.PluginsRegistry.find_all() - namespace = cli.prepare_and_parse_args(PLUGINS, [ - '--allow-subset-of-names', - '--authenticator', 'apache', - ]) - expected_relevant_values = { - 'server': constants.CLI_DEFAULTS['server'], - 'key_type': 'ecdsa', - 'allow_subset_of_names': True, - 'authenticator': 'apache', - } - - assert relevant_values(namespace) == expected_relevant_values - class BaseRenewableCertTest(test_util.ConfigTestCase): """Base class for setting up Renewable Cert tests. @@ -441,9 +419,9 @@ class RenewableCertTests(BaseRenewableCertTest): with pytest.raises(errors.CertStorageError): self.test_rc.names() - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') + @mock.patch("certbot._internal.storage.cli") @mock.patch("certbot._internal.storage.datetime") - def test_time_interval_judgments(self, mock_datetime, mock_set_by_user): + def test_time_interval_judgments(self, mock_datetime, mock_cli): """Test should_autorenew() on the basis of expiry time windows.""" test_cert = test_util.load_vector("cert_512.pem") @@ -457,7 +435,7 @@ class RenewableCertTests(BaseRenewableCertTest): f.write(test_cert) mock_datetime.timedelta = datetime.timedelta - mock_set_by_user.return_value = False + mock_cli.set_by_cli.return_value = False self.test_rc.configuration["renewalparams"] = {} for (current_time, interval, result) in [ @@ -495,12 +473,12 @@ class RenewableCertTests(BaseRenewableCertTest): self.test_rc.configuration["renewalparams"]["autorenew"] = "False" assert not self.test_rc.autorenewal_is_enabled() - @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') + @mock.patch("certbot._internal.storage.cli") @mock.patch("certbot._internal.storage.RenewableCert.ocsp_revoked") - def test_should_autorenew(self, mock_ocsp, mock_set_by_user): + def test_should_autorenew(self, mock_ocsp, mock_cli): """Test should_autorenew on the basis of reasons other than expiry time window.""" - mock_set_by_user.return_value = False + mock_cli.set_by_cli.return_value = False # Autorenewal turned off self.test_rc.configuration["renewalparams"] = {"autorenew": "False"} assert not self.test_rc.should_autorenew() @@ -516,7 +494,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_save_successor(self, mock_rv): # Mock relevant_values() to claim that all values are relevant here # (to avoid instantiating parser) - mock_rv.side_effect = lambda x: x.to_dict() + mock_rv.side_effect = lambda x: x for ver in range(1, 6): for kind in ALL_FOUR: @@ -575,7 +553,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_save_successor_maintains_group_mode(self, mock_rv): # Mock relevant_values() to claim that all values are relevant here # (to avoid instantiating parser) - mock_rv.side_effect = lambda x: x.to_dict() + mock_rv.side_effect = lambda x: x for kind in ALL_FOUR: self._write_out_kind(kind, 1) self.test_rc.update_all_links_to(1) @@ -597,7 +575,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_save_successor_maintains_gid(self, mock_ownership, mock_rv): # Mock relevant_values() to claim that all values are relevant here # (to avoid instantiating parser) - mock_rv.side_effect = lambda x: x.to_dict() + mock_rv.side_effect = lambda x: x for kind in ALL_FOUR: self._write_out_kind(kind, 1) self.test_rc.update_all_links_to(1) @@ -611,7 +589,7 @@ class RenewableCertTests(BaseRenewableCertTest): """Test for new_lineage() class method.""" # Mock relevant_values to say everything is relevant here (so we # don't have to mock the parser to help it decide!) - mock_rv.side_effect = lambda x: x.to_dict() + mock_rv.side_effect = lambda x: x from certbot._internal import storage result = storage.RenewableCert.new_lineage( @@ -665,7 +643,7 @@ class RenewableCertTests(BaseRenewableCertTest): """Test that directories can be created if they don't exist.""" # Mock relevant_values to say everything is relevant here (so we # don't have to mock the parser to help it decide!) - mock_rv.side_effect = lambda x: x.to_dict() + mock_rv.side_effect = lambda x: x from certbot._internal import storage shutil.rmtree(self.config.renewal_configs_dir) diff --git a/certbot/certbot/configuration.py b/certbot/certbot/configuration.py index 7e77b10ba..9e0d3f9de 100644 --- a/certbot/certbot/configuration.py +++ b/certbot/certbot/configuration.py @@ -1,10 +1,7 @@ """Certbot user-supplied configuration.""" import argparse import copy -import enum -import logging from typing import Any -from typing import Dict from typing import List from typing import Optional from urllib import parse @@ -17,24 +14,6 @@ from certbot.compat import misc from certbot.compat import os -logger = logging.getLogger(__name__) - - -class ArgumentSource(enum.Enum): - """Enum for describing where a configuration argument was set.""" - - COMMAND_LINE = enum.auto() - """Argument was specified on the command line""" - CONFIG_FILE = enum.auto() - """Argument was specified in a .ini config file""" - DEFAULT = enum.auto() - """Argument was not set by the user, and was assigned its default value""" - ENV_VAR = enum.auto() - """Argument was specified in an environment variable""" - RUNTIME = enum.auto() - """Argument was set at runtime by certbot""" - - class NamespaceConfig: """Configuration wrapper around :class:`argparse.Namespace`. @@ -59,17 +38,13 @@ class NamespaceConfig: :ivar namespace: Namespace typically produced by :meth:`argparse.ArgumentParser.parse_args`. :type namespace: :class:`argparse.Namespace` - :ivar argument_sources: dictionary of argument names to their :class:`ArgumentSource` - :type argument_sources: :class:`Dict[str, ArgumentSource]` """ - def __init__(self, namespace: argparse.Namespace, - argument_sources: Dict[str, ArgumentSource]) -> None: + def __init__(self, namespace: argparse.Namespace) -> None: self.namespace: argparse.Namespace # Avoid recursion loop because of the delegation defined in __setattr__ object.__setattr__(self, 'namespace', namespace) - object.__setattr__(self, 'argument_sources', argument_sources) self.namespace.config_dir = os.path.abspath(self.namespace.config_dir) self.namespace.work_dir = os.path.abspath(self.namespace.work_dir) @@ -78,61 +53,12 @@ class NamespaceConfig: # Check command line parameters sanity, and error out in case of problem. _check_config_sanity(self) - def set_by_user(self, var: str) -> bool: - """ - Return True if a particular config variable has been set by the user - (via CLI or config file) including if the user explicitly set it to the - default, or if it was dynamically set at runtime. Returns False if the - variable was assigned a default value. - """ - from certbot._internal.cli.cli_constants import DEPRECATED_OPTIONS - from certbot._internal.cli.cli_constants import VAR_MODIFIERS - from certbot._internal.plugins import selection - - # We should probably never actually hit this code. But if we do, - # a deprecated option has logically never been set by the CLI. - if var in DEPRECATED_OPTIONS: - return False - - if var in ['authenticator', 'installer']: - auth, inst = selection.cli_plugin_requests(self) - if var == 'authenticator': - return auth is not None - if var == 'installer': - return inst is not None - - if var in self.argument_sources and self.argument_sources[var] != ArgumentSource.DEFAULT: - logger.debug("Var %s=%s (set by user).", var, getattr(self, var)) - return True - - for modifier in VAR_MODIFIERS.get(var, []): - if self.set_by_user(modifier): - logger.debug("Var %s=%s (set by user).", - var, VAR_MODIFIERS.get(var, [])) - return True - - return False - - def to_dict(self) -> Dict[str, Any]: - """ - Returns a dictionary mapping all argument names to their values - """ - return vars(self.namespace) - - def _mark_runtime_override(self, name: str) -> None: - """ - Overwrites an argument's source to be ArgumentSource.RUNTIME. Used when certbot sets an - argument's values at runtime. - """ - self.argument_sources[name] = ArgumentSource.RUNTIME - # Delegate any attribute not explicitly defined to the underlying namespace object. def __getattr__(self, name: str) -> Any: return getattr(self.namespace, name) def __setattr__(self, name: str, value: Any) -> None: - self._mark_runtime_override(name) setattr(self.namespace, name, value) @property @@ -142,7 +68,6 @@ class NamespaceConfig: @server.setter def server(self, server_: str) -> None: - self._mark_runtime_override('server') self.namespace.server = server_ @property @@ -156,7 +81,6 @@ class NamespaceConfig: @email.setter def email(self, mail: str) -> None: - self._mark_runtime_override('email') self.namespace.email = mail @property @@ -167,7 +91,6 @@ class NamespaceConfig: @rsa_key_size.setter def rsa_key_size(self, ksize: int) -> None: """Set the rsa_key_size property""" - self._mark_runtime_override('rsa_key_size') self.namespace.rsa_key_size = ksize @property @@ -181,7 +104,6 @@ class NamespaceConfig: @elliptic_curve.setter def elliptic_curve(self, ecurve: str) -> None: """Set the elliptic_curve property""" - self._mark_runtime_override('elliptic_curve') self.namespace.elliptic_curve = ecurve @property @@ -195,7 +117,6 @@ class NamespaceConfig: @key_type.setter def key_type(self, ktype: str) -> None: """Set the key_type property""" - self._mark_runtime_override('key_type') self.namespace.key_type = ktype @property @@ -401,8 +322,7 @@ class NamespaceConfig: # Work around https://bugs.python.org/issue1515 for py26 tests :( :( # https://travis-ci.org/letsencrypt/letsencrypt/jobs/106900743#L3276 new_ns = copy.deepcopy(self.namespace) - new_sources = copy.deepcopy(self.argument_sources) - return type(self)(new_ns, new_sources) + return type(self)(new_ns) def _check_config_sanity(config: NamespaceConfig) -> None: diff --git a/certbot/certbot/tests/util.py b/certbot/certbot/tests/util.py index 8ff7155b6..5db26c4cd 100644 --- a/certbot/certbot/tests/util.py +++ b/certbot/certbot/tests/util.py @@ -395,8 +395,7 @@ class ConfigTestCase(TempDirTestCase): def setUp(self) -> None: super().setUp() self.config = configuration.NamespaceConfig( - mock.MagicMock(**constants.CLI_DEFAULTS), - {}, + mock.MagicMock(**constants.CLI_DEFAULTS) ) self.config.namespace.verb = "certonly" self.config.namespace.config_dir = os.path.join(self.tempdir, 'config') diff --git a/tools/oldest_constraints.txt b/tools/oldest_constraints.txt index 9c3cc3aea..19d765f03 100644 --- a/tools/oldest_constraints.txt +++ b/tools/oldest_constraints.txt @@ -2,7 +2,7 @@ # that script. apacheconfig==0.3.2 ; python_full_version < "3.8.0" and python_version >= "3.7" asn1crypto==0.24.0 ; python_full_version >= "3.7.0" and python_full_version < "3.8.0" -astroid==2.15.3 ; python_full_version >= "3.7.2" and python_full_version < "3.8.0" +astroid==2.15.2 ; python_full_version >= "3.7.2" and python_full_version < "3.8.0" boto3==1.15.15 ; python_full_version < "3.8.0" and python_version >= "3.7" botocore==1.18.15 ; python_full_version < "3.8.0" and python_version >= "3.7" cachetools==5.3.0 ; python_version >= "3.7" and python_full_version < "3.8.0" @@ -11,7 +11,7 @@ cffi==1.11.5 ; python_full_version < "3.8.0" and python_version >= "3.7" chardet==3.0.4 ; python_full_version < "3.8.0" and python_version >= "3.7" cloudflare==1.5.1 ; python_full_version < "3.8.0" and python_version >= "3.7" colorama==0.4.6 ; python_full_version < "3.8.0" and sys_platform == "win32" and python_version >= "3.7" -configargparse==1.5.3 ; python_full_version < "3.8.0" and python_version >= "3.7" +configargparse==0.10.0 ; python_full_version < "3.8.0" and python_version >= "3.7" configobj==5.0.6 ; python_full_version < "3.8.0" and python_version >= "3.7" coverage==7.2.3 ; python_version >= "3.7" and python_full_version < "3.8.0" cryptography==3.2.1 ; python_full_version < "3.8.0" and python_version >= "3.7" @@ -86,7 +86,7 @@ types-python-dateutil==2.8.19.12 ; python_version >= "3.7" and python_full_versi types-pytz==2023.3.0.0 ; python_version >= "3.7" and python_full_version < "3.8.0" types-pywin32==306.0.0.1 ; python_version >= "3.7" and python_full_version < "3.8.0" types-requests==2.28.11.17 ; python_version >= "3.7" and python_full_version < "3.8.0" -types-setuptools==67.6.0.8 ; python_version >= "3.7" and python_full_version < "3.8.0" +types-setuptools==67.6.0.7 ; python_version >= "3.7" and python_full_version < "3.8.0" types-six==1.16.21.8 ; python_version >= "3.7" and python_full_version < "3.8.0" types-urllib3==1.26.25.10 ; python_version >= "3.7" and python_full_version < "3.8.0" typing-extensions==4.5.0 ; python_version < "3.8" and python_version >= "3.7" diff --git a/tools/pinning/oldest/pyproject.toml b/tools/pinning/oldest/pyproject.toml index 7063e7d0b..8fb13dc65 100644 --- a/tools/pinning/oldest/pyproject.toml +++ b/tools/pinning/oldest/pyproject.toml @@ -43,7 +43,7 @@ acme = {path = "../../../acme", extras = ["test"]} # dependency should be updated in our setup.py files as well to communicate # this information to our users. -ConfigArgParse = "1.5.3" +ConfigArgParse = "0.10.0" apacheconfig = "0.3.2" asn1crypto = "0.24.0" boto3 = "1.15.15"