fix: apply code review security and robustness fixes
- Add require_engineer_or_admin to POST/PUT/DELETE in target_lists.py (blocks viewers from write ops) - Add require_engineer_or_admin to POST/PATCH in maintenance_schedules.py (blocks viewers from write ops) - Add team ownership guard in batch_launch_sessions after active/published checks (Fix 2) - Wrap scheduler.remove_job in try/except for SchedulerNotRunningError and JobLookupError (Fix 3) - Recompute next_run_at when is_active flips to True, capturing was_active before update (Fix 4) - Add optional batch_id and target_label fields to Session type; remove unsafe cast in MaintenanceFlowDetailPage.tsx (Fix 5) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -8,7 +8,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
|
||||
from croniter import croniter
|
||||
import pytz
|
||||
|
||||
from app.api.deps import get_current_active_user, get_db
|
||||
from app.api.deps import get_current_active_user, get_db, require_engineer_or_admin
|
||||
from app.models.maintenance_schedule import MaintenanceSchedule
|
||||
from app.models.tree import Tree
|
||||
from app.models.user import User
|
||||
@@ -47,6 +47,7 @@ async def create_schedule(
|
||||
data: MaintenanceScheduleCreate,
|
||||
current_user: Annotated[User, Depends(get_current_active_user)],
|
||||
db: Annotated[AsyncSession, Depends(get_db)],
|
||||
_: None = Depends(require_engineer_or_admin),
|
||||
):
|
||||
"""Create a cron schedule for a maintenance flow. One per flow."""
|
||||
# Verify user's team owns the tree
|
||||
@@ -108,6 +109,7 @@ async def update_schedule(
|
||||
data: MaintenanceScheduleUpdate,
|
||||
current_user: Annotated[User, Depends(get_current_active_user)],
|
||||
db: Annotated[AsyncSession, Depends(get_db)],
|
||||
_: None = Depends(require_engineer_or_admin),
|
||||
):
|
||||
"""Update a schedule (disable, change cron, change timezone, change target list)."""
|
||||
result = await db.execute(
|
||||
@@ -121,6 +123,7 @@ async def update_schedule(
|
||||
await _get_tree_or_403(schedule.tree_id, current_user, db)
|
||||
|
||||
update_fields = data.model_fields_set
|
||||
was_active = schedule.is_active
|
||||
if "cron_expression" in update_fields and data.cron_expression is not None:
|
||||
schedule.cron_expression = data.cron_expression
|
||||
if "timezone" in update_fields and data.timezone is not None:
|
||||
@@ -130,8 +133,9 @@ async def update_schedule(
|
||||
if "is_active" in update_fields and data.is_active is not None:
|
||||
schedule.is_active = data.is_active
|
||||
|
||||
# Recompute next_run_at if schedule timing changed
|
||||
if "cron_expression" in update_fields or "timezone" in update_fields:
|
||||
# Recompute next_run_at if schedule timing changed or schedule is being re-activated
|
||||
reactivating = "is_active" in update_fields and data.is_active is True and not was_active
|
||||
if "cron_expression" in update_fields or "timezone" in update_fields or reactivating:
|
||||
try:
|
||||
schedule.next_run_at = _compute_next_run(schedule.cron_expression, schedule.timezone)
|
||||
except (ValueError, KeyError) as e:
|
||||
|
||||
Reference in New Issue
Block a user