diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 8f263f0..03c03b6 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -10,6 +10,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. ### Backend Architecture Review Findings +- Status: **done** — `backend/app/routers/config.py` now uses explicit dependency injection for fail2ban settings and no longer reads `request.app.state.settings` directly. + - `backend/app/routers/*` often reads config directly from `request.app.state.settings` instead of using dependency injection. This bypasses the dependency layer and creates hidden coupling between routers and application state. - Fix: replace direct `request.app.state.settings` access with `SettingsDep` or other explicit dependencies such as `ServerStatusDep` and `PendingRecoveryDep` in router function signatures. - Expected outcome: routers become easier to unit test, composition is more explicit, and shared state access is only available through documented FastAPI dependencies. diff --git a/backend/app/routers/config.py b/backend/app/routers/config.py index 1be79af..4352246 100644 --- a/backend/app/routers/config.py +++ b/backend/app/routers/config.py @@ -43,7 +43,15 @@ from typing import Annotated import structlog from fastapi import APIRouter, HTTPException, Path, Query, Request, status -from app.dependencies import AuthDep, DbDep +from app.dependencies import ( + AuthDep, + DbDep, + Fail2BanConfigDirDep, + Fail2BanSocketDep, + Fail2BanStartCommandDep, + PendingRecoveryDep, +) +from app.exceptions import ConfigOperationError, ConfigValidationError, JailNotFoundError, JailOperationError from app.models.config import ( ActionConfig, ActionCreateRequest, @@ -76,12 +84,14 @@ from app.models.config import ( RollbackResponse, ServiceStatusResponse, ) -from app.services import config_service, jail_service, log_service from app.services import ( action_config_service, config_file_service, + config_service, filter_config_service, jail_config_service, + jail_service, + log_service, ) from app.services.action_config_service import ( ActionAlreadyExistsError, @@ -103,7 +113,6 @@ from app.services.jail_config_service import ( JailNameError, JailNotFoundInConfigError, ) -from app.exceptions import ConfigOperationError, ConfigValidationError, JailNotFoundError, JailOperationError from app.tasks.health_check import _run_probe from app.utils.fail2ban_client import Fail2BanConnectionError @@ -159,6 +168,7 @@ def _bad_request(message: str) -> HTTPException: async def get_jail_configs( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, ) -> JailConfigListResponse: """Return editable configuration for every active fail2ban jail. @@ -172,7 +182,6 @@ async def get_jail_configs( Returns: :class:`~app.models.config.JailConfigListResponse`. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: return await config_service.list_jail_configs(socket_path) except Fail2BanConnectionError as exc: @@ -187,6 +196,8 @@ async def get_jail_configs( async def get_inactive_jails( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, ) -> InactiveJailListResponse: """Return all jails defined in fail2ban config files that are not running. @@ -201,8 +212,6 @@ async def get_inactive_jails( Returns: :class:`~app.models.config.InactiveJailListResponse`. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket return await jail_config_service.list_inactive_jails(config_dir, socket_path) @@ -214,6 +223,7 @@ async def get_inactive_jails( async def get_jail_config( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, name: _NamePath, ) -> JailConfigResponse: """Return the full editable configuration for one fail2ban jail. @@ -230,7 +240,6 @@ async def get_jail_config( HTTPException: 404 when the jail does not exist. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: return await config_service.get_jail_config(socket_path, name) except JailNotFoundError: @@ -247,6 +256,7 @@ async def get_jail_config( async def update_jail_config( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, name: _NamePath, body: JailConfigUpdate, ) -> None: @@ -267,7 +277,6 @@ async def update_jail_config( HTTPException: 400 when a set command is rejected. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: await config_service.update_jail_config(socket_path, name, body) except JailNotFoundError: @@ -293,6 +302,7 @@ async def update_jail_config( async def get_global_config( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, ) -> GlobalConfigResponse: """Return global fail2ban settings (log level, log target, database config). @@ -306,7 +316,6 @@ async def get_global_config( Raises: HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: return await config_service.get_global_config(socket_path) except Fail2BanConnectionError as exc: @@ -321,6 +330,7 @@ async def get_global_config( async def update_global_config( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, body: GlobalConfigUpdate, ) -> None: """Update global fail2ban settings. @@ -334,7 +344,6 @@ async def update_global_config( HTTPException: 400 when a set command is rejected. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: await config_service.update_global_config(socket_path, body) except ConfigOperationError as exc: @@ -356,6 +365,7 @@ async def update_global_config( async def reload_fail2ban( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, ) -> None: """Trigger a full fail2ban reload. @@ -369,7 +379,6 @@ async def reload_fail2ban( HTTPException: 409 when fail2ban reports the reload failed. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: await jail_service.reload_all(socket_path) except JailOperationError as exc: @@ -393,6 +402,8 @@ async def reload_fail2ban( async def restart_fail2ban( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, + start_cmd: Fail2BanStartCommandDep, ) -> None: """Trigger a full fail2ban service restart. @@ -414,8 +425,6 @@ async def restart_fail2ban( ``POST /api/config/jails/{name}/rollback`` if a specific jail is suspect. """ - socket_path: str = request.app.state.settings.fail2ban_socket - start_cmd: str = request.app.state.settings.fail2ban_start_command start_cmd_parts: list[str] = start_cmd.split() # Step 1: stop the daemon via socket. @@ -488,6 +497,7 @@ async def regex_test( async def add_log_path( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, name: _NamePath, body: AddLogPathRequest, ) -> None: @@ -507,7 +517,6 @@ async def add_log_path( HTTPException: 400 when the command is rejected. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: await config_service.add_log_path(socket_path, name, body) except JailNotFoundError: @@ -526,6 +535,7 @@ async def add_log_path( async def delete_log_path( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, name: _NamePath, log_path: str = Query(..., description="Absolute path of the log file to stop monitoring."), ) -> None: @@ -545,7 +555,6 @@ async def delete_log_path( HTTPException: 400 when the command is rejected. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: await config_service.delete_log_path(socket_path, name, log_path) except JailNotFoundError: @@ -669,6 +678,8 @@ async def update_map_color_thresholds( async def activate_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, body: ActivateJailRequest | None = None, ) -> JailActivationResponse: @@ -694,8 +705,6 @@ async def activate_jail( HTTPException: 409 if the jail is already active. HTTPException: 502 if fail2ban is unreachable. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket req = body if body is not None else ActivateJailRequest() try: @@ -748,6 +757,8 @@ async def activate_jail( async def deactivate_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, ) -> JailActivationResponse: """Disable an active jail and reload fail2ban. @@ -769,8 +780,6 @@ async def deactivate_jail( HTTPException: 409 if the jail is already inactive. HTTPException: 502 if fail2ban is unreachable. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: result = await jail_config_service.deactivate_jail(config_dir, socket_path, name) @@ -807,6 +816,8 @@ async def deactivate_jail( async def delete_jail_local_override( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, ) -> None: """Remove the ``jail.d/{name}.local`` override file for an inactive jail. @@ -828,8 +839,6 @@ async def delete_jail_local_override( HTTPException: 500 if the file cannot be deleted. HTTPException: 502 if fail2ban is unreachable. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: await jail_config_service.delete_jail_local_override(config_dir, socket_path, name) @@ -864,6 +873,7 @@ async def delete_jail_local_override( async def validate_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, name: _NamePath, ) -> JailValidationResult: """Run pre-activation validation checks on a jail configuration. @@ -883,7 +893,6 @@ async def validate_jail( HTTPException: 400 if *name* contains invalid characters. HTTPException: 404 if *name* is not found in any config file. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir try: return await jail_config_service.validate_jail_config(config_dir, name) except JailNameError as exc: @@ -896,8 +905,8 @@ async def validate_jail( summary="Return active crash-recovery record if one exists", ) async def get_pending_recovery( - request: Request, _auth: AuthDep, + pending_recovery: PendingRecoveryDep, ) -> PendingRecovery | None: """Return the current :class:`~app.models.config.PendingRecovery` record. @@ -912,7 +921,7 @@ async def get_pending_recovery( Returns: :class:`~app.models.config.PendingRecovery` or ``None``. """ - return getattr(request.app.state, "pending_recovery", None) + return pending_recovery @router.post( @@ -923,6 +932,9 @@ async def get_pending_recovery( async def rollback_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, + start_cmd: Fail2BanStartCommandDep, name: _NamePath, ) -> RollbackResponse: """Disable the specified jail and attempt to restart fail2ban. @@ -945,9 +957,6 @@ async def rollback_jail( HTTPException: 400 if *name* contains invalid characters. HTTPException: 500 if writing the .local override file fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket - start_cmd: str = request.app.state.settings.fail2ban_start_command start_cmd_parts: list[str] = start_cmd.split() try: @@ -981,6 +990,8 @@ async def rollback_jail( async def list_filters( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, ) -> FilterListResponse: """Return all filters discovered in ``filter.d/`` with active/inactive status. @@ -1001,8 +1012,6 @@ async def list_filters( :class:`~app.models.config.FilterListResponse` with all discovered filters. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket result = await filter_config_service.list_filters(config_dir, socket_path) # Sort: active first (by name), then inactive (by name). result.filters.sort(key=lambda f: (not f.active, f.name.lower())) @@ -1017,6 +1026,8 @@ async def list_filters( async def get_filter( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: Annotated[str, Path(description="Filter base name, e.g. ``sshd`` or ``sshd.conf``.")], ) -> FilterConfig: """Return the full parsed configuration and active/inactive status for one filter. @@ -1037,8 +1048,6 @@ async def get_filter( HTTPException: 404 if the filter is not found in ``filter.d/``. HTTPException: 502 if fail2ban is unreachable. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await filter_config_service.get_filter(config_dir, socket_path, name) except FilterNotFoundError: @@ -1074,6 +1083,8 @@ def _filter_not_found(name: str) -> HTTPException: async def update_filter( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _FilterNamePath, body: FilterUpdateRequest, reload: bool = Query(default=False, description="Reload fail2ban after writing."), @@ -1101,8 +1112,6 @@ async def update_filter( HTTPException: 422 if any regex pattern fails to compile. HTTPException: 500 if writing the ``.local`` file fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await filter_config_service.update_filter(config_dir, socket_path, name, body, do_reload=reload) except FilterNameError as exc: @@ -1127,6 +1136,8 @@ async def update_filter( async def create_filter( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, body: FilterCreateRequest, reload: bool = Query(default=False, description="Reload fail2ban after creating."), ) -> FilterConfig: @@ -1151,8 +1162,6 @@ async def create_filter( HTTPException: 422 if any regex pattern is invalid. HTTPException: 500 if writing fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await filter_config_service.create_filter(config_dir, socket_path, body, do_reload=reload) except FilterNameError as exc: @@ -1179,6 +1188,7 @@ async def create_filter( async def delete_filter( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, name: _FilterNamePath, ) -> None: """Delete a user-created filter's ``.local`` override file. @@ -1199,7 +1209,6 @@ async def delete_filter( HTTPException: 409 if the filter is a shipped default (conf-only). HTTPException: 500 if deletion fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir try: await filter_config_service.delete_filter(config_dir, name) except FilterNameError as exc: @@ -1226,6 +1235,8 @@ async def delete_filter( async def assign_filter_to_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, body: AssignFilterRequest, reload: bool = Query(default=False, description="Reload fail2ban after assigning."), @@ -1247,8 +1258,6 @@ async def assign_filter_to_jail( HTTPException: 404 if the jail or filter does not exist. HTTPException: 500 if writing fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: await filter_config_service.assign_filter_to_jail(config_dir, socket_path, name, body, do_reload=reload) except (JailNameError, FilterNameError) as exc: @@ -1292,6 +1301,8 @@ def _action_not_found(name: str) -> HTTPException: async def list_actions( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, ) -> ActionListResponse: """Return all actions discovered in ``action.d/`` with active/inactive status. @@ -1312,8 +1323,6 @@ async def list_actions( :class:`~app.models.config.ActionListResponse` with all discovered actions. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket result = await action_config_service.list_actions(config_dir, socket_path) result.actions.sort(key=lambda a: (not a.active, a.name.lower())) return result @@ -1327,6 +1336,8 @@ async def list_actions( async def get_action( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _ActionNamePath, ) -> ActionConfig: """Return the full parsed configuration and active/inactive status for one action. @@ -1346,8 +1357,6 @@ async def get_action( Raises: HTTPException: 404 if the action is not found in ``action.d/``. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await action_config_service.get_action(config_dir, socket_path, name) except ActionNotFoundError: @@ -1367,6 +1376,8 @@ async def get_action( async def update_action( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _ActionNamePath, body: ActionUpdateRequest, reload: bool = Query(default=False, description="Reload fail2ban after writing."), @@ -1391,8 +1402,6 @@ async def update_action( HTTPException: 404 if the action does not exist. HTTPException: 500 if writing the ``.local`` file fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await action_config_service.update_action(config_dir, socket_path, name, body, do_reload=reload) except ActionNameError as exc: @@ -1415,6 +1424,8 @@ async def update_action( async def create_action( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, body: ActionCreateRequest, reload: bool = Query(default=False, description="Reload fail2ban after creating."), ) -> ActionConfig: @@ -1437,8 +1448,6 @@ async def create_action( HTTPException: 409 if the action already exists. HTTPException: 500 if writing fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: return await action_config_service.create_action(config_dir, socket_path, body, do_reload=reload) except ActionNameError as exc: @@ -1463,6 +1472,7 @@ async def create_action( async def delete_action( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, name: _ActionNamePath, ) -> None: """Delete a user-created action's ``.local`` override file. @@ -1481,7 +1491,6 @@ async def delete_action( HTTPException: 409 if the action is a shipped default (conf-only). HTTPException: 500 if deletion fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir try: await action_config_service.delete_action(config_dir, name) except ActionNameError as exc: @@ -1508,6 +1517,8 @@ async def delete_action( async def assign_action_to_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, body: AssignActionRequest, reload: bool = Query(default=False, description="Reload fail2ban after assigning."), @@ -1530,8 +1541,6 @@ async def assign_action_to_jail( HTTPException: 404 if the jail or action does not exist. HTTPException: 500 if writing fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: await action_config_service.assign_action_to_jail(config_dir, socket_path, name, body, do_reload=reload) except (JailNameError, ActionNameError) as exc: @@ -1558,6 +1567,8 @@ async def assign_action_to_jail( async def remove_action_from_jail( request: Request, _auth: AuthDep, + config_dir: Fail2BanConfigDirDep, + socket_path: Fail2BanSocketDep, name: _NamePath, action_name: Annotated[str, Path(description="Action base name to remove.")], reload: bool = Query(default=False, description="Reload fail2ban after removing."), @@ -1579,10 +1590,14 @@ async def remove_action_from_jail( HTTPException: 404 if the jail is not found in config files. HTTPException: 500 if writing fails. """ - config_dir: str = request.app.state.settings.fail2ban_config_dir - socket_path: str = request.app.state.settings.fail2ban_socket try: - await action_config_service.remove_action_from_jail(config_dir, socket_path, name, action_name, do_reload=reload) + await action_config_service.remove_action_from_jail( + config_dir, + socket_path, + name, + action_name, + do_reload=reload, + ) except (JailNameError, ActionNameError) as exc: raise _bad_request(str(exc)) from exc except JailNotFoundInConfigError: @@ -1607,6 +1622,7 @@ async def remove_action_from_jail( async def get_fail2ban_log( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, lines: Annotated[int, Query(ge=1, le=2000, description="Number of lines to return from the tail.")] = 200, filter: Annotated[ # noqa: A002 str | None, @@ -1633,7 +1649,6 @@ async def get_fail2ban_log( the allowed directory. HTTPException: 502 when fail2ban is unreachable. """ - socket_path: str = request.app.state.settings.fail2ban_socket try: return await config_service.read_fail2ban_log(socket_path, lines, filter) except config_service.ConfigOperationError as exc: @@ -1650,6 +1665,7 @@ async def get_fail2ban_log( async def get_service_status( request: Request, _auth: AuthDep, + socket_path: Fail2BanSocketDep, ) -> ServiceStatusResponse: """Return fail2ban service health and current log configuration. @@ -1667,7 +1683,6 @@ async def get_service_status( HTTPException: 502 when fail2ban is unreachable (the service itself handles this gracefully and returns ``online=False``). """ - socket_path: str = request.app.state.settings.fail2ban_socket from app.services import health_service try: