[Policy Store | Management Spec] Add policy privileges to spec and update admin service impl #1529

HonahX · 2025-05-06T00:33:10Z

This PR adds new policy related privileges to polaris-management-api.yml and update PolarisAdminService to allow granting new privileges

…me integration test

integration-tests/src/main/java/org/apache/polaris/service/it/env/ManagementApi.java

eric-maynard

LGTM! At some point, we should update the relevant docs

flyrain · 2025-05-14T16:38:28Z

spec/polaris-management-service.yml

+              minLength: 1
+              maxLength: 256


It's interesting we have this limits here, not only new entity like Policy, but also for table/view, etc. This becomes a an extra Polaris limit. Should we lift it somehow? I could see a corner case that users migrate a table with a long name will fail. Not a blocker for this PR though. cc @eric-maynard @dennishuo @collado-mike for more context.

Yeah, I follow the existing pattern to include these limits for the new schema. May be we can do this in a follow-up to remove all the limits if they are not needed?

Definitely, we can deal with it in a follow-up.

@HonahX shall we also contain a pattern definition here? for example

pattern: '^(?!\s*[s|S][y|Y][s|S][t|T][e|E][m|M]\$).*$'

Good point, I checked the yaml and seems the pattern is defined for catalogRole, principal, catalogName. For namespace and table there is pattern

NamespaceGrant: allOf: - $ref: '#/components/schemas/GrantResource' - type: object properties: namespace: type: array items: type: string privilege: $ref: '#/components/schemas/NamespacePrivilege' required: - namespace - privilege TableGrant: allOf: - $ref: '#/components/schemas/GrantResource' - type: object properties: namespace: type: array items: type: string tableName: type: string minLength: 1 maxLength: 256 privilege: $ref: '#/components/schemas/TablePrivilege' required: - namespace - tableName - privilege

I think this pattern is mainly to prevent granting privileges to system defined roles/principals. If we have similar use case for namespace, table, policy, we can add that in the future. WDYT?

singhpk234 · 2025-05-14T16:46:38Z

spec/polaris-management-service.yml

+        - POLICY_CREATE
+        - POLICY_WRITE
+        - POLICY_READ
+        - POLICY_DROP
+        - POLICY_LIST
+        - POLICY_FULL_METADATA
+        - CATALOG_ATTACH_POLICY
+        - CATALOG_DETACH_POLICY


[not-blocker] is it possible to add some comments on what each of them mean ?

+1 but we should probably do this separately, since the existing ones need comments too

+1 on doing this in a follow-up. Also do you think if a pointer to the site "Access Control" section would be an option to consider, so that we will have a single source of truth for the privileges' meaning.

singhpk234 · 2025-05-14T17:00:57Z

...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java

+    managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+    List<CatalogPrivilege> policyPrivilegesOnCatalog =
+        List.of(
+            CatalogPrivilege.POLICY_LIST,
+            CatalogPrivilege.POLICY_CREATE,
+            CatalogPrivilege.POLICY_DROP,
+            CatalogPrivilege.POLICY_WRITE,
+            CatalogPrivilege.POLICY_READ,
+            CatalogPrivilege.POLICY_FULL_METADATA,
+            CatalogPrivilege.CATALOG_ATTACH_POLICY,
+            CatalogPrivilege.CATALOG_DETACH_POLICY);
+    Stream<CatalogGrant> catalogGrants =
+        policyPrivilegesOnCatalog.stream()
+            .map(p -> new CatalogGrant(p, GrantResource.TypeEnum.CATALOG));
+    catalogGrants.forEach(g -> managementApi.addGrant(currentCatalogName, CATALOG_ROLE_2, g));
+
+    Assertions.assertThat(managementApi.listGrants(currentCatalogName, CATALOG_ROLE_2))
+        .extracting(GrantResources::getGrants)
+        .asInstanceOf(InstanceOfAssertFactories.list(GrantResource.class))
+        .map(gr -> ((CatalogGrant) gr).getPrivilege())
+        .containsExactlyInAnyOrderElementsOf(policyPrivilegesOnCatalog);


should we not undo the grants at catalog level to not effect subsequent test ?

The integration test has a

@AfterEach public void cleanUp() { client.cleanUp(adminCredentials); }

to drop the catalog role and the catalog so everything happens in the current test won't affect the subsequent ones.

singhpk234 · 2025-05-14T17:08:43Z

