-
Notifications
You must be signed in to change notification settings - Fork 224
Implement PolicyCatalogHandler Stage 3: GetApplicablePolicies #1421
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
Implement PolicyCatalogHandler Stage 3: GetApplicablePolicies #1421
Conversation
cd5700f
to
19bfb40
Compare
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 questions and minor comments.
...ice/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java
Outdated
Show resolved
Hide resolved
targetCatalog, | ||
null /* secondary */); | ||
|
||
initializeCatalog(); |
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.
Just for my information: Is this mainly for refreshing the object resolutionManifest
in the polarisCatalog
? I guess we could give a better name to something like reinitiatCatalog()
. Not a blocker though.
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.
This is the time when the PolicyCatalog is initialized. The high-level workflow is that for each rest request, we first initialize a PoicyCatalogHandler
and put entities into resolutionManifest
, resolve and do authorization, if the authorization passed, a PolicyCatalog
is initialized to handle the request. So PolicyCatalog
object is never re-used after it serves one request
public void testGetApplicablePoliciesOnNamespaceInSufficientPrivileges() { | ||
doTestInsufficientPrivileges( | ||
List.of( | ||
PolarisPrivilege.CATALOG_READ_PROPERTIES, |
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.
Q: Does CATALOG_READ_PROPERTIES imply NAMESPACE_READ_PROPERTIES?
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.
No, CATALOG_READ_PROPERTIES
is not a super privilege of NAMESPACE_READ_PROPERTIES
.
polaris/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java
Lines 209 to 216 in 5f2a375
SUPER_PRIVILEGES.putAll( | |
NAMESPACE_READ_PROPERTIES, | |
List.of( | |
CATALOG_MANAGE_CONTENT, | |
CATALOG_MANAGE_METADATA, | |
NAMESPACE_FULL_METADATA, | |
NAMESPACE_READ_PROPERTIES, | |
NAMESPACE_WRITE_PROPERTIES)); |
Just to confirm: This should be the expected that user have right to getApplicablePolicies on catalog does not necessarily have the right to getApplicablePolicies on namespace?
public void testGetApplicablePoliciesOnTableSufficientPrivileges() { | ||
doTestSufficientPrivileges( | ||
List.of( | ||
PolarisPrivilege.TABLE_READ_PROPERTIES, |
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.
Q: is there a namespace level read privilege can cascade to privilege TABLE_READ_PROPERTIES?
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.
No, only CATALOG_MANAGE_METADATA
which is a super privilege implies TABLE_READ_PROPERTIES
polaris/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java
Lines 217 to 226 in 5f2a375
SUPER_PRIVILEGES.putAll( | |
TABLE_READ_PROPERTIES, | |
List.of( | |
CATALOG_MANAGE_CONTENT, | |
CATALOG_MANAGE_METADATA, | |
TABLE_FULL_METADATA, | |
TABLE_READ_DATA, | |
TABLE_READ_PROPERTIES, | |
TABLE_WRITE_DATA, | |
TABLE_WRITE_PROPERTIES)); |
This is a follow-up of #1357, and in parallel to #1416. It adds privilege and auth methods for
getApplicablePolicies
The tests are in PolicyCatalogHandlerAuthzTest.java
cc: @flyrain