Skip to content

fix(amber): require auth on retrieveWorkflowRuntimeStatistics - #7014

Merged
mengw15 merged 3 commits into
apache:mainfrom
mengw15:fix/6977-stats-endpoint-auth
Jul 29, 2026
Merged

fix(amber): require auth on retrieveWorkflowRuntimeStatistics#7014
mengw15 merged 3 commits into
apache:mainfrom
mengw15:fix/6977-stats-endpoint-auth

Conversation

@mengw15

@mengw15 mengw15 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

retrieveWorkflowRuntimeStatistics (GET /executions/{wid}/stats/{eid}) was the only endpoint in WorkflowExecutionsResource declaring neither @RolesAllowed nor an @Auth parameter — every other handler in the class carries @RolesAllowed(Array("REGULAR", "ADMIN")) + @Auth sessionUser, and there is no class-level annotation to fall back on. Auth is enforced per-method via RolesAllowedDynamicFeature, so this one handler was reachable without authentication.

  • Add @RolesAllowed(Array("REGULAR", "ADMIN")) and an @Auth sessionUser: SessionUser parameter, matching the sibling endpoints exactly.
  • Add an auth-annotation audit test to WorkflowExecutionsResourceSpec: it reflects over every @GET/@PUT/@POST/@DELETE handler in the class and asserts each declares @RolesAllowed and takes an @Auth parameter — so any future endpoint added without auth turns the suite red instead of shipping unprotected. exportResultToLocal is explicitly exempted with a comment: it serves a browser form-submit download (which can't carry an Authorization header), so its JWT arrives as a form field and is verified in-method via JwtParser.parseToken, including the role check.

The frontend is unaffected: JwtModule attaches the Authorization header to every request (app.module.ts), and the same service already calls sibling endpoints that require auth.

Any related issues, documentation, discussions?

Closes #6977.

How was this PR tested?

  • The audit test fails before the fix, naming exactly retrieveWorkflowRuntimeStatistics as the offender, and passes after — the failing run is the repro for the issue. (Its first run also flagged exportResultToLocal; on inspection that endpoint authenticates manually by design — see above — which is why the exemption is explicit and documented rather than silent.)
  • Full WorkflowExecutionsResourceSpec run locally: all tests pass; WorkflowExecutionService/scalafmtCheck (main + Test) passes.
  • Verified no other caller of the method exists (the added parameter breaks no call site), and that all sibling endpoints use the identical annotation pattern being applied here.

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

Generated-by: Claude Code (claude-opus-4-8)

retrieveWorkflowRuntimeStatistics (GET /executions/{wid}/stats/{eid}) was
the only endpoint in WorkflowExecutionsResource declaring neither
@RolesAllowed nor an @Auth parameter, and the class has no class-level
annotation to fall back on -- auth is enforced per-method via
RolesAllowedDynamicFeature, so the handler was reachable without
authentication. Add @RolesAllowed(REGULAR, ADMIN) and an @Auth sessionUser
parameter, matching every sibling endpoint.

Add an auth-annotation audit test that reflects over the class's
@GET/@PUT/@POST/@delete handlers and asserts each declares @RolesAllowed
and takes an @Auth parameter, so a future endpoint added without auth turns
the suite red. exportResultToLocal is explicitly exempted: it serves a
browser form-submit download that cannot carry an Authorization header, so
its JWT arrives as a form field and is verified in-method via
JwtParser.parseToken, including the role check.

The frontend is unaffected: JwtModule attaches the Authorization header to
every request, and the same service already calls sibling endpoints that
require auth.

Closes apache#6977.
@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: @Yicong-Huang, @Neilk1021
    You can notify them by mentioning @Yicong-Huang, @Neilk1021 in a comment.

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Jul 29, 2026
@mengw15
mengw15 requested a review from Copilot July 29, 2026 05:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Amber dashboard workflow-executions REST resource by ensuring the retrieveWorkflowRuntimeStatistics endpoint is protected by the same authentication/role requirements as its sibling endpoints, and adds a test to prevent future unauthenticated endpoints from being introduced in WorkflowExecutionsResource.

Changes:

  • Add @RolesAllowed(Array("REGULAR", "ADMIN")) and an @Auth sessionUser: SessionUser parameter to GET /executions/{wid}/stats/{eid} (retrieveWorkflowRuntimeStatistics).
  • Add a reflective audit test in WorkflowExecutionsResourceSpec to assert every HTTP handler declares @RolesAllowed and includes an @Auth parameter (with an explicit exemption for exportResultToLocal due to its manual token validation flow).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala Secures the previously unprotected runtime-stats endpoint by adding declarative auth/roles.
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala Adds an “auth-annotation audit” test to prevent future unauthenticated endpoints in this resource.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>

@aglinxinyuan aglinxinyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 812f177 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 425 0.26 22,644/31,967/31,967 us 🔴 +21.1% / 🔴 +102.3%
🟢 bs=100 sw=10 sl=64 944 0.576 104,470/126,086/126,086 us 🟢 -10.7% / 🔴 +17.5%
🔴 bs=1000 sw=10 sl=64 1,099 0.671 907,216/1,062,822/1,062,822 us 🔴 +11.9% / 🟢 -7.5%
Baseline details

Latest main 812f177 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 425 tuples/sec 479 tuples/sec 787.55 tuples/sec -11.3% -46.0%
bs=10 sw=10 sl=64 MB/s 0.26 MB/s 0.292 MB/s 0.481 MB/s -11.0% -45.9%
bs=10 sw=10 sl=64 p50 22,644 us 19,349 us 12,255 us +17.0% +84.8%
bs=10 sw=10 sl=64 p95 31,967 us 26,390 us 15,802 us +21.1% +102.3%
bs=10 sw=10 sl=64 p99 31,967 us 26,390 us 19,008 us +21.1% +68.2%
bs=100 sw=10 sl=64 throughput 944 tuples/sec 956 tuples/sec 997.81 tuples/sec -1.3% -5.4%
bs=100 sw=10 sl=64 MB/s 0.576 MB/s 0.584 MB/s 0.609 MB/s -1.4% -5.4%
bs=100 sw=10 sl=64 p50 104,470 us 101,263 us 100,690 us +3.2% +3.8%
bs=100 sw=10 sl=64 p95 126,086 us 141,231 us 107,316 us -10.7% +17.5%
bs=100 sw=10 sl=64 p99 126,086 us 141,231 us 113,823 us -10.7% +10.8%
bs=1000 sw=10 sl=64 throughput 1,099 tuples/sec 1,099 tuples/sec 1,030 tuples/sec 0.0% +6.7%
bs=1000 sw=10 sl=64 MB/s 0.671 MB/s 0.671 MB/s 0.629 MB/s 0.0% +6.7%
bs=1000 sw=10 sl=64 p50 907,216 us 907,666 us 981,213 us -0.0% -7.5%
bs=1000 sw=10 sl=64 p95 1,062,822 us 950,023 us 1,027,605 us +11.9% +3.4%
bs=1000 sw=10 sl=64 p99 1,062,822 us 950,023 us 1,055,466 us +11.9% +0.7%
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,470.19,200,128000,425,0.260,22644.28,31966.89,31966.89
1,100,10,64,20,2117.96,2000,1280000,944,0.576,104470.21,126086.08,126086.08
2,1000,10,64,20,18204.75,20000,12800000,1099,0.671,907216.04,1062822.29,1062822.29

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.35%. Comparing base (812f177) to head (cd06db5).

Files with missing lines Patch % Lines
...ard/user/workflow/WorkflowExecutionsResource.scala 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7014      +/-   ##
============================================
- Coverage     79.11%   78.35%   -0.76%     
+ Complexity     3788     3784       -4     
============================================
  Files          1160     1173      +13     
  Lines         46108    46490     +382     
  Branches       5117     5130      +13     
============================================
- Hits          36477    36429      -48     
- Misses         8005     8431     +426     
- Partials       1626     1630       +4     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 84e8727
agent-service 76.76% <ø> (ø) Carriedforward from 84e8727
amber 70.66% <0.00%> (-1.62%) ⬇️
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 84e8727
config-service 66.66% <ø> (ø) Carriedforward from 84e8727
file-service 67.21% <ø> (ø) Carriedforward from 84e8727
frontend 82.99% <ø> (ø) Carriedforward from 84e8727
notebook-migration-service 78.94% <ø> (ø) Carriedforward from 84e8727
pyamber 95.38% <ø> (-0.96%) ⬇️ Carriedforward from 84e8727
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 84e8727

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

@aglinxinyuan
aglinxinyuan enabled auto-merge July 29, 2026 05:49
@mengw15
mengw15 disabled auto-merge July 29, 2026 05:56
@mengw15
mengw15 added this pull request to the merge queue Jul 29, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 29, 2026
@mengw15
mengw15 added this pull request to the merge queue Jul 29, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 29, 2026
@mengw15
mengw15 added this pull request to the merge queue Jul 29, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 29, 2026
@mengw15
mengw15 added this pull request to the merge queue Jul 29, 2026
Merged via the queue into apache:main with commit 0635737 Jul 29, 2026
28 of 31 checks passed
@mengw15
mengw15 deleted the fix/6977-stats-endpoint-auth branch July 29, 2026 20:36
@github-actions

Copy link
Copy Markdown
Contributor

Backport PR opened: draft #7054 (#7054) to release/v1.2, assigned to @mengw15 — needs manual work because the backported tree failed its pre-merge build.

xuang7 pushed a commit to Yicong-Huang/texera that referenced this pull request Jul 30, 2026
…pache#7054)

### What changes were proposed in this PR?

Automated backport of apache#7014 to `release/v1.2`.

Source: 0635737 · [automation
run](https://github.com/apache/texera/actions/runs/30489096963)

### Any related issues, documentation, discussions?

Backport of apache#7014. Originally linked apache#6977.

### How was this PR tested?

Release-branch CI runs on this branch once the build is fixed and this
PR is marked ready for review.

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

No.

Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
Co-authored-by: Meng Wang <mengw15@uci.edu>
Co-authored-by: Xinyuan Lin <xinyual3@uci.edu>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.

retrieveWorkflowRuntimeStatistics is the only WorkflowExecutionsResource endpoint missing @RolesAllowed / @Auth

5 participants