Added CodeFlowAuthorizationTest #14173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cemnura
changed the title
sberyozkin
reviewed
Jan 8, 2021
integration-tests/oidc-wiremock/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
sberyozkin
reviewed
Jan 8, 2021
integration-tests/oidc-wiremock/src/main/resources/application.properties
Show resolved
Hide resolved
sberyozkin
reviewed
Jan 8, 2021
integration-tests/oidc-wiremock/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
sberyozkin
reviewed
Jan 8, 2021
...tion-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java
Outdated
Show resolved
Hide resolved
|
What did you mean by setting application-type=hybrid in all 3 I converted the application-type to Setting the application-type to The The same problems occures for Could this be a missing configuration or mock again? |
|
@cemnura oh, I'm sorry, for the moment I thought it was a quarkus unit test setup where per every test we'd have its own application properties. Now, if we want to test the code flow, then we can't replace the application type from
Would you like to try the multi-tenancy ? Lets see how it goes, keep the original configuration intact, that would ensure both existing bearer token tests continue passing Next update the new config like this: note but do not use a query check, use the path to check if it ends with the And finally - update the test code and endpoint for the request URIs to end with and that is it :-) Give it a try please |
|
Tenant configuration sounds like a good idea that way we can separate the test. I will try to configure the test with tenant before considering to making a separate module. For the sake of not duplicating code 😄 |
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
from
11db6c8 to
6d889b6
Compare
|
I added the following configuration Yet this lead to the following error. It is as if the dynamic configuration that is loaded here by the |
cemnura
commented
Jan 20, 2021
Comment on lines
28
to
30
| WebResponse webResponse = webClient | ||
| .loadWebResponse(new WebRequest(URI.create("http://localhost:8081/index.html").toURL())); | ||
| verifyLocationHeader(webClient, webResponse.getResponseHeaderValue("location"), null, "web-app", false); |
There was a problem hiding this comment.
Also during the CodeFlowAuthorizationTest test the response header location value does return when accessing the static file at http://localhost:8081/index.html
Would you have any suggestions for how we could get the redirect value in the response header location?
There was a problem hiding this comment.
@cemnura Lets remove index.html resource and simply have a code flow endpoint with the path ending with /code-flow and use it here and it should be fine :-) (or you can also update the resolver to check if the path also ends with index.html)
There was a problem hiding this comment.
I will give it a shoot and get back to you. I think that the code-flow solution would be more logical 😄
integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java
Show resolved
Hide resolved
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
from
ffc92c0 to
ed9ec53
Compare
|
Hi @cemnura re that connection error: |
Unfortunately no. I thought that maybe the dynamic configuration of So I changed the returned configuration here to the following and this solved the problem. It is as if when the Would you consider this as a reasonable fix? |
|
Hi @cemnura I'm not sure about it; looks like we are missing something here. What I don't understand is why it works when the default configuration is used - i.e - how does it pick up a correct Wiremock endpoint - while a few lines down the same property is initialized with the default value... should it be ? |
Hello @sberyozkin I added the configuration check here to the properties that are dynamically with the test KeycloakTestResource and it enables previous test to work without a problem. |
|
Now the only the codeflow test has to be complete. I will get back to you. Thanks again for your help |
|
I was able to get the |
sberyozkin
reviewed
Jan 27, 2021
integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/KeycloakTestResource.java
Show resolved
Hide resolved
sberyozkin
reviewed
Jan 27, 2021
...tion-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java
Outdated
Show resolved
Hide resolved
sberyozkin
reviewed
Jan 27, 2021
integration-tests/oidc-wiremock/src/main/resources/META-INF/resources/index.html
Outdated
Show resolved
Hide resolved
|
@cemnura Hi, thanks, we are nearly there, please address a few minor cleanup comments; so the last difficult part is to emulate the challenge. I've commented about it in the issue, copying it here: and Does it help ? |
sberyozkin
reviewed
Jan 27, 2021
cemnura
commented
Feb 5, 2021
There was a problem hiding this comment.
@cemnura Hi, thanks, we are nearly there, please address a few minor cleanup comments; so the last difficult part is to emulate the challenge. I've commented about it in the issue, copying it here:
The authorization endpoint - this is where the main complexity will be. When this endpoint is invoked (GET request) it needs to capture the state and redirect_uri query parameters and return the HTML form challenge with the fields as expected by the HtmlUnit test - more on it below - and this form should be able to **submit to another (non-discoverable) WireMock endpoint** - which would redirect back to Quarkus. This HTML form should likely be used to keep those state and redirect_uri values as the hidden form properties - or may be you can use the cookies to make it simpler. Next, once HtmlUnit submits the form (username and password) - you'd redirect back to the value saved in redirect_uri but also add the saved state parameter as a query parameter and also add a code query parameter. Next Vert.x would issue a code grant request (POST form) to the token endpoint - where that code and redirect_uri would be present - here you'd return the id token (JWT) - this token will be used to get the alice name, access token (JWT) and refresh token.and
Note the name of the form fields(used by the HtmlUnitTest) - these areusernameandpasswordDoes it help ?
So we need to add a html response via WireMock when quarkus hits /auth/realms/quarkus?redirect_uri=http%3A%2F%2Flocalhost%
If yes is there any example HTML?
And we will have to update the endpoint to mock the login request?
integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/KeycloakTestResource.java
Outdated
Show resolved
Hide resolved
sberyozkin
reviewed
Feb 17, 2021
integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/KeycloakTestResource.java
Outdated
Show resolved
Hide resolved
sberyozkin
reviewed
Feb 17, 2021
integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/KeycloakTestResource.java
Show resolved
Hide resolved
|
In all honesty I never been more happier to be able to login to an app 😄 |
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
2 times, most recently
from
c61c740 to
af13405
Compare
|
rebased 🎉 |
cemnura
changed the title
cemnura
changed the title
|
@cemnura I think we will resolve the linked issue with this PR, I can then open a new issue to have this Witemock code re-usable... thanks |
|
great to hear 🎉 I will re run the CI checks. |
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
2 times, most recently
from
dfba954 to
76970f6
Compare
|
Hey @cemnura Can you please check my 2 comments at https://github.com/quarkusio/quarkus/pull/14173/files#r580941716 ? |
|
I added Here is the URL request and response logs from wiremock. |
|
@cemnura |
|
@sberyozkin I added a stub for How should redirect the application to the '/token' URL after clicking the Login button. Currently the login button action is to make a request to Thanks for your patience and help 😄 |
|
Hi @cemnura sure, after Wiremock returns the redirect request to the HtmlUnit browser, it follows it to Quarkus - this is where it will send a direct |
|
@cemnura see the whole sequence here https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth |
|
Hello @sberyozkin I managed to correctly redirect with Yet the login page is reappearing Even though the token is being introspected. Shouldn't the quarkus app be responding at this point? Logs after |
|
Full logs |
|
@cemnura I'm not sure 100% but it looks like the problem is that you are returning the wrong token values:
|
|
@cemnura please also resolve the conflicts - though please do it after the test is working :-) |
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
2 times, most recently
from
8687db9 to
a972c78
Compare
|
Hey @cemnura Yes, please note it is better to generate the tokens from the stub endpoint - or generate them at the Wiremock init time and make them last forever :-) |
|
@sberyozkin you were right the access token was expired long before I was even able to run the first test yet alone further iterations. I dynamically added the There might be a few topics that we might need to discuss. Currently, I am creating We may need to make this dynamic according to the desired authenticated user. But how would we get the userName during the request to the token endpoint? |
|
@cemnura This is excellent work - thanks for trying hard to make it work and actually making the test pass :-) thanks |
Signed-off-by: Cem Nura <[email protected]>
cemnura
force-pushed
the
feature/oidc-wiremock-codeflow
branch
from
7d0cf6c to
f987e6a
Compare
|
🎉 Removed unused methods and rebased 😄 |
|
@cemnura thanks, merging now - I'll ping you later about other possible issues - I might follow with a quick modularization and doc update - but there are plenty of issues there :-) |
sberyozkin
approved these changes
Mar 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10412