Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
github-actions[bot] closed pull request #3852: Allow CatalogAdmin to list Principal Roles URL: https://github.com/apache/polaris/pull/3852 -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
github-actions[bot] commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-4414220718 This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r3045216801 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2331,4 +2400,336 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
collado-mike commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r3017562874 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2331,4 +2400,336 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r3004075314 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2335,4 +2354,195 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r3004072376
##
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:
renamed ro `principal_role_viewer`
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2991169225
##
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:
+1 to `principal_role_viewer`
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
flyrain commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2985045392
##
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:
The name `catalog_role_manager` feels confusing to me. It is not really
about managing catalog roles. In this PR, it is only granted because of
`catalog_admin`, but the role itself is about listing principal roles, and the
same capability could be reused in other contexts that have nothing to do with
catalog roles.
A name like `principal_role_viewer` or `principal_role_reader` seems more
accurate and easier to understand. cc @singhpk234 @collado-mike
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
flyrain commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2985045392
##
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:
The name `catalog_role_manager` feels confusing to me. It is not really
about managing catalog roles. In this PR, it is only granted because of
`catalog_admin`, but the role itself is about listing principal roles, and the
same capability could be reused in other contexts that have nothing to do with
catalog roles.
A name like `principal_role_viewer` or `principal_role_reader` seems more
accurate and easier to understand.
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
flyrain commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2984450343 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2335,4 +2354,195 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2983752415 ## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java: ## @@ -104,6 +104,23 @@ public static PrincipalSecretsResult createPolarisPrincipalForRealm( rootContainer, PolarisPrivilege.SERVICE_MANAGE_ACCESS); +// create the catalog_role_manager principal role for catalog admins to list principal roles +PrincipalRoleEntity catalogRoleManagerPrincipalRole = +new PrincipalRoleEntity.Builder() +.setId(generateId(metaStoreManager, ctx)) + .setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()) +.setCreateTimestamp(System.currentTimeMillis()) +.build(); +metaStoreManager.createEntityIfNotExists(ctx, null, catalogRoleManagerPrincipalRole); + +// grant PRINCIPAL_ROLE_LIST on the rootContainer to the catalogRoleManagerPrincipalRole +metaStoreManager.grantPrivilegeOnSecurableToRole( +ctx, +catalogRoleManagerPrincipalRole, +null, +rootContainer, +PolarisPrivilege.PRINCIPAL_ROLE_LIST); Review Comment: My rationale: We should name the role according to the "job" its members perform. `PRINCIPAL_ROLE_LIST` allows listing "principal" roles, in a way managing "principal" roles, therefore I think `principal_role_manager` fits this use case. `principal_role_reader` might be more precise. -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2983728196 ## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java: ## @@ -104,6 +104,23 @@ public static PrincipalSecretsResult createPolarisPrincipalForRealm( rootContainer, PolarisPrivilege.SERVICE_MANAGE_ACCESS); +// create the catalog_role_manager principal role for catalog admins to list principal roles +PrincipalRoleEntity catalogRoleManagerPrincipalRole = +new PrincipalRoleEntity.Builder() +.setId(generateId(metaStoreManager, ctx)) + .setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()) +.setCreateTimestamp(System.currentTimeMillis()) +.build(); +metaStoreManager.createEntityIfNotExists(ctx, null, catalogRoleManagerPrincipalRole); + +// grant PRINCIPAL_ROLE_LIST on the rootContainer to the catalogRoleManagerPrincipalRole +metaStoreManager.grantPrivilegeOnSecurableToRole( +ctx, +catalogRoleManagerPrincipalRole, +null, +rootContainer, +PolarisPrivilege.PRINCIPAL_ROLE_LIST); Review Comment: We are creating a principal role which as `PRINCIPAL_ROLE_LIST` and assigning it to all principals which have `catalog_admin` catalog role, in a way we are managing a catalog role(catalog_admin) by adding a new principal role to provide access to principals - based on this I had named it catalog_role_manager. Based on this functionality WDYT would be the right name for this? I can update it accordingly -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2983720613 ## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java: ## @@ -104,6 +104,23 @@ public static PrincipalSecretsResult createPolarisPrincipalForRealm( rootContainer, PolarisPrivilege.SERVICE_MANAGE_ACCESS); +// create the catalog_role_manager principal role for catalog admins to list principal roles +PrincipalRoleEntity catalogRoleManagerPrincipalRole = +new PrincipalRoleEntity.Builder() +.setId(generateId(metaStoreManager, ctx)) + .setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()) +.setCreateTimestamp(System.currentTimeMillis()) +.build(); +metaStoreManager.createEntityIfNotExists(ctx, null, catalogRoleManagerPrincipalRole); Review Comment: from my POV, the new role introduced in this PR makes sense. I'm just not sure about automatically granting it to every user with a `catalog_admin` role. My rationale is here: https://github.com/apache/polaris/pull/3852#discussion_r2983661300 -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2983709672 ## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java: ## @@ -104,6 +104,23 @@ public static PrincipalSecretsResult createPolarisPrincipalForRealm( rootContainer, PolarisPrivilege.SERVICE_MANAGE_ACCESS); +// create the catalog_role_manager principal role for catalog admins to list principal roles +PrincipalRoleEntity catalogRoleManagerPrincipalRole = +new PrincipalRoleEntity.Builder() +.setId(generateId(metaStoreManager, ctx)) + .setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()) +.setCreateTimestamp(System.currentTimeMillis()) +.build(); +metaStoreManager.createEntityIfNotExists(ctx, null, catalogRoleManagerPrincipalRole); Review Comment: Well, #363 was reported by @collado-mike , but @collado-mike is not active on this PR, unfortunately :shrug: -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2983689403 ## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java: ## @@ -104,6 +104,23 @@ public static PrincipalSecretsResult createPolarisPrincipalForRealm( rootContainer, PolarisPrivilege.SERVICE_MANAGE_ACCESS); +// create the catalog_role_manager principal role for catalog admins to list principal roles +PrincipalRoleEntity catalogRoleManagerPrincipalRole = +new PrincipalRoleEntity.Builder() +.setId(generateId(metaStoreManager, ctx)) + .setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()) +.setCreateTimestamp(System.currentTimeMillis()) +.build(); +metaStoreManager.createEntityIfNotExists(ctx, null, catalogRoleManagerPrincipalRole); Review Comment: Yes, this is possible via CLI for a service_admin to create a new principal role with PRINCIPAL_ROLE_LIST privilege and manually assign it to all principals having the catalog_admin catalog role (and revoke it when principals no longer have catalog_admin). However, this manual process can be error-prone and operationally cumbersome. This feature automates the above mentioned grant and revoke process as described in https://github.com/apache/polaris/issues/363. After https://github.com/apache/polaris/pull/361 enabled catalog_admin to grant catalog roles to principal roles, issue #363 was that catalog_admin still cannot list available principal roles, making it difficult to know which roles to grant. This PR solves that problem through automatic role management. -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2983661300
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1569,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:
The role named `catalog_admin` may exist in every catalog, but conceptually
it is scoped down to each catalog individually. However, Principals are global
to the realm. So automatically exposing principal role listing to every catalog
admin sounds like an overreach to me... 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2983646310
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1569,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:
TIL - thx for the info!
I'm still a bit uncomfortable about the automatic granting part... Is that
critical?
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2983617650
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1569,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:
As per my understanding this is not possible as `catalog_admin` is a catalog
scoped role that cannot hold root-scoped privilege `PRINCIPAL_ROLE_LIST`. That
is the reason we need a principal role (catalog_role_manager) which can have
`PRINCIPAL_ROLE_LIST` privilege, and this principal role is automatically
granted to principals who have catalog_admin.
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2976515674
##
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java:
##
@@ -104,6 +104,23 @@ public static PrincipalSecretsResult
createPolarisPrincipalForRealm(
rootContainer,
PolarisPrivilege.SERVICE_MANAGE_ACCESS);
+// create the catalog_role_manager principal role for catalog admins to
list principal roles
+PrincipalRoleEntity catalogRoleManagerPrincipalRole =
+new PrincipalRoleEntity.Builder()
+.setId(generateId(metaStoreManager, ctx))
+
.setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole())
+.setCreateTimestamp(System.currentTimeMillis())
+.build();
+metaStoreManager.createEntityIfNotExists(ctx, null,
catalogRoleManagerPrincipalRole);
+
+// grant PRINCIPAL_ROLE_LIST on the rootContainer to the
catalogRoleManagerPrincipalRole
+metaStoreManager.grantPrivilegeOnSecurableToRole(
+ctx,
+catalogRoleManagerPrincipalRole,
+null,
+rootContainer,
+PolarisPrivilege.PRINCIPAL_ROLE_LIST);
Review Comment:
We grant a "principal" privilege to a "catalog admin" role. This looks a bit
out of line with the general distinction of principals (which are global to the
realm) and catalog-specific roles (scoped under a catalog). Should the new role
be named "principal role manager" perhaps?
##
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java:
##
@@ -104,6 +104,23 @@ public static PrincipalSecretsResult
createPolarisPrincipalForRealm(
rootContainer,
PolarisPrivilege.SERVICE_MANAGE_ACCESS);
+// create the catalog_role_manager principal role for catalog admins to
list principal roles
+PrincipalRoleEntity catalogRoleManagerPrincipalRole =
+new PrincipalRoleEntity.Builder()
+.setId(generateId(metaStoreManager, ctx))
+
.setName(PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole())
+.setCreateTimestamp(System.currentTimeMillis())
+.build();
+metaStoreManager.createEntityIfNotExists(ctx, null,
catalogRoleManagerPrincipalRole);
Review Comment:
As an alternative to adding a new default role in the server code, WDYT
about using the new `setup` CLI (#3929) command instead?
This way, the server does not have to manage a lot of default roles, and
admin users can still have convenient methods of managing new grants.
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1569,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:
Why not grant `PRINCIPAL_ROLE_LIST` to `catalog_admin` directly?
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-4112324214 @vignesh-manel : please add a CHANGELOG entry for this since it affects Polaris administration. -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-4111377070 Gentle Reminder for PR review -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2913107683
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1531,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:
updated to handle this case 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
flyingImer commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2908498163
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -1518,8 +1531,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:
IIUC, the current fix covers the assign-then-grant order, but I think the
reverse order still looks open: if a principal role already holds
`catalog_admin` on a catalog and a principal is added later via
`assignPrincipalRole`, the new principal does not seem to inherit
`catalog_role_manager`. Would it make sense to handle that mirror case as well,
or add a test showing why the current lifecycle is still safe?
--
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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-4024922072 @dennishuo @collado-mike : 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852:
URL: https://github.com/apache/polaris/pull/3852#discussion_r2899514660
##
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:
made it similar to service_admin
##
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:
good catch. made relevant changes to cleanup on catalog drop and also to
revoke catalog_role_manager when principal loses a principal role
##
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##
@@ -2335,4 +2354,195 @@ private static PolarisEntitySubType
selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on code in PR #3852: URL: https://github.com/apache/polaris/pull/3852#discussion_r2899514822 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2335,4 +2354,195 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
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
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-3954532480 @vignesh-manel : just heads up: I'm willing to facilitate the progress of this PR, but I do not have enough context for the internal Polaris RBAC system and its applications for an approval. I hope @dennishuo, @collado-mike, @flyrain could provide feedback on the actual behaviour changes. -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-3954519588 `dev` thread for reference: https://lists.apache.org/thread/ws0blghsv8jl9rbwpgfgcbzjs7d38242 -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-3954517460 @vignesh-manel : no worries, let's continue on this PR :+1: -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-3948582308 > @vignesh-manel : please avoid opening multiple PRs for the same feature (cf. https://polaris.apache.org/community/contributing-guidelines/) > > You should be able to take the commits from this PR, put them into your local `vignesh-manel:main` branch and force-push into #3761 > > Once that PR is merged, you'll be able to reuse `main` for a different purpose locally :wink: @dimas-b I had tried that, I had forced push the same commits to the main branch of my fork, even though they showed up in the fork, GitHub wasn't giving me the option to reopen the old PR. Seems to be an issue with GitHub https://github.com/isaacs/github/issues/361 hence I created this new PR -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3852: URL: https://github.com/apache/polaris/pull/3852#issuecomment-3947857164 @vignesh-manel : please avoid opening multiple PRs for the same feature (cf. https://polaris.apache.org/community/contributing-guidelines/) You should be able to take the commits from this PR, put them into your local `vignesh-manel:main` branch and force-push into #3761 Once that PR is merged, you'll be able to reuse `main` for a different purpose locally :wink: -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on PR #3761: URL: https://github.com/apache/polaris/pull/3761#issuecomment-3934162507 This PR got closed accidentally while rebasing branches, and unable to reopen it. So created a new PR https://github.com/apache/polaris/pull/3852 Apologies for the inconvenience -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel closed pull request #3761: Allow CatalogAdmin to list Principal Roles URL: https://github.com/apache/polaris/pull/3761 -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3761: URL: https://github.com/apache/polaris/pull/3761#issuecomment-3921865896 Thanks, @vignesh-manel ! `dev` thread for reference: https://lists.apache.org/thread/ws0blghsv8jl9rbwpgfgcbzjs7d38242 -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
vignesh-manel commented on PR #3761: URL: https://github.com/apache/polaris/pull/3761#issuecomment-3921796813 > Thanks for you contribution, @vignesh-manel ! > > Given that this is a major change to the Polaris RBAC system, I believe it deserves a discussion on the `dev` [ML](https://polaris.apache.org/community/). Would you mind starting it there? Thx! @dimas-b I have sent a mail to dev ML. Thanks -- 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]
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
flyingImer commented on code in PR #3761: URL: https://github.com/apache/polaris/pull/3761#discussion_r2819453931 ## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java: ## @@ -2335,4 +2354,195 @@ private static PolarisEntitySubType selectEntitySubType(List
Re: [PR] Allow CatalogAdmin to list Principal Roles [polaris]
dimas-b commented on PR #3761: URL: https://github.com/apache/polaris/pull/3761#issuecomment-3916788753 Thanks for you contribution, @vignesh-manel ! Given that this is a major change to the Polaris RBAC system, I believe it deserves a discussion on the `dev` [ML](https://polaris.apache.org/community/). Would you mind starting it there? Thx! -- 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]
