Skip to content

fix(amber): return 403 instead of 401 when a user lacks access to an execution - #9325

Merged
mengw15 merged 1 commit into
apache:mainfrom
mengw15:fix/9324-execution-access-denied-status
Nov 5, 2026
Merged

mengw15 merged 1 commit into
apache:mainfrom
mengw15:fix/9324-execution-access-denied-status

Conversation

@mengw15

@mengw15 mengw15 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

validateUserCanAccessExecutions now throws ForbiddenException (403) instead of a 401. The frontend reads any 401 on an authenticated request as an expired session, so a read-only user who bookmarked, renamed or deleted an execution was logged out. With a 403 they stay logged in.

Bookmark puts its icon back on a refusal. Rename and delete have no error handler, so they fail without a message; that is left out of this PR to keep it small.

Any related issues, documentation, discussions?

Closes #9324.

How was this PR tested?

Added a read-only bookmark case to WorkflowExecutionsResourceSpec, and the refusal cases now assert 403. Ran sbt "WorkflowExecutionService/testOnly *UserQuotaResourceSpec *WorkflowExecutionsResourceSpec": 84 of 85 pass. The other one, the per-user warehouse case of retrieveWorkflowRuntimeStatistics, fails the same way locally on unmodified main. Putting the 401 back fails the four cases this PR touches.

Not clicked through in a running instance.

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

Generated-by: Claude Code (claude-opus-5-5)

@Yicong-Huang Yicong-Huang added release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels Nov 5, 2026
@github-actions
github-actions Bot requested a review from xuang7 November 5, 2026 01:56
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch Analysis
✅ release/v1.3 Change detected on this branch — label added; this fix is queued to backport here.
✅ release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other. Review requested.

Auto-label run.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

  • Committers with relevant context: @parshimers
    You can request their reviews formally with /request-review @parshimers.

@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main e70d67e 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 401 0.245 24,220/32,628/32,628 us 🔴 +13.2% / 🔴 +148.6%
🔴 bs=100 sw=10 sl=64 799 0.488 120,698/147,885/147,885 us 🔴 +8.6% / 🔴 +62.3%
⚪ bs=1000 sw=10 sl=64 954 0.582 1,039,062/1,131,347/1,131,347 us ⚪ within ±5% / 🔴 +27.2%
Baseline details

Latest main e70d67e from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 401 tuples/sec 454 tuples/sec 931.98 tuples/sec -11.7% -57.0%
bs=10 sw=10 sl=64 MB/s 0.245 MB/s 0.277 MB/s 0.569 MB/s -11.6% -56.9%
bs=10 sw=10 sl=64 p50 24,220 us 21,539 us 10,818 us +12.4% +123.9%
bs=10 sw=10 sl=64 p95 32,628 us 28,828 us 13,127 us +13.2% +148.6%
bs=10 sw=10 sl=64 p99 32,628 us 28,828 us 16,504 us +13.2% +97.7%
bs=100 sw=10 sl=64 throughput 799 tuples/sec 854 tuples/sec 1,210 tuples/sec -6.4% -34.0%
bs=100 sw=10 sl=64 MB/s 0.488 MB/s 0.521 MB/s 0.739 MB/s -6.3% -33.9%
bs=100 sw=10 sl=64 p50 120,698 us 111,117 us 85,326 us +8.6% +41.5%
bs=100 sw=10 sl=64 p95 147,885 us 153,428 us 91,092 us -3.6% +62.3%
bs=100 sw=10 sl=64 p99 147,885 us 153,428 us 102,379 us -3.6% +44.4%
bs=1000 sw=10 sl=64 throughput 954 tuples/sec 955 tuples/sec 1,240 tuples/sec -0.1% -23.0%
bs=1000 sw=10 sl=64 MB/s 0.582 MB/s 0.583 MB/s 0.757 MB/s -0.2% -23.1%
bs=1000 sw=10 sl=64 p50 1,039,062 us 1,044,788 us 840,622 us -0.5% +23.6%
bs=1000 sw=10 sl=64 p95 1,131,347 us 1,107,648 us 889,172 us +2.1% +27.2%
bs=1000 sw=10 sl=64 p99 1,131,347 us 1,107,648 us 931,615 us +2.1% +21.4%
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,498.31,200,128000,401,0.245,24220.27,32627.72,32627.72
1,100,10,64,20,2501.85,2000,1280000,799,0.488,120697.60,147884.58,147884.58
2,1000,10,64,20,20972.53,20000,12800000,954,0.582,1039061.73,1131346.82,1131346.82

