diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 197169d95..2ba0c981a 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -87,6 +87,48 @@ More detailed help: """ +# These argparse parameters should be removed when detecting defaults. +ARGPARSE_PARAMS_TO_REMOVE = ("const", "nargs", "type",) + + +# These sets are used when to help detect options set by the user. +EXIT_ACTIONS = set(("help", "version",)) + + +ZERO_ARG_ACTIONS = set(("store_const", "store_true", + "store_false", "append_const", "count",)) + + +# Maps a config option to a set of config options that may have modified it. +# This dictionary is used recursively, so if A modifies B and B modifies C, +# it is determined that C was modified by the user if A was modified. +VAR_MODIFIERS = {"account": set(("server",)), + "server": set(("dry_run", "staging",)), + "webroot_map": set(("webroot_path",))} + + +def report_config_interaction(modified, modifiers): + """Registers config option interaction to be checked by set_by_cli. + + This function can be called by during the __init__ or + add_parser_arguments methods of plugins to register interactions + between config options. + + :param modified: config options that can be modified by modifiers + :type modified: iterable or str + :param modifiers: config options that modify modified + :type modifiers: iterable or str + + """ + if isinstance(modified, str): + modified = (modified,) + if isinstance(modifiers, str): + modifiers = (modifiers,) + + for var in modified: + VAR_MODIFIERS.setdefault(var, set()).update(modifiers) + + def usage_strings(plugins): """Make usage strings late so that plugins can be initialised late""" if "nginx" in plugins: @@ -100,6 +142,22 @@ def usage_strings(plugins): return USAGE % (apache_doc, nginx_doc), SHORT_USAGE +class _Default(object): + """A class to use as a default to detect if a value is set by a user""" + + def __bool__(self): + return False + + def __eq__(self, other): + return isinstance(other, _Default) + + def __hash__(self): + return id(_Default) + + def __nonzero__(self): + return self.__bool__() + + def set_by_cli(var): """ Return True if a particular config variable has been set by the user @@ -116,30 +174,18 @@ def set_by_cli(var): 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 = plugin_selection.cli_plugin_requests(detector) - detector.authenticator = auth if auth else "" - detector.installer = inst if inst else "" + detector.authenticator, detector.installer = ( + plugin_selection.cli_plugin_requests(detector)) logger.debug("Default Detector is %r", detector) - try: - # Is detector.var something that isn't false? - change_detected = getattr(detector, var) - except AttributeError: - logger.warning("Missing default analysis for %r", var) - return False + if not isinstance(getattr(detector, var), _Default): + return True - if change_detected: - return True - # Special case: we actually want account to be set to "" if the server - # the account was on has changed - elif var == "account" and (detector.server or detector.dry_run or detector.staging): - return True - # Special case: vars like --no-redirect that get set True -> False - # default to None; False means they were set - elif var in detector.store_false_vars and change_detected is not None: - return True - else: - return False + for modifier in VAR_MODIFIERS.get(var, []): + if set_by_cli(modifier): + return True + + return False # static housekeeping var set_by_cli.detector = None @@ -188,21 +234,23 @@ def config_help(name, hidden=False): return interfaces.IConfig[name].__doc__ -class SilentParser(object): # pylint: disable=too-few-public-methods - """Silent wrapper around argparse. +class HelpfulArgumentGroup(object): + """Emulates an argparse group for use with HelpfulArgumentParser. - A mini parser wrapper that doesn't print help for its - arguments. This is needed for the use of callbacks to define - arguments within plugins. + This class is used in the add_group method of HelpfulArgumentParser. + Command line arguments can be added to the group, but help + suppression and default detection is applied by + HelpfulArgumentParser when necessary. """ - def __init__(self, parser): - self.parser = parser + def __init__(self, helpful_arg_parser, topic): + self._parser = helpful_arg_parser + self._topic = topic def add_argument(self, *args, **kwargs): - """Wrap, but silence help""" - kwargs["help"] = argparse.SUPPRESS - self.parser.add_argument(*args, **kwargs) + """Add a new command line argument to the argument group.""" + self._parser.add(self._topic, *args, **kwargs) + class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -235,15 +283,8 @@ class HelpfulArgumentParser(object): # This is the only way to turn off overly verbose config flag documentation self.parser._add_config_file_help = False # pylint: disable=protected-access - self.silent_parser = SilentParser(self.parser) - # This setting attempts to force all default values to things that are - # pythonically false; it is used to detect when values have been - # explicitly set by the user, including when they are set to their - # normal default value self.detect_defaults = detect_defaults - if detect_defaults: - self.store_false_vars = {} # vars that use "store_false" self.args = args self.determine_verb() @@ -269,6 +310,9 @@ class HelpfulArgumentParser(object): parsed_args.func = self.VERBS[self.verb] parsed_args.verb = self.verb + if self.detect_defaults: + return parsed_args + # Do any post-parsing homework here # we get domains from -d, but also from the webroot map... @@ -304,9 +348,6 @@ class HelpfulArgumentParser(object): "cannot be used with --csr") self.handle_csr(parsed_args) - if self.detect_defaults: # plumbing - parsed_args.store_false_vars = self.store_false_vars - hooks.validate_hooks(parsed_args) return parsed_args @@ -415,7 +456,7 @@ class HelpfulArgumentParser(object): """ if self.detect_defaults: - kwargs = self.modify_arg_for_default_detection(self, *args, **kwargs) + kwargs = self.modify_kwargs_for_default_detection(**kwargs) if self.visible_topics[topic]: if topic in self.groups: @@ -427,39 +468,28 @@ class HelpfulArgumentParser(object): kwargs["help"] = argparse.SUPPRESS self.parser.add_argument(*args, **kwargs) + def modify_kwargs_for_default_detection(self, **kwargs): + """Modify an arg so we can check if it was set by the user. - 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. + Changes the parameters given to argparse when adding an argument + so we can properly detect if the value was set by the user. - :param list *args: the names of this argument flag - :param dict **kwargs: various argparse settings for this argument + :param dict kwargs: various argparse settings for this argument :returns: a modified versions of kwargs + :rtype: dict + """ - # argument either doesn't have a default, or the default doesn't - # isn't Pythonically false - if kwargs.get("default", True): - arg_type = kwargs.get("type", None) - if arg_type == int or kwargs.get("action", "") == "count": - kwargs["default"] = 0 - elif arg_type == read_file or "-c" in args: - kwargs["default"] = "" - kwargs["type"] = str - else: - kwargs["default"] = "" - # This doesn't matter at present (none of the store_false args - # are renewal-relevant), but implement it for future sanity: - # detect the setting of args whose presence causes True -> False - if kwargs.get("action", "") == "store_false": - kwargs["default"] = None - for var in args: - self.store_false_vars[var] = True + 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, num_args): """Adds a deprecated argument with the name argument_name. @@ -475,22 +505,22 @@ class HelpfulArgumentParser(object): self.parser.add_argument, argument_name, num_args) def add_group(self, topic, **kwargs): - """ + """Create a new argument group. - This has to be called once for every topic; but we leave those calls - next to the argument definitions for clarity. Return something - arguments can be added to if necessary, either the parser or an argument - group. + This method must be called once for every topic, however, calls + to this function are left next to the argument definitions for + clarity. + + :param str topic: Name of the new argument group. + + :returns: The new argument group. + :rtype: `HelpfulArgumentGroup` """ if self.visible_topics[topic]: - #print("Adding visible group " + topic) - group = self.parser.add_argument_group(topic, **kwargs) - self.groups[topic] = group - return group - else: - #print("Invisible group " + topic) - return self.silent_parser + self.groups[topic] = self.parser.add_argument_group(topic, **kwargs) + + return HelpfulArgumentGroup(self, topic) def add_plugin_args(self, plugins): """ @@ -501,7 +531,6 @@ class HelpfulArgumentParser(object): """ for name, plugin_ep in six.iteritems(plugins): parser_or_group = self.add_group(name, description=plugin_ep.description) - #print(parser_or_group) plugin_ep.plugin_cls.inject_parser_options(parser_or_group, name) def determine_help_topics(self, chosen_topic): diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py index a9410d514..c66857096 100644 --- a/letsencrypt/plugins/common.py +++ b/letsencrypt/plugins/common.py @@ -45,15 +45,14 @@ class Plugin(object): def add_parser_arguments(cls, add): """Add plugin arguments to the CLI argument parser. + NOTE: If some of your flags interact with others, you can + use cli.report_config_interaction to register this to ensure + values are correctly saved/overridable during renewal. + :param callable add: Function that proxies calls to `argparse.ArgumentParser.add_argument` prepending options with unique plugin name prefix. - 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. - """ @classmethod diff --git a/letsencrypt/renewal.py b/letsencrypt/renewal.py index 8fcb013d3..d89b78566 100644 --- a/letsencrypt/renewal.py +++ b/letsencrypt/renewal.py @@ -102,16 +102,14 @@ def _restore_webroot_config(config, renewalparams): form. """ 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 (cli.set_by_cli("webroot_map") or cli.set_by_cli("webroot_path")): - setattr(config.namespace, "webroot_map", renewalparams["webroot_map"]) + if not cli.set_by_cli("webroot_map"): + config.namespace.webroot_map = renewalparams["webroot_map"] elif "webroot_path" in renewalparams: logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path") wp = renewalparams["webroot_path"] if isinstance(wp, str): # prior to 0.1.0, webroot_path was a string wp = [wp] - setattr(config.namespace, "webroot_path", wp) + config.namespace.webroot_path = wp def _restore_plugin_configs(config, renewalparams): diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index de8b0c7e8..f69d74d0d 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -12,6 +12,7 @@ import unittest import mock import six +from six.moves import reload_module # pylint: disable=import-error from acme import jose @@ -1023,5 +1024,79 @@ class DuplicativeCertsTest(storage_test.BaseRenewableCertTest): self.assertEqual(result, (None, None)) +class DefaultTest(unittest.TestCase): + """Tests for letsencrypt.cli._Default.""" + + def setUp(self): + # pylint: disable=protected-access + self.default1 = cli._Default() + self.default2 = cli._Default() + + def test_boolean(self): + self.assertFalse(self.default1) + self.assertFalse(self.default2) + + def test_equality(self): + self.assertEqual(self.default1, self.default2) + + def test_hash(self): + self.assertEqual(hash(self.default1), hash(self.default2)) + + +class SetByCliTest(unittest.TestCase): + """Tests for letsencrypt.set_by_cli and related functions.""" + + def setUp(self): + reload_module(cli) + + def test_webroot_map(self): + args = '-w /var/www/html -d example.com'.split() + verb = 'renew' + self.assertTrue(_call_set_by_cli('webroot_map', args, verb)) + + def test_report_config_interaction_str(self): + cli.report_config_interaction('manual_public_ip_logging_ok', + 'manual_test_mode') + cli.report_config_interaction('manual_test_mode', 'manual') + + self._test_report_config_interaction_common() + + def test_report_config_interaction_iterable(self): + cli.report_config_interaction(('manual_public_ip_logging_ok',), + ('manual_test_mode',)) + cli.report_config_interaction(('manual_test_mode',), ('manual',)) + + self._test_report_config_interaction_common() + + def _test_report_config_interaction_common(self): + """Tests implied interaction between manual flags. + + --manual implies --manual-test-mode which implies + --manual-public-ip-logging-ok. These interactions don't actually + exist in the client, but are used here for testing purposes. + + """ + + args = ['--manual'] + verb = 'renew' + for v in ('manual', 'manual_test_mode', 'manual_public_ip_logging_ok'): + self.assertTrue(_call_set_by_cli(v, args, verb)) + + cli.set_by_cli.detector = None + + args = ['--manual-test-mode'] + for v in ('manual_test_mode', 'manual_public_ip_logging_ok'): + self.assertTrue(_call_set_by_cli(v, args, verb)) + + self.assertFalse(_call_set_by_cli('manual', args, verb)) + + +def _call_set_by_cli(var, args, verb): + with mock.patch('letsencrypt.cli.helpful_parser') as mock_parser: + mock_parser.args = args + mock_parser.verb = verb + return cli.set_by_cli(var) + + if __name__ == '__main__': unittest.main() # pragma: no cover