From 1b55f94e73c7a25be8f932da54a73a9baffaccd3 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 23 Feb 2018 17:31:00 +0200 Subject: [PATCH] Present dialog only once per domain, added tests --- certbot-apache/certbot_apache/configurator.py | 22 +++-- certbot-apache/certbot_apache/obj.py | 12 ++- .../certbot_apache/tests/configurator_test.py | 86 +++++++++++++++++++ .../certbot_apache/tests/display_ops_test.py | 30 +++++++ certbot/tests/display/ops_test.py | 2 +- certbot/tests/main_test.py | 8 +- 6 files changed, 145 insertions(+), 15 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 85ca7a924..2a489a2a6 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -153,9 +153,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.assoc = dict() # Outstanding challenges self._chall_out = set() - # List of vhosts configured for wildcard certificates on this run. + # List of vhosts configured per wildcard domain on this run. # used by deploy_cert() and enhance() - self.wildcard_vhosts = list() + self.wildcard_vhosts = dict() # Maps enhancements to vhosts we've enabled the enhancement for self._enhanced_vhosts = defaultdict(set) @@ -306,7 +306,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): for vhost in deploy_vhosts: self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path) - self.wildcard_vhosts.append(vhost) + if domain not in self.wildcard_vhosts.keys(): + self.wildcard_vhosts[domain] = list() + self.wildcard_vhosts[domain].append(vhost) else: vhost = self.choose_vhost(domain) self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path) @@ -1476,10 +1478,16 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginError( "Unsupported enhancement: {0}".format(enhancement)) try: - # Handle virtualhosts configured for a wildcard certificate - if self.wildcard_domain(domain) and self.wildcard_vhosts: - for vhost in self.wildcard_vhosts: - func(vhost, options) + if self.wildcard_domain(domain): + # Handle virtualhosts configured for a wildcard domain + if domain in self.wildcard_vhosts.keys(): + # Vhosts for a wildcard domain were already selected + for vhost in self.wildcard_vhosts[domain]: + func(vhost, options) + else: + # Ask which Vhosts to enhance for wildcard domain + for vhost in self.choose_vhosts_wildcard(domain, create_ssl=False): + func(vhost, options) else: func(self.choose_vhost(domain), options) except errors.PluginError: diff --git a/certbot-apache/certbot_apache/obj.py b/certbot-apache/certbot_apache/obj.py index d6ec9a5ed..2b4d3913b 100644 --- a/certbot-apache/certbot_apache/obj.py +++ b/certbot-apache/certbot_apache/obj.py @@ -168,7 +168,17 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods modmacro="Yes" if self.modmacro else "No")) def display_repr(self): - return str(self) + """Return a representation of VHost to be used in dialog""" + return ( + "File: {filename}\n" + "Addresses: {addrs}\n" + "Names: {names}\n" + "HTTPS: {https}\n\n".format( + filename=self.filep, + addrs=", ".join(str(addr) for addr in self.addrs), + names=", ".join(self.get_names()), + https="Yes" if self.ssl else "No")) + def __eq__(self, other): if isinstance(other, self.__class__): diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 8f34d33d3..0962d5888 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1337,6 +1337,92 @@ class MultipleVhostsTest(util.ApacheTest): self.config.enable_mod, "whatever") + def test_wildcard_domain(self): + cases = {u"*.example.org": True, b"*.x.example.org": True, + u"a.example.org": False, b"a.x.example.org": False} + for key in cases.keys(): + self.assertEqual(self.config.wildcard_domain(key), cases[key]) + + def test_choose_vhosts_wildcard(self): + mock_path = "certbot_apache.display_ops.select_vhost_multiple" + with mock.patch(mock_path) as mock_select_vhs: + mock_select_vhs.return_value = [self.vh_truth[3]] + vhs = self.config.choose_vhosts_wildcard("*.certbot.demo", + create_ssl=True) + # Check that the dialog was called with one vh: certbot.demo + self.assertEquals(mock_select_vhs.call_args[0][0][0], self.vh_truth[3]) + self.assertEquals(len(mock_select_vhs.call_args_list), 1) + + # And the actual returned values + self.assertEquals(len(vhs), 1) + self.assertTrue(vhs[0].name == "certbot.demo") + self.assertTrue(vhs[0].ssl) + + self.assertFalse(vhs[0] == self.vh_truth[3]) + + @mock.patch("certbot_apache.configurator.ApacheConfigurator.make_vhost_ssl") + def test_choose_vhosts_wildcard_no_ssl(self, mock_makessl): + mock_path = "certbot_apache.display_ops.select_vhost_multiple" + with mock.patch(mock_path) as mock_select_vhs: + mock_select_vhs.return_value = [self.vh_truth[3]] + vhs = self.config.choose_vhosts_wildcard("*.certbot.demo", + create_ssl=False) + self.assertFalse(mock_makessl.called) + self.assertEquals(vhs[0], self.vh_truth[3]) + + @mock.patch("certbot_apache.configurator.ApacheConfigurator.vhosts_for_wildcard") + @mock.patch("certbot_apache.configurator.ApacheConfigurator.make_vhost_ssl") + def test_choose_vhosts_wildcard_already_ssl(self, mock_makessl, mock_vh_for_w): + # Already SSL vhost + mock_vh_for_w.return_value = [self.vh_truth[7]] + mock_path = "certbot_apache.display_ops.select_vhost_multiple" + with mock.patch(mock_path) as mock_select_vhs: + mock_select_vhs.return_value = [self.vh_truth[7]] + vhs = self.config.choose_vhosts_wildcard("whatever", + create_ssl=True) + self.assertEquals(mock_select_vhs.call_args[0][0][0], self.vh_truth[7]) + self.assertEquals(len(mock_select_vhs.call_args_list), 1) + # Ensure that make_vhost_ssl was not called, vhost.ssl == true + self.assertFalse(mock_makessl.called) + + # And the actual returned values + self.assertEquals(len(vhs), 1) + self.assertTrue(vhs[0].ssl) + self.assertEquals(vhs[0], self.vh_truth[7]) + + + def test_deploy_cert_wildcard(self): + mock_choose_vhosts = mock.MagicMock() + mock_choose_vhosts.return_value = [self.vh_truth[7]] + self.config.choose_vhosts_wildcard = mock_choose_vhosts + mock_d = "certbot_apache.configurator.ApacheConfigurator._deploy_cert" + with mock.patch(mock_d): + self.config.deploy_cert("*.wildcard.example.org", "/tmp/path", + "/tmp/path", "/tmp/path", "/tmp/path") + self.assertTrue("*.wildcard.example.org" in + self.config.wildcard_vhosts.keys()) + self.assertTrue(self.vh_truth[7] in + self.config.wildcard_vhosts["*.wildcard.example.org"]) + + @mock.patch("certbot_apache.configurator.ApacheConfigurator.choose_vhosts_wildcard") + def test_enhance_wildcard_after_install(self, mock_choose): + self.config.parser.modules.add("mod_ssl.c") + self.config.parser.modules.add("headers_module") + self.config.wildcard_vhosts["*.certbot.demo"] = [self.vh_truth[3]] + self.config.enhance("*.certbot.demo", "ensure-http-header", + "Upgrade-Insecure-Requests") + self.assertFalse(mock_choose.called) + + @mock.patch("certbot_apache.configurator.ApacheConfigurator.choose_vhosts_wildcard") + def test_enhance_wildcard_no_install(self, mock_choose): + mock_choose.return_value = [self.vh_truth[3]] + self.config.parser.modules.add("mod_ssl.c") + self.config.parser.modules.add("headers_module") + self.config.enhance("*.certbot.demo", "ensure-http-header", + "Upgrade-Insecure-Requests") + self.assertTrue(mock_choose.called) + + class AugeasVhostsTest(util.ApacheTest): """Test vhosts with illegal names dependent on augeas version.""" # pylint: disable=protected-access diff --git a/certbot-apache/certbot_apache/tests/display_ops_test.py b/certbot-apache/certbot_apache/tests/display_ops_test.py index e59d411bd..df5cdbac0 100644 --- a/certbot-apache/certbot_apache/tests/display_ops_test.py +++ b/certbot-apache/certbot_apache/tests/display_ops_test.py @@ -11,9 +11,39 @@ from certbot.tests import util as certbot_util from certbot_apache import obj +from certbot_apache.display_ops import select_vhost_multiple from certbot_apache.tests import util +class SelectVhostMultiTest(unittest.TestCase): + """Tests for certbot_apache.display_ops.select_vhost_multiple.""" + + def setUp(self): + self.base_dir = "/example_path" + self.vhosts = util.get_vh_truth( + self.base_dir, "debian_apache_2_4/multiple_vhosts") + + def test_select_no_input(self): + self.assertFalse(select_vhost_multiple([])) + + @certbot_util.patch_get_utility() + def test_select_correct(self, mock_util): + mock_util().checklist.return_value = ( + display_util.OK, [self.vhosts[3].display_repr(), + self.vhosts[2].display_repr()]) + vhs = select_vhost_multiple([self.vhosts[3], + self.vhosts[2], + self.vhosts[1]]) + self.assertTrue(self.vhosts[2] in vhs) + self.assertTrue(self.vhosts[3] in vhs) + self.assertFalse(self.vhosts[1] in vhs) + + @certbot_util.patch_get_utility() + def test_select_cancel(self, mock_util): + mock_util().checklist.return_value = (display_util.CANCEL, "whatever") + vhs = select_vhost_multiple([self.vhosts[2], self.vhosts[3]]) + self.assertFalse(vhs) + class SelectVhostTest(unittest.TestCase): """Tests for certbot_apache.display_ops.select_vhost.""" diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 57d82f839..98bc7fbf1 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -301,7 +301,7 @@ class ChooseNamesTest(unittest.TestCase): all_valid = ["example.com", "second.example.com", "also.example.com", "under_score.example.com", "justtld"] - all_invalid = ["öóòps.net", "*.wildcard.com", "uniçodé.com"] + all_invalid = ["öóòps.net", "uniçodé.com"] two_valid = ["example.com", "úniçøde.com", "also.example.com"] self.assertEqual(get_valid_domains(all_valid), all_valid) self.assertEqual(get_valid_domains(all_invalid), []) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 518653a53..5c33d099f 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1,3 +1,4 @@ +# coding=utf-8 """Tests for certbot.main.""" # pylint: disable=too-many-lines from __future__ import print_function @@ -939,11 +940,6 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met self.assertRaises(errors.ConfigurationError, self._call, ['-d', (('a' * 50) + '.') * 10]) - # Wildcard - self.assertRaises(errors.ConfigurationError, - self._call, - ['-d', '*.wildcard.tld']) - # Bare IP address (this is actually a different error message now) self.assertRaises(errors.ConfigurationError, self._call, @@ -1232,7 +1228,7 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met def test_renew_with_bad_domain(self): renewalparams = {'authenticator': 'webroot'} - names = ['*.example.com'] + names = ['exámple.com'] self._test_renew_common(renewalparams=renewalparams, error_expected=True, names=names, assert_oc_called=False)