test(amber): cover the workflow resource's permission and failure paths - #7592
Merged
Conversation
WorkflowResource sat at 82.6% of lines. Its residue was entirely permission-guard arms, exception paths, and one whole endpoint that had never been called: cloneWorkflow. Every collaborator it reaches is database-only, so MockTexeraDB is enough. Adds 16 tests to the existing spec, taking lines to 100%. Three claims are stated rather than implied, because reviewing my own tests found them overclaiming: the duplicate-failure test pins the exception wrapping but not transactionality, the delete test pins that an undecodable URI is tolerated but not the cleanup tail it happens to execute, and two assertions in the writer test are guards that already hold before the call. No production file is touched.
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7592 +/- ##
============================================
- Coverage 88.63% 87.40% -1.24%
+ Complexity 4346 4340 -6
============================================
Files 1177 1177
Lines 46894 46780 -114
Branches 5229 5191 -38
============================================
- Hits 41566 40886 -680
- Misses 3568 4108 +540
- Partials 1760 1786 +26
*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:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 557 | 0.34 | 17,447/26,126/26,126 us | 🔴 +9.7% / 🔴 +60.2% |
| 🔴 | bs=100 sw=10 sl=64 | 1,185 | 0.723 | 81,282/120,829/120,829 us | 🔴 +26.0% / 🟢 -19.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,420 | 0.867 | 704,355/755,034/755,034 us | ⚪ within ±5% / 🟢 +38.4% |
Baseline details
Latest main 5021bc6 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 557 tuples/sec | 597 tuples/sec | 775.33 tuples/sec | -6.7% | -28.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.34 MB/s | 0.364 MB/s | 0.473 MB/s | -6.6% | -28.2% |
| bs=10 sw=10 sl=64 | p50 | 17,447 us | 15,904 us | 12,743 us | +9.7% | +36.9% |
| bs=10 sw=10 sl=64 | p95 | 26,126 us | 28,238 us | 16,310 us | -7.5% | +60.2% |
| bs=10 sw=10 sl=64 | p99 | 26,126 us | 28,238 us | 18,926 us | -7.5% | +38.0% |
| bs=100 sw=10 sl=64 | throughput | 1,185 tuples/sec | 1,254 tuples/sec | 1,001 tuples/sec | -5.5% | +18.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.723 MB/s | 0.766 MB/s | 0.611 MB/s | -5.6% | +18.4% |
| bs=100 sw=10 sl=64 | p50 | 81,282 us | 78,672 us | 101,399 us | +3.3% | -19.8% |
| bs=100 sw=10 sl=64 | p95 | 120,829 us | 95,893 us | 108,206 us | +26.0% | +11.7% |
| bs=100 sw=10 sl=64 | p99 | 120,829 us | 95,893 us | 118,195 us | +26.0% | +2.2% |
| bs=1000 sw=10 sl=64 | throughput | 1,420 tuples/sec | 1,453 tuples/sec | 1,026 tuples/sec | -2.3% | +38.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.867 MB/s | 0.887 MB/s | 0.626 MB/s | -2.3% | +38.4% |
| bs=1000 sw=10 sl=64 | p50 | 704,355 us | 680,082 us | 996,304 us | +3.6% | -29.3% |
| bs=1000 sw=10 sl=64 | p95 | 755,034 us | 746,863 us | 1,042,531 us | +1.1% | -27.6% |
| bs=1000 sw=10 sl=64 | p99 | 755,034 us | 746,863 us | 1,074,934 us | +1.1% | -29.8% |
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,359.38,200,128000,557,0.340,17446.84,26125.73,26125.73
1,100,10,64,20,1687.75,2000,1280000,1185,0.723,81281.75,120828.90,120828.90
2,1000,10,64,20,14084.87,20000,12800000,1420,0.867,704354.81,755034.40,755034.40
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:1063
- This new test section belongs to issue #7591 according to the PR and linked issue; referencing #7224 sends maintainers to the unrelated earlier test effort.
// ─── shared-access and failure paths (issue #7224) ──────────────────────────
The clone test seeded a private workflow owned by sessionUser1 and asserted that sessionUser2, holding no access to it, could read its full content. That succeeds today -- cloneWorkflow fetches by wid with no hasReadAccess guard, unlike retrieveWorkflow and duplicateWorkflow -- but pinning it turns a gap in the resource into an asserted contract. Publish the source first, which is the flow the endpoint actually serves (the hub's clone button on someone else's published workflow), and record the gap in a comment instead of asserting it. cloneWorkflow branches on nothing, so the covered lines are unchanged. The isPublished assertion gets stronger: with a public source it now pins that publicness is not inherited by the clone. Also point the section header at apache#7591 rather than the earlier apache#7224.
renovate-bot
pushed a commit
to renovate-bot/apache-_-texera
that referenced
this pull request
Aug 13, 2026
### What changes were proposed in this PR?
`cloneWorkflow` fetched the source workflow by `wid` and copied its
content into a workflow owned by the caller, with no access check on the
way in. Any authenticated REGULAR user could `POST
/workflow/clone/<wid>` for a wid they hold no privilege on and receive a
full copy of a private workflow's content — operator configurations,
file paths and all.
**Root cause.** Every sibling on this path guards; this one endpoint did
not.
| Endpoint | Guard |
|---|---|
| `retrieveWorkflow` | `hasReadAccess` directly |
| `duplicateWorkflow` | `hasReadAccess` directly |
| `cloneVersion` (`/version/clone/{vid}`) | inherits it via
`retrieveWorkflowVersion` |
| **`cloneWorkflow`** | **none** |
That reads as an oversight rather than a decision. SECURITY.md states
that REGULAR users "cannot access other users' private resources without
granted permissions", so the endpoint contradicted the project's own
declared model.
**Before → after**
```
caller with no privilege on wid
|
v v
POST /workflow/clone/{wid} POST /workflow/clone/{wid}
| |
| (no check) +-- hasReadAccess(wid, uid)? --> no --> 403
v |
fetchOneByWid(wid) v yes (owner / READ grant / public)
| fetchOneByWid(wid)
v |
full content copied to caller v
full content copied to caller
```
The fix adds the same three lines the siblings use. `hasReadAccess`
already returns true for public workflows, so the hub's clone button —
the only caller, and always acting on a published workflow — is
unaffected. A caller holding an explicit READ grant can still clone.
### Any related issues, documentation, discussions?
Found while reviewing the clone-endpoint test in apache#7592, now merged; the
new cases here build on the spec helpers that landed with it. Not filed
as an issue, because SECURITY.md asks that security bugs not be reported
through public issues.
The `release/v1.2` backport preflight comes back grey: the guard itself
applies, but the two new test cases depend on helpers that arrived with
apache#7592, which was not backported. The backport needs those cases
rewritten self-contained, so it will have to be resolved by hand rather
than pushed straight through.
### How was this PR tested?
Two cases added to the existing `WorkflowResourceSpec`, and the
pre-existing success case in apache#7592 now publishes its source first so it
exercises the public path.
| Test | Pins |
|---|---|
| `clone a private workflow the caller has been granted read access to`
| the guard does not over-block a legitimate READ grant |
| `reject a caller with no access to the source workflow` | 403, no copy
reaches the caller, and no `WORKFLOW_USER_CLONES` row is written for the
rejected attempt |
Red before the guard, green after — with the guard reverted the
rejection case fails and the other 69 pass, so it is the guard the test
is pinning and not a fixture.
```
sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.file.WorkflowResourceSpec"
```
```
[info] Total number of tests run: 70
[info] Tests: succeeded 70, failed 0, canceled 0, ignored 0, pending 0
```
`scalafmtCheck` and `scalafix --check` pass for both `Compile` and
`Test`.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
WorkflowResourcesat at 82.6% of lines, and its residue was not scattered: permission-guard arms, exception paths, and one whole endpoint that had never been called —cloneWorkflow. Permission guards are exactly where a silent regression matters, which is why this is worth doing despite the modest line count.Adds 16 tests to the existing spec (no third spec file), taking the file to 100% of lines. Everything it reaches is database-only, so
MockTexeraDBsuffices —WorkflowVersionResource.insertVersionis jOOQ plus JacksonJsonDiff,HubResource.recordCloneis jOOQ, and nothing needs LakeFS, Docker or an engine.Verification
22 mutations applied and reverted, production diff confirmed empty each time — the clone's
isPublicargument, the version-insert ordering, the access-level comparisons, and the exception-wrapping arms among them.Reviewing my own tests then found three that claimed more than they pinned. All three are now stated in the spec rather than left to be discovered:
assignNewOperatorIdsfails beforecreateWorkflowinserts, so "no copy was created" holds with or without a transaction — replacingcontext.transactionwith a plain block leaves the suite greenLargeBinaryManageris an S3-backedobjectwith no seam, and document cleanup needs Iceberg fixtures this spec lackscase NonFatalarm of the outer catch turns it red, so an undecodable URI really is tolerated rather than aborting the deletepersistWorkflowis called; its write branch touches onlyWORKFLOWandWORKFLOW_VERSIONA note on the numbers
jacoco reports a wide line-versus-branch split here — branch coverage stays low even at 100% lines — because of the synthetic branches the Scala compiler generates for this style of code. The line figure is the meaningful one on this file.
No production file is touched.
Any related issues, documentation, discussions?
Closes #7591
How was this PR tested?
16 new on top of the existing 52.
Test/scalafmtCheckandTest/scalafix --checkboth pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)