From e5f4d0cb5c91fa6db1f98757a87eb4f1c90f424f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 15 Nov 2016 11:56:05 -0800 Subject: [PATCH] Fix reinstall message (#3784) * Changed informational messages because of confusing message on reinstallation. Certbot prompts the user when it detects that an appropriately fresh certificate is already available: You have an existing certificate that contains exactly the same domains you requested and isn't close to expiry. (ref: ) What would you like to do? ------------------------------------------------------------------------------- 1: Attempt to reinstall this existing certificate 2: Renew & replace the cert (limit ~5 per 7 days) ------------------------------------------------------------------------------- Select the appropriate number [1-2] then [enter] (press 'c' to cancel): 1 On selecting '1' (reinstall), the resulting message is: ------------------------------------------------------------------------------- Your existing certificate has been successfully reinstalled, and the new certificate has been installed. The new certificate covers the following domains: https:// You should test your configuration at: https://www.ssllabs.com/ssltest/analyze.html?d= ------------------------------------------------------------------------------- "Your existing certificate has been successfully reinstalled" <-- Okay "and the new certificate has been installed." <-- Wait, what? The issue appears to come from assumptions in certbot/certbot/main.py It uses `len(lineage.available_versions("cert"))` to determine if this was a fresh install or renewal, and then calls either `display_ops.success_renewal()` (which produces the "existing certificate ... and the new certificate" language) or `display_ops.success_installation()` (which has no messaging about existing vs. new certificates). The len(lineage) test isn't the right way to make this choice. The certificate's lineage length doesn't imply anything about whether we've just obtained a new certificate, because there is no new certificate in the case of a "reinstall" action. The new logic calls `display_ops.success_installation()` on all "reinstall" actions, and otherwise employs the existing `len(lineage)` test. Additionally the `display_ops.success_installation()` has been enhanced to accept an action parameter, and has the message reworded slightly to make sense regardless of the action passed. The messaging is mostly unchanged if it's called without the action parameter: Original message: ------------------------------------------------------------------------------- Congratulations! You have successfully enabled https:// You should test your configuration at: https://www.ssllabs.com/ssltest/analyze.html?d= ------------------------------------------------------------------------------- New message on initial install: ------------------------------------------------------------------------------- Congratulations! You have successfully installed a certificate for https:// You should test your configuration at: https://www.ssllabs.com/ssltest/analyze.html?d= ------------------------------------------------------------------------------- New message on re-install: ------------------------------------------------------------------------------- Congratulations! You have successfully reinstalled a certificate for https:// You should test your configuration at: https://www.ssllabs.com/ssltest/analyze.html?d= ------------------------------------------------------------------------------- * Typo in display message. * Typo, characters transposed. * undo changes to certbot/display/ops.py * remove invalid todos * Test success_installation() called for reinstall * Simplify display_ops.success* functions * refactor and expand run() tests --- certbot/display/ops.py | 12 ++------ certbot/main.py | 4 +-- certbot/tests/display/ops_test.py | 2 +- certbot/tests/main_test.py | 50 +++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 12 deletions(-) diff --git a/certbot/display/ops.py b/certbot/display/ops.py index 6762c2244..97f050318 100644 --- a/certbot/display/ops.py +++ b/certbot/display/ops.py @@ -209,8 +209,6 @@ def _choose_names_manually(prompt_prefix=""): def success_installation(domains): """Display a box confirming the installation of HTTPS. - .. todo:: This should be centered on the screen - :param list domains: domain names which were enabled """ @@ -223,24 +221,20 @@ def success_installation(domains): pause=False) -def success_renewal(domains, action): +def success_renewal(domains): """Display a box confirming the renewal of an existing certificate. - .. todo:: This should be centered on the screen - :param list domains: domain names which were renewed - :param str action: can be "reinstall" or "renew" """ z_util(interfaces.IDisplay).notification( - "Your existing certificate has been successfully {3}ed, and the " + "Your existing certificate has been successfully renewed, and the " "new certificate has been installed.{1}{1}" "The new certificate covers the following domains: {0}{1}{1}" "You should test your configuration at:{1}{2}".format( _gen_https_names(domains), os.linesep, - os.linesep.join(_gen_ssl_lab_urls(domains)), - action), + os.linesep.join(_gen_ssl_lab_urls(domains))), pause=False) diff --git a/certbot/main.py b/certbot/main.py index 39c19bd7a..a84a77dc4 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -528,10 +528,10 @@ def run(config, plugins): # pylint: disable=too-many-branches,too-many-locals le_client.enhance_config(domains, config, lineage.chain) - if len(lineage.available_versions("cert")) == 1: + if action in ("newcert", "reinstall",): display_ops.success_installation(domains) else: - display_ops.success_renewal(domains, action) + display_ops.success_renewal(domains) _suggest_donation_if_appropriate(config, action) diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 82ed325c8..ebb695024 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -321,7 +321,7 @@ class SuccessRenewalTest(unittest.TestCase): @classmethod def _call(cls, names): from certbot.display.ops import success_renewal - success_renewal(names, "renew") + success_renewal(names) @mock.patch("certbot.display.ops.z_util") def test_success_renewal(self, mock_util): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 045a593e9..133606a19 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -13,9 +13,11 @@ from certbot import configuration from certbot import errors from certbot.plugins import disco as plugins_disco + class MainTest(unittest.TestCase): def setUp(self): pass + def tearDown(self): pass @@ -27,6 +29,54 @@ class MainTest(unittest.TestCase): ret = main._handle_identical_cert_request(mock.Mock(), mock_lineage) self.assertEqual(ret, ("reinstall", mock_lineage)) + +class RunTest(unittest.TestCase): + """Tests for certbot.main.run.""" + + def setUp(self): + self.domain = 'example.org' + self.patches = [ + mock.patch('certbot.main._auth_from_domains'), + mock.patch('certbot.main.display_ops.success_installation'), + mock.patch('certbot.main.display_ops.success_renewal'), + mock.patch('certbot.main._init_le_client'), + mock.patch('certbot.main._suggest_donation_if_appropriate')] + + self.mock_auth = self.patches[0].start() + self.mock_success_installation = self.patches[1].start() + self.mock_success_renewal = self.patches[2].start() + self.mock_init = self.patches[3].start() + self.mock_suggest_donation = self.patches[4].start() + + def tearDown(self): + for patch in self.patches: + patch.stop() + + def _call(self): + args = '-a webroot -i null -d {0}'.format(self.domain).split() + plugins = plugins_disco.PluginsRegistry.find_all() + config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, args)) + + from certbot.main import run + run(config, plugins) + + def test_newcert_success(self): + self.mock_auth.return_value = ('newcert', mock.Mock()) + self._call() + self.mock_success_installation.assert_called_once_with([self.domain]) + + def test_reinstall_success(self): + self.mock_auth.return_value = ('reinstall', mock.Mock()) + self._call() + self.mock_success_installation.assert_called_once_with([self.domain]) + + def test_renewal_success(self): + self.mock_auth.return_value = ('renewal', mock.Mock()) + self._call() + self.mock_success_renewal.assert_called_once_with([self.domain]) + + class ObtainCertTest(unittest.TestCase): """Tests for certbot.main.obtain_cert."""