refactor: comprehensive codebase review — security, performance, quality, UX
Security: - Fix NUT protocol command injection (validate names against safe regex) - Enable Jinja2 autoescape=True to prevent HTML injection via external data - Add WebhookProviderConfig validation model Performance: - Shared aiohttp.ClientSession singleton (replaces 40+ per-request sessions) - Fix 4 N+1 queries with batch IN loads (poller, scheduler, memory, broadcast) - asyncio.gather for Gitea commands and notification dispatcher - Add DB indexes on NotificationTrackerState.tracker_id, CommandTrackerListener - LRU cache for compiled Jinja2 templates - Daily EventLog cleanup job (90-day retention) - 30s HTTP timeout on all external calls - GROUP BY for target type counts (replaces 7 sequential queries) Code quality: - Extract get_owned_entity() helper (replaces 11 duplicate functions) - Extract slot_helpers.py (load_slots, save_slots, render_template_preview) - Extract command_utils.py (tracker lookup, last event, collection IDs) - Extract http_session.py (shared session lifecycle) - Provider connection validation dedup (3x → 1 helper) - Command dispatch tables replacing if/elif chains - Album+links fetch helper (fetch_albums_with_links) - Provider dispatch polymorphism (list_provider_collections) - Immutable _enrich_assets (no longer mutates in-place) - Fix _format_assets return type + handler unpacking Frontend: - Fix 18+ hardcoded English strings → t() with new i18n keys (en + ru) - Mobile "More" nav panel with provider filter and search - Shared Button.svelte component (4 variants, 2 sizes) - Shared ErrorBanner.svelte component (8 pages updated) - SvelteKit goto() replacing window.location.href - Dashboard grid fixed for 4 cards, paginator opacity consistency Functionality: - max_instances=1 on scheduler jobs (prevents duplicate events) - Webhook provider in watcher (prevents error spam) - Fix stale SQLModel reference in poller - Gitea get_repo() direct API call
This commit is contained in:
@@ -13,7 +13,12 @@ import aiohttp
|
||||
from ..auth.dependencies import get_current_user
|
||||
from ..database.engine import get_session
|
||||
from ..database.models import ServiceProvider, User
|
||||
from ..services import make_immich_provider, make_gitea_provider, make_planka_provider, make_nut_provider, make_google_photos_provider
|
||||
from ..services import (
|
||||
make_immich_provider, make_gitea_provider, make_planka_provider,
|
||||
make_nut_provider, make_google_photos_provider, list_provider_collections,
|
||||
)
|
||||
from ..services.http_session import get_http_session
|
||||
from .helpers import get_owned_entity
|
||||
|
||||
_LOGGER = logging.getLogger(__name__)
|
||||
|
||||
@@ -82,6 +87,20 @@ class GooglePhotosProviderConfig(BaseModel):
|
||||
refresh_token: str
|
||||
|
||||
|
||||
class PayloadMapping(BaseModel):
|
||||
variable: str
|
||||
jsonpath: str
|
||||
default: str | None = None
|
||||
|
||||
|
||||
class WebhookProviderConfig(BaseModel):
|
||||
auth_mode: str = "none"
|
||||
webhook_secret: str | None = None
|
||||
payload_mappings: list[PayloadMapping] = []
|
||||
event_type_path: str | None = None
|
||||
collection_path: str | None = None
|
||||
|
||||
|
||||
_PROVIDER_CONFIG_MODELS: dict[str, type[BaseModel]] = {
|
||||
"immich": ImmichProviderConfig,
|
||||
"gitea": GiteaProviderConfig,
|
||||
@@ -89,6 +108,7 @@ _PROVIDER_CONFIG_MODELS: dict[str, type[BaseModel]] = {
|
||||
"scheduler": SchedulerProviderConfig,
|
||||
"nut": NutProviderConfig,
|
||||
"google_photos": GooglePhotosProviderConfig,
|
||||
"webhook": WebhookProviderConfig,
|
||||
}
|
||||
|
||||
|
||||
@@ -106,6 +126,70 @@ def _validate_provider_config(provider_type: str, config: dict[str, Any]) -> Non
|
||||
)
|
||||
|
||||
|
||||
async def _test_provider_connection(provider: ServiceProvider) -> dict[str, Any]:
|
||||
"""Test provider connection and return the result dict.
|
||||
|
||||
For providers that lack optional credentials (gitea without api_token,
|
||||
planka without api_key), returns a success stub.
|
||||
"""
|
||||
http_session = await get_http_session()
|
||||
|
||||
if provider.type == "immich":
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
return await immich.test_connection()
|
||||
|
||||
if provider.type == "gitea":
|
||||
if not provider.config.get("api_token"):
|
||||
return {"ok": True, "message": "Gitea webhook-only mode (no API token for testing)"}
|
||||
gitea = make_gitea_provider(http_session, provider)
|
||||
return await gitea.test_connection()
|
||||
|
||||
if provider.type == "planka":
|
||||
if not provider.config.get("api_key"):
|
||||
return {"ok": True, "message": "Planka webhook-only mode (no API key for testing)"}
|
||||
planka = make_planka_provider(http_session, provider)
|
||||
return await planka.test_connection()
|
||||
|
||||
if provider.type == "nut":
|
||||
nut = make_nut_provider(provider)
|
||||
return await nut.test_connection()
|
||||
|
||||
if provider.type == "google_photos":
|
||||
gp = make_google_photos_provider(http_session, provider)
|
||||
return await gp.test_connection()
|
||||
|
||||
if provider.type in ("scheduler", "webhook"):
|
||||
return {"ok": True, "message": "Virtual provider — always available"}
|
||||
|
||||
return {"ok": False, "message": f"Unknown provider type: {provider.type}"}
|
||||
|
||||
|
||||
async def _validate_provider_connection(provider: ServiceProvider) -> dict[str, Any]:
|
||||
"""Test provider connection. Raise HTTPException on failure.
|
||||
|
||||
Returns the test_result dict on success (caller may inspect extra fields
|
||||
like ``external_domain``).
|
||||
"""
|
||||
try:
|
||||
test_result = await _test_provider_connection(provider)
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
except OSError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", f"Cannot connect to {provider.type} provider"),
|
||||
)
|
||||
return test_result
|
||||
|
||||
|
||||
@router.get("")
|
||||
async def list_providers(
|
||||
user: User = Depends(get_current_user),
|
||||
@@ -128,96 +212,15 @@ async def create_provider(
|
||||
"""Add a new service provider (validates connection for known types)."""
|
||||
_validate_provider_config(body.type, body.config)
|
||||
|
||||
# Validate connection for known provider types
|
||||
try:
|
||||
if body.type == "immich":
|
||||
from notify_bridge_core.providers.immich import ImmichServiceProvider
|
||||
config = body.config
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = ImmichServiceProvider(
|
||||
http_session, config.get("url", ""), config.get("api_key", ""),
|
||||
config.get("external_domain"), body.name,
|
||||
)
|
||||
test_result = await immich.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", f"Cannot connect to {body.type} provider"),
|
||||
)
|
||||
# Store external_domain from server config if available
|
||||
if test_result.get("external_domain"):
|
||||
config["external_domain"] = test_result["external_domain"]
|
||||
# Build a temporary ServiceProvider for connection testing
|
||||
temp_provider = ServiceProvider(
|
||||
id=0, user_id=0, type=body.type, name=body.name, config=body.config,
|
||||
)
|
||||
test_result = await _validate_provider_connection(temp_provider)
|
||||
|
||||
elif body.type == "gitea":
|
||||
config = body.config
|
||||
# api_token is optional (webhook_secret is required, but token only for repo listing)
|
||||
if config.get("api_token"):
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
from notify_bridge_core.providers.gitea import GiteaServiceProvider
|
||||
gitea = GiteaServiceProvider(
|
||||
http_session, config.get("url", ""), config.get("api_token", ""), body.name,
|
||||
)
|
||||
test_result = await gitea.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Gitea"),
|
||||
)
|
||||
|
||||
elif body.type == "planka":
|
||||
config = body.config
|
||||
if config.get("api_key"):
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
from notify_bridge_core.providers.planka import PlankaServiceProvider
|
||||
planka = PlankaServiceProvider(
|
||||
http_session, config.get("url", ""), config.get("api_key", ""), body.name,
|
||||
)
|
||||
test_result = await planka.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Planka"),
|
||||
)
|
||||
|
||||
elif body.type == "nut":
|
||||
nut = make_nut_provider(ServiceProvider(
|
||||
id=0, user_id=0, type="nut", name=body.name, config=body.config,
|
||||
))
|
||||
test_result = await nut.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to NUT server"),
|
||||
)
|
||||
|
||||
elif body.type == "google_photos":
|
||||
config = body.config
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
from notify_bridge_core.providers.google_photos import GooglePhotosServiceProvider
|
||||
gp = GooglePhotosServiceProvider(
|
||||
http_session, config.get("client_id", ""), config.get("client_secret", ""),
|
||||
config.get("refresh_token", ""), body.name,
|
||||
)
|
||||
test_result = await gp.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Google Photos"),
|
||||
)
|
||||
except HTTPException:
|
||||
raise
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
except OSError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
|
||||
# Scheduler: no validation needed (virtual provider)
|
||||
# Store external_domain from Immich server config if available
|
||||
if test_result.get("external_domain"):
|
||||
body.config["external_domain"] = test_result["external_domain"]
|
||||
|
||||
provider = ServiceProvider(
|
||||
user_id=user.id,
|
||||
@@ -307,78 +310,10 @@ async def update_provider(
|
||||
provider.config = body.config
|
||||
|
||||
# Re-validate connection when config changes for known provider types
|
||||
if config_changed and provider.type == "immich":
|
||||
try:
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
test_result = await immich.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", f"Cannot connect to {provider.type} provider"),
|
||||
)
|
||||
if test_result.get("external_domain"):
|
||||
provider.config = {**provider.config, "external_domain": test_result["external_domain"]}
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
elif config_changed and provider.type == "gitea":
|
||||
if provider.config.get("api_token"):
|
||||
try:
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gitea = make_gitea_provider(http_session, provider)
|
||||
test_result = await gitea.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Gitea"),
|
||||
)
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
elif config_changed and provider.type == "planka":
|
||||
if provider.config.get("api_key"):
|
||||
try:
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
planka = make_planka_provider(http_session, provider)
|
||||
test_result = await planka.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Planka"),
|
||||
)
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
elif config_changed and provider.type == "nut":
|
||||
nut = make_nut_provider(provider)
|
||||
test_result = await nut.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to NUT server"),
|
||||
)
|
||||
elif config_changed and provider.type == "google_photos":
|
||||
try:
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gp = make_google_photos_provider(http_session, provider)
|
||||
test_result = await gp.test_connection()
|
||||
if not test_result.get("ok"):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=test_result.get("message", "Cannot connect to Google Photos"),
|
||||
)
|
||||
except aiohttp.ClientError as err:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=f"Connection error: {err}",
|
||||
)
|
||||
if config_changed:
|
||||
test_result = await _validate_provider_connection(provider)
|
||||
if test_result.get("external_domain"):
|
||||
provider.config = {**provider.config, "external_domain": test_result["external_domain"]}
|
||||
|
||||
session.add(provider)
|
||||
await session.commit()
|
||||
@@ -408,39 +343,7 @@ async def test_provider(
|
||||
):
|
||||
"""Check if a service provider is reachable."""
|
||||
provider = await _get_user_provider(session, provider_id, user.id)
|
||||
|
||||
if provider.type == "immich":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
return await immich.test_connection()
|
||||
|
||||
if provider.type == "gitea":
|
||||
if not provider.config.get("api_token"):
|
||||
return {"ok": True, "message": "Gitea webhook-only mode (no API token for testing)"}
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gitea = make_gitea_provider(http_session, provider)
|
||||
return await gitea.test_connection()
|
||||
|
||||
if provider.type == "planka":
|
||||
if not provider.config.get("api_key"):
|
||||
return {"ok": True, "message": "Planka webhook-only mode (no API key for testing)"}
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
planka = make_planka_provider(http_session, provider)
|
||||
return await planka.test_connection()
|
||||
|
||||
if provider.type == "scheduler":
|
||||
return {"ok": True, "message": "Virtual provider — always available"}
|
||||
|
||||
if provider.type == "nut":
|
||||
nut = make_nut_provider(provider)
|
||||
return await nut.test_connection()
|
||||
|
||||
if provider.type == "google_photos":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gp = make_google_photos_provider(http_session, provider)
|
||||
return await gp.test_connection()
|
||||
|
||||
return {"ok": False, "message": f"Unknown provider type: {provider.type}"}
|
||||
return await _test_provider_connection(provider)
|
||||
|
||||
|
||||
@router.get("/{provider_id}/people")
|
||||
@@ -454,14 +357,14 @@ async def list_people(
|
||||
|
||||
if provider.type == "immich":
|
||||
from notify_bridge_core.providers.immich.client import ImmichClient
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
client = ImmichClient(
|
||||
http_session,
|
||||
provider.config.get("url", ""),
|
||||
provider.config.get("api_key", ""),
|
||||
)
|
||||
people = await client.get_people()
|
||||
return [{"id": pid, "name": name} for pid, name in people.items()]
|
||||
http_session = await get_http_session()
|
||||
client = ImmichClient(
|
||||
http_session,
|
||||
provider.config.get("url", ""),
|
||||
provider.config.get("api_key", ""),
|
||||
)
|
||||
people = await client.get_people()
|
||||
return [{"id": pid, "name": name} for pid, name in people.items()]
|
||||
|
||||
return []
|
||||
|
||||
@@ -475,35 +378,7 @@ async def list_collections(
|
||||
"""Fetch collections from a service provider."""
|
||||
provider = await _get_user_provider(session, provider_id, user.id)
|
||||
|
||||
if provider.type == "immich":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
return await immich.list_collections()
|
||||
|
||||
if provider.type == "gitea":
|
||||
if not provider.config.get("api_token"):
|
||||
return []
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gitea = make_gitea_provider(http_session, provider)
|
||||
return await gitea.list_collections()
|
||||
|
||||
if provider.type == "planka":
|
||||
if not provider.config.get("api_key"):
|
||||
return []
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
planka = make_planka_provider(http_session, provider)
|
||||
return await planka.list_collections()
|
||||
|
||||
if provider.type == "nut":
|
||||
nut = make_nut_provider(provider)
|
||||
return await nut.list_collections()
|
||||
|
||||
if provider.type == "google_photos":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
gp = make_google_photos_provider(http_session, provider)
|
||||
return await gp.list_collections()
|
||||
|
||||
return []
|
||||
return await list_provider_collections(provider)
|
||||
|
||||
|
||||
@router.get("/{provider_id}/albums/{album_id}/shared-links")
|
||||
@@ -517,19 +392,19 @@ async def get_album_shared_links(
|
||||
provider = await _get_user_provider(session, provider_id, user.id)
|
||||
|
||||
if provider.type == "immich":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
links = await immich.client.get_shared_links(album_id)
|
||||
return [
|
||||
{
|
||||
"id": link.id,
|
||||
"key": link.key,
|
||||
"has_password": link.has_password,
|
||||
"is_expired": link.is_expired,
|
||||
"is_accessible": link.is_accessible,
|
||||
}
|
||||
for link in links
|
||||
]
|
||||
http_session = await get_http_session()
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
links = await immich.client.get_shared_links(album_id)
|
||||
return [
|
||||
{
|
||||
"id": link.id,
|
||||
"key": link.key,
|
||||
"has_password": link.has_password,
|
||||
"is_expired": link.is_expired,
|
||||
"is_accessible": link.is_accessible,
|
||||
}
|
||||
for link in links
|
||||
]
|
||||
|
||||
return []
|
||||
|
||||
@@ -545,15 +420,13 @@ async def create_album_shared_link(
|
||||
provider = await _get_user_provider(session, provider_id, user.id)
|
||||
|
||||
if provider.type == "immich":
|
||||
async with aiohttp.ClientSession() as http_session:
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
success = await immich.client.create_shared_link(album_id)
|
||||
if success:
|
||||
return {"success": True}
|
||||
from fastapi import HTTPException
|
||||
raise HTTPException(status_code=400, detail="Failed to create shared link")
|
||||
http_session = await get_http_session()
|
||||
immich = make_immich_provider(http_session, provider)
|
||||
success = await immich.client.create_shared_link(album_id)
|
||||
if success:
|
||||
return {"success": True}
|
||||
raise HTTPException(status_code=400, detail="Failed to create shared link")
|
||||
|
||||
from fastapi import HTTPException
|
||||
raise HTTPException(status_code=400, detail="Provider type does not support shared links")
|
||||
|
||||
|
||||
@@ -580,7 +453,7 @@ async def _get_user_provider(
|
||||
session: AsyncSession, provider_id: int, user_id: int
|
||||
) -> ServiceProvider:
|
||||
"""Get a provider owned by the user, or raise 404."""
|
||||
provider = await session.get(ServiceProvider, provider_id)
|
||||
if not provider or provider.user_id != user_id:
|
||||
raise HTTPException(status_code=404, detail="Provider not found")
|
||||
return provider
|
||||
return await get_owned_entity(
|
||||
session, ServiceProvider, provider_id, user_id,
|
||||
not_found_msg="Provider not found",
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user