fix: Fixes auth for forwarded requests for pull queries #6895
Conversation
| @@ -392,11 +400,24 @@ public static void makeAdminRequest(TestKsqlRestApp restApp, final String sql) { | |||
| RestIntegrationTestUtil.makeKsqlRequest(restApp, sql, Optional.empty()); | |||
| } | |||
|
|
|||
| public static void makeAdminRequest( | |||
There was a problem hiding this comment.
I'm curious why we have a makeAdminRequest and makeAdminRequestWithResponse here? Both of them call the same callee. Could we just have one makeAdminRequest that always returns (a potential empty list), and its callers can just ignore the returned value or not?
There was a problem hiding this comment.
Sorry, just noticed your question. I don't think there's a reason to have two separate methods. I agree with you. I'll do a quick followup PR to fix this.
| @@ -122,6 +129,19 @@ | |||
| SerdeFeatures.of() | |||
| ); | |||
|
|
|||
| private static final String PROPS_JAAS_REALM = "KsqlServer-Props"; | |||
| private static final String KSQL_RESOURCE = "ksql-user"; | |||
| private static final String USER_WITH_ACCESS = "harry"; | |||
There was a problem hiding this comment.
do we have a test trying out a user with no access? that will make sure that we haven't accidentally just changed it to allow all users 😆
There was a problem hiding this comment.
We do have tests elsewhere that test auth for pull queries and check the negative case.
Co-authored-by: Alan Sheinberg <[email protected]>
Description
The PR #6665 made
HARoutinga singleton. Unfortunately, it also kept a reference to the service context rather than using the user-aware context. What this meant is that all forward requests did not provide authentication and failed. This didn't fail in testing because it requires more than two nodes to expose the forwarding failure, since HA makes the request fall back to the fowarder itself. For that reason, I made the routing functional test use auth, which does test the three node case and was able to expose the bug.Testing done
PullQueryRoutingFunctionalTestwith auth.Reviewer checklist