From 4f7c5b32e87809e650ff05d9a326295283fcad71 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 23 Mar 2016 14:57:01 -0700 Subject: [PATCH] Cleanup responding to protected-access problems --- letsencrypt/cli.py | 18 +++++++++--------- letsencrypt/plugins/common.py | 2 +- letsencrypt/storage.py | 6 +++--- letsencrypt/tests/cli_test.py | 4 ++-- letsencrypt/tests/storage_test.py | 3 +++ 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index fccebe02f..fa1e0090e 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -271,20 +271,20 @@ def record_chosen_plugins(config, plugins, auth, inst): cn.installer = plugins.find_init(inst).name if inst else "none" -def _set_by_cli(var): +def set_by_cli(var): """ 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. """ - detector = _set_by_cli.detector + detector = set_by_cli.detector if detector is 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 = _parser.args + [_parser.verb] - detector = _set_by_cli.detector = prepare_and_parse_args( + detector = set_by_cli.detector = prepare_and_parse_args( plugins, reconstructed_args, detect_defaults=True) # propagate plugin requests: eg --standalone modifies config.authenticator auth, inst = cli_plugin_requests(detector) @@ -312,7 +312,7 @@ def _set_by_cli(var): else: return False # static housekeeping var -_set_by_cli.detector = None +set_by_cli.detector = None def _restore_required_config_elements(config, renewalparams): """Sets non-plugin specific values in config from renewalparams @@ -325,7 +325,7 @@ def _restore_required_config_elements(config, renewalparams): """ # string-valued items to add if they're present for config_item in STR_CONFIG_ITEMS: - if config_item in renewalparams and not _set_by_cli(config_item): + if config_item in renewalparams and not set_by_cli(config_item): value = renewalparams[config_item] # Unfortunately, we've lost type information from ConfigObj, # so we don't know if the original was NoneType or str! @@ -334,7 +334,7 @@ def _restore_required_config_elements(config, renewalparams): setattr(config.namespace, config_item, value) # int-valued items to add if they're present for config_item in INT_CONFIG_ITEMS: - if config_item in renewalparams and not _set_by_cli(config_item): + if config_item in renewalparams and not set_by_cli(config_item): config_value = renewalparams[config_item] # the default value for http01_port was None during private beta if config_item == "http01_port" and config_value == "None": @@ -378,7 +378,7 @@ def _restore_plugin_configs(config, renewalparams): plugin_prefixes.append(renewalparams["installer"]) for plugin_prefix in set(plugin_prefixes): for config_item, config_value in six.iteritems(renewalparams): - if config_item.startswith(plugin_prefix + "_") and not _set_by_cli(config_item): + if config_item.startswith(plugin_prefix + "_") and not set_by_cli(config_item): # Values None, True, and False need to be treated specially, # As they don't get parsed correctly based on type if config_value in ("None", "True", "False"): @@ -403,7 +403,7 @@ def _restore_webroot_config(config, renewalparams): if "webroot_map" in renewalparams: # if the user does anything that would create a new webroot map on the # CLI, don't use the old one - if not (_set_by_cli("webroot_map") or _set_by_cli("webroot_path")): + if not (set_by_cli("webroot_map") or set_by_cli("webroot_path")): setattr(config.namespace, "webroot_map", renewalparams["webroot_map"]) elif "webroot_path" in renewalparams: logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path") @@ -844,7 +844,7 @@ class HelpfulArgumentParser(object): def modify_arg_for_default_detection(self, *args, **kwargs): """ Adding an arg, but ensure that it has a default that evaluates to false, - so that _set_by_cli can tell if it was set. Only called if detect_defaults==True. + so that set_by_cli can tell if it was set. Only called if detect_defaults==True. :param list *args: the names of this argument flag :param dict **kwargs: various argparse settings for this argument diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py index 319692344..f6a2c3d76 100644 --- a/letsencrypt/plugins/common.py +++ b/letsencrypt/plugins/common.py @@ -52,7 +52,7 @@ class Plugin(object): NOTE: if you add argpase arguments such that users setting them can create a config entry that python's bool() would consider false (ie, the use might set the variable to "", [], 0, etc), please ensure that - cli._set_by_cli() works for your variable. + cli.set_by_cli() works for your variable. """ diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index 5e5d70518..3807cb63d 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -152,7 +152,7 @@ def relevant_values(all_values): # Look through the CLI parser defaults and see if this option is # both present and equal to the specified value. If not, return # False. - for x in cli._parser.parser._actions: + for x in cli._parser.parser._actions: # pylint: disable=protected-access if x.dest == option: if x.default == value: return True @@ -164,10 +164,10 @@ def relevant_values(all_values): for option, value in all_values.iteritems(): # Try to find reasons to store this item in the # renewal config. It can be stored if it is relevant and - # (it is _set_by_cli() or flag_default() is different + # (it is set_by_cli() or flag_default() is different # from the value or flag_default() doesn't exist). if _relevant(option): - if (cli._set_by_cli(option) + if (cli.set_by_cli(option) or not _is_cli_default(option, value)): # or option not in constants.CLI_DEFAULTS # or constants.CLI_DEFAULTS[option] != value): diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 920bc2d99..74229cb6b 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -54,7 +54,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods shutil.rmtree(self.tmp_dir) # Reset globals in cli # pylint: disable=protected-access - cli._parser = cli._set_by_cli.detector = None + cli._parser = cli.set_by_cli.detector = None def _call(self, args): "Run the cli with output streams and actual client mocked out" @@ -660,7 +660,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods args = ["renew", "--dry-run", "-tvv"] self._test_renewal_common(True, [], args=args, renew=True) - @mock.patch("letsencrypt.cli._set_by_cli") + @mock.patch("letsencrypt.cli.set_by_cli") def test_ancient_webroot_renewal_conf(self, mock_set_by_cli): mock_set_by_cli.return_value = False rc_path = self._make_test_renewal_conf('sample-renewal-ancient.conf') diff --git a/letsencrypt/tests/storage_test.py b/letsencrypt/tests/storage_test.py index 9660e2688..0d007a831 100644 --- a/letsencrypt/tests/storage_test.py +++ b/letsencrypt/tests/storage_test.py @@ -565,6 +565,7 @@ class RenewableCertTests(BaseRenewableCertTest): @mock.patch("letsencrypt.cli._parser") def test_relevant_values(self, mock_parser): """Test that relevant_values() can reject an irrelevant value.""" + # pylint: disable=protected-access from letsencrypt import storage mock_parser.verb = "certonly" mock_parser.args = ["--standalone"] @@ -575,6 +576,7 @@ class RenewableCertTests(BaseRenewableCertTest): @mock.patch("letsencrypt.cli._parser") def test_relevant_values_default(self, mock_parser): """Test that relevant_values() can reject a default value.""" + # pylint: disable=protected-access from letsencrypt import storage mock_parser.verb = "certonly" mock_parser.args = ["--standalone"] @@ -585,6 +587,7 @@ class RenewableCertTests(BaseRenewableCertTest): @mock.patch("letsencrypt.cli._parser") def test_relevant_values_nondefault(self, mock_parser): """Test that relevant_values() can retain a non-default value.""" + # pylint: disable=protected-access from letsencrypt import storage mock_parser.verb = "certonly" mock_parser.args = ["--standalone"]