fix(auth): JwtAuthFilter eager-401 with @PermitAll opt-out#4903
Open
Yicong-Huang wants to merge 4 commits intoapache:mainfrom
Open
fix(auth): JwtAuthFilter eager-401 with @PermitAll opt-out#4903Yicong-Huang wants to merge 4 commits intoapache:mainfrom
Yicong-Huang wants to merge 4 commits intoapache:mainfrom
Conversation
github-actions
Bot
added
dependencies
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4903 +/- ##
============================================
+ Coverage 42.41% 42.55% +0.13%
- Complexity 2155 2187 +32
============================================
Files 954 954
Lines 34675 34686 +11
Branches 3629 3633 +4
============================================
+ Hits 14709 14760 +51
+ Misses 19061 19015 -46
- Partials 905 911 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Today JwtAuthFilter silently passes through any request that lacks a valid Bearer token; the 401 only surfaces later when Dropwizard's @Auth injection fails. amber's toastshaman path returns 401 directly from its filter with a WWW-Authenticate challenge — strictly more correct. Align the microservice filter: - No `Authorization: Bearer …` header → throw 401 with bare `WWW-Authenticate: Bearer realm="texera"` (RFC 6750 §3 challenge). - Header present but token verification / claim extraction fails → throw 401 with `error="invalid_token"` so a well-behaved client can discard the bad token instead of retrying. - Header present and valid → install SecurityContext as before. @permitAll opt-out: a resource method (or class) annotated with `jakarta.annotation.security.PermitAll` skips the eager 401 only on the "no header" path. The `@Auth Optional[SessionUser]` parameter is then injected as empty. An invalid token still 401s on @permitAll endpoints — a tampered or stale token is never silently treated as anonymous. The single in-tree consumer of the optional pattern is `file-service/.../DatasetResource.getDatasetCover` (anonymous read of public dataset covers); annotate it with @permitAll. Failure is signaled by throwing WebApplicationException rather than abortWith — the JAX-RS-idiomatic shape, plus it composes with Dropwizard's WebApplicationExceptionCatchingFilter when reused elsewhere. Tests: 9-case JwtAuthFilterSpec covering required-auth (no header / non-Bearer / unverifiable / valid), method-level @permitAll (unauthenticated → pass / invalid token → 401 / valid → SecurityContext), class-level @permitAll, and resourceInfo-absent fallback to required-auth. Common/auth gains two test-scope deps (jakarta.annotation-api for @permitAll inspection; jersey-common to provide a RuntimeDelegate so Response.build() works in unit tests without a Jersey runtime). Closes apache#4901
Yicong-Huang
changed the title
Yicong-Huang
force-pushed
the
refactor/jwt-filter-eager-401
branch
from
ef84db6 to
7a95fbe
Compare
Building a JAX-RS Response from inside the filter requires a RuntimeDelegate on the classpath. The earlier attempt added jersey-common 3.0.12 to common/auth's test classpath to satisfy that; amber declares `Auth % "test->test"` in build.sbt, so this jar bled into amber's test classpath and triggered a LinkageError on javax.ws.rs.ext.RuntimeDelegate (mismatched ClassLoader on the same class) — breaking 4 WorkflowAccessResourceSpec tests. Avoid the dependency entirely: throw a custom `UnauthorizedException` (extends RuntimeException) carrying the RFC 6750 §3 challenge string as a field. A new `UnauthorizedExceptionMapper` translates it to a `401` with `WWW-Authenticate` at the JAX-RS edge. Tests inspect the field directly — no Response build, no RuntimeDelegate needed, no test-classpath cross-contamination. Side benefits: `BearerChallenge` and `InvalidTokenChallenge` move to the JwtAuthFilter companion as exposed `val`s, so the contract is asserted via plain string equality. Each of the 4 microservices that registers `JwtAuthFilter` now also registers `UnauthorizedExceptionMapper`. amber's existing tests pass unchanged.
scalafmt: rewrap the 4-symbol auth import line in each microservice's Application class — the previous one-liner blew the column limit once `UnauthorizedExceptionMapper` joined the import list. Tests: add `UnauthorizedExceptionMapperSpec` covering the three things that matter at the JAX-RS edge — status is 401, the exception's challenge string is forwarded verbatim into `WWW-Authenticate`, and the response has no entity body (so clients see the auth challenge instead of a JSON error). Test classpath needs a Jersey RuntimeDelegate to call `Response.build()`, so add `dropwizard-jersey` as a Test-only dep on common/auth.
Yicong-Huang
force-pushed
the
refactor/jwt-filter-eager-401
branch
from
f2473d9 to
655db3b
Compare
Contributor
Author
|
The missing three lines of coverage are on the target services who uses this auth filter. Those services need additional efforts on adding more tests for their own logic. |
Contributor
Author
|
@kunwp1 could you please help review this Pr? Thanks! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the shared common/auth JWT authentication behavior used by Texera’s microservices to eagerly return RFC 6750-compliant 401 Unauthorized responses (with WWW-Authenticate: Bearer …) from the auth layer, while allowing specific endpoints to opt out of the missing-header 401 via @PermitAll.
Changes:
- Updated
JwtAuthFilterto eagerly 401 on missing/non-Bearer headers and to 401 witherror="invalid_token"on verification failures, with@PermitAllopting out of the missing-header 401. - Added
UnauthorizedException+UnauthorizedExceptionMapperto translate auth failures into a 401 +WWW-Authenticatechallenge. - Wired the mapper into the affected microservices and added unit tests for the new filter/mapper behavior; annotated the dataset cover endpoint as
@PermitAllfor anonymous public access.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala | Marks dataset cover endpoint @PermitAll so anonymous callers can reach @Auth Optional[SessionUser] logic. |
| file-service/src/main/scala/org/apache/texera/service/FileService.scala | Registers UnauthorizedExceptionMapper alongside JwtAuthFilter. |
| config-service/src/main/scala/org/apache/texera/service/ConfigService.scala | Registers UnauthorizedExceptionMapper alongside JwtAuthFilter. |
| computing-unit-managing-service/src/main/scala/org/apache/texera/service/ComputingUnitManagingService.scala | Registers UnauthorizedExceptionMapper alongside JwtAuthFilter. |
| access-control-service/src/main/scala/org/apache/texera/service/AccessControlService.scala | Registers UnauthorizedExceptionMapper alongside JwtAuthFilter. |
| common/auth/src/main/scala/org/apache/texera/auth/JwtAuthFilter.scala | Implements eager-401 behavior and @PermitAll opt-out for missing/non-Bearer headers. |
| common/auth/src/main/scala/org/apache/texera/auth/UnauthorizedException.scala | Adds a dedicated exception + JAX-RS exception mapper for 401 + challenge header responses. |
| common/auth/src/test/scala/org/apache/texera/auth/JwtAuthFilterSpec.scala | Adds unit coverage for missing/non-Bearer/invalid/valid token flows and @PermitAll semantics. |
| access-control-service/src/test/scala/org/apache/texera/auth/UnauthorizedExceptionMapperSpec.scala | Adds unit coverage ensuring mapper returns 401 and preserves challenge verbatim without an entity. |
| common/auth/build.sbt | Adds jakarta.annotation-api dependency for @PermitAll. |
| amber/LICENSE-binary-java | Updates binary license inventory to include jakarta.annotation-api. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+66
| if (authHeader == null || !authHeader.startsWith("Bearer ")) { | ||
| if (isPermitAll) return | ||
| throw new UnauthorizedException(JwtAuthFilter.BearerChallenge) | ||
| } | ||
|
|
||
| val token = authHeader.substring(7) // Remove "Bearer " prefix |
| * implementation. The companion [[UnauthorizedExceptionMapper]] converts | ||
| * the exception to the actual HTTP response at the JAX-RS edge. | ||
| */ | ||
| class UnauthorizedException(val challenge: String) extends RuntimeException(challenge) |
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?
Align the 4 microservices'
JwtAuthFilterwith amber's behavior: return401directly from the filter with an RFC 6750WWW-Authenticate: Bearer …challenge, instead of silently passing through to Dropwizard's@Authinjection.@PermitAll(JSR-250) opts a resource out of the no-header401. An invalid token still fails — a tampered/stale token is never treated as anonymous. The only existing consumer isDatasetResource.getDatasetCoverfor anonymous public-dataset reads.Any related issues, documentation, discussions?
Closes #4901.
Out of scope:
RolesAllowedDynamicFeatureis registered only in amber. The 4 microservices'@RolesAllowedannotations are currently decorative;workflow-compiling-serviceregisters no auth filter at all. Worth a separate issue.How was this PR tested?
Auth/testcoversJwtAuthFilter(header-missing / non-Bearer / invalid-token / valid-token; method- and class-level@PermitAll; resource-info-absent fallback) andUnauthorizedExceptionMapper(status, challenge passthrough, no entity body). Format / lint clean; all 4 microservices recompile.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7)