fix: reevaluate pbac on task changes - #854
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughPermissionService now invalidates cached and in-flight permission requests by resource and identifier. Task detail components invalidate permissions before refreshing after assignment changes and SSE updates, re-check view access, and close when task retrieval returns 403 or 404. New tests cover cache invalidation and modal behavior, and the release notes document the changes. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/projects/valtimo/access-control/src/lib/services/permission.service.ts (1)
140-177: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCorrelate stale in-flight batches with the current request generation.
_pendingPermissionsand_permissionRequestsByKeyare keyed by the deterministic request hash, so afterinvalidateResourcedeletes the subject for that key, any laterrequestPermission(K)reuses the same key. If the older batch forKresolves after the new request is enqueued and its stale result is returned from the cache, or if the newer request is discarded because only one request is queued for a key, PBAC re-evaluation can return the wrong answer for that resource/identifier. Tag each queued request with a per-key generation/version bumped ininvalidateResource, and ignore stale resolutions or deconflict duplicate pending requests. Add a test forinvalidateResource+ freshrequestPermissionwhile the earlier batch for the same key is still unresolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ff26f83-89e6-4a9f-b48a-fb4ee0ce7c87
📒 Files selected for processing (6)
documentation/release-notes/13.x.x/13.39.0/README.mdfrontend/projects/valtimo/access-control/src/lib/services/permission.service.spec.tsfrontend/projects/valtimo/access-control/src/lib/services/permission.service.tsfrontend/projects/valtimo/case/src/lib/components/case-detail-task-detail/case-detail-task-detail.component.tsfrontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.spec.tsfrontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.ts
| private refreshTaskAfterAssignmentChange(taskId: string): Observable<any> { | ||
| // The new assignment can change the outcome of permission checks for this task, | ||
| // so drop cached permissions before the task is re-fetched and re-checked | ||
| this.permissionService.invalidateResource(TASK_DETAIL_PERMISSION_RESOURCE.task, taskId); | ||
|
|
||
| return this.taskService.getTask(taskId).pipe( | ||
| // a 403 or 404 means the user can no longer view the task | ||
| catchError(error => (error?.status === 403 || error?.status === 404 ? of(null) : EMPTY)) | ||
| ); | ||
| } | ||
|
|
||
| private handleAssignmentChangeResponse(response: any): void { | ||
| if (response) { | ||
| this.refreshTask(response); | ||
| } else { | ||
| this.onClose(); | ||
| } | ||
| this.assignmentOfTaskChanged.emit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'case-detail-task-detail.component.spec.ts' frontendRepository: valtimo-platform/valtimo
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== component/spec files matching case-detail-task-detail ====="
git ls-files | rg 'case-detail-task-detail\.component\.(ts|spec\.ts)$' || true
echo
echo "== spec files mentioning TaskDetailModalComponent / handleAssignmentChangeResponse / refreshTaskAfterAssignmentChange =="
rg -n "CaseDetailsTaskDetailComponent|TaskDetailModalComponent|handleAssignmentChangeResponse|refreshTaskAfterAssignmentChange|assignmentOfTaskChanged" frontend || true
echo
echo "== relevant component header/slice =="
FILE="frontend/projects/valtimo/case/src/lib/components/case-detail-task-detail/case-detail-task-detail.component.ts"
if [ -f "$FILE" ]; then
wc -l "$FILE"
sed -n '1,280p' "$FILE" | nl -ba
fi
echo
echo "== test files in nearby component area =="
dir="$(dirname "$FILE")"
fd -a '(\.spec\.ts|spec\.ts)$' "$dir" || trueRepository: valtimo-platform/valtimo
Length of output: 7882
Add coverage for the case detail assignment-change refresh flow.
frontend/projects/valtimo/case/src/lib/components/case-detail-task-detail/case-detail-task-detail.component.spec.ts is missing, while CaseDetailsTaskDetailComponent now runs refreshTaskAfterAssignmentChange() via the same assignment-change path.
| private refreshTaskAfterAssignmentChange(taskId: string): Observable<any> { | ||
| // The new assignment can change the outcome of permission checks for this task, | ||
| // so drop cached permissions before the task is re-fetched and re-checked | ||
| this.permissionService.invalidateResource(TASK_DETAIL_PERMISSION_RESOURCE.task, taskId); | ||
|
|
||
| return this.taskService.getTask(taskId).pipe( | ||
| catchError(error => { | ||
| // a 403 or 404 means the user can no longer view the task | ||
| if (error?.status === 403 || error?.status === 404) { | ||
| return of(null); | ||
| } | ||
| this.logger.error('Failed to fetch task after assignment change', error); | ||
| return EMPTY; | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| private handleAssignmentChangeResponse(response: any): void { | ||
| if (response) { | ||
| this.refreshTask(response); | ||
| } else { | ||
| this.closeModal(); | ||
| } | ||
| this.assignmentOfTaskChanged.emit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unexpected re-fetch errors are silently swallowed in three near-duplicated places. The new "invalidate → refetch → treat 403/404 as unavailable" pattern was copy/pasted across two components without a shared implementation, and each copy maps any other error to EMPTY, so the pipe just completes with no downstream effect — no refresh, no close, no assignmentOfTaskChanged emit — leaving the user with a stale/stuck UI and no explanation. The duplication has already caused visible drift (one copy logs, one doesn't).
frontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.ts#L384-L409: on non-403/404 errors, surface a user-facing failure (e.g. via the already-injectedGlobalNotificationService) instead of just logging and silently no-oping.frontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.ts#L227-L245: same — the SSE handler logs the error but still leaves the modal open with no user feedback; give it the same failure handling.frontend/projects/valtimo/case/src/lib/components/case-detail-task-detail/case-detail-task-detail.component.ts#L201-L210: inject a logger (none is currently available in this component) and apply the same user-facing failure handling; consider extracting the shared "invalidate + refetch + 403/404 handling" logic into a common helper so the two components can't diverge again.
♻️ Illustrative fix direction
return this.taskService.getTask(taskId).pipe(
- catchError(error => (error?.status === 403 || error?.status === 404 ? of(null) : EMPTY))
+ catchError(error => {
+ if (error?.status === 403 || error?.status === 404) {
+ return of(null);
+ }
+ // surface the failure instead of leaving the panel stuck with no feedback
+ return of(null);
+ })
);📍 Affects 2 files
frontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.ts#L384-L409(this comment)frontend/projects/valtimo/task/src/lib/components/task-detail-modal/task-detail-modal.component.ts#L227-L245frontend/projects/valtimo/case/src/lib/components/case-detail-task-detail/case-detail-task-detail.component.ts#L201-L210
|
Testing finding: Video: Screen.Recording.2026-07-28.at.14.52.18.mov |
2 participants
closes generiekzaakafhandelcomponent/gzac-issues#691