You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "collado-mike (via GitHub)" <gi...@apache.org> on 2024/10/09 00:10:19 UTC
[PR] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
collado-mike opened a new pull request, #361:
URL: https://github.com/apache/polaris/pull/361
# Description
This updates the authorization model to allow `catalog_admin` roles to grant other CatalogRoles within their catalog to a PrincipalRole. This addresses https://github.com/apache/polaris/issues/359 , in which it is stated that neither the `catalog_admin` or the `service_admin` roles alone are sufficient to grant catalog access to principal roles.
The fix is implemented by removing the requirement for `PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE` privilege from the `ASSIGN_CATALOG_ROLE_TO_PRINCIPAL_ROLE` operation. Implicitly, this states that a user only needs privileges to manage the grants on the catalog as a securable, but does not need privilege to manage the grants on a PrincipalRole as a grantee.
A caller _does_ require `PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE` to revoke a privilege, which, admittedly, does make this operation a little bit lopsided. However, having the ability to create grants does not implicitly mean the user has the privilege to revoke grants for the same securable. For example, a `service_admin` may delegate `catalog_admin` to another user/role, but may wish to retain `catalog_admin` privileges. Without requiring `PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE`, a `catalog_admin` could revoke grants from any PrincipalRole, even if the grants weren't originally created by the `catalog_admin` itself.
For consistency, I also removed the corresponding `*_MANAGE_GRANTS_FOR_GRANTEE` requirements for other `ASSIGN*` operations. So a user, having privilege to manage grants on a securable, can create a grant for that securable for any grantee without needing special privilege on that grantee. As an example, a user having `CATALOG_MANAGE_ACCESS` at a namespace level can create grants for any entity at that namespace or below and grant it to any CatalogRole in the catalog. The revoke authorization is also consistent with the `ASSIGN_CATALOG_ROLE_TO_PRINCIPAL_ROLE` operation.
I confirmed with tests that the `catalog_admin` and other roles cannot create grant records that cross catalogs (i.e., can't grant `CREATE_TABLE` privilege to a catalog role in another catalog).
Fixes #359
## Type of change
Please delete options that are not relevant.
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation update
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
# How Has This Been Tested?
Added tests to confirm that a `service_admin` can grant `catalog_admin` to another principal role and a principal with that role can create CatalogRoles and grant them to other PrincipalRoles. Also confirmed that callers that have the `CATALOG_MANAGE_ACCESSS` privilege can create grants on CatalogRoles within their own catalog, but across catalogs.
- [ ] Test A
- [ ] Test B
**Test Configuration**:
* Hardware:
* Toolchain:
* SDK:
# Checklist:
Please delete options that are not relevant.
- [ ] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike merged PR #361:
URL: https://github.com/apache/polaris/pull/361
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793833986
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role. Use the catalog admin principal to list principal roles and
+ // grant a catalog role to the new principal role
+ String principalRoleName2 = "mypr2";
Review Comment:
ditto
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793834727
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role. Use the catalog admin principal to list principal roles and
+ // grant a catalog role to the new principal role
+ String principalRoleName2 = "mypr2";
+ PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
+ createPrincipalRole(principalRole2);
+
+ // create a catalog role and grant it manage_content privilege
+ String catalogRoleName = "mycr1";
+ createCatalogRole(catalogName, catalogRoleName, catalogAdminToken);
+
+ CatalogPrivilege privilege = CatalogPrivilege.CATALOG_MANAGE_CONTENT;
+ grantPrivilegeToCatalogRole(
+ catalogName,
+ catalogRoleName,
+ new CatalogGrant(privilege, GrantResource.TypeEnum.CATALOG),
+ catalogAdminToken,
+ Response.Status.CREATED);
+
+ // The catalog admin can grant the new catalog role to the mypr2 principal role
+ grantCatalogRoleToPrincipalRole(
+ principalRoleName2, catalogName, new CatalogRole(catalogRoleName), catalogAdminToken);
+
+ // But the catalog admin cannot revoke the role because it requires
+ // PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName
+ + "/"
+ + catalogRoleName,
+ catalogAdminToken)
+ .delete()) {
+ assertThat(response).returns(Response.Status.FORBIDDEN.getStatusCode(), Response::getStatus);
+ }
+
+ // The service admin can revoke the role because it has the
+ // PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE privilege
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName
+ + "/"
+ + catalogRoleName,
+ userToken)
+ .delete()) {
+ assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
+ }
+ }
+
+ @Test
+ public void testServiceAdminCanTransferCatalogAdmin() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myothertestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // service_admin revokes the catalog_admin privilege from its principal role
+ try {
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/service_admin/catalog-roles/"
+ + catalogName
+ + "/catalog_admin",
+ userToken)
+ .delete()) {
+ assertThat(response)
+ .returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
+ }
+
+ // the service_admin can not revoke the catalog_admin privilege from the new principal role
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName
+ + "/catalog_admin",
+ catalogAdminToken)
+ .delete()) {
+ assertThat(response)
+ .returns(Response.Status.FORBIDDEN.getStatusCode(), Response::getStatus);
+ }
+ } finally {
+ // grant the admin role back to service_admin so that cleanup can happen
+ grantCatalogRoleToPrincipalRole(
+ "service_admin", catalogName, catalogAdminRole, catalogAdminToken);
+ }
+ }
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRolesFromWrongCatalog() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ // create a catalog
+ String catalogName = "mytestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ // create a second catalog
+ String catalogName2 = "anothercatalog";
+ Catalog catalog2 =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName2)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog2);
+
+ // create a catalog role *in the second catalog* and grant it manage_content privilege
+ String catalogRoleName = "mycr1";
+ createCatalogRole(catalogName2, catalogRoleName, userToken);
+
+ // Get the catalog admin role from the *first* catalog and grant that role to the principal role
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ // Create a principal and grant the principal role to it
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role.
+ String principalRoleName2 = "mypr2";
+ PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
+ createPrincipalRole(principalRole2);
+
+ // The catalog admin cannot grant the new catalog role to the mypr2 principal role because the
+ // catalog role is in the wrong catalog
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName2,
+ catalogAdminToken)
+ .put(Entity.json(new GrantCatalogRoleRequest(new CatalogRole(catalogRoleName))))) {
+ assertThat(response).returns(Response.Status.FORBIDDEN.getStatusCode(), Response::getStatus);
+ }
+ }
+
+ @Test
+ public void testTableManageAccessCanGrantAndRevokeFromCatalogRoles() {
+ // Create a PrincipalRole and a new catalog.
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ // create a catalog
+ String catalogName = "mytablemanagecatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ // create a valid target CatalogRole in this catalog
+ createCatalogRole(catalogName, "target_catalog_role", userToken);
+
+ // create a second catalog
+ String catalogName2 = "anothertablemanagecatalog";
Review Comment:
ditto
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793835495
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
Review Comment:
If we're not actually using S3, I think we may as well use `FILE`
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
Review Comment:
nit: If we're not actually using S3, I think we may as well use `FILE`
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793835138
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
Review Comment:
ditto
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793843389
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
Review Comment:
I actually did fix this more correctly by adding the logic to the teardown method. Previously things weren't getting deleted because the catalogs weren't empty, but now they're emptied before everything else gets deleted.
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1792663319
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role. Use the catalog admin principal to list principal roles and
+ // grant a catalog role to the new principal role
+ String principalRoleName2 = "mypr2";
+ PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
+ createPrincipalRole(principalRole2);
+
+ // create a catalog role and grant it manage_content privilege
Review Comment:
For people reading the test, we might want to expand this comment to make it clear that the new catalog role itself doesn't get any grant-management abilities from CATALOG_MANAGE_CONTENT, just so the reader knows that this grant here isn't related to whether or not the catalogAdmin can successfully grant this new catalogRole to `mypr2`.
Otherwise at first glance it's easy to read the test as line 1847 saying "the cataalog admin can grant the new catalog role... (because of the privilege grant we see right before the line?)"
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role. Use the catalog admin principal to list principal roles and
+ // grant a catalog role to the new principal role
+ String principalRoleName2 = "mypr2";
+ PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
+ createPrincipalRole(principalRole2);
+
+ // create a catalog role and grant it manage_content privilege
+ String catalogRoleName = "mycr1";
+ createCatalogRole(catalogName, catalogRoleName, catalogAdminToken);
+
+ CatalogPrivilege privilege = CatalogPrivilege.CATALOG_MANAGE_CONTENT;
+ grantPrivilegeToCatalogRole(
+ catalogName,
+ catalogRoleName,
+ new CatalogGrant(privilege, GrantResource.TypeEnum.CATALOG),
+ catalogAdminToken,
+ Response.Status.CREATED);
+
+ // The catalog admin can grant the new catalog role to the mypr2 principal role
Review Comment:
Would help to make this comment more explicit by adding `... because the catalog admin has CATALOG_MANAGE_ACCESS on the whole catalog, which transitively provides the required CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE on all the CatalogRoles in the catalog`.
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793836463
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(
+ new AwsStorageConfigInfo(
+ "arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ createCatalog(catalog);
+
+ CatalogRole catalogAdminRole = readCatalogRole(catalogName, "catalog_admin");
+ grantCatalogRoleToPrincipalRole(principalRoleName, catalogName, catalogAdminRole, userToken);
+
+ PrincipalWithCredentials catalogAdminPrincipal = createPrincipal("principal1");
+
+ grantPrincipalRoleToPrincipal(catalogAdminPrincipal.getPrincipal().getName(), principalRole1);
+
+ String catalogAdminToken =
+ TokenUtils.getTokenFromSecrets(
+ EXT.client(),
+ EXT.getLocalPort(),
+ catalogAdminPrincipal.getCredentials().getClientId(),
+ catalogAdminPrincipal.getCredentials().getClientSecret(),
+ realm);
+
+ // Create a second principal role. Use the catalog admin principal to list principal roles and
+ // grant a catalog role to the new principal role
+ String principalRoleName2 = "mypr2";
+ PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2);
+ createPrincipalRole(principalRole2);
+
+ // create a catalog role and grant it manage_content privilege
+ String catalogRoleName = "mycr1";
+ createCatalogRole(catalogName, catalogRoleName, catalogAdminToken);
+
+ CatalogPrivilege privilege = CatalogPrivilege.CATALOG_MANAGE_CONTENT;
+ grantPrivilegeToCatalogRole(
+ catalogName,
+ catalogRoleName,
+ new CatalogGrant(privilege, GrantResource.TypeEnum.CATALOG),
+ catalogAdminToken,
+ Response.Status.CREATED);
+
+ // The catalog admin can grant the new catalog role to the mypr2 principal role
+ grantCatalogRoleToPrincipalRole(
+ principalRoleName2, catalogName, new CatalogRole(catalogRoleName), catalogAdminToken);
+
+ // But the catalog admin cannot revoke the role because it requires
+ // PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName
+ + "/"
+ + catalogRoleName,
+ catalogAdminToken)
+ .delete()) {
+ assertThat(response).returns(Response.Status.FORBIDDEN.getStatusCode(), Response::getStatus);
+ }
+
+ // The service admin can revoke the role because it has the
+ // PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE privilege
+ try (Response response =
+ newRequest(
+ "http://localhost:%d/api/management/v1/principal-roles/"
+ + principalRoleName
+ + "/catalog-roles/"
+ + catalogName
+ + "/"
+ + catalogRoleName,
+ userToken)
+ .delete()) {
+ assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
+ }
+ }
+
+ @Test
+ public void testServiceAdminCanTransferCatalogAdmin() {
Review Comment:
The same comments on conflicting entity names apply here as well
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793833714
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
Review Comment:
Is this just to avoid conflicts across tests? If that's true, we might want to harden this against multiple implementations of `PolarisServiceImplIntegrationTest` running. Can we just randomize it until we have better isolation across tests?
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
+ PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+ createPrincipalRole(principalRole1);
+
+ String catalogName = "myuniquetestcatalog";
Review Comment:
Same comment as above
--
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] Update privilege model to support granting CatalogRole access to PrincipalRoles [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793849000
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
assertThat(response).returns(204, Response::getStatus);
}
}
+
+ @Test
+ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+ // Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
+ // role
+ String principalRoleName = "mypr33";
Review Comment:
That sounds really good for protecting us across sequential runs, but I think there are still potentially issues with concurrent tests. Anyway, all sorts of tests have this issue so it's not urgent to address here.
--
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