-
Notifications
You must be signed in to change notification settings - Fork 1k
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: respect authentication.skip.paths properly #9224
Conversation
} | ||
|
||
@Override | ||
public void handle(final RoutingContext routingContext) { | ||
|
||
final String path = routingContext.normalisedPath(); | ||
|
||
if (KSQL_AUTHENTICATION_SKIP_PATHS.contains(path)) { | ||
if (unauthenticatedpaths.matcher(path).matches()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, so AUTHENTICATION_SKIP_PATHS_CONFIG was not taken into account at all before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the FIX @agavra !
Changes LGTM
This should also land to 7.0.x etc right? |
@agavra Thanks a lot for the fix! |
@pgaref Once this PR is merged to 6.0.x, we need to |
@cadonna there are no existing tests for these classes - I will add tests for EDIT: I see what you mean about testing handle. I'll test that to make sure it picks up the right skip paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agavra Thanks for the update!
I have two comments regarding the tests.
final Pattern skips = AuthenticationPluginHandler.getAuthorizationSkipPaths(configs); | ||
|
||
// Then: | ||
assertThat(skips.matcher("/heartbeat").matches(), is(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not also verify for /lags
?
Could you also add a negative case, like /shouldnotbefound
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not also verify for /lags?
that doesn't add any coverage, I'm ensuring that what is added in the config is able to skip
Could you also add a negative case, like /shouldnotbefound?
Sure I'll add a negative test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that doesn't add any coverage, I'm ensuring that what is added in the config is able to skip
I cannot follow. Why does verifying for /lag
not add any coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding /lag
in the config list makes sure that the regex works when you have multiple components - I'm not sure why checking that it works for /lag
adds any coverage beyond checking for /heartbeat
(basically it uses the same exact code path to check for /lag
as it does to check for /heartbeat
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional verification would check that the code works for all entries in the config. The code could not consider the other entry. Unit test should not only test the current code but they should also ensure that future refactorings of the code do not change the intended behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I can see your point :) I'll add it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private RoutingContext routingContext; | ||
|
||
@Test | ||
public void shouldRespectServerAuthSkipPathsConfig() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a case where authentication is not skipped?
Description
fixes #9206 by respecting the
authentication.skip.paths
inKsqlAuthorizationProviderHandler.java
Testing done
spun up CFK with MTLS and RBAC enabled on the normal endpoints. see relevant portion of configuration:
setting up unit testing for this is nigh impossible and the fix is blocking production users
Reviewer checklist