mirror of
https://github.com/certbot/certbot.git
synced 2026-06-08 16:22:18 -04:00
Address review comments
This commit is contained in:
Joona Hoikkala
parent
e6649c0e25
commit
5ad44d6126
4 changed files with 157 additions and 184 deletions
|
|
@ -101,7 +101,6 @@ class StandaloneBindError(Error):
|
|||
self.port = port
|
||||
|
||||
|
||||
|
||||
class ConfigurationError(Error):
|
||||
"""Configuration sanity error."""
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
120
certbot/plugins/pluginstorage.py
Normal file
120
certbot/plugins/pluginstorage.py
Normal file
|
|
@ -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]
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue