From 1820b426e7180e5c729b7620ec4c154336b7cc81 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 28 Feb 2018 18:13:14 +0200 Subject: [PATCH] Review comment fixes --- certbot-apache/certbot_apache/configurator.py | 12 +++---- certbot-apache/certbot_apache/display_ops.py | 33 ++++++++++--------- certbot-apache/certbot_apache/obj.py | 2 +- .../certbot_apache/tests/configurator_test.py | 4 +-- certbot/tests/display/ops_test.py | 2 +- 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 224b1035d..16a939b55 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -303,13 +303,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): for vhost in vhosts: self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path) - def choose_vhosts(self, domain, cached=False, create_if_no_ssl=True): + def choose_vhosts(self, domain, create_if_no_ssl=True): """ Finds VirtualHosts that can be used with the provided domain :param str domain: Domain name to match VirtualHosts to - :param bool cached: Return VHosts from a list cached previously on this - execution. :param bool create_if_no_ssl: If found VirtualHost doesn't have a HTTPS counterpart, should one get created @@ -318,7 +316,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ if self._wildcard_domain(domain): - if cached and domain in self._wildcard_vhosts: + if domain in self._wildcard_vhosts: # Vhosts for a wildcard domain were already selected return self._wildcard_vhosts[domain] # Ask user which VHosts to support. @@ -368,7 +366,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if vhost.ssl: # Always prefer SSL vhosts filtered_vhosts[name] = vhost - elif name not in filtered_vhosts: + elif name not in filtered_vhosts and create_ssl: # Add if not in list previously filtered_vhosts[name] = vhost @@ -382,7 +380,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # if requested. return_vhosts = list() for vhost in dialog_output: - if not vhost.ssl and create_ssl: + if not vhost.ssl: return_vhosts.append(self.make_vhost_ssl(vhost)) else: return_vhosts.append(vhost) @@ -1493,7 +1491,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginError( "Unsupported enhancement: {0}".format(enhancement)) - vhosts = self.choose_vhosts(domain, cached=True, create_if_no_ssl=False) + vhosts = self.choose_vhosts(domain, create_if_no_ssl=False) try: for vhost in vhosts: func(vhost, options) diff --git a/certbot-apache/certbot_apache/display_ops.py b/certbot-apache/certbot_apache/display_ops.py index 427f5aaf9..097b84b96 100644 --- a/certbot-apache/certbot_apache/display_ops.py +++ b/certbot-apache/certbot_apache/display_ops.py @@ -24,15 +24,17 @@ def select_vhost_multiple(vhosts): """ if not vhosts: return list() - tags_list = [vhost.display_repr() for vhost in vhosts] - while True: - code, names = zope.component.getUtility(interfaces.IDisplay).checklist( - "Which VirtualHosts would you like to install the wildcard certificate for?", - tags=tags_list, force_interactive=True) - if code == display_util.OK: - return_vhosts = _reversemap_vhosts(names, vhosts) - return return_vhosts - return [] + tags_list = [vhost.display_repr()+"\n" for vhost in vhosts] + # Remove the extra newline from the last entry + if len(tags_list): + tags_list[-1] = tags_list[-1][:-1] + code, names = zope.component.getUtility(interfaces.IDisplay).checklist( + "Which VirtualHosts would you like to install the wildcard certificate for?", + tags=tags_list, force_interactive=True) + if code == display_util.OK: + return_vhosts = _reversemap_vhosts(names, vhosts) + return return_vhosts + return [] def _reversemap_vhosts(names, vhosts): """Helper function for select_vhost_multiple for mapping string @@ -41,7 +43,7 @@ def _reversemap_vhosts(names, vhosts): for selection in names: for vhost in vhosts: - if vhost.display_repr() == selection: + if vhost.display_repr().strip() == selection.strip(): return_vhosts.append(vhost) return return_vhosts @@ -57,12 +59,11 @@ def select_vhost(domain, vhosts): """ if not vhosts: return None - while True: - code, tag = _vhost_menu(domain, vhosts) - if code == display_util.OK: - return vhosts[tag] - else: - return None + code, tag = _vhost_menu(domain, vhosts) + if code == display_util.OK: + return vhosts[tag] + else: + return None def _vhost_menu(domain, vhosts): """Select an appropriate Apache Vhost. diff --git a/certbot-apache/certbot_apache/obj.py b/certbot-apache/certbot_apache/obj.py index 2b4d3913b..fcf3bfe08 100644 --- a/certbot-apache/certbot_apache/obj.py +++ b/certbot-apache/certbot_apache/obj.py @@ -173,7 +173,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods "File: {filename}\n" "Addresses: {addrs}\n" "Names: {names}\n" - "HTTPS: {https}\n\n".format( + "HTTPS: {https}\n".format( filename=self.filep, addrs=", ".join(str(addr) for addr in self.addrs), names=", ".join(self.get_names()), diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 3d39df429..4091f209f 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1367,11 +1367,11 @@ class MultipleVhostsTest(util.ApacheTest): # pylint: disable=protected-access 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]] + mock_select_vhs.return_value = [self.vh_truth[1]] vhs = self.config._choose_vhosts_wildcard("*.certbot.demo", create_ssl=False) self.assertFalse(mock_makessl.called) - self.assertEquals(vhs[0], self.vh_truth[3]) + self.assertEquals(vhs[0], self.vh_truth[1]) @mock.patch("certbot_apache.configurator.ApacheConfigurator._vhosts_for_wildcard") @mock.patch("certbot_apache.configurator.ApacheConfigurator.make_vhost_ssl") diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 98bc7fbf1..c4f58ba7c 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -300,7 +300,7 @@ class ChooseNamesTest(unittest.TestCase): from certbot.display.ops import get_valid_domains all_valid = ["example.com", "second.example.com", "also.example.com", "under_score.example.com", - "justtld"] + "justtld", "*.wildcard.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)