-
Notifications
You must be signed in to change notification settings - Fork 27
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
Added support for disabling /auth context path. Resolves #126 #143
Conversation
Issue: TNG#126 Signed-off-by: kilmajster <lukasz.createam@gmail.com>
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.
Thank you for your contribution. In general I like the proposal. But please check your commits against the google formatter (e.g. by using the pre-commit hook).
mock/src/main/java/com/tngtech/keycloakmock/impl/UrlConfiguration.java
Outdated
Show resolved
Hide resolved
mock/src/test/java/com/tngtech/keycloakmock/impl/UrlConfigurationTest.java
Outdated
Show resolved
Hide resolved
Issue: TNG#126 Signed-off-by: kilmajster <lukasz.createam@gmail.com>
Thanks for the remarks @ostrya, I pushed my updates, please have a look :) |
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.
Sorry for not noticing it last time, just two tiny remarks. Otherwise looks good now.
mock/src/main/java/com/tngtech/keycloakmock/impl/UrlConfiguration.java
Outdated
Show resolved
Hide resolved
@@ -301,6 +301,7 @@ void unexpected_issuer_causes_exception() { | |||
Builder builder = aTokenConfig(); | |||
assertThatThrownBy(() -> builder.withSourceToken(TOKEN_WITH_UNEXPECTED_ISSUER_URL)) | |||
.isInstanceOf(IllegalArgumentException.class) | |||
.hasMessageContaining("did not conform to the expected format"); | |||
.hasMessageContaining("did not conform to the expected format") | |||
.hasMessageContaining("'http[s]://$HOSTNAME[:port][:contextPath]/realms/$REALM'"); |
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 colon in front of port
was not meant as an indicator for parameter replacement but as part of the string, e.g. ":8080". Thus, I'd prefix the context path with a slash instead. To make it more clear, I think we can also use the same variable replacement syntax as for the hostname and the realm:
.hasMessageContaining("'http[s]://$HOSTNAME[:port][:contextPath]/realms/$REALM'"); | |
.hasMessageContaining("'http[s]://$HOSTNAME[:$PORT][/$CONTEXT_PATH]/realms/$REALM'"); |
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 explanation, now this format makes more sense.
Signed-off-by: Kai Helbig <kai.helbig@tngtech.com>
Signed-off-by: Kai Helbig <kai.helbig@tngtech.com>
I can see you already applied those suggestions, that's great! So I guess it's good to go now? 😄 |
Yes, just forgot to click merge because I wanted to wait for the checks to get green. |
Hi, I've added support for disabling or overriding
/auth
context path so that mock can be used with Keycloak 18.0.0+