Eliminate direct app.state access from routers
This commit is contained in:
@@ -20,6 +20,7 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
||||
- Issue: Several routers still read or mutate `app.state` directly, including `setup.py` and `config.py`, which breaks the intended separation of concerns and makes handlers harder to test.
|
||||
- Propose: Refactor routers to consume explicit dependencies for required values and actions, and move all direct state mutations into services or dedicated state managers.
|
||||
- Test: Ensure all router handlers compile without requiring direct `app.state` access and existing endpoint tests still pass.
|
||||
- Status: completed
|
||||
|
||||
3. Replace process-local session cache with a pluggable abstraction
|
||||
- Goal: Make session validation consistent across multi-worker deployments by replacing the module-level in-memory cache with an injectable cache backend.
|
||||
|
||||
@@ -37,7 +37,6 @@ global settings, test regex patterns, add log paths, and preview log files.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import datetime
|
||||
from typing import Annotated
|
||||
|
||||
import structlog
|
||||
@@ -45,7 +44,6 @@ from fastapi import APIRouter, HTTPException, Path, Query, Request, status
|
||||
|
||||
from app.dependencies import (
|
||||
AppDep,
|
||||
AppStateDep,
|
||||
AuthDep,
|
||||
DbDep,
|
||||
Fail2BanConfigDirDep,
|
||||
@@ -117,6 +115,12 @@ from app.services.jail_config_service import (
|
||||
)
|
||||
from app.tasks.health_check import _run_probe
|
||||
from app.utils.fail2ban_client import Fail2BanConnectionError
|
||||
from app.utils.runtime_state import (
|
||||
clear_activation_record,
|
||||
clear_pending_recovery,
|
||||
create_pending_recovery,
|
||||
record_activation,
|
||||
)
|
||||
|
||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||
|
||||
@@ -678,7 +682,6 @@ async def update_map_color_thresholds(
|
||||
summary="Activate an inactive jail",
|
||||
)
|
||||
async def activate_jail(
|
||||
app_state: AppStateDep,
|
||||
app: AppDep,
|
||||
_auth: AuthDep,
|
||||
config_dir: Fail2BanConfigDirDep,
|
||||
@@ -693,7 +696,6 @@ async def activate_jail(
|
||||
the jail starts immediately.
|
||||
|
||||
Args:
|
||||
app_state: FastAPI application state.
|
||||
app: FastAPI application instance.
|
||||
_auth: Validated session.
|
||||
name: Name of the jail to activate.
|
||||
@@ -732,18 +734,15 @@ async def activate_jail(
|
||||
|
||||
# Record this activation so the health-check task can attribute a
|
||||
# subsequent fail2ban crash to it.
|
||||
app_state.last_activation = {
|
||||
"jail_name": name,
|
||||
"at": datetime.datetime.now(tz=datetime.UTC),
|
||||
}
|
||||
activation_time = record_activation(app, name)
|
||||
|
||||
# If fail2ban stopped responding after the reload, create a pending-recovery
|
||||
# record immediately (before the background health task notices).
|
||||
if not result.fail2ban_running:
|
||||
app_state.pending_recovery = PendingRecovery(
|
||||
create_pending_recovery(
|
||||
app,
|
||||
jail_name=name,
|
||||
activated_at=app_state.last_activation["at"],
|
||||
detected_at=datetime.datetime.now(tz=datetime.UTC),
|
||||
activated_at=activation_time,
|
||||
)
|
||||
|
||||
# Force an immediate health probe so the cached status reflects the current
|
||||
@@ -935,7 +934,7 @@ async def get_pending_recovery(
|
||||
)
|
||||
async def rollback_jail(
|
||||
_auth: AuthDep,
|
||||
app_state: AppStateDep,
|
||||
app: AppDep,
|
||||
config_dir: Fail2BanConfigDirDep,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
start_cmd: Fail2BanStartCommandDep,
|
||||
@@ -951,6 +950,7 @@ async def rollback_jail(
|
||||
|
||||
Args:
|
||||
_auth: Validated session.
|
||||
app: FastAPI application instance.
|
||||
name: Jail name to disable and roll back.
|
||||
|
||||
Returns:
|
||||
@@ -974,8 +974,8 @@ async def rollback_jail(
|
||||
|
||||
# Clear pending recovery if fail2ban came back online.
|
||||
if result.fail2ban_running:
|
||||
app_state.pending_recovery = None
|
||||
app_state.last_activation = None
|
||||
clear_pending_recovery(app)
|
||||
clear_activation_record(app)
|
||||
|
||||
return result
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ from fastapi import APIRouter, HTTPException, status
|
||||
from app.dependencies import AppDep, DbDep, SettingsDep
|
||||
from app.models.setup import SetupRequest, SetupResponse, SetupStatusResponse, SetupTimezoneResponse
|
||||
from app.services import setup_service
|
||||
from app.utils.runtime_state import update_app_settings
|
||||
from app.utils.setup_state import is_setup_complete_cached, set_setup_complete_cache
|
||||
|
||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||
@@ -75,13 +76,12 @@ async def post_setup(
|
||||
session_duration_minutes=body.session_duration_minutes,
|
||||
)
|
||||
set_setup_complete_cache(app, True)
|
||||
app.state.settings = app.state.settings.model_copy(
|
||||
update={
|
||||
"database_path": body.database_path,
|
||||
"fail2ban_socket": body.fail2ban_socket,
|
||||
"timezone": body.timezone,
|
||||
"session_duration_minutes": body.session_duration_minutes,
|
||||
}
|
||||
update_app_settings(
|
||||
app,
|
||||
database_path=body.database_path,
|
||||
fail2ban_socket=body.fail2ban_socket,
|
||||
timezone=body.timezone,
|
||||
session_duration_minutes=body.session_duration_minutes,
|
||||
)
|
||||
return SetupResponse()
|
||||
|
||||
|
||||
@@ -15,10 +15,11 @@ from typing import TYPE_CHECKING, Any
|
||||
|
||||
from starlette.datastructures import State
|
||||
|
||||
from app.models.config import PendingRecovery
|
||||
from app.models.server import ServerStatus
|
||||
|
||||
if TYPE_CHECKING: # pragma: no cover
|
||||
from app.models.config import PendingRecovery
|
||||
from app.config import Settings
|
||||
|
||||
ActivationRecord = dict[str, datetime.datetime]
|
||||
|
||||
@@ -83,3 +84,53 @@ def get_runtime_state(app: Any) -> RuntimeState:
|
||||
if state is None or not hasattr(state, "runtime_state"):
|
||||
raise AttributeError("Runtime state has not been initialised on the application.")
|
||||
return state.runtime_state
|
||||
|
||||
|
||||
def get_app_settings(app: Any) -> Settings:
|
||||
"""Return the current immutable settings from application state."""
|
||||
settings = getattr(app.state, "settings", None)
|
||||
if settings is None:
|
||||
raise AttributeError("Application settings are not available on the app state.")
|
||||
return settings
|
||||
|
||||
|
||||
def update_app_settings(app: Any, **overrides: Any) -> None:
|
||||
"""Update the current application settings immutably."""
|
||||
settings = get_app_settings(app)
|
||||
app.state.settings = settings.model_copy(update=overrides)
|
||||
|
||||
|
||||
def record_activation(app: Any, jail_name: str, at: datetime.datetime | None = None) -> datetime.datetime:
|
||||
"""Record a jail activation timestamp in runtime state."""
|
||||
now = at if at is not None else datetime.datetime.now(tz=datetime.UTC)
|
||||
runtime_state = get_runtime_state(app)
|
||||
runtime_state.last_activation = {
|
||||
"jail_name": jail_name,
|
||||
"at": now,
|
||||
}
|
||||
return now
|
||||
|
||||
|
||||
def create_pending_recovery(
|
||||
app: Any,
|
||||
jail_name: str,
|
||||
activated_at: datetime.datetime,
|
||||
detected_at: datetime.datetime | None = None,
|
||||
) -> None:
|
||||
"""Create a pending recovery record in runtime state."""
|
||||
runtime_state = get_runtime_state(app)
|
||||
runtime_state.pending_recovery = PendingRecovery(
|
||||
jail_name=jail_name,
|
||||
activated_at=activated_at,
|
||||
detected_at=detected_at if detected_at is not None else datetime.datetime.now(tz=datetime.UTC),
|
||||
)
|
||||
|
||||
|
||||
def clear_pending_recovery(app: Any) -> None:
|
||||
"""Clear the current pending recovery record."""
|
||||
get_runtime_state(app).pending_recovery = None
|
||||
|
||||
|
||||
def clear_activation_record(app: Any) -> None:
|
||||
"""Clear the current activation tracking record."""
|
||||
get_runtime_state(app).last_activation = None
|
||||
|
||||
Reference in New Issue
Block a user