Skip to content

fix: enforce @RolesAllowed on microservice resources#5199

Open
Yicong-Huang wants to merge 1 commit into
apache:mainfrom
Yicong-Huang:fix/roles-allowed-redo
Open

fix: enforce @RolesAllowed on microservice resources#5199
Yicong-Huang wants to merge 1 commit into
apache:mainfrom
Yicong-Huang:fix/roles-allowed-redo

Conversation

@Yicong-Huang
Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Re-applies #5049 (Jersey @RolesAllowed enforcement on config-service, computing-unit-managing-service, and workflow-compiling-service) and additionally marks the two pre-login ConfigResource endpoints — /api/config/gui and /api/config/user-system — as @PermitAll. Those endpoints are loaded by GuiConfigService.load() in the Angular APP_INITIALIZER before any login, so once role enforcement is on they must keep returning 200 to unauthenticated callers; missing this was what broke bootstrap and got #5049 reverted in #5173. Everything outside config-service matches #5049 byte-for-byte.

Any related issues, documentation, or discussions?

Closes: #4904
Prior attempt: #5049, reverted by #5173. The bootstrap root cause was diagnosed inline at #5049 (comment).

How was this PR tested?

Added ConfigResourceAuthSpec: wires ConfigResource through the same JwtAuthFilter + RolesAllowedDynamicFeature pipeline production uses (via Dropwizard's ResourceExtension) and fires HTTP requests with no Authorization header.

  • GET /config/gui → expects 200
  • GET /config/user-system → expects 200
  • GET /auth-probe (an in-test @RolesAllowed resource) → expects 403

The 403 sanity guard ensures the feature is actually enforcing, so a future "200 everywhere" regression cannot silently slip through. Kept the three *ServiceRunSpec structural tests from #5049 verifying that RolesAllowedDynamicFeature is registered. Manual reproduction with curl against a local dev server confirmed the unauthenticated bootstrap path returns 200 while a low-role JWT against an annotated endpoint returns 403.

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

Co-authored with Claude Opus 4.7 in compliance with ASF.

Re-applies apache#5049 (revert in apache#5173) and additionally marks the two pre-login
ConfigResource endpoints (/config/gui, /config/user-system) as @permitAll
so the frontend's APP_INITIALIZER can still bootstrap once role enforcement
is on. Adds an HTTP-pipeline regression test that fires unauthenticated
requests through JwtAuthFilter + RolesAllowedDynamicFeature and asserts
the two pre-login endpoints return 200 while a sibling @RolesAllowed probe
returns 403.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file fix common platform Non-amber Scala service paths labels May 25, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 25, 2026

Codecov Report

❌ Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.50%. Comparing base (a25c543) to head (148873d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../texera/service/ComputingUnitManagingService.scala 44.44% 5 Missing ⚠️
...ache/texera/service/WorkflowCompilingService.scala 66.66% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5199      +/-   ##
============================================
+ Coverage     45.39%   45.50%   +0.11%     
- Complexity     2218     2223       +5     
============================================
  Files          1042     1042              
  Lines         39989    39997       +8     
  Branches       4260     4260              
============================================
+ Hits          18152    18201      +49     
+ Misses        20720    20677      -43     
- Partials       1117     1119       +2     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø)
agent-service 33.76% <ø> (ø) Carriedforward from a25c543
amber 45.74% <ø> (-0.01%) ⬇️
computing-unit-managing-service 1.38% <44.44%> (+1.38%) ⬆️
config-service 64.51% <100.00%> (+64.51%) ⬆️
file-service 32.18% <ø> (ø)
frontend 37.79% <ø> (ø) Carriedforward from a25c543
python 90.50% <ø> (ø) Carriedforward from a25c543
workflow-compiling-service 58.39% <66.66%> (+1.57%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Ma77Ball
Copy link
Copy Markdown
Contributor

Ma77Ball commented May 25, 2026

@Yicong-Huang I raised a similar pr to fix this. #5198

@Yicong-Huang
Copy link
Copy Markdown
Contributor Author

@Yicong-Huang I raised a similar pr to fix this. #5198

I hope to minimize the change, this PR is reapplying #5049 with only two more line diff about permit all usage on configservice. could you please help test this one?

@Ma77Ball
Copy link
Copy Markdown
Contributor

@Yicong-Huang I raised a similar pr to fix this. #5198

I hope to minimize the change, this PR is reapplying #5049 with only two more line diff about permit all usage on configservice. could you please help test this one?

That makes sense. I can help test and review later today when I get back home.

@carloea2
Copy link
Copy Markdown
Contributor

carloea2 commented May 25, 2026

Shouldn’t Matthew’s PR have priority?

Also this PR is bigger

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

Labels

common dependencies Pull requests that update a dependency file fix platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Authorization bypass: @RolesAllowed unenforced on microservices, workflow-compiling-service requires no token at all

4 participants