dasniko / testcontainers-keycloak

A Testcontainer implementation for Keycloak IAM & SSO.
Apache License 2.0
327 stars 50 forks source link

Fix default context path #108

Closed daniel-shuy closed 12 months ago

daniel-shuy commented 1 year ago

Currently, the default contextPath, KEYCLOAK_CONTEXT_PATH, is a /, which matches the Keycloak documentation.

The URL returned from getAuthServerUrl() ends with the contextPath: https://github.com/dasniko/testcontainers-keycloak/blob/8f8bb8471447f3af0816e75e3b906da7106e8197/src/main/java/dasniko/testcontainers/keycloak/ExtendableKeycloakContainer.java#L392-L395

If the contextPath is not modified, then the URL returned from getAuthServerUrl() ends with a /, which is problematic.

For example, if I were to configure Spring Security OAuth2 Resource Server with the return value from getAuthServerUrl(), e.g.

var issuerUri = keycloakContainer.getAuthServerUrl() + "/realms/" + realm;
TestPropertyValues.of(
    "spring.security.oauth2.resourceserver.jwt.issuer-uri", issuerUri)
    .applyTo(configurableApplicationContext)

If the contextPath is modified, e.g. /auth, then the example above works. But if the contextPath is not modified, then the example above fails with:

The Issuer "http://localhost:/realms/realm" provided in the configuration metadata did not match the requested issuer "http://localhost://realms/realm"

While it is possible to write code that checks if the returned URL ends with a /, it is much easier if it was simply changed to an empty String. I don't see anywhere in the code that it would make a difference.

dasniko commented 1 year ago

Thanks for this PR, @daniel-shuy I already thought about this in the past, after I did the implementation and published it... Unfortunately this would break implementations and usage of this extension at runtime in users' projects. It's kind of a soft-breaking-change, as not the api itself changed, but the behavior.

Most likely you didn't run the tests after your change, as some of these would fail. And that's exactly what might happen at users' projects tests out there...

So, I'm open to do the change, but currently only with the next major version of this extension.

daniel-shuy commented 1 year ago

I see, I forgot that while internally there would be no difference, it would be a breaking change for those that depend on there being a / at the end of the URL. Reminds me of https://xkcd.com/1172 😆: https://xkcd.com/1172/

Most likely you didn't run the tests after your change, as some of these would fail. And that's exactly what might happen at users' projects tests out there...

My bad, I tested it in my project by calling .withContextPath(""). I didn't run the tests because I made the changes in GitHub without cloning the project since it was a simple change.

So, I'm open to do the change, but currently only with the next major version of this extension.

Ok, that's fine with me, thanks!

dasniko commented 1 year ago

Yes, it's my fault, I've implemented it badly. This is the price of being fast to the market... 😉

daniel-shuy commented 1 year ago

Haha its ok, we all make mistakes. Thanks for this awesome tool!

daniel-shuy commented 12 months ago

@dasniko Thanks for the fix!