Copilot commented on code in PR #10967:
URL: https://github.com/apache/gravitino/pull/10967#discussion_r3193747997
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticatorFactory.java:
##########
@@ -32,11 +32,12 @@
public class AuthenticatorFactory {
private static final Logger LOG =
LoggerFactory.getLogger(AuthenticatorFactory.class);
-
public static final ImmutableMap<String, String> AUTHENTICATORS =
ImmutableMap.of(
AuthenticatorType.SIMPLE.name().toLowerCase(),
SimpleAuthenticator.class.getCanonicalName(),
+ AuthenticatorType.BASIC.name().toLowerCase(),
+ BasicAuthenticator.class.getCanonicalName(),
Review Comment:
The new BASIC wiring does not match the PR description: the factory maps
`basic` to `org.apache.gravitino.server.authentication.BasicAuthenticator` via
a direct class reference, but there is no
`org.apache.gravitino.auth.local.BasicAuthenticator` in the codebase. If the
intent is to load the authenticator from the new
`authenticators:authenticator-basic` module, prefer mapping to a
fully-qualified class name string (to avoid compile-time coupling from
`server-common`) and ensure the implementation class lives in that
module/package (or update the PR description to reflect the actual class being
loaded).
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/BasicAuthenticator.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.authentication;
+
+import java.nio.charset.StandardCharsets;
+import java.security.Principal;
+import java.util.Base64;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.UserPrincipal;
+import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.exceptions.BadRequestException;
+import org.apache.gravitino.exceptions.UnauthorizedException;
+
+/**
+ * Basic authenticator wiring for the local authentication module.
+ *
+ * <p>The credential verification flow will be implemented in follow-up
subtasks. For now, keep the
+ * current Basic header parsing behavior so the module can be loaded safely
when {@code
+ * gravitino.authenticators=basic}.
+ */
+public class BasicAuthenticator implements Authenticator {
+ private static final String BASIC_AUTH_CHALLENGE = "Basic";
+
+ @Override
+ public boolean isDataFromToken() {
+ return true;
+ }
+
+ @Override
+ public Principal authenticateToken(byte[] tokenData) {
+ if (tokenData == null) {
+ throw new UnauthorizedException("Empty token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ String authData = new String(tokenData, StandardCharsets.UTF_8);
+ if (authData.trim().isEmpty()) {
+ throw new UnauthorizedException("Empty token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ if (!authData.startsWith(AuthConstants.AUTHORIZATION_BASIC_HEADER)) {
+ throw new UnauthorizedException("Invalid token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ String credential =
authData.substring(AuthConstants.AUTHORIZATION_BASIC_HEADER.length());
+ if (credential.trim().isEmpty()) {
+ throw new BadRequestException("Malformed Basic authorization header:
missing credentials");
+ }
+
+ try {
+ String decodedCredential =
+ new String(Base64.getDecoder().decode(credential),
StandardCharsets.UTF_8);
+ int separatorIndex = decodedCredential.indexOf(':');
+ if (separatorIndex < 0) {
+ throw new BadRequestException(
+ "Malformed Basic authorization header: credentials must be in
username:password format");
+ }
+
+ String userName = decodedCredential.substring(0, separatorIndex);
+ if (userName.isEmpty()) {
+ throw new BadRequestException(
+ "Malformed Basic authorization header: username must not be
empty");
+ }
+
+ return new UserPrincipal(userName, authData);
+ } catch (IllegalArgumentException e) {
+ throw new BadRequestException(e, "Malformed Basic authorization header:
invalid base64");
+ }
Review Comment:
`BasicAuthenticator` currently treats any well-formed `Basic
<base64(username:password)>` header as authenticated and returns a
`UserPrincipal` without validating the password. With
`gravitino.authenticators=basic`, this enables user impersonation (including
privileged usernames) and is not safe as a default behavior. Consider failing
closed (always `UnauthorizedException`) until credential verification is
implemented, or implement minimal credential validation in this PR so BASIC is
not a bypass.
##########
server-common/src/test/java/org/apache/gravitino/server/authentication/TestBasicAuthenticator.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.authentication;
+
+import java.nio.charset.StandardCharsets;
+import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.exceptions.BadRequestException;
+import org.apache.gravitino.exceptions.UnauthorizedException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestBasicAuthenticator {
+
+ private final BasicAuthenticator authenticator = new BasicAuthenticator();
+
+ @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));
+ }
+
+ @Test
+ public void testAuthenticateTokenThrowsUnauthorizedWhenHeaderBlank() {
+ UnauthorizedException exception =
+ Assertions.assertThrows(
+ UnauthorizedException.class,
+ () -> authenticator.authenticateToken("
".getBytes(StandardCharsets.UTF_8)));
+
+ Assertions.assertEquals("Empty token authorization header",
exception.getMessage());
+ Assertions.assertEquals("Basic", exception.getChallenges().get(0));
+ }
+
+ @Test
+ public void
testAuthenticateTokenThrowsBadRequestWhenBasicCredentialMissing() {
+ BadRequestException exception =
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () ->
+ authenticator.authenticateToken(
+
AuthConstants.AUTHORIZATION_BASIC_HEADER.getBytes(StandardCharsets.UTF_8)));
+
+ Assertions.assertEquals(
+ "Malformed Basic authorization header: missing credentials",
exception.getMessage());
+ }
+
+ @Test
+ public void
testAuthenticateTokenThrowsBadRequestWhenBasicCredentialIsMalformed() {
+ BadRequestException exception =
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () ->
+ authenticator.authenticateToken(
+ (AuthConstants.AUTHORIZATION_BASIC_HEADER + "not-base64")
+ .getBytes(StandardCharsets.UTF_8)));
+
+ Assertions.assertEquals(
+ "Malformed Basic authorization header: invalid base64",
exception.getMessage());
+ }
Review Comment:
`BasicAuthenticator` introduces additional malformed-credential branches
(e.g., decoded credentials missing `:` and empty username) with specific error
messages, but the test suite only covers missing/blank headers, missing
credentials, and invalid base64. Add tests for the remaining new branches to
ensure the intended 400/401 behavior and messages stay stable.
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/BasicAuthenticator.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.authentication;
+
+import java.nio.charset.StandardCharsets;
+import java.security.Principal;
+import java.util.Base64;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.UserPrincipal;
+import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.exceptions.BadRequestException;
+import org.apache.gravitino.exceptions.UnauthorizedException;
+
+/**
+ * Basic authenticator wiring for the local authentication module.
+ *
+ * <p>The credential verification flow will be implemented in follow-up
subtasks. For now, keep the
+ * current Basic header parsing behavior so the module can be loaded safely
when {@code
+ * gravitino.authenticators=basic}.
+ */
+public class BasicAuthenticator implements Authenticator {
+ private static final String BASIC_AUTH_CHALLENGE = "Basic";
+
+ @Override
+ public boolean isDataFromToken() {
+ return true;
+ }
+
+ @Override
+ public Principal authenticateToken(byte[] tokenData) {
+ if (tokenData == null) {
+ throw new UnauthorizedException("Empty token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ String authData = new String(tokenData, StandardCharsets.UTF_8);
+ if (authData.trim().isEmpty()) {
+ throw new UnauthorizedException("Empty token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ if (!authData.startsWith(AuthConstants.AUTHORIZATION_BASIC_HEADER)) {
+ throw new UnauthorizedException("Invalid token authorization header",
BASIC_AUTH_CHALLENGE);
+ }
+
+ String credential =
authData.substring(AuthConstants.AUTHORIZATION_BASIC_HEADER.length());
+ if (credential.trim().isEmpty()) {
+ throw new BadRequestException("Malformed Basic authorization header:
missing credentials");
+ }
+
+ try {
+ String decodedCredential =
+ new String(Base64.getDecoder().decode(credential),
StandardCharsets.UTF_8);
+ int separatorIndex = decodedCredential.indexOf(':');
Review Comment:
`credential` is checked with `trim().isEmpty()` but is then decoded without
trimming. Headers like `"Basic <base64>"` (multiple spaces after the scheme)
will include leading spaces in `credential` and will be rejected as invalid
base64 even though they are syntactically valid per HTTP auth header grammar.
Trim/strip the credential before Base64 decoding to make the parser robust.
##########
authenticators/authenticator-basic/build.gradle.kts:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+plugins {
+ `maven-publish`
+ id("java")
+ id("idea")
+}
+
+dependencies {
+ testImplementation(libs.junit.jupiter.api)
+ testRuntimeOnly(libs.junit.jupiter.engine)
+}
+
+tasks {
Review Comment:
The new authenticator module currently contains only a build file and no
production sources/resources. As a result, publishing/depending on
`:authenticators:authenticator-basic` adds build surface area but does not
actually provide the `BasicAuthenticator` implementation (which is in
`server-common`). Either move the Basic authenticator implementation into this
module (and add the required `implementation` dependencies), or drop this
module until it contains the intended code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
