[#12405] fix(core): prevent silent authorization updates on closed catalogs by yuqi1129 · Pull Request #12793 · apache/gravitino

yuqi1129 · 2026-09-01T14:21:20Z

Depends on #12404. This branch adds one commit on top of its current head.

What changes were proposed in this pull request?

Preserve whether a BaseCatalog was configured with an authorization provider after close() clears its plugin.
Use a checked authorization-plugin lookup in AuthorizationUtils and FutureGrantManager.
Throw AuthorizationPluginException when a configured plugin is unexpectedly unavailable instead of silently skipping the authorization update.
Add coverage for closed catalogs, catalogs without authorization providers, privilege removal, and the future-grant path.

Why are the changes needed?

A catalog cache eviction can close the authorization plugin while a catalog reference is still in use. Silently treating the resulting null plugin as “authorization is disabled” can leave stale grants in the external authorization system.

#12404 makes the authorization call paths lease-scoped. This patch also enforces the lifecycle invariant so a future regression fails visibly instead of losing authorization updates silently.

Fix: #12405

Does this PR introduce any user-facing change?

Yes. If an authorization provider is configured but its plugin is unexpectedly unavailable, the operation now fails with AuthorizationPluginException instead of silently succeeding without updating the external authorization system.

There are no REST API or configuration-key changes.

How was this patch tested?

./gradlew :core:spotlessJavaCheck :core:test \
  --tests org.apache.gravitino.authorization.TestAuthorizationUtils \
  --tests org.apache.gravitino.authorization.TestFutureGrantManager \
  --tests org.apache.gravitino.connector.authorization.TestAuthorization \
  :core:javadoc -PskipITs

…ions CatalogManager closed CatalogWrapper synchronously from the catalog cache removal listener. Caffeine runs that listener asynchronously and outside the local TreeLock, so an expiry, a remote change-log invalidation or a drop could close the catalog, clear its reference and release the pooled ClassLoader while another thread was still running an operation on that wrapper. CatalogWrapper now counts active operations: tryAcquire() takes a lease, release() returns it and retire() (called from the removal listener) only marks the wrapper unusable for new leases. The catalog and the ClassLoader are cleaned up exactly once, when the wrapper is retired and its last lease is released. Operations obtain a CatalogLease from CatalogManager.acquireCatalogLease(), which reloads a fresh wrapper when the cached one has already retired, and all production uses of the wrapper were migrated to it.

…vate Drop CatalogLease.of(), which existed only so tests with a mocked CatalogManager could hand out a lease, and narrow tryAcquire/release/retire/ isRetired to package-private: every production caller lives in the catalog package. Tests outside that package now build leases through a test-only CatalogTestUtils helper. Also document that the deferred cleanup runs on the thread that releases the last lease, which is a request thread when the wrapper was evicted while an operation was in flight.

…tCapability CapabilityHelpers.getCapability() acquired the catalog lease inside the try block that wraps failures into a RuntimeException, so a missing catalog turned a 404 into a 500 on the normalize paths. Acquire the lease outside the try, as the catalog load was before, and wrap only the capability lookup.

- Make CatalogWrapper#catalog volatile so the cleanup that nulls it outside leaseLock is properly published to unleased readers (loadCatalogAndWrap callers); keep the nulling out of the lock so a slow catalog close cannot stall tryAcquire. Make classLoader and pool final, they never change after construction. - Keep the Javadoc @link to acquireCatalogLease on one line by importing NameIdentifier, so the link renders. - Build the expiring test's SecretManager from the same config as its CatalogManager.

…rapper-lease

…-wrapper-lease # Conflicts: # core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java # iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java # iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/provider/TestDynamicIcebergConfigProvider.java

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR hardens catalog/authorization lifecycle handling by introducing lease-scoped access to live BaseCatalog instances and failing fast when an authorization provider is configured but its plugin has become unavailable (e.g., due to cache eviction/close), preventing silent privilege drift.

Changes:

Introduces CatalogLease + lease-aware CatalogManager.acquireCatalogLease()/doWithCatalog() and updates call paths to avoid exposing/using live catalogs after eviction.
Updates authorization update paths (AuthorizationUtils, FutureGrantManager, hook dispatchers) to require a valid authorization plugin when configured, throwing AuthorizationPluginException otherwise.
Adds/updates tests to cover closed-catalog scenarios, lease behavior, and updated catalog fetching (including Iceberg REST auxiliary mode).

Reviewed changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataIdConverter.java	Updates mocking to use the new lease-aware catalog access path.
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/provider/TestDynamicIcebergConfigProvider.java	Refactors tests to use `CatalogManager`/lease-aware fetching and secret plaintext merging.
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergAsyncPurge.java	Stubs `acquireCatalogLease` for lease-aware code paths in purge tests.
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java	Switches internal fetcher to `CatalogManager` and adds lease-scoped property loading.
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergCleanupHelper.java	Uses `CatalogLease` to safely read catalog ID under concurrent eviction.
core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java	Updates tests to stub leases / `doWithCatalog` for owner hook behavior.
core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java	Updates tests to stub leases used by hook dispatchers.
core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java	Updates tests to stub leases used by hook dispatchers.
core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java	Updates tests to stub leases used by hook dispatchers.
core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java	Updates tests to stub leases used by hook dispatchers.
core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java	Updates tests to stub leases / `doWithCatalog` for owner hook behavior.
core/src/test/java/org/apache/gravitino/connector/authorization/TestAuthorization.java	Adds tests for “configured auth provider survives close” and “not configured” cases.
core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java	Adjusts tests away from `loadCatalog()` (now metadata snapshot) toward leased/live usage.
core/src/test/java/org/apache/gravitino/catalog/TestTopicOperationDispatcher.java	Adjusts tests to obtain live catalog via wrapper instead of `loadCatalog()`.
core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java	Adjusts tests for lease-safe access and live-catalog assertions.
core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java	Stubs `acquireCatalogLease` for normalize dispatcher tests.
core/src/test/java/org/apache/gravitino/catalog/TestSemanticModelNormalizeDispatcher.java	Stubs `acquireCatalogLease` for normalize dispatcher tests.
core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java	Updates missing-schema test to run live ops via `doWithCatalog`.
core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java	Stubs `acquireCatalogLease` for normalize dispatcher tests.
core/src/test/java/org/apache/gravitino/catalog/TestFunctionOperationDispatcher.java	Stubs `acquireCatalogLease` for function operation dispatcher tests.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogWrapperLease.java	New tests validating eviction/close behavior with leases and exactly-once cleanup semantics.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java	Updates tests for snapshot-returning `loadCatalog()` and wrapper lease behavior.
core/src/test/java/org/apache/gravitino/catalog/TestCapabilityHelpers.java	Adds tests for exception propagation + lease release on capability lookup failures.
core/src/test/java/org/apache/gravitino/catalog/CatalogTestUtils.java	New test utilities for stubbing `doWithCatalog` and creating unmanaged leases for mocks.
core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java	Switches owner-manager tests to lease-aware mocked paths.
core/src/test/java/org/apache/gravitino/authorization/TestFutureGrantManager.java	Adds coverage for failing when configured auth plugin is unavailable.
core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java	Adds coverage for new “checked plugin lookup” behavior and closed-catalog failures.
core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java	Switches tests to lease-aware mocked calls.
core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java	Switches tests to lease-aware mocked calls.
core/src/main/java/org/apache/gravitino/utils/ClassLoaderPool.java	Adds lifecycle locking and introduces `closeWhenIdle()` for graceful shutdown semantics.
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java	Uses lease-aware calls for future-grant and privilege removal on drop.
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java	Persists “authorization provider configured” flag across `close()` clearing the plugin.
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java	Updates dispatch helpers to acquire/close catalog leases around wrapper operations.
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java	Core lease implementation: wrapper retirement, active-op tracking, snapshot-based `loadCatalog()`, `doWithCatalog()`, and graceful shutdown.
core/src/main/java/org/apache/gravitino/catalog/CatalogLease.java	New lease type ensuring wrapper resources survive concurrent cache eviction.
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java	Uses `CatalogLease` to keep wrappers alive and preserve `NoSuchCatalogException` type.
core/src/main/java/org/apache/gravitino/authorization/FutureGrantManager.java	Uses checked authorization-plugin lookup via `AuthorizationUtils.getAuthorizationPlugin`.
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java	Converts authorization calls to lease-aware catalog access and adds “configured but null plugin” failure.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+  public static void removeCatalogPrivileges(NameIdentifier catalogIdent, List<String> locations) {
    // If we enable authorization, we should remove the privileges about the entity in the
    // authorization plugin.
    MetadataObject metadataObject =
-        MetadataObjects.of(null, catalog.name(), MetadataObject.Type.CATALOG);
+        MetadataObjects.of(null, catalogIdent.name(), MetadataObject.Type.CATALOG);
    MetadataObjectChange removeObject = MetadataObjectChange.remove(metadataObject, locations);

    callAuthorizationPluginImpl(
        authorizationPlugin -> {
          authorizationPlugin.onMetadataUpdated(removeObject);
        },
-        catalog);
+        GravitinoEnv.getInstance().catalogManager(),
+        catalogIdent);
  }


