Copilot commented on code in PR #11226: URL: https://github.com/apache/gravitino/pull/11226#discussion_r3302175426
########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/auth/ServiceAdminInitializer.java: ########## @@ -0,0 +1,184 @@ +/* + * 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.idp.auth; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.auth.AuthenticatorType; +import org.apache.gravitino.idp.basic.password.PasswordHasher; +import org.apache.gravitino.idp.basic.password.PasswordHasherFactory; +import org.apache.gravitino.idp.storage.po.IdpUserPO; +import org.apache.gravitino.idp.storage.service.IdpUserMetaService; +import org.apache.gravitino.json.JsonUtils; +import org.apache.gravitino.storage.IdGenerator; +import org.apache.gravitino.storage.relational.utils.POConverters; + +/** Initializes configured service admins in the built-in IdP during server startup. */ +public final class ServiceAdminInitializer { + + static final String INITIAL_ADMIN_PASSWORD_ENV = "GRAVITINO_INITIAL_ADMIN_PASSWORD"; + + private static final String BASIC_AUTHENTICATOR = AuthenticatorType.BASIC.name().toLowerCase(); + + private ServiceAdminInitializer() {} + + /** + * Initialize the service admins using the current runtime environment. + * + * @param config The configuration object to initialize the service admins. + */ + public static void initialize(Config config) throws IOException { + initialize( + config, + IdpUserMetaService.getInstance(), + PasswordHasherFactory.create(), + GravitinoEnv.getInstance().idGenerator(), + System.getenv(INITIAL_ADMIN_PASSWORD_ENV)); + } + + static void initialize( + Config config, + IdpUserMetaService userMetaService, + PasswordHasher passwordHasher, + IdGenerator idGenerator, + @Nullable String initialAdminPasswords) + throws IOException { + if (!enabledBasicAuthenticator(config)) { + return; + } + + List<String> serviceAdmins = configuredServiceAdmins(config); + if (serviceAdmins.isEmpty()) { + return; + } + + Map<String, String> initialPasswords = + parseInitialAdminPasswords(serviceAdmins, initialAdminPasswords); + for (String serviceAdmin : serviceAdmins) { + validateUserName(serviceAdmin); + if (userMetaService.idpUserExists(serviceAdmin)) { + continue; + } + + String password = initialPasswords.get(serviceAdmin); + Preconditions.checkArgument( + StringUtils.isNotBlank(password), + "Missing initial password for configured service admin %s; declare %s", + serviceAdmin, + INITIAL_ADMIN_PASSWORD_ENV); + userMetaService.insertIdpUser( + IdpUserPO.builder() + .withUserId(idGenerator.nextId()) + .withUsername(serviceAdmin) + .withPasswordHash(passwordHasher.hash(password)) + .withCurrentVersion(POConverters.INIT_VERSION) + .withLastVersion(POConverters.INIT_VERSION) + .withDeletedAt(POConverters.DEFAULT_DELETED_AT) + .build()); + } + } + + private static boolean enabledBasicAuthenticator(Config config) { + return config.get(Configs.AUTHENTICATORS).contains(BASIC_AUTHENTICATOR); + } + + private static List<String> configuredServiceAdmins(Config config) { + List<String> serviceAdmins = config.get(Configs.SERVICE_ADMINS); + if (serviceAdmins == null || serviceAdmins.isEmpty()) { + return ImmutableList.of(); + } + return ImmutableList.copyOf(serviceAdmins); + } + + private static Map<String, String> parseInitialAdminPasswords( + List<String> serviceAdmins, @Nullable String initialAdminPasswords) { + if (StringUtils.isBlank(initialAdminPasswords)) { + return ImmutableMap.of(); + } + + final List<String> entries; + try { + entries = + JsonUtils.objectMapper() + .readValue(initialAdminPasswords, new TypeReference<List<String>>() {}); + } catch (JsonProcessingException e) { + throw new IllegalArgumentException( + INITIAL_ADMIN_PASSWORD_ENV + " must be a JSON array of 'username:password' strings", e); + } + + Map<String, String> passwordsByAdmin = new LinkedHashMap<>(); + for (String entry : entries) { + Preconditions.checkArgument( + StringUtils.isNotBlank(entry), + "%s must not contain blank entries", + INITIAL_ADMIN_PASSWORD_ENV); + + int separatorIndex = entry.indexOf(':'); + Preconditions.checkArgument( + separatorIndex > 0, + "%s entry '%s' must use the format username:password", + INITIAL_ADMIN_PASSWORD_ENV, + entry); + + String userName = entry.substring(0, separatorIndex); + String password = entry.substring(separatorIndex + 1); + validateUserName(userName); + validatePassword(password); + Preconditions.checkArgument( + serviceAdmins.contains(userName), + "%s entry '%s' is not a configured service admin", + INITIAL_ADMIN_PASSWORD_ENV, + userName); + Preconditions.checkArgument( + !passwordsByAdmin.containsKey(userName), + "%s contains duplicate entries for service admin %s", + INITIAL_ADMIN_PASSWORD_ENV, + userName); + passwordsByAdmin.put(userName, password); + } Review Comment: Inside the loop, `serviceAdmins.contains(userName)` is an O(n) lookup on a `List`, making the parsing step O(n*m) for m password entries (and similarly in other call sites that check membership repeatedly). Consider converting `serviceAdmins` to a `Set` once (e.g., `ImmutableSet`) and using `contains` on that to keep lookups O(1), especially if the configured admin list can grow. ########## server-common/src/main/java/org/apache/gravitino/server/plugin/ServerPluginBootstrapper.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.plugin; + +import java.util.ServiceLoader; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Loads and runs {@link ServerPluginBootstrap} providers from the server classpath. */ +public final class ServerPluginBootstrapper { + + private static final Logger LOG = LoggerFactory.getLogger(ServerPluginBootstrapper.class); + + private ServerPluginBootstrapper() {} + + /** Initializes all {@link ServerPluginBootstrap} providers present on the classpath. */ + public static void initialize() { + ServiceLoader<ServerPluginBootstrap> loader = ServiceLoader.load(ServerPluginBootstrap.class); + for (ServerPluginBootstrap bootstrap : loader) { + try { + LOG.info("Initializing server plugin bootstrap: {}", bootstrap.name()); + bootstrap.initializeOnce(); + } catch (RuntimeException e) { + throw e; + } catch (Exception e) { + throw new IllegalStateException( + String.format("Failed to initialize server plugin bootstrap: %s", bootstrap.name()), e); + } + } + } Review Comment: Two issues here: (1) the `catch (RuntimeException e) { throw e; }` block is redundant and provides no additional behavior; (2) `ServiceLoader` iteration can throw `ServiceConfigurationError` (an `Error`), which will currently bypass this handling and lose the bootstrap name context. Consider removing the redundant runtime catch, and explicitly catching `ServiceConfigurationError` (or more generally `Throwable` if desired) to wrap/log with the provider name to improve startup diagnosability. ########## server-common/src/test/java/org/apache/gravitino/server/authentication/TestBasicAuthentication.java: ########## @@ -0,0 +1,126 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.common.collect.Lists; +import java.lang.reflect.Constructor; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.Collections; +import java.util.Vector; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.apache.gravitino.UserPrincipal; +import org.apache.gravitino.auth.AuthConstants; +import org.apache.gravitino.idp.auth.BasicAuthenticator; +import org.apache.gravitino.idp.basic.password.PasswordHasher; +import org.apache.gravitino.idp.exception.NotFoundException; +import org.apache.gravitino.idp.storage.po.IdpUserPO; +import org.apache.gravitino.idp.storage.service.IdpUserMetaService; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +public class TestBasicAuthentication { + + private static final String USER = "alice"; + private static final String PASSWORD = "Passw0rd-For-Alice"; + private static final String PASSWORD_HASH = "hash-1"; + + @Test + public void testFilterSuccess() throws Exception { + BasicAuthenticator authenticator = aliceAuthenticator(true); + FilterChain chain = mock(FilterChain.class); + HttpServletRequest request = mock(HttpServletRequest.class); + HttpServletResponse response = mock(HttpServletResponse.class); + stubAuthHeader(request, USER, PASSWORD); + + new AuthenticationFilter(Lists.newArrayList(authenticator)).doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + verify(response, never()).sendError(anyInt(), anyString()); + ArgumentCaptor<Object> principalCaptor = ArgumentCaptor.forClass(Object.class); + verify(request) + .setAttribute( + eq(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME), principalCaptor.capture()); + assertEquals(USER, ((UserPrincipal) principalCaptor.getValue()).getName()); + } + + @Test + public void testFilterUnauthorized() throws Exception { + IdpUserMetaService userMetaService = mock(IdpUserMetaService.class); + when(userMetaService.getIdpUserByUsername(USER)) + .thenThrow(new NotFoundException("IdP user not found: %s", USER)); + BasicAuthenticator authenticator = + createBasicAuthenticator(userMetaService, mock(PasswordHasher.class)); + FilterChain chain = mock(FilterChain.class); + HttpServletRequest request = mock(HttpServletRequest.class); + HttpServletResponse response = mock(HttpServletResponse.class); + stubAuthHeader(request, USER, "wrong"); + + new AuthenticationFilter(Lists.newArrayList(authenticator)).doFilter(request, response, chain); + + verify(response).setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, "Basic"); + verify(response).sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid username or password"); + verify(chain, never()).doFilter(request, response); + } + + private static BasicAuthenticator aliceAuthenticator(boolean passwordValid) throws Exception { + IdpUserMetaService userMetaService = mock(IdpUserMetaService.class); + PasswordHasher passwordHasher = mock(PasswordHasher.class); + IdpUserPO userPO = mock(IdpUserPO.class); + when(userMetaService.getIdpUserByUsername(USER)).thenReturn(userPO); + when(userPO.getPasswordHash()).thenReturn(PASSWORD_HASH); + when(passwordHasher.verify(PASSWORD, PASSWORD_HASH)).thenReturn(passwordValid); + when(userMetaService.listGroupNamesByUsername(USER)).thenReturn(Collections.emptyList()); + return createBasicAuthenticator(userMetaService, passwordHasher); + } + + private static void stubAuthHeader(HttpServletRequest request, String username, String password) { + when(request.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION)) + .thenReturn( + new Vector<>(Collections.singletonList(basicAuthHeader(username, password))) + .elements()); + } + + private static BasicAuthenticator createBasicAuthenticator( + IdpUserMetaService userMetaService, PasswordHasher passwordHasher) throws Exception { + Constructor<BasicAuthenticator> constructor = + BasicAuthenticator.class.getDeclaredConstructor( + IdpUserMetaService.class, PasswordHasher.class); + constructor.setAccessible(true); + return constructor.newInstance(userMetaService, passwordHasher); + } Review Comment: This test relies on reflective access (`setAccessible(true)`) to a non-public constructor, which is brittle under stronger Java access controls and makes future refactors riskier. Prefer exposing a public (or `@VisibleForTesting`) constructor/factory on `BasicAuthenticator`, or move this test to the same package/module as `BasicAuthenticator` so it can use package-private access without reflection. ########## clients/client-java/src/main/java/org/apache/gravitino/client/BasicTokenProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.client; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import org.apache.gravitino.auth.AuthConstants; + +/** Provides HTTP Basic credentials for Gravitino built-in IdP authentication. */ +final class BasicTokenProvider implements AuthDataProvider { + + private final byte[] token; + + BasicTokenProvider(String userName, String password) { + this.token = buildToken(userName, password); + } + + private static byte[] buildToken(String userName, String password) { + String userInformation = userName + ":" + password; + return (AuthConstants.AUTHORIZATION_BASIC_HEADER + + new String( + Base64.getEncoder().encode(userInformation.getBytes(StandardCharsets.UTF_8)), + StandardCharsets.UTF_8)) Review Comment: This does an extra encode-to-bytes then bytes-to-String conversion. Using `Base64.getEncoder().encodeToString(...)` avoids the intermediate byte array/string creation and is simpler, while producing the same output. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpServerPluginBootstrap.java: ########## @@ -0,0 +1,67 @@ +/* + * 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.idp; + +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.gravitino.Config; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.idp.auth.ServiceAdminInitializer; +import org.apache.gravitino.server.plugin.ServerPluginBootstrap; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Server plugin bootstrap for the built-in IdP (idp-basic). + * + * <p>Initializes configured service admins once per JVM when the plugin is on the server classpath. + */ +public final class IdpServerPluginBootstrap implements ServerPluginBootstrap { + + private static final Logger LOG = LoggerFactory.getLogger(IdpServerPluginBootstrap.class); + + private static final String NAME = "idp-basic"; + + private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false); + + @Override + public String name() { + return NAME; + } + + @Override + public void initializeOnce() throws Exception { + if (!INITIALIZED.compareAndSet(false, true)) { + return; + } + + Config config = GravitinoEnv.getInstance().config(); + try { + ServiceAdminInitializer.initialize(config); + } catch (RuntimeException e) { + INITIALIZED.set(false); + LOG.error("Failed to initialize built-in IdP plugin", e); + throw e; + } catch (Exception e) { + INITIALIZED.set(false); + LOG.error("Failed to initialize built-in IdP plugin", e); + throw e; + } Review Comment: These two catch blocks are identical. You can simplify the method and reduce duplication by catching a single type (e.g., `Exception`) and rethrowing it; if you need to preserve unchecked exceptions separately, catch `Throwable` and rethrow after resetting `INITIALIZED` and logging. -- 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]
