lidavidm commented on a change in pull request #8724:
URL: https://github.com/apache/arrow/pull/8724#discussion_r527973482
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -191,6 +194,14 @@ class ARROW_FLIGHT_EXPORT FlightClient {
Status Authenticate(const FlightCallOptions& options,
std::unique_ptr<ClientAuthHandler> auth_handler);
+ /// \brief Authenticate to the server using the given handler.
Review comment:
There's no handler in play here.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.cc
##########
@@ -0,0 +1,124 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Interfaces for defining middleware for Flight clients. Currently
+// experimental.
+
+#include "client_header_auth_middleware.h"
+#include "client_middleware.h"
+#include "client_auth.h"
+#include "client.h"
+
+namespace arrow {
+namespace flight {
+
+ std::string base64_encode(const std::string& input);
+
+ ClientBearerTokenMiddleware::ClientBearerTokenMiddleware(
+ std::pair<std::string, std::string>* bearer_token_)
+ : bearer_token(bearer_token_) { }
+
+ void ClientBearerTokenMiddleware::SendingHeaders(AddCallHeaders*
outgoing_headers) { }
+
+ void ClientBearerTokenMiddleware::ReceivedHeaders(
+ const CallHeaders& incoming_headers) {
+ // Grab the auth token if one exists.
+ auto bearer_iter = incoming_headers.find(AUTH_HEADER);
+ if (bearer_iter == incoming_headers.end()) {
+ return;
+ }
+
+ // Check if the value of the auth token starts with the bearer prefix,
latch the token.
+ std::string bearer_val = bearer_iter->second.to_string();
+ if (bearer_val.size() > BEARER_PREFIX.size()) {
+ bool hasPrefix = std::equal(bearer_val.begin(), bearer_val.begin() +
BEARER_PREFIX.size(), BEARER_PREFIX.begin(),
+ [] (const char& char1, const char& char2) {
+ return (std::toupper(char1) == std::toupper(char2));
+ }
+ );
+ if (hasPrefix) {
+ *bearer_token = std::make_pair(AUTH_HEADER, bearer_val);
+ }
+ }
+ }
+
+ void ClientBearerTokenMiddleware::CallCompleted(const Status& status) { }
+
+ void ClientBearerTokenFactory::StartCall(const CallInfo& info,
std::unique_ptr<ClientMiddleware>* middleware) {
+ *middleware = std::unique_ptr<ClientBearerTokenMiddleware>(new
ClientBearerTokenMiddleware(bearer_token));
+ }
+
+ void ClientBearerTokenFactory::Reset() {
+ *bearer_token = std::make_pair("", "");
+ }
+
+ template<typename ... Args>
+ std::string string_format(const std::string& format, const Args... args) {
+ // Check size requirement for new string and increment by 1 for null
terminator.
+ size_t size = std::snprintf(nullptr, 0, format.c_str(), args ...) + 1;
+ if(size <= 0){
+ throw std::runtime_error("Error during string formatting. Format: '" +
format + "'.");
Review comment:
And Arrow disallows exceptions.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -328,7 +332,7 @@ class GrpcClientInterceptorAdapterFactory
: public grpc::experimental::ClientInterceptorFactoryInterface {
public:
GrpcClientInterceptorAdapterFactory(
- std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware)
+ std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware)
Review comment:
So, I'd rather we store a reference to this interceptor factory instead
of a reference to the middleware with a mutable pointer back, i.e. I'd rather
have this class own the middleware as it currently does, and have FlightClient
call a method of this class to add more middleware at runtime.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.h
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Interfaces for defining middleware for Flight clients. Currently
+// experimental.
+
+#pragma once
+
+#include "arrow/flight/client_middleware.h"
+#include "arrow/flight/client_auth.h"
+#include "arrow/flight/client.h"
+
+#ifdef GRPCPP_PP_INCLUDE
+#include <grpcpp/grpcpp.h>
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+#include <grpcpp/security/tls_credentials_options.h>
Review comment:
I don't think we need this include?
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -65,6 +65,9 @@ class ARROW_FLIGHT_EXPORT FlightCallOptions {
/// \brief IPC writer options, if applicable for the call.
ipc::IpcWriteOptions write_options;
+
+ /// \brief Metadata for client to add to context.
+ std::vector<std::pair<std::string, std::string>> metadata;
Review comment:
I believe we call it headers elsewhere, so this should stay consistent
with that.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.h
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Interfaces for defining middleware for Flight clients. Currently
+// experimental.
+
+#pragma once
+
+#include "arrow/flight/client_middleware.h"
+#include "arrow/flight/client_auth.h"
+#include "arrow/flight/client.h"
+
+#ifdef GRPCPP_PP_INCLUDE
+#include <grpcpp/grpcpp.h>
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+#include <grpcpp/security/tls_credentials_options.h>
+#endif
+#else
+#include <grpc++/grpc++.h>
+#endif
+
+#include <algorithm>
+#include <iostream>
+#include <cctype>
+#include <string>
+
+const std::string AUTH_HEADER = "authorization";
Review comment:
These should be named kAuthHeader etc. and should go in the .cc file
unless we actually want to expose these to users.
Or alternatively, the other constants are currently in one of the
_internal.h headers.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.h
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Interfaces for defining middleware for Flight clients. Currently
+// experimental.
+
+#pragma once
+
+#include "arrow/flight/client_middleware.h"
+#include "arrow/flight/client_auth.h"
+#include "arrow/flight/client.h"
+
+#ifdef GRPCPP_PP_INCLUDE
+#include <grpcpp/grpcpp.h>
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+#include <grpcpp/security/tls_credentials_options.h>
+#endif
+#else
+#include <grpc++/grpc++.h>
+#endif
+
+#include <algorithm>
+#include <iostream>
+#include <cctype>
+#include <string>
+
+const std::string AUTH_HEADER = "authorization";
+const std::string BEARER_PREFIX = "Bearer ";
+const std::string BASIC_PREFIX = "Basic ";
+
+namespace arrow {
+namespace flight {
+
+// TODO: Need to add documentation in this file.
+void ARROW_FLIGHT_EXPORT AddBasicAuthHeaders(grpc::ClientContext* context,
Review comment:
This is internal - it shouldn't be in a public header. (Ditto for the
grpc include.)
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -371,7 +375,7 @@ class GrpcClientInterceptorAdapterFactory
}
private:
- std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware_;
+ std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware_;
Review comment:
This should be a pointer if not a copy.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.h
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review comment:
This should probably be an _internal.h header since I don't think any of
this is intended to be directly used outside of the implementation here.
##########
File path: cpp/src/arrow/flight/client_header_auth_middleware.cc
##########
@@ -0,0 +1,124 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Interfaces for defining middleware for Flight clients. Currently
+// experimental.
+
+#include "client_header_auth_middleware.h"
+#include "client_middleware.h"
+#include "client_auth.h"
+#include "client.h"
+
+namespace arrow {
+namespace flight {
+
+ std::string base64_encode(const std::string& input);
+
+ ClientBearerTokenMiddleware::ClientBearerTokenMiddleware(
+ std::pair<std::string, std::string>* bearer_token_)
+ : bearer_token(bearer_token_) { }
+
+ void ClientBearerTokenMiddleware::SendingHeaders(AddCallHeaders*
outgoing_headers) { }
+
+ void ClientBearerTokenMiddleware::ReceivedHeaders(
+ const CallHeaders& incoming_headers) {
+ // Grab the auth token if one exists.
+ auto bearer_iter = incoming_headers.find(AUTH_HEADER);
+ if (bearer_iter == incoming_headers.end()) {
+ return;
+ }
+
+ // Check if the value of the auth token starts with the bearer prefix,
latch the token.
+ std::string bearer_val = bearer_iter->second.to_string();
+ if (bearer_val.size() > BEARER_PREFIX.size()) {
+ bool hasPrefix = std::equal(bearer_val.begin(), bearer_val.begin() +
BEARER_PREFIX.size(), BEARER_PREFIX.begin(),
+ [] (const char& char1, const char& char2) {
+ return (std::toupper(char1) == std::toupper(char2));
+ }
+ );
+ if (hasPrefix) {
+ *bearer_token = std::make_pair(AUTH_HEADER, bearer_val);
+ }
+ }
+ }
+
+ void ClientBearerTokenMiddleware::CallCompleted(const Status& status) { }
+
+ void ClientBearerTokenFactory::StartCall(const CallInfo& info,
std::unique_ptr<ClientMiddleware>* middleware) {
+ *middleware = std::unique_ptr<ClientBearerTokenMiddleware>(new
ClientBearerTokenMiddleware(bearer_token));
+ }
+
+ void ClientBearerTokenFactory::Reset() {
+ *bearer_token = std::make_pair("", "");
+ }
+
+ template<typename ... Args>
Review comment:
I don't think we need an entire templated function to concatenate two
strings.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -328,7 +332,7 @@ class GrpcClientInterceptorAdapterFactory
: public grpc::experimental::ClientInterceptorFactoryInterface {
public:
GrpcClientInterceptorAdapterFactory(
- std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware)
+ std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware)
Review comment:
This should either be const reference or a pointer.
----------------------------------------------------------------
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
