Fix Namespace resolution on grant/revoke privilege operations by poojanilangekar · Pull Request #2170 · apache/polaris

poojanilangekar · 2025-07-24T00:02:53Z

I encountered the bug while implementing RBAC for federation.
For grant/revoke operations on tables, polaris checks the subtype of the leaf entity and make sure it matches. However, for namespace the only check is that the resolved path is not null.

Ideally we should check that the entire path matches the one sent in the request. Further, all entities except the root should be of the type NAMESPACE.

Testing:
Added unit tests.

poojanilangekar · 2025-07-24T00:03:20Z

CC: @eric-maynard @dennishuo

dimas-b

Hi @poojanilangekar , this fix makes sense to me, but I've got some comments/suggestions :)

dimas-b · 2025-07-24T00:17:56Z

service/common/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java

+            isNamespaceFullyResolvedMethod.invoke(
+                adminService, null, "test-catalog", Namespace.of("ns1"));
+
+    assertThat(result).isFalse();


nit: Why not inline result? The expression is already multi-line 🤔

dimas-b · 2025-07-24T00:18:53Z

service/common/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java

+        (boolean)
+            isNamespaceFullyResolvedMethod.invoke(
+                adminService, resolvedPathWrapper, catalogName, namespace);


This pattern is repeated... Maybe add a help method in this test class?

I've moved the test, so that fixed it.

dimas-b · 2025-07-24T00:20:33Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

