Copilot commented on code in PR #9785:
URL: https://github.com/apache/gravitino/pull/9785#discussion_r2797250351
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -988,15 +997,16 @@ private CatalogWrapper createCatalogWrapper(
* properties) of the catalog entity.
*
* @param entity The catalog entity.
- * @return The resolved properties.
+ * @return The resolved BaseCatalog instance.
*/
- private Map<String, String> getResolvedProperties(CatalogEntity entity) {
+ private BaseCatalog getBaseCatalog(CatalogEntity entity) {
Map<String, String> conf = entity.getProperties();
String provider = entity.getProvider();
try (IsolatedClassLoader classLoader = createClassLoader(provider, conf)) {
BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity);
- return classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
Review Comment:
`getBaseCatalog` only preloads `catalog.properties()` within
`IsolatedClassLoader.withClassLoader(...)`, but callers subsequently access
`catalog.catalogPropertiesMetadata()` outside the isolated classloader context.
If metadata construction/initialization depends on the thread context
classloader (e.g., ServiceLoader-based discovery of sensitive keys), this can
lead to incorrect metadata and potential secret exposure. Preload/initialize
`catalog.catalogPropertiesMetadata()` (and/or `propertyEntries()`) under
`withClassLoader(...)` as well, or return extracted properties+metadata instead
of returning the raw catalog instance.
```suggestion
classLoader.withClassLoader(
cl -> {
// Preload properties, capability, and properties metadata
within the isolated
// classloader context so that any classloader-sensitive
initialization (for example,
// ServiceLoader-based discovery) uses the correct thread
context classloader.
catalog.properties();
catalog.capability();
catalog.catalogPropertiesMetadata();
return null;
},
RuntimeException.class);
```
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -372,7 +375,13 @@ public Catalog[] listCatalogsInfo(Namespace namespace)
throws NoSuchMetalakeExce
// provider is "fileset", still using "hadoop" will lead to catalog
loading issue. So
// after reading the catalog entity, we convert it to the new
fileset catalog entity.
.map(this::convertFilesetCatalogEntity)
- .map(e -> e.toCatalogInfoWithResolvedProps(getResolvedProperties(e)))
+ .map(
+ e -> {
+ BaseCatalog catalog = getBaseCatalog(e);
+ return new CatalogInfoWithPropertyMeta(
+ e.toCatalogInfoWithResolvedProps(catalog.properties()),
+ catalog.catalogPropertiesMetadata());
Review Comment:
In listCatalogsInfo, `catalog.catalogPropertiesMetadata()` is invoked
outside the isolated classloader context (and after `getBaseCatalog` has closed
its `IsolatedClassLoader`). Some catalog PropertiesMetadata implementations
(e.g., FilesetCatalogPropertiesMetadata now uses ServiceLoader via the thread
context classloader to discover credential-provider sensitive keys) can be
initialized incorrectly in this case, which may cause sensitive keys to not be
marked as sensitive and therefore not filtered from REST responses when
`filterSensitiveProperties` is enabled. Consider resolving/initializing
`catalogPropertiesMetadata` (and ideally `propertyEntries()`) inside
`IsolatedClassLoader.withClassLoader(...)` and passing the resulting metadata
object/data out, rather than calling it here.
```suggestion
Thread currentThread = Thread.currentThread();
ClassLoader originalClassLoader =
currentThread.getContextClassLoader();
try {
currentThread.setContextClassLoader(catalog.getClass().getClassLoader());
return new CatalogInfoWithPropertyMeta(
e.toCatalogInfoWithResolvedProps(catalog.properties()),
catalog.catalogPropertiesMetadata());
} finally {
currentThread.setContextClassLoader(originalClassLoader);
}
```
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -256,6 +256,142 @@ public static <E> E[] filterByExpression(
(unused) -> null);
}
+ /**
+ * Partitions an array of entities into two arrays based on two
authorization expressions. An
+ * entity that satisfies the first expression goes into the first array. An
entity that does not
+ * satisfy the first expression but satisfies the second expression goes
into the second array.
+ * Entities that satisfy neither expression are excluded from both arrays.
+ *
+ * @param metalake metalake name
+ * @param firstExpression authorization expression for the first array
+ * @param secondExpression authorization expression for the second array
+ * @param entityType entity type
+ * @param entities array of metadata entities to partition
+ * @param toNameIdentifier function to convert entity to NameIdentifier
+ * @return A FilterResult containing both partitioned arrays
+ * @param <E> Entity class
+ */
+ public static <E> FilterResult<E, E> partitionByTwoExpressions(
+ String metalake,
+ String firstExpression,
+ String secondExpression,
+ Entity.EntityType entityType,
+ E[] entities,
+ Function<E, NameIdentifier> toNameIdentifier) {
+ Class<?> componentType = entities.getClass().getComponentType();
+ if (!enableAuthorization()) {
+ E[] emptyArray = createArray(componentType, 0);
+ return new FilterResult<>(entities, emptyArray);
+ }
+ checkExecutor();
+
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ GravitinoAuthorizer authorizer =
+ GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
+ AuthorizationRequestContext authorizationRequestContext = new
AuthorizationRequestContext();
+ List<CompletableFuture<int[]>> futures = new ArrayList<>();
Review Comment:
`partitionByTwoExpressions` creates a new `AuthorizationRequestContext` but
never sets `originalAuthorizationExpression` (unlike `doFilter`). This makes
downstream authz debug logs (e.g., JcasbinAuthorizer) print a null expression
for these checks, which reduces observability when diagnosing filtering issues.
Consider setting it (e.g., to the first/second expression or a combined label)
before running the async evaluations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
