You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/18 03:02:01 UTC
[GitHub] [arrow] tifflhl opened a new pull request #8959: Arrow 10486: [Python] Header-based auth in servers
tifflhl opened a new pull request #8959:
URL: https://github.com/apache/arrow/pull/8959
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8959: Arrow 10486: [Python] Header-based auth in servers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#issuecomment-747837491
<!--
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.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548056216
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
+ """Lookup the value of given key in the given headers.
+ The key lookup is case insensitive.
+ """
+ for key in headers:
+ if key.lower() == lookup_key.lower():
+ return headers.get(key)
+
+ raise flight.FlightUnauthenticatedError(
+ 'No authorization header found.')
+
+
+class ClientHeaderAuthMiddlewareFactory(ClientMiddlewareFactory):
+ """ClientMiddlewareFactory that creates ClientAuthHeaderMiddleware."""
+
+ def __init__(self):
+ self.call_credential = []
+
+ def start_call(self, info):
+ return ClientHeaderAuthMiddleware(self)
+
+ def set_call_credential(self, call_credential):
+ self.call_credential = call_credential
+
+
+class ClientHeaderAuthMiddleware(ClientMiddleware):
+ """
+ ClientMiddleware that extracts the authorization header
+ from the server.
+
+ This is an example of a ClientMiddleware that can extract
+ the bearer token authorization header from a HTTP header
+ authentication enabled server.
+
+ Parameters
+ ----------
+ factory : ClientHeaderAuthMiddlewareFactory
+ This factory is used to set call credentials if an
+ authorization header is found in the headers from the server.
+ """
+
+ def __init__(self, factory):
+ self.factory = factory
+
+ def received_headers(self, headers):
+ auth_header = case_insensitive_header_lookup(headers, 'Authorization')
Review comment:
There seems to be a mix of single and double quotes for strings. Should these be consistent?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] tifflhl commented on a change in pull request #8959: ARROW-11004: [Python] Header-based auth in client
Posted by GitBox <gi...@apache.org>.
tifflhl commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r546951872
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
Review comment:
This PR is only for the client implementation of header based auth in Python, same as the C++ implementation done here: https://github.com/apache/arrow/pull/8724. I've created a new subtask in ARROW-10486 that reflects this scope of work: https://issues.apache.org/jira/browse/ARROW-11004.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548050472
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
Review comment:
Do you need to add a description for self in the list of parameters below?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #8959:
URL: https://github.com/apache/arrow/pull/8959
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548194584
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -118,12 +118,18 @@ cdef class FlightCallOptions(_Weakrefable):
write_options : pyarrow.ipc.IpcWriteOptions, optional
IPC write options. The default options can be controlled
by environment variables (see pyarrow.ipc).
-
+ headers : vector[pair[c_string, c_string]], optional
Review comment:
This type hint should use Python conventions (e.g. `List[Tuple[str, str]]`)
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
Review comment:
Let's follow Python naming conventions - this should use snake_case.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
Review comment:
This is specifically to extract an authentication header based n the exception it raises, let's make sure the name reflects that.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
reader.poison()
Review comment:
Not a typo.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -996,6 +1152,100 @@ def test_token_auth_invalid():
client.authenticate(TokenClientAuthHandler('test', 'wrong'))
+header_auth_server_middleware_factory = HeaderAuthServerMiddlewareFactory()
+no_op_auth_handler = NoopAuthHandler()
+
+
+def test_authenticate_basic_token():
+ """Test authenticateBasicToken with bearer token and auth headers."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
+
+
+def test_authenticate_basic_token_invalid_password():
+ """Test authenticateBasicToken with an invalid password."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ with pytest.raises(flight.FlightUnauthenticatedError):
+ client.authenticateBasicToken(b'test', b'badpassword')
+
+
+def test_authenticate_basic_token_and_action():
+ """Test authenticateBasicToken and doAction after authentication."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
+ options = flight.FlightCallOptions(headers=[token_pair])
+ result = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result[0].body.to_pybytes() == b'token1234'
+
+
+def test_authenticate_basic_token_with_client_middleware():
+ """Test authenticateBasicToken with client middleware
+ to intercept authorization header returned by the
+ HTTP header auth enabled server.
+ """
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client_auth_middleware = ClientHeaderAuthMiddlewareFactory()
+ client = FlightClient(
+ ('localhost', server.port),
+ middleware=[client_auth_middleware]
+ )
+ encoded_credentials = base64.b64encode(b'test:password')
+ options = flight.FlightCallOptions(headers=[
+ (b'authorization', b'Basic ' + encoded_credentials)
+ ])
+ result = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result[0].body.to_pybytes() == b'token1234'
+ assert client_auth_middleware.call_credential[0] == b'authorization'
+ assert client_auth_middleware.call_credential[1] == \
+ b'Bearer ' + b'token1234'
+ result2 = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result2[0].body.to_pybytes() == b'token1234'
+ assert client_auth_middleware.call_credential[0] == b'authorization'
+ assert client_auth_middleware.call_credential[1] == \
+ b'Bearer ' + b'token1234'
+
+
+def test_arbitrary_headers_in_flight_call_options():
+ """Test passing multiple arbitrary headers to the middleware."""
+ with ArbitraryHeadersFlightServer(
+ auth_handler=no_op_auth_handler,
+ middleware={
+ "auth": HeaderAuthServerMiddlewareFactory(),
+ "arbitrary-headers": ArbitraryHeadersServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
Review comment:
nit: why concat here? (and above)
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
Review comment:
self is never documented - it's implicitly passed by Python (it's equivalent to `this` in Java et al)
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with HTTP basic authentication.
+
+ Parameters
+ ----------
+ username : string
+ Username to authenticate with
+ password : string
+ Password to authenticate with
+ options : FlightCallOptions
+ Options for this call
+
+ Returns
+ -------
+ pair : pair[string, string]
Review comment:
Same for the type hints here - use `str` and `Tuple`
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
Review comment:
Headers are supposed to be treated case-insensitively so even though other languages may use Authorization, it will all get folded to the same case
##########
File path: python/pyarrow/includes/libarrow_flight.pxd
##########
@@ -307,6 +308,11 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CStatus Authenticate(CFlightCallOptions& options,
unique_ptr[CClientAuthHandler] auth_handler)
+ CResult[pair[c_string, c_string]] AuthenticateBasicToken(
+ CFlightCallOptions& options,
+ const c_string& username,
Review comment:
The mixture here is because this is a set of Cython type definitions for C++ code, which uses the C++ conventions (e.g. CResult), but the overall project is in Python, which uses Python conventions (e.g. c_string)
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
reader.poison()
Review comment:
This is to ensure a server doesn't use the Python reader beyond the lifetime of the C++ reader it wraps.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r547117857
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
Review comment:
Should that be a comment for the headers parameter?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#issuecomment-751390132
@lidavidm yes these changes are sufficient.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] tifflhl commented on a change in pull request #8959: ARROW-11004: [Python] Header-based auth in client
Posted by GitBox <gi...@apache.org>.
tifflhl commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r546952806
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with header token authentication.
+
+ Parameters
+ ----------
+ username : string
+ Username to authenticate with
+ password : string
+ Password to authenticate with
+ options : FlightCallOptions
+ Options for this call
+
+ Returns
+ -------
+ pair : pair[string, string]
+ A pair representing the FlightCallOptions header
+ entry of a bearer token.
+ """
+ cdef:
+ CResult[pair[c_string, c_string]] result
+ CFlightCallOptions* c_options = FlightCallOptions.unwrap(options)
+ c_string user = tobytes(username)
+ c_string pw = tobytes(password)
+
+ with nogil:
+ result = self.client.get().AuthenticateBasicToken(deref(c_options),
Review comment:
Based on how the C++ code was implemented , the Python wrapper can only wrap the AuthenticateBasicToken method in the C++ client. Seems like we would need to expand the C++ client implementation if we were to have C++/Python impl match the Java client implementation. https://github.com/apache/arrow/pull/8724 -> C++ impl only exposed authenticateBasicToken.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548051765
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
reader.poison()
Review comment:
Is this a typo "poison"?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548050472
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
Review comment:
Do you need to add a description for self in the list of parameters?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] tifflhl commented on pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
tifflhl commented on pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#issuecomment-749275421
I added a test example implementation called ClientHeaderAuthMiddleware, it intercepts headers from a HTTP header auth enabled server and make it accessible to the code using the FlightClient. A new test also demonstrates how to use the ClientMiddleware to intercept an Authorization header returned from the server and to use it in subsequent calls.
I don't think it makes sense to include the example ClientMiddleware to the .pyx pr .pxd, since they are not callable objects in C++. Also seems like a convention to not define explicit middleware objects in python, but rather, provide examples?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8959: Arrow-10486: [Python] Header-based auth in client
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r545598826
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
Review comment:
This class should not be part of test_flight.py to be used in production. It should also be changed to have some API for checking username/password/bearer rather than being hard coded.
Unless this PR is really for header-based auth in just clients, in which case the description is not really correct.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with header token authentication.
Review comment:
It's not really header token authentication, it's HTTP basic authentication
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
Review comment:
Better to return None than empty string?
##########
File path: python/pyarrow/includes/libarrow_flight.pxd
##########
@@ -307,6 +308,12 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CStatus Authenticate(CFlightCallOptions& options,
unique_ptr[CClientAuthHandler] auth_handler)
+ # TODO: Add AuthenticateBasicToken
Review comment:
Remove TODO
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
Review comment:
This is test code, but this should be a case insensitive lookup.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
+
+
+class HeaderAuthFlightServer(FlightServerBase):
+ """A Flight server that tests with basic token authentication. """
+
+ def do_action(self, context, action):
+ middleware = context.get_middleware("auth")
+ if middleware:
+ headers = middleware.sending_headers()
+ auth_header = headers.get('authorization')
+ values = auth_header.split(' ')
+ return [values[1].encode("utf-8")]
+ raise flight.FlightUnauthenticatedError(
Review comment:
This seems like it's an internal server error.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with header token authentication.
+
+ Parameters
+ ----------
+ username : string
+ Username to authenticate with
+ password : string
+ Password to authenticate with
+ options : FlightCallOptions
+ Options for this call
+
+ Returns
+ -------
+ pair : pair[string, string]
+ A pair representing the FlightCallOptions header
+ entry of a bearer token.
+ """
+ cdef:
+ CResult[pair[c_string, c_string]] result
+ CFlightCallOptions* c_options = FlightCallOptions.unwrap(options)
+ c_string user = tobytes(username)
+ c_string pw = tobytes(password)
+
+ with nogil:
+ result = self.client.get().AuthenticateBasicToken(deref(c_options),
Review comment:
Shouldn't there be a FlightCallOption implementation to do this? How do you use HTTP basic auth without using authenticateBasicToken? (eg in Java you can directly call getFlightInfo without calling an authenticate function and supply a FlightCallOption with the basic auth info).
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
Review comment:
We had been using Authorization (title case) in other languages.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548058361
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
+ """Lookup the value of given key in the given headers.
+ The key lookup is case insensitive.
+ """
+ for key in headers:
+ if key.lower() == lookup_key.lower():
+ return headers.get(key)
+
+ raise flight.FlightUnauthenticatedError(
+ 'No authorization header found.')
+
+
+class ClientHeaderAuthMiddlewareFactory(ClientMiddlewareFactory):
+ """ClientMiddlewareFactory that creates ClientAuthHeaderMiddleware."""
+
+ def __init__(self):
+ self.call_credential = []
+
+ def start_call(self, info):
+ return ClientHeaderAuthMiddleware(self)
+
+ def set_call_credential(self, call_credential):
+ self.call_credential = call_credential
+
+
+class ClientHeaderAuthMiddleware(ClientMiddleware):
+ """
+ ClientMiddleware that extracts the authorization header
+ from the server.
+
+ This is an example of a ClientMiddleware that can extract
+ the bearer token authorization header from a HTTP header
+ authentication enabled server.
+
+ Parameters
+ ----------
+ factory : ClientHeaderAuthMiddlewareFactory
+ This factory is used to set call credentials if an
+ authorization header is found in the headers from the server.
+ """
+
+ def __init__(self, factory):
+ self.factory = factory
+
+ def received_headers(self, headers):
+ auth_header = case_insensitive_header_lookup(headers, 'Authorization')
+ self.factory.set_call_credential([
+ b'authorization',
+ auth_header[0].encode("utf-8")])
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = case_insensitive_header_lookup(
+ headers,
+ 'Authorization'
+ )
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
Review comment:
3 uses of the string 'Invalid credentials'. Should this be made a constant?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#issuecomment-749225142
https://issues.apache.org/jira/browse/ARROW-11004
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r549010231
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with header token authentication.
+
+ Parameters
+ ----------
+ username : string
+ Username to authenticate with
+ password : string
+ Password to authenticate with
+ options : FlightCallOptions
+ Options for this call
+
+ Returns
+ -------
+ pair : pair[string, string]
+ A pair representing the FlightCallOptions header
+ entry of a bearer token.
+ """
+ cdef:
+ CResult[pair[c_string, c_string]] result
+ CFlightCallOptions* c_options = FlightCallOptions.unwrap(options)
+ c_string user = tobytes(username)
+ c_string pw = tobytes(password)
+
+ with nogil:
+ result = self.client.get().AuthenticateBasicToken(deref(c_options),
Review comment:
@jduo are the changes in this PR sufficient?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] tifflhl commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
tifflhl commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r546987299
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with header token authentication.
Review comment:
Addressed in new commit.
##########
File path: python/pyarrow/includes/libarrow_flight.pxd
##########
@@ -307,6 +308,12 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CStatus Authenticate(CFlightCallOptions& options,
unique_ptr[CClientAuthHandler] auth_handler)
+ # TODO: Add AuthenticateBasicToken
Review comment:
Addressed in new commit.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
Review comment:
Returning None gives the error `Flight RPC failed with Python exception "TypeError: expected bytes, NoneType found"`. Based on the method's documentation [here](https://github.com/apache/arrow/blob/a2e7d3a87fb8fa1cc98a54029c0262df468838fa/python/pyarrow/_flight.pyx#L2046), returning an empty string is allowed, I added a comment to clarify the behaviour.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
Review comment:
Addressed in new commit.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
Review comment:
It would seem the C++ grpc impl does not allow capitals in key, when using capital A, the header was rejected due to https://github.com/grpc/grpc/blob/b1df40104c1720bde3b22ded451a037f11b5dc54/src/core/lib/surface/validate_metadata.cc#L78
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,95 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """Do nothing."""
+ return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = headers.get('authorization')
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ token = 'token1234'
+ elif values[0] == 'Bearer':
+ token = values[1]
+ if not token == 'token1234':
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+ else:
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
+
+ return HeaderAuthServerMiddleware(token)
+
+
+class HeaderAuthServerMiddleware(ServerMiddleware):
+ """A ServerMiddleware that transports incoming username and passowrd."""
+
+ def __init__(self, token):
+ self.token = token
+
+ def sending_headers(self):
+ return {'authorization': 'Bearer ' + self.token}
+
+
+class HeaderAuthFlightServer(FlightServerBase):
+ """A Flight server that tests with basic token authentication. """
+
+ def do_action(self, context, action):
+ middleware = context.get_middleware("auth")
+ if middleware:
+ headers = middleware.sending_headers()
+ auth_header = headers.get('authorization')
+ values = auth_header.split(' ')
+ return [values[1].encode("utf-8")]
+ raise flight.FlightUnauthenticatedError(
Review comment:
Addressed in new commit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] tifflhl commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
tifflhl commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548374081
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
+ """Lookup the value of given key in the given headers.
+ The key lookup is case insensitive.
+ """
+ for key in headers:
+ if key.lower() == lookup_key.lower():
+ return headers.get(key)
+
+ raise flight.FlightUnauthenticatedError(
+ 'No authorization header found.')
+
+
+class ClientHeaderAuthMiddlewareFactory(ClientMiddlewareFactory):
+ """ClientMiddlewareFactory that creates ClientAuthHeaderMiddleware."""
+
+ def __init__(self):
+ self.call_credential = []
+
+ def start_call(self, info):
+ return ClientHeaderAuthMiddleware(self)
+
+ def set_call_credential(self, call_credential):
+ self.call_credential = call_credential
+
+
+class ClientHeaderAuthMiddleware(ClientMiddleware):
+ """
+ ClientMiddleware that extracts the authorization header
+ from the server.
+
+ This is an example of a ClientMiddleware that can extract
+ the bearer token authorization header from a HTTP header
+ authentication enabled server.
+
+ Parameters
+ ----------
+ factory : ClientHeaderAuthMiddlewareFactory
+ This factory is used to set call credentials if an
+ authorization header is found in the headers from the server.
+ """
+
+ def __init__(self, factory):
+ self.factory = factory
+
+ def received_headers(self, headers):
+ auth_header = case_insensitive_header_lookup(headers, 'Authorization')
Review comment:
The two are interchangeable.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
+ """Lookup the value of given key in the given headers.
+ The key lookup is case insensitive.
+ """
+ for key in headers:
+ if key.lower() == lookup_key.lower():
+ return headers.get(key)
+
+ raise flight.FlightUnauthenticatedError(
+ 'No authorization header found.')
+
+
+class ClientHeaderAuthMiddlewareFactory(ClientMiddlewareFactory):
+ """ClientMiddlewareFactory that creates ClientAuthHeaderMiddleware."""
+
+ def __init__(self):
+ self.call_credential = []
+
+ def start_call(self, info):
+ return ClientHeaderAuthMiddleware(self)
+
+ def set_call_credential(self, call_credential):
+ self.call_credential = call_credential
+
+
+class ClientHeaderAuthMiddleware(ClientMiddleware):
+ """
+ ClientMiddleware that extracts the authorization header
+ from the server.
+
+ This is an example of a ClientMiddleware that can extract
+ the bearer token authorization header from a HTTP header
+ authentication enabled server.
+
+ Parameters
+ ----------
+ factory : ClientHeaderAuthMiddlewareFactory
+ This factory is used to set call credentials if an
+ authorization header is found in the headers from the server.
+ """
+
+ def __init__(self, factory):
+ self.factory = factory
+
+ def received_headers(self, headers):
+ auth_header = case_insensitive_header_lookup(headers, 'Authorization')
+ self.factory.set_call_credential([
+ b'authorization',
+ auth_header[0].encode("utf-8")])
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):
+ """Validates incoming username and password."""
+
+ def start_call(self, info, headers):
+ auth_header = case_insensitive_header_lookup(
+ headers,
+ 'Authorization'
+ )
+ values = auth_header[0].split(' ')
+ token = ''
+
+ if values[0] == 'Basic':
+ decoded = base64.b64decode(values[1])
+ pair = decoded.decode("utf-8").split(':')
+ if not (pair[0] == 'test' and pair[1] == 'password'):
+ raise flight.FlightUnauthenticatedError('Invalid credentials')
Review comment:
Addressed.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
Review comment:
Addressed.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -118,12 +118,18 @@ cdef class FlightCallOptions(_Weakrefable):
write_options : pyarrow.ipc.IpcWriteOptions, optional
IPC write options. The default options can be controlled
by environment variables (see pyarrow.ipc).
-
+ headers : vector[pair[c_string, c_string]], optional
Review comment:
Addressed.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))
+ def authenticateBasicToken(self, username, password,
+ options: FlightCallOptions = None):
+ """Authenticate to the server with HTTP basic authentication.
+
+ Parameters
+ ----------
+ username : string
+ Username to authenticate with
+ password : string
+ Password to authenticate with
+ options : FlightCallOptions
+ Options for this call
+
+ Returns
+ -------
+ pair : pair[string, string]
Review comment:
Addressed.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -506,6 +505,162 @@ def get_token(self):
return self.token
+class NoopAuthHandler(ServerAuthHandler):
+ """A no-op auth handler."""
+
+ def authenticate(self, outgoing, incoming):
+ """Do nothing."""
+
+ def is_valid(self, token):
+ """
+ Returning an empty string.
+ Returning None causes Type error.
+ """
+ return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):
Review comment:
I modified the function to not raise an exception when the header is not found. The method is used in a test not related to authentication as well.
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -996,6 +1152,100 @@ def test_token_auth_invalid():
client.authenticate(TokenClientAuthHandler('test', 'wrong'))
+header_auth_server_middleware_factory = HeaderAuthServerMiddlewareFactory()
+no_op_auth_handler = NoopAuthHandler()
+
+
+def test_authenticate_basic_token():
+ """Test authenticateBasicToken with bearer token and auth headers."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
+
+
+def test_authenticate_basic_token_invalid_password():
+ """Test authenticateBasicToken with an invalid password."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ with pytest.raises(flight.FlightUnauthenticatedError):
+ client.authenticateBasicToken(b'test', b'badpassword')
+
+
+def test_authenticate_basic_token_and_action():
+ """Test authenticateBasicToken and doAction after authentication."""
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
+ options = flight.FlightCallOptions(headers=[token_pair])
+ result = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result[0].body.to_pybytes() == b'token1234'
+
+
+def test_authenticate_basic_token_with_client_middleware():
+ """Test authenticateBasicToken with client middleware
+ to intercept authorization header returned by the
+ HTTP header auth enabled server.
+ """
+ with HeaderAuthFlightServer(auth_handler=no_op_auth_handler, middleware={
+ "auth": HeaderAuthServerMiddlewareFactory()
+ }) as server:
+ client_auth_middleware = ClientHeaderAuthMiddlewareFactory()
+ client = FlightClient(
+ ('localhost', server.port),
+ middleware=[client_auth_middleware]
+ )
+ encoded_credentials = base64.b64encode(b'test:password')
+ options = flight.FlightCallOptions(headers=[
+ (b'authorization', b'Basic ' + encoded_credentials)
+ ])
+ result = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result[0].body.to_pybytes() == b'token1234'
+ assert client_auth_middleware.call_credential[0] == b'authorization'
+ assert client_auth_middleware.call_credential[1] == \
+ b'Bearer ' + b'token1234'
+ result2 = list(client.do_action(
+ action=flight.Action('test-action', b''), options=options))
+ assert result2[0].body.to_pybytes() == b'token1234'
+ assert client_auth_middleware.call_credential[0] == b'authorization'
+ assert client_auth_middleware.call_credential[1] == \
+ b'Bearer ' + b'token1234'
+
+
+def test_arbitrary_headers_in_flight_call_options():
+ """Test passing multiple arbitrary headers to the middleware."""
+ with ArbitraryHeadersFlightServer(
+ auth_handler=no_op_auth_handler,
+ middleware={
+ "auth": HeaderAuthServerMiddlewareFactory(),
+ "arbitrary-headers": ArbitraryHeadersServerMiddlewareFactory()
+ }) as server:
+ client = FlightClient(('localhost', server.port))
+ token_pair = client.authenticateBasicToken(b'test', b'password')
+ assert token_pair[0] == b'authorization'
+ assert token_pair[1] == b'Bearer ' + b'token1234'
Review comment:
Addressed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] stevelorddremio commented on a change in pull request #8959: ARROW-11004: [FlightRPC][Python] Header-based auth in clients
Posted by GitBox <gi...@apache.org>.
stevelorddremio commented on a change in pull request #8959:
URL: https://github.com/apache/arrow/pull/8959#discussion_r548055425
##########
File path: python/pyarrow/includes/libarrow_flight.pxd
##########
@@ -307,6 +308,11 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CStatus Authenticate(CFlightCallOptions& options,
unique_ptr[CClientAuthHandler] auth_handler)
+ CResult[pair[c_string, c_string]] AuthenticateBasicToken(
+ CFlightCallOptions& options,
+ const c_string& username,
Review comment:
For the uninitiated, what are the coding standards on Class & variable names? I see a mixture of camel case and underscore separated names.
----------------------------------------------------------------
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:
users@infra.apache.org