@codecov-commenter

codecov-commenter commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.40%. Comparing base (e70d67e) to head (92bb73b).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9325      +/-   ##
============================================
+ Coverage     92.20%   92.40%   +0.20%     
- Complexity     4977     6066    +1089     
============================================
  Files          1275     1307      +32     
  Lines         55568    59996    +4428     
  Branches       7163     7759     +596     
============================================
+ Hits          51237    55442    +4205     
- Misses         2555     2702     +147     
- Partials       1776     1852      +76     
Flag Coverage Δ *Carryforward flag
access-control-service 77.44% <ø> (ø) Carriedforward from e70d67e
agent-service 99.53% <ø> (ø) Carriedforward from e70d67e
amber 88.30% <ø> (+1.52%) ⬆️
computing-unit-managing-service 64.02% <ø> (ø) Carriedforward from e70d67e
config-service 87.61% <ø> (ø) Carriedforward from e70d67e
file-service 82.29% <ø> (ø) Carriedforward from e70d67e
frontend 96.57% <ø> (ø) Carriedforward from e70d67e
notebook-migration-service 83.77% <ø> (ø) Carriedforward from e70d67e
pyamber 98.54% <ø> (ø) Carriedforward from e70d67e
workflow-compiling-service 77.95% <ø> (ø) Carriedforward from e70d67e

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

@mengw15 mengw15 removed the release/v1.2 back porting to release/v1.2 label Nov 5, 2026
@mengw15
mengw15 added this pull request to the merge queue Nov 5, 2026
Merged via the queue into apache:main with commit 312a0ce Nov 5, 2026
64 of 74 checks passed
@mengw15
mengw15 deleted the fix/9324-execution-access-denied-status branch November 5, 2026 06:38
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Backport PR opened: #9333 (#9333) to release/v1.3 — the cherry-pick was clean, so it is ready for review, waiting on @mengw15. It needs to be reopened once to start its checks; the PR says how.

renovate-bot pushed a commit to renovate-bot/apache-_-texera that referenced this pull request Nov 6, 2026
### What changes were proposed in this PR?

