Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add getAuthToken method to AuthenticationPlugin interface #9239

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

nateab
Copy link
Member

@nateab nateab commented Jul 5, 2022

Description

Websocket pull queries that were forwarded to a different node were not correctly extracting their required credentials from the original pull query request. This PR fixes this by adding a getAuthToken method to the AuthenticationPlugin interface, with the default being the original behaviour of checking only for an Authorization header, and plugins being able to add any custom authentication they need for multi-node websocket pull queries.

Testing done

Integration test added

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@nateab nateab requested a review from a team as a code owner July 5, 2022 21:50
@AlanConfluent
Copy link
Member

Just to understand a bit better, do we have a case where the WS was used with an authentication plugin where the header wasn't what was used for authentication? Does this have anything special to do with the WebSocket vs any other interface? Just to be clear, when the requests are forwarded, they are done using the /query endpoint regardless of which they came in on.

@@ -46,4 +46,9 @@ public interface AuthenticationPlugin {
CompletableFuture<Principal> handleAuth(RoutingContext routingContext,
WorkerExecutor workerExecutor);


default String getAuthToken(final RoutingContext routingContext) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an additional comment here about how this is different from handleAuth. This is a little funny because we expose the auth token (as opposed to just treating it as entirely internal to the plugin, as handleAuth does) in addition to exposing handleAuth, since we need it for the purpose of forwarding on requests inter-node as using it as a backup header for connect. Since we need it for those two purposes, I don't think we can easily hide it in a plugin method like prepareRequest().

Also, I assume that since this is default, it shouldn't break any existing plugins.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a javadoc explaining why we need to expose this method. And yes the default just defaults back to the old logic so that nothing existing should break.

@nateab
Copy link
Member Author

nateab commented Jul 6, 2022

@AlanConfluent the internal jira that I slacked you has more context about how we identified the problem, but essentially it is because the WS requests do not use Authorization headers for authorization, they use either query params or cookies. So when the request was forwarded, it would only check the header which would be null since we didn't use it in the original WS request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants