You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "HonahX (via GitHub)" <gi...@apache.org> on 2025/05/06 00:33:12 UTC
[PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
HonahX opened a new pull request, #1529:
URL: https://github.com/apache/polaris/pull/1529
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
Not ready for review
This PR adds new policy related privileges to `polaris-management-api.yml` and update `PolarisAdminService` to allow granting new privileges
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096578731
##########
spec/polaris-management-service.yml:
##########
@@ -1387,6 +1417,26 @@ components:
- tableName
- privilege
+ PolicyGrant:
+ allOf:
+ - $ref: '#/components/schemas/GrantResource'
+ - type: object
+ properties:
+ namespace:
+ type: array
+ items:
+ type: string
+ policyName:
+ type: string
+ minLength: 1
+ maxLength: 256
Review Comment:
Yes, it's weird that we have these length limitations on tableNames, namespaceName, etc. Let me remove that for policy in this PR, since even if we need the limitation, we need that to appear in `policy-api.yaml` too. Let's deal with other existing length limitations in a follow-up PR/dev discussion.
I do not think we necessarily need pattern restriction on policy name here as management APIs does not create new policies, any invalid names would just result in an not found error.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX merged PR #1529:
URL: https://github.com/apache/polaris/pull/1529
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096198389
##########
spec/polaris-management-service.yml:
##########
@@ -1278,6 +1278,20 @@ components:
- TABLE_READ_DATA
- TABLE_WRITE_DATA
- TABLE_FULL_METADATA
+ - TABLE_ATTACH_POLICY
+ - TABLE_DETACH_POLICY
+
+ PolicyPrivilege:
+ type: string
+ enum:
+ - CATALOG_MANAGE_ACCESS
+ - POLICY_READ
+ - POLICY_DROP
+ - POLICY_WRITE
+ - POLICY_LIST
+ - POLICY_FULL_METADATA
Review Comment:
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?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096225357
##########
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(
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096134438
##########
spec/polaris-management-service.yml:
##########
@@ -1334,6 +1356,14 @@ components:
- NAMESPACE_FULL_METADATA
- TABLE_FULL_METADATA
- VIEW_FULL_METADATA
+ - POLICY_CREATE
+ - POLICY_WRITE
+ - POLICY_READ
+ - POLICY_DROP
+ - POLICY_LIST
+ - POLICY_FULL_METADATA
+ - CATALOG_ATTACH_POLICY
+ - CATALOG_DETACH_POLICY
Review Comment:
+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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "sfc-gh-yzou (via GitHub)" <gi...@apache.org>.
sfc-gh-yzou commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096555697
##########
spec/polaris-management-service.yml:
##########
@@ -1387,6 +1417,26 @@ components:
- tableName
- privilege
+ PolicyGrant:
+ allOf:
+ - $ref: '#/components/schemas/GrantResource'
+ - type: object
+ properties:
+ namespace:
+ type: array
+ items:
+ type: string
+ policyName:
+ type: string
+ minLength: 1
+ maxLength: 256
Review Comment:
I am actually not quite sure if i got this part, I double checked the policyName definition, which is the following
```
type: string
pattern: '^[A-Za-z0-9\-_]+$'
```
there is no min and max limitation, will we run into the problem that when we create the policy, the name might exceed the limitation, but when we grant the the policy, user see the error. Should all those limitation be defined on the policyName, and here we just refer to the policy name definition, similar as privilege?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096215517
##########
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:
+ {
Review Comment:
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?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096221531
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1937,4 +2023,68 @@ private boolean revokePrivilegeOnTableLikeFromRole(
privilege)
.isSuccess();
}
+
+ private boolean grantPrivilegeOnPolicyEntityToRole(
+ String catalogName,
+ String catalogRoleName,
+ PolicyIdentifier identifier,
+ PolarisPrivilege privilege) {
+ if (findCatalogByName(catalogName).isEmpty()) {
+ throw new NotFoundException("Parent catalog %s not found", catalogName);
+ }
+ PolarisEntity catalogRoleEntity =
+ findCatalogRoleByName(catalogName, catalogRoleName)
+ .orElseThrow(() -> new NotFoundException("CatalogRole %s not found", catalogRoleName));
+
+ PolarisResolvedPathWrapper resolvedPathWrapper =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.POLICY, PolarisEntitySubType.ANY_SUBTYPE);
Review Comment:
I double-checked and find that `getResolvedPath` only verify the subtypes so it does not help for policy. I will remove that.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096188949
##########
spec/polaris-management-service.yml:
##########
@@ -1278,6 +1278,20 @@ components:
- TABLE_READ_DATA
- TABLE_WRITE_DATA
- TABLE_FULL_METADATA
+ - TABLE_ATTACH_POLICY
+ - TABLE_DETACH_POLICY
+
+ PolicyPrivilege:
+ type: string
+ enum:
+ - CATALOG_MANAGE_ACCESS
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096522587
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1937,4 +2023,68 @@ private boolean revokePrivilegeOnTableLikeFromRole(
privilege)
.isSuccess();
}
+
+ private boolean grantPrivilegeOnPolicyEntityToRole(
+ String catalogName,
+ String catalogRoleName,
+ PolicyIdentifier identifier,
+ PolarisPrivilege privilege) {
+ if (findCatalogByName(catalogName).isEmpty()) {
+ throw new NotFoundException("Parent catalog %s not found", catalogName);
+ }
+ PolarisEntity catalogRoleEntity =
+ findCatalogRoleByName(catalogName, catalogRoleName)
+ .orElseThrow(() -> new NotFoundException("CatalogRole %s not found", catalogRoleName));
+
+ PolarisResolvedPathWrapper resolvedPathWrapper =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.POLICY, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPathWrapper == null) {
+ throw new NoSuchPolicyException(String.format("Policy not exists: %s", identifier));
+ }
+
+ List<PolarisEntity> catalogPath = resolvedPathWrapper.getRawParentPath();
Review Comment:
Thanks! Similar as above, how about we do all the refactoring as a whole in https://github.com/apache/polaris/pull/1621 once this is in.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096165510
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java:
##########
@@ -427,6 +442,176 @@ NS2_T1, new Schema(Types.NestedField.optional(1, "string", Types.StringType.get(
restCatalog.dropTable(NS2_T1);
}
+ @Test
+ public void testGrantsOnPolicy() {
+ restCatalog.createNamespace(NS1);
+ try {
+ policyApi.createPolicy(
+ currentCatalogName,
+ NS1_P1,
+ PredefinedPolicyTypes.DATA_COMPACTION,
+ EXAMPLE_TABLE_MAINTENANCE_POLICY_CONTENT,
+ "test policy");
+ managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+ Stream<PolicyGrant> policyGrants =
+ Arrays.stream(PolicyPrivilege.values())
+ .map(
+ p ->
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ p,
+ GrantResource.TypeEnum.POLICY));
+ policyGrants.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 -> ((PolicyGrant) gr).getPrivilege())
+ .containsExactlyInAnyOrder(PolicyPrivilege.values());
+
+ PolicyGrant policyReadGrant =
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ PolicyPrivilege.POLICY_READ,
+ GrantResource.TypeEnum.POLICY);
+ managementApi.revokeGrant(currentCatalogName, CATALOG_ROLE_2, policyReadGrant);
+
+ Assertions.assertThat(managementApi.listGrants(currentCatalogName, CATALOG_ROLE_2))
+ .extracting(GrantResources::getGrants)
+ .asInstanceOf(InstanceOfAssertFactories.list(GrantResource.class))
+ .map(gr -> ((PolicyGrant) gr).getPrivilege())
+ .doesNotContain(PolicyPrivilege.POLICY_READ);
+ } finally {
+ policyApi.purge(currentCatalogName, NS1);
+ }
+ }
+
+ @Test
+ public void testGrantsOnNonExistingPolicy() {
+ restCatalog.createNamespace(NS1);
+
+ try {
+ managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+ Stream<PolicyGrant> policyGrants =
+ Arrays.stream(PolicyPrivilege.values())
+ .map(
+ p ->
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ p,
+ GrantResource.TypeEnum.POLICY));
+ policyGrants.forEach(
+ g -> {
+ try (Response response =
+ managementApi
+ .request(
+ "v1/catalogs/{cat}/catalog-roles/{role}/grants",
+ Map.of("cat", currentCatalogName, "role", "catalogrole2"))
+ .put(Entity.json(g))) {
+
+ assertThat(response.getStatus()).isEqualTo(NOT_FOUND.getStatusCode());
+ }
+ });
+ } finally {
+ policyApi.purge(currentCatalogName, NS1);
+ }
+ }
+
+ @Test
+ public void testGrantsOnNamespace() {
+ restCatalog.createNamespace(NS1);
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096138502
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java:
##########
@@ -427,6 +442,176 @@ NS2_T1, new Schema(Types.NestedField.optional(1, "string", Types.StringType.get(
restCatalog.dropTable(NS2_T1);
}
+ @Test
+ public void testGrantsOnPolicy() {
+ restCatalog.createNamespace(NS1);
+ try {
+ policyApi.createPolicy(
+ currentCatalogName,
+ NS1_P1,
+ PredefinedPolicyTypes.DATA_COMPACTION,
+ EXAMPLE_TABLE_MAINTENANCE_POLICY_CONTENT,
+ "test policy");
+ managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+ Stream<PolicyGrant> policyGrants =
+ Arrays.stream(PolicyPrivilege.values())
+ .map(
+ p ->
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ p,
+ GrantResource.TypeEnum.POLICY));
+ policyGrants.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 -> ((PolicyGrant) gr).getPrivilege())
+ .containsExactlyInAnyOrder(PolicyPrivilege.values());
+
+ PolicyGrant policyReadGrant =
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ PolicyPrivilege.POLICY_READ,
+ GrantResource.TypeEnum.POLICY);
+ managementApi.revokeGrant(currentCatalogName, CATALOG_ROLE_2, policyReadGrant);
+
+ Assertions.assertThat(managementApi.listGrants(currentCatalogName, CATALOG_ROLE_2))
+ .extracting(GrantResources::getGrants)
+ .asInstanceOf(InstanceOfAssertFactories.list(GrantResource.class))
+ .map(gr -> ((PolicyGrant) gr).getPrivilege())
+ .doesNotContain(PolicyPrivilege.POLICY_READ);
+ } finally {
+ policyApi.purge(currentCatalogName, NS1);
+ }
+ }
+
+ @Test
+ public void testGrantsOnNonExistingPolicy() {
+ restCatalog.createNamespace(NS1);
+
+ try {
+ managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+ Stream<PolicyGrant> policyGrants =
+ Arrays.stream(PolicyPrivilege.values())
+ .map(
+ p ->
+ new PolicyGrant(
+ Arrays.asList(NS1.levels()),
+ NS1_P1.getName(),
+ p,
+ GrantResource.TypeEnum.POLICY));
+ policyGrants.forEach(
+ g -> {
+ try (Response response =
+ managementApi
+ .request(
+ "v1/catalogs/{cat}/catalog-roles/{role}/grants",
+ Map.of("cat", currentCatalogName, "role", "catalogrole2"))
+ .put(Entity.json(g))) {
+
+ assertThat(response.getStatus()).isEqualTo(NOT_FOUND.getStatusCode());
+ }
+ });
+ } finally {
+ policyApi.purge(currentCatalogName, NS1);
+ }
+ }
+
+ @Test
+ public void testGrantsOnNamespace() {
+ restCatalog.createNamespace(NS1);
+ try {
+ managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_2);
+ List<NamespacePrivilege> policyPrivilegesOnNamespace =
+ List.of(
+ NamespacePrivilege.POLICY_LIST,
+ NamespacePrivilege.POLICY_CREATE,
+ NamespacePrivilege.POLICY_DROP,
+ NamespacePrivilege.POLICY_WRITE,
+ NamespacePrivilege.POLICY_READ,
+ NamespacePrivilege.POLICY_FULL_METADATA,
+ NamespacePrivilege.NAMESPACE_ATTACH_POLICY,
+ NamespacePrivilege.NAMESPACE_DETACH_POLICY);
+ Stream<NamespaceGrant> namespaceGrants =
+ policyPrivilegesOnNamespace.stream()
+ .map(
+ p ->
+ new NamespaceGrant(
+ Arrays.asList(NS1.levels()), p, GrantResource.TypeEnum.NAMESPACE));
+ namespaceGrants.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 -> ((NamespaceGrant) gr).getPrivilege())
+ .containsExactlyInAnyOrderElementsOf(policyPrivilegesOnNamespace);
+ } finally {
+ policyApi.purge(currentCatalogName, NS1);
+ }
+ }
+
+ @Test
+ public void testGrantsOnCatalog() {
+ 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);
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "sfc-gh-yzou (via GitHub)" <gi...@apache.org>.
sfc-gh-yzou commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096542651
##########
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(
Review Comment:
i see.
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -528,6 +533,47 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
catalogRoleWrapper);
}
+ private void authorizeGrantOnPolicyOperationOrThrow(
+ PolarisAuthorizableOperation op,
+ String catalogName,
+ PolicyIdentifier identifier,
+ String catalogRoleName) {
+ resolutionManifest =
+ entityManager.prepareResolutionManifest(callContext, securityContext, catalogName);
+ resolutionManifest.addPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.identifierToList(identifier.getNamespace(), identifier.getName()),
+ PolarisEntityType.POLICY),
+ identifier);
+ resolutionManifest.addPath(
+ new ResolverPath(List.of(catalogRoleName), PolarisEntityType.CATALOG_ROLE),
+ catalogRoleName);
+ ResolverStatus status = resolutionManifest.resolveAll();
+ if (status.getStatus() == ResolverStatus.StatusEnum.ENTITY_COULD_NOT_BE_RESOLVED) {
Review Comment:
sg!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #1529:
URL: https://github.com/apache/polaris/pull/1529#discussion_r2096522140
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -528,6 +533,47 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
catalogRoleWrapper);
}
+ private void authorizeGrantOnPolicyOperationOrThrow(
+ PolarisAuthorizableOperation op,
+ String catalogName,
+ PolicyIdentifier identifier,
+ String catalogRoleName) {
+ resolutionManifest =
+ entityManager.prepareResolutionManifest(callContext, securityContext, catalogName);
+ resolutionManifest.addPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.identifierToList(identifier.getNamespace(), identifier.getName()),
+ PolarisEntityType.POLICY),
+ identifier);
+ resolutionManifest.addPath(
+ new ResolverPath(List.of(catalogRoleName), PolarisEntityType.CATALOG_ROLE),
+ catalogRoleName);
+ ResolverStatus status = resolutionManifest.resolveAll();
+ if (status.getStatus() == ResolverStatus.StatusEnum.ENTITY_COULD_NOT_BE_RESOLVED) {
Review Comment:
Thanks for the suggestion! I agree there are lots of chances here to reduce code duplication. Since the refactor may impact many methods, how about we doing that in a follow-up? I've opened a draft to attempt a merging for catalog/namespace/table-like: https://github.com/apache/polaris/pull/1621. We can add refactor policy ones after this PR gets in.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org