Nothing can merge into main right now. Every job that sets up sbt
(`amber`, `amber-integration`, `platform` ×6, `platform-integration` ×6,
`Bench`, the image builds and the dependency graph) has died at `Setup
sbt launcher` since 2026-11-05 15:36 UTC, on main itself and on every
PR, so `Required Checks` is red and the merge queue takes nothing.
Nothing in this repository changed: main at `312a0cefc` (apache#9325, run at
07:16 UTC) was green, main at `8b0fb168e` (apache#8848, run at 20:54 UTC) is
red, and that commit touches three frontend files.

Before: Scala job -> sbt/setup-sbt v1.5.12 -> "Verify the sbt
distribution signature" reads carabiner-dev/policies at HEAD -> ampel
v1.3.6 cannot parse it -> exit 1 before sbt is installed
After: Scala job -> sbt/setup-sbt v1.5.14 -> the same step reads the
policy pinned at carabiner-dev/policies@971d670e9 -> sbt installs -> the
job runs

Root cause. v1.5.12 hands `carabiner-dev/actions/ampel/verify@32587e8`
(ampel v1.3.6) the policy argument
`git+https://github.com/carabiner-dev/policies#signature/signature.json`,
with no ref, so the policy is whatever that repository's main holds at
run time. On 2026-11-05 `signature/signature.json` gained a `meta.name`
field that ampel v1.3.6 rejects, and ampel exits while loading the
policy with `compiling policy: unable to parse a policy, group or
policySet from data`. The step's `fail: false` covers a failed
verification verdict, not the loader exiting, so the composite action
fails and the job stops. sbt/setup-sbt#155, released as v1.5.14 the same
day, pins the policy to
`carabiner-dev/policies@971d670e93f2fb4da304b0e5f071066dc4686a5a`, a
commit without the field.

Change. All 9 call sites (`build.yml` ×4, `build-and-push-images.yml`
×3, `benchmarks.yml`, `dependency-graph.yml`) move from
`ecad189dd245444b9ae0a0cd5922fd938b88906f` (v1.5.12) to
`67d461eba2c4753196c230e02da850757ba4775e` (v1.5.14), as apache#9100 moved
them to v1.5.12. Between the two versions `action.yml` differs in two
lines: the policy pin above and the default `sbt-runner-version` (2.0.10
-> 2.0.12), which this repository does not use because
`project/build.properties` sets `sbt.version`. The nested actions,
`actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9` and
`carabiner-dev/actions/ampel/verify@32587e82f960d49b36101e8c45d1956e511965d3`,
are the same in both, and no call site passes inputs.

Scope. v1.5.13 is already on the ASF allow list but carries the same
unpinned policy line, so it fails the same way; v1.5.14 is the first
version that does not. This PR changes nothing about what the signature
step does when it runs; it only stops the step from reading a moving
target.

**Blocked on apache/infrastructure-actions#1498.**
`sbt/setup-sbt@67d461eba` is not on the ASF actions allow list yet;
apache#1498 (opened 2026-11-05 13:39 UTC) adds it. Until it merges, every sbt
job on this PR fails at `Set up job` with the action not allowed, which
is how v1.5.7 failed in apache#9100, so the red checks here are expected and
this PR must not be merged before apache#1498 lands. Once it does, a re-run of
the failed jobs is the proof; nothing in this branch needs to change.

### Any related issues, documentation, discussions?

Closes apache#9345.

Upstream fix: sbt/setup-sbt#155 (v1.5.14). Allow-list entry:
apache/infrastructure-actions#1498. Precedent: apache#9100 moved the same 9
call sites to v1.5.12 after the ASF allow list dropped the actions
v1.5.7 reached, with backports apache#9132 (v1.3) and apache#9133 (v1.2); apache#7710 and
apache#6990 are the earlier bumps of the same kind.

### How was this PR tested?

Every changed file parses:

```
python -I -c "import sys, yaml; [yaml.safe_load(open(f, encoding='utf-8')) for f in sys.argv[1:]]" .github/workflows/build.yml .github/workflows/build-and-push-images.yml .github/workflows/benchmarks.yml .github/workflows/dependency-graph.yml
```

`git ls-remote --tags https://github.com/sbt/setup-sbt.git` resolves
`refs/tags/v1.5.14^{}` to `67d461eba2c4753196c230e02da850757ba4775e`.
Diffing `action.yml` at `ecad189` against `67d461eba` shows only the two
lines described above, with identical `uses:` lines. Walking the `uses:`
chain under v1.5.14 reaches `ampel/verify@32587e8`, which is in
`apache/infrastructure-actions` `approved_patterns.yml`;
`sbt/setup-sbt@67d461eba` itself is not yet, which
apache/infrastructure-actions#1498 adds.

The rest is CI. The labeler applies `ci` for `.github/**`, which runs
every stack that sets up sbt. Those jobs cannot pass until
apache/infrastructure-actions#1498 lands (see above); the re-run after
that is the test, and the checks on this PR are the first run of v1.5.14
in this repository.

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

Yes. Generated-by: Claude Code (claude-fable-5-1). Co-authored with
Claude; the author reviewed the change before submission.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read-only users are logged out when they bookmark, rename or delete an execution

4 participants