diff --git a/.pylintrc b/.pylintrc index bf318704a..268d61ec6 100644 --- a/.pylintrc +++ b/.pylintrc @@ -38,7 +38,7 @@ load-plugins=linter_plugin # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use"--disable=all --enable=classes # --disable=W" -disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use +disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use,invalid-name # abstract-class-not-used cannot be disabled locally (at least in pylint 1.4.1) diff --git a/letsencrypt/account.py b/letsencrypt/account.py index c97e4f6fe..81d31b831 100644 --- a/letsencrypt/account.py +++ b/letsencrypt/account.py @@ -54,7 +54,7 @@ class Account(object): # pylint: disable=too-few-public-methods tz=pytz.UTC).replace(microsecond=0), creation_host=socket.getfqdn()) if meta is None else meta - self.id = hashlib.md5( # pylint: disable=invalid-name + self.id = hashlib.md5( self.key.key.public_key().public_bytes( encoding=serialization.Encoding.DER, format=serialization.PublicFormat.SubjectPublicKeyInfo) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 0b7d17909..73dd24bdb 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -80,8 +80,8 @@ More detailed help: -h, --help [topic] print this message, or detailed help on a topic; the available topics are: - all, apache, automation, nginx, paths, security, testing, or any of the - subcommands + all, apache, automation, manual, nginx, paths, security, testing, or any of + the subcommands """ @@ -500,9 +500,6 @@ class SilentParser(object): # pylint: disable=too-few-public-methods self.parser.add_argument(*args, **kwargs) -HELP_TOPICS = ["all", "security", "paths", "automation", "testing", "plugins"] - - class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -524,6 +521,7 @@ class HelpfulArgumentParser(object): self.parser._add_config_file_help = False # pylint: disable=protected-access self.silent_parser = SilentParser(self.parser) + self.verb = None self.args = self.preprocess_args(args) help1 = self.prescan_for_flag("-h", self.help_topics) help2 = self.prescan_for_flag("--help", self.help_topics) @@ -540,13 +538,22 @@ class HelpfulArgumentParser(object): def preprocess_args(self, args): """Work around some limitations in argparse. - Currently, add the default verb "run" as a default. + Currently: add the default verb "run" as a default, and ensure that the + subcommand / verb comes last. """ + if "-h" in args or "--help" in args: + # all verbs double as help arguments; don't get them confused + self.verb = "help" + return args - for token in args: + for i, token in enumerate(args): if token in VERBS: - return args - return ["run"] + args + reordered = args[:i] + args[i+1:] + [args[i]] + self.verb = token + return reordered + + self.verb = "run" + return args + ["run"] def prescan_for_flag(self, flag, possible_arguments): """Checks cli input for flags. @@ -724,77 +731,79 @@ def create_parser(plugins, args): # For now unfortunately this constant just needs to match the code below; # there isn't an elegant way to autogenerate it in time. -VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", - "plugins", "--help"] - +VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"] +HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name, func): # pylint: disable=missing-docstring - subparser = subparsers.add_parser( - name, help=func.__doc__.splitlines()[0], description=func.__doc__) + def add_subparser(name): # pylint: disable=missing-docstring + if name == "plugins": + func = plugins_cmd + else: + func = eval(name) # pylint: disable=eval-used + h = func.__doc__.splitlines()[0] + subparser = subparsers.add_parser(name, help=h, description=func.__doc__) subparser.set_defaults(func=func) return subparser # the order of add_subparser() calls is important: it defines the # order in which subparser names will be displayed in --help - add_subparser("run", run) - parser_auth = add_subparser("auth", auth) - parser_install = add_subparser("install", install) - parser_revoke = add_subparser("revoke", revoke) - parser_rollback = add_subparser("rollback", rollback) - add_subparser("config_changes", config_changes) - parser_plugins = add_subparser("plugins", plugins_cmd) + # these add_subparser objects return objects to which arguments could be + # attached, but they have annoying arg ordering constrains so we use + # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820 + for v in VERBS: + add_subparser(v) - parser_auth.add_argument( - "--csr", type=read_file, help="Path to a Certificate Signing " - "Request (CSR) in DER format.") - parser_auth.add_argument( - "--cert-path", default=flag_default("auth_cert_path"), - help="When using --csr this is where certificate is saved.") - parser_auth.add_argument( - "--chain-path", default=flag_default("auth_chain_path"), - help="When using --csr this is where certificate chain is saved.") + helpful.add_group("auth", description="Options for modifying how a cert is obtained") + helpful.add_group("install", description="Options for modifying how a cert is deployed") + helpful.add_group("revoke", description="Options for revocation of certs") + helpful.add_group("rollback", description="Options for reverting config changes") + helpful.add_group("plugins", description="Plugin options") - parser_install.add_argument( - "--cert-path", required=True, help="Path to a certificate that " - "is going to be installed.") - parser_install.add_argument( - "--key-path", required=True, help="Accompanying private key") - parser_install.add_argument( - "--chain-path", help="Accompanying path to a certificate chain.") - parser_revoke.add_argument( - "--cert-path", type=read_file, help="Revoke a specific certificate.", - required=True) - parser_revoke.add_argument( - "--key-path", type=read_file, - help="Revoke certificate using its accompanying key. Useful if " - "Account Key is lost.") - - parser_rollback.add_argument( + helpful.add("auth", + "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") + helpful.add("rollback", "--checkpoints", type=int, metavar="N", default=flag_default("rollback_checkpoints"), help="Revert configuration N number of checkpoints.") - parser_plugins.add_argument( + helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") - parser_plugins.add_argument( - "--prepare", action="store_true", - help="Initialize and prepare plugins.") - parser_plugins.add_argument( + helpful.add("plugins", + "--prepare", action="store_true", help="Initialize and prepare plugins.") + helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", - const=interfaces.IAuthenticator, - help="Limit to authenticator plugins only.") - parser_plugins.add_argument( + const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") + helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") def _paths_parser(helpful): add = helpful.add + verb = helpful.verb helpful.add_group( "paths", description="Arguments changing execution paths & servers") + + cph = "Path to where cert is saved (with auth), installed (with install --csr) or revoked." + if verb == "auth": + add("paths", "--cert-path", default=flag_default("auth_cert_path"), help=cph) + elif verb == "revoke": + add("paths", "--cert-path", type=read_file, required=True, help=cph) + else: + add("paths", "--cert-path", help=cph, required=(verb == "install")) + + # revoke --key-path reads a file, install --key-path takes a string + add("paths", "--key-path", type=((verb == "revoke" and read_file) or str), + required=(verb == "install"), + help="Path to private key for cert creation or revocation (if account key is missing)") + + default_cp = None + if verb == "auth": + default_cp = flag_default("auth_chain_path") + add("paths", "--chain-path", default=default_cp, + help="Accompanying path to a certificate chain.") add("paths", "--config-dir", default=flag_default("config_dir"), help=config_help("config_dir")) add("paths", "--work-dir", default=flag_default("work_dir"), diff --git a/letsencrypt/display/enhancements.py b/letsencrypt/display/enhancements.py index 8edc72ba0..c56198161 100644 --- a/letsencrypt/display/enhancements.py +++ b/letsencrypt/display/enhancements.py @@ -11,7 +11,7 @@ from letsencrypt.display import util as display_util logger = logging.getLogger(__name__) # Define a helper function to avoid verbose code -util = zope.component.getUtility # pylint: disable=invalid-name +util = zope.component.getUtility def ask(enhancement): diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py index 43705e309..cb424a81b 100644 --- a/letsencrypt/display/ops.py +++ b/letsencrypt/display/ops.py @@ -12,7 +12,7 @@ from letsencrypt.display import util as display_util logger = logging.getLogger(__name__) # Define a helper function to avoid verbose code -util = zope.component.getUtility # pylint: disable=invalid-name +util = zope.component.getUtility def choose_plugin(prepared, question): diff --git a/letsencrypt/log.py b/letsencrypt/log.py index e800d37c9..6436f6fc2 100644 --- a/letsencrypt/log.py +++ b/letsencrypt/log.py @@ -25,7 +25,7 @@ class DialogHandler(logging.Handler): # pylint: disable=too-few-public-methods logging.Handler.__init__(self, level) self.height = height self.width = width - # "dialog" collides with module name... pylint: disable=invalid-name + # "dialog" collides with module name... self.d = dialog.Dialog() if d is None else d self.lines = [] diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py index 59598a35e..95ad56a0a 100644 --- a/letsencrypt/plugins/common.py +++ b/letsencrypt/plugins/common.py @@ -23,10 +23,10 @@ def dest_namespace(name): """ArgumentParser dest namespace (prefix of all destinations).""" return name.replace("-", "_") + "_" -private_ips_regex = re.compile( # pylint: disable=invalid-name +private_ips_regex = re.compile( r"(^127\.0\.0\.1)|(^10\.)|(^172\.1[6-9]\.)|" r"(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)") -hostname_regex = re.compile( # pylint: disable=invalid-name +hostname_regex = re.compile( r"^(([a-z0-9]|[a-z0-9][a-z0-9\-]*[a-z0-9])\.)*[a-z]+$", re.IGNORECASE) @@ -173,7 +173,7 @@ class Dvsni(object): achall.chall.encode("token") + '.pem') def _setup_challenge_cert(self, achall, s=None): - # pylint: disable=invalid-name + """Generate and write out challenge certificate.""" cert_path = self.get_cert_path(achall) key_path = self.get_key_path(achall) diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index ed29ead25..18ee56081 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/letsencrypt/tests/auth_handler_test.py @@ -355,7 +355,7 @@ class GenChallengePathTest(unittest.TestCase): class MutuallyExclusiveTest(unittest.TestCase): """Tests for letsencrypt.auth_handler.mutually_exclusive.""" - # pylint: disable=invalid-name,missing-docstring,too-few-public-methods + # pylint: disable=missing-docstring,too-few-public-methods class A(object): pass diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index a59bc414e..0a92aba62 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -2,6 +2,7 @@ import itertools import os import shutil +import StringIO import traceback import tempfile import unittest @@ -42,12 +43,54 @@ class CLITest(unittest.TestCase): ret = cli.main(args) return ret, stdout, stderr, client + def _call_stdout(self, args): + """ + Variant of _call that preserves stdout so that it can be mocked by the + caller. + """ + from letsencrypt import cli + args = ['--text', '--config-dir', self.config_dir, + '--work-dir', self.work_dir, '--logs-dir', self.logs_dir, + '--agree-eula'] + args + with mock.patch('letsencrypt.cli.sys.stderr') as stderr: + with mock.patch('letsencrypt.cli.client') as client: + ret = cli.main(args) + return ret, None, stderr, client + + def test_no_flags(self): - self.assertRaises(SystemExit, self._call, []) + with mock.patch('letsencrypt.cli.run') as mock_run: + self._call([]) + self.assertEqual(1, mock_run.call_count) def test_help(self): self.assertRaises(SystemExit, self._call, ['--help']) - self.assertRaises(SystemExit, self._call, ['--help all']) + self.assertRaises(SystemExit, self._call, ['--help', 'all']) + output = StringIO.StringIO() + with mock.patch('letsencrypt.cli.sys.stdout', new=output): + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'all']) + out = output.getvalue() + self.assertTrue("--configurator" in out) + self.assertTrue("how a cert is deployed" in out) + self.assertTrue("--manual-test-mode" in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['-h', 'nginx']) + out = output.getvalue() + self.assertTrue("--nginx-ctl" in out) + self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--checkpoints" not in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'plugins']) + out = output.getvalue() + self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--prepare" in out) + self.assertTrue("Plugin options" in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['-h']) + out = output.getvalue() + from letsencrypt import cli + self.assertTrue(cli.USAGE in out) + def test_rollback(self): _, _, _, client = self._call(['rollback']) diff --git a/letsencrypt/tests/log_test.py b/letsencrypt/tests/log_test.py index 50d0712e7..c1afd2c8a 100644 --- a/letsencrypt/tests/log_test.py +++ b/letsencrypt/tests/log_test.py @@ -8,7 +8,7 @@ import mock class DialogHandlerTest(unittest.TestCase): def setUp(self): - self.d = mock.MagicMock() # pylint: disable=invalid-name + self.d = mock.MagicMock() from letsencrypt.log import DialogHandler self.handler = DialogHandler(height=2, width=6, d=self.d) diff --git a/letsencrypt/tests/reverter_test.py b/letsencrypt/tests/reverter_test.py index 62c47f8d6..d31b6f2cc 100644 --- a/letsencrypt/tests/reverter_test.py +++ b/letsencrypt/tests/reverter_test.py @@ -85,7 +85,7 @@ class ReverterCheckpointLocalTest(unittest.TestCase): self.assertEqual(read_in(self.config1), "directive-dir1") def test_multiple_registration_fail_and_revert(self): - # pylint: disable=invalid-name + config3 = os.path.join(self.dir1, "config3.txt") update_file(config3, "Config3") config4 = os.path.join(self.dir2, "config4.txt") @@ -173,7 +173,7 @@ class ReverterCheckpointLocalTest(unittest.TestCase): self.assertRaises(errors.ReverterError, self.reverter.recovery_routine) def test_recover_checkpoint_revert_temp_failures(self): - # pylint: disable=invalid-name + mock_recover = mock.MagicMock( side_effect=errors.ReverterError("e")) @@ -291,7 +291,7 @@ class TestFullCheckpointsReverter(unittest.TestCase): errors.ReverterError, self.reverter.rollback_checkpoints, "one") def test_rollback_finalize_checkpoint_valid_inputs(self): - # pylint: disable=invalid-name + config3 = self._setup_three_checkpoints() # Check resulting backup directory @@ -334,7 +334,7 @@ class TestFullCheckpointsReverter(unittest.TestCase): @mock.patch("letsencrypt.reverter.os.rename") def test_finalize_checkpoint_no_rename_directory(self, mock_rename): - # pylint: disable=invalid-name + self.reverter.add_to_checkpoint(self.sets[0], "perm save") mock_rename.side_effect = OSError