+  private static List<NameIdentifier> getMetadataObjectCatalogs(
      String metalake, MetadataObject metadataObject) {
    CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
-    List<Catalog> loadedCatalogs = Lists.newArrayList();
+    List<NameIdentifier> catalogIdents = Lists.newArrayList();
    if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
      NameIdentifier[] catalogs = catalogManager.listCatalogs(Namespace.of(metalake));
-      // ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we need `BaseCatalog` to
-      // call authorization plugin method.
-      for (NameIdentifier catalog : catalogs) {
-        loadedCatalogs.add(catalogManager.loadCatalog(catalog));
-      }
+      catalogIdents.addAll(Arrays.asList(catalogs));
    } else if (needApplyAuthorization(metadataObject.type())) {
      NameIdentifier catalogIdent =
          NameIdentifierUtil.getCatalogIdentifier(
              MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
-      Catalog catalog = catalogManager.loadCatalog(catalogIdent);
-      loadedCatalogs.add(catalog);
+      catalogIdents.add(catalogIdent);
    }

-    return loadedCatalogs;
+    return catalogIdents;
  }


yuqi1129 · 2026-09-01T14:42:43Z

We need to merge #12404 first.

github-actions · 2026-09-01T16:36:52Z

Code Coverage Report

Overall Project	68.89% `+0.07%`	🟢
Files changed	72.2%	🟢

