bharos commented on code in PR #10704:
URL: https://github.com/apache/gravitino/pull/10704#discussion_r3082206910
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java:
##########
@@ -205,4 +213,23 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
return IcebergExceptionMapper.toRESTResponse(e);
}
}
+
+ // Assuming all REST catalog instances are Gravitino servers, for this
scenario, the Gravitino
Review Comment:
The comment says "Assuming all REST catalog instances are Gravitino servers"
but:
This assumption has no validation and cannot be overridden
A REST backend pointing to a non-Gravitino server that doesn't enforce
authorization would silently become unprotected
Should we consider explicit configuration flag (e.g.,
gravitino.iceberg-rest.skip-authorization-for-rest-backend=true so operators
can opt-in/out ?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java:
##########
@@ -205,4 +213,23 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
return IcebergExceptionMapper.toRESTResponse(e);
}
}
+
+ // Assuming all REST catalog instances are Gravitino servers, for this
scenario, the Gravitino
+ // server would not perform authorization and would only act as a proxy.
+ private boolean skipAuthorizationForRestCatalog(
Review Comment:
Are we adding Iceberg-specific logic here in base class
The skipAuthorizationForRestCatalog() method imports and depends on
IcebergCatalogWrapper, IcebergCatalogWrapperManager, IcebergRESTServerContext,
and RESTCatalog. This couples the "Base" authorization interceptor to Iceberg
REST internals. The method should live in
IcebergMetadataAuthorizationMethodInterceptor (the subclass), or the base class
should expose an overridable hook like shouldSkipAuthorization(Map<EntityType,
NameIdentifier>) that the subclass implements ?
I think there is this method `authorizationCompleted` , can that be reused
here ?
##########
docs/iceberg-rest-service.md:
##########
@@ -129,6 +129,7 @@ If you are using multiple JDBC catalog backends, setting
`jdbc-initialize` to tr
#### REST backend configuration
Use the REST backend to proxy another Iceberg REST catalog server (IRC2). The
Gravitino Iceberg REST service acts as IRC1 and forwards catalog operations to
IRC2.
+Assuming IRC2 will be the Gravitino server, too. for this scenario, the IRC
would not perform authorization and would only act as a proxy. IRC2 will
perform authorization
Review Comment:
nit : "F" in for should be uppercase
we can just say
When IRC2 is another Gravitino server, IRC1 acts as a proxy and does not
perform authorization. IRC2 handles authorization.
--
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]
