flyingImer commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2849916099
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityConstants.java:
##########
@@ -43,6 +43,9 @@ public class PolarisEntityConstants {
// the name of the principal role we create to manage the entire Polaris
service
private static final String ADMIN_PRINCIPAL_ROLE_NAME = "service_admin";
+ // the name of the principal role for catalog admins to list principal roles
+ private static final String CATALOG_ROLE_MANAGER_PRINCIPAL_ROLE_NAME =
"catalog_role_manager";
Review Comment:
should it be protected from being droppable?
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -2335,4 +2354,195 @@ private static PolarisEntitySubType
selectEntitySubType(List<PolarisEntitySubTyp
subTypes));
}
}
+
+ /**
+ * Grants the catalog_role_manager principal role to all principals assigned
to the specified
+ * principal role. This allows catalog admins to list principal roles.
+ */
+ private void grantCatalogRoleManagerIfNeeded(PrincipalRoleEntity
principalRoleEntity) {
+ // Load catalog_role_manager directly from metastore
+ EntityResult catalogRoleManagerResult =
+ metaStoreManager.readEntityByName(
+ getCurrentPolarisContext(),
+ null,
+ PolarisEntityType.PRINCIPAL_ROLE,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole());
+
+ if (!catalogRoleManagerResult.isSuccess() ||
catalogRoleManagerResult.getEntity() == null) {
+ return;
Review Comment:
2 cents: log something when nothing found. This should be helpful to capture
the missing catalog_role_manager role, as currently it is created only during
bootstrap IIUC, i.e., we don't have upgrade/backfill path for already
bootstrapped realms. I'm fine with temporary gaps for upgrade and backfill, but
strongly recommend at least to warn on role not found
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1534,8 +1544,17 @@ public PrivilegeResult
revokeCatalogRoleFromPrincipalRole(
getPrincipalRoleByName(resolutionManifest, principalRoleName);
CatalogEntity catalogEntity = getCatalogByName(resolutionManifest,
catalogName);
CatalogRoleEntity catalogRoleEntity =
getCatalogRoleByName(resolutionManifest, catalogRoleName);
- return metaStoreManager.revokeUsageOnRoleFromGrantee(
- getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+ PrivilegeResult result =
+ metaStoreManager.revokeUsageOnRoleFromGrantee(
+ getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+
+ // if revoking catalog_admin, check if principal still has catalog_admin
on other catalogs
+ if (result.isSuccess()
+ &&
PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) {
+ revokeCatalogRoleManagerIfNeeded(principalRoleEntity);
Review Comment:
I see the `catalog_role_manager` is revoked only via
`revokeCatalogRoleFromPrincipalRole`, but `catalog_role_manager` is granted to
principal (not principal roles). Say when a principal is unassigned from a
principal role (i.e., revoke principal role from principal API),
`catalog_role_manager` granted to the principal would be never revoked.
I feel it should be cleaned up as well to avoid leaking PRINCIPAL_ROLE_LIST
privilege
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -2335,4 +2354,195 @@ private static PolarisEntitySubType
selectEntitySubType(List<PolarisEntitySubTyp
subTypes));
}
}
+
+ /**
+ * Grants the catalog_role_manager principal role to all principals assigned
to the specified
+ * principal role. This allows catalog admins to list principal roles.
+ */
+ private void grantCatalogRoleManagerIfNeeded(PrincipalRoleEntity
principalRoleEntity) {
+ // Load catalog_role_manager directly from metastore
+ EntityResult catalogRoleManagerResult =
+ metaStoreManager.readEntityByName(
+ getCurrentPolarisContext(),
+ null,
+ PolarisEntityType.PRINCIPAL_ROLE,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole());
+
+ if (!catalogRoleManagerResult.isSuccess() ||
catalogRoleManagerResult.getEntity() == null) {
+ return;
+ }
+
+ PrincipalRoleEntity catalogRoleManagerEntity =
+ PrincipalRoleEntity.of(catalogRoleManagerResult.getEntity());
+
+ // Find all principals that have this principal role and grant
catalog_role_manager to them
+ LoadGrantsResult grantsResult =
+ metaStoreManager.loadGrantsOnSecurable(getCurrentPolarisContext(),
principalRoleEntity);
+
+ if (grantsResult.isSuccess()) {
+ for (PolarisGrantRecord grant : grantsResult.getGrantRecords()) {
+ // Check if this is a PRINCIPAL_ROLE_USAGE grant (principal using this
role)
+ if (grant.getPrivilegeCode() ==
PolarisPrivilege.PRINCIPAL_ROLE_USAGE.getCode()) {
+ // Load the principal (grantee)
+ EntityResult principalResult =
+ metaStoreManager.loadEntity(
+ getCurrentPolarisContext(),
+ grant.getGranteeCatalogId(),
+ grant.getGranteeId(),
+ PolarisEntityType.PRINCIPAL);
+
+ if (principalResult.isSuccess() && principalResult.getEntity() !=
null) {
+ PrincipalEntity principal =
PrincipalEntity.of(principalResult.getEntity());
+
+ // Check if the principal already has catalog_role_manager
+ LoadGrantsResult principalGrantsResult =
+
metaStoreManager.loadGrantsToGrantee(getCurrentPolarisContext(), principal);
+
+ boolean alreadyHasCatalogRoleManager = false;
+ if (principalGrantsResult.isSuccess()) {
+ for (PolarisGrantRecord existingGrant :
principalGrantsResult.getGrantRecords()) {
+ if (existingGrant.getSecurableId() ==
catalogRoleManagerEntity.getId()
+ && existingGrant.getPrivilegeCode()
+ == PolarisPrivilege.PRINCIPAL_ROLE_USAGE.getCode()) {
+ alreadyHasCatalogRoleManager = true;
+ break;
+ }
+ }
+ }
+
+ // Only grant if not already granted
+ if (!alreadyHasCatalogRoleManager) {
+ metaStoreManager.grantUsageOnRoleToGrantee(
+ getCurrentPolarisContext(), null, catalogRoleManagerEntity,
principal);
+ }
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * Revokes the catalog_role_manager principal role from all principals
assigned to the specified
+ * principal role if they no longer have any catalog_admin grants.
+ */
+ private void revokeCatalogRoleManagerIfNeeded(PrincipalRoleEntity
principalRoleEntity) {
+ // Load catalog_role_manager directly from metastore
+ EntityResult catalogRoleManagerResult =
+ metaStoreManager.readEntityByName(
+ getCurrentPolarisContext(),
+ null,
+ PolarisEntityType.PRINCIPAL_ROLE,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole());
+
+ if (!catalogRoleManagerResult.isSuccess() ||
catalogRoleManagerResult.getEntity() == null) {
+ return;
Review Comment:
2 cents: log something when nothing found
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1518,8 +1519,17 @@ public PrivilegeResult assignCatalogRoleToPrincipalRole(
CatalogEntity catalogEntity = getCatalogByName(resolutionManifest,
catalogName);
CatalogRoleEntity catalogRoleEntity =
getCatalogRoleByName(resolutionManifest, catalogRoleName);
- return metaStoreManager.grantUsageOnRoleToGrantee(
- getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+ PrivilegeResult result =
+ metaStoreManager.grantUsageOnRoleToGrantee(
+ getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+
+ // if granting catalog_admin, also grant catalog_role_manager to allow
listing principal roles
+ if (result.isSuccess()
+ &&
PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) {
+ grantCatalogRoleManagerIfNeeded(principalRoleEntity);
Review Comment:
catalog role manager should be granted on catalog_admin grant to a role as
well, i.e., inside `assignPrincipalRole`
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1534,8 +1544,17 @@ public PrivilegeResult
revokeCatalogRoleFromPrincipalRole(
getPrincipalRoleByName(resolutionManifest, principalRoleName);
CatalogEntity catalogEntity = getCatalogByName(resolutionManifest,
catalogName);
CatalogRoleEntity catalogRoleEntity =
getCatalogRoleByName(resolutionManifest, catalogRoleName);
- return metaStoreManager.revokeUsageOnRoleFromGrantee(
- getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+ PrivilegeResult result =
+ metaStoreManager.revokeUsageOnRoleFromGrantee(
+ getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+
+ // if revoking catalog_admin, check if principal still has catalog_admin
on other catalogs
+ if (result.isSuccess()
+ &&
PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) {
+ revokeCatalogRoleManagerIfNeeded(principalRoleEntity);
Review Comment:
Similarly, when catalog drops, it should be cleaned up
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1534,8 +1544,17 @@ public PrivilegeResult
revokeCatalogRoleFromPrincipalRole(
getPrincipalRoleByName(resolutionManifest, principalRoleName);
CatalogEntity catalogEntity = getCatalogByName(resolutionManifest,
catalogName);
CatalogRoleEntity catalogRoleEntity =
getCatalogRoleByName(resolutionManifest, catalogRoleName);
- return metaStoreManager.revokeUsageOnRoleFromGrantee(
- getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+ PrivilegeResult result =
+ metaStoreManager.revokeUsageOnRoleFromGrantee(
+ getCurrentPolarisContext(), catalogEntity, catalogRoleEntity,
principalRoleEntity);
+
+ // if revoking catalog_admin, check if principal still has catalog_admin
on other catalogs
+ if (result.isSuccess()
+ &&
PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) {
+ revokeCatalogRoleManagerIfNeeded(principalRoleEntity);
Review Comment:
we should have tests capturing them
--
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]
