jduo commented on a change in pull request #7994:
URL: https://github.com/apache/arrow/pull/7994#discussion_r476900989
##########
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/auth/BasicAuthCallCredentials.java
##########
@@ -17,42 +17,39 @@
package org.apache.arrow.flight.auth;
-import java.util.Iterator;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.concurrent.Executor;
-import org.apache.arrow.flight.impl.Flight.BasicAuth;
+import io.grpc.CallCredentials;
+import io.grpc.Metadata;
/**
* A client auth handler that supports username and password.
*/
-public class BasicClientAuthHandler implements ClientAuthHandler {
+public final class BasicAuthCallCredentials extends CallCredentials {
Review comment:
> I started looking at the changes, but IMO, this can be done a
different way.
>
> If the goal here is to be able to use standard gRPC call credentials and
the standard Authorization header, then there are only a few changes needed:
>
> `flight-grpc` should re-export `GrpcCallOption`. This would let you define
a custom call option that injects a gRPC CallCredentials.
>
https://github.com/apache/arrow/blob/maint-1.0.x/java/flight/flight-core/src/main/java/org/apache/arrow/flight/CallOptions.java#L59
>
> FlightClient should have an option or a way to specify default call
options, so you could add the auth call option on every call.
>
> Then, on the server side, you can use `flight-grpc` to get the gRPC
service instance and register it with your own gRPC server, and configure
authentication there. Your custom auth interceptor could still put the identity
of the user in AuthConstants.PEER_IDENTITY_KEY, so that you could still read it
from the Flight CallContext.
>
> This PR includes default implementations of various auth options. Those
could be included as part of flight-grpc.
>
> I admit I don't love the current auth implementation (specifically,
Handshake causes issues because it's intentionally unauthenticated and we
should use the standard Authorization header, and we can't handle more complex
things like WWW-Authenticate and multiple authentication headers, X-Forwarded-*
headers, etc.), but as is, this PR breaks compatibility and re-exports a lot of
gRPC types from the Flight core.
Hi David, just want to clarify comments about not re-exporting gRPC types.
Currently flight-grpc has very little -- just FlightGrpcUtils. flight-core
however has a package that has several gRPC dependencies such as
MetadataAdapter, ClientInterceptorAdapter, etc.
Is the intent to move these to flight-grpc? Should I move any new classes I
add that depend on gRPC there?
FlightServer itself depends on gRPC as well, and it is in flight-core and
not in the gRPC package.
Would a separate PR be a better place for cleaning up separation of
flight-core from grpc?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
