From 5371be2b398f673b0a48800a45be0d1d4155ddbe Mon Sep 17 00:00:00 2001 From: Unrud Date: Wed, 19 Feb 2020 09:50:27 +0100 Subject: [PATCH] Mark internal configuration options and sections with underscore --- radicale/__main__.py | 4 +- radicale/app/__init__.py | 2 +- radicale/config.py | 62 +++++++++----------- radicale/server.py | 4 +- radicale/storage/multifilesystem/__init__.py | 4 +- radicale/tests/helpers.py | 14 ++--- radicale/tests/test_auth.py | 4 +- radicale/tests/test_base.py | 8 +-- radicale/tests/test_config.py | 8 +-- radicale/tests/test_rights.py | 3 +- radicale/tests/test_server.py | 5 +- radicale/tests/test_web.py | 3 +- 12 files changed, 58 insertions(+), 63 deletions(-) diff --git a/radicale/__main__.py b/radicale/__main__.py index 41f567c3..9e0c3e47 100644 --- a/radicale/__main__.py +++ b/radicale/__main__.py @@ -51,7 +51,7 @@ def run(): groups = {} for section, values in config.DEFAULT_CONFIG_SCHEMA.items(): - if values.get("_internal", False): + if section.startswith("_"): continue group = parser.add_argument_group(section) groups[group] = [] @@ -65,7 +65,7 @@ def run(): kwargs["dest"] = "%s_%s" % (section, option) groups[group].append(kwargs["dest"]) del kwargs["value"] - if "internal" in kwargs: + with contextlib.suppress(KeyError): del kwargs["internal"] if kwargs["type"] == bool: diff --git a/radicale/app/__init__.py b/radicale/app/__init__.py index e6bc51e0..f8357c88 100644 --- a/radicale/app/__init__.py +++ b/radicale/app/__init__.py @@ -281,7 +281,7 @@ class Application( logger.warning("Access to principal path %r denied by " "rights backend", principal_path) - if self.configuration.get("internal", "internal_server"): + if self.configuration.get("_internal", "internal_server"): # Verify content length content_length = int(environ.get("CONTENT_LENGTH") or 0) if content_length: diff --git a/radicale/config.py b/radicale/config.py index f1a4bcea..63e55f59 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -87,6 +87,7 @@ def _convert_to_bool(value): return RawConfigParser.BOOLEAN_STATES[value.lower()] +INTERNAL_OPTIONS = ("_allow_extra",) # Default configuration DEFAULT_CONFIG_SCHEMA = OrderedDict([ ("server", OrderedDict([ @@ -204,8 +205,7 @@ DEFAULT_CONFIG_SCHEMA = OrderedDict([ "type": bool})])), ("headers", OrderedDict([ ("_allow_extra", True)])), - ("internal", OrderedDict([ - ("_internal", True), + ("_internal", OrderedDict([ ("filesystem_fsync", { "value": "True", "help": "sync all changes to filesystem during requests", @@ -292,16 +292,13 @@ class Configuration: self._schema = schema self._values = {} self._configs = [] - values = {} - for section in schema: - values[section] = {} - for option in schema[section]: - if option.startswith("_"): - continue - values[section][option] = schema[section][option]["value"] - self.update(values, "default config", internal=True) + default = {section: {option: self._schema[section][option]["value"] + for option in self._schema[section] + if option not in INTERNAL_OPTIONS} + for section in self._schema} + self.update(default, "default config", privileged=True) - def update(self, config, source=None, internal=False): + def update(self, config, source=None, privileged=False): """Update the configuration. ``config`` a dict of the format {SECTION: {OPTION: VALUE, ...}, ...}. @@ -312,34 +309,33 @@ class Configuration: ``source`` a description of the configuration source (used in error messages). - ``internal`` allows updating "_internal" sections. + ``privileged`` allows updating sections and options starting with "_". """ source = source or "unspecified config" new_values = {} for section in config: - if (section not in self._schema or not internal and - self._schema[section].get("_internal", False)): - raise RuntimeError( + if (section not in self._schema or + section.startswith("_") and not privileged): + raise ValueError( "Invalid section %r in %s" % (section, source)) new_values[section] = {} - if "_allow_extra" in self._schema[section]: - allow_extra_options = self._schema[section]["_allow_extra"] - elif "type" in self._schema[section]: + extra_type = None + if self._schema[section].get("_allow_extra"): + extra_type = str + if "type" in self._schema[section]: if "type" in config[section]: - plugin_type = config[section]["type"] + plugin = config[section]["type"] else: - plugin_type = self.get(section, "type") - allow_extra_options = plugin_type not in self._schema[section][ - "type"].get("internal", []) - else: - allow_extra_options = False + plugin = self.get(section, "type") + if plugin not in self._schema[section]["type"]["internal"]: + extra_type = str for option in config[section]: + type_ = extra_type if option in self._schema[section]: type_ = self._schema[section][option]["type"] - elif allow_extra_options: - type_ = str - else: + if (not type_ or option in INTERNAL_OPTIONS or + option.startswith("_") and not privileged): raise RuntimeError("Invalid option %r in section %r in " "%s" % (option, section, source)) raw_value = config[section][option] @@ -352,12 +348,10 @@ class Configuration: "Invalid %s value for option %r in section %r in %s: " "%r" % (type_.__name__, option, section, source, raw_value)) from e - self._configs.append((config, source, bool(internal))) + self._configs.append((config, source, bool(privileged))) for section in new_values: - if section not in self._values: - self._values[section] = {} - for option in new_values[section]: - self._values[section][option] = new_values[section][option] + self._values[section] = self._values.get(section, {}) + self._values[section].update(new_values[section]) def get(self, section, option): """Get the value of ``option`` in ``section``.""" @@ -417,6 +411,6 @@ class Configuration: section, option)) schema[section][option] = value copy = type(self)(schema) - for config, source, internal in self._configs: - copy.update(config, source, internal) + for config, source, privileged in self._configs: + copy.update(config, source, privileged) return copy diff --git a/radicale/server.py b/radicale/server.py index c2cccfa0..4adbe0c0 100644 --- a/radicale/server.py +++ b/radicale/server.py @@ -200,8 +200,8 @@ def serve(configuration, shutdown_socket): logger.info("Starting Radicale") # Copy configuration before modifying configuration = configuration.copy() - configuration.update({"internal": {"internal_server": "True"}}, "server", - internal=True) + configuration.update({"_internal": {"internal_server": "True"}}, "server", + privileged=True) use_ssl = configuration.get("server", "ssl") server_class = ParallelHTTPSServer if use_ssl else ParallelHTTPServer diff --git a/radicale/storage/multifilesystem/__init__.py b/radicale/storage/multifilesystem/__init__.py index 81817f43..ce3169e8 100644 --- a/radicale/storage/multifilesystem/__init__.py +++ b/radicale/storage/multifilesystem/__init__.py @@ -125,7 +125,7 @@ class Storage( return os.path.join(filesystem_folder, "collection-root") def _fsync(self, fd): - if self.configuration.get("internal", "filesystem_fsync"): + if self.configuration.get("_internal", "filesystem_fsync"): pathutils.fsync(fd) def _sync_directory(self, path): @@ -134,7 +134,7 @@ class Storage( This only works on POSIX and does nothing on other systems. """ - if not self.configuration.get("internal", "filesystem_fsync"): + if not self.configuration.get("_internal", "filesystem_fsync"): return if os.name == "posix": try: diff --git a/radicale/tests/helpers.py b/radicale/tests/helpers.py index db3f9ac1..97ae6a70 100644 --- a/radicale/tests/helpers.py +++ b/radicale/tests/helpers.py @@ -42,11 +42,9 @@ def get_file_content(file_name): def configuration_to_dict(configuration): - d = {} - for section in configuration.sections(): - if configuration._schema[section].get("_internal", False): - continue - d[section] = {} - for option in configuration.options(section): - d[section][option] = configuration.get_raw(section, option) - return d + """Convert configuration to a dict with raw values.""" + return {section: {option: configuration.get_raw(section, option) + for option in configuration.options(section) + if not option.startswith("_")} + for section in configuration.sections() + if not section.startswith("_")} diff --git a/radicale/tests/test_auth.py b/radicale/tests/test_auth.py index 7faad0db..c5b22dde 100644 --- a/radicale/tests/test_auth.py +++ b/radicale/tests/test_auth.py @@ -44,9 +44,9 @@ class TestBaseAuthRequests(BaseTest): self.configuration.update({ "storage": {"filesystem_folder": self.colpath}, # Disable syncing to disk for better performance - "internal": {"filesystem_fsync": "False"}, + "_internal": {"filesystem_fsync": "False"}, # Set incorrect authentication delay to a very low value - "auth": {"delay": "0.002"}}, "test", internal=True) + "auth": {"delay": "0.002"}}, "test", privileged=True) def teardown(self): shutil.rmtree(self.colpath) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index b460f6aa..d9bd04d1 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -1350,9 +1350,9 @@ permissions: RrWw""") "storage": {"type": self.storage_type, "filesystem_folder": self.colpath}, # Disable syncing to disk for better performance - "internal": {"filesystem_fsync": "False"}, + "_internal": {"filesystem_fsync": "False"}, "rights": {"file": rights_file_path, - "type": "from_file"}}, "test", internal=True) + "type": "from_file"}}, "test", privileged=True) self.application = Application(self.configuration) def teardown(self): @@ -1373,8 +1373,8 @@ class TestMultiFileSystem(BaseFileSystemTest, BaseRequestsMixIn): def test_fsync(self): """Create a directory and file with syncing enabled.""" - self.configuration.update({ - "internal": {"filesystem_fsync": "True"}}, "test", internal=True) + self.configuration.update({"_internal": {"filesystem_fsync": "True"}}, + "test", privileged=True) self.application = Application(self.configuration) self.mkcalendar("/calendar.ics/") diff --git a/radicale/tests/test_config.py b/radicale/tests/test_config.py index a1f0b918..54965c28 100644 --- a/radicale/tests/test_config.py +++ b/radicale/tests/test_config.py @@ -133,13 +133,13 @@ class TestConfig: def test_internal(self): configuration = config.load() - configuration.update({"internal": {"internal_server": "True"}}, "test", - internal=True) + configuration.update({"_internal": {"internal_server": "True"}}, + "test", privileged=True) with pytest.raises(Exception) as exc_info: configuration.update( - {"internal": {"internal_server": "True"}}, "test") + {"_internal": {"internal_server": "True"}}, "test") e = exc_info.value - assert "Invalid section 'internal'" in str(e) + assert "Invalid section '_internal'" in str(e) def test_plugin_schema(self): plugin_schema = {"auth": {"new_option": {"value": "False", diff --git a/radicale/tests/test_rights.py b/radicale/tests/test_rights.py index 7d070399..f0a94cc7 100644 --- a/radicale/tests/test_rights.py +++ b/radicale/tests/test_rights.py @@ -37,7 +37,8 @@ class TestBaseRightsRequests(BaseTest): self.configuration.update({ "storage": {"filesystem_folder": self.colpath}, # Disable syncing to disk for better performance - "internal": {"filesystem_fsync": "False"}}, "test", internal=True) + "_internal": {"filesystem_fsync": "False"}}, + "test", privileged=True) def teardown(self): shutil.rmtree(self.colpath) diff --git a/radicale/tests/test_server.py b/radicale/tests/test_server.py index f9b2c61d..1f918695 100644 --- a/radicale/tests/test_server.py +++ b/radicale/tests/test_server.py @@ -68,7 +68,8 @@ class TestBaseServerRequests(BaseTest): # Enable debugging for new processes "logging": {"level": "debug"}, # Disable syncing to disk for better performance - "internal": {"filesystem_fsync": "False"}}, "test", internal=True) + "_internal": {"filesystem_fsync": "False"}}, + "test", privileged=True) self.thread = threading.Thread(target=server.serve, args=( self.configuration, shutdown_socket_out)) ssl_context = ssl.create_default_context() @@ -147,7 +148,7 @@ class TestBaseServerRequests(BaseTest): def test_command_line_interface(self): config_args = [] for section, values in config.DEFAULT_CONFIG_SCHEMA.items(): - if values.get("_internal", False): + if section.startswith("_"): continue for option, data in values.items(): if option.startswith("_"): diff --git a/radicale/tests/test_web.py b/radicale/tests/test_web.py index a19d3876..a5a56a48 100644 --- a/radicale/tests/test_web.py +++ b/radicale/tests/test_web.py @@ -35,7 +35,8 @@ class TestBaseWebRequests(BaseTest): self.configuration.update({ "storage": {"filesystem_folder": self.colpath}, # Disable syncing to disk for better performance - "internal": {"filesystem_fsync": "False"}}, "test", internal=True) + "_internal": {"filesystem_fsync": "False"}}, + "test", privileged=True) self.application = Application(self.configuration) def teardown(self):