Skip to content

fix(amber): hold result export to the same rules the export dialog applies - #7994

Open
aicam wants to merge 1 commit into
apache:mainfrom
aicam:fix/result-export-authz
Open

fix(amber): hold result export to the same rules the export dialog applies#7994
aicam wants to merge 1 commit into
apache:mainfrom
aicam:fix/result-export-authz

Conversation

@aicam

@aicam aicam commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

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/downloadability computes, per operator, which datasets marked is_downloadable = false feed 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.
  • The dialog can only name the computing unit the workspace is attached to, but getLatestExecutionID selects 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:

  1. read access to the workflow (unchanged);
  2. read access to the computing unit named in the request, through ComputingUnitAccess.getComputingUnitAccess — a unit shared with the caller still exports, an unrelated one does not;
  3. no requested operator carries data out of a non-downloadable dataset the caller does not own, using the same map the downloadability endpoint already returns to the UI.

The two new refusals answer 403 rather than the 401 the workflow check returns. The caller's session is valid in both new cases, and the frontend's UnauthorizedHttpInterceptor treats any 401 on 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:

Case What it pins
computing unit the caller cannot reach (local + dataset) 403 and the message, for a unit owned by another user
computing unit shared with the caller a unit someone else owns but granted the caller READ on still exports — the predicate is access, not ownership
operator fed by a non-downloadable dataset (local + dataset) 403 naming the blocking dataset; the local case asks for the downstream operator, so the restriction's propagation along the links is covered too
unrestricted operator of a restricted workflow one restriction in a workflow does not close down the whole workflow

Two 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.

STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.user.workflow.WorkflowExecutionsResourceSpec"
[info] Total number of tests run: 73
[info] Tests: succeeded 73, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

67 before, 73 after.

Test/scalafmtCheck and Test/scalafix --check pass.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

…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
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @aglinxinyuan, @tanishqgandhi1908
    You can notify them by mentioning @aglinxinyuan, @tanishqgandhi1908 in a comment.

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 26, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 26, 2026 17:58
@github-actions

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 1 worse · ⚪ 12 noise (<±5%) · 0 without baseline

Compared against main 5de9bf9 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.21%. Comparing base (5de9bf9) to head (8090b24).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ard/user/workflow/WorkflowExecutionsResource.scala 86.66% 2 Missing and 2 partials ⚠️
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     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 5de9bf9
agent-service 99.32% <ø> (ø) Carriedforward from 5de9bf9
amber 89.33% <86.66%> (+0.01%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 5de9bf9
config-service 86.73% <ø> (ø) Carriedforward from 5de9bf9
file-service 86.70% <ø> (ø) Carriedforward from 5de9bf9
frontend 95.67% <ø> (ø) Carriedforward from 5de9bf9
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 5de9bf9
pyamber 97.74% <ø> (ø) Carriedforward from 5de9bf9
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 5de9bf9

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix release/v1.2 back porting to release/v1.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants