Copilot commented on code in PR #5107: URL: https://github.com/apache/polaris/pull/5107#discussion_r3614371572
########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakTestResource.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Splitter; +import io.quarkus.test.common.QuarkusTestResourceLifecycleManager; +import java.util.Map; + +public class KeycloakTestResource implements QuarkusTestResourceLifecycleManager { + + /** + * Initialization argument key for specifying roles to be created in the Keycloak server. + * + * <p>The value associated with this key should be a comma-separated list of role names. + */ + public static final String ROLES_INIT_ARG = "roles"; + + /** + * Initialization argument key for specifying users to be created in the Keycloak server. + * + * <p>The value associated with this key should be a comma-separated list of user entries, where + * each entry is represented as a key-value pair in the format "username=password". + */ + public static final String USERS_INIT_ARG = "users"; + + /** + * Initialization argument key for specifying role grants to users in the Keycloak server. + * + * <p>The value associated with this key should be a comma-separated list of grant entries, where + * each entry is represented as a key-value pair in the format "username=role". + */ + public static final String GRANTS_INIT_ARG = "grants"; + + /** + * Initialization argument key for specifying service accounts to be created in the Keycloak + * server. + * + * <p>The value associated with this key should be a comma-separated list of client entries, where + * each entry is represented as a key-value pair in the format "client_id=client_secret". + */ + public static final String CLIENTS_INIT_ARG = "clients"; + + private Map<String, String> initArgs = Map.of(); + private KeycloakContainer keycloak; + + @Override + public void init(Map<String, String> initArgs) { + this.initArgs = Map.copyOf(initArgs); + } + + @Override + public void inject(TestInjector testInjector) { + testInjector.injectIntoFields( + keycloak, new TestInjector.AnnotatedAndMatchesType(Keycloak.class, KeycloakAccess.class)); + } + + @Override + public Map<String, String> start() { + keycloak = new KeycloakContainer(); + keycloak.start(); + + var roles = Splitter.on(",").split(initArgs.get(ROLES_INIT_ARG)); + var users = Splitter.on(",").withKeyValueSeparator("=").split(initArgs.get(USERS_INIT_ARG)); + var grants = Splitter.on(",").withKeyValueSeparator("=").split(initArgs.get(GRANTS_INIT_ARG)); + var clients = Splitter.on(",").withKeyValueSeparator("=").split(initArgs.get(CLIENTS_INIT_ARG)); + Review Comment: Splitter usage will throw when an init arg is missing (null) and will also treat an empty string as a single empty entry (e.g., clients="" becomes one entry without '=' and fails). This makes the Keycloak test resource fragile when callers omit args or have no clients/roles to create. ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogKeycloakFileIT.java: ########## @@ -19,28 +19,62 @@ package org.apache.polaris.service.it; import io.quarkus.test.junit.QuarkusIntegrationTest; +import io.quarkus.test.junit.QuarkusTestProfile; import io.quarkus.test.junit.TestProfile; +import java.util.List; import java.util.Map; +import java.util.Optional; +import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; import org.apache.polaris.service.it.env.ClientPrincipal; import org.apache.polaris.service.it.env.ManagementApi; import org.apache.polaris.service.it.env.PolarisClient; import org.apache.polaris.service.it.test.PolarisRestCatalogFileIntegrationTest; -import org.apache.polaris.test.commons.keycloak.KeycloakAccess; -import org.apache.polaris.test.commons.keycloak.KeycloakProfile; +import org.apache.polaris.test.keycloak.Keycloak; +import org.apache.polaris.test.keycloak.KeycloakAccess; +import org.apache.polaris.test.keycloak.KeycloakTestResource; @QuarkusIntegrationTest -@TestProfile(KeycloakProfile.class) +@TestProfile(RestCatalogKeycloakFileIT.Profile.class) public class RestCatalogKeycloakFileIT extends PolarisRestCatalogFileIntegrationTest { - KeycloakAccess keycloak; + public static class Profile implements QuarkusTestProfile { + + @Override + public Map<String, String> getConfigOverrides() { + return Map.of( + "quarkus.oidc.tenant-enabled", "true", + "quarkus.oidc.client-id", "polaris", + "polaris.authentication.type", "external", + "polaris.oidc.principal-mapper.name-claim-path", KeycloakAccess.PRINCIPAL_NAME_CLAIM, + "polaris.oidc.principal-roles-mapper.filter", "PRINCIPAL_ROLE:.*"); + } + + @Override + public List<TestResourceEntry> testResources() { + Optional<String> clients = + RootCredentialsSet.fromEnvironment().credentials().values().stream() + .map(creds -> creds.clientId() + "=" + creds.clientSecret()) + .reduce((a, b) -> a + "," + b); + return List.of( + new TestResourceEntry( + KeycloakTestResource.class, + Map.of( + KeycloakTestResource.ROLES_INIT_ARG, "PRINCIPAL_ROLE:ALL", + KeycloakTestResource.USERS_INIT_ARG, "root=s3cr3t", + KeycloakTestResource.GRANTS_INIT_ARG, "root=PRINCIPAL_ROLE:ALL", + KeycloakTestResource.CLIENTS_INIT_ARG, clients.orElse("")))); + } + } + + @Keycloak KeycloakAccess keycloak; @Override protected ClientPrincipal createTestPrincipal( PolarisClient client, String principalName, String principalRole) { ClientPrincipal principal = super.createTestPrincipal(client, principalName, principalRole); - keycloak.createRole(principalRole); - keycloak.createUser(principalName); - keycloak.assignRoleToUser(principalRole, principalName); + keycloak.createRole("PRINCIPAL_ROLE:" + principalRole); + keycloak.createUser(principalName, "s3cr3t"); + keycloak.assignRoleToUser("PRINCIPAL_ROLE:" + principalRole, principalName); Review Comment: Role names are being passed with the "PRINCIPAL_ROLE:" prefix even though KeycloakAccess documents callers should pass unprefixed role names. This couples test code to a particular implementation detail and can lead to inconsistent role naming. ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogKeycloakFileIT.java: ########## @@ -19,28 +19,62 @@ package org.apache.polaris.service.it; import io.quarkus.test.junit.QuarkusIntegrationTest; +import io.quarkus.test.junit.QuarkusTestProfile; import io.quarkus.test.junit.TestProfile; +import java.util.List; import java.util.Map; +import java.util.Optional; +import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; import org.apache.polaris.service.it.env.ClientPrincipal; import org.apache.polaris.service.it.env.ManagementApi; import org.apache.polaris.service.it.env.PolarisClient; import org.apache.polaris.service.it.test.PolarisRestCatalogFileIntegrationTest; -import org.apache.polaris.test.commons.keycloak.KeycloakAccess; -import org.apache.polaris.test.commons.keycloak.KeycloakProfile; +import org.apache.polaris.test.keycloak.Keycloak; +import org.apache.polaris.test.keycloak.KeycloakAccess; +import org.apache.polaris.test.keycloak.KeycloakTestResource; @QuarkusIntegrationTest -@TestProfile(KeycloakProfile.class) +@TestProfile(RestCatalogKeycloakFileIT.Profile.class) public class RestCatalogKeycloakFileIT extends PolarisRestCatalogFileIntegrationTest { - KeycloakAccess keycloak; + public static class Profile implements QuarkusTestProfile { + + @Override + public Map<String, String> getConfigOverrides() { + return Map.of( + "quarkus.oidc.tenant-enabled", "true", + "quarkus.oidc.client-id", "polaris", + "polaris.authentication.type", "external", + "polaris.oidc.principal-mapper.name-claim-path", KeycloakAccess.PRINCIPAL_NAME_CLAIM, + "polaris.oidc.principal-roles-mapper.filter", "PRINCIPAL_ROLE:.*"); + } + + @Override + public List<TestResourceEntry> testResources() { + Optional<String> clients = + RootCredentialsSet.fromEnvironment().credentials().values().stream() + .map(creds -> creds.clientId() + "=" + creds.clientSecret()) + .reduce((a, b) -> a + "," + b); + return List.of( + new TestResourceEntry( + KeycloakTestResource.class, + Map.of( + KeycloakTestResource.ROLES_INIT_ARG, "PRINCIPAL_ROLE:ALL", + KeycloakTestResource.USERS_INIT_ARG, "root=s3cr3t", + KeycloakTestResource.GRANTS_INIT_ARG, "root=PRINCIPAL_ROLE:ALL", + KeycloakTestResource.CLIENTS_INIT_ARG, clients.orElse("")))); Review Comment: KeycloakAccess explicitly documents that role names passed to createRole/assignRoleToUser/deleteRole should NOT include the "PRINCIPAL_ROLE:" prefix. Passing prefixed role names here makes callers inconsistent and risks double-prefixing if the KeycloakAccess implementation applies the prefix internally (as the previous implementation did). ########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakContainer.java: ########## @@ -0,0 +1,308 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Preconditions; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URLEncoder; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import org.apache.polaris.containerspec.ContainerSpecHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import tools.jackson.databind.JsonNode; +import tools.jackson.databind.json.JsonMapper; + +public class KeycloakContainer extends GenericContainer<KeycloakContainer> + implements KeycloakAccess { + + private static final Logger LOGGER = LoggerFactory.getLogger(KeycloakContainer.class); + + private static final int KEYCLOAK_PORT = 8080; + private static final String REALM = "master"; + private static final String ADMIN_USERNAME = "admin"; + private static final String ADMIN_PASSWORD = "admin"; + + private URI issuerUrl; + private URI tokenEndpoint; + private HttpClient httpClient; + + @SuppressWarnings("resource") + public KeycloakContainer() { + super( + ContainerSpecHelper.containerSpecHelper("keycloak", KeycloakContainer.class) + .dockerImageName(null) + .asCanonicalNameString()); + withExposedPorts(KEYCLOAK_PORT); + withEnv("KEYCLOAK_ADMIN", ADMIN_USERNAME); + withEnv("KEYCLOAK_ADMIN_PASSWORD", ADMIN_PASSWORD); + withEnv("KC_LOG_LEVEL", getRootLoggerLevel() + ",org.keycloak:" + getKeycloakLoggerLevel()); + withCommand("start-dev"); + waitingFor( + Wait.forHttp("/realms/" + REALM) + .forStatusCode(200) + .withStartupTimeout(Duration.ofMinutes(2))); + withLogConsumer(new Slf4jLogConsumer(LOGGER)); + } + + @Override + public void start() { + super.start(); + httpClient = HttpClient.newHttpClient(); + String baseUrl = "http://"; + getHost() + ":" + getMappedPort(KEYCLOAK_PORT); + issuerUrl = URI.create(baseUrl + "/realms/" + REALM + "/"); + tokenEndpoint = issuerUrl.resolve("protocol/openid-connect/token"); + } + + @Override + public void stop() { + super.stop(); + httpClient = null; + } + + @Override + public URI getIssuerUrl() { + return issuerUrl; + } + + @Override + public URI getTokenEndpoint() { + return tokenEndpoint; + } + + /* + * The methods below were taken from org.keycloak:keycloak-admin-client. We don't depend on that + * artifact directly to avoid bringing in RESTEasy Classic transitively. + */ + + @Override + public void createRole(String roleName) { + String token = getAdminToken(); + int status = adminPost(realmAdminUrl("/roles"), "{\"name\":\"" + roleName + "\"}", token); + Preconditions.checkState( + status == 201, "Failed to create role '%s', status: %s", roleName, status); + } Review Comment: KeycloakAccess specifies that role names passed to createRole should not include the "PRINCIPAL_ROLE:" prefix, but this implementation currently uses the provided name verbatim. This makes callers responsible for prefixing and conflicts with the interface contract (and previous behavior). ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogKeycloakFileIT.java: ########## @@ -72,7 +106,7 @@ protected void cleanUp(PolarisClient client, String adminToken) { }); managementApi.listPrincipalRoles().stream() .filter(r -> client.ownedName(r.getName())) - .forEach(role -> keycloak.deleteRole(role.getName())); + .forEach(role -> keycloak.deleteRole("PRINCIPAL_ROLE:" + role.getName())); Review Comment: KeycloakAccess documents deleteRole should receive an unprefixed role name, but this call passes a prefixed value. Keeping callers unprefixed avoids double-prefixing and keeps behavior consistent across implementations. ########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakContainer.java: ########## @@ -0,0 +1,308 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Preconditions; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URLEncoder; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import org.apache.polaris.containerspec.ContainerSpecHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import tools.jackson.databind.JsonNode; +import tools.jackson.databind.json.JsonMapper; + +public class KeycloakContainer extends GenericContainer<KeycloakContainer> + implements KeycloakAccess { + + private static final Logger LOGGER = LoggerFactory.getLogger(KeycloakContainer.class); + + private static final int KEYCLOAK_PORT = 8080; + private static final String REALM = "master"; + private static final String ADMIN_USERNAME = "admin"; + private static final String ADMIN_PASSWORD = "admin"; + + private URI issuerUrl; + private URI tokenEndpoint; + private HttpClient httpClient; + + @SuppressWarnings("resource") + public KeycloakContainer() { + super( + ContainerSpecHelper.containerSpecHelper("keycloak", KeycloakContainer.class) + .dockerImageName(null) + .asCanonicalNameString()); + withExposedPorts(KEYCLOAK_PORT); + withEnv("KEYCLOAK_ADMIN", ADMIN_USERNAME); + withEnv("KEYCLOAK_ADMIN_PASSWORD", ADMIN_PASSWORD); + withEnv("KC_LOG_LEVEL", getRootLoggerLevel() + ",org.keycloak:" + getKeycloakLoggerLevel()); + withCommand("start-dev"); + waitingFor( + Wait.forHttp("/realms/" + REALM) + .forStatusCode(200) + .withStartupTimeout(Duration.ofMinutes(2))); + withLogConsumer(new Slf4jLogConsumer(LOGGER)); + } + + @Override + public void start() { + super.start(); + httpClient = HttpClient.newHttpClient(); + String baseUrl = "http://"; + getHost() + ":" + getMappedPort(KEYCLOAK_PORT); + issuerUrl = URI.create(baseUrl + "/realms/" + REALM + "/"); + tokenEndpoint = issuerUrl.resolve("protocol/openid-connect/token"); + } + + @Override + public void stop() { + super.stop(); + httpClient = null; + } + + @Override + public URI getIssuerUrl() { + return issuerUrl; + } + + @Override + public URI getTokenEndpoint() { + return tokenEndpoint; + } + + /* + * The methods below were taken from org.keycloak:keycloak-admin-client. We don't depend on that + * artifact directly to avoid bringing in RESTEasy Classic transitively. + */ + + @Override + public void createRole(String roleName) { + String token = getAdminToken(); + int status = adminPost(realmAdminUrl("/roles"), "{\"name\":\"" + roleName + "\"}", token); + Preconditions.checkState( + status == 201, "Failed to create role '%s', status: %s", roleName, status); + } + + @Override + public void createUser(String name, String password) { + String token = getAdminToken(); + String body = + """ + { + "enabled": true, + "username": "%s", + "firstName": "%s", + "lastName": "%s", + "email": "%[email protected]", + "emailVerified": true, + "requiredActions": [], + "credentials": [ + { + "type": "password", + "value": "%s", + "temporary": false + } + ] + }""" + .formatted(name, name, name, name, password); + int status = adminPost(realmAdminUrl("/users"), body, token); + Preconditions.checkState(status == 201, "Failed to create user '%s', status: %s", name, status); + } + + @Override + public void assignRoleToUser(String role, String user) { + String token = getAdminToken(); + String userId = findUserId(user, token); + JsonNode roleRep = adminGet(realmAdminUrl("/roles/" + encode(role)), token); + String body = "[" + JsonMapper.shared().writeValueAsString(roleRep) + "]"; + int status = adminPost(realmAdminUrl("/users/" + userId + "/role-mappings/realm"), body, token); + Preconditions.checkState( + status == 204, "Failed to assign role '%s' to user '%s', status: %s", role, user, status); + } + + @Override + public void createServiceAccount(String clientId, String clientSecret) { + String token = getAdminToken(); + String body = + """ + { + "clientId": "%s", + "secret": "%s", + "publicClient": false, + "serviceAccountsEnabled": true, + "directAccessGrantsEnabled": true + }""" + .formatted(clientId, clientSecret); + int status = adminPost(realmAdminUrl("/clients"), body, token); + Preconditions.checkState( + status == 201, "Failed to create client '%s', status: %s", clientId, status); + } + + @Override + public void deleteRole(String name) { + String token = getAdminToken(); + int status = adminDelete(realmAdminUrl("/roles/" + encode(name)), token); + Preconditions.checkState(status == 204, "Failed to delete role '%s', status: %s", name, status); + } + + @Override + public void deleteUser(String name) { + String token = getAdminToken(); + JsonNode users = adminGet(realmAdminUrl("/users?username=" + encode(name)), token); + for (JsonNode user : users) { + if (name.equals(user.get("username").asString())) { + adminDelete(realmAdminUrl("/users/" + user.get("id").asString()), token); + } + } + } + + @Override + public void deleteServiceAccount(String clientId) { + String token = getAdminToken(); + JsonNode clients = adminGet(realmAdminUrl("/clients?clientId=" + encode(clientId)), token); + for (JsonNode client : clients) { + adminDelete(realmAdminUrl("/clients/" + client.get("id").asString()), token); + } + } + + private String getAdminToken() { + String formBody = + "grant_type=password" + + "&client_id=admin-cli" + + "&username=" + + ADMIN_USERNAME + + "&password=" + + ADMIN_PASSWORD; + try { + HttpRequest request = + HttpRequest.newBuilder() + .uri(tokenEndpoint) + .header("Content-Type", "application/x-www-form-urlencoded") + .POST(HttpRequest.BodyPublishers.ofString(formBody)) + .build(); + HttpResponse<InputStream> response = + httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream()); + return JsonMapper.shared().readTree(response.body()).get("access_token").asString(); Review Comment: getAdminToken() parses the response body without validating the HTTP status or presence of access_token. If Keycloak returns a non-200 (startup timing, auth error, etc.), this will fail with a confusing Jackson/NullPointer error instead of a clear message. ########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakContainer.java: ########## @@ -0,0 +1,308 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Preconditions; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URLEncoder; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import org.apache.polaris.containerspec.ContainerSpecHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import tools.jackson.databind.JsonNode; +import tools.jackson.databind.json.JsonMapper; + +public class KeycloakContainer extends GenericContainer<KeycloakContainer> + implements KeycloakAccess { + + private static final Logger LOGGER = LoggerFactory.getLogger(KeycloakContainer.class); + + private static final int KEYCLOAK_PORT = 8080; + private static final String REALM = "master"; + private static final String ADMIN_USERNAME = "admin"; + private static final String ADMIN_PASSWORD = "admin"; + + private URI issuerUrl; + private URI tokenEndpoint; + private HttpClient httpClient; + + @SuppressWarnings("resource") + public KeycloakContainer() { + super( + ContainerSpecHelper.containerSpecHelper("keycloak", KeycloakContainer.class) + .dockerImageName(null) + .asCanonicalNameString()); + withExposedPorts(KEYCLOAK_PORT); + withEnv("KEYCLOAK_ADMIN", ADMIN_USERNAME); + withEnv("KEYCLOAK_ADMIN_PASSWORD", ADMIN_PASSWORD); + withEnv("KC_LOG_LEVEL", getRootLoggerLevel() + ",org.keycloak:" + getKeycloakLoggerLevel()); + withCommand("start-dev"); + waitingFor( + Wait.forHttp("/realms/" + REALM) + .forStatusCode(200) + .withStartupTimeout(Duration.ofMinutes(2))); + withLogConsumer(new Slf4jLogConsumer(LOGGER)); + } + + @Override + public void start() { + super.start(); + httpClient = HttpClient.newHttpClient(); + String baseUrl = "http://"; + getHost() + ":" + getMappedPort(KEYCLOAK_PORT); + issuerUrl = URI.create(baseUrl + "/realms/" + REALM + "/"); + tokenEndpoint = issuerUrl.resolve("protocol/openid-connect/token"); + } + + @Override + public void stop() { + super.stop(); + httpClient = null; + } + + @Override + public URI getIssuerUrl() { + return issuerUrl; + } + + @Override + public URI getTokenEndpoint() { + return tokenEndpoint; + } + + /* + * The methods below were taken from org.keycloak:keycloak-admin-client. We don't depend on that + * artifact directly to avoid bringing in RESTEasy Classic transitively. + */ + + @Override + public void createRole(String roleName) { + String token = getAdminToken(); + int status = adminPost(realmAdminUrl("/roles"), "{\"name\":\"" + roleName + "\"}", token); + Preconditions.checkState( + status == 201, "Failed to create role '%s', status: %s", roleName, status); + } + + @Override + public void createUser(String name, String password) { + String token = getAdminToken(); + String body = + """ + { + "enabled": true, + "username": "%s", + "firstName": "%s", + "lastName": "%s", + "email": "%[email protected]", + "emailVerified": true, + "requiredActions": [], + "credentials": [ + { + "type": "password", + "value": "%s", + "temporary": false + } + ] + }""" + .formatted(name, name, name, name, password); + int status = adminPost(realmAdminUrl("/users"), body, token); + Preconditions.checkState(status == 201, "Failed to create user '%s', status: %s", name, status); + } + + @Override + public void assignRoleToUser(String role, String user) { + String token = getAdminToken(); + String userId = findUserId(user, token); + JsonNode roleRep = adminGet(realmAdminUrl("/roles/" + encode(role)), token); + String body = "[" + JsonMapper.shared().writeValueAsString(roleRep) + "]"; + int status = adminPost(realmAdminUrl("/users/" + userId + "/role-mappings/realm"), body, token); + Preconditions.checkState( + status == 204, "Failed to assign role '%s' to user '%s', status: %s", role, user, status); + } + + @Override + public void createServiceAccount(String clientId, String clientSecret) { + String token = getAdminToken(); + String body = + """ + { + "clientId": "%s", + "secret": "%s", + "publicClient": false, + "serviceAccountsEnabled": true, + "directAccessGrantsEnabled": true + }""" + .formatted(clientId, clientSecret); + int status = adminPost(realmAdminUrl("/clients"), body, token); + Preconditions.checkState( + status == 201, "Failed to create client '%s', status: %s", clientId, status); + } + + @Override + public void deleteRole(String name) { + String token = getAdminToken(); + int status = adminDelete(realmAdminUrl("/roles/" + encode(name)), token); + Preconditions.checkState(status == 204, "Failed to delete role '%s', status: %s", name, status); + } Review Comment: KeycloakAccess documents deleteRole should receive an unprefixed role name, but this implementation currently deletes the role using the provided name verbatim. To keep callers consistent, apply the "PRINCIPAL_ROLE:" prefix inside the container before calling the admin API. ########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakContainer.java: ########## @@ -0,0 +1,308 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Preconditions; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URLEncoder; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import org.apache.polaris.containerspec.ContainerSpecHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import tools.jackson.databind.JsonNode; +import tools.jackson.databind.json.JsonMapper; + +public class KeycloakContainer extends GenericContainer<KeycloakContainer> + implements KeycloakAccess { + + private static final Logger LOGGER = LoggerFactory.getLogger(KeycloakContainer.class); + + private static final int KEYCLOAK_PORT = 8080; + private static final String REALM = "master"; + private static final String ADMIN_USERNAME = "admin"; + private static final String ADMIN_PASSWORD = "admin"; + + private URI issuerUrl; + private URI tokenEndpoint; + private HttpClient httpClient; + + @SuppressWarnings("resource") + public KeycloakContainer() { + super( + ContainerSpecHelper.containerSpecHelper("keycloak", KeycloakContainer.class) + .dockerImageName(null) + .asCanonicalNameString()); + withExposedPorts(KEYCLOAK_PORT); + withEnv("KEYCLOAK_ADMIN", ADMIN_USERNAME); + withEnv("KEYCLOAK_ADMIN_PASSWORD", ADMIN_PASSWORD); + withEnv("KC_LOG_LEVEL", getRootLoggerLevel() + ",org.keycloak:" + getKeycloakLoggerLevel()); + withCommand("start-dev"); + waitingFor( + Wait.forHttp("/realms/" + REALM) + .forStatusCode(200) + .withStartupTimeout(Duration.ofMinutes(2))); + withLogConsumer(new Slf4jLogConsumer(LOGGER)); + } + + @Override + public void start() { + super.start(); + httpClient = HttpClient.newHttpClient(); + String baseUrl = "http://"; + getHost() + ":" + getMappedPort(KEYCLOAK_PORT); + issuerUrl = URI.create(baseUrl + "/realms/" + REALM + "/"); + tokenEndpoint = issuerUrl.resolve("protocol/openid-connect/token"); + } + + @Override + public void stop() { + super.stop(); + httpClient = null; + } + + @Override + public URI getIssuerUrl() { + return issuerUrl; + } + + @Override + public URI getTokenEndpoint() { + return tokenEndpoint; + } + + /* + * The methods below were taken from org.keycloak:keycloak-admin-client. We don't depend on that + * artifact directly to avoid bringing in RESTEasy Classic transitively. + */ + + @Override + public void createRole(String roleName) { + String token = getAdminToken(); + int status = adminPost(realmAdminUrl("/roles"), "{\"name\":\"" + roleName + "\"}", token); + Preconditions.checkState( + status == 201, "Failed to create role '%s', status: %s", roleName, status); + } + + @Override + public void createUser(String name, String password) { + String token = getAdminToken(); + String body = + """ + { + "enabled": true, + "username": "%s", + "firstName": "%s", + "lastName": "%s", + "email": "%[email protected]", + "emailVerified": true, + "requiredActions": [], + "credentials": [ + { + "type": "password", + "value": "%s", + "temporary": false + } + ] + }""" + .formatted(name, name, name, name, password); + int status = adminPost(realmAdminUrl("/users"), body, token); + Preconditions.checkState(status == 201, "Failed to create user '%s', status: %s", name, status); + } + + @Override + public void assignRoleToUser(String role, String user) { + String token = getAdminToken(); + String userId = findUserId(user, token); + JsonNode roleRep = adminGet(realmAdminUrl("/roles/" + encode(role)), token); + String body = "[" + JsonMapper.shared().writeValueAsString(roleRep) + "]"; + int status = adminPost(realmAdminUrl("/users/" + userId + "/role-mappings/realm"), body, token); + Preconditions.checkState( + status == 204, "Failed to assign role '%s' to user '%s', status: %s", role, user, status); + } + + @Override + public void createServiceAccount(String clientId, String clientSecret) { + String token = getAdminToken(); + String body = + """ + { + "clientId": "%s", + "secret": "%s", + "publicClient": false, + "serviceAccountsEnabled": true, + "directAccessGrantsEnabled": true + }""" + .formatted(clientId, clientSecret); + int status = adminPost(realmAdminUrl("/clients"), body, token); + Preconditions.checkState( + status == 201, "Failed to create client '%s', status: %s", clientId, status); + } + + @Override + public void deleteRole(String name) { + String token = getAdminToken(); + int status = adminDelete(realmAdminUrl("/roles/" + encode(name)), token); + Preconditions.checkState(status == 204, "Failed to delete role '%s', status: %s", name, status); + } + + @Override + public void deleteUser(String name) { + String token = getAdminToken(); + JsonNode users = adminGet(realmAdminUrl("/users?username=" + encode(name)), token); + for (JsonNode user : users) { + if (name.equals(user.get("username").asString())) { + adminDelete(realmAdminUrl("/users/" + user.get("id").asString()), token); + } + } + } + + @Override + public void deleteServiceAccount(String clientId) { + String token = getAdminToken(); + JsonNode clients = adminGet(realmAdminUrl("/clients?clientId=" + encode(clientId)), token); + for (JsonNode client : clients) { + adminDelete(realmAdminUrl("/clients/" + client.get("id").asString()), token); + } + } + + private String getAdminToken() { + String formBody = + "grant_type=password" + + "&client_id=admin-cli" + + "&username=" + + ADMIN_USERNAME + + "&password=" + + ADMIN_PASSWORD; + try { + HttpRequest request = + HttpRequest.newBuilder() + .uri(tokenEndpoint) + .header("Content-Type", "application/x-www-form-urlencoded") + .POST(HttpRequest.BodyPublishers.ofString(formBody)) + .build(); + HttpResponse<InputStream> response = + httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream()); + return JsonMapper.shared().readTree(response.body()).get("access_token").asString(); + } catch (IOException e) { + throw new UncheckedIOException("Failed to obtain Keycloak admin token", e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException("Failed to obtain Keycloak admin token", e); + } + } + + private JsonNode adminGet(URI uri, String token) { + try { + HttpRequest request = + HttpRequest.newBuilder() + .uri(uri) + .header("Authorization", "Bearer " + token) + .GET() + .build(); + HttpResponse<InputStream> response = + httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream()); + return JsonMapper.shared().readTree(response.body()); Review Comment: adminGet() always attempts to parse the response body as JSON even when the server returns a non-200 status. This can mask the real failure (e.g., 401/404) behind a JSON parsing exception. ########## tools/testcontainers/keycloak/src/main/java/org/apache/polaris/test/keycloak/KeycloakContainer.java: ########## @@ -0,0 +1,308 @@ +/* + * 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.polaris.test.keycloak; + +import com.google.common.base.Preconditions; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URLEncoder; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import org.apache.polaris.containerspec.ContainerSpecHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import tools.jackson.databind.JsonNode; +import tools.jackson.databind.json.JsonMapper; + +public class KeycloakContainer extends GenericContainer<KeycloakContainer> + implements KeycloakAccess { + + private static final Logger LOGGER = LoggerFactory.getLogger(KeycloakContainer.class); + + private static final int KEYCLOAK_PORT = 8080; + private static final String REALM = "master"; + private static final String ADMIN_USERNAME = "admin"; + private static final String ADMIN_PASSWORD = "admin"; + + private URI issuerUrl; + private URI tokenEndpoint; + private HttpClient httpClient; + + @SuppressWarnings("resource") + public KeycloakContainer() { + super( + ContainerSpecHelper.containerSpecHelper("keycloak", KeycloakContainer.class) + .dockerImageName(null) + .asCanonicalNameString()); + withExposedPorts(KEYCLOAK_PORT); + withEnv("KEYCLOAK_ADMIN", ADMIN_USERNAME); + withEnv("KEYCLOAK_ADMIN_PASSWORD", ADMIN_PASSWORD); + withEnv("KC_LOG_LEVEL", getRootLoggerLevel() + ",org.keycloak:" + getKeycloakLoggerLevel()); + withCommand("start-dev"); + waitingFor( + Wait.forHttp("/realms/" + REALM) + .forStatusCode(200) + .withStartupTimeout(Duration.ofMinutes(2))); + withLogConsumer(new Slf4jLogConsumer(LOGGER)); + } + + @Override + public void start() { + super.start(); + httpClient = HttpClient.newHttpClient(); + String baseUrl = "http://"; + getHost() + ":" + getMappedPort(KEYCLOAK_PORT); + issuerUrl = URI.create(baseUrl + "/realms/" + REALM + "/"); + tokenEndpoint = issuerUrl.resolve("protocol/openid-connect/token"); + } + + @Override + public void stop() { + super.stop(); + httpClient = null; + } + + @Override + public URI getIssuerUrl() { + return issuerUrl; + } + + @Override + public URI getTokenEndpoint() { + return tokenEndpoint; + } + + /* + * The methods below were taken from org.keycloak:keycloak-admin-client. We don't depend on that + * artifact directly to avoid bringing in RESTEasy Classic transitively. + */ + + @Override + public void createRole(String roleName) { + String token = getAdminToken(); + int status = adminPost(realmAdminUrl("/roles"), "{\"name\":\"" + roleName + "\"}", token); + Preconditions.checkState( + status == 201, "Failed to create role '%s', status: %s", roleName, status); + } + + @Override + public void createUser(String name, String password) { + String token = getAdminToken(); + String body = + """ + { + "enabled": true, + "username": "%s", + "firstName": "%s", + "lastName": "%s", + "email": "%[email protected]", + "emailVerified": true, + "requiredActions": [], + "credentials": [ + { + "type": "password", + "value": "%s", + "temporary": false + } + ] + }""" + .formatted(name, name, name, name, password); + int status = adminPost(realmAdminUrl("/users"), body, token); + Preconditions.checkState(status == 201, "Failed to create user '%s', status: %s", name, status); + } + + @Override + public void assignRoleToUser(String role, String user) { + String token = getAdminToken(); + String userId = findUserId(user, token); + JsonNode roleRep = adminGet(realmAdminUrl("/roles/" + encode(role)), token); + String body = "[" + JsonMapper.shared().writeValueAsString(roleRep) + "]"; + int status = adminPost(realmAdminUrl("/users/" + userId + "/role-mappings/realm"), body, token); + Preconditions.checkState( + status == 204, "Failed to assign role '%s' to user '%s', status: %s", role, user, status); + } Review Comment: KeycloakAccess documents assignRoleToUser should take an unprefixed role name, but this implementation looks up the role using the provided name directly. To keep the interface contract, apply the "PRINCIPAL_ROLE:" prefix inside the container when calling the admin API. -- 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]
