fix: address Task 6 quality review — rename helper, restore 403 for intra-account, add docs test
- Rename _get_tree_or_403 → _get_tree_or_404 in maintenance_schedules.py (function now raises 404, old name was misleading) - Restore HTTP 403 for intra-account permission failures in update_tree: same-account users who can see a tree but can't edit it got 404 (wrong); only cross-account lookups should return 404 to avoid confirming existence - Apply same 403/404 distinction to update_tree_visibility - Add test: get_documentation must return 404 for cross-user session access - Add comment documenting owner-only design for documentation endpoints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -921,7 +921,9 @@ async def get_documentation(
|
||||
db: Annotated[AsyncSession, Depends(get_db)],
|
||||
):
|
||||
"""Get auto-generated documentation for a session."""
|
||||
# Verify session ownership — return 404 (not 403) to avoid confirming existence.
|
||||
# Verify session ownership — owner only. Documentation endpoints require direct
|
||||
# ownership; escalated_to_id / picked_up_by handlers use get_session (read-only).
|
||||
# This is consistent with stream_documentation which has the same owner-only check.
|
||||
result = await db.execute(
|
||||
select(AISession).where(
|
||||
AISession.id == session_id,
|
||||
|
||||
@@ -29,8 +29,8 @@ def _compute_next_run(cron_expression: str, tz_name: str) -> datetime:
|
||||
return cron.get_next(datetime).astimezone(timezone.utc)
|
||||
|
||||
|
||||
async def _get_tree_or_403(tree_id: UUID, current_user: User, db: AsyncSession) -> "Tree":
|
||||
"""Fetch tree and verify the current user's team owns it."""
|
||||
async def _get_tree_or_404(tree_id: UUID, current_user: User, db: AsyncSession) -> "Tree":
|
||||
"""Fetch tree and verify the current user's team owns it. Raises 404 if not found or access denied."""
|
||||
result = await db.execute(select(Tree).where(Tree.id == tree_id))
|
||||
tree = result.scalar_one_or_none()
|
||||
if not tree:
|
||||
@@ -51,7 +51,7 @@ async def create_schedule(
|
||||
):
|
||||
"""Create a cron schedule for a maintenance flow. One per flow."""
|
||||
# Verify user's team owns the tree
|
||||
tree = await _get_tree_or_403(data.tree_id, current_user, db)
|
||||
tree = await _get_tree_or_404(data.tree_id, current_user, db)
|
||||
if tree.tree_type != "maintenance":
|
||||
raise HTTPException(status_code=400, detail="Schedules are only supported for maintenance flows")
|
||||
|
||||
@@ -94,7 +94,7 @@ async def get_schedule_for_tree(
|
||||
):
|
||||
"""Get the schedule for a specific maintenance flow."""
|
||||
# Verify user's team owns the tree before returning schedule data
|
||||
await _get_tree_or_403(tree_id, current_user, db)
|
||||
await _get_tree_or_404(tree_id, current_user, db)
|
||||
|
||||
result = await db.execute(
|
||||
select(MaintenanceSchedule).where(MaintenanceSchedule.tree_id == tree_id)
|
||||
@@ -122,7 +122,7 @@ async def update_schedule(
|
||||
raise HTTPException(status_code=404, detail="Schedule not found")
|
||||
|
||||
# Verify user's team owns the tree this schedule belongs to
|
||||
await _get_tree_or_403(schedule.tree_id, current_user, db)
|
||||
await _get_tree_or_404(schedule.tree_id, current_user, db)
|
||||
|
||||
update_fields = data.model_fields_set
|
||||
was_active = schedule.is_active
|
||||
|
||||
@@ -610,6 +610,14 @@ async def update_tree(
|
||||
)
|
||||
|
||||
if not can_edit_tree(current_user, tree):
|
||||
# If the user can see this tree (same account, team visibility), give a 403 with
|
||||
# a clear message — returning 404 here would be confusing since GET returns 200.
|
||||
# For truly inaccessible trees (cross-account), return 404 to avoid confirming existence.
|
||||
if can_access_tree(current_user, tree):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail="You do not have permission to edit this flow"
|
||||
)
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_404_NOT_FOUND,
|
||||
detail="Tree not found"
|
||||
@@ -1144,6 +1152,14 @@ async def update_tree_visibility(
|
||||
)
|
||||
|
||||
if not can_edit_tree(current_user, tree):
|
||||
# If the user can see this tree (same account, team visibility), give a 403 with
|
||||
# a clear message — returning 404 here would be confusing since GET returns 200.
|
||||
# For truly inaccessible trees (cross-account), return 404 to avoid confirming existence.
|
||||
if can_access_tree(current_user, tree):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail="You do not have permission to edit this flow"
|
||||
)
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_404_NOT_FOUND,
|
||||
detail="Tree not found"
|
||||
|
||||
Reference in New Issue
Block a user