From 131641e963c701b05dc2d5552544939657aba8c4 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 2 Feb 2016 10:17:54 -0800 Subject: [PATCH 1/5] cli.py should be less argumenentative - remove all the passing around of `args`, limiting ourselves to just `config`. - fixes #2341 --- letsencrypt/cli.py | 210 +++++++++++++++++----------------- letsencrypt/tests/cli_test.py | 56 ++++----- 2 files changed, 134 insertions(+), 132 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 2da82412d..5b6b1c52f 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -129,14 +129,14 @@ def _find_domains(config, installer): return domains -def _determine_account(args, config): +def _determine_account(config): """Determine which account to use. In order to make the renewer (configuration de/serialization) happy, - if ``args.account`` is ``None``, it will be updated based on the - user input. Same for ``args.email``. + if ``config.account`` is ``None``, it will be updated based on the + user input. Same for ``config.email``. - :param argparse.Namespace args: CLI arguments + :param argparse.Namespace config: CLI arguments :param letsencrypt.interface.IConfig config: Configuration object :param .AccountStorage account_storage: Account storage. @@ -149,8 +149,8 @@ def _determine_account(args, config): account_storage = account.AccountFileStorage(config) acme = None - if args.account is not None: - acc = account_storage.load(args.account) + if config.account is not None: + acc = account_storage.load(config.account) else: accounts = account_storage.find_all() if len(accounts) > 1: @@ -158,11 +158,11 @@ def _determine_account(args, config): elif len(accounts) == 1: acc = accounts[0] else: # no account registered yet - if args.email is None and not args.register_unsafely_without_email: - args.email = display_ops.get_email() + if config.email is None and not config.register_unsafely_without_email: + config.email = display_ops.get_email() def _tos_cb(regr): - if args.tos: + if config.tos: return True msg = ("Please read the Terms of Service at {0}. You " "must agree in order to register with the ACME " @@ -181,14 +181,14 @@ def _determine_account(args, config): raise errors.Error( "Unable to register an account with ACME server") - args.account = acc.id + config.account = acc.id return acc, acme -def _init_le_client(args, config, authenticator, installer): +def _init_le_client(config, authenticator, installer): if authenticator is not None: # if authenticator was given, then we will need account... - acc, acme = _determine_account(args, config) + acc, acme = _determine_account(config) logger.debug("Picked account: %r", acc) # XXX #crypto_util.validate_key_csr(acc.key) @@ -497,27 +497,27 @@ def set_configurator(previously, now): raise errors.PluginSelectionError(msg.format(repr(previously), repr(now))) return now -def cli_plugin_requests(args): +def cli_plugin_requests(config): """ Figure out which plugins the user requested with CLI and config options :returns: (requested authenticator string or None, requested installer string or None) :rtype: tuple """ - req_inst = req_auth = args.configurator - req_inst = set_configurator(req_inst, args.installer) - req_auth = set_configurator(req_auth, args.authenticator) - if args.nginx: + req_inst = req_auth = config.configurator + req_inst = set_configurator(req_inst, config.installer) + req_auth = set_configurator(req_auth, config.authenticator) + if config.nginx: req_inst = set_configurator(req_inst, "nginx") req_auth = set_configurator(req_auth, "nginx") - if args.apache: + if config.apache: req_inst = set_configurator(req_inst, "apache") req_auth = set_configurator(req_auth, "apache") - if args.standalone: + if config.standalone: req_auth = set_configurator(req_auth, "standalone") - if args.webroot: + if config.webroot: req_auth = set_configurator(req_auth, "webroot") - if args.manual: + if config.manual: req_auth = set_configurator(req_auth, "manual") logger.debug("Requested authenticator %s and installer %s", req_auth, req_inst) return req_auth, req_inst @@ -525,13 +525,19 @@ def cli_plugin_requests(args): noninstaller_plugins = ["webroot", "manual", "standalone"] -def choose_configurator_plugins(args, config, plugins, verb): +def choose_configurator_plugins(config, plugins, verb): """ - Figure out which configurator we're going to use + Figure out which configurator we're going to use, modifies + config.authenticator and config.istaller strings to reflect that choice if + necessary. + :raises errors.PluginSelectionError if there was a problem + + :returns: (an `IAuthenticator` or None, an `IInstaller` or None) + :rtype: tuple """ - req_auth, req_inst = cli_plugin_requests(args) + req_auth, req_inst = cli_plugin_requests(config) # Which plugins do we need? if verb == "run": @@ -550,11 +556,9 @@ def choose_configurator_plugins(args, config, plugins, verb): need_auth = True if verb == "install": need_inst = True - if args.authenticator: + if config.authenticator: logger.warn("Specifying an authenticator doesn't make sense in install mode") - - # Try to meet the user's request and/or ask them to pick plugins authenticator = installer = None if verb == "run" and req_auth == req_inst: @@ -586,18 +590,18 @@ def record_chosen_plugins(config, plugins, auth, inst): # TODO: Make run as close to auth + install as possible -# Possible difficulties: args.csr was hacked into auth -def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-locals +# Possible difficulties: config.csr was hacked into auth +def run(config, plugins): # pylint: disable=too-many-branches,too-many-locals """Obtain a certificate and install.""" try: - installer, authenticator = choose_configurator_plugins(args, config, plugins, "run") + installer, authenticator = choose_configurator_plugins(config, plugins, "run") except errors.PluginSelectionError as e: return e.message domains = _find_domains(config, installer) # TODO: Handle errors from _init_le_client? - le_client = _init_le_client(args, config, authenticator, installer) + le_client = _init_le_client(config, authenticator, installer) lineage, action = _auth_from_domains(le_client, config, domains) @@ -615,29 +619,29 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo _suggest_donate() -def obtain_cert(args, config, plugins): +def obtain_cert(config, plugins): """Implements "certonly": authenticate & obtain cert, but do not install it.""" - if args.domains and args.csr is not None: + if config.domains and config.csr is not None: # TODO: --csr could have a priority, when --domains is # supplied, check if CSR matches given domains? return "--domains and --csr are mutually exclusive" try: # installers are used in auth mode to determine domain names - installer, authenticator = choose_configurator_plugins(args, config, plugins, "certonly") + installer, authenticator = choose_configurator_plugins(config, plugins, "certonly") except errors.PluginSelectionError as e: return e.message # TODO: Handle errors from _init_le_client? - le_client = _init_le_client(args, config, authenticator, installer) + le_client = _init_le_client(config, authenticator, installer) # This is a special case; cert and chain are simply saved - if args.csr is not None: + if config.csr is not None: certr, chain = le_client.obtain_certificate_from_csr(le_util.CSR( - file=args.csr[0], data=args.csr[1], form="der")) + file=config.csr[0], data=config.csr[1], form="der")) cert_path, _, cert_fullchain = le_client.save_certificate( - certr, chain, args.cert_path, args.chain_path, args.fullchain_path) + certr, chain, config.cert_path, config.chain_path, config.fullchain_path) _report_new_cert(cert_path, cert_fullchain) else: domains = _find_domains(config, installer) @@ -646,51 +650,49 @@ def obtain_cert(args, config, plugins): _suggest_donate() -def install(args, config, plugins): +def install(config, plugins): """Install a previously obtained cert in a server.""" # XXX: Update for renewer/RenewableCert # FIXME: be consistent about whether errors are raised or returned from # this function ... try: - installer, _ = choose_configurator_plugins(args, config, - plugins, "install") + installer, _ = choose_configurator_plugins(config, plugins, "install") except errors.PluginSelectionError as e: return e.message domains = _find_domains(config, installer) - le_client = _init_le_client( - args, config, authenticator=None, installer=installer) - assert args.cert_path is not None # required=True in the subparser + le_client = _init_le_client(config, authenticator=None, installer=installer) + assert config.cert_path is not None # required=True in the subparser le_client.deploy_certificate( - domains, args.key_path, args.cert_path, args.chain_path, - args.fullchain_path) + domains, config.key_path, config.cert_path, config.chain_path, + config.fullchain_path) le_client.enhance_config(domains, config) -def revoke(args, config, unused_plugins): # TODO: coop with renewal config +def revoke(config, unused_plugins): # TODO: coop with renewal config """Revoke a previously obtained certificate.""" # For user-agent construction config.namespace.installer = config.namespace.authenticator = "none" - if args.key_path is not None: # revocation by cert key + if config.key_path is not None: # revocation by cert key logger.debug("Revoking %s using cert key %s", - args.cert_path[0], args.key_path[0]) - key = jose.JWK.load(args.key_path[1]) + config.cert_path[0], config.key_path[0]) + key = jose.JWK.load(config.key_path[1]) else: # revocation by account key - logger.debug("Revoking %s using Account Key", args.cert_path[0]) - acc, _ = _determine_account(args, config) + logger.debug("Revoking %s using Account Key", config.cert_path[0]) + acc, _ = _determine_account(config) key = acc.key acme = client.acme_from_config_key(config, key) - cert = crypto_util.pyopenssl_load_certificate(args.cert_path[1])[0] + cert = crypto_util.pyopenssl_load_certificate(config.cert_path[1])[0] acme.revoke(jose.ComparableX509(cert)) -def rollback(args, config, plugins): +def rollback(config, plugins): """Rollback server configuration changes made during install.""" - client.rollback(args.installer, args.checkpoints, config, plugins) + client.rollback(config.installer, config.checkpoints, config, plugins) -def config_changes(unused_args, config, unused_plugins): +def config_changes(config, unused_plugins): """Show changes made to server config during installation View checkpoints and associated configuration changes. @@ -699,15 +701,15 @@ def config_changes(unused_args, config, unused_plugins): client.view_config_changes(config) -def plugins_cmd(args, config, plugins): # TODO: Use IDisplay rather than print +def plugins_cmd(config, plugins): # TODO: Use IDisplay rather than print """List server software plugins.""" - logger.debug("Expected interfaces: %s", args.ifaces) + logger.debug("Expected interfaces: %s", config.ifaces) - ifaces = [] if args.ifaces is None else args.ifaces + ifaces = [] if config.ifaces is None else config.ifaces filtered = plugins.visible().ifaces(ifaces) logger.debug("Filtered plugins: %r", filtered) - if not args.init and not args.prepare: + if not config.init and not config.prepare: print(str(filtered)) return @@ -715,7 +717,7 @@ def plugins_cmd(args, config, plugins): # TODO: Use IDisplay rather than print verified = filtered.verify(ifaces) logger.debug("Verified plugins: %r", verified) - if not args.prepare: + if not config.prepare: print(str(verified)) return @@ -1273,63 +1275,63 @@ class WebrootPathProcessor(argparse.Action): # pylint: disable=missing-docstring self.domain_before_webroot = False argparse.Action.__init__(self, *args, **kwargs) - def __call__(self, parser, config, webroot, option_string=None): + def __call__(self, parser, args, webroot, option_string=None): """ Keep a record of --webroot-path / -w flags during processing, so that we know which apply to which -d flags """ - if config.webroot_path is None: # first -w flag encountered - config.webroot_path = [] + if args.webroot_path is None: # first -w flag encountered + args.webroot_path = [] # if any --domain flags preceded the first --webroot-path flag, # apply that webroot path to those; subsequent entries in - # config.webroot_map are filled in by cli.DomainFlagProcessor - if config.domains: + # args.webroot_map are filled in by cli.DomainFlagProcessor + if args.domains: self.domain_before_webroot = True - for d in config.domains: - config.webroot_map.setdefault(d, webroot) + for d in args.domains: + args.webroot_map.setdefault(d, webroot) elif self.domain_before_webroot: - # FIXME if you set domains in a config file, you should get a different error + # FIXME if you set domains in a args file, you should get a different error # here, pointing you to --webroot-map raise errors.Error("If you specify multiple webroot paths, one of " "them must precede all domain flags") - config.webroot_path.append(webroot) + args.webroot_path.append(webroot) _undot = lambda domain: domain[:-1] if domain.endswith('.') else domain -def _process_domain(config, domain_arg, webroot_path=None): +def _process_domain(args, domain_arg, webroot_path=None): """ Process a new -d flag, helping the webroot plugin construct a map of {domain : webrootpath} if -w / --webroot-path is in use """ - webroot_path = webroot_path if webroot_path else config.webroot_path + webroot_path = webroot_path if webroot_path else args.webroot_path for domain in (d.strip() for d in domain_arg.split(",")): - if domain not in config.domains: + if domain not in args.domains: domain = _undot(domain) - config.domains.append(domain) + args.domains.append(domain) # Each domain has a webroot_path of the most recent -w flag # unless it was explicitly included in webroot_map if webroot_path: - config.webroot_map.setdefault(domain, webroot_path[-1]) + args.webroot_map.setdefault(domain, webroot_path[-1]) class WebrootMapProcessor(argparse.Action): # pylint: disable=missing-docstring - def __call__(self, parser, config, webroot_map_arg, option_string=None): + def __call__(self, parser, args, webroot_map_arg, option_string=None): webroot_map = json.loads(webroot_map_arg) for domains, webroot_path in webroot_map.iteritems(): - _process_domain(config, domains, [webroot_path]) + _process_domain(args, domains, [webroot_path]) class DomainFlagProcessor(argparse.Action): # pylint: disable=missing-docstring - def __call__(self, parser, config, domain_arg, option_string=None): + def __call__(self, parser, args, domain_arg, option_string=None): """Just wrap _process_domain in argparseese.""" - _process_domain(config, domain_arg) + _process_domain(args, domain_arg) -def setup_log_file_handler(args, logfile, fmt): +def setup_log_file_handler(config, logfile, fmt): """Setup file debug logging.""" - log_file_path = os.path.join(args.logs_dir, logfile) + log_file_path = os.path.join(config.logs_dir, logfile) handler = logging.handlers.RotatingFileHandler( log_file_path, maxBytes=2 ** 20, backupCount=10) # rotate on each invocation, rollover only possible when maxBytes @@ -1343,8 +1345,8 @@ def setup_log_file_handler(args, logfile, fmt): return handler, log_file_path -def _cli_log_handler(args, level, fmt): - if args.text_mode: +def _cli_log_handler(config, level, fmt): + if config.text_mode: handler = colored_logging.StreamHandler() handler.setFormatter(logging.Formatter(fmt)) else: @@ -1355,13 +1357,13 @@ def _cli_log_handler(args, level, fmt): return handler -def setup_logging(args, cli_handler_factory, logfile): +def setup_logging(config, cli_handler_factory, logfile): """Setup logging.""" fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s" - level = -args.verbose_count * 10 + level = -config.verbose_count * 10 file_handler, log_file_path = setup_log_file_handler( - args, logfile=logfile, fmt=fmt) - cli_handler = cli_handler_factory(args, level, fmt) + config, logfile=logfile, fmt=fmt) + cli_handler = cli_handler_factory(config, level, fmt) # TODO: use fileConfig? @@ -1374,12 +1376,12 @@ def setup_logging(args, cli_handler_factory, logfile): logger.info("Saving debug log to %s", log_file_path) -def _handle_exception(exc_type, exc_value, trace, args): +def _handle_exception(exc_type, exc_value, trace, config): """Logs exceptions and reports them to the user. - Args is used to determine how to display exceptions to the user. In - general, if args.debug is True, then the full exception and traceback is - shown to the user, otherwise it is suppressed. If args itself is None, + Config is used to determine how to display exceptions to the user. In + general, if config.debug is True, then the full exception and traceback is + shown to the user, otherwise it is suppressed. If config itself is None, then the traceback and exception is attempted to be written to a logfile. If this is successful, the traceback is suppressed, otherwise it is shown to the user. sys.exit is always called with a nonzero status. @@ -1390,8 +1392,8 @@ def _handle_exception(exc_type, exc_value, trace, args): os.linesep, "".join(traceback.format_exception(exc_type, exc_value, trace))) - if issubclass(exc_type, Exception) and (args is None or not args.debug): - if args is None: + if issubclass(exc_type, Exception) and (config is None or not config.debug): + if config is None: logfile = "letsencrypt.log" try: with open(logfile, "w") as logfd: @@ -1413,14 +1415,14 @@ def _handle_exception(exc_type, exc_value, trace, args): # malformed :: Error creating new registration :: Validation of contact # mailto:none@longrandomstring.biz failed: Server failure at resolver if ("urn:acme" in err and ":: " in err - and args.verbose_count <= flag_default("verbose_count")): + and config.verbose_count <= flag_default("verbose_count")): # prune ACME error code, we have a human description _code, _sep, err = err.partition(":: ") msg = "An unexpected error occurred:\n" + err + "Please see the " - if args is None: + if config is None: msg += "logfile '{0}' for more details.".format(logfile) else: - msg += "logfiles in {0} for more details.".format(args.logs_dir) + msg += "logfiles in {0} for more details.".format(config.logs_dir) sys.exit(msg) else: sys.exit("".join( @@ -1429,7 +1431,7 @@ def _handle_exception(exc_type, exc_value, trace, args): def main(cli_args=sys.argv[1:]): """Command line argument parsing and main script execution.""" - sys.excepthook = functools.partial(_handle_exception, args=None) + sys.excepthook = functools.partial(_handle_exception, config=None) # note: arg parser internally handles --help (and exits afterwards) plugins = plugins_disco.PluginsRegistry.find_all() @@ -1446,20 +1448,20 @@ def main(cli_args=sys.argv[1:]): # TODO: logs might contain sensitive data such as contents of the # private key! #525 le_util.make_or_verify_dir( - args.logs_dir, 0o700, os.geteuid(), "--strict-permissions" in cli_args) - setup_logging(args, _cli_log_handler, logfile='letsencrypt.log') + config.logs_dir, 0o700, os.geteuid(), "--strict-permissions" in cli_args) + setup_logging(config, _cli_log_handler, logfile='letsencrypt.log') logger.debug("letsencrypt version: %s", letsencrypt.__version__) - # do not log `args`, as it contains sensitive data (e.g. revoke --key)! + # do not log `config`, as it contains sensitive data (e.g. revoke --key)! logger.debug("Arguments: %r", cli_args) logger.debug("Discovered plugins: %r", plugins) - sys.excepthook = functools.partial(_handle_exception, args=args) + sys.excepthook = functools.partial(_handle_exception, config=config) # Displayer - if args.noninteractive_mode: + if config.noninteractive_mode: displayer = display_util.NoninteractiveDisplay(sys.stdout) - elif args.text_mode: + elif config.text_mode: displayer = display_util.FileDisplay(sys.stdout) else: displayer = display_util.NcursesDisplay() @@ -1481,7 +1483,7 @@ def main(cli_args=sys.argv[1:]): # "{0}Root is required to run letsencrypt. Please use sudo.{0}" # .format(os.linesep)) - return args.func(args, config, plugins) + return config.func(config, plugins) if __name__ == "__main__": err_string = main() diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 43127dc8a..4a9d618f7 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -237,7 +237,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods with mock.patch("letsencrypt.cli._init_le_client") as mock_init: with mock.patch("letsencrypt.cli._auth_from_domains"): self._call(["certonly", "--manual", "-d", "foo.bar"]) - auth = mock_init.call_args[0][2] + _config, auth, _installer = mock_init.call_args[0] self.assertTrue(isinstance(auth, manual.Authenticator)) with MockedVerb("certonly") as mock_certonly: @@ -318,11 +318,11 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods '--chain-path', 'chain', '--fullchain-path', 'fullchain']) - args = mock_obtaincert.call_args[0][0] - self.assertEqual(args.cert_path, os.path.abspath(cert)) - self.assertEqual(args.key_path, os.path.abspath(key)) - self.assertEqual(args.chain_path, os.path.abspath(chain)) - self.assertEqual(args.fullchain_path, os.path.abspath(fullchain)) + config, _plugins = mock_obtaincert.call_args[0] + self.assertEqual(config.cert_path, os.path.abspath(cert)) + self.assertEqual(config.key_path, os.path.abspath(key)) + self.assertEqual(config.chain_path, os.path.abspath(chain)) + self.assertEqual(config.fullchain_path, os.path.abspath(fullchain)) def test_certonly_bad_args(self): ret, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) @@ -560,14 +560,14 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods # pylint: disable=protected-access from acme import messages - args = mock.MagicMock() + config = mock.MagicMock() mock_open = mock.mock_open() with mock.patch('letsencrypt.cli.open', mock_open, create=True): exception = Exception('detail') - args.verbose_count = 1 + config.verbose_count = 1 cli._handle_exception( - Exception, exc_value=exception, trace=None, args=None) + Exception, exc_value=exception, trace=None, config=None) mock_open().write.assert_called_once_with(''.join( traceback.format_exception_only(Exception, exception))) error_msg = mock_sys.exit.call_args_list[0][0][0] @@ -577,24 +577,24 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mock_open.side_effect = [KeyboardInterrupt] error = errors.Error('detail') cli._handle_exception( - errors.Error, exc_value=error, trace=None, args=None) + errors.Error, exc_value=error, trace=None, config=None) # assert_any_call used because sys.exit doesn't exit in cli.py mock_sys.exit.assert_any_call(''.join( traceback.format_exception_only(errors.Error, error))) exception = messages.Error(detail='alpha', typ='urn:acme:error:triffid', title='beta') - args = mock.MagicMock(debug=False, verbose_count=-3) + config = mock.MagicMock(debug=False, verbose_count=-3) cli._handle_exception( - messages.Error, exc_value=exception, trace=None, args=args) + messages.Error, exc_value=exception, trace=None, config=config) error_msg = mock_sys.exit.call_args_list[-1][0][0] self.assertTrue('unexpected error' in error_msg) self.assertTrue('acme:error' not in error_msg) self.assertTrue('alpha' in error_msg) self.assertTrue('beta' in error_msg) - args = mock.MagicMock(debug=False, verbose_count=1) + config = mock.MagicMock(debug=False, verbose_count=1) cli._handle_exception( - messages.Error, exc_value=exception, trace=None, args=args) + messages.Error, exc_value=exception, trace=None, config=config) error_msg = mock_sys.exit.call_args_list[-1][0][0] self.assertTrue('unexpected error' in error_msg) self.assertTrue('acme:error' in error_msg) @@ -602,7 +602,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods interrupt = KeyboardInterrupt('detail') cli._handle_exception( - KeyboardInterrupt, exc_value=interrupt, trace=None, args=None) + KeyboardInterrupt, exc_value=interrupt, trace=None, config=None) mock_sys.exit.assert_called_with(''.join( traceback.format_exception_only(KeyboardInterrupt, interrupt))) @@ -640,20 +640,20 @@ class DetermineAccountTest(unittest.TestCase): from letsencrypt.cli import _determine_account with mock.patch('letsencrypt.cli.account.AccountFileStorage') as mock_storage: mock_storage.return_value = self.account_storage - return _determine_account(self.args, self.config) + return _determine_account(self.config) def test_args_account_set(self): self.account_storage.save(self.accs[1]) - self.args.account = self.accs[1].id + self.config.account = self.accs[1].id self.assertEqual((self.accs[1], None), self._call()) - self.assertEqual(self.accs[1].id, self.args.account) - self.assertTrue(self.args.email is None) + self.assertEqual(self.accs[1].id, self.config.account) + self.assertTrue(self.config.email is None) def test_single_account(self): self.account_storage.save(self.accs[0]) self.assertEqual((self.accs[0], None), self._call()) - self.assertEqual(self.accs[0].id, self.args.account) - self.assertTrue(self.args.email is None) + self.assertEqual(self.accs[0].id, self.config.account) + self.assertTrue(self.config.email is None) @mock.patch('letsencrypt.client.display_ops.choose_account') def test_multiple_accounts(self, mock_choose_accounts): @@ -663,8 +663,8 @@ class DetermineAccountTest(unittest.TestCase): self.assertEqual((self.accs[1], None), self._call()) self.assertEqual( set(mock_choose_accounts.call_args[0][0]), set(self.accs)) - self.assertEqual(self.accs[1].id, self.args.account) - self.assertTrue(self.args.email is None) + self.assertEqual(self.accs[1].id, self.config.account) + self.assertTrue(self.config.email is None) @mock.patch('letsencrypt.client.display_ops.get_email') def test_no_accounts_no_email(self, mock_get_email): @@ -677,16 +677,16 @@ class DetermineAccountTest(unittest.TestCase): client.register.assert_called_once_with( self.config, self.account_storage, tos_cb=mock.ANY) - self.assertEqual(self.accs[0].id, self.args.account) - self.assertEqual('foo@bar.baz', self.args.email) + self.assertEqual(self.accs[0].id, self.config.account) + self.assertEqual('foo@bar.baz', self.config.email) def test_no_accounts_email(self): - self.args.email = 'other email' + self.config.email = 'other email' with mock.patch('letsencrypt.cli.client') as client: client.register.return_value = (self.accs[1], mock.sentinel.acme) self._call() - self.assertEqual(self.accs[1].id, self.args.account) - self.assertEqual('other email', self.args.email) + self.assertEqual(self.accs[1].id, self.config.account) + self.assertEqual('other email', self.config.email) class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest): From 623fd9f41716ec0c7439c8f6d89a7df560ae582e Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 2 Feb 2016 10:59:01 -0800 Subject: [PATCH 2/5] fix editing error --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 0dfed928d..d17a24bdc 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -664,7 +664,7 @@ def obtain_cert(config, plugins): "Dry run: skipping saving certificate to %s", config.cert_path) else: cert_path, _, cert_fullchain = le_client.save_certificate( - , chain, config.cert_path, config.chain_path, args.fullchain_path) + certr, chain, config.cert_path, config.chain_path, args.fullchain_path) _report_new_cert(cert_path, cert_fullchain) else: domains = _find_domains(config, installer) From aafe7f2a844c82c7c58ecddb6eb4f1bbf57dc0eb Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 2 Feb 2016 12:39:48 -0800 Subject: [PATCH 3/5] Fix another merge glitch --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index d17a24bdc..5cee595c0 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -670,7 +670,7 @@ def obtain_cert(config, plugins): domains = _find_domains(config, installer) _auth_from_domains(le_client, config, domains) - if args.dry_run: + if config.dry_run: _report_successful_dry_run() _suggest_donation_if_appropriate(config) From 14334ea77516c18a29eaa91532acfffffd0e7622 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 2 Feb 2016 12:42:05 -0800 Subject: [PATCH 4/5] And another... --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 5cee595c0..6f9fa7229 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -664,7 +664,7 @@ def obtain_cert(config, plugins): "Dry run: skipping saving certificate to %s", config.cert_path) else: cert_path, _, cert_fullchain = le_client.save_certificate( - certr, chain, config.cert_path, config.chain_path, args.fullchain_path) + certr, chain, config.cert_path, config.chain_path, config.fullchain_path) _report_new_cert(cert_path, cert_fullchain) else: domains = _find_domains(config, installer) From 747bd2715f83dd0262052d67aac58519ff0e3f66 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 2 Feb 2016 14:29:32 -0800 Subject: [PATCH 5/5] Fix merge bugs & address other review comments --- letsencrypt/cli.py | 4 +--- letsencrypt/tests/cli_test.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 6f9fa7229..92e985313 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1337,8 +1337,6 @@ class WebrootPathProcessor(argparse.Action): # pylint: disable=missing-docstring args.webroot_path.append(webroot) -_undot = lambda domain: domain[:-1] if domain.endswith('.') else domain - def _process_domain(args_or_config, domain_arg, webroot_path=None): """ Process a new -d flag, helping the webroot plugin construct a map of @@ -1352,8 +1350,8 @@ def _process_domain(args_or_config, domain_arg, webroot_path=None): webroot_path = webroot_path if webroot_path else args_or_config.webroot_path for domain in (d.strip() for d in domain_arg.split(",")): + domain = le_util.enforce_domain_sanity(domain) if domain not in args_or_config.domains: - domain = _undot(domain) args_or_config.domains.append(domain) # Each domain has a webroot_path of the most recent -w flag # unless it was explicitly included in webroot_map diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 7529f4548..f0ac954f9 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -235,7 +235,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods with mock.patch("letsencrypt.cli._init_le_client") as mock_init: with mock.patch("letsencrypt.cli._auth_from_domains"): self._call(["certonly", "--manual", "-d", "foo.bar"]) - _config, auth, _installer = mock_init.call_args[0] + unused_config, auth, unused_installer = mock_init.call_args[0] self.assertTrue(isinstance(auth, manual.Authenticator)) with MockedVerb("certonly") as mock_certonly: @@ -316,7 +316,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods '--chain-path', 'chain', '--fullchain-path', 'fullchain']) - config, _plugins = mock_obtaincert.call_args[0] + config, unused_plugins = mock_obtaincert.call_args[0] self.assertEqual(config.cert_path, os.path.abspath(cert)) self.assertEqual(config.key_path, os.path.abspath(key)) self.assertEqual(config.chain_path, os.path.abspath(chain))