sunyuhan1998 commented on PR #10895: URL: https://github.com/apache/gravitino/pull/10895#issuecomment-4412139877
Thanks @yuqi1129 for the review! Replying to your questions: --- **`CredentialPropertyUtils.java:55`** — Those values are only used for Lance. Is it more proper to move them to the lance module? These constants and the `lanceCredentialPropertyMap` / `toLanceProperties()` are indeed Lance-specific, but `CredentialPropertyUtils` follows a centralized pattern where all table format credential transformations live in one class. The Iceberg equivalents (`ICEBERG_S3_ACCESS_KEY_ID`, `toIcebergProperties()`, etc.) are in the same class. Keeping them together seems consistent with the existing pattern. Would it be acceptable to keep it as is, or do you think moving them to the lance module would be better? --- **`LanceTableOperations.java:100`** — So, by default, we will get the credentials in the storage options? No, credentials are only injected when the client explicitly sets `vendCredentials = true` in the request. When `vendCredentials` is not set or is `false`, `credentialPrivilege` remains `null`, and the code falls into the `else` branch which returns only the original storage options without any credentials. See the `if (credentialPrivilege != null)` check in `GravitinoLanceTableOperations.describeTable()`. --- **`LanceTableOperations.java:117`** — Why is it `CredentialPrivilege.READ` when the length of `parts` is not 3? The `parts` should always have length 3 (catalog/schema/table) in normal flow. When it doesn't, using `READ` is a conservative safe default — it's better to under-provision than over-provision credentials. That said, this branch should never be reached in practice since upstream validation already enforces the 3-level structure. Would it be better to throw an explicit exception here instead? --- **`LanceTableOperations.java:126`** — Is this to check whether the current user has the write privileges? How can we get the current user? Yes, `MetadataAuthzHelper.checkPermission()` checks whether the current authenticated user has write privilege on the table. The current user is obtained from the Gravitino authentication context — when the request arrives, the identity is already established and bound to the thread context. During credential generation, `CatalogCredentialManager` calls `PrincipalUtils.getCurrentUserName()` to retrieve the authenticated username and generates credentials scoped to that user's permissions. -- 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]
