From 4263f9e2c892c187c96658892417a84809cc9752 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Tue, 23 Jun 2020 17:36:41 -0700 Subject: [PATCH] cfg: Drop the default configuration file - The configuration module defaults to values in the production configuration file. - If the file is found, it is read and the read values overwrite the defaults. If the file is not found, no error is raised. This allows us to not ship the configuration file. User may create the configuration if they want to change the defaults. This eases upgrades when configuration is edited. This also make FreedomBox robust to deployments where /etc/ is not populated by default such as OSTree. It is also a good practice for daemons as followed by the likes of systemd. - If the file partly populated only the values read override the defaults and the remaining values don't change. This allows the user to write simpler configuration file. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- conftest.py | 2 +- data/etc/plinth/plinth.config | 36 ----------- debian/freedombox.maintscript | 1 + plinth/cfg.py | 64 ++++++++++++------- .../data/plinth.config.with_missing_options | 2 +- .../data/plinth.config.with_missing_sections | 2 +- plinth/tests/test_cfg.py | 63 ++++++------------ 7 files changed, 67 insertions(+), 103 deletions(-) delete mode 100644 data/etc/plinth/plinth.config diff --git a/conftest.py b/conftest.py index d892c6120..c250213c8 100644 --- a/conftest.py +++ b/conftest.py @@ -55,7 +55,7 @@ def fixture_load_cfg(): cfg_file = test_data_dir / 'etc' / 'plinth' / 'plinth.config' cfg.read(str(cfg_file), str(root_dir)) yield cfg - cfg.read() + cfg.read(str(cfg_file), str(root_dir)) @pytest.fixture(name='develop_mode') diff --git a/data/etc/plinth/plinth.config b/data/etc/plinth/plinth.config deleted file mode 100644 index f1bd618fe..000000000 --- a/data/etc/plinth/plinth.config +++ /dev/null @@ -1,36 +0,0 @@ -[Path] -# directory locations -file_root = /usr/share/plinth -config_dir = /etc/plinth -data_dir = /var/lib/plinth -server_dir = /plinth -actions_dir = /usr/share/plinth/actions -doc_dir = /usr/share/freedombox -custom_static_dir = /var/www/plinth/custom/static - -# file locations -store_file = %(data_dir)s/plinth.sqlite3 - -[Network] -host = 127.0.0.1 -port = 8000 - -# Enable the following only if Plinth is behind a proxy server. The -# proxy server should properly clean and the following HTTP headers: -# X-Forwarded-For -# X-Forwarded-Host -# X-Forwarded-Proto -# If you enable these unnecessarily, this will lead to serious security -# problems. For more information, see -# https://docs.djangoproject.com/en/1.7/ref/settings/ -# -# These are enabled by default in Plinth because the default -# configuration allows only connections from localhost -# -# Leave the values blank to disable -use_x_forwarded_for = True -use_x_forwarded_host = True -secure_proxy_ssl_header = HTTP_X_FORWARDED_PROTO - -[Misc] -box_name = FreedomBox diff --git a/debian/freedombox.maintscript b/debian/freedombox.maintscript index 9349c7399..2ccd25060 100644 --- a/debian/freedombox.maintscript +++ b/debian/freedombox.maintscript @@ -10,3 +10,4 @@ rm_conffile /etc/plinth/modules-enabled/udiskie 0.39.0~ rm_conffile /etc/plinth/modules-enabled/restore 20.1~ rm_conffile /etc/plinth/modules-enabled/repro 20.1~ rm_conffile /etc/apt/preferences.d/50freedombox3.pref 20.5~ +rm_conffile /etc/plinth/plinth.config 20.12~ diff --git a/plinth/cfg.py b/plinth/cfg.py index c270d2b1b..0494cd810 100644 --- a/plinth/cfg.py +++ b/plinth/cfg.py @@ -1,4 +1,7 @@ # SPDX-License-Identifier: AGPL-3.0-or-later +""" +Configuration parser and default values for configuration options. +""" import configparser import logging @@ -6,28 +9,46 @@ import os logger = logging.getLogger(__name__) -box_name = None +# [Path] section root = None -file_root = None -config_dir = None -data_dir = None -custom_static_dir = None -store_file = None -actions_dir = None -doc_dir = None -host = None -port = None -use_x_forwarded_for = False -use_x_forwarded_host = False -secure_proxy_ssl_header = None +file_root = '/usr/share/plinth' +config_dir = '/etc/plinth' +data_dir = '/var/lib/plinth' +custom_static_dir = '/var/www/plinth/custom/static' +store_file = data_dir + '/plinth.sqlite3' +actions_dir = '/usr/share/plinth/actions' +doc_dir = '/usr/share/freedombox' +server_dir = '/plinth' + +# [Network] section +host = '127.0.0.1' +port = 8000 + +# Enable the following only if Plinth is behind a proxy server. The +# proxy server should properly clean and the following HTTP headers: +# X-Forwarded-For +# X-Forwarded-Host +# X-Forwarded-Proto +# If you enable these unnecessarily, this will lead to serious security +# problems. For more information, see +# https://docs.djangoproject.com/en/1.7/ref/settings/ +# +# These are enabled by default in FreedomBox because the default +# configuration allows only connections from localhost +# +# Leave the values blank to disable +use_x_forwarded_for = True +use_x_forwarded_host = True +secure_proxy_ssl_header = 'HTTP_X_FORWARDED_PROTO' + +# [Misc] section +box_name = 'FreedomBox' + +# Other globals develop = False -server_dir = '/' config_file = None -DEFAULT_CONFIG_FILE = '/etc/plinth/plinth.config' -DEFAULT_ROOT = '/' - def get_fallback_config_paths(): """Get config paths of the current source code folder""" @@ -54,8 +75,8 @@ def read(config_path=None, root_directory=None): config_path, root_directory = get_config_paths() if not os.path.isfile(config_path): - msg = 'No plinth.config file could be found on path: %s' % config_path - raise FileNotFoundError(msg) + # Ignore missing configuration files + return global config_file # pylint: disable-msg=invalid-name,global-statement config_file = config_path @@ -87,9 +108,8 @@ def read(config_path=None, root_directory=None): try: value = parser.get(section, name) except (configparser.NoSectionError, configparser.NoOptionError): - logger.error('Configuration does not contain option: %s.%s', - section, name) - raise + # Use default values for any missing keys in configuration + continue else: if datatype == 'int': value = int(value) diff --git a/plinth/tests/data/plinth.config.with_missing_options b/plinth/tests/data/plinth.config.with_missing_options index 684b415d1..f66c8f4d3 100644 --- a/plinth/tests/data/plinth.config.with_missing_options +++ b/plinth/tests/data/plinth.config.with_missing_options @@ -1,5 +1,5 @@ [Misc] -box_name = FreedomBox +box_name = FreedomBoxTestMissingOptions [Path] diff --git a/plinth/tests/data/plinth.config.with_missing_sections b/plinth/tests/data/plinth.config.with_missing_sections index fbb473246..1d76ef7a8 100644 --- a/plinth/tests/data/plinth.config.with_missing_sections +++ b/plinth/tests/data/plinth.config.with_missing_sections @@ -1,2 +1,2 @@ [Misc] -box_name = FreedomBox +box_name = FreedomBoxTestMissingSections diff --git a/plinth/tests/test_cfg.py b/plinth/tests/test_cfg.py index f33c2c586..0fab9bb89 100644 --- a/plinth/tests/test_cfg.py +++ b/plinth/tests/test_cfg.py @@ -6,6 +6,7 @@ Test module for configuration module. import configparser import logging import os +from unittest.mock import patch import pytest @@ -23,54 +24,33 @@ logging.disable(logging.CRITICAL) pytestmark = pytest.mark.usefixtures('load_cfg') -@pytest.fixture(name='test_config_file') -def fixture_test_config_file(load_cfg): - """Test fixture to return the configuration file path""" - return cfg.get_config_paths()[0] - - -@pytest.fixture(name='test_config_dir') -def fixture_test_config_dir(load_cfg): - """Test fixture to return the configuration file directory.""" - return cfg.get_config_paths()[1] - - -def test_read_default_config_file(test_config_dir, test_config_file): +def test_read_default_config_file(): """Verify that the default config file can be read correctly.""" + config_file, config_dir = cfg.get_fallback_config_paths() + # Read the plinth.config file directly - parser = configparser.ConfigParser(defaults={'root': test_config_dir}) - parser.read(test_config_file) + parser = configparser.ConfigParser(defaults={'root': config_dir}) + parser.read(config_file) # Read the plinth.config file via the cfg module - cfg.read(test_config_file, test_config_dir) + cfg.read(config_file, config_dir) # Compare the two results compare_configurations(parser) -def test_read_primary_config_file(): +@patch('plinth.cfg.get_config_paths') +def test_read_primary_config_file(get_config_paths): """Verify that the primary config file is used by default.""" - original_config_path = cfg.DEFAULT_CONFIG_FILE - original_root_directory = cfg.DEFAULT_ROOT - expected_config_path = CONFIG_FILE_WITH_MISSING_OPTIONS root_directory = 'x-default-root' expected_root_directory = os.path.realpath(root_directory) + get_config_paths.return_value = (expected_config_path, root_directory) - try: - cfg.DEFAULT_CONFIG_FILE = expected_config_path - cfg.DEFAULT_ROOT = root_directory - # reading the config file will fail, but still cfg.root and - # cfg.config_file will be set for parsing the config file - try: - cfg.read() - except configparser.NoOptionError: - pass - assert cfg.config_file == expected_config_path - assert cfg.root == expected_root_directory - finally: - cfg.DEFAULT_CONFIG_FILE = original_config_path - cfg.DEFAULT_ROOT = original_root_directory + cfg.read() + + assert cfg.config_file == expected_config_path + assert cfg.root == expected_root_directory def test_read_fallback_config_file(): @@ -86,20 +66,19 @@ def test_read_fallback_config_file(): def test_read_missing_config_file(): """Verify that an exception is raised when there's no config file.""" - with pytest.raises(FileNotFoundError): - cfg.read('x-non-existant-file', 'x-root-directory') + cfg.read('x-non-existant-file', 'x-root-directory') -def test_read_config_file_with_missing_sections(test_config_dir): +def test_read_config_file_with_missing_sections(): """Verify that missing configuration sections can be detected.""" - with pytest.raises(configparser.NoSectionError): - cfg.read(CONFIG_FILE_WITH_MISSING_SECTIONS, test_config_dir) + cfg.read(CONFIG_FILE_WITH_MISSING_SECTIONS, TEST_CONFIG_DIR) + assert cfg.box_name == 'FreedomBoxTestMissingSections' -def test_read_config_file_with_missing_options(test_config_dir): +def test_read_config_file_with_missing_options(): """Verify that missing configuration options can be detected.""" - with pytest.raises(configparser.NoOptionError): - cfg.read(CONFIG_FILE_WITH_MISSING_OPTIONS, test_config_dir) + cfg.read(CONFIG_FILE_WITH_MISSING_OPTIONS, TEST_CONFIG_DIR) + assert cfg.box_name == 'FreedomBoxTestMissingOptions' def compare_configurations(parser):