[C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler

[kou]

Describe the enhancement requested

arrow::flight::ServerAuthHandler::Authenticate() can't refer HTTP headers nor HTTP query parameters:

arrow/cpp/src/arrow/flight/server_auth.h

Lines 56 to 58 in 0ea1a10

	/// \brief Authenticate the client on initial connection. The server
	/// can send and read responses from the client at any time.
	virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;

I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .

How about adding const CallHeaders& incoming_headers parameter (like for arrow::flight::ServerMiddlewareFactory::StartCall()) or something to arrow::flight::ServerAuthHandler::Authenticate()?

@lidavidm @cyb70289 What do you think about this feature request?

Component(s)

C++, FlightRPC

[kou]

Or adding const ServerCallContext& context argument?

But we also need to add ServerCallContext::incoming_headers() for this idea.

[lidavidm]

Is this basically the same idea as #34817?

I'm reluctant to expand Handshake because it's stateful. I think the explicit messages in that PR are better, along with support for just passing the value via headers always.

That said, if we are to expand this method I think it should be via adding const ServerCallContext& context because technically you are in an RPC call during execution.

But we also need to add ServerCallContext::incoming_headers() for this idea.

I think we should do this anyways.

I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .

I don't think query parameters get passed through via gRPC, unfortunately. So only headers would work.

[kou]

Is this basically the same idea as #34817?

Ah, sorry. I haven't read the discussion on the PR.

I'm reluctant to expand Handshake because it's stateful. I think the explicit messages in that PR are better, along with support for just passing the value via headers always.

OK. I withdraw this idea.

But we also need to add ServerCallContext::incoming_headers() for this idea.

I think we should do this anyways.

OK. I've opened #35375.

I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .

I don't think query parameters get passed through via gRPC, unfortunately. So only headers would work.

Oh, sorry. I should have learned more about gRPC...

[lidavidm]

Or adding const ServerCallContext& context argument?

Ah to be clear: I think this would also be a good idea regardless.

[kou]

Ah, OK. I'll open a new issue for it.

Should we keep a backward compatibility for it?

If we should keep a backward compatibility, we will use the following changes:

diff --git a/cpp/src/arrow/flight/server_auth.h b/cpp/src/arrow/flight/server_auth.h
index b1ccb096d..2f63d0b23 100644
--- a/cpp/src/arrow/flight/server_auth.h
+++ b/cpp/src/arrow/flight/server_auth.h
@@ -55,6 +55,11 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   virtual ~ServerAuthHandler();
   /// \brief Authenticate the client on initial connection. The server
   /// can send and read responses from the client at any time.
+  virtual Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) {
+    return Authenticate(outgoing, incoming);
+  }
+  /// \brief Authenticate the client on initial connection. The server
+  /// can send and read responses from the client at any time.
   virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
   /// \brief Validate a per-call client token.
   /// \param[in] token The client token. May be the empty string if
@@ -63,6 +68,15 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   /// authentication method supports it.
   /// \return Status OK if the token is valid, any other status if
   /// validation failed
+  virtual Status IsValid(const ServerCallContext& context, const std::string& token, std::string* peer_identity) {
+    return IsValid(outgoing, incoming);
+  }
+  /// \param[in] token The client token. May be the empty string if
+  /// the client does not provide a token.
+  /// \param[out] peer_identity The identity of the peer, if this
+  /// authentication method supports it.
+  /// \return Status OK if the token is valid, any other status if
+  /// validation failed
   virtual Status IsValid(const std::string& token, std::string* peer_identity) = 0;
 };
 

If we should not keep a backward compatibility, we will use the following changes:

diff --git a/cpp/src/arrow/flight/server_auth.h b/cpp/src/arrow/flight/server_auth.h
index b1ccb096d..391910548 100644
--- a/cpp/src/arrow/flight/server_auth.h
+++ b/cpp/src/arrow/flight/server_auth.h
@@ -55,7 +55,7 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   virtual ~ServerAuthHandler();
   /// \brief Authenticate the client on initial connection. The server
   /// can send and read responses from the client at any time.
-  virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
+  virtual Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
   /// \brief Validate a per-call client token.
   /// \param[in] token The client token. May be the empty string if
   /// the client does not provide a token.
@@ -63,7 +63,7 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   /// authentication method supports it.
   /// \return Status OK if the token is valid, any other status if
   /// validation failed
-  virtual Status IsValid(const std::string& token, std::string* peer_identity) = 0;
+  virtual Status IsValid(const ServerCallContext& context, const std::string& token, std::string* peer_identity) = 0;
 };
 
 /// \brief An authentication mechanism that does nothing.

I think that we should keep backward compatibility, but user code will be redundant. Because user code needs to implement Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) too:

Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) {
  // Implement...
}

Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming)  {
  return Status::NotImplemented("This version is never used");
}

[lidavidm]

I think we should keep compatibility for a few releases. The redundancy is unfortunate but we can't avoid it.

[kou]

I agree with you. I've opened #35377.