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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
