From 85efa34ec4bccc8e3f8f5f7c999c8461210d565b Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 17 Apr 2018 13:19:08 +0300 Subject: [PATCH] Added tests and addressed review comments --- certbot/main.py | 11 ++-- certbot/tests/client_test.py | 23 +++++++++ certbot/tests/main_test.py | 98 +++++++++++++++++++++++++++++++++++- 3 files changed, 126 insertions(+), 6 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index e753ccdaf..dd0991c8d 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -893,14 +893,15 @@ def enhance(config, plugins): config, "enhance", allow_multiple=False, custom_prompt=certname_question)[0] cert_domains = cert_manager.domains_for_certname(config, config.certname) - domain_question = ("Which domain names would you like to enable the selected " - "enhancements for?") if config.noninteractive_mode: - domains = cert_manager.domains_for_certname(config, config.certname) + domains = cert_domains else: + domain_question = ("Which domain names would you like to enable the " + "selected enhancements for?") domains = display_ops.choose_values(cert_domains, domain_question) - if not domains: - raise errors.Error("No domains found to enhance.") + if not domains: + raise errors.Error("User cancelled the domain selection. No domains " + "defined, exiting.") if not config.chain_path: lineage = cert_manager.lineage_for_certname(config, config.certname) config.chain_path = lineage.chain_path diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 0f2c58161..6add141d4 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -433,6 +433,22 @@ class EnhanceConfigTest(ClientTestCommon): self.client.installer.enhance.assert_not_called() mock_enhancements.ask.assert_not_called() + @mock.patch("certbot.client.logger") + def test_already_exists_header(self, mock_log): + self.config.hsts = True + self._test_with_already_existing() + self.assertTrue(mock_log.warning.called) + self.assertEquals(mock_log.warning.call_args[0][1], + 'Strict-Transport-Security') + + @mock.patch("certbot.client.logger") + def test_already_exists_redirect(self, mock_log): + self.config.redirect = True + self._test_with_already_existing() + self.assertTrue(mock_log.warning.called) + self.assertEquals(mock_log.warning.call_args[0][1], + 'redirect') + def test_no_ask_hsts(self): self.config.hsts = True self._test_with_all_supported() @@ -508,6 +524,13 @@ class EnhanceConfigTest(ClientTestCommon): self.assertEqual(self.client.installer.save.call_count, 1) self.assertEqual(self.client.installer.restart.call_count, 1) + def _test_with_already_existing(self): + self.client.installer = mock.MagicMock() + self.client.installer.supported_enhancements.return_value = [ + "ensure-http-header", "redirect", "staple-ocsp"] + self.client.installer.enhance.side_effect = errors.PluginEnhancementAlreadyPresent() + self.client.enhance_config([self.domain], None) + class RollbackTest(unittest.TestCase): """Tests for certbot.client.rollback.""" diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 5e7fb32e2..060534542 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1533,6 +1533,103 @@ class MakeOrVerifyNeededDirs(test_util.ConfigTestCase): strict=self.config.strict_permissions) +class CertManagerTest(unittest.TestCase): + """Tests for certbot.cert_manager.""" + + def setUp(self): + self.get_utility_patch = test_util.patch_get_utility() + self.mock_get_utility = self.get_utility_patch.start() + plugins = disco.PluginsRegistry.find_all() + self.config = configuration.NamespaceConfig( + cli.prepare_and_parse_args(plugins, ["enhance"])) + + def tearDown(self): + self.get_utility_patch.stop() + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + prompt = "Which certificate would you" + self.mock_get_utility().menu.return_value = ('ok', 0) + self.assertEquals( + cert_manager.get_certnames( + self.config, "verb", allow_multiple=False), ['example.com']) + self.assertTrue( + prompt in self.mock_get_utility().menu.call_args[0][0]) + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames_custom_prompt(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + prompt = "custom prompt" + self.mock_get_utility().menu.return_value = ('ok', 0) + self.assertEquals( + cert_manager.get_certnames( + self.config, "verb", allow_multiple=False, custom_prompt=prompt), + ['example.com']) + self.assertEquals(self.mock_get_utility().menu.call_args[0][0], + prompt) + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames_user_abort(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + self.mock_get_utility().menu.return_value = ('cancel', 0) + self.assertRaises( + errors.Error, + cert_manager.get_certnames, + self.config, "erroring_anyway", allow_multiple=False) + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames_allow_multiple(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + prompt = "Which certificate(s) would you" + self.mock_get_utility().checklist.return_value = ('ok', ['example.com']) + self.assertEquals( + cert_manager.get_certnames( + self.config, "verb", allow_multiple=True), ['example.com']) + self.assertTrue( + prompt in self.mock_get_utility().checklist.call_args[0][0]) + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames_allow_multiple_custom_prompt(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + prompt = "custom prompt" + self.mock_get_utility().checklist.return_value = ('ok', ['example.com']) + self.assertEquals( + cert_manager.get_certnames( + self.config, "verb", allow_multiple=True, custom_prompt=prompt), + ['example.com']) + self.assertEquals( + self.mock_get_utility().checklist.call_args[0][0], + prompt) + + @mock.patch('certbot.storage.renewal_conf_files') + @mock.patch('certbot.storage.lineagename_for_filename') + def test_get_certnames_allow_multiple_user_abort(self, mock_name, mock_files): + mock_files.return_value = ['example.com.conf'] + mock_name.return_value = 'example.com' + from certbot import cert_manager + self.mock_get_utility().checklist.return_value = ('cancel', []) + self.assertRaises( + errors.Error, + cert_manager.get_certnames, + self.config, "erroring_anyway", allow_multiple=True) + + class EnhanceTest(unittest.TestCase): """Tests for certbot.main.enhance.""" @@ -1619,7 +1716,6 @@ class EnhanceTest(unittest.TestCase): self.assertTrue(mock_client.enhance_config.called) self.assertFalse(mock_choose.called) - @mock.patch('certbot.main.display_ops.choose_values') @mock.patch('certbot.main.plug_sel.record_chosen_plugins') def test_user_abort_domains(self, _rec, mock_choose):