bharos commented on PR #10276:
URL: https://github.com/apache/gravitino/pull/10276#issuecomment-4015532617
> **Code Review — Major Issues, Do Not Merge**
>
> This is a security-model change and needs more work before it can be
merged.
>
> ### 1. PR description is unfilled
> The "What changes were proposed", "Why are the changes needed",
"User-facing change", and "How was this patch tested" sections all contain only
default template text. Please fill them in per the project PR guidelines.
>
> ### 2. No unit tests for the new authorization logic
> The `hasMetadataPrivilegePermission` traversal loop in `JcasbinAuthorizer`
is security-critical and has no test coverage. Please add tests covering at
least:
>
> * Grant at METALAKE level → covers all children (existing behavior)
> * Grant at CATALOG level → covers TABLE/SCHEMA within that catalog
> * Grant at SCHEMA level → covers TABLE within that schema
> * No grant at any level → returns false
> * `hasSetOwnerPermission` fallback is exercised correctly
>
> ### 3. Potential parse issue with `fullName.split("\\.", -1)`
> Object names in Gravitino can theoretically contain dots (e.g., in some
JDBC catalogs). `fullName.split("\\.", -1)` will incorrectly split such names.
Please verify that `fullName` passed to this method is always a dot-separated
path of simple names, or use a more robust parsing approach (e.g.,
`MetadataObjects.parse`).
>
> ### 4. `MetadataObject.Type.valueOf(type.toUpperCase())` — unchecked
conversion
> If `type` is an unexpected string, this throws `IllegalArgumentException`
with no context. Consider wrapping with a meaningful error message.
>
> ### 5. `GRANT_SUPPORTED_TYPES` naming
> Consider naming it `MANAGE_GRANTS_SUPPORTED_TYPES` to make it
self-documenting (the existing set is named `FILESET_SUPPORTED_TYPES` etc.
which are privilege-specific).
>
> Please address the above and add tests before requesting re-review.
Addressed the comments
--
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]
