Skip to content

Conversation

sbrunk
Copy link
Contributor

@sbrunk sbrunk commented Jul 18, 2022

Keycloak has a concept called realm for multi-tenancy.

Realms are part of the uri, so a baseUri looks something like this: https://<domain>/auth/realms/<realm-name>. The issue is that baseUri.withPath ignores this prefix, so I've changed that to baseUri.addPath which should still work fine with other providers.

Keycloak also supports looking up endpoints for login, token, logout etc. through a well-known uri:
https://<domain>/auth/realms/<realm-name>/.well-known/openid-configuration but that would a bit require more work so I've just added a static config like for the other providers.

@majk-p majk-p requested review from majk-p and matwojcik July 18, 2022 11:25
@sbrunk
Copy link
Contributor Author

sbrunk commented Jul 18, 2022

Perhaps my assumption that we can just append to the baseUri was wrong, given the failing test:

https://github.com/ocadotechnology/sttp-oauth2/blob/b834b6f791310d388a9efe138bf6c97839e2e7e7/oauth2/src/test/scala/com/ocadotechnology/sttp/oauth2/AuthorizationCodeSpec.scala#L39-L53

Do other endpoints expect this behaviour?

Copy link
Member

@majk-p majk-p left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, much appreciated! I've enabled the pipeline and it seems the tests have failed. The usage of withPath implicitly assumed that the path would be stripped. I think this is not a big deal and the end users of the library can do it themselves. We would just need the tests updated to reflect the change.

@majk-p
Copy link
Member

majk-p commented Jul 18, 2022

Do other endpoints expect this behaviour?

I think this is just a legacy assumption from when this library was an internal one. I would say this change is fine, just that we'll have to make a breaking release to let the users know they have to strip the path on their own in case someone relied on that behavior.

@sbrunk
Copy link
Contributor Author

sbrunk commented Jul 18, 2022

We would just need the tests updated to reflect the change.

I've just updated the test code to do that. The tests should pass now.

Copy link
Collaborator

@matwojcik matwojcik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for the contribution!

@majk-p majk-p merged commit c8d0ab0 into polyvariant:main Jul 18, 2022
@majk-p majk-p mentioned this pull request Jul 19, 2022
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.

3 participants