99% test coverage

certbot-postfix/MANIFEST.in

@@ -1,3 +1,4 @@
 include LICENSE.txt
 include README.rst
 recursive-include certbot_postfix/testdata *
+recursive-include certbot_postfix/docs *

certbot-postfix/certbot_postfix/installer.py

@@ -189,7 +189,7 @@ class Installer(plugins_common.Installer):
         """Sets all parameters in var_dict to config file.
         """
         for param, acceptable in six.iteritems(var_dict):
-            if util.is_acceptable_value(param, self.postconf.get(param), acceptable):
+            if not util.is_acceptable_value(param, self.postconf.get(param), acceptable):
                 if isinstance(acceptable, tuple):
                     self.postconf.set(param, acceptable[0], acceptable)
                 else:
@@ -205,7 +205,7 @@ class Installer(plugins_common.Installer):
         for name, value in six.iteritems(updates):
             output_string += "{0} = {1}\n".format(name, value)
         output_string += "Is this okay?\n"
-        if not zope.component.getUtility(interfaces.IDisplay).yesno(output_string):
+        if not zope.component.getUtility(interfaces.IDisplay).yesno(output_string, default=False):
             raise errors.PluginError(
                 "Manually rejected configuration changes.\n"
                 "Try using --tls-only or --server-only to change a particular"
@@ -244,18 +244,11 @@ class Installer(plugins_common.Installer):
         self._confirm_changes()
 
     def enhance(self, domain, enhancement, options=None):
-        """Raises an exception for request for unsupported enhancement.
+        """Raises an exception since this installer doesn't support any enhancements.
         """
-        try:
-            func = self._enhance_func[enhancement]
-        except (KeyError, ValueError):
-            raise errors.PluginError(
-                "Unsupported enhancement: {0}".format(enhancement))
-        try:
-            func(domain, options)
-        except errors.PluginError:
-            logger.warning("Failed %s for %s", enhancement, domain)
-            raise
+        # pylint: disable=unused-argument
+        raise errors.PluginError(
+            "Unsupported enhancement: {0}".format(enhancement))
 
     def supported_enhancements(self):
         """Returns a list of supported enhancements.

certbot-postfix/certbot_postfix/postconf.py

@@ -105,8 +105,8 @@ class ConfigMain(util.PostfixUtilBase):
             args.append('{0}={1}'.format(name, value))
         try:
             self._get_output(args)
-        except:
-            raise errors.PluginError("Unable to save to Postfix config!")
+        except IOError as e:
+            raise errors.PluginError("Unable to save to Postfix config: %v", e)
         for name, value in six.iteritems(self._updated):
             self._db[name] = value
         self._updated = {}

certbot-postfix/certbot_postfix/tests/installer_test.py

@@ -1,7 +1,10 @@
 """Tests for certbot_postfix.installer."""
+from contextlib import contextmanager
+import copy
 import functools
 import os
 import pkg_resources
+import six
 import unittest
 
 import mock
@@ -9,7 +12,35 @@ import mock
 from certbot import errors
 from certbot.tests import util as certbot_test_util
 
+DEFAULT_MAIN_CF = {
+    "smtpd_tls_cert_file": "",
+    "smtpd_tls_key_file": "",
+    "smtpd_tls_dh1024_param_file": "",
+    "smtpd_tls_security_level": "none",
+    "smtpd_tls_auth_only": "",
+    "smtpd_tls_mandatory_protocols": "",
+    "smtpd_tls_protocols": "",
+    "smtpd_tls_ciphers": "",
+    "smtpd_exclude_ciphers": "",
+    "smtpd_tls_mandatory_ciphers": "",
+    "smtpd_tls_eecdh_grade": "medium",
+    "smtp_tls_security_level": "",
+    "smtp_tls_ciphers": "",
+    "smtp_exclude_ciphers": "",
+    "smtp_tls_mandatory_ciphers": "",
+    "mail_version": "3.2.3"
+}
+
+DEFAULT_MASTER_CF = {
+}
+
+def _main_cf_with(obj):
+    main_cf = copy.copy(DEFAULT_MAIN_CF)
+    main_cf.update(obj)
+    return main_cf
+
 class InstallerTest(certbot_test_util.ConfigTestCase):
+    # pylint: disable=too-many-public-methods
 
     def setUp(self):
         super(InstallerTest, self).setUp()
@@ -18,12 +49,84 @@ class InstallerTest(certbot_test_util.ConfigTestCase):
         self.config.postfix_ctl = "postfix"
         self.config.postfix_config_dir = self.tempdir
         self.config.postfix_config_utility = "postconf"
+        self.config.postfix_tls_only = False
+        self.config.postfix_server_only = False
         self.config.config_dir = self.tempdir
-        self.mock_postfix = MockPostfix()
-        self.mock_postconf = MockPostconf(self.tempdir, {"mail_version": "3.1.4"})
 
-    def test_confirm_changes(self):
-        pass
+    @mock.patch('certbot_postfix.installer.util.is_acceptable_value')
+    def test_set_vars(self, mock_is_acceptable_value):
+        mock_is_acceptable_value.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            mock_is_acceptable_value.return_value = False
+
+    @mock.patch('certbot_postfix.installer.util.is_acceptable_value')
+    def test_acceptable_value(self, mock_is_acceptable_value):
+        mock_is_acceptable_value.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            mock_is_acceptable_value.return_value = False
+
+    @certbot_test_util.patch_get_utility()
+    def test_confirm_changes_no_raises_error(self, mock_util):
+        mock_util().yesno.return_value = False
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            self.assertRaises(errors.PluginError, installer.deploy_cert,
+                              'example.com', 'cert_path', 'key_path',
+                              'chain_path', 'fullchain_path')
+
+    @certbot_test_util.patch_get_utility()
+    def test_save(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.postconf.flush = mock.Mock()
+            installer.reverter = mock.Mock()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            installer.save()
+            self.assertEqual(installer.save_notes, [])
+            self.assertEqual(installer.postconf.flush.call_count, 1)
+            self.assertEqual(installer.reverter.add_to_checkpoint.call_count, 1)
+
+    @certbot_test_util.patch_get_utility()
+    def test_save_with_title(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.postconf.flush = mock.Mock()
+            installer.reverter = mock.Mock()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            installer.save(title='new_file!')
+            self.assertEqual(installer.reverter.finalize_checkpoint.call_count, 1)
+
+    @certbot_test_util.patch_get_utility()
+    def test_rollback_checkpoints_resets_postconf(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            installer.rollback_checkpoints()
+            self.assertEqual(installer.postconf.get_changes(), {})
+
+    @certbot_test_util.patch_get_utility()
+    def test_recovery_routine_resets_postconf(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            installer.recovery_routine()
+            self.assertEqual(installer.postconf.get_changes(), {})
+
+    def test_restart(self):
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.restart()
+            self.assertEqual(installer.postfix.restart.call_count, 1)
 
     def test_add_parser_arguments(self):
         options = set(('ctl', 'config-dir', 'config-utility',
@@ -37,173 +140,171 @@ class InstallerTest(certbot_test_util.ConfigTestCase):
             self.assertTrue(call[0][0] in options)
 
     def test_no_postconf_prepare(self):
-        installer = self._create_installer()
+        with create_installer(self.config) as installer:
+            installer_path = "certbot_postfix.installer"
+            exe_exists_path = installer_path + ".certbot_util.exe_exists"
+            path_surgery_path = "certbot_postfix.util.plugins_util.path_surgery"
+            with mock.patch(path_surgery_path, return_value=False):
+                with mock.patch(exe_exists_path, return_value=False):
+                    self.assertRaises(errors.NoInstallationError,
+                                      installer.prepare)
 
-        installer_path = "certbot_postfix.installer"
-        exe_exists_path = installer_path + ".certbot_util.exe_exists"
-        path_surgery_path = "certbot_postfix.util.plugins_util.path_surgery"
-
-        with mock.patch(path_surgery_path, return_value=False):
-            with mock.patch(exe_exists_path, return_value=False):
-                self.assertRaises(errors.NoInstallationError,
-                                  installer.prepare)
-
-    @mock.patch("certbot_postfix.installer.certbot_util.exe_exists")
-    def test_old_version(self, mock_exe_exists):
-        installer = self._create_installer()
-        mock_exe_exists.return_value = True
-        self.mock_postconf.set("mail_version", "0.0.1")
-        self._mock_postfix_and_call(
-            self.assertRaises, errors.NotSupportedError, installer.prepare)
+    def test_old_version(self):
+        with create_installer(self.config, main_cf=_main_cf_with({'mail_version': '0.0.1'}))\
+                as installer:
+            self.assertRaises(errors.NotSupportedError, installer.prepare)
 
     def test_lock_error(self):
-        assert_raises = functools.partial(self.assertRaises,
-                                          errors.PluginError,
-                                          self._create_prepared_installer)
-        certbot_test_util.lock_and_call(assert_raises, self.tempdir)
+        with create_installer(self.config) as installer:
+            assert_raises = functools.partial(self.assertRaises,
+                                              errors.PluginError,
+                                              installer.prepare)
+            certbot_test_util.lock_and_call(assert_raises, self.tempdir)
 
     def test_more_info(self):
-        installer = self._create_prepared_installer()
-        version = "3.1.2"
-        # self.mock_postfix.set_value("mail_version", version)
-        self.mock_postconf.set("mail_version", version)
-
-        output = self._mock_postfix_and_call(installer.more_info)
-        self.assertTrue("Postfix" in output)
-        self.assertTrue(self.tempdir in output)
-        self.assertTrue(version in output)
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            output = installer.more_info()
+            self.assertTrue("Postfix" in output)
+            self.assertTrue(self.tempdir in output)
+            self.assertTrue(DEFAULT_MAIN_CF["mail_version"] in output)
 
     def test_get_all_names(self):
         config = {"mydomain": "example.org",
                   "myhostname": "mail.example.org",
                   "myorigin": "example.org"}
-        for name, value in config.items():
-            # self.mock_postfix.set_value(name, value)
-            self.mock_postconf.set(name, value)
+        with create_installer(self.config, main_cf=_main_cf_with(config)) as installer:
+            installer.prepare()
+            result = installer.get_all_names()
+            self.assertEqual(result, set(config.values()))
 
-        installer = self._create_prepared_installer()
-        result = self._mock_postfix_and_call(installer.get_all_names)
-        self.assertEqual(result, set(config.values()))
+    @certbot_test_util.patch_get_utility()
+    def test_deploy(self, mock_util):
+        mock_util().yesno.return_value = True
+        from certbot_postfix import constants
+        with create_installer(self.config) as installer:
+            installer.prepare()
 
-    def test_deploy(self):
-        installer = self._create_prepared_installer()
-
-        def deploy_cert(domain):
-            """Calls deploy_cert for the given domain.
-
-            :param str domain: domain to deploy cert for
-
-            """
             # pylint: disable=protected-access
-            installer._confirm_changes = lambda: "noop"
-            installer.deploy_cert(domain, "foo", "bar", "baz", "qux")
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            changes = installer.postconf.get_changes()
+            expected = {}
+            expected.update(constants.TLS_SERVER_VARS)
+            expected.update(constants.DEFAULT_SERVER_VARS)
+            expected.update(constants.DEFAULT_CLIENT_VARS)
+            self.assertEqual(changes['smtpd_tls_key_file'], 'key_path')
+            self.assertEqual(changes['smtpd_tls_cert_file'], 'cert_path')
+            for name, value in six.iteritems(expected):
+                if isinstance(value, tuple):
+                    self.assertEqual(changes[name], value[0])
+                else:
+                    self.assertEqual(changes[name], value)
 
-        self._mock_postfix_and_call(deploy_cert, "example.org")
-        # No calls to postconf are expected so mock isn't needed
-        deploy_cert("mail.example.org")
+    @certbot_test_util.patch_get_utility()
+    def test_tls_only(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.conf = lambda x: x == 'tls_only'
+            installer.postconf.set = mock.Mock()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            self.assertEqual(installer.postconf.set.call_count, 8)
+
+    @certbot_test_util.patch_get_utility()
+    def test_server_only(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.conf = lambda x: x == 'server_only'
+            installer.postconf.set = mock.Mock()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            self.assertEqual(installer.postconf.set.call_count, 11)
+
+    @certbot_test_util.patch_get_utility()
+    def test_tls_and_server_only(self, mock_util):
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            installer.conf = lambda x: True
+            installer.postconf.set = mock.Mock()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            self.assertEqual(installer.postconf.set.call_count, 4)
+
+    @certbot_test_util.patch_get_utility()
+    def test_deploy_twice(self, mock_util):
+        # Deploying twice on the same installer shouldn't do anything!
+        mock_util().yesno.return_value = True
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            from certbot_postfix.postconf import ConfigMain
+            with mock.patch.object(ConfigMain, 'set', wraps=installer.postconf.set) as fake_set:
+                installer.deploy_cert('example.com', "cert_path", "key_path",
+                                      "chain_path", "fullchain_path")
+                self.assertEqual(fake_set.call_count, 15)
+                fake_set.reset_mock()
+                installer.deploy_cert('example.com', "cert_path", "key_path",
+                                      "chain_path", "fullchain_path")
+                fake_set.assert_not_called()
+
+    @certbot_test_util.patch_get_utility()
+    def test_deploy_already_secure(self, mock_util):
+        # Should not overwrite "more-secure" parameters
+        mock_util().yesno.return_value = True
+        more_secure = {
+            'smtpd_tls_security_level': 'encrypt',
+            'smtpd_tls_protocols': '!SSLv3, !SSLv2, !TLSv1',
+            'smtpd_tls_eecdh_grade': 'strong'
+        }
+        with create_installer(self.config,\
+            main_cf=_main_cf_with(more_secure)) as installer:
+            installer.prepare()
+            installer.deploy_cert('example.com', "cert_path", "key_path",
+                                  "chain_path", "fullchain_path")
+            for param in more_secure.keys():
+                self.assertFalse(param in installer.postconf.get_changes())
 
     def test_enhance(self):
-        self.assertRaises(errors.PluginError,
-                          self._create_prepared_installer().enhance,
-                          "example.org", "redirect")
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            self.assertRaises(errors.PluginError,
+                              installer.enhance,
+                              "example.org", "redirect")
 
     def test_supported_enhancements(self):
-        self.assertEqual(
-            self._create_prepared_installer().supported_enhancements(),
-            [])
+        with create_installer(self.config) as installer:
+            installer.prepare()
+            self.assertEqual(installer.supported_enhancements(), [])
 
-    def _create_prepared_installer(self):
-        """Creates and returns a new prepared Postfix Installer.
-
-        Calls in prepare() are mocked out so the Postfix version check
-        is successful.
-
-        :returns: a prepared Postfix installer
-        :rtype: certbot_postfix.installer.Installer
-
-        """
-        installer = self._create_installer()
+@contextmanager
+def create_installer(config, main_cf=DEFAULT_MAIN_CF, master_cf=DEFAULT_MASTER_CF):
+# pylint: disable=dangerous-default-value
+    """Creates a Postfix installer with calls to `postconf` and `postfix` mocked out.
 
+    In particular, creates a ConfigMain object that does regular things, but seeds it
+    with values from `main_cf` and `master_cf` dicts.
+    """
+    from certbot_postfix.postconf import ConfigMain
+    from certbot_postfix import installer
+    def _mock_init_postconf(postconf, executable, handle_overrides, config_dir=None):
+        # pylint: disable=protected-access
+        super(ConfigMain, postconf).__init__(executable, config_dir)
+        postconf._handle_overrides = handle_overrides
+        postconf._db = main_cf
+        postconf._master_db = master_cf
+        postconf._updated = {}
+        # override get_default to get from main
+        postconf.get_default = lambda name: main_cf[name]
+    with mock.patch.object(ConfigMain, "__init__", _mock_init_postconf):
         exe_exists_path = "certbot_postfix.installer.certbot_util.exe_exists"
         with mock.patch(exe_exists_path, return_value=True):
-            self._mock_postfix_and_call(installer.prepare)
-
-        return installer
-
-    def _create_installer(self):
-        """Creates and returns a new Postfix Installer.
-
-        :returns: a new Postfix installer
-        :rtype: certbot_postfix.installer.Installer
-
-        """
-        name = "postfix"
-
-        from certbot_postfix import installer
-        return installer.Installer(self.config, name)
-
-    def _mock_postfix_and_call(self, func, *args, **kwargs):
-        """Calls func with mocked responses from Postfix utilities.
-
-        :param callable func: function to call with mocked args
-        :param tuple args: positional arguments to func
-        :param dict kwargs: keyword arguments to func
-
-        :returns: the return value of func
-
-        """
-
-        with mock.patch("certbot_postfix.installer.postconf.ConfigMain",
-                         return_value=self.mock_postconf):
             with mock.patch("certbot_postfix.installer.util.PostfixUtil",
-                             return_value=self.mock_postfix):
-                return func(*args, **kwargs)
-
-class MockPostfix(object):
-    """Mock utility for Postfix command-line wrapper.
-    """
-    def __init__(self):
-        pass
-
-    def test(self):
-        """Mocks Postfix's 'test' call."""
-        pass
-
-    def restart(self):
-        """Mocks Postfix's 'reload' call."""
-        pass
-
-class MockPostconf(object):
-    """Mock utility for Postconf command-line wrapper.
-    """
-    def __init__(self, tempdir, init_keys=None):
-        self._db = init_keys
-        if self._db is None:
-            self._db = {}
-        self._db['config_directory'] = tempdir
-
-    def get(self, name):
-        """Mocks Postconf object's 'get' call."""
-        if name not in self._db:
-            return None
-        return self._db[name]
-
-    def get_default(self, name):
-        """Mocks Postconf object's 'get_default' call."""
-        if name in self._db:
-            return self._db[name]
-        if name == "mail_version":
-            return "3.2.3"
-        return None
-
-    def set(self, name, value, check_override=None):
-        """Mocks Postconf object's 'set' call."""
-        # pylint: disable=unused-argument
-        self._db[name] = value
-
-    def flush(self):
-        """Mocks Postconf object's 'flush' call."""
-        pass
+                             return_value=mock.Mock()):
+                yield installer.Installer(config, "postfix")
 
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover
+

certbot-postfix/certbot_postfix/tests/postconf_test.py

@@ -3,6 +3,8 @@
 import mock
 import unittest
 
+from certbot import errors
+
 class PostConfTest(unittest.TestCase):
     """Tests for certbot_postfix.util.PostConf."""
     def setUp(self):
@@ -30,6 +32,7 @@ class PostConfTest(unittest.TestCase):
     @mock.patch('certbot_postfix.util.PostfixUtilBase._call')
     def test_set(self, mock_call):
         self.config.set('extra_param', 'other_value')
+        self.assertEqual(self.config.get('extra_param'), 'other_value')
         self.config.flush()
         mock_call.assert_called_with(['-e', 'extra_param=other_value'])
 
@@ -67,5 +70,27 @@ class PostConfTest(unittest.TestCase):
         self.config._handle_overrides = _check_overrides
         self.config.set('overridden_by_master', 'new_value')
 
+    @mock.patch('certbot_postfix.util.PostfixUtilBase._get_output')
+    def test_flush(self, mock_out):
+        self.config.set('default_parameter', 'new_value')
+        self.config.set('extra_param', 'another_value')
+        self.config.flush()
+        mock_out.assert_called_with(
+            ['-e', 'default_parameter=new_value', 'extra_param=another_value'])
+
+    @mock.patch('certbot_postfix.util.PostfixUtilBase._get_output')
+    def test_flush_updates_object(self, mock_out):
+        self.config.set('default_parameter', 'new_value')
+        self.config.flush()
+        mock_out.reset_mock()
+        self.config.set('default_parameter', 'new_value')
+        mock_out.assert_not_called()
+
+    @mock.patch('certbot_postfix.util.PostfixUtilBase._get_output')
+    def test_flush_throws_error_on_fail(self, mock_out):
+        mock_out.side_effect = [IOError("oh no!")]
+        self.config.set('default_parameter', 'new_value')
+        self.assertRaises(errors.PluginError, self.config.flush)
+
 if __name__ == '__main__':  # pragma: no cover
     unittest.main()

certbot-postfix/certbot_postfix/tests/util_test.py

@@ -26,6 +26,54 @@ class PostfixUtilBaseTest(unittest.TestCase):
         with mock.patch('certbot_postfix.util.verify_exe_exists'):
             self._create_object('existent')
 
+    @mock.patch('certbot_postfix.util.check_all_output')
+    def test_call_extends_args(self, mock_output):
+        # pylint: disable=protected-access
+        with mock.patch('certbot_postfix.util.verify_exe_exists'):
+            mock_output.return_value = 'expected'
+            postfix = self._create_object('executable')
+            postfix._call(['many', 'extra', 'args'])
+            mock_output.assert_called_with(['executable', 'many', 'extra', 'args'])
+            postfix._call()
+            mock_output.assert_called_with(['executable'])
+
+class PostfixUtilTest(unittest.TestCase):
+    def setUp(self):
+        # pylint: disable=protected-access
+        from certbot_postfix.util import PostfixUtil
+        self.postfix = PostfixUtil()
+        self.postfix._call = mock.Mock()
+        self.mock_call = self.postfix._call
+
+    def test_test(self):
+        self.postfix.test()
+        self.mock_call.assert_called_with(['check'])
+
+    def test_test_raises_error_when_check_fails(self):
+        self.mock_call.side_effect = [subprocess.CalledProcessError(None, None, None)]
+        self.assertRaises(errors.MisconfigurationError, self.postfix.test)
+        self.mock_call.assert_called_with(['check'])
+
+    def test_restart_while_running(self):
+        self.mock_call.side_effect = [subprocess.CalledProcessError(None, None, None), None]
+        self.postfix.restart()
+        self.mock_call.assert_called_with(['start'])
+
+    def test_restart_while_not_running(self):
+        self.postfix.restart()
+        self.mock_call.assert_called_with(['reload'])
+
+    def test_restart_raises_error_when_reload_fails(self):
+        self.mock_call.side_effect = [None, subprocess.CalledProcessError(None, None, None)]
+        self.assertRaises(errors.PluginError, self.postfix.restart)
+        self.mock_call.assert_called_with(['reload'])
+
+    def test_restart_raises_error_when_start_fails(self):
+        self.mock_call.side_effect = [
+             subprocess.CalledProcessError(None, None, None),
+             subprocess.CalledProcessError(None, None, None)]
+        self.assertRaises(errors.PluginError, self.postfix.restart)
+        self.mock_call.assert_called_with(['start'])
 
 class CheckAllOutputTest(unittest.TestCase):
     """Tests for certbot_postfix.util.check_all_output."""
@@ -96,11 +144,39 @@ class VerifyExeExistsTest(unittest.TestCase):
         mock_path_surgery.return_value = True
         self._call('foo')
 
-class WriteDomainwiseTlsPoliciesTest(unittest.TestCase):
-    pass
+class TestUtils(unittest.TestCase):
+    """ Testing random utility functions in util.py
+    """
+    def test_report_master_overrides(self):
+        from certbot_postfix.util import report_master_overrides
+        self.assertRaises(errors.PluginError, report_master_overrides, 'name',
+                          [('service/type', 'value')])
+        # Shouldn't raise error
+        report_master_overrides('name', [('service/type', 'value')],
+                                acceptable_overrides='value')
+        report_master_overrides('name', [('service/type', 'value')],
+                                acceptable_overrides=('value', 'value1'))
 
-class ReportMasterOverridesTest(unittest.TestCase):
-    pass
+    def test_is_acceptable_value(self):
+        from certbot_postfix.util import is_acceptable_value
+        self.assertTrue(is_acceptable_value('name', 'value', 'value'))
+        self.assertFalse(is_acceptable_value('name', 'bad', 'value'))
 
-if __name__ == '__main__':  # pragma: no cover
+    def test_is_acceptable_tuples(self):
+        from certbot_postfix.util import is_acceptable_value
+        self.assertTrue(is_acceptable_value('name', 'value', ('value', 'value1')))
+        self.assertFalse(is_acceptable_value('name', 'bad', ('value', 'value1')))
+
+    def test_is_acceptable_protocols(self):
+        from certbot_postfix.util import is_acceptable_value
+        self.assertFalse(is_acceptable_value('something_protocols_lol',
+            'SSLv2, SSLv3', ''))
+        self.assertFalse(is_acceptable_value('something_protocols_lol',
+            '!SSLv2, !TLSv1', ''))
+        self.assertTrue(is_acceptable_value('something_protocols_lol',
+            '!SSLv2, !SSLv3', ''))
+        self.assertTrue(is_acceptable_value('something_protocols_lol',
+            '!SSLv3, !TLSv1, !SSLv2', ''))
+
+if __name__ == '__main__': # pragma: no cover
     unittest.main()

certbot-postfix/certbot_postfix/util.py

@@ -219,10 +219,10 @@ def report_master_overrides(name, overrides, acceptable_overrides=None):
         if acceptable_overrides is not None and \
             is_acceptable_value(name, value, acceptable_overrides):
             continue
-        error_string += "  {1}: {2}\n".format(service, value)
+        error_string += "  {0}: {1}\n".format(service, value)
     if len(error_string) > 0:
         raise errors.PluginError("{0} is overridden with less secure options by the "
-             "following services in master.cf:\n" + error_string)
+             "following services in master.cf:\n".format(name) + error_string)
 
 def is_acceptable_value(parameter, value, acceptable):
     """ Returns whether the `value` for this `parameter` is acceptable,
@@ -231,8 +231,7 @@ def is_acceptable_value(parameter, value, acceptable):
     # If it's a tuple, there's multiple acceptable options.
     # Only set a param if it's not acceptable.
     if isinstance(acceptable, tuple):
-        if value not in acceptable:
-            return False
+        return value in acceptable
     # Check if param value is a comma-separated list of protocols.
     elif 'protocols' in parameter:
         return _has_acceptable_tls_versions(value)

tox.cover.sh

@@ -44,7 +44,7 @@ cover () {
   elif [ "$1" = "certbot_nginx" ]; then
     min=97
   elif [ "$1" = "certbot_postfix" ]; then
-    min=100
+    min=99
   elif [ "$1" = "letshelp_certbot" ]; then
     min=100
   else