From 9bfcf828eafcace938a118f16d5d75a355a14fe4 Mon Sep 17 00:00:00 2001 From: Alex <25013571+alexhb1@users.noreply.github.com> Date: Sun, 29 Mar 2026 16:39:40 +0100 Subject: [PATCH] Fixes: Env variable config usage, retry availability, Entrypoint permissions (#817) - Clean up a few uses of config options that may miss the env variable if this is set - Add enhanced retry availability utilising the DB to persist download errors / retries across restarts, request failures, and pass Prowlarr detail through the download task to maintain retry data. - Strip back entrypoint permissions for less intensive chown operations. Fixes #796 --- entrypoint.sh | 51 ++-- scripts/fix_permissions.py | 20 +- shelfmark/config/notifications_settings.py | 6 +- shelfmark/config/security.py | 5 +- shelfmark/core/activity_routes.py | 7 +- shelfmark/core/admin_routes.py | 30 +- shelfmark/core/admin_settings_routes.py | 11 +- shelfmark/core/auth_modes.py | 33 +-- shelfmark/core/config.py | 15 +- shelfmark/core/download_history_service.py | 98 ++++++- shelfmark/core/models.py | 7 + shelfmark/core/oidc_routes.py | 25 +- shelfmark/core/request_helpers.py | 9 +- shelfmark/core/requests_service.py | 25 +- shelfmark/core/user_db.py | 9 + shelfmark/download/orchestrator.py | 267 +++++++++++++++++- shelfmark/main.py | 98 +++++-- shelfmark/release_sources/prowlarr/handler.py | 46 ++- src/frontend/src/components/ReleaseModal.tsx | 13 +- .../components/activity/activityCardModel.ts | 5 +- .../components/activity/activityMappers.ts | 2 + .../src/components/activity/activityTypes.ts | 1 + src/frontend/src/hooks/useDownloadTracking.ts | 8 +- .../src/tests/activityCardModel.node.test.ts | 29 ++ src/frontend/src/types/index.ts | 1 + tests/config/test_environment.py | 18 ++ tests/core/test_activity_routes_api.py | 194 +++++++++++++ .../core/test_activity_terminal_snapshots.py | 58 ++++ tests/core/test_admin_users_api.py | 21 +- tests/core/test_download_api_guardrails.py | 142 ++++++++++ tests/core/test_oidc_integration.py | 24 +- tests/core/test_oidc_routes.py | 15 +- tests/core/test_requests_service.py | 37 +++ tests/download/test_orchestrator_retry.py | 30 ++ .../test_orchestrator_user_output_mode.py | 42 +++ tests/e2e/test_auth_endpoints.py | 80 +++--- tests/e2e/test_proxy_auth_middleware.py | 151 +++++----- tests/prowlarr/test_handler.py | 26 ++ 38 files changed, 1366 insertions(+), 293 deletions(-) diff --git a/entrypoint.sh b/entrypoint.sh index ed3e51d..dc978d6 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -186,34 +186,43 @@ test_write() { } make_writable() { - folder=$1 - did_full_chown=0 + local folder="$1" + local mode="${2:-tree}" + local did_full_chown=0 + local is_writable set +e - test_write $folder + test_write "$folder" is_writable=$? set -e if [ $is_writable -eq 0 ]; then echo "Folder $folder is writable, no need to change ownership" else - echo "Folder $folder is not writable, changing ownership" - change_ownership $folder - chmod -R g+r,g+w $folder || echo "Failed to change group permissions for ${folder}, continuing..." + if [ "$mode" = "root" ]; then + echo "Folder $folder is not writable, fixing top-level ownership and permissions" + mkdir -p "$folder" + chown "${RUN_UID}:${RUN_GID}" "$folder" || echo "Failed to change ownership for ${folder}, continuing..." + chmod u+rwx "$folder" || echo "Failed to change owner permissions for ${folder}, continuing..." + else + echo "Folder $folder is not writable, changing ownership" + change_ownership "$folder" + chmod -R g+r,g+w "$folder" || echo "Failed to change group permissions for ${folder}, continuing..." + fi did_full_chown=1 fi # Fix any misowned subdirectories/files (e.g., from previous runs as root) - if [ "$did_full_chown" -eq 0 ] && [ -d "$folder" ]; then + if [ "$mode" = "tree" ] && [ "$did_full_chown" -eq 0 ] && [ -d "$folder" ]; then echo "Checking for misowned files/directories in $folder" # Stay on the same filesystem to avoid traversing mounted subpaths # (for example read-only bind mounts under /app in dev setups). find "$folder" -xdev -mindepth 1 \( ! -user "$RUN_UID" -o ! -group "$RUN_GID" \) \ -exec chown "$RUN_UID:$RUN_GID" {} + 2>/dev/null || true fi - test_write $folder || echo "Failed to test write to ${folder}, continuing..." + test_write "$folder" || echo "Failed to test write to ${folder}, continuing..." } fix_misowned() { - folder=$1 - mkdir -p $folder + local folder="$1" + mkdir -p "$folder" echo "Checking for misowned files/directories in $folder" # Stay on the same filesystem to avoid traversing mounted subpaths # (for example read-only bind mounts under /app in dev setups). @@ -223,8 +232,8 @@ fix_misowned() { # Ensure proper ownership of application directories change_ownership() { - folder=$1 - mkdir -p $folder + local folder="$1" + mkdir -p "$folder" echo "Changing ownership of $folder to $USERNAME:$RUN_GID" chown -R "${RUN_UID}:${RUN_GID}" "${folder}" || echo "Failed to change ownership for ${folder}, continuing..." } @@ -273,7 +282,6 @@ ensure_symlinked_dir() { fi } -fix_misowned /app fix_misowned /var/log/shelfmark fix_misowned /tmp/shelfmark @@ -299,19 +307,22 @@ if [ "${USING_EXTERNAL_BYPASSER}" != "true" ]; then fi fi -# Test write to all folders -make_writable ${CONFIG_DIR:-/config} -make_writable ${INGEST_DIR:-/books} +# Config can contain existing state we must keep accessing, so it keeps the +# thorough repair path. Output destination roots only need top-level writability. +make_writable "${CONFIG_DIR:-/config}" tree +# Entrypoint only has env vars available at this stage, so use the legacy +# INGEST_DIR env var as the fallback source for the default destination root. +make_writable "${INGEST_DIR:-/books}" root -# Fix permissions on directories configured in settings -echo "Checking for additional configured directories..." +# Check any additional configured destination roots from saved settings +echo "Checking for additional configured destination roots..." if [ -f /app/scripts/fix_permissions.py ]; then configured_dirs=$(python3 /app/scripts/fix_permissions.py 2>/dev/null || echo "") if [ -n "$configured_dirs" ]; then echo "$configured_dirs" | while read -r dir; do if [ -n "$dir" ] && [ -d "$dir" ]; then - echo "Checking configured directory: $dir" - make_writable "$dir" + echo "Checking configured destination root: $dir" + make_writable "$dir" root fi done fi diff --git a/scripts/fix_permissions.py b/scripts/fix_permissions.py index a4477a8..b219de3 100755 --- a/scripts/fix_permissions.py +++ b/scripts/fix_permissions.py @@ -1,10 +1,8 @@ #!/usr/bin/env python3 -"""Fix permissions on all configured directories. +"""List configured destination roots that may need permission repair. -This script is called by the entrypoint to ensure all user-configured -directories have correct ownership. It reads directory paths from: -- CONFIG_DIR environment variable -- Config files in CONFIG_DIR/plugins/ +This script is called by the entrypoint to find configured output destination +roots from config files under CONFIG_DIR/plugins/. Outputs directory paths that need permission fixing (one per line). The entrypoint handles the actual chown operations. @@ -17,7 +15,7 @@ from pathlib import Path def get_directories_from_config() -> set[str]: - """Extract all directory paths from config files.""" + """Extract configured destination-style paths from config files.""" directories = set() config_dir = Path(os.getenv("CONFIG_DIR", "/config")) @@ -26,12 +24,12 @@ def get_directories_from_config() -> set[str]: if not plugins_dir.exists(): return directories - # Keys that contain directory paths + # Keys that can point at output destination roots or legacy equivalents directory_keys = { - # Main destinations + # Current destination settings "DESTINATION", "DESTINATION_AUDIOBOOK", - # Content type routing directories + # Content-type routing destinations "AA_CONTENT_TYPE_DIR_FICTION", "AA_CONTENT_TYPE_DIR_NON_FICTION", "AA_CONTENT_TYPE_DIR_UNKNOWN", @@ -40,7 +38,7 @@ def get_directories_from_config() -> set[str]: "AA_CONTENT_TYPE_DIR_STANDARDS", "AA_CONTENT_TYPE_DIR_MUSICAL_SCORE", "AA_CONTENT_TYPE_DIR_OTHER", - # Legacy keys (in case of old configs) + # Legacy path settings still recognized in older configs "INGEST_DIR", "INGEST_DIR_AUDIOBOOK", "INGEST_DIR_BOOK_FICTION", @@ -73,7 +71,7 @@ def get_directories_from_config() -> set[str]: def main(): - """Output all configured directories that exist.""" + """Output configured destination roots that currently exist.""" directories = get_directories_from_config() # Filter to directories that actually exist diff --git a/shelfmark/config/notifications_settings.py b/shelfmark/config/notifications_settings.py index b452cd8..cd2d244 100644 --- a/shelfmark/config/notifications_settings.py +++ b/shelfmark/config/notifications_settings.py @@ -6,6 +6,7 @@ import re from typing import Any from urllib.parse import urlsplit +from shelfmark.core.config import config as app_config from shelfmark.core.notifications import NotificationEvent, send_test_notification from shelfmark.core.settings_registry import ( ActionButton, @@ -245,8 +246,9 @@ def _on_save_notifications(values: dict[str, Any]) -> dict[str, Any]: def _test_admin_notification_action(current_values: dict[str, Any]) -> dict[str, Any]: - persisted = load_config_file("notifications") - effective: dict[str, Any] = dict(persisted) + effective: dict[str, Any] = { + "ADMIN_NOTIFICATION_ROUTES": app_config.get("ADMIN_NOTIFICATION_ROUTES", []), + } if isinstance(current_values, dict): effective.update(current_values) diff --git a/shelfmark/config/security.py b/shelfmark/config/security.py index fee3994..95daff1 100644 --- a/shelfmark/config/security.py +++ b/shelfmark/config/security.py @@ -7,6 +7,7 @@ from shelfmark.config.security_handlers import ( on_save_security, test_oidc_connection, ) +from shelfmark.core.config import config as app_config from shelfmark.core.logger import setup_logger from shelfmark.core.settings_registry import ( register_settings, @@ -58,7 +59,9 @@ def _on_save_security(values: Dict[str, Any]) -> Dict[str, Any]: def _test_oidc_connection(current_values: Dict[str, Any] = None) -> Dict[str, Any]: return test_oidc_connection( - load_security_config=lambda: load_config_file("security"), + load_security_config=lambda: { + "OIDC_DISCOVERY_URL": app_config.get("OIDC_DISCOVERY_URL", ""), + }, current_values=current_values or {}, logger=logger, ) diff --git a/shelfmark/core/activity_routes.py b/shelfmark/core/activity_routes.py index 9cd43f2..87b10e5 100644 --- a/shelfmark/core/activity_routes.py +++ b/shelfmark/core/activity_routes.py @@ -336,6 +336,7 @@ def _effective_download_row_for_activity( effective_row = dict(row) effective_row["final_status"] = QueueStatus.ERROR.value + effective_row["retry_final_status"] = final_status status_message = effective_row.get("status_message") if not isinstance(status_message, str) or not status_message.strip(): @@ -364,9 +365,9 @@ def _build_download_status_from_db( continue final_status = row.get("final_status") + queue_entry = queue_index.pop(task_id, None) if final_status == ACTIVE_DOWNLOAD_STATUS: - queue_entry = queue_index.pop(task_id, None) if queue_entry is not None: bucket_key, queue_payload = queue_entry status[bucket_key][task_id] = queue_payload @@ -379,6 +380,10 @@ def _build_download_status_from_db( status[QueueStatus.ERROR][task_id] = download_payload elif final_status in VALID_TERMINAL_STATUSES: download_payload = DownloadHistoryService.to_download_payload(row) + if queue_entry is not None: + _, queue_payload = queue_entry + if isinstance(queue_payload, dict) and "retry_available" in queue_payload: + download_payload["retry_available"] = bool(queue_payload.get("retry_available")) # For complete/cancelled the saved status_message is a stale # progress string (e.g. "Fetching download sources") — clear it # so the frontend only shows its own status label. Error rows diff --git a/shelfmark/core/admin_routes.py b/shelfmark/core/admin_routes.py index 344a039..0d6ceb4 100644 --- a/shelfmark/core/admin_routes.py +++ b/shelfmark/core/admin_routes.py @@ -17,6 +17,7 @@ from shelfmark.config.booklore_settings import ( get_booklore_path_options, ) from shelfmark.config.env import CWA_DB_PATH +from shelfmark.core.config import config as app_config from shelfmark.core.admin_settings_routes import ( register_admin_settings_routes, validate_user_settings, @@ -32,7 +33,6 @@ from shelfmark.core.auth_modes import ( ) from shelfmark.core.cwa_user_sync import sync_cwa_users_from_rows from shelfmark.core.logger import setup_logger -from shelfmark.core.settings_registry import load_config_file from shelfmark.core.user_db import UserDB logger = setup_logger(__name__) @@ -47,10 +47,12 @@ def _get_user_edit_capabilities( user.get("auth_source"), user.get("oidc_subject"), ) - if security_config is None and auth_source == AUTH_SOURCE_OIDC: - security_config = load_config_file("security") - - oidc_use_admin_group = bool((security_config or {}).get("OIDC_USE_ADMIN_GROUP", True)) + oidc_use_admin_group = bool( + (security_config or {}).get( + "OIDC_USE_ADMIN_GROUP", + app_config.get("OIDC_USE_ADMIN_GROUP", True), + ) + ) role_managed_by_oidc_group = auth_source == AUTH_SOURCE_OIDC and oidc_use_admin_group can_edit_role = auth_source == AUTH_SOURCE_BUILTIN or ( auth_source == AUTH_SOURCE_OIDC and not role_managed_by_oidc_group @@ -72,8 +74,11 @@ def _sanitize_user(user: dict) -> dict: return sanitized -def _oidc_role_management_message(security_config: dict[str, Any]) -> str: - admin_group = security_config.get("OIDC_ADMIN_GROUP", "") +def _oidc_role_management_message(security_config: dict[str, Any] | None = None) -> str: + admin_group = (security_config or {}).get( + "OIDC_ADMIN_GROUP", + app_config.get("OIDC_ADMIN_GROUP", ""), + ) if admin_group: return ( "Admin roles for OIDC users are managed by the " @@ -152,9 +157,8 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: """List all users.""" users = user_db.list_users() auth_mode = g.auth_mode - security_config = load_config_file("security") return jsonify([ - _serialize_user(u, auth_mode, security_config=security_config) + _serialize_user(u, auth_mode) for u in users ]) @@ -217,7 +221,6 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: _serialize_user( user, g.auth_mode, - security_config=load_config_file("security"), ) ), 201 @@ -232,7 +235,6 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: result = _serialize_user( user, g.auth_mode, - security_config=load_config_file("security"), ) result["settings"] = user_db.get_user_settings(user_id) return jsonify(result) @@ -246,12 +248,11 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: return jsonify({"error": "User not found"}), 404 data = request.get_json() or {} - security_config = load_config_file("security") auth_source = normalize_auth_source( user.get("auth_source"), user.get("oidc_subject"), ) - capabilities = _get_user_edit_capabilities(user, security_config=security_config) + capabilities = _get_user_edit_capabilities(user) # Handle optional password update password = data.get("password", "") @@ -285,7 +286,7 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: if auth_source == AUTH_SOURCE_OIDC: return jsonify({ "error": "Cannot change role for OIDC user when group-based authorization is enabled", - "message": _oidc_role_management_message(security_config), + "message": _oidc_role_management_message(), }), 400 return jsonify({ @@ -347,7 +348,6 @@ def register_admin_routes(app: Flask, user_db: UserDB) -> None: result = _serialize_user( updated, g.auth_mode, - security_config=security_config, ) result["settings"] = user_db.get_user_settings(user_id) logger.info(f"Admin updated user {user_id}") diff --git a/shelfmark/core/admin_settings_routes.py b/shelfmark/core/admin_settings_routes.py index d3a21eb..2676544 100644 --- a/shelfmark/core/admin_settings_routes.py +++ b/shelfmark/core/admin_settings_routes.py @@ -4,6 +4,7 @@ from typing import Any, Callable from flask import Flask, jsonify, request +from shelfmark.core.config import config as app_config from shelfmark.config.notifications_settings import ( build_notification_test_result, is_valid_notification_url, @@ -143,16 +144,14 @@ def register_admin_settings_routes( @app.route("/api/admin/download-defaults", methods=["GET"]) @require_admin def admin_download_defaults(): - config = load_config_file("downloads") defaults = { - key: ("" if (value := config.get(key, field.default)) is None else value) + key: ("" if (value := app_config.get(key, field.default)) is None else value) for key, field in _get_ordered_user_overridable_fields("downloads") } - security_config = load_config_file("security") - defaults["OIDC_ADMIN_GROUP"] = security_config.get("OIDC_ADMIN_GROUP", "") - defaults["OIDC_USE_ADMIN_GROUP"] = security_config.get("OIDC_USE_ADMIN_GROUP", True) - defaults["OIDC_AUTO_PROVISION"] = security_config.get("OIDC_AUTO_PROVISION", True) + defaults["OIDC_ADMIN_GROUP"] = app_config.get("OIDC_ADMIN_GROUP", "") + defaults["OIDC_USE_ADMIN_GROUP"] = app_config.get("OIDC_USE_ADMIN_GROUP", True) + defaults["OIDC_AUTO_PROVISION"] = app_config.get("OIDC_AUTO_PROVISION", True) return jsonify(defaults) @app.route("/api/admin/booklore-options", methods=["GET"]) diff --git a/shelfmark/core/auth_modes.py b/shelfmark/core/auth_modes.py index ca8c9ea..94ba972 100644 --- a/shelfmark/core/auth_modes.py +++ b/shelfmark/core/auth_modes.py @@ -75,30 +75,6 @@ def determine_auth_mode( return "none" -def _load_security_config() -> dict[str, Any]: - """Load security settings with environment-backed values applied.""" - from shelfmark.core.settings_registry import ( - get_setting_value, - get_settings_field_map, - load_config_file, - ) - - try: - import shelfmark.config.security # noqa: F401 - except Exception: - return load_config_file("security") - - config = load_config_file("security") - field_map = get_settings_field_map(tab_name="security") - if not field_map: - return config - - resolved = dict(config) - for key, (field, tab_name) in field_map.items(): - resolved[key] = get_setting_value(field, tab_name) - return resolved - - def load_active_auth_mode( cwa_db_path: Any | None, *, @@ -106,7 +82,14 @@ def load_active_auth_mode( ) -> str: """Resolve active auth mode using current security config and runtime prerequisites.""" try: - security_config = _load_security_config() + from shelfmark.core.config import config as app_config + + security_config = { + "AUTH_METHOD": app_config.get("AUTH_METHOD", "none"), + "PROXY_AUTH_USER_HEADER": app_config.get("PROXY_AUTH_USER_HEADER", ""), + "OIDC_DISCOVERY_URL": app_config.get("OIDC_DISCOVERY_URL", ""), + "OIDC_CLIENT_ID": app_config.get("OIDC_CLIENT_ID", ""), + } return determine_auth_mode( security_config, cwa_db_path, diff --git a/shelfmark/core/config.py b/shelfmark/core/config.py index c462ec4..d15ef78 100644 --- a/shelfmark/core/config.py +++ b/shelfmark/core/config.py @@ -106,18 +106,9 @@ class Config: self._field_map.clear() self._cache.clear() - for tab in registry.get_all_settings_tabs(): - for field in tab.fields: - # Skip action buttons and headings - they don't have values - if isinstance(field, (registry.ActionButton, registry.HeadingField)): - continue - - key = field.key - self._field_map[key] = (field, tab.name) - - # Load current value - value = registry.get_setting_value(field, tab.name) - self._cache[key] = value + for key, (field, tab_name) in registry.get_settings_field_map().items(): + self._field_map[key] = (field, tab_name) + self._cache[key] = registry.get_setting_value(field, tab_name) self._loaded = True diff --git a/shelfmark/core/download_history_service.py b/shelfmark/core/download_history_service.py index d3712c4..d662df6 100644 --- a/shelfmark/core/download_history_service.py +++ b/shelfmark/core/download_history_service.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import os import sqlite3 import threading @@ -74,9 +75,17 @@ class DownloadHistoryService: conn.execute("PRAGMA foreign_keys = ON") return conn - @staticmethod - def _row_to_dict(row: sqlite3.Row | None) -> dict[str, Any] | None: - return dict(row) if row is not None else None + @classmethod + def _normalize_row_dict(cls, row: dict[str, Any] | None) -> dict[str, Any] | None: + if row is None: + return None + normalized = dict(row) + normalized["retry_payload"] = cls._deserialize_retry_payload(normalized.get("retry_payload")) + return normalized + + @classmethod + def _row_to_dict(cls, row: sqlite3.Row | None) -> dict[str, Any] | None: + return cls._normalize_row_dict(dict(row) if row is not None else None) @staticmethod def _to_item_key(task_id: str) -> str: @@ -89,6 +98,69 @@ class DownloadHistoryService: return None return normalized if os.path.exists(normalized) else None + @staticmethod + def _serialize_retry_payload(payload: Any) -> str | None: + if payload is None: + return None + try: + return json.dumps(payload, ensure_ascii=False, separators=(",", ":")) + except (TypeError, ValueError) as exc: + raise ValueError("retry_payload must be JSON-serializable") from exc + + @staticmethod + def _deserialize_retry_payload(value: Any) -> dict[str, Any] | None: + if isinstance(value, dict): + return dict(value) + normalized = normalize_optional_text(value) + if normalized is None: + return None + try: + parsed = json.loads(normalized) + except json.JSONDecodeError: + return None + return parsed if isinstance(parsed, dict) else None + + @staticmethod + def _has_staged_retry_source(retry_payload: dict[str, Any]) -> bool: + staged_path = retry_payload.get("staged_path") + normalized_staged_path = normalize_optional_text(staged_path) + if normalized_staged_path is None: + return False + return os.path.exists(normalized_staged_path) + + @staticmethod + def _can_retry_without_staged_source(retry_payload: dict[str, Any]) -> bool: + return bool(retry_payload.get("can_retry_without_staged_source", True)) + + @staticmethod + def is_retry_available(row: dict[str, Any]) -> bool: + final_status = str( + row.get("retry_final_status") or row.get("final_status") or "" + ).strip().lower() + retry_payload = DownloadHistoryService._deserialize_retry_payload(row.get("retry_payload")) + if retry_payload is None: + return False + + has_staged_retry_source = DownloadHistoryService._has_staged_retry_source(retry_payload) + can_retry_without_staged_source = ( + DownloadHistoryService._can_retry_without_staged_source(retry_payload) + ) + request_id = normalize_optional_positive_int(row.get("request_id"), "request_id") + if request_id is None: + if final_status in {ACTIVE_DOWNLOAD_STATUS, "cancelled"}: + return can_retry_without_staged_source + if final_status == "error": + return has_staged_retry_source or can_retry_without_staged_source + return False + + if final_status in {ACTIVE_DOWNLOAD_STATUS, "cancelled"}: + return can_retry_without_staged_source + + if final_status != "error": + return False + + return has_staged_retry_source + @staticmethod def to_download_payload(row: dict[str, Any]) -> dict[str, Any]: return { @@ -107,6 +179,7 @@ class DownloadHistoryService: "user_id": row.get("user_id"), "username": row.get("username"), "request_id": row.get("request_id"), + "retry_available": DownloadHistoryService.is_retry_available(row), } @staticmethod @@ -163,6 +236,7 @@ class DownloadHistoryService: preview: str | None, content_type: str | None, origin: str, + retry_payload: dict[str, Any] | None = None, ) -> None: """Record a download at queue time with final_status='active'. @@ -180,6 +254,7 @@ class DownloadHistoryService: if normalized_title is None: raise ValueError("title must be a non-empty string") normalized_origin = _normalize_origin(origin) + normalized_retry_payload = self._serialize_retry_payload(retry_payload) recorded_at = now_utc_iso() with self._lock: @@ -192,14 +267,15 @@ class DownloadHistoryService: source, source_display_name, title, author, format, size, preview, content_type, origin, final_status, - status_message, download_path, + status_message, download_path, retry_payload, queued_at, terminal_at ) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'active', NULL, NULL, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'active', NULL, NULL, ?, ?, ?) ON CONFLICT(task_id) DO UPDATE SET final_status = 'active', status_message = NULL, download_path = NULL, + retry_payload = excluded.retry_payload, terminal_at = ? """, ( @@ -216,6 +292,7 @@ class DownloadHistoryService: normalize_optional_text(preview), normalize_optional_text(content_type), normalized_origin, + normalized_retry_payload, recorded_at, recorded_at, recorded_at, @@ -232,12 +309,14 @@ class DownloadHistoryService: final_status: str, status_message: str | None = None, download_path: str | None = None, + retry_payload: dict[str, Any] | None = None, ) -> None: """Update an existing download row to its terminal state.""" normalized_task_id = _normalize_task_id(task_id) normalized_final_status = _normalize_final_status(final_status) normalized_status_message = normalize_optional_text(status_message) normalized_download_path = normalize_optional_text(download_path) + normalized_retry_payload = self._serialize_retry_payload(retry_payload) effective_terminal_at = now_utc_iso() with self._lock: @@ -249,6 +328,7 @@ class DownloadHistoryService: SET final_status = ?, status_message = ?, download_path = ?, + retry_payload = COALESCE(?, retry_payload), terminal_at = ? WHERE task_id = ? AND final_status = 'active' """, @@ -256,6 +336,7 @@ class DownloadHistoryService: normalized_final_status, normalized_status_message, normalized_download_path, + normalized_retry_payload, effective_terminal_at, normalized_task_id, ), @@ -301,6 +382,11 @@ class DownloadHistoryService: conn = self._connect() try: rows = conn.execute(query, params).fetchall() - return [dict(row) for row in rows] + result: list[dict[str, Any]] = [] + for row in rows: + normalized = self._normalize_row_dict(dict(row)) + if normalized is not None: + result.append(normalized) + return result finally: conn.close() diff --git a/shelfmark/core/models.py b/shelfmark/core/models.py index 53be667..fe97549 100644 --- a/shelfmark/core/models.py +++ b/shelfmark/core/models.py @@ -84,6 +84,13 @@ class DownloadTask: preview: Optional[str] = None content_type: Optional[str] = None # "book (fiction)", "audiobook", "magazine", etc. source_url: Optional[str] = None # Original release URL used by source-specific handlers + retry_download_url: Optional[str] = None # Resolved download URL for restart-safe retries + retry_download_protocol: Optional[str] = None # Protocol for retry_download_url (e.g. torrent, usenet) + retry_release_name: Optional[str] = None # Display name to send back to external download clients + retry_expected_hash: Optional[str] = None # Optional torrent hash used to match client downloads + retry_ratio_limit: Optional[float] = None # Optional post-download seeding ratio + retry_seeding_time_limit_minutes: Optional[int] = None # Optional post-download seeding time limit + can_retry_without_staged_source: bool = True # Whether the source can restart without a preserved staged file # Series info (for library naming templates) series_name: Optional[str] = None diff --git a/shelfmark/core/oidc_routes.py b/shelfmark/core/oidc_routes.py index 62b8bd6..f9b4d37 100644 --- a/shelfmark/core/oidc_routes.py +++ b/shelfmark/core/oidc_routes.py @@ -11,13 +11,13 @@ from authlib.jose.errors import InvalidClaimError from authlib.integrations.flask_client import OAuth from flask import Flask, jsonify, redirect, request, session +from shelfmark.core.config import config as app_config from shelfmark.core.logger import setup_logger from shelfmark.core.oidc_auth import ( extract_user_info, parse_group_claims, provision_oidc_user, ) -from shelfmark.core.settings_registry import load_config_file from shelfmark.core.user_db import UserDB from shelfmark.download.network import get_ssl_verify @@ -118,14 +118,13 @@ def _post_login_redirect_target(return_to: str | None) -> str: def _get_oidc_client() -> tuple[Any, dict[str, Any]]: """Register and return an OIDC client from the current security config.""" - config = load_config_file("security") - discovery_url = config.get("OIDC_DISCOVERY_URL", "") - client_id = config.get("OIDC_CLIENT_ID", "") + discovery_url = str(app_config.get("OIDC_DISCOVERY_URL", "") or "") + client_id = str(app_config.get("OIDC_CLIENT_ID", "") or "") if not discovery_url or not client_id: raise ValueError("OIDC not configured") - configured_scopes = config.get("OIDC_SCOPES", ["openid", "email", "profile"]) + configured_scopes = app_config.get("OIDC_SCOPES", ["openid", "email", "profile"]) if isinstance(configured_scopes, list): scope_values = [str(scope).strip() for scope in configured_scopes if str(scope).strip()] elif isinstance(configured_scopes, str): @@ -136,9 +135,9 @@ def _get_oidc_client() -> tuple[Any, dict[str, Any]]: scopes = list(dict.fromkeys(["openid"] + scope_values)) - admin_group = config.get("OIDC_ADMIN_GROUP", "") - group_claim = config.get("OIDC_GROUP_CLAIM", "groups") - use_admin_group = config.get("OIDC_USE_ADMIN_GROUP", True) + admin_group = app_config.get("OIDC_ADMIN_GROUP", "") + group_claim = app_config.get("OIDC_GROUP_CLAIM", "groups") + use_admin_group = app_config.get("OIDC_USE_ADMIN_GROUP", True) if admin_group and use_admin_group and group_claim and group_claim not in scopes: scopes.append(group_claim) @@ -151,7 +150,7 @@ def _get_oidc_client() -> tuple[Any, dict[str, Any]]: oauth.register( name="shelfmark_idp", client_id=client_id, - client_secret=config.get("OIDC_CLIENT_SECRET", ""), + client_secret=app_config.get("OIDC_CLIENT_SECRET", ""), server_metadata_url=discovery_url, client_kwargs={ "scope": " ".join(scopes), @@ -165,7 +164,13 @@ def _get_oidc_client() -> tuple[Any, dict[str, Any]]: if client is None: raise RuntimeError("OIDC client initialization failed") - return client, config + return client, { + "OIDC_DISCOVERY_URL": discovery_url, + "OIDC_GROUP_CLAIM": group_claim, + "OIDC_ADMIN_GROUP": admin_group, + "OIDC_AUTO_PROVISION": app_config.get("OIDC_AUTO_PROVISION", True), + "OIDC_USE_ADMIN_GROUP": use_admin_group, + } def register_oidc_routes(app: Flask, user_db: UserDB) -> None: diff --git a/shelfmark/core/request_helpers.py b/shelfmark/core/request_helpers.py index 2fe9765..08c0d8e 100644 --- a/shelfmark/core/request_helpers.py +++ b/shelfmark/core/request_helpers.py @@ -5,8 +5,8 @@ from __future__ import annotations from datetime import datetime, timezone from typing import Any +from shelfmark.core.config import config as app_config from shelfmark.core.logger import setup_logger -from shelfmark.core.settings_registry import load_config_file _logger = setup_logger(__name__) @@ -38,7 +38,12 @@ def emit_ws_event( def load_users_request_policy_settings() -> dict[str, Any]: """Load global request-policy settings from the users config file.""" - return load_config_file("users") + from shelfmark.core.request_policy import REQUEST_POLICY_KEYS + + return { + key: app_config.get(key) + for key in REQUEST_POLICY_KEYS + } def coerce_bool(value: Any, default: bool = False) -> bool: diff --git a/shelfmark/core/requests_service.py b/shelfmark/core/requests_service.py index 6eea1ac..528dd31 100644 --- a/shelfmark/core/requests_service.py +++ b/shelfmark/core/requests_service.py @@ -198,6 +198,7 @@ def sync_delivery_states_from_queue_status( unique_request_ids_by_source.pop(source_id, None) request_delivery_states: dict[int, str] = {} + request_delivery_payloads: dict[int, dict[str, Any]] = {} for status_key in QueueStatus: status_bucket = queue_status.get(status_key) if not isinstance(status_bucket, dict): @@ -211,22 +212,42 @@ def sync_delivery_states_from_queue_status( if request_id is None: continue request_delivery_states[request_id] = status_key + if isinstance(task_payload, dict): + request_delivery_payloads[request_id] = dict(task_payload) if not request_delivery_states: return [] updated: list[dict[str, Any]] = [] for row in fulfilled_rows: - delivery_state = request_delivery_states.get(int(row["id"])) + request_id = int(row["id"]) + delivery_state = request_delivery_states.get(request_id) if delivery_state is None: continue + task_payload = request_delivery_payloads.get(request_id) or {} + retry_available = task_payload.get("retry_available") + if delivery_state == QueueStatus.ERROR and retry_available is False: + raw_status_message = task_payload.get("status_message") + failure_reason = ( + raw_status_message.strip() + if isinstance(raw_status_message, str) and raw_status_message.strip() + else "Download failed" + ) + reopened = user_db.reopen_failed_request( + request_id, + failure_reason=failure_reason, + ) + if reopened is not None: + updated.append(reopened) + continue + if row.get("delivery_state", DELIVERY_STATE_NONE) == delivery_state: continue updated.append( user_db.update_request( - row["id"], + request_id, delivery_state=delivery_state, delivery_updated_at=_now_timestamp(), ) diff --git a/shelfmark/core/user_db.py b/shelfmark/core/user_db.py index 0cf83c3..5c59650 100644 --- a/shelfmark/core/user_db.py +++ b/shelfmark/core/user_db.py @@ -85,6 +85,7 @@ CREATE TABLE IF NOT EXISTS download_history ( final_status TEXT NOT NULL, status_message TEXT, download_path TEXT, + retry_payload TEXT, queued_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, terminal_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ); @@ -190,6 +191,7 @@ class UserDB: self._migrate_auth_source_column(conn) self._migrate_request_delivery_columns(conn) self._migrate_download_history_queued_at(conn) + self._migrate_download_history_retry_payload(conn) conn.commit() # WAL mode must be changed outside an open transaction. conn.execute("PRAGMA journal_mode=WAL") @@ -254,6 +256,13 @@ class UserDB: "UPDATE download_history SET queued_at = CURRENT_TIMESTAMP WHERE queued_at IS NULL" ) + def _migrate_download_history_retry_payload(self, conn: sqlite3.Connection) -> None: + """Ensure download_history.retry_payload exists for restart-safe retries.""" + columns = conn.execute("PRAGMA table_info(download_history)").fetchall() + column_names = {str(col["name"]) for col in columns} + if "retry_payload" not in column_names: + conn.execute("ALTER TABLE download_history ADD COLUMN retry_payload TEXT") + def create_user( self, username: str, diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index 7ec7576..903bb3e 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -18,6 +18,7 @@ from shelfmark.core.config import config from shelfmark.core.logger import setup_logger from shelfmark.core.models import DownloadTask, QueueStatus, SearchMode from shelfmark.core.queue import book_queue +from shelfmark.core.request_helpers import normalize_optional_text, normalize_positive_int from shelfmark.core.utils import transform_cover_url, is_audiobook as check_audiobook from shelfmark.config import env as env_config from shelfmark.download.fs import run_blocking_io @@ -79,6 +80,8 @@ def _resolve_email_destination( return None, "Configured email recipient is invalid" return None, None + + def _parse_release_search_mode(value: Any) -> SearchMode: if isinstance(value, SearchMode): return value @@ -91,6 +94,65 @@ def _parse_release_search_mode(value: Any) -> SearchMode: raise ValueError(f"Invalid search_mode: {value}") from exc raise ValueError(f"Invalid search_mode: {value}") +def _optional_number(value: Any) -> Optional[float]: + if isinstance(value, (int, float)) and not isinstance(value, bool): + return float(value) + try: + return float(value) + except (TypeError, ValueError): + return None + + +def _optional_positive_int(value: Any) -> Optional[int]: + if isinstance(value, bool): + return None + try: + parsed = int(value) + except (TypeError, ValueError): + return None + return parsed if parsed > 0 else None + + +def _seed_time_seconds_to_minutes(value: Any) -> Optional[int]: + seed_time_seconds = _optional_positive_int(value) + if seed_time_seconds is None: + return None + return (seed_time_seconds + 59) // 60 + + +def _build_retry_resolution_fields( + release_data: dict[str, Any], +) -> Dict[str, Any]: + """Persist generic resolved-download data needed for restart-safe retries.""" + extra = release_data.get("extra") + if not isinstance(extra, dict): + extra = {} + + protocol = normalize_optional_text(release_data.get("protocol")) + ratio_limit = _optional_number(release_data.get("ratio_limit")) + if ratio_limit is None: + ratio_limit = _optional_number(extra.get("minimum_ratio")) + + seeding_time_limit_minutes = _optional_positive_int( + release_data.get("seeding_time_limit_minutes") + ) + if seeding_time_limit_minutes is None: + seeding_time_limit_minutes = _seed_time_seconds_to_minutes( + extra.get("minimum_seed_time") + ) + + return { + "retry_download_url": normalize_optional_text(release_data.get("download_url")), + "retry_download_protocol": protocol.lower() if protocol is not None else None, + "retry_release_name": normalize_optional_text(release_data.get("title")), + "retry_expected_hash": normalize_optional_text( + release_data.get("expected_hash") or extra.get("info_hash") + ), + "retry_ratio_limit": ratio_limit, + "retry_seeding_time_limit_minutes": seeding_time_limit_minutes, + "can_retry_without_staged_source": True, + } + def queue_release( release_data: dict, @@ -136,6 +198,7 @@ def queue_release( output_mode = "folder" if is_audiobook else books_output_mode output_args: Dict[str, Any] = {} + retry_resolution_fields = _build_retry_resolution_fields(release_data) if output_mode == "email" and not is_audiobook: email_to, email_error = _resolve_email_destination(user_id=user_id) @@ -166,6 +229,7 @@ def queue_release( user_id=user_id, username=username, request_id=request_id, + **retry_resolution_fields, ) if not book_queue.add(task): @@ -204,7 +268,7 @@ def queue_status(user_id: Optional[int] = None) -> Dict[str, Dict[str, Any]]: # Convert Enum keys to strings and DownloadTask objects to dicts for JSON serialization return { status_type.value: { - task_id: _task_to_dict(task) + task_id: _task_to_dict(task, current_status=status_type) for task_id, task in tasks.items() } for status_type, tasks in status.items() @@ -230,10 +294,202 @@ def get_book_data(task_id: str) -> Tuple[Optional[bytes], Optional[DownloadTask] task.download_path = None return None, task -def _task_to_dict(task: DownloadTask) -> Dict[str, Any]: +def _has_staged_retry_source(task: DownloadTask) -> bool: + """Whether a failed task still has a staged file available for retry.""" + staged_path = task.staged_path.strip() if isinstance(task.staged_path, str) else "" + if not staged_path: + return False + try: + return run_blocking_io(Path(staged_path).exists) + except OSError: + return False + + +def _has_fresh_retry_context(task: DownloadTask) -> bool: + """Whether the task can restart without relying on a staged file.""" + return bool(getattr(task, "can_retry_without_staged_source", True)) + + +def can_retry_download_task( + task: Optional[DownloadTask], + status: Optional[QueueStatus], +) -> bool: + """Whether the task can be manually retried from the Activity UI.""" + if task is None or status not in (QueueStatus.ERROR, QueueStatus.CANCELLED): + return False + + if task.request_id is None: + return _has_staged_retry_source(task) or _has_fresh_retry_context(task) + + if status == QueueStatus.CANCELLED: + return _has_fresh_retry_context(task) + + return _has_staged_retry_source(task) + + +def serialize_task_for_retry(task: DownloadTask) -> Dict[str, Any]: + """Serialize the task state needed for restart-safe retries.""" + raw_search_mode = getattr(task, "search_mode", None) + search_mode: Optional[str] = None + if isinstance(raw_search_mode, SearchMode): + search_mode = raw_search_mode.value + elif isinstance(raw_search_mode, str): + normalized_search_mode = raw_search_mode.strip().lower() + search_mode = normalized_search_mode or None + + raw_output_args = getattr(task, "output_args", None) + + return { + "task_id": getattr(task, "task_id", None), + "source": getattr(task, "source", None), + "title": getattr(task, "title", None), + "author": getattr(task, "author", None), + "year": getattr(task, "year", None), + "format": getattr(task, "format", None), + "size": getattr(task, "size", None), + "preview": getattr(task, "preview", None), + "content_type": getattr(task, "content_type", None), + "source_url": getattr(task, "source_url", None), + "series_name": getattr(task, "series_name", None), + "series_position": getattr(task, "series_position", None), + "subtitle": getattr(task, "subtitle", None), + "search_mode": search_mode, + "output_mode": getattr(task, "output_mode", None), + "output_args": dict(raw_output_args) if isinstance(raw_output_args, dict) else {}, + "user_id": getattr(task, "user_id", None), + "username": getattr(task, "username", None), + "request_id": getattr(task, "request_id", None), + "staged_path": getattr(task, "staged_path", None), + "retry_download_url": getattr(task, "retry_download_url", None), + "retry_download_protocol": getattr(task, "retry_download_protocol", None), + "retry_release_name": getattr(task, "retry_release_name", None), + "retry_expected_hash": getattr(task, "retry_expected_hash", None), + "retry_ratio_limit": getattr(task, "retry_ratio_limit", None), + "retry_seeding_time_limit_minutes": getattr(task, "retry_seeding_time_limit_minutes", None), + "can_retry_without_staged_source": bool( + getattr(task, "can_retry_without_staged_source", True) + ), + } + + +def _restore_task_from_retry_payload(payload: Any) -> Optional[DownloadTask]: + if not isinstance(payload, dict): + return None + + task_id = normalize_optional_text(payload.get("task_id")) + source = normalize_optional_text(payload.get("source")) + title = normalize_optional_text(payload.get("title")) + if task_id is None or source is None or title is None: + return None + + search_mode = None + raw_search_mode = payload.get("search_mode") + if raw_search_mode is not None: + try: + search_mode = _parse_release_search_mode(raw_search_mode) + except ValueError: + search_mode = None + + output_args = payload.get("output_args") + + return DownloadTask( + task_id=task_id, + source=source, + title=title, + author=normalize_optional_text(payload.get("author")), + year=normalize_optional_text(payload.get("year")), + format=normalize_optional_text(payload.get("format")), + size=normalize_optional_text(payload.get("size")), + preview=normalize_optional_text(payload.get("preview")), + content_type=normalize_optional_text(payload.get("content_type")), + source_url=normalize_optional_text(payload.get("source_url")), + series_name=normalize_optional_text(payload.get("series_name")), + series_position=_optional_number(payload.get("series_position")), + subtitle=normalize_optional_text(payload.get("subtitle")), + search_mode=search_mode, + output_mode=normalize_optional_text(payload.get("output_mode")), + output_args=dict(output_args) if isinstance(output_args, dict) else {}, + user_id=normalize_positive_int(payload.get("user_id")), + username=normalize_optional_text(payload.get("username")), + request_id=normalize_positive_int(payload.get("request_id")), + staged_path=normalize_optional_text(payload.get("staged_path")), + retry_download_url=normalize_optional_text(payload.get("retry_download_url")), + retry_download_protocol=normalize_optional_text(payload.get("retry_download_protocol")), + retry_release_name=normalize_optional_text(payload.get("retry_release_name")), + retry_expected_hash=normalize_optional_text(payload.get("retry_expected_hash")), + retry_ratio_limit=_optional_number(payload.get("retry_ratio_limit")), + retry_seeding_time_limit_minutes=_optional_positive_int( + payload.get("retry_seeding_time_limit_minutes") + ), + can_retry_without_staged_source=bool( + payload.get("can_retry_without_staged_source", True) + ), + ) + + +def retry_persisted_download( + payload: Any, + *, + final_status: Any, + priority: int = -10, +) -> Tuple[bool, Optional[str]]: + """Retry a persisted download row after the in-memory task has been lost.""" + task = _restore_task_from_retry_payload(payload) + if task is None: + return False, "Download cannot be retried" + + normalized_status = normalize_optional_text(final_status) + if normalized_status is None: + return False, "Download cannot be retried" + normalized_status = normalized_status.lower() + if normalized_status not in {"active", "error", "cancelled"}: + return False, "Download cannot be retried" + + has_staged_retry_source = _has_staged_retry_source(task) + has_fresh_retry_context = _has_fresh_retry_context(task) + + if normalized_status in {"active", "cancelled"} and not has_fresh_retry_context: + return False, "Download cannot be retried" + + if ( + task.request_id is not None + and normalized_status == "error" + and not has_staged_retry_source + ): + if task.request_id is not None: + return False, "Request-linked downloads must be retried from requests" + + if ( + task.request_id is None + and normalized_status == "error" + and not has_staged_retry_source + and not has_fresh_retry_context + ): + return False, "Download cannot be retried" + + task.priority = priority + task.status_message = None + _clear_task_error_state(task) + + if not book_queue.add(task): + return False, "Failed to requeue download" + + book_queue.update_status_message(task.task_id, "Retrying now") + + if ws_manager: + ws_manager.broadcast_status_update(queue_status()) + + return True, None + + +def _task_to_dict( + task: DownloadTask, + current_status: Optional[QueueStatus] = None, +) -> Dict[str, Any]: """Convert DownloadTask to dict for frontend, transforming cover URLs.""" # Transform external preview URLs to local proxy URLs preview = transform_cover_url(task.preview, task.task_id) + retry_status = current_status or book_queue.get_task_status(task.task_id) return { 'id': task.task_id, @@ -254,6 +510,7 @@ def _task_to_dict(task: DownloadTask) -> Dict[str, Any]: 'user_id': task.user_id, 'username': task.username, 'request_id': task.request_id, + 'retry_available': can_retry_download_task(task, retry_status), } @@ -505,8 +762,8 @@ def cancel_download(book_id: str) -> bool: def retry_download(book_id: str) -> Tuple[bool, Optional[str]]: """Retry a failed or cancelled download. - Request-linked downloads can only be retried when cancelled (errors - reopen the request for admin re-approval instead). + Request-linked downloads can only be manually retried when cancelled or + when a staged post-processing retry is available. """ task = book_queue.get_task(book_id) if task is None: @@ -516,7 +773,7 @@ def retry_download(book_id: str) -> Tuple[bool, Optional[str]]: if status not in (QueueStatus.ERROR, QueueStatus.CANCELLED): return False, "Download is not in an error or cancelled state" - if task.request_id and status != QueueStatus.CANCELLED: + if not can_retry_download_task(task, status): return False, "Request-linked downloads must be retried from requests" task.last_error_message = None diff --git a/shelfmark/main.py b/shelfmark/main.py index 348c3fa..92e3a21 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -549,8 +549,6 @@ def proxy_auth_middleware(): if request.path == '/api/health': return None - from shelfmark.core.settings_registry import load_config_file - def get_proxy_header(header_name: str) -> str | None: """Resolve proxy auth values from headers with WSGI env fallbacks.""" value = request.headers.get(header_name) @@ -569,8 +567,7 @@ def proxy_auth_middleware(): return None try: - security_config = load_config_file("security") - user_header = security_config.get("PROXY_AUTH_USER_HEADER", "X-Auth-User") + user_header = app_config.get("PROXY_AUTH_USER_HEADER", "X-Auth-User") # Extract username from proxy header username = get_proxy_header(user_header) @@ -586,8 +583,8 @@ def proxy_auth_middleware(): # If an admin group is configured, derive from groups header. # Otherwise preserve existing DB role for known users and default # first-time users to admin (to avoid lockouts). - admin_group_header = security_config.get("PROXY_AUTH_ADMIN_GROUP_HEADER", "X-Auth-Groups") - admin_group_name = str(security_config.get("PROXY_AUTH_ADMIN_GROUP_NAME", "") or "").strip() + admin_group_header = app_config.get("PROXY_AUTH_ADMIN_GROUP_HEADER", "X-Auth-Groups") + admin_group_name = str(app_config.get("PROXY_AUTH_ADMIN_GROUP_NAME", "") or "").strip() is_admin = True if admin_group_name: @@ -685,12 +682,9 @@ def login_required(f): # Check admin access for settings/onboarding endpoints. if is_settings_or_onboarding_path(request.path): - from shelfmark.core.settings_registry import load_config_file - try: - users_config = load_config_file("users") if ( - requires_admin_for_settings_access(request.path, users_config) + requires_admin_for_settings_access(request.path, {}) and not session.get('is_admin', False) ): return jsonify({"error": "Admin access required"}), 403 @@ -1185,6 +1179,7 @@ def _record_download_queued(task_id: str, task: Any) -> None: preview=normalize_optional_text(getattr(task, "preview", None)), content_type=normalize_optional_text(getattr(task, "content_type", None)), origin=origin, + retry_payload=backend.serialize_task_for_retry(task), ) except Exception as exc: logger.warning("Failed to record download at queue time for task %s: %s", task_id, exc) @@ -1231,6 +1226,7 @@ def _record_download_terminal_snapshot(task_id: str, status: QueueStatus, task: final_status=final_status, status_message=normalize_optional_text(getattr(task, "status_message", None)), download_path=normalize_optional_text(getattr(task, "download_path", None)), + retry_payload=backend.serialize_task_for_retry(task), ) finalized_download = True except Exception as exc: @@ -1252,6 +1248,8 @@ def _record_download_terminal_snapshot(task_id: str, status: QueueStatus, task: request_id = normalize_positive_int(getattr(task, "request_id", None)) if request_id is None: return + if backend.can_retry_download_task(task, status): + return raw_error_message = getattr(task, "status_message", None) fallback_reason = ( @@ -1298,6 +1296,23 @@ def _task_owned_by_actor(task: Any, *, actor_user_id: int | None, actor_username return False +def _download_row_owned_by_actor( + row: dict[str, Any], + *, + actor_user_id: int | None, + actor_username: str | None, +) -> bool: + owner_user_id = normalize_positive_int(row.get("user_id")) + if actor_user_id is not None and owner_user_id is not None: + return owner_user_id == actor_user_id + + row_username = normalize_optional_text(row.get("username")) + if row_username is not None and isinstance(actor_username, str): + return row_username == actor_username.strip() + + return False + + backend.book_queue.set_queue_hook(_record_download_queued) backend.book_queue.set_terminal_status_hook(_record_download_terminal_snapshot) @@ -1527,28 +1542,56 @@ def api_retry_download(book_id: str) -> Union[Response, Tuple[Response, int]]: """Retry a failed download.""" try: task = backend.book_queue.get_task(book_id) - if task is None: + history_row = None + if task is None and download_history_service is not None: + history_row = download_history_service.get_by_task_id(book_id) + if task is None and history_row is None: return jsonify({"error": "Download not found"}), 404 is_admin, db_user_id, can_access_status = _resolve_status_scope() + actor_username = session.get("user_id") + normalized_actor_username = actor_username if isinstance(actor_username, str) else None if not is_admin: if not can_access_status or db_user_id is None: return jsonify({"error": "User identity unavailable", "code": "user_identity_unavailable"}), 403 - actor_username = session.get("user_id") - normalized_actor_username = actor_username if isinstance(actor_username, str) else None - if not _task_owned_by_actor( - task, + if task is not None: + if not _task_owned_by_actor( + task, + actor_user_id=db_user_id, + actor_username=normalized_actor_username, + ): + return jsonify({"error": "Forbidden", "code": "download_not_owned"}), 403 + elif history_row is None or not _download_row_owned_by_actor( + history_row, actor_user_id=db_user_id, actor_username=normalized_actor_username, ): return jsonify({"error": "Forbidden", "code": "download_not_owned"}), 403 - task_status = backend.book_queue.get_task_status(book_id) - if getattr(task, "request_id", None) is not None and task_status != QueueStatus.CANCELLED: - return jsonify({"error": "Forbidden", "code": "requested_download_retry_forbidden"}), 403 + if task is not None: + task_status = backend.book_queue.get_task_status(book_id) + if ( + getattr(task, "request_id", None) is not None + and not backend.can_retry_download_task(task, task_status) + ): + return jsonify({"error": "Forbidden", "code": "requested_download_retry_forbidden"}), 403 + success, error = backend.retry_download(book_id) + else: + assert history_row is not None + request_id = normalize_positive_int(history_row.get("request_id")) + retry_payload = history_row.get("retry_payload") + final_status = history_row.get("final_status") + if ( + request_id is not None + and not download_history_service.is_retry_available(history_row) + ): + return jsonify({"error": "Forbidden", "code": "requested_download_retry_forbidden"}), 403 + success, error = backend.retry_persisted_download( + retry_payload, + final_status=final_status, + ) - success, error = backend.retry_download(book_id) if success: return jsonify({"status": "queued", "book_id": book_id}) @@ -1857,8 +1900,6 @@ def api_logout() -> Union[Response, Tuple[Response, int]]: Returns: flask.Response: JSON with success status and optional logout_url. """ - from shelfmark.core.settings_registry import load_config_file - try: auth_mode = get_auth_mode() ip_address = get_client_ip() @@ -1868,8 +1909,7 @@ def api_logout() -> Union[Response, Tuple[Response, int]]: # For proxy auth, include logout URL if configured if auth_mode == "proxy": - security_config = load_config_file("security") - logout_url = security_config.get("PROXY_AUTH_LOGOUT_URL", "") + logout_url = app_config.get("PROXY_AUTH_LOGOUT_URL", "") if logout_url: return jsonify({"success": True, "logout_url": logout_url}) @@ -1887,11 +1927,7 @@ def api_auth_check() -> Union[Response, Tuple[Response, int]]: flask.Response: JSON with authentication status, whether auth is required, which auth mode is active, and whether user has admin privileges. """ - from shelfmark.core.settings_registry import load_config_file - try: - security_config = load_config_file("security") - users_config = load_config_file("users") auth_mode = get_auth_mode() # If no authentication is configured, access is allowed (full admin) @@ -1906,7 +1942,7 @@ def api_auth_check() -> Union[Response, Tuple[Response, int]]: # Check if user has a valid session is_authenticated = 'user_id' in session - is_admin = get_auth_check_admin_status(auth_mode, users_config, session) + is_admin = get_auth_check_admin_status(auth_mode, {}, session) display_name = None if is_authenticated and session.get('db_user_id') and user_db is not None: @@ -1927,14 +1963,14 @@ def api_auth_check() -> Union[Response, Tuple[Response, int]]: } # Add logout URL for proxy auth if configured - if auth_mode == "proxy" and security_config.get("PROXY_AUTH_USER_HEADER"): - logout_url = security_config.get("PROXY_AUTH_LOGOUT_URL", "") + if auth_mode == "proxy" and app_config.get("PROXY_AUTH_USER_HEADER", ""): + logout_url = app_config.get("PROXY_AUTH_LOGOUT_URL", "") if logout_url: response_data["logout_url"] = logout_url # Add custom OIDC button label and SSO enforcement flags if configured if auth_mode == "oidc": - oidc_button_label = security_config.get("OIDC_BUTTON_LABEL", "") + oidc_button_label = app_config.get("OIDC_BUTTON_LABEL", "") if oidc_button_label: response_data["oidc_button_label"] = oidc_button_label if HIDE_LOCAL_AUTH: diff --git a/shelfmark/release_sources/prowlarr/handler.py b/shelfmark/release_sources/prowlarr/handler.py index f40d779..e42468b 100644 --- a/shelfmark/release_sources/prowlarr/handler.py +++ b/shelfmark/release_sources/prowlarr/handler.py @@ -5,6 +5,7 @@ from typing import Callable, Optional from shelfmark.core.config import config # noqa: F401 (compat patch target in tests) from shelfmark.core.logger import setup_logger from shelfmark.core.models import DownloadTask +from shelfmark.core.request_helpers import normalize_optional_text from shelfmark.download.clients import DownloadClient, get_client, list_configured_clients from shelfmark.download.clients.base_handler import ( COMPLETED_PATH_MAX_ATTEMPTS as _DEFAULT_COMPLETED_PATH_MAX_ATTEMPTS, @@ -65,6 +66,41 @@ class ProwlarrHandler(ExternalClientHandler): def _completed_path_max_attempts(self) -> int: return COMPLETED_PATH_MAX_ATTEMPTS + @classmethod + def _restore_download_request_from_task(cls, task: DownloadTask) -> Optional[DownloadRequest]: + """Rebuild a DownloadRequest when the in-memory Prowlarr cache is gone.""" + retry_download_url = normalize_optional_text(getattr(task, "retry_download_url", None)) + retry_download_protocol = normalize_optional_text( + getattr(task, "retry_download_protocol", None) + ) + if retry_download_url is None or retry_download_protocol is None: + return None + + protocol = retry_download_protocol.lower() + if protocol not in {"torrent", "usenet"}: + return None + + ratio_limit = getattr(task, "retry_ratio_limit", None) + if not isinstance(ratio_limit, (int, float)) or isinstance(ratio_limit, bool): + ratio_limit = None + + seeding_time_limit = getattr(task, "retry_seeding_time_limit_minutes", None) + if not isinstance(seeding_time_limit, int) or isinstance(seeding_time_limit, bool): + seeding_time_limit = None + + return DownloadRequest( + url=retry_download_url, + protocol=protocol, + release_name=( + normalize_optional_text(getattr(task, "retry_release_name", None)) + or task.title + or "Unknown" + ), + expected_hash=normalize_optional_text(getattr(task, "retry_expected_hash", None)), + seeding_time_limit=seeding_time_limit, + ratio_limit=float(ratio_limit) if ratio_limit is not None else None, + ) + def _resolve_download( self, task: DownloadTask, @@ -74,9 +110,13 @@ class ProwlarrHandler(ExternalClientHandler): # Look up the cached release prowlarr_result = get_release(task.task_id) if not prowlarr_result: - logger.warning(f"Release cache miss: {task.task_id}") - status_callback("error", "Release not found in cache (may have expired)") - return None + restored_request = self._restore_download_request_from_task(task) + if restored_request is None: + logger.warning(f"Release cache miss: {task.task_id}") + status_callback("error", "Release not found in cache (may have expired)") + return None + logger.info("Restored Prowlarr download request for retry: %s", task.task_id) + return restored_request # Extract download URL download_url = get_preferred_download_url(prowlarr_result) diff --git a/src/frontend/src/components/ReleaseModal.tsx b/src/frontend/src/components/ReleaseModal.tsx index 35adb1a..0318deb 100644 --- a/src/frontend/src/components/ReleaseModal.tsx +++ b/src/frontend/src/components/ReleaseModal.tsx @@ -1336,9 +1336,18 @@ export const ReleaseModal = ({ const getButtonState = useCallback( (release: Release): ButtonStateInfo => { const releaseId = release.source_id; + const mode = getReleaseActionMode(release); // Check error first if (currentStatus.error && currentStatus.error[releaseId]) { - return { text: 'Failed', state: 'error' }; + if (mode === 'request_release') { + return { text: 'Request', state: 'download' }; + } + if (mode === 'blocked' || mode === 'request_book') { + return { text: 'Unavailable', state: 'blocked' }; + } + return currentStatus.error[releaseId].retry_available === true + ? { text: 'Retry', state: 'download' } + : { text: 'Failed', state: 'error' }; } // Check completed if (currentStatus.complete && currentStatus.complete[releaseId]) { @@ -1362,8 +1371,6 @@ export const ReleaseModal = ({ if (currentStatus.queued && currentStatus.queued[releaseId]) { return { text: 'Queued', state: 'queued' }; } - - const mode = getReleaseActionMode(release); if (mode === 'request_release') { return { text: 'Request', state: 'download' }; } diff --git a/src/frontend/src/components/activity/activityCardModel.ts b/src/frontend/src/components/activity/activityCardModel.ts index 4ee0ceb..cb6eee0 100644 --- a/src/frontend/src/components/activity/activityCardModel.ts +++ b/src/frontend/src/components/activity/activityCardModel.ts @@ -152,6 +152,7 @@ const buildRequestNoteLine = (item: ActivityItem): string | undefined => { const buildActions = (item: ActivityItem, isAdmin: boolean): ActivityCardAction[] => { if (item.kind === 'download' && item.downloadBookId) { + const canRetry = item.downloadRetryAvailable === true; if (item.visualStatus === 'queued') { return [{ kind: 'download-remove', bookId: item.downloadBookId }]; } @@ -162,7 +163,7 @@ const buildActions = (item: ActivityItem, isAdmin: boolean): ActivityCardAction[ ) { return [{ kind: 'download-stop', bookId: item.downloadBookId }]; } - if (item.visualStatus === 'error' && !item.requestId) { + if (item.visualStatus === 'error' && canRetry) { return [ { kind: 'download-retry', @@ -175,7 +176,7 @@ const buildActions = (item: ActivityItem, isAdmin: boolean): ActivityCardAction[ }, ]; } - if (item.visualStatus === 'cancelled') { + if (item.visualStatus === 'cancelled' && canRetry) { return [ { kind: 'download-retry', diff --git a/src/frontend/src/components/activity/activityMappers.ts b/src/frontend/src/components/activity/activityMappers.ts index 2e4f489..3ae7c93 100644 --- a/src/frontend/src/components/activity/activityMappers.ts +++ b/src/frontend/src/components/activity/activityMappers.ts @@ -94,6 +94,7 @@ export const downloadToActivityItem = (book: Book, statusKey: DownloadStatusKey) ]); const progress = getDownloadProgress(visualStatus, book.progress); const statusDetail = toOptionalText(book.status_message); + const downloadRetryAvailable = book.retry_available === true; return { id: book.id, @@ -110,6 +111,7 @@ export const downloadToActivityItem = (book: Book, statusKey: DownloadStatusKey) timestamp: toEpochMillis(book.added_time), username: toOptionalText(book.username), downloadBookId: book.id, + downloadRetryAvailable, downloadPath: toOptionalText(book.download_path), sizeRaw: toOptionalText(book.size), requestId, diff --git a/src/frontend/src/components/activity/activityTypes.ts b/src/frontend/src/components/activity/activityTypes.ts index 15408de..f1bda58 100644 --- a/src/frontend/src/components/activity/activityTypes.ts +++ b/src/frontend/src/components/activity/activityTypes.ts @@ -37,6 +37,7 @@ export interface ActivityItem { username?: string; downloadBookId?: string; + downloadRetryAvailable?: boolean; downloadPath?: string; requestId?: number; requestLevel?: 'book' | 'release'; diff --git a/src/frontend/src/hooks/useDownloadTracking.ts b/src/frontend/src/hooks/useDownloadTracking.ts index f1fd2c5..0e8e546 100644 --- a/src/frontend/src/hooks/useDownloadTracking.ts +++ b/src/frontend/src/hooks/useDownloadTracking.ts @@ -36,7 +36,9 @@ export function useDownloadTracking(currentStatus: StatusData): UseDownloadTrack // Get button state for a book in direct mode const getButtonState = useCallback((bookId: string): ButtonStateInfo => { if (currentStatus.error && currentStatus.error[bookId]) { - return { text: 'Failed', state: 'error' }; + return currentStatus.error[bookId].retry_available === true + ? { text: 'Retry', state: 'download' } + : { text: 'Failed', state: 'error' }; } if (currentStatus.complete && currentStatus.complete[bookId]) { return { text: 'Downloaded', state: 'complete' }; @@ -105,7 +107,9 @@ export function useDownloadTracking(currentStatus: StatusData): UseDownloadTrack foundActiveState = true; } else if (currentStatus.error && currentStatus.error[releaseId]) { if (bestState.state === 'download') { - bestState = { text: 'Failed', state: 'error' }; + bestState = currentStatus.error[releaseId].retry_available === true + ? { text: 'Retry', state: 'download' } + : { text: 'Failed', state: 'error' }; } } } diff --git a/src/frontend/src/tests/activityCardModel.node.test.ts b/src/frontend/src/tests/activityCardModel.node.test.ts index 3c78c48..7fa0e1a 100644 --- a/src/frontend/src/tests/activityCardModel.node.test.ts +++ b/src/frontend/src/tests/activityCardModel.node.test.ts @@ -213,4 +213,33 @@ describe('activityCardModel', () => { 42 ); }); + + it('shows retry for request-linked downloads when the backend marks them retryable', () => { + const model = buildActivityCardModel( + makeItem({ + visualStatus: 'error', + statusLabel: 'Failed', + requestId: 42, + downloadRetryAvailable: true, + }), + false + ); + + assert.equal(model.actions.length, 2); + assert.equal(model.actions[0]?.kind, 'download-retry'); + assert.equal(model.actions[1]?.kind, 'download-dismiss'); + }); + + it('does not show retry for error downloads without a live retry path', () => { + const model = buildActivityCardModel( + makeItem({ + visualStatus: 'error', + statusLabel: 'Failed', + }), + false + ); + + assert.equal(model.actions.length, 1); + assert.equal(model.actions[0]?.kind, 'download-dismiss'); + }); }); diff --git a/src/frontend/src/types/index.ts b/src/frontend/src/types/index.ts index 277a287..ffd12a6 100644 --- a/src/frontend/src/types/index.ts +++ b/src/frontend/src/types/index.ts @@ -55,6 +55,7 @@ export interface Book { authors?: string[]; titles_by_language?: Record; username?: string; + retry_available?: boolean; } // Status response types diff --git a/tests/config/test_environment.py b/tests/config/test_environment.py index dcd6a3e..7d78fc8 100644 --- a/tests/config/test_environment.py +++ b/tests/config/test_environment.py @@ -252,6 +252,24 @@ class TestSettingsSystem: assert string_to_bool("0") is False assert string_to_bool("anything_else") is False + def test_request_policy_loader_reads_env_backed_values(self, monkeypatch): + """Request-policy helpers should read effective values via the config singleton.""" + from shelfmark.core.config import config + from shelfmark.core.request_helpers import load_users_request_policy_settings + + monkeypatch.setenv("REQUESTS_ENABLED", "true") + monkeypatch.setenv("REQUEST_POLICY_DEFAULT_EBOOK", "blocked") + config.refresh(force=True) + + try: + settings = load_users_request_policy_settings() + assert settings["REQUESTS_ENABLED"] is True + assert settings["REQUEST_POLICY_DEFAULT_EBOOK"] == "blocked" + finally: + monkeypatch.delenv("REQUESTS_ENABLED", raising=False) + monkeypatch.delenv("REQUEST_POLICY_DEFAULT_EBOOK", raising=False) + config.refresh(force=True) + # ============================================================================= # Archive Handling Configuration Tests diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index a76dd03..87cf419 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -578,6 +578,57 @@ class TestActivityRoutes: assert history_response.json[0]["final_status"] == "error" assert history_response.json[0]["snapshot"]["download"]["status_message"] == "Interrupted" + def test_dismiss_many_preserves_retry_for_stale_active_requested_download_history(self, main_module, client): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + task_id = "dismiss-many-stale-requested-active" + retry_payload = { + "task_id": task_id, + "source": "prowlarr", + "title": "Interrupted Requested Download", + "user_id": user["id"], + "username": user["username"], + "request_id": 321, + "search_mode": "universal", + "retry_download_url": "magnet:?xt=urn:btih:dismissmany123", + "retry_download_protocol": "torrent", + "retry_release_name": "Interrupted Requested Download", + "can_retry_without_staged_source": True, + } + main_module.download_history_service.record_download( + task_id=task_id, + user_id=user["id"], + username=user["username"], + request_id=321, + source="prowlarr", + source_display_name="Prowlarr", + title="Interrupted Requested Download", + author="Stale Author", + format="epub", + size="1 MB", + preview=None, + content_type="ebook", + origin="requested", + retry_payload=retry_payload, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend, "queue_status", return_value=_sample_status_payload()): + dismiss_many_response = client.post( + "/api/activity/dismiss-many", + json={"items": [{"item_type": "download", "item_key": f"download:{task_id}"}]}, + ) + history_response = client.get("/api/activity/history?limit=10&offset=0") + + assert dismiss_many_response.status_code == 200 + assert dismiss_many_response.json["status"] == "dismissed" + assert history_response.status_code == 200 + assert len(history_response.json) == 1 + assert history_response.json[0]["item_key"] == f"download:{task_id}" + assert history_response.json[0]["snapshot"]["download"]["status_message"] == "Interrupted" + assert history_response.json[0]["snapshot"]["download"]["retry_available"] is True + def test_dismiss_many_returns_404_without_partial_dismiss_when_any_item_is_missing(self, main_module, client): user = _create_user(main_module, prefix="reader") _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) @@ -860,6 +911,149 @@ class TestActivityRoutes: assert "stale-active-task" in response.json["status"]["error"] assert response.json["status"]["error"]["stale-active-task"]["status_message"] == "Interrupted" + def test_snapshot_preserves_retry_for_stale_active_requested_download(self, main_module, client): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + task_id = "stale-active-requested-task" + retry_payload = { + "task_id": task_id, + "source": "prowlarr", + "title": "Stale Active Requested Task", + "user_id": user["id"], + "username": user["username"], + "request_id": 123, + "search_mode": "universal", + "retry_download_url": "magnet:?xt=urn:btih:staleactive123", + "retry_download_protocol": "torrent", + "retry_release_name": "Stale Active Requested Task", + "can_retry_without_staged_source": True, + } + main_module.download_history_service.record_download( + task_id=task_id, + user_id=user["id"], + username=user["username"], + request_id=123, + source="prowlarr", + source_display_name="Prowlarr", + title="Stale Active Requested Task", + author="Stale Author", + format="epub", + size="1 MB", + preview=None, + content_type="ebook", + origin="requested", + retry_payload=retry_payload, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend, "queue_status", return_value=_sample_status_payload()): + response = client.get("/api/activity/snapshot") + + assert response.status_code == 200 + assert response.json["status"]["error"][task_id]["status_message"] == "Interrupted" + assert response.json["status"]["error"][task_id]["retry_available"] is True + + def test_snapshot_includes_retry_available_for_live_terminal_downloads(self, main_module, client): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + _record_terminal_download( + main_module, + task_id="retryable-terminal-task", + user_id=user["id"], + username=user["username"], + title="Retryable Terminal Task", + origin="requested", + request_id=123, + final_status="error", + status_message="Destination not writable", + ) + + queue_status_payload = _sample_status_payload() + queue_status_payload["error"]["retryable-terminal-task"] = { + "retry_available": True, + } + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend, "queue_status", return_value=queue_status_payload): + response = client.get("/api/activity/snapshot") + + assert response.status_code == 200 + assert response.json["status"]["error"]["retryable-terminal-task"]["retry_available"] is True + + def test_snapshot_reopens_request_when_error_retry_is_no_longer_available(self, main_module, client): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + request_row = main_module.user_db.create_request( + user_id=user["id"], + content_type="ebook", + request_level="release", + policy_mode="request_release", + book_data={ + "title": "Retry Gone Request", + "author": "Retry Author", + "provider": "openlibrary", + "provider_id": "retry-gone-1", + }, + release_data={ + "source": "prowlarr", + "source_id": "retry-gone-task", + "title": "Retry Gone.epub", + }, + status="fulfilled", + delivery_state="queued", + ) + retry_payload = { + "task_id": "retry-gone-task", + "source": "prowlarr", + "title": "Retry Gone Request", + "user_id": user["id"], + "username": user["username"], + "request_id": request_row["id"], + "search_mode": "universal", + "retry_download_url": "magnet:?xt=urn:btih:abc123", + "retry_download_protocol": "torrent", + "retry_release_name": "Retry Gone Request", + "can_retry_without_staged_source": True, + } + main_module.download_history_service.record_download( + task_id="retry-gone-task", + user_id=user["id"], + username=user["username"], + request_id=request_row["id"], + source="prowlarr", + source_display_name="Prowlarr", + title="Retry Gone Request", + author="Retry Author", + format="epub", + size="1 MB", + preview=None, + content_type="ebook", + origin="requested", + retry_payload=retry_payload, + ) + main_module.download_history_service.finalize_download( + task_id="retry-gone-task", + final_status="error", + status_message="Output routing failed", + retry_payload=retry_payload, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend, "queue_status", return_value=_sample_status_payload()): + response = client.get("/api/activity/snapshot") + + assert response.status_code == 200 + refreshed_request = main_module.user_db.get_request(request_row["id"]) + assert refreshed_request["status"] == "pending" + assert refreshed_request["last_failure_reason"] == "Output routing failed" + assert any( + row["id"] == request_row["id"] and row["status"] == "pending" + for row in response.json["requests"] + ) + def test_snapshot_active_download_with_queue_entry_shows_in_correct_bucket(self, main_module, client): user = _create_user(main_module, prefix="reader") _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) diff --git a/tests/core/test_activity_terminal_snapshots.py b/tests/core/test_activity_terminal_snapshots.py index 31fb579..f43cfad 100644 --- a/tests/core/test_activity_terminal_snapshots.py +++ b/tests/core/test_activity_terminal_snapshots.py @@ -225,6 +225,64 @@ class TestTerminalSnapshotCapture: finally: main_module.backend.book_queue.cancel_download(task_id) + def test_error_transition_keeps_request_fulfilled_when_postprocess_retry_is_available( + self, + main_module, + tmp_path, + ): + user = _create_user(main_module, prefix="snap-retryable-request") + task_id = f"retryable-request-{uuid.uuid4().hex[:8]}" + staged_file = tmp_path / "retryable-request.epub" + staged_file.write_text("staged") + request_row = main_module.user_db.create_request( + user_id=user["id"], + content_type="ebook", + request_level="release", + policy_mode="request_release", + book_data={ + "title": "Retryable Request", + "author": "Retry Author", + "provider": "openlibrary", + "provider_id": "retryable-request-1", + }, + release_data={ + "source": "prowlarr", + "source_id": task_id, + "title": "Retryable Request.epub", + }, + status="fulfilled", + delivery_state="queued", + ) + task = DownloadTask( + task_id=task_id, + source="prowlarr", + title="Retryable Request", + user_id=user["id"], + username=user["username"], + request_id=request_row["id"], + staged_path=str(staged_file), + ) + assert main_module.backend.book_queue.add(task) is True + + try: + main_module.backend.book_queue.update_status_message(task_id, "Destination not writable") + with patch.object(main_module, "reopen_failed_request") as mock_reopen: + main_module.backend.book_queue.update_status(task_id, QueueStatus.ERROR) + + mock_reopen.assert_not_called() + persisted_request = next( + row for row in main_module.user_db.list_requests(user_id=user["id"]) + if row["id"] == request_row["id"] + ) + assert persisted_request["status"] == "fulfilled" + assert persisted_request["release_data"] is not None + + history_row = _read_download_history_row(main_module, task_id) + assert history_row is not None + assert history_row["final_status"] == "error" + finally: + main_module.backend.book_queue.cancel_download(task_id) + def test_queue_hook_records_active_row_at_queue_time(self, main_module): user = _create_user(main_module, prefix="snap-queue") task_id = f"queue-{uuid.uuid4().hex[:8]}" diff --git a/tests/core/test_admin_users_api.py b/tests/core/test_admin_users_api.py index 93aa7b5..e1ce34c 100644 --- a/tests/core/test_admin_users_api.py +++ b/tests/core/test_admin_users_api.py @@ -152,8 +152,10 @@ class TestAdminUsersListEndpoint: ) with patch( - "shelfmark.core.admin_routes.load_config_file", - return_value={"OIDC_USE_ADMIN_GROUP": False}, + "shelfmark.core.admin_routes.app_config.get", + side_effect=lambda key, default=None, user_id=None: { + "OIDC_USE_ADMIN_GROUP": False, + }.get(key, default), ): resp = admin_client.get("/api/admin/users") @@ -981,9 +983,11 @@ class TestAdminDownloadDefaults: """Create a temporary downloads config file.""" import json from pathlib import Path + from shelfmark.core.config import config as app_config config_dir = str(tmp_path) monkeypatch.setenv("CONFIG_DIR", config_dir) + monkeypatch.delenv("INGEST_DIR", raising=False) monkeypatch.setattr("shelfmark.config.env.CONFIG_DIR", Path(config_dir)) plugins_dir = tmp_path / "plugins" plugins_dir.mkdir() @@ -996,6 +1000,9 @@ class TestAdminDownloadDefaults: "EMAIL_RECIPIENT": "reader@example.com", } (plugins_dir / "downloads.json").write_text(json.dumps(config)) + app_config.refresh(force=True) + yield + app_config.refresh(force=True) def test_returns_download_defaults(self, admin_client): resp = admin_client.get("/api/admin/download-defaults") @@ -1097,7 +1104,7 @@ class TestAdminDeliveryPreferences: (plugins_dir / "downloads.json").write_text(json.dumps(downloads_config)) from shelfmark.core.config import config as app_config - app_config.refresh() + app_config.refresh(force=True) def test_returns_curated_fields_and_effective_values(self, admin_client, user_db): user = user_db.create_user(username="alice") @@ -1181,7 +1188,7 @@ class TestAdminSearchPreferences: (plugins_dir / "search_mode.json").write_text(json.dumps(search_mode_config)) from shelfmark.core.config import config as app_config - app_config.refresh() + app_config.refresh(force=True) def test_returns_curated_fields_and_effective_values(self, admin_client, user_db): user = user_db.create_user(username="alice") @@ -1268,7 +1275,7 @@ class TestAdminNotificationPreferences: (plugins_dir / "notifications.json").write_text(json.dumps(notifications_config)) from shelfmark.core.config import config as app_config - app_config.refresh() + app_config.refresh(force=True) def test_returns_curated_fields_and_effective_values(self, admin_client, user_db): user = user_db.create_user(username="alice") @@ -1341,7 +1348,7 @@ class TestAdminNotificationPreferencesTestAction: (plugins_dir / "notifications.json").write_text(json.dumps(notifications_config)) from shelfmark.core.config import config as app_config - app_config.refresh() + app_config.refresh(force=True) def test_requires_admin(self, regular_client, user_db): user = user_db.create_user(username="alice") @@ -1500,7 +1507,7 @@ class TestAdminEffectiveSettings: # Ensure config singleton sees the current test env/config dir. from shelfmark.core.config import config as app_config - app_config.refresh() + app_config.refresh(force=True) def test_returns_effective_values_with_sources(self, admin_client, user_db): user = user_db.create_user(username="alice") diff --git a/tests/core/test_download_api_guardrails.py b/tests/core/test_download_api_guardrails.py index e0ccf65..debb30c 100644 --- a/tests/core/test_download_api_guardrails.py +++ b/tests/core/test_download_api_guardrails.py @@ -494,6 +494,54 @@ class TestRetryDownloadEndpointGuardrails: assert resp.get_json() == {"status": "queued", "book_id": "direct-task-retry-1"} mock_retry.assert_called_once_with("direct-task-retry-1") + def test_owner_can_retry_persisted_direct_download_when_live_task_is_missing(self, main_module, client): + user = _create_user(main_module, prefix="reader") + _set_authenticated_session( + client, + user_id=user["username"], + db_user_id=user["id"], + is_admin=False, + ) + + retry_payload = { + "task_id": "persisted-direct-retry-1", + "source": "direct_download", + "title": "Persisted Direct Task", + "user_id": user["id"], + "username": user["username"], + "search_mode": "direct", + } + main_module.download_history_service.record_download( + task_id="persisted-direct-retry-1", + user_id=user["id"], + username=user["username"], + request_id=None, + source="direct_download", + source_display_name="Direct Download", + title="Persisted Direct Task", + author="Direct Author", + format="epub", + size="1 MB", + preview=None, + content_type="ebook", + origin="direct", + retry_payload=retry_payload, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend.book_queue, "get_task", return_value=None): + with patch.object( + main_module.backend, + "retry_persisted_download", + return_value=(True, None), + ) as mock_retry: + resp = client.post("/api/download/persisted-direct-retry-1/retry") + + assert resp.status_code == 200 + assert resp.get_json() == {"status": "queued", "book_id": "persisted-direct-retry-1"} + assert mock_retry.call_args.args[0] == retry_payload + assert mock_retry.call_args.kwargs["final_status"] == "active" + def test_non_owner_cannot_retry_download(self, main_module, client): owner = _create_user(main_module, prefix="owner") actor = _create_user(main_module, prefix="actor") @@ -591,6 +639,100 @@ class TestRetryDownloadEndpointGuardrails: assert resp.get_json()["code"] == "requested_download_retry_forbidden" mock_retry.assert_not_called() + def test_retry_allows_request_linked_postprocess_error_with_staged_file(self, main_module, client, tmp_path): + user = _create_user(main_module, prefix="requester") + _set_authenticated_session( + client, + user_id=user["username"], + db_user_id=user["id"], + is_admin=False, + ) + staged_file = tmp_path / "requested-postprocess.epub" + staged_file.write_text("staged") + task = DownloadTask( + task_id="requested-retry-postprocess-1", + source="prowlarr", + title="Requested Book", + user_id=user["id"], + username=user["username"], + request_id=123, + staged_path=str(staged_file), + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend.book_queue, "get_task", return_value=task): + with patch.object( + main_module.backend.book_queue, + "get_task_status", + return_value=main_module.QueueStatus.ERROR, + ): + with patch.object(main_module.backend, "retry_download", return_value=(True, None)) as mock_retry: + resp = client.post("/api/download/requested-retry-postprocess-1/retry") + + assert resp.status_code == 200 + assert resp.get_json() == {"status": "queued", "book_id": "requested-retry-postprocess-1"} + mock_retry.assert_called_once_with("requested-retry-postprocess-1") + + def test_retry_allows_persisted_request_postprocess_error_with_staged_file( + self, main_module, client, tmp_path + ): + user = _create_user(main_module, prefix="requester") + _set_authenticated_session( + client, + user_id=user["username"], + db_user_id=user["id"], + is_admin=False, + ) + + staged_file = tmp_path / "persisted-request-postprocess.epub" + staged_file.write_text("staged") + retry_payload = { + "task_id": "persisted-request-retry-1", + "source": "prowlarr", + "title": "Persisted Requested Book", + "user_id": user["id"], + "username": user["username"], + "request_id": 123, + "search_mode": "universal", + "staged_path": str(staged_file), + } + main_module.download_history_service.record_download( + task_id="persisted-request-retry-1", + user_id=user["id"], + username=user["username"], + request_id=123, + source="prowlarr", + source_display_name="Prowlarr", + title="Persisted Requested Book", + author="Request Author", + format="epub", + size="1 MB", + preview=None, + content_type="ebook", + origin="requested", + retry_payload=retry_payload, + ) + main_module.download_history_service.finalize_download( + task_id="persisted-request-retry-1", + final_status="error", + status_message="Output routing failed", + retry_payload=retry_payload, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object(main_module.backend.book_queue, "get_task", return_value=None): + with patch.object( + main_module.backend, + "retry_persisted_download", + return_value=(True, None), + ) as mock_retry: + resp = client.post("/api/download/persisted-request-retry-1/retry") + + assert resp.status_code == 200 + assert resp.get_json() == {"status": "queued", "book_id": "persisted-request-retry-1"} + assert mock_retry.call_args.args[0] == retry_payload + assert mock_retry.call_args.kwargs["final_status"] == "error" + def test_retry_returns_409_for_non_retryable_state(self, main_module, client): user = _create_user(main_module, prefix="reader") _set_authenticated_session( diff --git a/tests/core/test_oidc_integration.py b/tests/core/test_oidc_integration.py index 6f8fecb..2920fdc 100644 --- a/tests/core/test_oidc_integration.py +++ b/tests/core/test_oidc_integration.py @@ -64,8 +64,11 @@ class TestDetermineAuthMode: assert determine_auth_mode(config, cwa_db_path=None, has_local_admin=False) == "none" def test_load_active_auth_mode_reads_env_backed_cwa_setting(self, monkeypatch, tmp_path): + from shelfmark.core.config import config as app_config + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) monkeypatch.setenv("AUTH_METHOD", "cwa") + app_config.refresh(force=True) cwa_db_path = tmp_path / "app.db" conn = sqlite3.connect(cwa_db_path) @@ -73,7 +76,26 @@ class TestDetermineAuthMode: conn.commit() conn.close() - assert load_active_auth_mode(cwa_db_path) == "cwa" + try: + assert load_active_auth_mode(cwa_db_path) == "cwa" + finally: + monkeypatch.delenv("AUTH_METHOD", raising=False) + app_config.refresh(force=True) + + def test_load_active_auth_mode_reads_env_backed_proxy_setting(self, monkeypatch, tmp_path): + from shelfmark.core.config import config as app_config + + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + monkeypatch.setenv("AUTH_METHOD", "proxy") + monkeypatch.setenv("PROXY_AUTH_USER_HEADER", "X-Forwarded-User") + app_config.refresh(force=True) + + try: + assert load_active_auth_mode(cwa_db_path=None) == "proxy" + finally: + monkeypatch.delenv("AUTH_METHOD", raising=False) + monkeypatch.delenv("PROXY_AUTH_USER_HEADER", raising=False) + app_config.refresh(force=True) class TestSettingsRestrictionPolicy: diff --git a/tests/core/test_oidc_routes.py b/tests/core/test_oidc_routes.py index f5647cb..1cb815f 100644 --- a/tests/core/test_oidc_routes.py +++ b/tests/core/test_oidc_routes.py @@ -21,6 +21,13 @@ def _get_oidc_error(resp) -> str | None: return errors[0] if errors else None +def _config_getter(values: dict[str, object]): + def _get(key: str, default: object = None, user_id: object = None): + return values.get(key, default) + + return _get + + @pytest.fixture def db_path(): with tempfile.TemporaryDirectory() as tmpdir: @@ -64,7 +71,7 @@ def client(app): class TestOIDCClientRegistration: - @patch("shelfmark.core.oidc_routes.load_config_file", return_value=MOCK_OIDC_CONFIG) + @patch("shelfmark.core.oidc_routes.app_config.get", side_effect=_config_getter(MOCK_OIDC_CONFIG)) @patch("shelfmark.core.oidc_routes.oauth.create_client") @patch("shelfmark.core.oidc_routes.oauth.register") def test_registers_client_with_pkce_and_expected_scopes( @@ -78,7 +85,7 @@ class TestOIDCClientRegistration: client_obj, config = _get_oidc_client() assert client_obj is fake_client - assert config["OIDC_CLIENT_ID"] == "shelfmark" + assert config["OIDC_DISCOVERY_URL"] == MOCK_OIDC_CONFIG["OIDC_DISCOVERY_URL"] kwargs = mock_register.call_args.kwargs assert kwargs["name"] == "shelfmark_idp" assert kwargs["server_metadata_url"] == MOCK_OIDC_CONFIG["OIDC_DISCOVERY_URL"] @@ -90,7 +97,7 @@ class TestOIDCClientRegistration: assert "profile" in scope_str assert "groups" in scope_str - @patch("shelfmark.core.oidc_routes.load_config_file") + @patch("shelfmark.core.oidc_routes.app_config.get") @patch("shelfmark.core.oidc_routes.oauth.create_client") @patch("shelfmark.core.oidc_routes.oauth.register") def test_does_not_append_group_claim_when_admin_group_auth_disabled( @@ -104,7 +111,7 @@ class TestOIDCClientRegistration: "OIDC_USE_ADMIN_GROUP": False, "OIDC_GROUP_CLAIM": "groups", } - mock_config.return_value = config + mock_config.side_effect = _config_getter(config) mock_create_client.return_value = Mock() _get_oidc_client() diff --git a/tests/core/test_requests_service.py b/tests/core/test_requests_service.py index a31ee14..132ee94 100644 --- a/tests/core/test_requests_service.py +++ b/tests/core/test_requests_service.py @@ -706,6 +706,43 @@ def test_sync_delivery_states_from_queue_status_uses_request_id_for_duplicate_so assert user_db.get_request(newer_request["id"])["delivery_state"] == "downloading" +def test_sync_delivery_states_reopens_fulfilled_request_when_error_is_not_retryable(user_db): + user = user_db.create_user(username="alice") + fulfilled_request = user_db.create_request( + user_id=user["id"], + source_hint="prowlarr", + content_type="ebook", + request_level="release", + policy_mode="request_release", + book_data=_book_data(), + release_data={"source": "prowlarr", "source_id": "retry-gone-rel", "title": "Retry Gone"}, + status="fulfilled", + delivery_state="queued", + ) + + updated = sync_delivery_states_from_queue_status( + user_db, + queue_status={ + "error": { + "retry-gone-rel": { + "id": "retry-gone-rel", + "request_id": fulfilled_request["id"], + "retry_available": False, + "status_message": "Staged retry source no longer exists", + }, + }, + }, + user_id=user["id"], + ) + + assert [row["id"] for row in updated] == [fulfilled_request["id"]] + refreshed = user_db.get_request(fulfilled_request["id"]) + assert refreshed["status"] == "pending" + assert refreshed["delivery_state"] == "none" + assert refreshed["release_data"] is None + assert refreshed["last_failure_reason"] == "Staged retry source no longer exists" + + # --------------------------------------------------------------------------- # book_data validation # --------------------------------------------------------------------------- diff --git a/tests/download/test_orchestrator_retry.py b/tests/download/test_orchestrator_retry.py index 2af3c8f..c7dfb27 100644 --- a/tests/download/test_orchestrator_retry.py +++ b/tests/download/test_orchestrator_retry.py @@ -62,6 +62,36 @@ def test_retry_download_rejects_request_linked_tasks(monkeypatch): mock_queue.enqueue_existing.assert_not_called() +def test_can_retry_download_task_allows_request_postprocess_retry_when_staged_file_exists(tmp_path): + import shelfmark.download.orchestrator as orchestrator + + staged_file = tmp_path / "requested-staged.epub" + staged_file.write_text("staged") + task = DownloadTask( + task_id="task-request-staged-1", + source="prowlarr", + title="Requested Retryable", + request_id=123, + staged_path=str(staged_file), + ) + + assert orchestrator.can_retry_download_task(task, QueueStatus.ERROR) is True + + +def test_can_retry_download_task_blocks_request_error_retry_without_staged_file(): + import shelfmark.download.orchestrator as orchestrator + + task = DownloadTask( + task_id="task-request-staged-2", + source="prowlarr", + title="Requested Not Retryable", + request_id=123, + staged_path="/tmp/does-not-exist.epub", + ) + + assert orchestrator.can_retry_download_task(task, QueueStatus.ERROR) is False + + def test_finalize_download_failure_sets_terminal_error(monkeypatch): import shelfmark.download.orchestrator as orchestrator diff --git a/tests/download/test_orchestrator_user_output_mode.py b/tests/download/test_orchestrator_user_output_mode.py index ddf609f..bd6233f 100644 --- a/tests/download/test_orchestrator_user_output_mode.py +++ b/tests/download/test_orchestrator_user_output_mode.py @@ -110,3 +110,45 @@ def test_queue_release_email_mode_without_recipient_is_queued(monkeypatch): task = captured["task"] assert task.output_mode == "email" assert task.output_args == {} + + +def test_queue_release_persists_generic_retry_resolution_fields(monkeypatch): + import shelfmark.download.orchestrator as orchestrator + + captured: dict[str, object] = {} + + def fake_add(task): + captured["task"] = task + return True + + monkeypatch.setattr(orchestrator.book_queue, "add", fake_add) + monkeypatch.setattr(orchestrator, "ws_manager", None) + + success, error = orchestrator.queue_release( + { + "source": "prowlarr", + "source_id": "prowlarr-release-1", + "title": "Queued Prowlarr Release", + "download_url": "magnet:?xt=urn:btih:abc123", + "protocol": "torrent", + "indexer": "MyIndexer", + "extra": { + "minimum_ratio": 1.25, + "minimum_seed_time": 5400, + "info_hash": "ABC123", + }, + }, + user_id=42, + username="alice", + ) + + assert success is True + assert error is None + task = captured["task"] + assert task.retry_download_url == "magnet:?xt=urn:btih:abc123" + assert task.retry_download_protocol == "torrent" + assert task.retry_release_name == "Queued Prowlarr Release" + assert task.retry_expected_hash == "ABC123" + assert task.retry_ratio_limit == 1.25 + assert task.retry_seeding_time_limit_minutes == 90 + assert task.can_retry_without_staged_source is True diff --git a/tests/e2e/test_auth_endpoints.py b/tests/e2e/test_auth_endpoints.py index 08e4a9c..36ab48d 100644 --- a/tests/e2e/test_auth_endpoints.py +++ b/tests/e2e/test_auth_endpoints.py @@ -24,6 +24,13 @@ def _as_response(result: Any): return result +def _config_getter(values: dict[str, Any]): + def _get(key: str, default: Any = None, user_id: Any = None): + return values.get(key, default) + + return _get + + @pytest.fixture(scope="module") def main_module(): """Import `shelfmark.main` with background thread startup disabled.""" @@ -37,49 +44,43 @@ def main_module(): class TestGetAuthMode: def test_get_auth_mode_none(self, main_module): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={"AUTH_METHOD": "none"}): + with patch.object(main_module.app_config, "get", side_effect=_config_getter({"AUTH_METHOD": "none"})): assert main_module.get_auth_mode() == "none" def test_get_auth_mode_builtin(self, main_module): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"AUTH_METHOD": "builtin"}, - ): + with patch.object(main_module.app_config, "get", side_effect=_config_getter({"AUTH_METHOD": "builtin"})): with patch("shelfmark.core.auth_modes.has_local_password_admin", return_value=True): assert main_module.get_auth_mode() == "builtin" def test_get_auth_mode_builtin_without_local_admin_falls_back_to_none(self, main_module): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"AUTH_METHOD": "builtin"}, - ): + with patch.object(main_module.app_config, "get", side_effect=_config_getter({"AUTH_METHOD": "builtin"})): with patch("shelfmark.core.auth_modes.has_local_password_admin", return_value=False): assert main_module.get_auth_mode() == "none" def test_get_auth_mode_proxy(self, main_module): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"AUTH_METHOD": "proxy", "PROXY_AUTH_USER_HEADER": "X-Auth-User"}, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"AUTH_METHOD": "proxy", "PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): assert main_module.get_auth_mode() == "proxy" def test_get_auth_mode_cwa(self, main_module): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={"AUTH_METHOD": "cwa"}): + with patch.object(main_module.app_config, "get", side_effect=_config_getter({"AUTH_METHOD": "cwa"})): with patch.object(main_module, "CWA_DB_PATH", object()): assert main_module.get_auth_mode() == "cwa" def test_get_auth_mode_default_on_error(self, main_module): - with patch("shelfmark.core.settings_registry.load_config_file", side_effect=Exception("boom")): + with patch.object(main_module.app_config, "get", side_effect=Exception("boom")): assert main_module.get_auth_mode() == "none" class TestAuthCheckEndpoint: def test_auth_check_no_auth(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="none"): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={}): - with main_module.app.test_request_context("/api/auth/check"): - resp = _as_response(main_module.api_auth_check()) - data = resp.get_json() + with main_module.app.test_request_context("/api/auth/check"): + resp = _as_response(main_module.api_auth_check()) + data = resp.get_json() assert resp.status_code == 200 assert data == { @@ -91,10 +92,9 @@ class TestAuthCheckEndpoint: def test_auth_check_builtin_not_authenticated(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={}): - with main_module.app.test_request_context("/api/auth/check"): - resp = _as_response(main_module.api_auth_check()) - data = resp.get_json() + with main_module.app.test_request_context("/api/auth/check"): + resp = _as_response(main_module.api_auth_check()) + data = resp.get_json() assert resp.status_code == 200 assert data["authenticated"] is False @@ -105,12 +105,11 @@ class TestAuthCheckEndpoint: def test_auth_check_builtin_authenticated(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={}): - with main_module.app.test_request_context("/api/auth/check"): - main_module.session["user_id"] = "admin" - main_module.session["is_admin"] = True - resp = _as_response(main_module.api_auth_check()) - data = resp.get_json() + with main_module.app.test_request_context("/api/auth/check"): + main_module.session["user_id"] = "admin" + main_module.session["is_admin"] = True + resp = _as_response(main_module.api_auth_check()) + data = resp.get_json() assert resp.status_code == 200 assert data["authenticated"] is True @@ -121,12 +120,13 @@ class TestAuthCheckEndpoint: def test_auth_check_proxy_includes_logout_url(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({ "PROXY_AUTH_USER_HEADER": "X-Auth-User", "PROXY_AUTH_LOGOUT_URL": "https://auth.example.com/logout", - }, + }), ): with main_module.app.test_request_context("/api/auth/check"): main_module.session["user_id"] = "proxyuser" @@ -287,9 +287,10 @@ class TestLoginEndpoint: class TestLogoutEndpoint: def test_logout_proxy_returns_logout_url(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"PROXY_AUTH_LOGOUT_URL": "https://auth.example.com/logout"}, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_LOGOUT_URL": "https://auth.example.com/logout"}), ): with main_module.app.test_request_context("/api/auth/logout", method="POST"): main_module.session["user_id"] = "proxyuser" @@ -302,11 +303,10 @@ class TestLogoutEndpoint: def test_logout_basic(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch("shelfmark.core.settings_registry.load_config_file", return_value={}): - with main_module.app.test_request_context("/api/auth/logout", method="POST"): - main_module.session["user_id"] = "admin" - resp = _as_response(main_module.api_logout()) - data = resp.get_json() + with main_module.app.test_request_context("/api/auth/logout", method="POST"): + main_module.session["user_id"] = "admin" + resp = _as_response(main_module.api_logout()) + data = resp.get_json() assert resp.status_code == 200 assert data["success"] is True diff --git a/tests/e2e/test_proxy_auth_middleware.py b/tests/e2e/test_proxy_auth_middleware.py index c37f234..b74ee38 100644 --- a/tests/e2e/test_proxy_auth_middleware.py +++ b/tests/e2e/test_proxy_auth_middleware.py @@ -18,6 +18,13 @@ def _as_response(result: Any): return result +def _config_getter(values: dict[str, Any]): + def _get(key: str, default: Any = None, user_id: Any = None): + return values.get(key, default) + + return _get + + @pytest.fixture(scope="module") def main_module(): with patch("shelfmark.download.orchestrator.start"): @@ -43,9 +50,10 @@ class TestProxyAuthMiddleware: def test_allows_auth_check_without_header(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"PROXY_AUTH_USER_HEADER": "X-Auth-User"}, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context("/api/auth/check"): result = main_module.proxy_auth_middleware() @@ -54,11 +62,10 @@ class TestProxyAuthMiddleware: def test_sets_session_from_header(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ - "PROXY_AUTH_USER_HEADER": "X-Auth-User", - }, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context( "/api/releases", @@ -84,9 +91,10 @@ class TestProxyAuthMiddleware: ) with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"PROXY_AUTH_USER_HEADER": "X-Auth-User"}, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context( "/api/releases", @@ -104,11 +112,10 @@ class TestProxyAuthMiddleware: def test_reprovisions_when_proxy_identity_changes(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ - "PROXY_AUTH_USER_HEADER": "X-Auth-User", - }, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context( "/api/releases", @@ -130,11 +137,10 @@ class TestProxyAuthMiddleware: assert main_module.user_db.get_user(user_id=stale_user_id) is None with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ - "PROXY_AUTH_USER_HEADER": "X-Auth-User", - }, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context( "/api/releases", @@ -164,11 +170,10 @@ class TestProxyAuthMiddleware: ) with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ - "PROXY_AUTH_USER_HEADER": "X-Auth-User", - }, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context( "/api/releases", @@ -191,9 +196,10 @@ class TestProxyAuthMiddleware: def test_returns_401_when_header_missing_on_protected_path(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"PROXY_AUTH_USER_HEADER": "X-Auth-User"}, + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({"PROXY_AUTH_USER_HEADER": "X-Auth-User"}), ): with main_module.app.test_request_context("/api/releases"): resp = _as_response(main_module.proxy_auth_middleware()) @@ -204,13 +210,14 @@ class TestProxyAuthMiddleware: def test_admin_group_membership(self, main_module): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={ + with patch.object( + main_module.app_config, + "get", + side_effect=_config_getter({ "PROXY_AUTH_USER_HEADER": "X-Auth-User", "PROXY_AUTH_ADMIN_GROUP_HEADER": "X-Auth-Groups", "PROXY_AUTH_ADMIN_GROUP_NAME": "admins", - }, + }), ): with main_module.app.test_request_context( "/api/releases", @@ -259,78 +266,58 @@ class TestLoginRequiredDecorator: def test_settings_access_requires_admin_even_when_legacy_toggle_off(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"RESTRICT_SETTINGS_TO_ADMIN": False}, - ): - with main_module.app.test_request_context("/api/settings/general"): - main_module.session["user_id"] = "user" - main_module.session["is_admin"] = False - decorated = main_module.login_required(view) - resp = _as_response(decorated()) - data = resp.get_json() + with main_module.app.test_request_context("/api/settings/general"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() assert resp.status_code == 403 assert "Admin access required" in (data.get("error") or "") def test_security_tab_always_blocks_non_admin_even_when_toggle_off(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"RESTRICT_SETTINGS_TO_ADMIN": False}, - ): - with main_module.app.test_request_context("/api/settings/security"): - main_module.session["user_id"] = "user" - main_module.session["is_admin"] = False - decorated = main_module.login_required(view) - resp = _as_response(decorated()) - data = resp.get_json() + with main_module.app.test_request_context("/api/settings/security"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() assert resp.status_code == 403 assert "Admin access required" in (data.get("error") or "") def test_users_tab_always_blocks_non_admin_even_when_toggle_off(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="builtin"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"RESTRICT_SETTINGS_TO_ADMIN": False}, - ): - with main_module.app.test_request_context("/api/settings/users"): - main_module.session["user_id"] = "user" - main_module.session["is_admin"] = False - decorated = main_module.login_required(view) - resp = _as_response(decorated()) - data = resp.get_json() + with main_module.app.test_request_context("/api/settings/users"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() assert resp.status_code == 403 assert "Admin access required" in (data.get("error") or "") def test_proxy_admin_restriction_blocks_non_admin(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="proxy"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"RESTRICT_SETTINGS_TO_ADMIN": True}, - ): - with main_module.app.test_request_context("/api/settings/general"): - main_module.session["user_id"] = "user" - main_module.session["is_admin"] = False - decorated = main_module.login_required(view) - resp = _as_response(decorated()) - data = resp.get_json() + with main_module.app.test_request_context("/api/settings/general"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) + data = resp.get_json() assert resp.status_code == 403 assert "Admin access required" in (data.get("error") or "") def test_cwa_admin_restriction_blocks_non_admin(self, main_module, view): with patch.object(main_module, "get_auth_mode", return_value="cwa"): - with patch( - "shelfmark.core.settings_registry.load_config_file", - return_value={"RESTRICT_SETTINGS_TO_ADMIN": True}, - ): - with main_module.app.test_request_context("/api/settings/general"): - main_module.session["user_id"] = "user" - main_module.session["is_admin"] = False - decorated = main_module.login_required(view) - resp = _as_response(decorated()) + with main_module.app.test_request_context("/api/settings/general"): + main_module.session["user_id"] = "user" + main_module.session["is_admin"] = False + decorated = main_module.login_required(view) + resp = _as_response(decorated()) assert resp.status_code == 403 diff --git a/tests/prowlarr/test_handler.py b/tests/prowlarr/test_handler.py index abfeefe..90467f5 100644 --- a/tests/prowlarr/test_handler.py +++ b/tests/prowlarr/test_handler.py @@ -108,6 +108,32 @@ class TestProwlarrHandlerDownloadErrors: assert recorder.last_message is not None assert "cache" in recorder.last_message.lower() + def test_resolve_download_uses_task_retry_fields_when_cache_is_missing(self): + """Generic retry fields should let restarts recover without the in-memory cache.""" + with patch( + "shelfmark.release_sources.prowlarr.handler.get_release", + return_value=None, + ): + handler = ProwlarrHandler() + task = DownloadTask( + task_id="retry-context-release", + source="prowlarr", + title="Recovered Release", + retry_download_url="magnet:?xt=urn:btih:abc123", + retry_download_protocol="torrent", + retry_release_name="Recovered Release", + retry_seeding_time_limit_minutes=60, + retry_ratio_limit=1.5, + ) + + request = handler._resolve_download(task, lambda *_: None) + + assert request is not None + assert request.url == "magnet:?xt=urn:btih:abc123" + assert request.protocol == "torrent" + assert request.seeding_time_limit == 60 + assert request.ratio_limit == 1.5 + def test_download_fails_without_download_url(self): """Test that download fails when release has no download URL.""" with patch(