Copilot commented on code in PR #10704:
URL: https://github.com/apache/gravitino/pull/10704#discussion_r3084869953
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java:
##########
@@ -131,4 +134,27 @@ protected Optional<AuthorizationHandler>
createAuthorizationHandler(
protected boolean isExceptionPropagate(Exception e) {
return e.getClass().getName().startsWith("org.apache.iceberg.exceptions");
}
+
+ @Override
+ protected boolean shouldSkipAuthorization(Map<EntityType, NameIdentifier>
nameIdentifierMap) {
+ if
(!IcebergRESTServerContext.getInstance().skipAuthorizationForRestBackend()) {
+ return false;
+ }
+
+ NameIdentifier catalogId = nameIdentifierMap.get(EntityType.CATALOG);
+ if (catalogId == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapperManager wrapperManager =
+ IcebergRESTServerContext.getInstance().catalogWrapperManager();
+ if (wrapperManager == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapper catalogWrapper =
wrapperManager.getCatalogWrapper(catalogId.name());
+ // When IRC2 is another Gravitino server, IRC1 acts as a proxy and does
not perform
+ // authorization. IRC2 handles authorization.
+ return catalogWrapper != null && catalogWrapper.getCatalog() instanceof
RESTCatalog;
Review Comment:
`shouldSkipAuthorization` calls
`wrapperManager.getCatalogWrapper(catalogId.name())`, but
`getCatalogWrapper(...)` can throw (e.g., `IllegalArgumentException` when auth
is enabled with a non-dynamic provider, or `NoSuchCatalogException` for unknown
catalogs). If that happens here, the interceptor will treat it as an
authorization system error rather than letting the normal request path
handle/mapping the error. Consider making `shouldSkipAuthorization`
non-throwing (catch and return `false`), or use a side-effect-free check (e.g.,
inspect the catalog config/provider to determine backend type) to avoid
changing error semantics and doing extra work just to decide whether to skip
auth.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java:
##########
@@ -124,6 +133,10 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
Map<Entity.EntityType, NameIdentifier> nameIdentifierMap =
extractNameIdentifierFromParameters(parameters, args);
+ if (shouldSkipAuthorization(nameIdentifierMap)) {
+ return methodInvocation.proceed();
+ }
Review Comment:
`shouldSkipAuthorization(...)` returns `methodInvocation.proceed()` inside
the main authorization `try` block. If `proceed()` throws (e.g., a
validation/ICEBERG exception), it will be caught by the `catch (Exception ex)`
intended for authorization failures and may be converted into an "Authorization
failed due to system internal error" response instead of being handled by the
final `try/catch (Throwable e)` that consistently maps operation exceptions via
`IcebergExceptionMapper`. Consider avoiding the early `return` here (e.g., set
a flag and fall through to the common `proceed()` path, or delegate to a shared
`safeProceed(...)` helper) so exception-to-response mapping remains consistent
when authorization is skipped.
--
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]