+
+    List<PolarisEntity> fullPath = resolvedPathWrapper.getRawFullPath();
+    int expectedPathLength = 1 + namespace.levels().length;
+    if (fullPath.size() < expectedPathLength) {


why not ==?

Done, changed it to !=

dimas-b · 2025-07-24T00:23:07Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

+   * @param namespace the namespace we're trying to resolve
+   * @return true if the namespace is considered fully resolved for the given catalog type
+   */
+  private boolean isNamespaceFullyResolved(


Why keep it as a private instance method? It does not use any state from this... It could be a package-private static method in a utility class.

... or perhaps a public method in PolarisResolvedPathWrapper? 🤔

poojanilangekar

Done, I have addressed your comments.

poojanilangekar · 2025-07-24T00:51:36Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

+
+    List<PolarisEntity> fullPath = resolvedPathWrapper.getRawFullPath();
+    int expectedPathLength = 1 + namespace.levels().length;
+    if (fullPath.size() < expectedPathLength) {


Done, changed it to !=

poojanilangekar · 2025-07-24T00:51:55Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

+   * @param namespace the namespace we're trying to resolve
+   * @return true if the namespace is considered fully resolved for the given catalog type
+   */
+  private boolean isNamespaceFullyResolved(


poojanilangekar · 2025-07-24T00:52:03Z

service/common/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java

+            isNamespaceFullyResolvedMethod.invoke(
+                adminService, null, "test-catalog", Namespace.of("ns1"));
+
+    assertThat(result).isFalse();


poojanilangekar · 2025-07-24T00:52:38Z

service/common/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java

+        (boolean)
+            isNamespaceFullyResolvedMethod.invoke(
+                adminService, resolvedPathWrapper, catalogName, namespace);


I've moved the test, so that fixed it.

dimas-b · 2025-07-24T00:58:00Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisResolvedPathWrapper.java

+    }
+
+    for (int i = 0; i < namespace.levels().length; i++) {
+      if (!fullPath.get(i + 1).getName().equals(namespace.levels()[i])


This may not be a big deal, but since we're always scanning this list from head to tail, using iterators might be more portable and easier to understand. Probably as efficient as .get(N) for array-based lists, but potentially more efficient for linked lists.

It might be preferable to move namespace.levels() to a local variable. Iterate it in for, but do simple iterator.next() for fullPath.

dimas-b · 2025-07-24T00:58:56Z

...s-core/src/test/java/org/apache/polaris/core/persistence/PolarisResolvedPathWrapperTest.java

+
+    boolean result = wrapper.isFullyResolvedNamespace("test-catalog", Namespace.of("ns1"));
+
+    assertThat(result).isFalse();


nit: inline?

dimas-b · 2025-07-24T01:01:02Z

service/common/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java

+      throws Exception {
+    String catalogName = "test-catalog";
+    String catalogRoleName = "test-role";
+    Namespace namespace = Namespace.of("incomplete-ns");


This is a single-level namespace... How is "incomplete" different from "non existent"?

dimas-b · 2025-07-24T01:05:14Z

...s-core/src/test/java/org/apache/polaris/core/persistence/PolarisResolvedPathWrapperTest.java

+    Namespace namespace = Namespace.of("ns1");
+
+    PolarisEntity catalogEntity = createEntity(catalogName, PolarisEntityType.CATALOG);
+    PolarisEntity wrongTypeEntity = createEntity("ns1", PolarisEntityType.TABLE_LIKE);


For completeness this test should probably also have some NS level with the right type and some with another type.

dimas-b

Thanks, @poojanilangekar ! I'll be merging late Friday if there are no more review (or Mon if I forget :) )

poojanilangekar · 2025-07-28T16:14:41Z

Hi @dimas-b, could you please merge this PR?

* fix(deps): update dependency com.google.errorprone:error_prone_core to v2.41.0 (apache#2181) * Simplify bootstrapServiceAndCreatePolarisPrincipalForRealm (apache#2172) this is a small follow-up to 5faa371 because the same pattern existed for this method. note that we do some minor additional "formatting" changes to minimize the diff between the two files (as they were originally copy pasted). this could lead to having a common base class in the future. * fix(deps): update dependency boto3 to v1.39.13 (apache#2182) * Add podman support (apache#2143) * Add Polaris Community Meeting 2025-07-24 (apache#2184) * fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.7.0 (apache#2185) * Push AccessConfig creation to PolarisStorageIntegration (apache#2171) This refactoring does not change Polaris behaviour. * Move storage-specific access properties processing logic from core code to storage integration implementations. * Add `isExpirationTimestamp` flag to `StorageAccessProperty` to allow them to be processed uniformly. * Prepare for supporting access config properties that may have different values in Polaris Servers and Clients. This enables future enhancements to support different S3 endpoint DNS names in servers and clients for apache#1530 * Fix doc to remove privileges may take up to one hour to take effect and add Policy to securable object (apache#2009) * fix(deps): update dependency boto3 to v1.39.14 (apache#2186) * chore(deps): update plugin jetbrains-changelog to v2.3.0 (apache#2187) * fix(deps): update dependency software.amazon.awssdk:bom to v2.32.9 (apache#2191) * Add Principal lookup helpers to PolarisMetaStoreManager (apache#2174) `PolarisMetaStoreManager.readEntityByName` is quite a low-level api, so we can simplify a lot of callers with additional helpers: - add `PolarisMetaStoreManager.findRootPrincipal` - add `PolarisMetaStoreManager.findPrincipalByName` - add `PolarisMetaStoreManager.findPrincipalRoleByName` also we now prefer `PolarisEntityConstants` where applicable * Remove PolarisDiagnostics from json utils (apache#2176) With transitive cleanups of, PolarisStorageConfigurationInfo, ConnectionConfigInfoDpo, BaseMetaStoreManager, PolarisObjectMapperUtil, CurrentContext * Fix Namespace resolution on grant/revoke privilege operations (apache#2170) * Fix Namespace resolution on grant/revoke privilege operations * Move isFullyResolvedNamespace to PolarisResolvedPathWrapper * fix(deps): update dependency boto3 to v1.39.15 (apache#2199) * fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.54.0 (apache#2200) * fix(deps): update dependency boto3 to v1.39.16 (apache#2209) * chore(deps): update actions/stale digest to a92fd57 (apache#2208) * fix(deps): update quarkus platform and group to v3.25.0 (apache#2167) * NoSQL updates * Last merged commit 914be46 --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: CG <[email protected]> Co-authored-by: Pooja Nilangekar <[email protected]>

Fix Namespace resolution on grant/revoke privilege operations

d846a7c

github-project-automation bot added this to Basic Kanban Board Jul 24, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 24, 2025

dimas-b reviewed Jul 24, 2025

View reviewed changes

Move isFullyResolvedNamespace to PolarisResolvedPathWrapper

e6b3c01

poojanilangekar commented Jul 24, 2025

View reviewed changes

dimas-b reviewed Jul 24, 2025

View reviewed changes

Address review comments

51c840e

dimas-b approved these changes Jul 24, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 24, 2025

dimas-b merged commit 364b53c into apache:main Jul 28, 2025
12 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 28, 2025

poojanilangekar deleted the check-namespace-resolution branch August 6, 2025 20:07


		boolean result = wrapper.isFullyResolvedNamespace("test-catalog", Namespace.of("ns1"));

		assertThat(result).isFalse();

Conversation

poojanilangekar commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poojanilangekar commented Jul 24, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poojanilangekar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

poojanilangekar commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

poojanilangekar commented Jul 24, 2025 •

edited

Loading