search

TNG / keycloak-mock

A Java library to test REST endpoints secured by Keycloak via OpenID connect.
Apache License 2.0
122 stars 27 forks source link

Feature/token route #57

Closed andreconrado closed 1 year ago

andreconrado commented 3 years ago

Extends keycloak-mock handlers with token endpoint.

This endpoint is usefully to use in integration test, for example a application that need to get a access token (with roles) to consume another REST API or to easily test authorized/unauthorized requests.

andreconrado commented 3 years ago

Hello @ostrya,

Did you already have a chance to looking at my merge propose?

Thanks.

ostrya commented 3 years ago

Hi @andreconrado , thanks for your contribution! In general, I like the idea of enhancing the functionality of the mock in the direction of mocking the actual API behavior. One thing which I have been thinking about (and which is the reason I have not come back to you sooner, sorry for that) is that as a prerequisite for mocking the Keycloak API, I would like the mock to actually offer the full API which is currently only available in the standalone module.

If you can wait a bit longer still, I would refactor the code as described to move the REST API code into the mock in preparation. Then with your change the user can decide whether to use the "normal" implementation or some mock responses.

ostrya commented 3 years ago

Hi again, I have prepared a PR of my own for the envisioned refactoring. You can already have a look: https://github.com/TNG/keycloak-mock/pull/72. I hope we can get this merged within the next few days.

andreconrado commented 3 years ago

Hi, I made a brief review and seems ok. Thanks.

ostrya commented 3 years ago

Hi @andreconrado, the refactoring has now been merged, so you can now see if you can rebase your changes.

ostrya commented 3 years ago

First of all: please rebase your branch rather than merging the master branch directly. Also, I think the merge broke the build and removed most of your changes regarding the token route, especially the part where a mock response can be configured.

Furthermore, I am still not 100% clear on what you would like to achieve that goes beyond the default token route implementation. Could you maybe describe your use case for this change?

ostrya commented 3 years ago

Hey @andreconrado, can you please have a look again at my comments above? In the current state, the merge request does not offer any valuable functionality to the user, as the method getTokenRoute which you added now returns the default implementation of the route which does not have any extra methods like withErrorResponse().

First of all, I would really like to understand the use case you have for which you would like to extend the current functionality. Can you please provide a simple example?

Also, please drop the two merge commits in your PR if you want this to be merged, and rebase the remaining commits onto the current upstream master.

ostrya commented 1 year ago

I will close this MR due to lack of feedback. Feel free to re-open it if you feel your use case is valuable, but then please explain it with an example.