jerryshao commented on PR #13252: URL: https://github.com/apache/gravitino/pull/13252#issuecomment-5750534325
Verdict: ship it — no blocking issues found. ### Findings 1. `server-common/src/main/java/org/apache/gravitino/server/authorization/AuthorizationRequestScope.java:58,114` — `open()` overwrites `CURRENT` without saving the previous scope and `close()` unconditionally calls `CURRENT.remove()`, so the class is not nest-safe. This is latent rather than live: the only two openers are `GravitinoInterceptionService.java:166`, whose `getMethodInterceptors` at `:134` returns exactly one interceptor, and `BaseMetadataAuthorizationMethodInterceptor.java:226`, whose only subclasses are the Iceberg and Lance interceptors, each installed on its own resource methods — so no invocation can nest today. If an intercepted method ever calls another intercepted method, the inner `close()` silently unbinds the outer scope and reuse stops with no signal (authorization stays correct, since `getOrCreate` falls back to a fresh context). Save and restore the previous value in `open()`/`close()`, or assert `CURRENT.get() == null` in `open()`. (verified by: read ing both openers and `getMethodInterceptors`, and grepping every `AuthorizationRequestScope.` reference in the repo — those are the only non-test `open()` sites.) 2. `server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:302,506` — now that `getOrCreate` at `:396` can hand back the interceptor's context, `allVisibleViaParentScope` and `doFilter` overwrite `originalAuthorizationExpression` on it, making that field last-writer-wins across entry authorization and filtering. No functional impact today: it is read only for DEBUG logging in `JcasbinAuthorizer.java:301,332,372` and for the denial-event message at `GravitinoInterceptionService.java:264`, which runs before `bindIfRead` at `:274`. Worth a sentence on the field in `AuthorizationRequestContext` so a future reader after `proceed()` doesn't assume it still carries the entry expression. (verified by: grepping every get/set of the field and checking the call ordering inside the interceptor.) ### Tests Coverage of the new behaviour is good: parallel filtering with a table-level deny plus the short-circuit hit/miss split (`TestBaseMetadataAuthorizationMethodInterceptor#testReadListReusesEntryContext`), cleanup after the endpoint throws, POST non-reuse, principal-identity and metalake isolation and worker non-inheritance (`TestAuthorizationRequestScope`), and prefetch reuse with revalidation on the next request (`TestJcasbinAuthorizer#testReadScopeReusesEntryRolePrefetch`). Two gaps: - `BaseMetadataAuthorizationMethodInterceptor.java:329` — the `skipStandardCheck ? null : metalakeIdent` branch is never exercised. `shouldSkipAuthorization` is overridden only by `IcebergMetadataAuthorizationMethodInterceptor.java:183`, and the new `TestInterceptor` does not override it, so nothing asserts that a skipped standard check leaves the scope unbound on a `@GET` method. A one-line override in the test interceptor covers it. - No test opens a nested scope, which is what would catch finding 1 if it ever becomes reachable. Verified separately, since it is the crux of whether the optimization fires at all: the identity check at `AuthorizationRequestScope.java:104` holds in production. `AuthenticationFilter.runAsPrincipal` (`server-common/.../authentication/AuthenticationFilter.java:248`) publishes the same finalized `Principal` instance both as the request attribute and into the `Subject`, and `Utils.doAs` (`server-common/.../web/Utils.java:282`) re-binds that same instance, so `PrincipalUtils.getCurrentPrincipal()` inside the resource method returns the object bound by the interceptor. The `@GET` gate also misses nothing: all 17 `filterByExpression` call sites under `server/.../web/rest/` and the Lance list endpoints (`LanceNamespaceOperations.java:69,82,189`) are `@GET`, and every module uses `javax.ws.rs`, so the annotation check matches. ### Nits - `AuthorizationRequestScope.java:86` — `bind` is public but only `bindIfRead` and tests call it; `@VisibleForTesting` would document that the read-only gate lives in `bindIfRead` and discourage future callers from going around it. - `AuthorizationRequestScope.java:101` — a cache miss silently returns a fresh context. A DEBUG log on the mismatch branch (principal vs. metalake) would make "why is reuse not happening here" diagnosable without a debugger. --- _Generated by [Claude Code](https://claude.ai/code)_ -- 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]
