GH-445: [Flight] Add middleware for auth2 bearer tokens #503
Conversation
devinrsmith
requested review from
lidavidm,
laurentgo and
wgtmac
as code owners
There was a problem hiding this comment.
I think this might be useful, but we probably need to sit down and sort out all the existing auth options...there are too many at this point and it's unclear how each of them work without having to go read every implementation (I think this PR does not overlap with any of them, but it's hard to make sure)
I think this needs at least a basic test using an in-process server. For instance the server could track the last token it received, and the test could assert that the client and the server see the 'correct' token as it is changed (on either side).
|
|
||
| public static class Factory implements FlightClientMiddleware.Factory { | ||
|
|
||
| private volatile String bearerValue; |
There was a problem hiding this comment.
I think AtomicReference would be more appropriate here
| } | ||
|
|
||
| void setBearerValue(String bearerValue) { | ||
| // Avoid volatile write if the value hasn't changed |
There was a problem hiding this comment.
Doesn't this optimization introduce a TOC/TOU error?
Rationale for this change
This makes it easier to work with auth2 from the Java client without needing to resort to ClientCookieMiddleware (ie, don't need to force the server to support a non-standard auth scheme). I think it could be highly beneficial for ease-of-use when directly creating a FlightClient, and I would expect that the various FlightSqlClient drivers (ADBC, JDBC) could benefit from supporting it as a configuration option.
What changes are included in this PR?
A new FlightClientMiddleware had been added.
Are these changes tested?
These changes have been manually tested; if more testing is necessary, I'm hoping another contributor can provide it or guide me through a test plan.
Note: this is a re-creation of #446, I think some codeowner logic was not in place when I originally created the PR.