Skip to content

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

Merged
merged 4 commits into from
Apr 22, 2025

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Apr 22, 2025

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

@HonahX HonahX force-pushed the honahx_get_applicable_policies_auth branch from cd5700f to 19bfb40 Compare April 22, 2025 17:46
flyrain
flyrain previously approved these changes Apr 22, 2025
Copy link
Contributor

@flyrain flyrain left a 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.

targetCatalog,
null /* secondary */);

initializeCatalog();
Copy link
Contributor

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.

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,
Copy link
Contributor

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?

Copy link
Contributor Author

@HonahX HonahX Apr 22, 2025

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.

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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

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));

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Apr 22, 2025
@flyrain flyrain merged commit bb7bce4 into apache:main Apr 22, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants