fix(amber): hold result export to the same rules the export dialog applies - #7994
fix(amber): hold result export to the same rules the export dialog applies#7994aicam wants to merge 1 commit into
Conversation
…plies Both export endpoints build their work out of the request body, and the only rule either applied was workflow read access. The per-operator dataset restriction the downloadability endpoint computes for the UI, and the computing unit the execution lookup keys on, were left to the client. Both endpoints now share one validateUserCanExportResult: workflow read access, access to the named computing unit, and no requested operator carrying data out of a non-downloadable dataset the caller does not own. The two new refusals answer 403 — the session is valid, and the frontend's interceptor logs a user out on any 401. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qVdnpDpXZePfB9Zfa3noz
Automated Reviewer SuggestionsBased on the
|
Backport auto-label reportThis
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 558 | 0.34 | 17,037/22,915/22,915 us | 🔴 +6.0% / 🔴 +65.5% |
| 🟢 | bs=100 sw=10 sl=64 | 1,218 | 0.744 | 81,593/91,843/91,843 us | 🟢 -11.2% / 🟢 -10.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,437 | 0.877 | 693,026/758,460/758,460 us | ⚪ within ±5% / 🟢 +19.9% |
Baseline details
Latest main 5de9bf9 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 558 tuples/sec | 585 tuples/sec | 894.86 tuples/sec | -4.6% | -37.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.34 MB/s | 0.357 MB/s | 0.546 MB/s | -4.8% | -37.7% |
| bs=10 sw=10 sl=64 | p50 | 17,037 us | 16,069 us | 11,139 us | +6.0% | +53.0% |
| bs=10 sw=10 sl=64 | p95 | 22,915 us | 24,033 us | 13,843 us | -4.7% | +65.5% |
| bs=10 sw=10 sl=64 | p99 | 22,915 us | 24,033 us | 16,839 us | -4.7% | +36.1% |
| bs=100 sw=10 sl=64 | throughput | 1,218 tuples/sec | 1,184 tuples/sec | 1,166 tuples/sec | +2.9% | +4.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.744 MB/s | 0.723 MB/s | 0.711 MB/s | +2.9% | +4.6% |
| bs=100 sw=10 sl=64 | p50 | 81,593 us | 80,628 us | 87,509 us | +1.2% | -6.8% |
| bs=100 sw=10 sl=64 | p95 | 91,843 us | 103,461 us | 93,675 us | -11.2% | -2.0% |
| bs=100 sw=10 sl=64 | p99 | 91,843 us | 103,461 us | 102,153 us | -11.2% | -10.1% |
| bs=1000 sw=10 sl=64 | throughput | 1,437 tuples/sec | 1,460 tuples/sec | 1,198 tuples/sec | -1.6% | +19.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.877 MB/s | 0.891 MB/s | 0.731 MB/s | -1.6% | +19.9% |
| bs=1000 sw=10 sl=64 | p50 | 693,026 us | 687,406 us | 859,766 us | +0.8% | -19.4% |
| bs=1000 sw=10 sl=64 | p95 | 758,460 us | 770,591 us | 902,897 us | -1.6% | -16.0% |
| bs=1000 sw=10 sl=64 | p99 | 758,460 us | 770,591 us | 937,957 us | -1.6% | -19.1% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,358.72,200,128000,558,0.340,17037.49,22915.06,22915.06
1,100,10,64,20,1641.46,2000,1280000,1218,0.744,81593.41,91842.78,91842.78
2,1000,10,64,20,13915.41,20000,12800000,1437,0.877,693026.49,758459.85,758459.85
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7994 +/- ##
=========================================
Coverage 93.21% 93.21%
- Complexity 4664 4670 +6
=========================================
Files 1175 1175
Lines 47646 47665 +19
Branches 5319 5324 +5
=========================================
+ Hits 44413 44433 +20
+ Misses 1752 1750 -2
- Partials 1481 1482 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What changes were proposed in this PR?
The two result-export endpoints build their work entirely out of the request body —
workflowId,computingUnitId, and the operator list — and the only thing either of them checks today is that the caller has read access to the workflow.Two rules that the export dialog applies in the browser never reach the server:
GET /executions/{wid}/result/downloadabilitycomputes, per operator, which datasets markedis_downloadable = falsefeed it, propagated downstream through the workflow graph. The dialog greys those operators out. Neither export endpoint consults the same map, so the restriction holds only for as long as the client cooperates.getLatestExecutionIDselects on(wid, cuid)alone, so the unit id is otherwise just another body field.So the same request is answered differently depending on which client sends it, and the dataset owner's downloadability setting is enforced in exactly one of the two places that act on it.
Both endpoints now share one
validateUserCanExportResult, which keeps the existing workflow read-access check and adds the two missing rules:ComputingUnitAccess.getComputingUnitAccess— a unit shared with the caller still exports, an unrelated one does not;The two new refusals answer
403rather than the401the workflow check returns. The caller's session is valid in both new cases, and the frontend'sUnauthorizedHttpInterceptortreats any401on an authenticated request as an expired session and logs the user out — the wrong outcome for a live session that merely asked for something it is not entitled to.Any related issues, documentation, discussions?
No issue. The per-operator restriction map and the dialog that consumes it already exist; this PR only makes the endpoints behind the dialog apply the same rule.
How was this PR tested?
WorkflowExecutionsResourceSpec, six new cases covering both endpoints:403and the message, for a unit owned by another userREADon still exports — the predicate is access, not ownership403naming the blocking dataset; the local case asks for the downstream operator, so the restriction's propagation along the links is covered tooTwo existing cases named computing unit
0, which no unit can have; they now insert a real unit so they still reach the code they were written for.67 before, 73 after.
Test/scalafmtCheckandTest/scalafix --checkpass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)