Module	Coverage
aliyun	19.74%	🔴
api	51.62%	🟢
authorization-common	85.96%	🟢
authorization-ranger	4.38%	🔴
aws	53.54%	🟢
azure	32.1%	🔴
catalog-common	19.1%	🔴
catalog-fileset	80.3%	🟢
catalog-glue	69.24%	🟢
catalog-hive	82.96%	🟢
catalog-jdbc-common	45.69%	🟢
catalog-jdbc-doris	82.69%	🟢
catalog-jdbc-mysql	79.33%	🟢
catalog-jdbc-postgresql	83.39%	🟢
catalog-jdbc-starrocks	79.16%	🟢
catalog-kafka	76.99%	🟢
catalog-lakehouse-generic	60.55%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.93%	🟢
catalog-lakehouse-paimon	84.26%	🟢
catalog-model	77.99%	🟢
cli	44.51%	🟢
client-java	77.37%	🟢
common	56.34%	🟢
core	83.88% `-0.74%`	🟢
filesystem-hadoop3	76.45%	🟢
flink	0.0%	🔴
flink-common	52.1%	🟢
flink-runtime	0.0%	🔴
gcp	32.2%	🔴
hadoop-auth	68.0%	🟢
hadoop-common	17.84%	🔴
hive-metastore-common	53.4%	🟢
iceberg-aliyun-bundle	0.0%	🔴
iceberg-common	64.75%	🟢
iceberg-rest-server	75.87% `-0.72%`	🟢
idp-basic	86.42%	🟢
integration-test-common	0.0%	🔴
jobs	62.92%	🟢
lance-common	32.52%	🔴
lance-rest-server	65.69%	🟢
lineage	53.02%	🟢
optimizer	83.17%	🟢
optimizer-api	21.95%	🔴
server	88.47%	🟢
server-common	80.67%	🟢
spark	28.57%	🔴
spark-common	48.92%	🟢
tencent	81.78%	🟢
trino-connector	51.26%	🟢

