From 03028eee470aa8e875b804df5a1c59ea575f6d3e Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 24 Jan 2017 15:23:21 +0200 Subject: [PATCH] Final fixes --- certbot-apache/certbot_apache/configurator.py | 23 +++---- certbot-apache/certbot_apache/parser.py | 1 - .../certbot_apache/tests/configurator_test.py | 62 ++++++++++++------- certbot-apache/certbot_apache/tests/util.py | 2 - 4 files changed, 49 insertions(+), 39 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 829bf9c5b..820247d05 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -145,7 +145,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return os.path.join(self.config.config_dir, constants.MOD_SSL_CONF_DEST) - def prepare(self): """Prepare the authenticator/installer. @@ -288,11 +287,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if chain_path is not None: self.save_notes += "\tSSLCertificateChainFile %s\n" % chain_path - # Make sure vhost is enabled if distro with enabled / available - if self.conf("handle-sites"): - if not vhost.enabled: - self.enable_site(vhost) - def choose_vhost(self, target_name, temp=False): """Chooses a virtual host based on the given domain name. @@ -589,15 +583,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if realpath not in vhost_paths.keys(): vhs.append(new_vhost) vhost_paths[realpath] = new_vhost.filep - elif realpath == new_vhost.filep: - # Prefer "real" vhost paths instead of symlinked ones - # ex: sites-enabled/vh.conf -> sites-available/vh.conf - - # remove old (most likely) symlinked one - vhs = [v for v in vhs if v.filep != vhost_paths[realpath]] - vhs.append(new_vhost) - vhost_paths[realpath] = realpath - return vhs def is_name_vhost(self, target_addr): @@ -836,10 +821,18 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # This is for compliance with versions of Apache < 2.4 self._add_name_vhost_if_necessary(ssl_vhost) + # Enable the new vhost if needed + if self.conf("handle-sites"): + if not self.is_site_enabled(ssl_fp): + self.enable_site(ssl_vhost) + return ssl_vhost def _get_ssl_vhost_path(self, non_ssl_vh_fp): # Get filepath of new ssl_vhost + # Make sure we use the realpath (eg. sites-available instead of + # sites-enabled + non_ssl_vh_fp = os.path.realpath(non_ssl_vh_fp) if non_ssl_vh_fp.endswith(".conf"): return non_ssl_vh_fp[:-(len(".conf"))] + self.conf("le_vhost_ext") else: diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index d087dcdd8..97eb0fdef 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -618,7 +618,6 @@ class ApacheParser(object): for name in location: if os.path.isfile(os.path.join(self.root, name)): return os.path.join(self.root, name) - raise errors.NoInstallationError("Could not find configuration root") diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index e73ba2e13..a9490177d 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -92,6 +92,29 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.NotSupportedError, self.config.prepare) + @mock.patch("certbot_apache.parser.ApacheParser._parse_file") + @mock.patch("certbot_apache.parser.ApacheParser.update_runtime_variables") + def test_prepare_custom_vhost_dir(self, _, mock_parsefile): + server_root = self.config.conf("server-root") + vhost_root = self.config.conf("vhost-root") + version = self.config.version + with mock.patch("certbot_apache.constants.os_constant") as mock_const: + mock_const.side_effect = [True, "*.conf"] + parser.ApacheParser(self.config.aug, + server_root, + vhost_root, + version) + self.assertEqual(mock_parsefile.call_count, 9) + mock_parsefile.reset_mock() + + with mock.patch("certbot_apache.constants.os_constant") as mock_const: + mock_const.side_effect = [False, "*.conf"] + parser.ApacheParser(self.config.aug, + server_root, + vhost_root.replace( + "sites-available", "sites-enabled"), + self.config.version) + self.assertEqual(mock_parsefile.call_count, 10) def test_add_parser_arguments(self): # pylint: disable=no-self-use from certbot_apache.configurator import ApacheConfigurator @@ -163,9 +186,7 @@ class MultipleVhostsTest(util.ApacheTest): found += 1 break else: - if "default-ssl-port-only.conf" not in vhost.filep: - # Ignore default-ssl-port-only without ServerName or Alias - raise Exception("Missed: %s" % vhost) # pragma: no cover + raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 8) @@ -320,14 +341,9 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.MisconfigurationError, self.config.enable_mod, "ssl") - # def test_enable_site(self): - # Default 443 vhost - # self.assertFalse(self.vh_truth[8].enabled) - # self.config.enable_site(self.vh_truth[8]) - # self.assertTrue(self.vh_truth[8].enabled) - - # Go again to make sure nothing fails - #self.config.enable_site(self.vh_truth[8]) + def test_enable_site_already_enabled(self): + self.assertTrue(self.vh_truth[1].enabled) + self.config.enable_site(self.vh_truth[1]) def test_enable_site_failure(self): self.assertRaises( @@ -364,12 +380,12 @@ class MultipleVhostsTest(util.ApacheTest): # Verify one directive was found in the correct file self.assertEqual(len(loc_cert), 1) self.assertEqual( - os.path.realpath(configurator.get_file_path(loc_cert[0])), + configurator.get_file_path(loc_cert[0]), self.vh_truth[1].filep) self.assertEqual(len(loc_key), 1) self.assertEqual( - os.path.realpath(configurator.get_file_path(loc_key[0])), + configurator.get_file_path(loc_key[0]), self.vh_truth[1].filep) def test_deploy_cert_newssl_no_fullchain(self): @@ -430,17 +446,17 @@ class MultipleVhostsTest(util.ApacheTest): # Verify one directive was found in the correct file self.assertEqual(len(loc_cert), 1) self.assertEqual( - os.path.realpath(configurator.get_file_path(loc_cert[0])), + configurator.get_file_path(loc_cert[0]), self.vh_truth[1].filep) self.assertEqual(len(loc_key), 1) self.assertEqual( - os.path.realpath(configurator.get_file_path(loc_key[0])), + configurator.get_file_path(loc_key[0]), self.vh_truth[1].filep) self.assertEqual(len(loc_chain), 1) self.assertEqual( - os.path.realpath(configurator.get_file_path(loc_chain[0])), + configurator.get_file_path(loc_chain[0]), self.vh_truth[1].filep) # One more time for chain directive setting @@ -592,7 +608,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(set([obj.Addr.fromstring("*:443")]), ssl_vhost.addrs) self.assertEqual(ssl_vhost.name, "encryption-example.demo") self.assertTrue(ssl_vhost.ssl) - self.assertFalse(ssl_vhost.enabled) + self.assertTrue(ssl_vhost.enabled) self.assertTrue(self.config.parser.find_dir( "SSLCertificateFile", None, ssl_vhost.path, False)) @@ -873,10 +889,10 @@ class MultipleVhostsTest(util.ApacheTest): "SSLUseStapling", "on", ssl_vhost.path) self.assertEqual(len(ssl_use_stapling_aug_path), 1) - + ssl_vhost_aug_path = parser.get_aug_path(ssl_vhost.filep) stapling_cache_aug_path = self.config.parser.find_dir('SSLStaplingCache', "shmcb:/var/run/apache2/stapling_cache(128000)", - ssl_vhost.path) + ssl_vhost_aug_path) self.assertEqual(len(stapling_cache_aug_path), 1) @@ -1294,13 +1310,17 @@ class AugeasVhostsTest(util.ApacheTest): for name in names: self.assertFalse(name in self.config.choose_vhost(name).aliases) - def test_choose_vhost_without_matching_wildcard(self): + @mock.patch("certbot_apache.obj.VirtualHost.conflicts") + def test_choose_vhost_without_matching_wildcard(self, mock_conflicts): + mock_conflicts.return_value = False mock_path = "certbot_apache.display_ops.select_vhost" with mock.patch(mock_path, lambda _, vhosts: vhosts[0]): for name in ("a.example.net", "other.example.net"): self.assertTrue(name in self.config.choose_vhost(name).aliases) - def test_choose_vhost_wildcard_not_found(self): + @mock.patch("certbot_apache.obj.VirtualHost.conflicts") + def test_choose_vhost_wildcard_not_found(self, mock_conflicts): + mock_conflicts.return_value = False mock_path = "certbot_apache.display_ops.select_vhost" names = ( "abc.example.net", "not.there.tld", "aa.wildcard.tld" diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 2eecf42e4..5dda6b4b2 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -121,8 +121,6 @@ def get_vh_truth(temp_dir, config_name): prefix = os.path.join( temp_dir, config_name, "apache2/sites-enabled") - alt_prefix = os.path.join( - temp_dir, config_name, "apache2/sites-available") aug_pre = "/files" + prefix vh_truth = [ obj.VirtualHost(