From 5ad44d6126f14ca79cd8843ebe163805513d5327 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Thu, 5 Apr 2018 00:43:44 +0300 Subject: [PATCH] Address review comments --- certbot/errors.py | 1 - certbot/plugins/common.py | 128 +------------------------- certbot/plugins/pluginstorage.py | 120 ++++++++++++++++++++++++ certbot/plugins/pluginstorage_test.py | 92 +++++++----------- 4 files changed, 157 insertions(+), 184 deletions(-) create mode 100644 certbot/plugins/pluginstorage.py diff --git a/certbot/errors.py b/certbot/errors.py index eb1ce7e31..48aebc267 100644 --- a/certbot/errors.py +++ b/certbot/errors.py @@ -101,7 +101,6 @@ class StandaloneBindError(Error): self.port = port - class ConfigurationError(Error): """Configuration sanity error.""" diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index e14643c93..23a6e64a8 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -1,5 +1,4 @@ """Plugin common functions.""" -import json import logging import os import re @@ -19,6 +18,8 @@ from certbot import interfaces from certbot import reverter from certbot import util +from certbot.plugins.pluginstorage import PluginStorage + logger = logging.getLogger(__name__) @@ -102,131 +103,6 @@ class Plugin(object): return getattr(self.config, self.dest(var)) -class PluginStorage(object): - """Class implementing storage functionality for plugins""" - - def __init__(self, config, classkey): - """Initializes PluginStorage object storing required configuration - options. - - :param .configuration.NamespaceConfig config: Configuration object - :param str classkey: class name to use as root key in storage file - - :returns: Plugin storage object - :rtype: dict - - :raises .errors.PluginStorageError: when unable to open or read the file - """ - - self.config = config - self.classkey = classkey - self.initialized = False - self._data = None - self.storagepath = None - - def initialize_storage(self): - """Initializes PluginStorage data and reads current state from the disk - if the storage json exists.""" - - if hasattr(self.config, "config_dir") and os.path.isdir(self.config.config_dir): - self.storagepath = os.path.join(self.config.config_dir, ".pluginstorage.json") - self._data = self.load() - else: - self.storagepath = None - self.initialized = True - - def load(self): - """Reads PluginStorage content from the disk to a dict structure - - :returns: Plugin storage object - :rtype: dict - - :raises .errors.PluginStorageError: when unable to open or read the file - """ - data = dict() - filedata = "" - try: - with open(self.storagepath, 'r') as fh: - filedata = fh.read() - except IOError as e: - errmsg = "Could not read PluginStorage data file: {0} : {1}".format( - self.storagepath, str(e)) - if os.path.isfile(self.storagepath): - # Only error out if file exists, but cannot be read - logger.error(errmsg) - raise errors.PluginStorageError(errmsg) - except TypeError: - # This happens if storagepath is not set, eg. None - raise errors.PluginStorageError("Could not open PluginStorage") - try: - data = json.loads(filedata) - except ValueError: - if len(filedata) == 0: - logger.debug("Plugin storage file %s was empty, no values loaded", - self.storagepath) - else: - errmsg = "PluginStorage file {0} is corrupted.".format( - self.storagepath) - logger.error(errmsg) - raise errors.PluginStorageError(errmsg) - return data - - def save(self): - """Saves PluginStorage content to disk - - :raises .errors.PluginStorageError: when unable to serialize the data - or write it to the filesystem - """ - if not self.initialized or not self.storagepath: - errmsg = "Unable to save, problem with configuration directory" - logger.error(errmsg) - raise errors.PluginStorageError(errmsg) - - try: - serialized = json.dumps(self._data) - except TypeError as e: - errmsg = "Could not serialize PluginStorage data: {0}".format( - str(e)) - logger.error(errmsg) - raise errors.PluginStorageError(errmsg) - try: - with os.fdopen(os.open(self.storagepath, - os.O_WRONLY | os.O_CREAT, 0o600), 'w') as fh: - fh.truncate() - fh.write(serialized) - except IOError as e: - errmsg = "Could not write PluginStorage data to file {0} : {1}".format( - self.storagepath, str(e)) - logger.error(errmsg) - raise errors.PluginStorageError(errmsg) - - def put(self, key, value): - """Put configuration value to PluginStorage - - :param str key: Key to store the value to - :param value: Data to store - """ - if not self.initialized: - self.initialize_storage() - - if not self.classkey in self._data.keys(): - self._data[self.classkey] = dict() - self._data[self.classkey][key] = value - - def fetch(self, key): - """Get configuration value from PluginStorage - - :param str key: Key to get value from the storage - """ - if not self.initialized: - self.initialize_storage() - - try: - return self._data[self.classkey][key] - except KeyError: - return None - - class Installer(Plugin): """An installer base class with reverter and ssl_dhparam methods defined. diff --git a/certbot/plugins/pluginstorage.py b/certbot/plugins/pluginstorage.py new file mode 100644 index 000000000..52490de15 --- /dev/null +++ b/certbot/plugins/pluginstorage.py @@ -0,0 +1,120 @@ +"""Plugin storage class.""" +import json +import logging +import os + +from certbot import errors + +logger = logging.getLogger(__name__) + +class PluginStorage(object): + """Class implementing storage functionality for plugins""" + + def __init__(self, config, classkey): + """Initializes PluginStorage object storing required configuration + options. + + :param .configuration.NamespaceConfig config: Configuration object + :param str classkey: class name to use as root key in storage file + + :returns: Plugin storage object + + """ + + self._config = config + self._classkey = classkey + self._initialized = False + self._data = None + self._storagepath = None + + def _initialize_storage(self): + """Initializes PluginStorage data and reads current state from the disk + if the storage json exists.""" + + if hasattr(self._config, "config_dir") and os.path.isdir(self._config.config_dir): + self._storagepath = os.path.join(self._config.config_dir, ".pluginstorage.json") + self._load() + self._initialized = True + + def _load(self): + """Reads PluginStorage content from the disk to a dict structure + + :raises .errors.PluginStorageError: when unable to open or read the file + """ + data = dict() + filedata = "" + try: + with open(self._storagepath, 'r') as fh: + filedata = fh.read() + except IOError as e: + errmsg = "Could not read PluginStorage data file: {0} : {1}".format( + self._storagepath, str(e)) + if os.path.isfile(self._storagepath): + # Only error out if file exists, but cannot be read + logger.error(errmsg) + raise errors.PluginStorageError(errmsg) + try: + data = json.loads(filedata) + except ValueError: + if not filedata: + logger.debug("Plugin storage file %s was empty, no values loaded", + self._storagepath) + else: + errmsg = "PluginStorage file {0} is corrupted.".format( + self._storagepath) + logger.error(errmsg) + raise errors.PluginStorageError(errmsg) + self._data = data + + def save(self): + """Saves PluginStorage content to disk + + :raises .errors.PluginStorageError: when unable to serialize the data + or write it to the filesystem + """ + if not self._initialized: + errmsg = "Unable to save, the PluginStorage was never initialized" + logger.error(errmsg) + raise errors.PluginStorageError(errmsg) + + try: + serialized = json.dumps(self._data) + except TypeError as e: + errmsg = "Could not serialize PluginStorage data: {0}".format( + str(e)) + logger.error(errmsg) + raise errors.PluginStorageError(errmsg) + try: + with os.fdopen(os.open(self._storagepath, + os.O_WRONLY | os.O_CREAT, 0o600), 'w') as fh: + fh.write(serialized) + except IOError as e: + errmsg = "Could not write PluginStorage data to file {0} : {1}".format( + self._storagepath, str(e)) + logger.error(errmsg) + raise errors.PluginStorageError(errmsg) + + def put(self, key, value): + """Put configuration value to PluginStorage + + :param str key: Key to store the value to + :param value: Data to store + """ + if not self._initialized: + self._initialize_storage() + + if not self._classkey in self._data.keys(): + self._data[self._classkey] = dict() + self._data[self._classkey][key] = value + + def fetch(self, key): + """Get configuration value from PluginStorage + + :param str key: Key to get value from the storage + + :raises KeyError: If the key doesn't exist in the storage + """ + if not self._initialized: + self._initialize_storage() + + return self._data[self._classkey][key] diff --git a/certbot/plugins/pluginstorage_test.py b/certbot/plugins/pluginstorage_test.py index 176f355bf..e3e0ebcdb 100644 --- a/certbot/plugins/pluginstorage_test.py +++ b/certbot/plugins/pluginstorage_test.py @@ -2,75 +2,56 @@ import json import mock import os -import shutil import tempfile import unittest from certbot import errors from certbot.plugins import common +from certbot.tests import util as test_util - -class PluginStorageTest(unittest.TestCase): +class PluginStorageTest(test_util.ConfigTestCase): """Test for certbot.plugins.common.PluginStorage""" def setUp(self): - - class MockPlugin(common.Installer): - """Mock plugin""" - pass - self.plugin_cls = MockPlugin - self.config_dir = tempfile.mkdtemp() - self.config = mock.MagicMock(config_dir=self.config_dir) + super(PluginStorageTest, self).setUp() + self.plugin_cls = common.Installer + self.config.config_dir = tempfile.mkdtemp() with mock.patch("certbot.reverter.util"): - self.plugin = MockPlugin(config=self.config, name="mockplugin") + self.plugin = self.plugin_cls(config=self.config, name="mockplugin") - def tearDown(self): - shutil.rmtree(self.config_dir) - - def test_initialize_no_configdir(self): - delattr(self.plugin.config, "config_dir") - self.plugin.storage.initialize_storage() - self.assertTrue(self.plugin.storage.storagepath == None) - - def test_load_errors(self): - with open(os.path.join(self.config_dir, ".pluginstorage.json"), "w") as fh: + def test_load_errors_cant_read(self): + with open(os.path.join(self.config.config_dir, + ".pluginstorage.json"), "w") as fh: fh.write("dummy") - # When unable to read file that exists mock_open = mock.mock_open() mock_open.side_effect = IOError - self.plugin.storage.storagepath = os.path.join(self.config_dir, + self.plugin.storage.storagepath = os.path.join(self.config.config_dir, ".pluginstorage.json") with mock.patch("six.moves.builtins.open", mock_open): with mock.patch('os.path.isfile', return_value=True): with mock.patch("certbot.reverter.util"): self.assertRaises(errors.PluginStorageError, - self.plugin.storage.load) + self.plugin.storage._load) # pylint: disable=protected-access - # When pluginstorage path is None - mock_open.side_effect = TypeError - with mock.patch("six.moves.builtins.open", mock_open): - with mock.patch("certbot.reverter.util"): - nostoragepath = self.plugin_cls(self.config, "mockplugin") - self.assertRaises(errors.PluginStorageError, - nostoragepath.storage.load) - - # When file exists but is completely empty - with open(os.path.join(self.config_dir, ".pluginstorage.json"), "w") as fh: + def test_load_errors_empty(self): + with open(os.path.join(self.config.config_dir, ".pluginstorage.json"), "w") as fh: fh.write('') - with mock.patch("certbot.plugins.common.logger.debug") as mock_log: + with mock.patch("certbot.plugins.pluginstorage.logger.debug") as mock_log: # Should not error out but write a debug log line instead with mock.patch("certbot.reverter.util"): nocontent = self.plugin_cls(self.config, "mockplugin") - nocontent.storage.fetch("value") + self.assertRaises(KeyError, + nocontent.storage.fetch, "value") self.assertTrue(mock_log.called) self.assertTrue("no values loaded" in mock_log.call_args[0][0]) - # File is corrupted - with open(os.path.join(self.config_dir, ".pluginstorage.json"), "w") as fh: + def test_load_errors_corrupted(self): + with open(os.path.join(self.config.config_dir, + ".pluginstorage.json"), "w") as fh: fh.write('invalid json') - with mock.patch("certbot.plugins.common.logger.error") as mock_log: + with mock.patch("certbot.plugins.pluginstorage.logger.error") as mock_log: with mock.patch("certbot.reverter.util"): corrupted = self.plugin_cls(self.config, "mockplugin") self.assertRaises(errors.PluginError, @@ -78,39 +59,41 @@ class PluginStorageTest(unittest.TestCase): "value") self.assertTrue("is corrupted" in mock_log.call_args[0][0]) - def test_save_no_storagepath(self): - with mock.patch("certbot.plugins.common.logger.error") as mock_log: - self.assertRaises(errors.PluginStorageError, - self.plugin.storage.save) - self.assertTrue("Unable to save" in mock_log.call_args[0][0]) - - def test_save_errors(self): - with mock.patch("certbot.plugins.common.logger.error") as mock_log: + def test_save_errors_cant_serialize(self): + with mock.patch("certbot.plugins.pluginstorage.logger.error") as mock_log: # Set data as something that can't be serialized - self.plugin.storage.initialized = True + self.plugin.storage._initialized = True # pylint: disable=protected-access self.plugin.storage.storagepath = "/tmp/whatever" self.plugin.storage._data = self.plugin_cls # pylint: disable=protected-access self.assertRaises(errors.PluginStorageError, self.plugin.storage.save) self.assertTrue("Could not serialize" in mock_log.call_args[0][0]) - # When unable to write to file + def test_save_errors_unable_to_write_file(self): mock_open = mock.mock_open() mock_open.side_effect = IOError with mock.patch("os.open", mock_open): - with mock.patch("certbot.plugins.common.logger.error") as mock_log: + with mock.patch("certbot.plugins.pluginstorage.logger.error") as mock_log: self.plugin.storage._data = {"valid": "data"} # pylint: disable=protected-access + self.plugin.storage._initialized = True # pylint: disable=protected-access self.assertRaises(errors.PluginStorageError, self.plugin.storage.save) self.assertTrue("Could not write" in mock_log.call_args[0][0]) + def test_save_uninitialized(self): + with mock.patch("certbot.reverter.util"): + self.assertRaises(errors.PluginStorageError, + self.plugin_cls(self.config, "x").storage.save) + def test_namespace_isolation(self): with mock.patch("certbot.reverter.util"): plugin1 = self.plugin_cls(self.config, "first") plugin2 = self.plugin_cls(self.config, "second") plugin1.storage.put("first_key", "first_value") - self.assertEqual(plugin2.storage.fetch("first_key"), None) - self.assertEqual(plugin2.storage.fetch("first"), None) + self.assertRaises(KeyError, + plugin2.storage.fetch, "first_key") + self.assertRaises(KeyError, + plugin2.storage.fetch, "first") self.assertEqual(plugin1.storage.fetch("first_key"), "first_value") @@ -131,10 +114,5 @@ class PluginStorageTest(unittest.TestCase): self.assertEqual(psjson["mockplugin"]["testkey"], "testvalue") - - - - - if __name__ == "__main__": unittest.main() # pragma: no cover