[#10960] feat(authentication): Wire basic authenticator module#10967
Open
lasdf1234 wants to merge 2 commits intoapache:mainfrom
Open
[#10960] feat(authentication): Wire basic authenticator module#10967lasdf1234 wants to merge 2 commits intoapache:mainfrom
lasdf1234 wants to merge 2 commits intoapache:mainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires a new authenticators:authenticator-basic Gradle module into Gravitino’s server authentication flow, adds AuthenticatorType.BASIC, and updates server-side HTTP status mapping so malformed Basic headers can return 400 Bad Request instead of being treated as internal errors.
Changes:
- Adds the new
authenticators:authenticator-basicmodule and wires it into the Gradle build (settings + server dependency). - Extends authenticator selection to recognize
basic(AuthenticatorType.BASIC) and loadorg.apache.gravitino.auth.local.BasicAuthenticator. - Updates
AuthenticationFilterto mapBadRequestExceptionto HTTP 400 and adds/updates tests around the new wiring and error mapping.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Includes the new authenticator-basic submodule in the Gradle build. |
| server/build.gradle.kts | Adds a dependency from server to the new authenticator-basic module. |
| common/src/main/java/org/apache/gravitino/auth/AuthenticatorType.java | Introduces AuthenticatorType.BASIC. |
| server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticatorFactory.java | Maps basic to the Basic authenticator class name for reflective loading. |
| server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java | Maps BadRequestException to HTTP 400 in auth error responses. |
| server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationFilter.java | Adds a regression test asserting BadRequestException maps to HTTP 400. |
| server/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticatorFactory.java | Adds a test verifying basic loads BasicAuthenticator. |
| authenticators/authenticator-basic/build.gradle.kts | Defines the new module build, dependencies, and test env vars. |
| authenticators/authenticator-basic/src/main/java/org/apache/gravitino/auth/local/BasicAuthenticator.java | Implements Basic auth header parsing and exception behavior. |
| authenticators/authenticator-basic/src/test/java/org/apache/gravitino/auth/local/TestBasicAuthenticator.java | Adds unit tests for Basic header parsing/error handling. |
Comment on lines
+95
to
+97
| return tokenData != null | ||
| && new String(tokenData, StandardCharsets.UTF_8) | ||
| .startsWith(AuthConstants.AUTHORIZATION_BASIC_HEADER); |
Comment on lines
+81
to
+85
|
|
||
| return new UserPrincipal(userName, authData); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new BadRequestException(e, "Malformed Basic authorization header: invalid base64"); | ||
| } |
Comment on lines
+33
to
+41
| @Test | ||
| public void testAuthenticateTokenThrowsUnauthorizedWhenHeaderMissing() { | ||
| UnauthorizedException exception = | ||
| Assertions.assertThrows( | ||
| UnauthorizedException.class, () -> authenticator.authenticateToken(null)); | ||
|
|
||
| Assertions.assertEquals("Empty token authorization header", exception.getMessage()); | ||
| Assertions.assertEquals("Basic", exception.getChallenges().get(0)); | ||
| } |
lasdf1234
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
roryqi
reviewed
May 6, 2026
Contributor
roryqi
left a comment
roryqi
left a comment
There was a problem hiding this comment.
I don't think this should be the first pull request. Because this relies on built-in idp.
| * under the License. | ||
| */ | ||
|
|
||
| plugins { |
Contributor
There was a problem hiding this comment.
Why do we need this module? It seems unrelated about this pull request. You didn't put any files in this module.
Contributor
Author
There was a problem hiding this comment.
May be 'Add password hashing support' should be the first sub task(#10961)? This sub-task is the most basic one, and it has accomplished the most basic functions of the authenticator-basic module.
If this sub task is the first sub task, I will adjust the sequence of the sub-tasks, and then you can review the code. How about that?
Contributor
There was a problem hiding this comment.
You should adjust the orders.
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 pull request?
This PR wires the new
authenticators:authenticator-basicmodule into the server authentication flow.The changes include:
authenticators:authenticator-basicGradle moduleservermodule depend onauthenticator-basicAuthenticatorType.BASICgravitino.authenticators=basicto loadorg.apache.gravitino.auth.local.BasicAuthenticator400 Bad Requestfor malformed Basic authorization headers and401 Unauthorizedfor missing or invalid Basic authorization headersWhy are the changes needed?
This is the module wiring work for local authentication. It enables Gravitino to recognize and load the new Basic authenticator module when
gravitino.authenticators=basic, which is the foundation for the follow-up local authentication tasks.Fix: #10960
Does this PR introduce any user-facing change?
Yes.
basicas a valid value ofgravitino.authenticators400 Bad Request401 UnauthorizedHow was this patch tested?
TestAuthenticatorFactoryto verifybasicloadsBasicAuthenticatorTestBasicAuthenticatorto verify Basic header error handlingTestAuthenticationFilterto verifyBadRequestExceptionmaps to HTTP 400./gradlew --no-daemon :authenticators:authenticator-basic:build./gradlew --no-daemon :server-common:test --tests org.apache.gravitino.server.authentication.TestAuthenticationFilter./gradlew --no-daemon :server:test --tests org.apache.gravitino.server.authentication.TestAuthenticatorFactory./gradlew --no-daemon classes testClasses