smolnar82 commented on code in PR #1361: URL: https://github.com/apache/knox/pull/1361#discussion_r3913454025
########## gateway-server/src/main/java/org/apache/knox/gateway/services/factory/DelegationPolicyServiceFactory.java: ########## @@ -0,0 +1,84 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.factory; + +import org.apache.knox.gateway.GatewayMessages; +import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.i18n.messages.MessagesFactory; +import org.apache.knox.gateway.services.GatewayServices; +import org.apache.knox.gateway.services.Service; +import org.apache.knox.gateway.services.ServiceLifecycleException; +import org.apache.knox.gateway.services.ServiceType; +import org.apache.knox.gateway.services.knoxidf.delegation.EmptyDelegationPolicyService; +import org.apache.knox.gateway.services.knoxidf.delegation.JdbcDelegationPolicyService; + +import java.util.Collection; +import java.util.List; +import java.util.Map; + +public class DelegationPolicyServiceFactory extends AbstractServiceFactory { + + private static final GatewayMessages LOG = MessagesFactory.get(GatewayMessages.class); + private static final String DEFAULT_IMPLEMENTATION = EmptyDelegationPolicyService.class.getName(); + + @Override + protected Service createService(GatewayServices gatewayServices, ServiceType serviceType, + GatewayConfig gatewayConfig, Map<String, String> options, String implementation) + throws ServiceLifecycleException { + + String implementationToUse = implementation; + if (isEmptyDefaultImplementation(implementationToUse) && isKnoxIdfEnabledInAnyTopology(gatewayServices, gatewayConfig)) { + implementationToUse = JdbcDelegationPolicyService.class.getName(); Review Comment: I'd follow what we already have in `TrustedOidcIssuerServiceFactory` and `FederatedIdentityServiceFactory`: use JdbcDelegationPolicyService only when we have an external DB configured. Otherwise, use DerbyDBDelegationPolicyService (needs to be added here) that uses our embedded DerbyDB and is useful for testing locally. ########## gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java: ########## @@ -0,0 +1,429 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.knoxidf.delegation; + +import org.apache.commons.io.IOUtils; +import org.apache.knox.gateway.database.DatabaseType; +import org.apache.knox.gateway.database.JDBCUtils; +import org.apache.knox.gateway.database.KnoxDatabase; + +import javax.sql.DataSource; +import java.io.InputStream; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; + +import static java.nio.charset.StandardCharsets.UTF_8; + +/** + * JDBC helper for the five DELEGATION_POLICIES tables. + * All SQL uses {@link PreparedStatement} with {@code ?} parameters only. + * Each public method manages its own {@link Connection} and, for multi-table writes, + * its own transaction boundaries (setAutoCommit / commit / rollback). + */ +class DelegationPolicyDatabase extends KnoxDatabase { + + static final String CORE_TABLE = "DELEGATION_POLICIES"; + + private static final String INSERT_REGISTRATION_SQL = + "INSERT INTO " + CORE_TABLE + + " (registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange) " + + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"; + + private static final String UPDATE_CORE_SQL = + "UPDATE " + CORE_TABLE + " SET " + + "actor_authority = ?, actor_id = ?, name = ?, status = ?, token_ttl_sec = ?, " + + "description = ?, created_by = ?, created_at = ?, updated_at = ?, " + + "allow_headless_exchange = ? " + + "WHERE registration_id = ?"; + + private static final String DELETE_REGISTRATION_SQL = + "DELETE FROM " + CORE_TABLE + " WHERE registration_id = ?"; + + private static final String SELECT_BY_ID_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE + " WHERE registration_id = ?"; + + private static final String SELECT_BY_ACTOR_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE + " WHERE actor_authority = ? AND actor_id = ?"; + + private static final String SELECT_ALL_BASE_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE; + + // Built at construction time with limit+1 baked in as an integer literal (Derby does not + // support ? parameters in FETCH FIRST n ROWS ONLY). Fetching one extra row lets selectAll() + // detect truncation without a second COUNT query. + private final int listMaxTotal; + private final int listMaxPerAuthority; + private final String selectAllSql; + private final String selectAllFilteredSql; + + private static final String INSERT_USER_SQL = + "INSERT INTO DELEGATION_POLICY_USERS (registration_id, username) VALUES (?, ?)"; + + private static final String INSERT_GROUP_SQL = + "INSERT INTO DELEGATION_POLICY_GROUPS (registration_id, group_name) VALUES (?, ?)"; + + private static final String INSERT_RESOURCE_SQL = + "INSERT INTO DELEGATION_POLICY_RESOURCES (registration_id, resource_uri) VALUES (?, ?)"; + + private static final String INSERT_SCOPE_SQL = + "INSERT INTO DELEGATION_POLICY_RESOURCE_SCOPES (registration_id, resource_uri, scope) VALUES (?, ?, ?)"; + + private static final String SELECT_USERS_SQL = + "SELECT username FROM DELEGATION_POLICY_USERS WHERE registration_id = ?"; + + private static final String SELECT_GROUPS_SQL = + "SELECT group_name FROM DELEGATION_POLICY_GROUPS WHERE registration_id = ?"; + + private static final String SELECT_RESOURCES_SQL = + "SELECT resource_uri FROM DELEGATION_POLICY_RESOURCES WHERE registration_id = ?"; + + private static final String SELECT_SCOPES_SQL = + "SELECT scope FROM DELEGATION_POLICY_RESOURCE_SCOPES WHERE registration_id = ? AND resource_uri = ?"; + + private static final String DELETE_USERS_SQL = + "DELETE FROM DELEGATION_POLICY_USERS WHERE registration_id = ?"; + + private static final String DELETE_GROUPS_SQL = + "DELETE FROM DELEGATION_POLICY_GROUPS WHERE registration_id = ?"; + + private static final String DELETE_RESOURCES_SQL = + "DELETE FROM DELEGATION_POLICY_RESOURCES WHERE registration_id = ?"; + + DelegationPolicyDatabase(DataSource dataSource, String dbType, int listMaxTotal, int listMaxPerAuthority) throws Exception { + super(dataSource); + this.listMaxTotal = listMaxTotal; + this.listMaxPerAuthority = listMaxPerAuthority; + this.selectAllSql = SELECT_ALL_BASE_SQL + " FETCH FIRST " + (listMaxTotal + 1) + " ROWS ONLY"; + this.selectAllFilteredSql = SELECT_ALL_BASE_SQL + " WHERE actor_authority = ? FETCH FIRST " + (listMaxPerAuthority + 1) + " ROWS ONLY"; + final DatabaseType databaseType = DatabaseType.fromString(dbType); + createDelegationTablesIfNotExists(databaseType.delegationPolicyTablesSql()); Review Comment: Why not reuse KnoxDatabase#createTableIfNotExists here? Ohh, because these .sql files bundle five `CREATE TABLE` statements and `JDBCUtils.createTableFromSQL` runs the file as a single `Statement.execute()`, which Derby rejects. Rather than a private splitter here (also duplicated in `DelegationPolicySchemaTest.runScript`), could we push this into the shared layer? Two small additions: - a JDBCUtils.createTableFromSQL(String sql, DataSource) overload that executes one already-loaded statement, and - a KnoxDatabase.createTablesFromSQL(...) that splits the script and calls it per statement. Another option is to split that "all in one" SQL file into several create table SQL files, which fits into what we have now and adds less complexity (not to mentioned, it's well tested). I can see one more drawback with your approach: you only check if the `DELEGATION_POLICIES` has been created already. What happens, if that condition is true, but the previous attempt of creating ALL delegation policy tables failed in the middle. The next time, this check will be true, and we find ourselves in a place where some of the tables are missing. All of the above points the same way for me: splitting the all-in-one script into per-table files (as we already do for the other KnoxToken/KnoxIDF tables) removes the fragile parser, the test duplication, and the partial-creation guard weakness in one move, with no new shared machinery. That'd be my preference. -- 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]
