yuqi1129 commented on PR #12573:
URL: https://github.com/apache/gravitino/pull/12573#issuecomment-5437354372
> Thanks for adding the Lance namespace-read authorization framework. The
scoped behavior looks reasonable, especially filtering before pagination and
keeping `describe` stricter than the existence probe.
>
> Before the follow-up table authorization PRs build on this foundation,
could we align the expression framework more closely with the Iceberg
authorization design?
>
> 1. Please consider avoiding a second expression abstraction
(`LanceAuthorizationExpression`) and a parallel authorization pipeline in
`LanceMetadataAuthorizationMethodInterceptor`. The new interceptor repeats user
validation, active-role validation, request-context handling, expression
evaluation, and denial/error handling from the Iceberg/common flow. This means
later fixes and features such as secondary expressions, existence checks, and
audit behavior can drift between Iceberg and Lance. A preferred direction would
be to extract the Iceberg base interceptor into a protocol-neutral pipeline,
with Iceberg and Lance providing only target resolution and protocol-specific
error mapping.
> 2. Lance dynamic namespace IDs can be resolved once into `(identifiers,
targetEntityType)`. The existing `CAN_ACCESS_METADATA` expression already
dispatches to the catalog/schema/table load expression based on `entityType`,
so this may allow Lance endpoints to keep using the shared
`AuthorizationExpression` model instead of declaring one expression per
namespace level. The root namespace could resolve explicitly to `METALAKE`; a
missing target annotation should be treated as a configuration error rather
than being conflated with root through `orElse("")`.
> 3. Please move `SCHEMA_NAMESPACE_EXISTS` into
`AuthorizationExpressionConstants` as a semantic shared constant, for example
`SCHEMA_EXISTS_AUTHORIZATION_EXPRESSION` or
`PROBE_SCHEMA_AUTHORIZATION_EXPRESSION`, and reuse it from both Iceberg and
Lance. Copying the current Iceberg inline string does not protect the two
protocols from future privilege or deny-rule drift. Lance `describeNamespace`
can still use the stricter `LOAD_SCHEMA_AUTHORIZATION_EXPRESSION`.
> 4. Please add expression-parity tests using the real expressions. The
current interceptor unit tests substitute `catalog-expression` /
`schema-expression`, while the integration test does not cover the
`CREATE_SCHEMA` probe-only path or deny precedence. At minimum, it would be
useful to verify that `CREATE_SCHEMA` permits `exists` but not `describe`, and
that catalog/schema deny rules override allows consistently with Iceberg.
>
> The main concern is not the current namespace-read result, but making this
PR a shared authorization foundation before table/create/mutation support
expands the Lance-specific model.
All fixed and polished again.
--
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]
