-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update privilege model to support granting CatalogRole access to PrincipalRoles #361
Update privilege model to support granting CatalogRole access to PrincipalRoles #361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM; I agree the ability to grant a privilege to a grantee in other systems usually only requires the grant-management-equivalent privilege on the securable being granted rather than requiring both ends, whereas revoke is a more privileged action with regards to the grantee so it makes sense to keep both ends for revoke.
Some suggestions on test comments - all the RBAC stuff is hard for newcomers to learn, so if they read the test cases to better understand, we could be more explicit about "why" a given grant is able to happen, especially since the privileges of catalog_admins are kind of hidden in the original setup of catalog_admins outside of the test case itself.
PrincipalRole principalRole2 = new PrincipalRole(principalRoleName2); | ||
createPrincipalRole(principalRole2); | ||
|
||
// create a catalog role and grant it manage_content privilege |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)"
catalogAdminToken, | ||
Response.Status.CREATED); | ||
|
||
// The catalog admin can grant the new catalog role to the mypr2 principal role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
public void testCatalogAdminGrantAndRevokeCatalogRoles() { | ||
// Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal | ||
// role | ||
String principalRoleName = "mypr33"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...ervice/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java
Show resolved
Hide resolved
...ervice/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java
Show resolved
Hide resolved
...ervice/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java
Show resolved
Hide resolved
...ervice/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java
Show resolved
Hide resolved
new AwsStorageConfigInfo( | ||
"arn:aws:iam::012345678901:role/jdoe", StorageConfigInfo.StorageTypeEnum.S3)) | ||
.setProperties(new CatalogProperties("s3://bucket1/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If we're not actually using S3, I think we may as well use FILE
...ervice/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some non-blocking comments on the tests
Fix consistency of ASSIGN_PRINCIPAL_ROLE in line with apache#361 Test cases for: assignPrincipalRoleToPrincipal revokePrincipalRoleFromPrincipal assignCatalogRoleToPrincipalRole revokeCatalogRoleFromPrincipalRole grantPrivilegeOnRootContainerToPrincipalRole revokePrivilegeOnRootContainerFromPrincipalRole grantPrivilegeOnCatalogToRole revokePrivilegeOnCatalogFromRole grantPrivilegeOnNamespaceToRole revokePrivilegeOnNamespaceFromRole grantPrivilegeOnTableToRole revokePrivilegeOnTableFromRole grantPrivilegeOnViewToRole revokePrivilegeOnViewFromRole
…ns (#364) Fix consistency of ASSIGN_PRINCIPAL_ROLE in line with #361 Test cases for: assignPrincipalRoleToPrincipal revokePrincipalRoleFromPrincipal assignCatalogRoleToPrincipalRole revokeCatalogRoleFromPrincipalRole grantPrivilegeOnRootContainerToPrincipalRole revokePrivilegeOnRootContainerFromPrincipalRole grantPrivilegeOnCatalogToRole revokePrivilegeOnCatalogFromRole grantPrivilegeOnNamespaceToRole revokePrivilegeOnNamespaceFromRole grantPrivilegeOnTableToRole revokePrivilegeOnTableFromRole grantPrivilegeOnViewToRole revokePrivilegeOnViewFromRole
Description
This updates the authorization model to allow
catalog_admin
roles to grant other CatalogRoles within their catalog to a PrincipalRole. This addresses #359 , in which it is stated that neither thecatalog_admin
or theservice_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 theASSIGN_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, aservice_admin
may delegatecatalog_admin
to another user/role, but may wish to retaincatalog_admin
privileges. Without requiringPRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE
, acatalog_admin
could revoke grants from any PrincipalRole, even if the grants weren't originally created by thecatalog_admin
itself.For consistency, I also removed the corresponding
*_MANAGE_GRANTS_FOR_GRANTEE
requirements for otherASSIGN*
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 havingCATALOG_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 theASSIGN_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 grantCREATE_TABLE
privilege to a catalog role in another catalog).Fixes #359
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added tests to confirm that a
service_admin
can grantcatalog_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 theCATALOG_MANAGE_ACCESSS
privilege can create grants on CatalogRoles within their own catalog, but across catalogs.Test Configuration:
Checklist:
Please delete options that are not relevant.