diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 2330bb4..8a30dfd 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -84,12 +84,19 @@ Acceptance criteria: ### Correct environment parsing and defaults -- [ ] Replace direct `os.getenv()` assignments in `start.py` with typed parsers. -- [ ] Preserve the documented defaults for upload limit, download limit, and - delete-confirm threshold when variables are absent. -- [ ] Validate UID, GID, limits, threshold, and booleans before daemon startup. -- [ ] Reject negative and malformed numeric values with useful messages. -- [ ] Do not pass `None` as a Seafile configuration value. +- [x] Replace direct `os.getenv()` assignments in `start.py` with typed parsers. + See `dsc/config.py` and `defaults_from_env()`. +- [x] Preserve the documented defaults for upload limit, download limit, and + delete-confirm threshold when variables are absent. They were not: an unset + variable overwrote the argparse default with `None`, so a default container + sent the literal string `None` for all three. +- [x] Validate UID, GID, limits, threshold, and booleans before daemon startup. + UID and GID are now integers; as strings they never equalled the integers + they were compared against, so `usermod` ran on every start. +- [x] Reject negative and malformed numeric values with useful messages. +- [x] Do not pass `None` as a Seafile configuration value. Booleans are also + rendered as `true`/`false` rather than Python's `True`/`False`, since the + daemon reads that setting with its boolean accessor. Acceptance criteria: diff --git a/README.md b/README.md index a62caca..69a9ace 100644 --- a/README.md +++ b/README.md @@ -46,10 +46,15 @@ volumes: - `DELETE_CONFIRM_THRESHOLD` - represents the number of files that require confirmation before being deleted simultaneously. Default value is _500_. - `DISABLE_VERIFY_CERTIFICATE` - set to _true_ to disable server's certificate - verification. Default value is _false_. + verification. Default value is _false_. Booleans accept _true_/_false_, + _1_/_0_, _yes_/_no_ and _on_/_off_, in any case. - `UPLOAD_LIMIT` / `DOWNLOAD_LIMIT` - upload/download speed limit in B/s (bytes per second). Default values are _0_ (unlimited). +Numeric variables must be whole, non-negative numbers. A malformed or negative + value stops the container at startup with a message naming the variable, + rather than being passed on to the Seafile client. + ### Using Docker secrets: Mount the secret and point `PASSWORD_FILE` at it, so the password never enters the environment: diff --git a/dsc/client.py b/dsc/client.py index 62c56f0..a898594 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -14,7 +14,7 @@ import seafile from dsc import const from dsc.errors import ConfigError, DaemonError, DaemonTimeout -from dsc.misc import create_dir, hide_secrets, user_cmd +from dsc.misc import config_value, create_dir, hide_secrets, user_cmd _lg = logging.getLogger(__name__) @@ -304,6 +304,9 @@ class SeafileClient: if key not in const.AVAILABLE_SEAFCLI_OPTIONS: continue + if value is None: + raise ConfigError(f"No value for seafile option {key}") + # check current value cmd = ["seaf-cli", "config", "-k", key] _lg.info("Checking seafile client option: %s", " ".join(cmd)) @@ -314,11 +317,11 @@ class SeafileClient: cur_value = cur_value.split(sep="=")[1].strip() except IndexError: cur_value = None - if cur_value == str(value): + if cur_value == config_value(value): continue # set new value - cmd = ["seaf-cli", "config", "-k", key, "-v", str(value)] + cmd = ["seaf-cli", "config", "-k", key, "-v", config_value(value)] _lg.info("Setting seafile client option: %s", " ".join(cmd)) self.__run(cmd) need_restart = True diff --git a/dsc/config.py b/dsc/config.py new file mode 100644 index 0000000..d6f5f86 --- /dev/null +++ b/dsc/config.py @@ -0,0 +1,65 @@ +"""Typed reading of the container's environment. + +Everything in the environment is text, including values the client later +compares as numbers or passes to seaf-cli. Reading them directly leaves two +failures that are silent until much later: an unset variable becomes None and +reaches seaf-cli as the string "None", and a numeric setting stays a string +that never equals the integer it is compared against. +""" + +import os + +from dsc.errors import ConfigError + +_TRUE_VALUES = {"true", "1", "yes", "on"} +_FALSE_VALUES = {"false", "0", "no", "off"} + + +def env_str(name: str, default: str = None) -> str: + """Return a text setting. An empty variable counts as unset.""" + value = os.getenv(name) + if value is None or value == "": + return default + return value + + +def env_int(name: str, default: int, minimum: int = 0) -> int: + """ + Return a numeric setting as an int, never as text and never as None. + + Values below ``minimum`` are rejected: a negative speed limit or deletion + threshold is not something to pass on to seaf-cli and hope for the best. + """ + raw = env_str(name) + if raw is None: + return default + + stripped = raw.strip() + try: + value = int(stripped, 10) + except ValueError: + raise ConfigError( + f"{name} must be a whole number, got {stripped!r}" + ) + + if value < minimum: + raise ConfigError( + f"{name} must be {minimum} or greater, got {stripped!r}" + ) + return value + + +def env_bool(name: str, default: bool = False) -> bool: + """Return a boolean setting, accepting the usual spellings.""" + raw = env_str(name) + if raw is None: + return default + + value = raw.strip().lower() + if value in _TRUE_VALUES: + return True + if value in _FALSE_VALUES: + return False + + accepted = ", ".join(sorted(_TRUE_VALUES | _FALSE_VALUES)) + raise ConfigError(f"{name} must be one of {accepted}, got {raw.strip()!r}") diff --git a/dsc/misc.py b/dsc/misc.py index 2b214eb..6744b74 100644 --- a/dsc/misc.py +++ b/dsc/misc.py @@ -65,3 +65,15 @@ def hide_secrets(cmd: list, *secrets: str) -> list: if secret in arg: cmd[i] = arg.replace(secret, "********") return cmd + + +def config_value(value) -> str: + """ + Render a setting the way seaf-cli stores it. + + Booleans are written as "true" and "false": the daemon reads this setting + with its boolean accessor, and Python's str() would produce "True". + """ + if isinstance(value, bool): + return "true" if value else "false" + return str(value) diff --git a/start.py b/start.py index f51cc46..cf1b409 100755 --- a/start.py +++ b/start.py @@ -7,6 +7,7 @@ import signal import sys from dsc import SeafileClient, const +from dsc.config import env_bool, env_int, env_str from dsc.errors import ConfigError, DscError, GracefulShutdown from dsc.misc import setup_uid, create_dir from dsc.paths import plan_lib_dirs @@ -26,6 +27,30 @@ def install_signal_handlers(): signal.signal(signum, handle_signal) +def defaults_from_env() -> dict: + """ + Read every setting from the environment with its documented default. + + Reading these directly leaves an unset variable as None, which reaches + seaf-cli as the string "None" and silently replaces the documented value. + """ + return dict( + server=env_str("SERVER_HOST"), + username=env_str("USERNAME"), + password=env_str("PASSWORD"), + password_file=env_str("PASSWORD_FILE"), + token=env_str("TOKEN"), + token_file=env_str("TOKEN_FILE"), + libraries=env_str("LIBRARY_ID"), + uid=env_int("SEAFILE_UID", 1000), + gid=env_int("SEAFILE_GID", 1000), + upload_limit=env_int("UPLOAD_LIMIT", 0), + download_limit=env_int("DOWNLOAD_LIMIT", 0), + delete_confirm_threshold=env_int("DELETE_CONFIRM_THRESHOLD", 500), + disable_verify_certificate=env_bool("DISABLE_VERIFY_CERTIFICATE", False), + ) + + def resolve_libraries(client, requested: str, server: str) -> set: """Map the requested names or IDs to library IDs that are not synced yet.""" libs_to_sync = set() @@ -94,21 +119,11 @@ def main(): parser.add_argument("--disable-verify-certificate", action="store_true") parser.add_argument("--delete-confirm-threshold", type=int, default=500) - parser.set_defaults( - server=os.getenv("SERVER_HOST"), - username=os.getenv("USERNAME"), - password=os.getenv("PASSWORD"), - password_file=os.getenv("PASSWORD_FILE"), - token=os.getenv("TOKEN"), - token_file=os.getenv("TOKEN_FILE"), - libraries=os.getenv("LIBRARY_ID"), - uid=os.getenv("SEAFILE_UID", default=1000), - gid=os.getenv("SEAFILE_GID", default=1000), - upload_limit=os.getenv("UPLOAD_LIMIT"), - download_limit=os.getenv("DOWNLOAD_LIMIT"), - disable_verify_certificate=os.getenv("DISABLE_VERIFY_CERTIFICATE") in ("true", "1", "True"), - delete_confirm_threshold=os.getenv("DELETE_CONFIRM_THRESHOLD"), - ) + try: + parser.set_defaults(**defaults_from_env()) + except ConfigError as err: + _lg.error("%s", err) + return 2 args = parser.parse_args() if not args.server: parser.error("Seafile server is not specified") diff --git a/tests/test_commands.py b/tests/test_commands.py index 082f16f..c442197 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -8,7 +8,7 @@ import pytest from dsc import client as client_module from dsc import const -from dsc.errors import DaemonError +from dsc.errors import ConfigError, DaemonError from dsc.client import SeafileClient from dsc.misc import hide_secrets, user_cmd @@ -235,3 +235,33 @@ def test_local_libraries_error_keeps_the_original_cause(client, monkeypatch): with pytest.raises(DaemonError) as err: client.get_local_libraries() assert err.value.__cause__ is not None + + +def test_configure_never_sends_none_as_a_value(client, calls): + """A None here reaches seaf-cli as the literal string "None".""" + with pytest.raises(ConfigError): + client.configure(argparse.Namespace(upload_limit=None), check_for_daemon=False) + + for argv, _ in calls: + assert "None" not in argv + + +@pytest.mark.parametrize("value, expected", [ + (True, "true"), + (False, "false"), + (0, "0"), + (500, "500"), +]) +def test_configure_renders_values_the_way_seaf_cli_stores_them( + client, calls, monkeypatch, value, expected): + # Changing an option restarts the daemon; keep this on the value only. + monkeypatch.setattr(client, "stop_daemon", lambda: None) + monkeypatch.setattr(client, "start_daemon", lambda: None) + + client.configure(argparse.Namespace(disable_verify_certificate=value) + if isinstance(value, bool) + else argparse.Namespace(upload_limit=value), + check_for_daemon=False) + + setting = [argv for argv, _ in calls if "-v" in argv][-1] + assert setting[setting.index("-v") + 1] == expected diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 0000000..d84e8f1 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,120 @@ +"""The environment is untyped text; everything derived from it is checked.""" + +import pytest + +import start +from dsc.config import env_bool, env_int, env_str +from dsc.errors import ConfigError + + +@pytest.fixture +def env(monkeypatch): + def set_env(**values): + for name, value in values.items(): + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, value) + return set_env + + +@pytest.mark.parametrize("raw, expected", [ + (None, 7), # unset keeps the documented default + ("", 7), # an empty variable is the same as unset + ("0", 0), # zero is a real value, not "missing" + ("1", 1), + ("4096", 4096), + (" 42 ", 42), + # int() accepts any Unicode decimal digit and normalises it; the value + # leaves as an ASCII integer, so this is harmless rather than rejected. + ("\u0663", 3), +]) +def test_env_int_accepts(env, raw, expected): + env(LIMIT=raw) + assert env_int("LIMIT", 7) == expected + + +@pytest.mark.parametrize("raw", ["-1", "-0.5", "abc", "1.5", "1e3", "0x10", "1 2", "٣ x"]) +def test_env_int_rejects(env, raw): + env(LIMIT=raw) + with pytest.raises(ConfigError) as err: + env_int("LIMIT", 7) + assert "LIMIT" in str(err.value) + assert raw.strip() in str(err.value) + + +def test_env_int_allows_a_negative_value_when_permitted(env): + env(LIMIT="-1") + assert env_int("LIMIT", 7, minimum=-1) == -1 + + +@pytest.mark.parametrize("raw, expected", [ + (None, False), + ("", False), + ("true", True), ("True", True), ("TRUE", True), + ("1", True), ("yes", True), ("on", True), + ("false", False), ("False", False), ("0", False), + ("no", False), ("off", False), (" true ", True), +]) +def test_env_bool_accepts(env, raw, expected): + env(FLAG=raw) + assert env_bool("FLAG", False) is expected + + +@pytest.mark.parametrize("raw", ["maybe", "2", "-1", "y3s", "truthy"]) +def test_env_bool_rejects(env, raw): + env(FLAG=raw) + with pytest.raises(ConfigError) as err: + env_bool("FLAG", False) + assert "FLAG" in str(err.value) + + +def test_env_bool_keeps_a_true_default_when_unset(env): + env(FLAG=None) + assert env_bool("FLAG", True) is True + + +@pytest.mark.parametrize("raw, expected", [ + (None, None), ("", None), ("value", "value"), (" spaced ", " spaced "), +]) +def test_env_str(env, raw, expected): + env(NAME=raw) + assert env_str("NAME") == expected + + +def test_a_default_container_gets_exactly_the_documented_defaults(monkeypatch): + """The values the README promises when nothing optional is configured.""" + for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD", + "SEAFILE_UID", "SEAFILE_GID", "DISABLE_VERIFY_CERTIFICATE"): + monkeypatch.delenv(name, raising=False) + + defaults = start.defaults_from_env() + + assert defaults["upload_limit"] == 0 + assert defaults["download_limit"] == 0 + assert defaults["delete_confirm_threshold"] == 500 + assert defaults["uid"] == 1000 + assert defaults["gid"] == 1000 + assert defaults["disable_verify_certificate"] is False + + +def test_no_default_is_none_where_a_value_is_documented(monkeypatch): + """A None here reaches seaf-cli as the literal string "None".""" + for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD", + "SEAFILE_UID", "SEAFILE_GID"): + monkeypatch.delenv(name, raising=False) + + defaults = start.defaults_from_env() + for key in ("upload_limit", "download_limit", "delete_confirm_threshold", + "uid", "gid"): + assert defaults[key] is not None, f"{key} must never default to None" + + +def test_numeric_settings_are_integers_not_strings(monkeypatch): + """A string UID never equals the integer it is compared against.""" + monkeypatch.setenv("SEAFILE_UID", "1000") + monkeypatch.setenv("UPLOAD_LIMIT", "50") + + defaults = start.defaults_from_env() + assert defaults["uid"] == 1000 and isinstance(defaults["uid"], int) + assert defaults["upload_limit"] == 50 and isinstance(defaults["upload_limit"], int)