Merge pull request #621 from Unrud/small

Small improvements

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",
@@ -101,7 +121,7 @@ INITIAL_CONFIG = OrderedDict([
             "type": str})])),
     ("auth", OrderedDict([
         ("type", {
-            "value": "None",
+            "value": "none",
             "help": "authentication method",
             "type": str}),
         ("htpasswd_filename", {
@@ -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",
@@ -212,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

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):
+    if debug:
+        logger.setLevel(logging.DEBUG)
+    else:
+        logger.addFilter(RemoveTracebackFilter())
+    if filename:
         # Configuration taken from file
-        configure_from_file(logger, filename, debug)
+        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):
-                configure_from_file(logger, filename, debug)
+                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
-        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())
     return logger

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" %

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)
@@ -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):
@@ -806,15 +807,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 +822,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,24 +833,25 @@ 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"))
-        status, headers, answer = self.request("GET", "/", REMOTE_USER="user")
+        self.configuration["storage"]["hook"] = (
+            "mkdir %s" % os.path.join("collection-root", "created_by_hook"))
+        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
 
     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 +877,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):

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)