...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java

+
+  @Test
+  public void testGrantsOnNamespace() {
+    restCatalog.createNamespace(NS1);


optional : should we add an nested namespace test as well, this could just be running this in a loop, wdyt ? just testing the resolution.

Good suggestion! I think it may be a good test to add in PolicyCatalogHandlerAuthzTest where it test the actual authorization/inheritance resolution. This test is used to ensure the correct PolarisPrivilege is granted, the listGrants itself does not resolve the inheritance. I will add related ones later.

gh-yzou · 2025-05-14T20:19:42Z

spec/polaris-management-service.yml

+    PolicyPrivilege:
+      type: string
+      enum:
+        - CATALOG_MANAGE_ACCESS


question, why do we need CATALOG_MANAGE_ACCESS for PolicyPrivilege?

+1 this shouldn't be here

Isn't this essentially just function is a super-privilege? I see it only TablePrivileges too, for example, even though it's not in the docs.

Yeah, I added it here to be consistent with TablePrivileges, ViewPrivileges, and NamespacePrivileges. My understanding of the usage here would be allowing privilege management on a specific policy entity.

gh-yzou · 2025-05-14T20:20:38Z

spec/polaris-management-service.yml

+        - POLICY_DROP
+        - POLICY_WRITE
+        - POLICY_LIST
+        - POLICY_FULL_METADATA


i am always confused by the FULL_METADATA, but policy doesn't have a metadata concept, that simply refers to full access capability, right? would POLICY_FULL_ACCESS make more sense?

Isn't that the same as for namespaces? We call that NAMESPACE_FULL_METADATA.

Naming is not an easy task. For me both _FULL_METADATA and _FULL_ACCESS makes sense, but since in polaris we use _FULL_METADATA frequently: CATALOG_FULL_METADATA, NAMESPACE_FULL_METADATA, ... I think POLICY_FULL_METADATA can be more consistent with Polaris' naming convention. WDYT?

gh-yzou · 2025-05-14T20:24:14Z

spec/polaris-management-service.yml

+              minLength: 1
+              maxLength: 256


@HonahX shall we also contain a pattern definition here? for example

pattern: '^(?!\s*[s|S][y|Y][s|S][t|T][e|E][m|M]\$).*$'

gh-yzou · 2025-05-14T20:26:14Z

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

@@ -621,6 +623,19 @@ public Response addGrantToCatalogRole(
          adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
          break;
        }
+      case PolicyGrant policyGrant:
+        {


Shall we do those in separate PR? so that we can separate the change of API spec and backend implementaiton?

Maybe the current way makes it a little bit harder to review. I include both in the same PR to ensure we have test to cover that all the names and grants added to the spec is correct. Given that there has been lots of review going on, would you be okay to keep it in one for this time?

gh-yzou · 2025-05-14T20:32:53Z

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

+        new ResolverPath(List.of(catalogRoleName), PolarisEntityType.CATALOG_ROLE),
+        catalogRoleName);
+    ResolverStatus status = resolutionManifest.resolveAll();
+    if (status.getStatus() == ResolverStatus.StatusEnum.ENTITY_COULD_NOT_BE_RESOLVED) {


is taht implementation mostly the same as authorizeGrantOnNamespaceOperationOrThrow, how about let's merge thoe two functions, let the function take a EntityType, and an entity name. The only thing you need to take care here is probably the error message, and i think we can do a simple enum.

It's mostly like authorizeGrantOnTableLikeOperationOrThrow, but it deals with a PolicyIdentifier instead of a TableIdentifier.

Oh, i am actually referring to the namespaces, but all those function seems very similar. I think we can either internally handle this part according to the type, or just let the caller pass this argument List entityNames.

gh-yzou · 2025-05-14T20:43:45Z

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

+
+    PolarisResolvedPathWrapper resolvedPathWrapper =
+        resolutionManifest.getResolvedPath(
+            identifier, PolarisEntityType.POLICY, PolarisEntitySubType.ANY_SUBTYPE);


i see other places don't need the subtype argument, is there a reason that we need that for Policy?

I double-checked and find that getResolvedPath only verify the subtypes so it does not help for policy. I will remove that.

gh-yzou · 2025-05-14T20:44:16Z

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

+      throw new NoSuchPolicyException(String.format("Policy not exists: %s", identifier));
+    }
+
+    List<PolarisEntity> catalogPath = resolvedPathWrapper.getRawParentPath();


Similar as above, i think we can have a common function that is used for both NameSpace and Policy

gh-yzou · 2025-05-14T20:56:47Z

polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrivilege.java

@@ -150,6 +150,11 @@ public enum PolarisPrivilege {
  CATALOG_DETACH_POLICY(81, PolarisEntityType.CATALOG),
  NAMESPACE_DETACH_POLICY(82, PolarisEntityType.NAMESPACE),
  TABLE_DETACH_POLICY(83, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE),
+  POLICY_MANAGE_GRANTS_ON_SECURABLE(


I am actually little bit confused for this privilege, is that a privilege neds to be granted for roles? does it needs to be configured in the spec somewhere also?

This along with

NAMESPACE_MANAGED_GRANTS_ON_SECURABLE

TABLE_MANAGED_GRANTS_ON_SECURABLE

VIEW_MANAGED_GRANTS_ON_SECURABLE

CATALOG_MANAGED_GRANTS_ON_SECURABLE

forms the CATALOG_MANAGE_ACCESS. We currently do not allow granting these sub-privileges through management APIs. I just follow the pattern to ensure consistency and flexibility to make the privilege granting more fine-grained in the future if needed.

github-project-automation bot added this to Basic Kanban Board May 6, 2025

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

HonahX added 4 commits May 13, 2025 16:51

spec, privileges, auth op, super privileges, auth service impl and so…

a584472

…me integration test

Add more tests and revise spec error

0b23987

Fix spec

0c2e141

Fix nit after rebase

0cd21a2

HonahX force-pushed the honahx_add_policy_privileges_and_grant branch from 3702203 to 0cd21a2 Compare May 13, 2025 23:51

HonahX marked this pull request as ready for review May 14, 2025 04:53

HonahX requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain, ebyhr, singhpk234 and pingtimeout as code owners May 14, 2025 04:53

eric-maynard reviewed May 14, 2025

View reviewed changes

integration-tests/src/main/java/org/apache/polaris/service/it/env/ManagementApi.java Show resolved Hide resolved

eric-maynard approved these changes May 14, 2025

View reviewed changes

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

flyrain reviewed May 14, 2025

View reviewed changes

flyrain approved these changes May 14, 2025

View reviewed changes

singhpk234 reviewed May 14, 2025

View reviewed changes

gh-yzou reviewed May 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Policy Store | Management Spec] Add policy privileges to spec and update admin service impl #1529

[Policy Store | Management Spec] Add policy privileges to spec and update admin service impl #1529

HonahX commented May 6, 2025 •

edited

Loading

eric-maynard left a comment

flyrain May 14, 2025

sfc-gh-jojiang May 14, 2025

flyrain May 14, 2025

gh-yzou May 14, 2025

HonahX May 14, 2025

singhpk234 May 14, 2025 •

edited

Loading

eric-maynard May 14, 2025 •

edited

Loading

HonahX May 19, 2025

singhpk234 May 14, 2025

HonahX May 19, 2025

singhpk234 May 14, 2025

HonahX May 19, 2025

gh-yzou May 14, 2025

collado-mike May 14, 2025

eric-maynard May 14, 2025

HonahX May 19, 2025

gh-yzou May 14, 2025

eric-maynard May 14, 2025

HonahX May 19, 2025

gh-yzou May 14, 2025

gh-yzou May 14, 2025

HonahX May 19, 2025

gh-yzou May 14, 2025

collado-mike May 14, 2025

gh-yzou May 14, 2025 •

edited

Loading

gh-yzou May 14, 2025

HonahX May 19, 2025

gh-yzou May 14, 2025

gh-yzou May 14, 2025

HonahX May 19, 2025

[Policy Store | Management Spec] Add policy privileges to spec and update admin service impl #1529

Are you sure you want to change the base?

[Policy Store | Management Spec] Add policy privileges to spec and update admin service impl #1529

Conversation

HonahX commented May 6, 2025 • edited Loading

eric-maynard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 May 14, 2025 • edited Loading

Choose a reason for hiding this comment

eric-maynard May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-yzou May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX commented May 6, 2025 •

edited

Loading

singhpk234 May 14, 2025 •

edited

Loading

eric-maynard May 14, 2025 •

edited

Loading

gh-yzou May 14, 2025 •

edited

Loading