Files

Module	File	Coverage
core	CatalogLease.java	100.0%	🟢
	FutureGrantManager.java	98.48%	🟢
	OperationDispatcher.java	88.64%	🟢
	ClassLoaderPool.java	86.05%	🟢
	CapabilityHelpers.java	79.09%	🟢
	CatalogManager.java	71.94%	🟢
	BaseCatalog.java	68.34%	🟢
	AuthorizationUtils.java	65.25%	🟢
	CatalogHookDispatcher.java	36.67%	🔴
iceberg-rest-server	IcebergCleanupHelper.java	100.0%	🟢
	DynamicIcebergConfigProvider.java	52.86%	🔴

A catalog cache eviction can close a catalog while a reference to it is still in use. Its authorization plugin is then null, which the call paths read as "this catalog has no authorization", so the update is skipped and the external authorization system keeps stale grants. BaseCatalog now remembers whether an authorization provider was configured, a flag close() does not clear, so a null plugin on such a catalog is distinguishable from a catalog that never had one. The authorization call paths route their lookup through a checked helper that throws AuthorizationPluginException in that case instead of silently skipping the update. Claude-Session: https://claude.ai/code/session_011A4FqHJarzs2xvs7WbusMT

yuqi1129 added 9 commits August 10, 2026 18:26

[apache#12403] fix(core): address catalog lease lifecycle gaps

06b0aaa

Merge remote-tracking branch 'apache/main' into issue-12403-catalog-w…

4d70335

…rapper-lease

[apache#12403] test(server-common): update catalog lease mock

50f663c

Merge upstream/main into issue-12403-catalog-wrapper-lease

fdc0848

Copilot AI lite review requested due to automatic review settings September 1, 2026 14:21

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Merge branch 'main' into issue-12403-catalog-wrapper-lease

5984f07

yuqi1129 marked this pull request as draft September 1, 2026 14:42

Copilot started reviewing on behalf of yuqi1129 September 1, 2026 14:48 View session

yuqi1129 force-pushed the issue-12405-authz-lease branch from 2583ae4 to 3c33d38 Compare September 2, 2026 12:56

yuqi1129 self-assigned this Sep 4, 2026

yuqi1129 added the branch-1.3 Automatically cherry-pick commit to branch-1.3 label Sep 4, 2026

Merge apache/main into issue-12405-authz-lease

fedc61e

Navigation Menu

Uh oh!

[#12405] fix(core): prevent silent authorization updates on closed catalogs - #12793

[#12405] fix(core): prevent silent authorization updates on closed catalogs#12793
yuqi1129 wants to merge 12 commits into
apache:mainfrom
yuqi1129:issue-12405-authz-lease

yuqi1129 commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

yuqi1129 commented Sep 1, 2026

Uh oh!

github-actions Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

yuqi1129 commented Sep 1, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

yuqi1129 commented Sep 1, 2026

Uh oh!

github-actions Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 1, 2026 •

edited

Loading