You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "hsheinblatt (via GitHub)" <gi...@apache.org> on 2026/08/28 03:57:53 UTC
[PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
hsheinblatt opened a new pull request, #1361:
URL: https://github.com/apache/knox/pull/1361
KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation
## What changes were proposed in this pull request?
This pull request adds the persistence layer for RFC 8693 delegation policies in Knox IDF.
Delegation policies control which actors (external token issuers and service accounts) are
authorized to perform cross-subject token exchange on behalf of specific users or groups
and for specific resources and scopes.
A five-table schema is added to store policies: a core DELEGATION_REGISTRY table keyed on
(actor_authority, actor_id) with an allow_headless_exchange flag and an optional per-policy
token TTL; four child tables for per-user, per-group, per-resource, and per-resource-scope
entries. Scopes are defined per resource rather than as a flat list, so a policy can allow
different scopes for different target resources. A resource with no scope entries means all
scopes are allowed for that resource.
The DelegationPolicyService gateway service interface is added to gateway-spi, where it is
visible to the filter layer (gateway-provider-security-jwt) that will enforce policies during
token exchange. The interface includes CRUD operations for policy lifecycle management and an
evaluate() method that accepts a PolicyCheckRequest and returns a PolicyDecision.
PolicyCheckRequest and PolicyDecision are also added to gateway-spi as the inputs and outputs
of evaluate(). DelegationPolicy, the POJO representing a stored policy, is also in gateway-spi
since it is returned by the CRUD methods on the same interface.
JdbcDelegationPolicyService, EmptyDelegationPolicyService, DelegationPolicyServiceFactory,
and the DDL scripts for standard, Derby, and Oracle dialects are added to gateway-server,
following the same structure used for JdbcTrustedOidcIssuerService. The factory activates
the JDBC implementation when a KNOXIDF or KNOXIDF_ADMIN topology is deployed, and falls back
to the empty stub otherwise.
The evaluate() implementation checks policies by field: user, groups, headless exchange
flag, subject user list, resource, and per-resource scope. A resource with no defined scopes is
treated as allowing any scope. canActFor.groups support is not yet implemented; if a policy has
a non-empty groups list and the subject is not in the users list, evaluate() returns a server
error rather than silently skipping the group check.
## How was this patch tested?
Schema tests verify that all three dialect DDL scripts create the five tables cleanly on a
Derby in-memory database and that NOT NULL, DEFAULT, UNIQUE, and foreign key constraints are
enforced. Default values (status='active', allow_headless_exchange=false, token_ttl_sec=null)
are verified by inserting a row without those fields. Constraint violations for missing required
fields and duplicate (actor_authority, actor_id) are confirmed.
CRUD lifecycle tests cover register, get, update (full overwrite), and delete. Update is verified
to replace child rows completely: registering with two users and updating with three different
users results in exactly three users, not five. Round-trip tests verify that resourcePolicy
entries, including resources with empty scope sets (all-scopes-allowed), are preserved across
register and get, with boundary cases for zero resources, one resource, and multiple resources,
and for resources with no scopes, one scope, and multiple scopes.
evaluate() tests cover the authorized path (actor registered, subject in users list, resource in
policy, scope in resource scopes), the all-scopes-allowed path (resource present but no scope
rows), and all denial paths: actor not registered, subject not in users list (with empty groups
list), both users and groups lists empty (deny - no open delegation), resource not in policy,
scope not in resource scopes, and headless exchange denied by policy flag. The not-yet-implemented
server error path is verified when a non-empty groups list is present and the user check fails.
If no TTL is specified, the default is used, and if specified, the policy value is used.
Factory tests confirm that the JDBC implementation is returned when KNOXIDF or KNOXIDF_ADMIN is
deployed and that the empty stub is returned otherwise.
## Integration Tests
No integration tests are added in this pull request. They will be added once the full delegation
enforcement flow is wired in a later task.
### Opt-in test suites (PR labels)
N/A
## UI changes
N/A
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884502564
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicy.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
+ * <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 java.time.Instant;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Immutable representation of a stored delegation policy record.
+ */
+public class DelegationPolicy {
+
+ private final String registrationId;
+ private final String actorAuthority;
+ private final String actorId;
+ private final String name;
+ private final String status;
+ private final Integer tokenTtlSec;
+ private final String description;
+ private final String createdBy;
+ private final Instant createdAt;
+ private final Instant updatedAt;
+ private final boolean allowHeadlessExchange;
+ private final Set<String> canActForUsers;
+ private final Set<String> canActForGroups;
+ private final Map<String, Set<String>> resourcePolicy;
+
+ public DelegationPolicy(String registrationId, String actorAuthority, String actorId,
+ String name, String status, Integer tokenTtlSec, String description, String createdBy,
+ Instant createdAt, Instant updatedAt, boolean allowHeadlessExchange,
+ Set<String> canActForUsers, Set<String> canActForGroups,
+ Map<String, Set<String>> resourcePolicy) {
+ this.registrationId = registrationId;
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.name = name;
+ this.status = status;
+ this.tokenTtlSec = tokenTtlSec;
+ this.description = description;
+ this.createdBy = createdBy;
+ this.createdAt = createdAt;
+ this.updatedAt = updatedAt;
+ this.allowHeadlessExchange = allowHeadlessExchange;
+ this.canActForUsers = Collections.unmodifiableSet(new HashSet<>(canActForUsers));
+ this.canActForGroups = Collections.unmodifiableSet(new HashSet<>(canActForGroups));
+ Map<String, Set<String>> copy = new HashMap<>();
+ for (Map.Entry<String, Set<String>> entry : resourcePolicy.entrySet()) {
+ copy.put(entry.getKey(), Collections.unmodifiableSet(new HashSet<>(entry.getValue())));
+ }
+ this.resourcePolicy = Collections.unmodifiableMap(copy);
Review Comment:
Seems fair enough.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 merged PR #1361:
URL: https://github.com/apache/knox/pull/1361
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884468550
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicySchemaTest.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.AbstractDataSourceFactory;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.UUID;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Validates that the DELEGATION_POLICIES DDL scripts parse and execute correctly
+ * against in-memory databases, and that schema constraints are enforced.
+ */
+public class DelegationPolicySchemaTest {
+
+ private static final String DERBY_DB = "delegationpolicies";
+ private static final String DERBY_URL = "jdbc:derby:memory:" + DERBY_DB + ";create=true";
+ private static final String DERBY_SHUTDOWN_URL = "jdbc:derby:memory:" + DERBY_DB + ";shutdown=true";
+ private static final String HSQL_URL = "jdbc:hsqldb:mem:delegationschema;ifexists=false";
+ private static final String HSQL_USER = "SA";
+ private static final String HSQL_PASSWORD = "";
+
+ private static Connection derbyConn;
+ private static Connection hsqlConn;
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ derbyConn = DriverManager.getConnection(DERBY_URL);
+ hsqlConn = DriverManager.getConnection(HSQL_URL, HSQL_USER, HSQL_PASSWORD);
+ // Run Derby DDL once - no IF NOT EXISTS, so run once at class level
+ runScript(derbyConn, loadSql(AbstractDataSourceFactory.DERBY_KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ // Run standard DDL on HSQLDB
+ runScript(hsqlConn, loadSql(AbstractDataSourceFactory.KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ }
Review Comment:
This is understandable given the lack of oracle backend to test with. @hsheinblatt what do you think about the suggestion here?
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3901313273
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicy.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
+ * <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 java.time.Instant;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Immutable representation of a stored delegation policy record.
+ */
+public class DelegationPolicy {
+
+ private final String registrationId;
+ private final String actorAuthority;
+ private final String actorId;
+ private final String name;
+ private final String status;
+ private final Integer tokenTtlSec;
+ private final String description;
+ private final String createdBy;
+ private final Instant createdAt;
+ private final Instant updatedAt;
+ private final boolean allowHeadlessExchange;
+ private final Set<String> canActForUsers;
+ private final Set<String> canActForGroups;
+ private final Map<String, Set<String>> resourcePolicy;
+
+ public DelegationPolicy(String registrationId, String actorAuthority, String actorId,
+ String name, String status, Integer tokenTtlSec, String description, String createdBy,
+ Instant createdAt, Instant updatedAt, boolean allowHeadlessExchange,
+ Set<String> canActForUsers, Set<String> canActForGroups,
+ Map<String, Set<String>> resourcePolicy) {
+ this.registrationId = registrationId;
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.name = name;
+ this.status = status;
+ this.tokenTtlSec = tokenTtlSec;
+ this.description = description;
+ this.createdBy = createdBy;
+ this.createdAt = createdAt;
+ this.updatedAt = updatedAt;
+ this.allowHeadlessExchange = allowHeadlessExchange;
+ this.canActForUsers = Collections.unmodifiableSet(new HashSet<>(canActForUsers));
+ this.canActForGroups = Collections.unmodifiableSet(new HashSet<>(canActForGroups));
+ Map<String, Set<String>> copy = new HashMap<>();
+ for (Map.Entry<String, Set<String>> entry : resourcePolicy.entrySet()) {
+ copy.put(entry.getKey(), Collections.unmodifiableSet(new HashSet<>(entry.getValue())));
+ }
+ this.resourcePolicy = Collections.unmodifiableMap(copy);
Review Comment:
Added
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/PolicyCheckRequest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 java.util.Set;
+
+/**
+ * Immutable input to {@link DelegationPolicyService#evaluate(PolicyCheckRequest)}.
+ */
+public class PolicyCheckRequest {
+
+ private final String actorAuthority;
+ private final String actorId;
+ private final String subjectName;
+ private final String requestedResource;
+ private final Set<String> requestedScopes;
+ private final boolean headlessExchange;
+
+ public PolicyCheckRequest(String actorAuthority, String actorId, String subjectName,
+ String requestedResource, Set<String> requestedScopes, boolean headlessExchange) {
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.subjectName = subjectName;
+ this.requestedResource = requestedResource;
+ this.requestedScopes = requestedScopes;
+ this.headlessExchange = headlessExchange;
+ }
Review Comment:
Added
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1361:
URL: https://github.com/apache/knox/pull/1361#issuecomment-5521633893
Let me merge this PR and submit the 2 DB-related changes as a separate one.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884510424
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/factory/DelegationPolicyServiceFactoryTest.java:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.commons.io.FileUtils;
+import org.apache.knox.gateway.config.impl.GatewayConfigImpl;
+import org.apache.knox.gateway.database.DatabaseType;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.Service;
+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 org.apache.knox.gateway.services.knoxidf.delegation.PolicyCheckRequest;
+import org.apache.knox.gateway.services.knoxidf.delegation.PolicyDecision;
+import org.apache.knox.gateway.services.knoxidf.delegation.DelegationPolicyService;
+import org.apache.knox.gateway.services.security.AliasService;
+import org.apache.knox.gateway.services.topology.TopologyService;
+import org.apache.knox.gateway.topology.Topology;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.sql.DriverManager;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class DelegationPolicyServiceFactoryTest {
+
+ private final DelegationPolicyServiceFactory factory = new DelegationPolicyServiceFactory();
+ private final Map<String, String> options = new HashMap<>();
+ private File tempDir;
+ private Service createdService;
+
+ @BeforeClass
+ public static void setUpClass() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf;create=true").close();
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf_admin;create=true").close();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (createdService != null) {
+ createdService.stop();
+ }
+ if (tempDir != null) {
+ FileUtils.forceDelete(tempDir);
+ }
+ }
+
+ // ------------------------------------------------------------------
+ // Empty (no KNOXIDF) cases
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectEmptyWhenNoTopologies() throws Exception {
+ final GatewayServices gws = servicesWithTopology(/* no topologies */);
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldSelectEmptyWhenNoKnoxIdfRole() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXSSO"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldHonorExplicitEmptyImplEvenWhenKnoxIdfIsDeployed() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXIDF"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options,
+ EmptyDelegationPolicyService.class.getName());
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ // ------------------------------------------------------------------
+ // JDBC when KNOXIDF deployed
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectJdbcWhenKnoxIdfDeployed() throws Exception {
+ tempDir = org.apache.knox.test.TestUtils.createTempDir(getClass().getName());
+ final AliasService aliasService = EasyMock.createNiceMock(AliasService.class);
+ EasyMock.replay(aliasService);
+
+ final TopologyService topologyService = EasyMock.createNiceMock(TopologyService.class);
+ EasyMock.expect(topologyService.getTopologies()).andReturn(
+ Collections.singletonList(topologyWithRole("KNOXIDF"))).anyTimes();
+ EasyMock.replay(topologyService);
+
+ final GatewayServices gws = EasyMock.createNiceMock(GatewayServices.class);
+ EasyMock.expect(gws.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(topologyService).anyTimes();
+ EasyMock.expect(gws.getService(ServiceType.ALIAS_SERVICE)).andReturn(aliasService).anyTimes();
+ EasyMock.replay(gws);
+
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.expect(config.getDatabaseType()).andReturn(DatabaseType.DERBY.type()).anyTimes();
+ EasyMock.expect(config.getDatabaseName())
+ .andReturn("memory:" + getClass().getSimpleName() + "_knoxidf").anyTimes();
+ EasyMock.expect(config.getDelegationServiceTokenTtlSec()).andReturn(3600).anyTimes();
+ EasyMock.replay(config);
Review Comment:
Seems fair.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3914975975
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/JdbcDelegationPolicyService.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.database.DataSourceProvider;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.AliasService;
+
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * JDBC-backed implementation of {@link DelegationPolicyService}.
+ * Transaction management is handled by {@link DelegationPolicyDatabase}; this class
+ * is responsible for service lifecycle, exception translation, and evaluate() logic.
+ */
+public class JdbcDelegationPolicyService implements DelegationPolicyService {
+
+ private static final DelegationPolicyServiceMessages LOG =
+ MessagesFactory.get(DelegationPolicyServiceMessages.class);
+
+ private final AtomicBoolean initialized = new AtomicBoolean(false);
+ private final Lock initLock = new ReentrantLock(true);
+
+ private AliasService aliasService;
+ private DelegationPolicyDatabase database;
+ private int configuredKnoxTokenTtlSec;
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ if (!initialized.get()) {
+ initLock.lock();
+ try {
+ if (!initialized.get()) {
+ if (aliasService == null) {
+ throw new ServiceLifecycleException("The required AliasService reference has not been set.");
+ }
+ try {
+ this.configuredKnoxTokenTtlSec = config.getDelegationServiceTokenTtlSec();
+ this.database = new DelegationPolicyDatabase(
+ DataSourceProvider.getDataSource(config, aliasService),
+ config.getDatabaseType(),
+ config.getDelegationServiceListMaxTotal(),
+ config.getDelegationServiceListMaxPerAuthority());
+ initialized.set(true);
+ } catch (ServiceLifecycleException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new ServiceLifecycleException("Error initializing JdbcDelegationPolicyService: " + e, e);
+ }
+ }
+ } finally {
+ initLock.unlock();
+ }
+ }
+ }
+
+ @Override
+ public void start() throws ServiceLifecycleException {
+ }
+
+ @Override
+ public void stop() throws ServiceLifecycleException {
+ }
+
+ public void setAliasService(AliasService aliasService) {
+ this.aliasService = aliasService;
+ }
+
+ @Override
+ public DelegationPolicy register(DelegationPolicy policy) {
+ try {
+ final String id = database.insertPolicy(policy);
+ return database.selectById(id).orElseThrow(
+ () -> new RuntimeException("Failed to read back registered policy " + id));
+ } catch (Exception e) {
+ LOG.errorRegisteringPolicy(policy.getActorAuthority(), policy.getActorId(), e.getMessage(), e);
+ throw new RuntimeException(
+ "Error registering delegation policy for actor (" + policy.getActorAuthority() + ", " + policy.getActorId() + "): " + e, e);
+ }
+ }
+
+ @Override
+ public void update(String registrationId, DelegationPolicy policy) {
+ try {
+ database.updatePolicy(registrationId, policy);
+ } catch (Exception e) {
+ LOG.errorUpdatingPolicy(registrationId, e.getMessage(), e);
+ throw new RuntimeException("Error updating delegation policy " + registrationId + ": " + e, e);
+ }
+ }
+
+ @Override
+ public void delete(String registrationId) {
+ try {
+ database.deletePolicy(registrationId);
+ } catch (Exception e) {
+ LOG.errorDeletingPolicy(registrationId, e.getMessage(), e);
+ throw new RuntimeException("Error deleting delegation policy " + registrationId + ": " + e, e);
+ }
+ }
+
+ @Override
+ public Optional<DelegationPolicy> get(String registrationId) {
+ try {
+ return database.selectById(registrationId);
+ } catch (Exception e) {
+ LOG.errorReadingPolicy(registrationId, e.getMessage(), e);
+ throw new RuntimeException("Error reading delegation policy " + registrationId + ": " + e, e);
+ }
+ }
+
+ @Override
+ public Optional<DelegationPolicy> findByActor(String actorAuthority, String actorId) {
+ try {
+ return database.selectByActor(actorAuthority, actorId);
+ } catch (Exception e) {
+ LOG.errorListingPolicies(e.getMessage(), e);
+ throw new RuntimeException("Error looking up delegation policy for actor (" + actorAuthority + ", " + actorId + "): " + e, e);
+ }
+ }
+
+ @Override
+ public DelegationPolicyList list(String actorAuthorityFilter) {
+ try {
+ return database.selectAll(actorAuthorityFilter);
+ } catch (Exception e) {
+ LOG.errorListingPolicies(e.getMessage(), e);
+ throw new RuntimeException("Error listing delegation policies: " + e, e);
+ }
+ }
+
+ @Override
+ public PolicyDecision evaluate(PolicyCheckRequest request) {
+ // Step 1: look up registration
+ final Optional<DelegationPolicy> policyOpt = findByActor(request.getActorAuthority(), request.getActorId());
+ if (!policyOpt.isPresent()) {
+ return deny("actor_not_registered");
+ }
+ final DelegationPolicy policy = policyOpt.get();
+
+ // Step 2: headless exchange check
+ if (request.isHeadlessExchange() && !policy.isAllowHeadlessExchange()) {
+ return deny("headless_not_allowed");
+ }
+
+ // Step 3: user check (remember result; group check deferred to the end)
+ final boolean userCheckPassed =
+ policy.getCanActForUsers().contains(request.getSubjectName());
+
+ // Step 4: resource check
+ final Map<String, Set<String>> resourcePolicy = policy.getResourcePolicy();
+ if (!resourcePolicy.containsKey(request.getRequestedResource())) {
+ return deny("resource_not_allowed");
+ }
+
+ // Step 5: scope check (requested scopes are optional; when non-empty, all must be in the allowed set)
+ final Set<String> scopeSet = resourcePolicy.get(request.getRequestedResource());
+ if (!request.getRequestedScopes().isEmpty() && !scopeSet.isEmpty()
+ && !scopeSet.containsAll(request.getRequestedScopes())) {
+ return deny("scope_not_allowed");
+ }
+
+ // Step 6: effective TTL — use the policy value if set, otherwise fall back to the configured default
+ final int effectiveTtlSec = policy.getTokenTtlSec() != null
+ ? policy.getTokenTtlSec()
+ : configuredKnoxTokenTtlSec;
+
+ // Step 7: group check (LDAP lookup — slowest, deferred past cheap checks)
+ if (!userCheckPassed) {
+ if (!policy.getCanActForGroups().isEmpty()) {
+ throw new UnsupportedOperationException("canActFor.groups evaluation not yet implemented");
Review Comment:
Claude's comment on this:
```
evaluate() throws on a policy shape that register()/update() persists (there are even tests storing canActForGroups),
so this fires on a valid, stored policy — and per the design (KNOX-3426 §6/§8) groups are a first-class MVP path, not an impossible input.
Once wired to the exchange handler this surfaces as 500 instead of the required access_denied.
Either reject non-empty canActForGroups at registration (can't persist what you can't evaluate), or at least fail closed here — return deny("subject_not_allowed") — and defer the real LDAP check to a follow-up.
```
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3899446728
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicySchemaTest.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.AbstractDataSourceFactory;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.UUID;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Validates that the DELEGATION_POLICIES DDL scripts parse and execute correctly
+ * against in-memory databases, and that schema constraints are enforced.
+ */
+public class DelegationPolicySchemaTest {
+
+ private static final String DERBY_DB = "delegationpolicies";
+ private static final String DERBY_URL = "jdbc:derby:memory:" + DERBY_DB + ";create=true";
+ private static final String DERBY_SHUTDOWN_URL = "jdbc:derby:memory:" + DERBY_DB + ";shutdown=true";
+ private static final String HSQL_URL = "jdbc:hsqldb:mem:delegationschema;ifexists=false";
+ private static final String HSQL_USER = "SA";
+ private static final String HSQL_PASSWORD = "";
+
+ private static Connection derbyConn;
+ private static Connection hsqlConn;
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ derbyConn = DriverManager.getConnection(DERBY_URL);
+ hsqlConn = DriverManager.getConnection(HSQL_URL, HSQL_USER, HSQL_PASSWORD);
+ // Run Derby DDL once - no IF NOT EXISTS, so run once at class level
+ runScript(derbyConn, loadSql(AbstractDataSourceFactory.DERBY_KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ // Run standard DDL on HSQLDB
+ runScript(hsqlConn, loadSql(AbstractDataSourceFactory.KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ }
Review Comment:
This seems reasonable, added. There are no other similar tests for oracle. The unit tests don't show the sql exists, it's parsable, or it works. We'd need an integration test to validate oracle integration.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3901309564
##########
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;
Review Comment:
On the use of the factory pattern rather than the service loader: the piece of the service I could see as pluggable is the storage layer, but there's no precedent I could find in the code base for using a pluggable storage layer for the RDBMS. They all use a factory pattern and manage the SQL engine to use based on internal config. The LDAP service uses a pluggable backend, which makes sense as that's standard. Arguably knox could have done that for RDBMS backends as well, but that is not the established pattern.
The current implementation has decision logic, which also requires a factory, but as noted above, all those could be removed. But even if they were, I'd think we'd just remove the factory and directly create the jdbc service, still not needing a plugin.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on PR #1361:
URL: https://github.com/apache/knox/pull/1361#issuecomment-5489852186
I've updated the PR with fixes and added comments in response to questions.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884291830
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/PolicyCheckRequest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 java.util.Set;
+
+/**
+ * Immutable input to {@link DelegationPolicyService#evaluate(PolicyCheckRequest)}.
+ */
+public class PolicyCheckRequest {
+
+ private final String actorAuthority;
+ private final String actorId;
+ private final String subjectName;
+ private final String requestedResource;
+ private final Set<String> requestedScopes;
+ private final boolean headlessExchange;
+
+ public PolicyCheckRequest(String actorAuthority, String actorId, String subjectName,
+ String requestedResource, Set<String> requestedScopes, boolean headlessExchange) {
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.subjectName = subjectName;
+ this.requestedResource = requestedResource;
+ this.requestedScopes = requestedScopes;
+ this.headlessExchange = headlessExchange;
+ }
Review Comment:
PolicyCheckRequest is documented as immutable, but it stores the caller-provided requestedScopes Set reference directly. That allows external mutation after construction, which can change evaluate() outcomes (and can also NPE later if null is passed). Consider defensively copying and wrapping the scopes set (defaulting null to empty).
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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";
Review Comment:
selectAllSql/selectAllFilteredSql apply a hard row limit but don’t specify an ORDER BY. Without deterministic ordering, list() results (and truncation/hasMore behavior) can vary across executions and DB implementations, which is risky for admin APIs/UI. Consider adding an explicit ORDER BY (e.g., actor_authority, actor_id) before applying FETCH FIRST.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicy.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
+ * <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 java.time.Instant;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Immutable representation of a stored delegation policy record.
+ */
+public class DelegationPolicy {
+
+ private final String registrationId;
+ private final String actorAuthority;
+ private final String actorId;
+ private final String name;
+ private final String status;
+ private final Integer tokenTtlSec;
+ private final String description;
+ private final String createdBy;
+ private final Instant createdAt;
+ private final Instant updatedAt;
+ private final boolean allowHeadlessExchange;
+ private final Set<String> canActForUsers;
+ private final Set<String> canActForGroups;
+ private final Map<String, Set<String>> resourcePolicy;
+
+ public DelegationPolicy(String registrationId, String actorAuthority, String actorId,
+ String name, String status, Integer tokenTtlSec, String description, String createdBy,
+ Instant createdAt, Instant updatedAt, boolean allowHeadlessExchange,
+ Set<String> canActForUsers, Set<String> canActForGroups,
+ Map<String, Set<String>> resourcePolicy) {
+ this.registrationId = registrationId;
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.name = name;
+ this.status = status;
+ this.tokenTtlSec = tokenTtlSec;
+ this.description = description;
+ this.createdBy = createdBy;
+ this.createdAt = createdAt;
+ this.updatedAt = updatedAt;
+ this.allowHeadlessExchange = allowHeadlessExchange;
+ this.canActForUsers = Collections.unmodifiableSet(new HashSet<>(canActForUsers));
+ this.canActForGroups = Collections.unmodifiableSet(new HashSet<>(canActForGroups));
+ Map<String, Set<String>> copy = new HashMap<>();
+ for (Map.Entry<String, Set<String>> entry : resourcePolicy.entrySet()) {
+ copy.put(entry.getKey(), Collections.unmodifiableSet(new HashSet<>(entry.getValue())));
+ }
+ this.resourcePolicy = Collections.unmodifiableMap(copy);
Review Comment:
DelegationPolicy’s constructor assumes canActForUsers/canActForGroups/resourcePolicy and each resourcePolicy value are non-null (new HashSet<>(...) and entry.getValue()). Since this is a public SPI POJO, a null input will currently throw a NullPointerException during construction. Either document non-null requirements explicitly or make the constructor null-safe (e.g., treat null collections as empty).
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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());
+ }
+
+ /**
+ * Multi-statement DDL runner: checks if DELEGATION_POLICIES exists, then strips SQL line
+ * comments, splits on {@code ;}, and executes each non-empty statement individually.
+ * {@link JDBCUtils#createTableFromSQL} handles only single statements; delegation needs five.
+ * Comment stripping must happen before the split because the ASF license header contains a
+ * semicolon inside a {@code --} comment line, which would otherwise produce a spurious token.
+ */
+ private void createDelegationTablesIfNotExists(String sqlFileName) throws Exception {
+ if (!JDBCUtils.tableExists(CORE_TABLE, dataSource)) {
+ try (InputStream is = getClass().getClassLoader().getResourceAsStream(sqlFileName);
+ Connection connection = dataSource.getConnection()) {
+ final String script = IOUtils.toString(is, UTF_8);
+ final StringBuilder stripped = new StringBuilder();
Review Comment:
createDelegationTablesIfNotExists() reads the DDL resource stream without checking for null. If the SQL file isn’t on the classpath (packaging regression, typo, etc.), this will throw a NullPointerException from IOUtils.toString() rather than a clear error. Add an explicit null check and fail fast with a descriptive exception.
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicySchemaTest.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.AbstractDataSourceFactory;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.UUID;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Validates that the DELEGATION_POLICIES DDL scripts parse and execute correctly
+ * against in-memory databases, and that schema constraints are enforced.
+ */
+public class DelegationPolicySchemaTest {
+
+ private static final String DERBY_DB = "delegationpolicies";
+ private static final String DERBY_URL = "jdbc:derby:memory:" + DERBY_DB + ";create=true";
+ private static final String DERBY_SHUTDOWN_URL = "jdbc:derby:memory:" + DERBY_DB + ";shutdown=true";
+ private static final String HSQL_URL = "jdbc:hsqldb:mem:delegationschema;ifexists=false";
+ private static final String HSQL_USER = "SA";
+ private static final String HSQL_PASSWORD = "";
+
+ private static Connection derbyConn;
+ private static Connection hsqlConn;
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ derbyConn = DriverManager.getConnection(DERBY_URL);
+ hsqlConn = DriverManager.getConnection(HSQL_URL, HSQL_USER, HSQL_PASSWORD);
+ // Run Derby DDL once - no IF NOT EXISTS, so run once at class level
+ runScript(derbyConn, loadSql(AbstractDataSourceFactory.DERBY_KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ // Run standard DDL on HSQLDB
+ runScript(hsqlConn, loadSql(AbstractDataSourceFactory.KNOXIDF_DELEGATION_POLICY_TABLES_SQL));
+ }
Review Comment:
PR description says schema tests verify all three dialect DDL scripts, but this test only executes Derby + the standard script. Oracle DDL isn’t currently exercised/validated here, so either update the description or add at least a classpath presence check for the Oracle script (full execution would require an Oracle-backed test).
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/factory/DelegationPolicyServiceFactoryTest.java:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.commons.io.FileUtils;
+import org.apache.knox.gateway.config.impl.GatewayConfigImpl;
+import org.apache.knox.gateway.database.DatabaseType;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.Service;
+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 org.apache.knox.gateway.services.knoxidf.delegation.PolicyCheckRequest;
+import org.apache.knox.gateway.services.knoxidf.delegation.PolicyDecision;
+import org.apache.knox.gateway.services.knoxidf.delegation.DelegationPolicyService;
+import org.apache.knox.gateway.services.security.AliasService;
+import org.apache.knox.gateway.services.topology.TopologyService;
+import org.apache.knox.gateway.topology.Topology;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.sql.DriverManager;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class DelegationPolicyServiceFactoryTest {
+
+ private final DelegationPolicyServiceFactory factory = new DelegationPolicyServiceFactory();
+ private final Map<String, String> options = new HashMap<>();
+ private File tempDir;
+ private Service createdService;
+
+ @BeforeClass
+ public static void setUpClass() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf;create=true").close();
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf_admin;create=true").close();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (createdService != null) {
+ createdService.stop();
+ }
+ if (tempDir != null) {
+ FileUtils.forceDelete(tempDir);
+ }
+ }
+
+ // ------------------------------------------------------------------
+ // Empty (no KNOXIDF) cases
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectEmptyWhenNoTopologies() throws Exception {
+ final GatewayServices gws = servicesWithTopology(/* no topologies */);
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldSelectEmptyWhenNoKnoxIdfRole() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXSSO"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldHonorExplicitEmptyImplEvenWhenKnoxIdfIsDeployed() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXIDF"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options,
+ EmptyDelegationPolicyService.class.getName());
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ // ------------------------------------------------------------------
+ // JDBC when KNOXIDF deployed
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectJdbcWhenKnoxIdfDeployed() throws Exception {
+ tempDir = org.apache.knox.test.TestUtils.createTempDir(getClass().getName());
+ final AliasService aliasService = EasyMock.createNiceMock(AliasService.class);
+ EasyMock.replay(aliasService);
+
+ final TopologyService topologyService = EasyMock.createNiceMock(TopologyService.class);
+ EasyMock.expect(topologyService.getTopologies()).andReturn(
+ Collections.singletonList(topologyWithRole("KNOXIDF"))).anyTimes();
+ EasyMock.replay(topologyService);
+
+ final GatewayServices gws = EasyMock.createNiceMock(GatewayServices.class);
+ EasyMock.expect(gws.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(topologyService).anyTimes();
+ EasyMock.expect(gws.getService(ServiceType.ALIAS_SERVICE)).andReturn(aliasService).anyTimes();
+ EasyMock.replay(gws);
+
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.expect(config.getDatabaseType()).andReturn(DatabaseType.DERBY.type()).anyTimes();
+ EasyMock.expect(config.getDatabaseName())
+ .andReturn("memory:" + getClass().getSimpleName() + "_knoxidf").anyTimes();
+ EasyMock.expect(config.getDelegationServiceTokenTtlSec()).andReturn(3600).anyTimes();
+ EasyMock.replay(config);
Review Comment:
These factory tests mock GatewayConfigImpl but don’t set expectations for getDelegationServiceListMaxTotal()/getDelegationServiceListMaxPerAuthority(). JdbcDelegationPolicyService init uses those values, so the mock will return 0 by default and the service will be initialized with unintended list limits. Setting explicit expectations to the defaults makes the test setup realistic and prevents accidental behavior changes if list() is exercised later.
This issue also appears on line 154 of the same file.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3898815619
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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";
Review Comment:
My opinion is that we should not add an order by. That would have performance implications for every query in normal operation. The case in question is already an error case: we've hit the scalability limit of the database and it's no longer working correctly. Clients have a flag so they can log the condition, but the administrator has to fix it. More resources can be assigned and the limit raised, rows could be cleaned up to free up capacity, but the system isn't working anymore. It seemed best to truncate results with a flag rather than throw an exception, but the client has no graceful recovery option here. There's no way to retrieve the missing rows even if there were an order by, and the data returned is incorrect because it is incomplete either way. The design could be changed to use paging instead, but would similarly still have capacity limits that could still be reached and cause error conditions.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3901311560
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/factory/DelegationPolicyServiceFactoryTest.java:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.commons.io.FileUtils;
+import org.apache.knox.gateway.config.impl.GatewayConfigImpl;
+import org.apache.knox.gateway.database.DatabaseType;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.Service;
+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 org.apache.knox.gateway.services.knoxidf.delegation.PolicyCheckRequest;
+import org.apache.knox.gateway.services.knoxidf.delegation.PolicyDecision;
+import org.apache.knox.gateway.services.knoxidf.delegation.DelegationPolicyService;
+import org.apache.knox.gateway.services.security.AliasService;
+import org.apache.knox.gateway.services.topology.TopologyService;
+import org.apache.knox.gateway.topology.Topology;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.sql.DriverManager;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class DelegationPolicyServiceFactoryTest {
+
+ private final DelegationPolicyServiceFactory factory = new DelegationPolicyServiceFactory();
+ private final Map<String, String> options = new HashMap<>();
+ private File tempDir;
+ private Service createdService;
+
+ @BeforeClass
+ public static void setUpClass() throws Exception {
+ java.util.Locale.setDefault(java.util.Locale.US);
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf;create=true").close();
+ DriverManager.getConnection("jdbc:derby:memory:DelegationPolicyServiceFactoryTest_knoxidf_admin;create=true").close();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (createdService != null) {
+ createdService.stop();
+ }
+ if (tempDir != null) {
+ FileUtils.forceDelete(tempDir);
+ }
+ }
+
+ // ------------------------------------------------------------------
+ // Empty (no KNOXIDF) cases
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectEmptyWhenNoTopologies() throws Exception {
+ final GatewayServices gws = servicesWithTopology(/* no topologies */);
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldSelectEmptyWhenNoKnoxIdfRole() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXSSO"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options, "");
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ @Test
+ public void shouldHonorExplicitEmptyImplEvenWhenKnoxIdfIsDeployed() throws Exception {
+ final GatewayServices gws = servicesWithTopology(topologyWithRole("KNOXIDF"));
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.replay(config);
+ createdService = factory.create(gws, ServiceType.DELEGATION_POLICY_SERVICE, config, options,
+ EmptyDelegationPolicyService.class.getName());
+ assertTrue(createdService instanceof EmptyDelegationPolicyService);
+ }
+
+ // ------------------------------------------------------------------
+ // JDBC when KNOXIDF deployed
+ // ------------------------------------------------------------------
+
+ @Test
+ public void shouldSelectJdbcWhenKnoxIdfDeployed() throws Exception {
+ tempDir = org.apache.knox.test.TestUtils.createTempDir(getClass().getName());
+ final AliasService aliasService = EasyMock.createNiceMock(AliasService.class);
+ EasyMock.replay(aliasService);
+
+ final TopologyService topologyService = EasyMock.createNiceMock(TopologyService.class);
+ EasyMock.expect(topologyService.getTopologies()).andReturn(
+ Collections.singletonList(topologyWithRole("KNOXIDF"))).anyTimes();
+ EasyMock.replay(topologyService);
+
+ final GatewayServices gws = EasyMock.createNiceMock(GatewayServices.class);
+ EasyMock.expect(gws.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(topologyService).anyTimes();
+ EasyMock.expect(gws.getService(ServiceType.ALIAS_SERVICE)).andReturn(aliasService).anyTimes();
+ EasyMock.replay(gws);
+
+ final GatewayConfigImpl config = EasyMock.createNiceMock(GatewayConfigImpl.class);
+ EasyMock.expect(config.getDatabaseType()).andReturn(DatabaseType.DERBY.type()).anyTimes();
+ EasyMock.expect(config.getDatabaseName())
+ .andReturn("memory:" + getClass().getSimpleName() + "_knoxidf").anyTimes();
+ EasyMock.expect(config.getDelegationServiceTokenTtlSec()).andReturn(3600).anyTimes();
+ EasyMock.replay(config);
Review Comment:
added
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3899099573
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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());
+ }
+
+ /**
+ * Multi-statement DDL runner: checks if DELEGATION_POLICIES exists, then strips SQL line
+ * comments, splits on {@code ;}, and executes each non-empty statement individually.
+ * {@link JDBCUtils#createTableFromSQL} handles only single statements; delegation needs five.
+ * Comment stripping must happen before the split because the ASF license header contains a
+ * semicolon inside a {@code --} comment line, which would otherwise produce a spurious token.
+ */
+ private void createDelegationTablesIfNotExists(String sqlFileName) throws Exception {
+ if (!JDBCUtils.tableExists(CORE_TABLE, dataSource)) {
+ try (InputStream is = getClass().getClassLoader().getResourceAsStream(sqlFileName);
+ Connection connection = dataSource.getConnection()) {
+ final String script = IOUtils.toString(is, UTF_8);
+ final StringBuilder stripped = new StringBuilder();
Review Comment:
This one is really hard to test, and a bit unlikely to occur. This is a private method a few layers down in init. In order to get to this error path, we'd need a databasetype that succeeded lookup but was missing a valid sql resource. I can add this protection, but then I'll skip the unit test for it. Ideally one of the layers leading to it should fail at build time not runtime if there is a problem.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3900519912
##########
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;
Review Comment:
For the empty implementation, I was following the pattern from FederatedIdentityService, so Sandor might have a better understanding of the intent. My understanding is that the empty service is a soft config failure.
On a config failure, you could say, that's not allowed, fail knox startup, everything stops with a clear config exception. Other services do that, like DefaultTokenAuthorityService. The pattern in the FederatedIdentityService, also used in TrustedOidcIssuer, is that if there is some failure, it doesn't block startup, instead it loads the empty service and that will cause runtime errors if those paths are hit.
One example failure case is you've configured the delegation service enabled flag to allow policy-driven delegation token exchanges, but you haven't configured the delegation service. Arguably this should just fail startup -- you want to use the delegation service but it won't work. But possibly there are other topologies that you'd want to run and not block for this.
Depending on your deployment model, failing startup is cleaner -- for example in k8s, you'd ideally get a crash loop backoff and not deploy the new version until you fixed the config. But in other deployment models, it may cause a real problem. With this implementation, knox starts up, but then if you try a delegation token exchange, they'll all fail with a configuration error.
It also fails over to an empty config on other init failures, so if the config is incorrect or there's a runtime problem initializing, knox will still start up but with the empty service. We could make a distinction on what causes a full failure and what fails over to empty based on the type of configuration problem or runtime issue.
The FederatedIdentity service is different: we may not need it at init time. Since the delegation service is to have an explicit enablement flag, we can fail startup deterministically if we're comfortable failing startup.
For now I suggest we keep this failure pattern, and follow up to further justify this pattern or rethink it across the three services.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884450052
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/PolicyCheckRequest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 java.util.Set;
+
+/**
+ * Immutable input to {@link DelegationPolicyService#evaluate(PolicyCheckRequest)}.
+ */
+public class PolicyCheckRequest {
+
+ private final String actorAuthority;
+ private final String actorId;
+ private final String subjectName;
+ private final String requestedResource;
+ private final Set<String> requestedScopes;
+ private final boolean headlessExchange;
+
+ public PolicyCheckRequest(String actorAuthority, String actorId, String subjectName,
+ String requestedResource, Set<String> requestedScopes, boolean headlessExchange) {
+ this.actorAuthority = actorAuthority;
+ this.actorId = actorId;
+ this.subjectName = subjectName;
+ this.requestedResource = requestedResource;
+ this.requestedScopes = requestedScopes;
+ this.headlessExchange = headlessExchange;
+ }
Review Comment:
Seems fair.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884476306
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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";
Review Comment:
Interesting suggestion. @hsheinblatt - thoughts? Seems like any test scenarios would benefit from that being more deterministic.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884471319
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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());
+ }
+
+ /**
+ * Multi-statement DDL runner: checks if DELEGATION_POLICIES exists, then strips SQL line
+ * comments, splits on {@code ;}, and executes each non-empty statement individually.
+ * {@link JDBCUtils#createTableFromSQL} handles only single statements; delegation needs five.
+ * Comment stripping must happen before the split because the ASF license header contains a
+ * semicolon inside a {@code --} comment line, which would otherwise produce a spurious token.
+ */
+ private void createDelegationTablesIfNotExists(String sqlFileName) throws Exception {
+ if (!JDBCUtils.tableExists(CORE_TABLE, dataSource)) {
+ try (InputStream is = getClass().getClassLoader().getResourceAsStream(sqlFileName);
+ Connection connection = dataSource.getConnection()) {
+ final String script = IOUtils.toString(is, UTF_8);
+ final StringBuilder stripped = new StringBuilder();
Review Comment:
Fair suggestion.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884476306
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java:
##########
@@ -0,0 +1,426 @@
+/*
+ * 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";
Review Comment:
Interesting suggestion. @hsheinblatt - thoughts?
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "lmccay (via GitHub)" <gi...@apache.org>.
lmccay commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3884569849
##########
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;
Review Comment:
I wonder whether we need an empty implementation. We should be able to configure whether the delegation service should be added or not. I see the check for isKnoxIdfEnabledInAnyTopology and note the intent there. I'm curious of the timing of that and how it is actually known at the time of gateway services being initiatlized.
##########
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;
Review Comment:
In general, I don't like the hard coding of classnames rather than usingthe service loader mechanism but I see that this within the factory implementation where the implementation is being provided. I need to dig into that more.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1361:
URL: https://github.com/apache/knox/pull/1361#issuecomment-5476229858
## Test Results
3 files 3 suites 8s ⏱️
52 tests 52 ✅ 0 💤 0 ❌
63 runs 63 ✅ 0 💤 0 ❌
Results for commit fd0913f8.
[test-results]:data:application/gzip;base64,H4sIAEhFlWoC/12MSQ6DMBAEv4J8zgFnMJh8JvLCSKMAjrycovw9BgEx3LqqpfowpHEI7FHBrWIhUTzAJq8iuTmjzJiPuFzivsMzJGMu5kXvbOpDoKLxJAbvnd+MT/NSbGHbe7AQ/97KRW7lsmbcNFHMwNDWPQeUnQDoGt0IqxWqHlBrazhwpQW2Vgr2/QHXe+tw/wAAAA==
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3426 - Add DelegationPolicyService schema, interface, and JDBC implementation [knox]
Posted by "hsheinblatt (via GitHub)" <gi...@apache.org>.
hsheinblatt commented on code in PR #1361:
URL: https://github.com/apache/knox/pull/1361#discussion_r3900979838
##########
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;
Review Comment:
For the conditional loading of the delegation policy service based on the configuration for either knoxidf or knoxadmin using isKnoxIdfEnabledInAnyTopology(), this has some issues. First, the primary check here is through the topology service, which is loaded after the delegation service so in this case will always fail through to the isKnoxIdfEnabledOnDisk() fallback. But even if we reordered the loading so it was initialized, it doesn't provide much value: checking the topology xml files should be correct anyway. So we could just remove that initial check and fix it to use the fallback reading of the xml files.
It's possible to dynamically load topologies from new files later. If at init time, no topologies used knoxidf or knoxadmin, so the delegation service didn't initialize, but then later a new topology was loaded that did include knoxidf or knoxadmin, the topology load wouldn't reload the gateway services, and so the delegation policy service wouldn't be reloaded for them, and they'd fail if they needed it. There are multiple paths here that cause dynamic loading. The simplified descriptor feature seems unlikely to apply to knoxidf. But API calls or direct file manipulation are possibilities.
Arguably if you want to later add a knoxidf or knoxadmin service topology with delegation, you'd want to configure the delegation service storage, and if you hadn't already configured it, you'd need to add that configuration, and so you'd need to restart knox for the gateway service storage update. So you could have known that the dynamic loading shouldn't work.
Given this is in the abstract base class, and it works similarly to the other knoxidf services, I suggest leaving it for now. It is convenient for the standard case: you want to use knoxidf or knoxadmin, and you need the service, and you don't want to have to add boilerplate. For follow on, we can consider skipping the topology service check or requiring a specific configuration to load the delegation service.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org