Allow CatalogAdmin to list Principal Roles#3852
Allow CatalogAdmin to list Principal Roles#3852vignesh-manel wants to merge 2 commits intoapache:mainfrom
Conversation
vignesh-manel
changed the title
|
@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 Once that PR is merged, you'll be able to reuse |
@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 isaacs/github#361 hence I created this new PR |
|
@vignesh-manel : no worries, let's continue on this PR 👍 |
|
|
|
@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. |
| 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"; |
There was a problem hiding this comment.
should it be protected from being droppable?
| // if revoking catalog_admin, check if principal still has catalog_admin on other catalogs | ||
| if (result.isSuccess() | ||
| && PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) { | ||
| revokeCatalogRoleManagerIfNeeded(principalRoleEntity); |
There was a problem hiding this 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
There was a problem hiding this comment.
Similarly, when catalog drops, it should be cleaned up
There was a problem hiding this comment.
we should have tests capturing them
| PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()); | ||
|
|
||
| if (!catalogRoleManagerResult.isSuccess() || catalogRoleManagerResult.getEntity() == null) { | ||
| return; |
There was a problem hiding this 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
| PolarisEntityConstants.getNameOfCatalogRoleManagerPrincipalRole()); | ||
|
|
||
| if (!catalogRoleManagerResult.isSuccess() || catalogRoleManagerResult.getEntity() == null) { | ||
| return; |
There was a problem hiding this comment.
2 cents: log something when nothing found
| // if granting catalog_admin, also grant catalog_role_manager to allow listing principal roles | ||
| if (result.isSuccess() | ||
| && PolarisEntityConstants.getNameOfCatalogAdminRole().equals(catalogRoleName)) { | ||
| grantCatalogRoleManagerIfNeeded(principalRoleEntity); |
There was a problem hiding this comment.
catalog role manager should be granted on catalog_admin grant to a role as well, i.e., inside assignPrincipalRole
3 participants
Duplicate of #3761 which got closed accidentally while rebasing
Implements automatic principal role listing for
catalog_adminusers via a new system-managedcatalog_role_managerrole. Fixes #363Implementation
catalog_role_managercreated at bootstrap withPRINCIPAL_ROLE_LISTprivilege (read-only)catalog_adminon any catalogcatalog_admingrants are removedLimitations
catalog_admin. If assigned after, revoke and re-grantcatalog_adminto trigger auto-grant.catalog_admingrants (requires manual grant or re-grant)CC: @collado-mike
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)