From 95a8c7b903f01271c80e5d40668f0c3b1ee328b4 Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:41:47 +0200 Subject: [PATCH 1/6] use mapping api to set config options Provides protection against typos in names --- radicale/tests/test_auth.py | 21 +++++++++---------- radicale/tests/test_base.py | 39 ++++++++++++++++------------------- radicale/tests/test_rights.py | 16 +++++++------- 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/radicale/tests/test_auth.py b/radicale/tests/test_auth.py index 422f98c..9b136b3 100644 --- a/radicale/tests/test_auth.py +++ b/radicale/tests/test_auth.py @@ -40,13 +40,13 @@ class TestBaseAuthRequests(BaseTest): def setup(self): self.configuration = config.load() self.colpath = tempfile.mkdtemp() - self.configuration.set("storage", "filesystem_folder", self.colpath) + self.configuration["storage"]["filesystem_folder"] = self.colpath # Disable syncing to disk for better performance - self.configuration.set("storage", "filesystem_fsync", "False") + self.configuration["storage"]["filesystem_fsync"] = "False" # Required on Windows, doesn't matter on Unix - self.configuration.set("storage", "filesystem_close_lock_file", "True") + self.configuration["storage"]["filesystem_close_lock_file"] = "True" # Set incorrect authentication delay to a very low value - self.configuration.set("auth", "delay", "0.002") + self.configuration["auth"]["delay"] = "0.002" def teardown(self): shutil.rmtree(self.colpath) @@ -56,10 +56,9 @@ class TestBaseAuthRequests(BaseTest): htpasswd_file_path = os.path.join(self.colpath, ".htpasswd") with open(htpasswd_file_path, "w") as f: f.write(htpasswd_content) - self.configuration.set("auth", "type", "htpasswd") - self.configuration.set("auth", "htpasswd_filename", htpasswd_file_path) - self.configuration.set("auth", "htpasswd_encryption", - htpasswd_encryption) + self.configuration["auth"]["type"] = "htpasswd" + self.configuration["auth"]["htpasswd_filename"] = htpasswd_file_path + self.configuration["auth"]["htpasswd_encryption"] = htpasswd_encryption self.application = Application(self.configuration, self.logger) for user, password, expected_status in ( ("tmp", "bepo", 207), ("tmp", "tmp", 401), ("tmp", "", 401), @@ -108,7 +107,7 @@ class TestBaseAuthRequests(BaseTest): "tmp:$2y$05$oD7hbiQFQlvCM7zoalo/T.MssV3VNTRI3w5KDnj8NTUKJNWfVpvRq") def test_remote_user(self): - self.configuration.set("auth", "type", "remote_user") + self.configuration["auth"]["type"] = "remote_user" self.application = Application(self.configuration, self.logger) status, _, answer = self.request( "PROPFIND", "/", @@ -122,7 +121,7 @@ class TestBaseAuthRequests(BaseTest): assert ">/test/<" in answer def test_http_x_remote_user(self): - self.configuration.set("auth", "type", "http_x_remote_user") + self.configuration["auth"]["type"] = "http_x_remote_user" self.application = Application(self.configuration, self.logger) status, _, answer = self.request( "PROPFIND", "/", @@ -137,7 +136,7 @@ class TestBaseAuthRequests(BaseTest): def test_custom(self): """Custom authentication.""" - self.configuration.set("auth", "type", "tests.custom.auth") + self.configuration["auth"]["type"] = "tests.custom.auth" self.application = Application(self.configuration, self.logger) status, headers, answer = self.request( "PROPFIND", "/tmp", HTTP_AUTHORIZATION="Basic %s" % diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 9ace4bf..773dc98 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -779,10 +779,10 @@ class BaseRequestsMixIn: def test_authentication(self): """Test if server sends authentication request.""" - self.configuration.set("auth", "type", "htpasswd") - self.configuration.set("auth", "htpasswd_filename", os.devnull) - self.configuration.set("auth", "htpasswd_encryption", "plain") - self.configuration.set("rights", "type", "owner_only") + self.configuration["auth"]["type"] = "htpasswd" + self.configuration["auth"]["htpasswd_filename"] = os.devnull + self.configuration["auth"]["htpasswd_encryption"] = "plain" + self.configuration["rights"]["type"] = "owner_only" self.application = Application(self.configuration, self.logger) status, headers, answer = self.request("MKCOL", "/user/") assert status in (401, 403) @@ -806,15 +806,14 @@ class BaseRequestsMixIn: def test_fsync(self): """Create a directory and file with syncing enabled.""" - self.configuration.set("storage", "filesystem_fsync", "True") + self.configuration["storage"]["filesystem_fsync"] = "True" status, headers, answer = self.request("MKCALENDAR", "/calendar.ics/") assert status == 201 def test_hook(self): """Run hook.""" - self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join( - "collection-root", "created_by_hook")) + self.configuration["storage"]["hook"] = ( + "mkdir %s" % os.path.join("collection-root", "created_by_hook")) status, headers, answer = self.request("MKCOL", "/calendar.ics/") assert status == 201 status, headers, answer = self.request("PROPFIND", "/created_by_hook/") @@ -822,9 +821,8 @@ class BaseRequestsMixIn: def test_hook_read_access(self): """Verify that hook is not run for read accesses.""" - self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join( - "collection-root", "created_by_hook")) + self.configuration["storage"]["hook"] = ( + "mkdir %s" % os.path.join("collection-root", "created_by_hook")) status, headers, answer = self.request("GET", "/") assert status == 303 status, headers, answer = self.request("GET", "/created_by_hook/") @@ -834,16 +832,15 @@ class BaseRequestsMixIn: reason="flock command not found") def test_hook_storage_locked(self): """Verify that the storage is locked when the hook runs.""" - self.configuration.set( - "storage", "hook", "flock -n .Radicale.lock || exit 0; exit 1") + self.configuration["storage"]["hook"] = ( + "flock -n .Radicale.lock || exit 0; exit 1") status, headers, answer = self.request("MKCOL", "/calendar.ics/") assert status == 201 def test_hook_principal_collection_creation(self): """Verify that the hooks runs when a new user is created.""" - self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join( - "collection-root", "created_by_hook")) + self.configuration["storage"]["hook"] = ( + "mkdir %s" % os.path.join("collection-root", "created_by_hook")) status, headers, answer = self.request("GET", "/", REMOTE_USER="user") assert status == 303 status, headers, answer = self.request("PROPFIND", "/created_by_hook/") @@ -851,7 +848,7 @@ class BaseRequestsMixIn: def test_hook_fail(self): """Verify that a request fails if the hook fails.""" - self.configuration.set("storage", "hook", "exit 1") + self.configuration["storage"]["hook"] = "exit 1" try: status, headers, answer = self.request("MKCOL", "/calendar.ics/") assert status != 201 @@ -877,13 +874,13 @@ class BaseFileSystemTest(BaseTest): def setup(self): self.configuration = config.load() - self.configuration.set("storage", "type", self.storage_type) + self.configuration["storage"]["type"] = self.storage_type self.colpath = tempfile.mkdtemp() - self.configuration.set("storage", "filesystem_folder", self.colpath) + self.configuration["storage"]["filesystem_folder"] = self.colpath # Disable syncing to disk for better performance - self.configuration.set("storage", "filesystem_fsync", "False") + self.configuration["storage"]["filesystem_fsync"] = "False" # Required on Windows, doesn't matter on Unix - self.configuration.set("storage", "filesystem_close_lock_file", "True") + self.configuration["storage"]["filesystem_close_lock_file"] = "True" self.application = Application(self.configuration, self.logger) def teardown(self): diff --git a/radicale/tests/test_rights.py b/radicale/tests/test_rights.py index 0bdaf57..36c2833 100644 --- a/radicale/tests/test_rights.py +++ b/radicale/tests/test_rights.py @@ -34,11 +34,11 @@ class TestBaseAuthRequests(BaseTest): def setup(self): self.configuration = config.load() self.colpath = tempfile.mkdtemp() - self.configuration.set("storage", "filesystem_folder", self.colpath) + self.configuration["storage"]["filesystem_folder"] = self.colpath # Disable syncing to disk for better performance - self.configuration.set("storage", "filesystem_fsync", "False") + self.configuration["storage"]["filesystem_fsync"] = "False" # Required on Windows, doesn't matter on Unix - self.configuration.set("storage", "filesystem_close_lock_file", "True") + self.configuration["storage"]["filesystem_close_lock_file"] = "True" def teardown(self): shutil.rmtree(self.colpath) @@ -49,10 +49,10 @@ class TestBaseAuthRequests(BaseTest): htpasswd_file_path = os.path.join(self.colpath, ".htpasswd") with open(htpasswd_file_path, "w") as f: f.write("tmp:bepo\nother:bepo") - self.configuration.set("rights", "type", rights_type) - self.configuration.set("auth", "type", "htpasswd") - self.configuration.set("auth", "htpasswd_filename", htpasswd_file_path) - self.configuration.set("auth", "htpasswd_encryption", "plain") + self.configuration["rights"]["type"] = rights_type + self.configuration["auth"]["type"] = "htpasswd" + self.configuration["auth"]["htpasswd_filename"] = htpasswd_file_path + self.configuration["auth"]["htpasswd_encryption"] = "plain" self.application = Application(self.configuration, self.logger) for u in ("tmp", "other"): status, _, _ = self.request( @@ -125,7 +125,7 @@ permission: rw user: .* collection: custom(/.*)? permission: r""") - self.configuration.set("rights", "file", rights_file_path) + self.configuration["rights"]["file"] = rights_file_path self._test_rights("from_file", "", "/other", "r", 401) self._test_rights("from_file", "tmp", "/other", "r", 403) self._test_rights("from_file", "", "/custom/sub", "r", 404) From 881757815f9b7722080825b3cabd51de37252853 Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:42:19 +0200 Subject: [PATCH 2/6] Add simple range checking to config options --- radicale/config.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/radicale/config.py b/radicale/config.py index 3db0e5b..593c457 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -23,10 +23,30 @@ Give a configparser-like interface to read and write configuration. """ +import math import os from collections import OrderedDict from configparser import RawConfigParser as ConfigParser + +def positive_int(value): + value = int(value) + if value < 0: + raise ValueError("value is negative: %d" % value) + return value + + +def positive_float(value): + value = float(value) + if not math.isfinite(value): + raise ValueError("value is infinite") + if math.isnan(value): + raise ValueError("value is not a number") + if value < 0: + raise ValueError("value is negative: %f" % value) + return value + + # Default configuration INITIAL_CONFIG = OrderedDict([ ("server", OrderedDict([ @@ -49,15 +69,15 @@ INITIAL_CONFIG = OrderedDict([ ("max_connections", { "value": "20", "help": "maximum number of parallel connections", - "type": int}), + "type": positive_int}), ("max_content_length", { "value": "10000000", "help": "maximum size of request body in bytes", - "type": int}), + "type": positive_int}), ("timeout", { "value": "10", "help": "socket timeout", - "type": int}), + "type": positive_int}), ("ssl", { "value": "False", "help": "use SSL connection", @@ -115,7 +135,7 @@ INITIAL_CONFIG = OrderedDict([ ("delay", { "value": "1", "help": "incorrect authentication delay", - "type": float})])), + "type": positive_float})])), ("rights", OrderedDict([ ("type", { "value": "owner_only", From 1812aeb2383c1c005dce74ad88502cd1e88c21a8 Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:42:40 +0200 Subject: [PATCH 3/6] include expected type of config option in error --- radicale/config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/radicale/config.py b/radicale/config.py index 593c457..5521190 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -232,6 +232,7 @@ def load(paths=(), extra_config=None, ignore_missing_paths=True): type_(config.get(section, option)) except Exception as e: raise RuntimeError( - "Invalid value %r for option %r in section %r in config" % - (config.get(section, option), option, section)) from e + "Invalid %s value for option %r in section %r in config: " + "%r" % (type_.__name__, option, section, + config.get(section, option))) from e return config From 5d27265d5ce8ad4aa1832c4693454491432322dc Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:43:03 +0200 Subject: [PATCH 4/6] fail when logging config file is not found --- radicale/log.py | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/radicale/log.py b/radicale/log.py index 2052c73..e803a19 100644 --- a/radicale/log.py +++ b/radicale/log.py @@ -24,7 +24,6 @@ http://docs.python.org/library/logging.config.html import logging import logging.config -import os import signal import sys @@ -47,26 +46,30 @@ class RemoveTracebackFilter(logging.Filter): def start(name="radicale", filename=None, debug=False): """Start the logging according to the configuration.""" logger = logging.getLogger(name) - if filename and os.path.exists(filename): - # Configuration taken from file - configure_from_file(logger, filename, debug) - # Reload config on SIGHUP (UNIX only) - if hasattr(signal, "SIGHUP"): - def handler(signum, frame): - configure_from_file(logger, filename, debug) - signal.signal(signal.SIGHUP, handler) - else: - # Default configuration, standard output - if filename: - logger.warning( - "WARNING: Logging configuration file %r not found, using " - "stderr" % filename) - handler = logging.StreamHandler(sys.stderr) - handler.setFormatter( - logging.Formatter("[%(thread)x] %(levelname)s: %(message)s")) - logger.addHandler(handler) if debug: logger.setLevel(logging.DEBUG) else: logger.addFilter(RemoveTracebackFilter()) + if filename: + # Configuration taken from file + try: + configure_from_file(logger, filename, debug) + except Exception as e: + raise RuntimeError("Failed to load logging configuration file %r: " + "%s" % (filename, e)) from e + # Reload config on SIGHUP (UNIX only) + if hasattr(signal, "SIGHUP"): + def handler(signum, frame): + try: + configure_from_file(logger, filename, debug) + except Exception as e: + logger.error("Failed to reload logging configuration file " + "%r: %s", filename, e, exc_info=True) + signal.signal(signal.SIGHUP, handler) + else: + # Default configuration, standard output + handler = logging.StreamHandler(sys.stderr) + handler.setFormatter( + logging.Formatter("[%(thread)x] %(levelname)s: %(message)s")) + logger.addHandler(handler) return logger From 6edaf27a38b7c8ea3cbf412ce18d81300d62d16f Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:43:23 +0200 Subject: [PATCH 5/6] rename backend from "None" to "none" --- radicale/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/radicale/config.py b/radicale/config.py index 5521190..9602068 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -121,7 +121,7 @@ INITIAL_CONFIG = OrderedDict([ "type": str})])), ("auth", OrderedDict([ ("type", { - "value": "None", + "value": "none", "help": "authentication method", "type": str}), ("htpasswd_filename", { From 428abf10de32c97d290fc1181ea29f21a9d63101 Mon Sep 17 00:00:00 2001 From: Unrud Date: Fri, 2 Jun 2017 12:43:44 +0200 Subject: [PATCH 6/6] don't use REMOTE_USER in tests --- radicale/tests/test_base.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 773dc98..60adb76 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -791,7 +791,8 @@ class BaseRequestsMixIn: def test_principal_collection_creation(self): """Verify existence of the principal collection.""" status, headers, answer = self.request( - "PROPFIND", "/user/", REMOTE_USER="user") + "PROPFIND", "/user/", HTTP_AUTHORIZATION=( + "Basic " + base64.b64encode(b"user:").decode())) assert status == 207 def test_existence_of_root_collections(self): @@ -841,7 +842,9 @@ class BaseRequestsMixIn: """Verify that the hooks runs when a new user is created.""" self.configuration["storage"]["hook"] = ( "mkdir %s" % os.path.join("collection-root", "created_by_hook")) - status, headers, answer = self.request("GET", "/", REMOTE_USER="user") + status, headers, answer = self.request( + "GET", "/", HTTP_AUTHORIZATION=( + "Basic " + base64.b64encode(b"user:").decode())) assert status == 303 status, headers, answer = self.request("PROPFIND", "/created_by_hook/") assert status == 207