Fix QA findings from performance, pentest, and code review
Perf-1: Eliminate duplicate permission query on task update. get_effective_task_permission now returns (effective, project_level) tuple so the SEC-P02 allowlist check reuses the project-level permission from the first call instead of querying again. Perf-2: Memoize member permission lookup in ProjectDetail. Replace 3 inline acceptedMembers.find() calls with useMemo-derived myPermission and canEditTasks. S-06: Pass members/currentUserId/ownerId/canAssign to mobile TaskDetailPanel (was missing — AssignmentPicker never appeared on mobile). S-08: Add missing selectinload(TaskComment.user) to subtask comments chain in _task_load_options. Subtask comment author_name was always null. W-01: useDeltaPoll stores queryKeyToInvalidate in a ref to prevent infinite re-render if caller passes inline array literal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
e0a5f4855f
commit
dd637bdc84
@ -54,7 +54,7 @@ def _task_load_options():
|
|||||||
"""All load options needed for task responses."""
|
"""All load options needed for task responses."""
|
||||||
return [
|
return [
|
||||||
selectinload(ProjectTask.comments).selectinload(TaskComment.user),
|
selectinload(ProjectTask.comments).selectinload(TaskComment.user),
|
||||||
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.comments),
|
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.comments).selectinload(TaskComment.user),
|
||||||
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.subtasks),
|
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.subtasks),
|
||||||
selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user),
|
selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user),
|
||||||
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user),
|
selectinload(ProjectTask.subtasks).selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user),
|
||||||
@ -375,7 +375,7 @@ async def update_project_task(
|
|||||||
current_user: User = Depends(get_current_user)
|
current_user: User = Depends(get_current_user)
|
||||||
):
|
):
|
||||||
"""Update a project task. Permission checked at project and task level."""
|
"""Update a project task. Permission checked at project and task level."""
|
||||||
perm = await get_effective_task_permission(db, current_user.id, task_id, project_id)
|
perm, project_perm = await get_effective_task_permission(db, current_user.id, task_id, project_id)
|
||||||
if perm is None:
|
if perm is None:
|
||||||
raise HTTPException(status_code=404, detail="Project not found")
|
raise HTTPException(status_code=404, detail="Project not found")
|
||||||
if perm == "read_only":
|
if perm == "read_only":
|
||||||
@ -395,7 +395,6 @@ async def update_project_task(
|
|||||||
update_data = task_update.model_dump(exclude_unset=True)
|
update_data = task_update.model_dump(exclude_unset=True)
|
||||||
|
|
||||||
# SEC-P02: Assignees (non-owner, non-project-member with create_modify) restricted to content fields
|
# SEC-P02: Assignees (non-owner, non-project-member with create_modify) restricted to content fields
|
||||||
project_perm = await get_project_permission(db, project_id, current_user.id)
|
|
||||||
if project_perm not in ("owner", "create_modify"):
|
if project_perm not in ("owner", "create_modify"):
|
||||||
# This user's create_modify comes from task assignment — enforce allowlist
|
# This user's create_modify comes from task assignment — enforce allowlist
|
||||||
disallowed = set(update_data.keys()) - ASSIGNEE_ALLOWED_FIELDS
|
disallowed = set(update_data.keys()) - ASSIGNEE_ALLOWED_FIELDS
|
||||||
|
|||||||
@ -110,19 +110,19 @@ async def validate_project_connections(
|
|||||||
|
|
||||||
async def get_effective_task_permission(
|
async def get_effective_task_permission(
|
||||||
db: AsyncSession, user_id: int, task_id: int, project_id: int
|
db: AsyncSession, user_id: int, task_id: int, project_id: int
|
||||||
) -> str | None:
|
) -> tuple[str | None, str | None]:
|
||||||
"""
|
"""
|
||||||
Returns effective permission for a specific task:
|
Returns (effective_permission, project_level_permission) for a specific task.
|
||||||
1. Get project-level permission (owner/create_modify/read_only)
|
1. Get project-level permission (owner/create_modify/read_only)
|
||||||
2. If user is assigned to THIS task → max(project_perm, create_modify)
|
2. If user is assigned to THIS task → max(project_perm, create_modify)
|
||||||
3. If task has parent and user assigned to PARENT → same as above
|
3. If task has parent and user assigned to PARENT → same as above
|
||||||
4. Return effective permission
|
4. Return (effective, project_level)
|
||||||
"""
|
"""
|
||||||
project_perm = await get_project_permission(db, project_id, user_id)
|
project_perm = await get_project_permission(db, project_id, user_id)
|
||||||
if project_perm is None:
|
if project_perm is None:
|
||||||
return None
|
return None, None
|
||||||
if project_perm == "owner":
|
if project_perm == "owner":
|
||||||
return "owner"
|
return "owner", "owner"
|
||||||
|
|
||||||
# Check direct assignment on this task
|
# Check direct assignment on this task
|
||||||
task_result = await db.execute(
|
task_result = await db.execute(
|
||||||
@ -130,7 +130,7 @@ async def get_effective_task_permission(
|
|||||||
)
|
)
|
||||||
task_row = task_result.one_or_none()
|
task_row = task_result.one_or_none()
|
||||||
if not task_row:
|
if not task_row:
|
||||||
return project_perm
|
return project_perm, project_perm
|
||||||
|
|
||||||
parent_task_id = task_row[0]
|
parent_task_id = task_row[0]
|
||||||
|
|
||||||
@ -148,10 +148,10 @@ async def get_effective_task_permission(
|
|||||||
if assignment_result.scalar_one_or_none() is not None:
|
if assignment_result.scalar_one_or_none() is not None:
|
||||||
# Assignment grants at least create_modify
|
# Assignment grants at least create_modify
|
||||||
if PERMISSION_RANK.get(project_perm, 0) >= PERMISSION_RANK["create_modify"]:
|
if PERMISSION_RANK.get(project_perm, 0) >= PERMISSION_RANK["create_modify"]:
|
||||||
return project_perm
|
return project_perm, project_perm
|
||||||
return "create_modify"
|
return "create_modify", project_perm
|
||||||
|
|
||||||
return project_perm
|
return project_perm, project_perm
|
||||||
|
|
||||||
|
|
||||||
async def ensure_auto_membership(
|
async def ensure_auto_membership(
|
||||||
|
|||||||
@ -165,7 +165,12 @@ export default function ProjectDetail() {
|
|||||||
},
|
},
|
||||||
enabled: !!id,
|
enabled: !!id,
|
||||||
});
|
});
|
||||||
const acceptedMembers = members.filter((m) => m.status === 'accepted');
|
const acceptedMembers = useMemo(() => members.filter((m) => m.status === 'accepted'), [members]);
|
||||||
|
const myPermission = useMemo(
|
||||||
|
() => acceptedMembers.find((m) => m.user_id === currentUserId)?.permission ?? null,
|
||||||
|
[acceptedMembers, currentUserId]
|
||||||
|
);
|
||||||
|
const canEditTasks = isOwner || myPermission === 'create_modify';
|
||||||
|
|
||||||
const toggleTaskMutation = useMutation({
|
const toggleTaskMutation = useMutation({
|
||||||
mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => {
|
mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => {
|
||||||
@ -425,7 +430,7 @@ export default function ProjectDetail() {
|
|||||||
{/* Permission badge for non-owners */}
|
{/* Permission badge for non-owners */}
|
||||||
{isShared && (
|
{isShared && (
|
||||||
<Badge className="shrink-0 bg-blue-500/10 text-blue-400 border-0">
|
<Badge className="shrink-0 bg-blue-500/10 text-blue-400 border-0">
|
||||||
{acceptedMembers.find((m) => m.user_id === currentUserId)?.permission === 'create_modify' ? 'Editor' : 'Viewer'}
|
{myPermission === 'create_modify' ? 'Editor' : 'Viewer'}
|
||||||
</Badge>
|
</Badge>
|
||||||
)}
|
)}
|
||||||
{canManageProject && (
|
{canManageProject && (
|
||||||
@ -602,7 +607,7 @@ export default function ProjectDetail() {
|
|||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
{(isOwner || acceptedMembers.find((m) => m.user_id === currentUserId)?.permission === 'create_modify') && (
|
{canEditTasks && (
|
||||||
<Button size="sm" onClick={() => openTaskForm(null, null)}>
|
<Button size="sm" onClick={() => openTaskForm(null, null)}>
|
||||||
<Plus className="mr-2 h-3.5 w-3.5" />
|
<Plus className="mr-2 h-3.5 w-3.5" />
|
||||||
Add Task
|
Add Task
|
||||||
@ -713,7 +718,7 @@ export default function ProjectDetail() {
|
|||||||
members={acceptedMembers}
|
members={acceptedMembers}
|
||||||
currentUserId={currentUserId}
|
currentUserId={currentUserId}
|
||||||
ownerId={project?.user_id ?? 0}
|
ownerId={project?.user_id ?? 0}
|
||||||
canAssign={isOwner || acceptedMembers.some(m => m.user_id === currentUserId && m.permission === 'create_modify')}
|
canAssign={canEditTasks}
|
||||||
onDelete={handleDeleteTask}
|
onDelete={handleDeleteTask}
|
||||||
onAddSubtask={(parentId) => openTaskForm(null, parentId)}
|
onAddSubtask={(parentId) => openTaskForm(null, parentId)}
|
||||||
onClose={() => setSelectedTaskId(null)}
|
onClose={() => setSelectedTaskId(null)}
|
||||||
|
|||||||
@ -22,6 +22,10 @@ export function useDeltaPoll(
|
|||||||
) {
|
) {
|
||||||
const queryClient = useQueryClient();
|
const queryClient = useQueryClient();
|
||||||
const sinceRef = useRef<string>(toLocalDatetime());
|
const sinceRef = useRef<string>(toLocalDatetime());
|
||||||
|
// Store key in ref to decouple from useEffect deps (prevents infinite loop
|
||||||
|
// if caller passes an inline array literal instead of a memoized one)
|
||||||
|
const keyRef = useRef(queryKeyToInvalidate);
|
||||||
|
keyRef.current = queryKeyToInvalidate;
|
||||||
|
|
||||||
const { data } = useQuery<PollResponse>({
|
const { data } = useQuery<PollResponse>({
|
||||||
queryKey: ['delta-poll', endpoint],
|
queryKey: ['delta-poll', endpoint],
|
||||||
@ -39,10 +43,10 @@ export function useDeltaPoll(
|
|||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (data?.has_changes) {
|
if (data?.has_changes) {
|
||||||
queryClient.invalidateQueries({ queryKey: queryKeyToInvalidate });
|
queryClient.invalidateQueries({ queryKey: keyRef.current });
|
||||||
sinceRef.current = toLocalDatetime();
|
sinceRef.current = toLocalDatetime();
|
||||||
}
|
}
|
||||||
}, [data, queryClient, queryKeyToInvalidate]);
|
}, [data, queryClient]);
|
||||||
|
|
||||||
const updateSince = (timestamp: string) => {
|
const updateSince = (timestamp: string) => {
|
||||||
sinceRef.current = timestamp;
|
sinceRef.current = timestamp;
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user