Skip to content
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

Keycloak Claim Information Point - NPE when trying to read body - 1.3.1.Final #8478

Closed
tpenakov opened this issue Apr 8, 2020 · 11 comments · Fixed by #8524
Closed

Keycloak Claim Information Point - NPE when trying to read body - 1.3.1.Final #8478

tpenakov opened this issue Apr 8, 2020 · 11 comments · Fixed by #8524
Labels
Milestone

Comments

@tpenakov
Copy link

tpenakov commented Apr 8, 2020

It is the same bug described in #5959.
After upgrading to 1.3.1.Final it does not working anymore.
You can use the sample project from #5959. Only one change is done there - because of the #8464.
You can run the sample project and to verify that is working.
Then change the quarkus version to 1.3.1.Final and the project will stop working

@tpenakov tpenakov added the kind/bug Something isn't working label Apr 8, 2020
@pedroigor
Copy link
Contributor

@tpenakov That is weird. We do have tests to check this behavior which was added when fixing #5959. I'll try to find out what is missing. Nothing else to add, then ?

@sberyozkin
Copy link
Member

Hi Pedro, @pedroigor, created area/keycloak-authorization as proposed, I'll go through other issues and update them too, thanks

@tpenakov
Copy link
Author

tpenakov commented Apr 9, 2020

Hi @pedroigor ,
It will be great to resolve it if possible, because in that way I will not need to wait for the new Quarkus release.
Also could you please check my comment/question from #5959? The comment/question is here. IMO it is important not to rely of {request.body} if you plan to use the body only in Keycloak SPI. Also in that way the entire body is sent to the Keycloak, but this is not required in most cases.

@pedroigor
Copy link
Contributor

@tpenakov I see now. It seems the behavior can be explained due to the fact that when you define a body CIP in your configuration, the extension produces a RequireBodyHandlerBuildItem which was added as a fix for extensions that need to read the body prior to invoking the application while still allowing the application to also read the body.

The RequireBodyHandlerBuildItem is a marker that configures a specific handler into the chain so that the body can be read multiple times.

It seems though that you can have the same behavior if your method is annotated with @Route. But I'm not sure.

In any case, I think that for this particular issue we are OK given that the body CIP is working fine. Agree?

If we want to support reading the body as we do with the body CIP I think you could either try to use the @Route approach (if that makes sense for your application) or have a separated enhancement to the keycloak-authorization extension asking to enable this behavior based on a configuration property.

@tpenakov
Copy link
Author

tpenakov commented Apr 9, 2020

Thank you @pedroigor !
So if I wrote a simple extension on my end which will produce a RequireBodyHandlerBuildItem, this will workaround the problem with missing {request.body} cip? Right?

About is the body CIP is working fine - My point of view as a developer is that in addition we need to have an explicit key that enable/disable body reusing. May be something like quarkus.vertx.reuse-request-body instead of tide it to the particular functionality only, which can leads to the unexpected results.

Did you have chance to look at my initial complain in this bug? After I have upgraded to 1.3.1.Final my microservice stopped to work. Because I have deadlines, which I have to keep as a temporary solution I reverted back to 1.2.1.Final where this problem together with #8464 is not present.
You can just start the the Sample Project as described in #5959 .
You can verify that is working fine, and then you can change the Quarkus version to 1.3.1.Final and will see that the project will stop working.

@pedroigor
Copy link
Contributor

@tpenakov I think I found the issue. Sent a PR. If you could try it out, I appreciate.

The issue is that the workaround I did for an issue in Keycloak (see KEYCLOAK-12412) was expecting a content-type to be set.

Another thing you can try is also to force a content-type. Which should not give you that error.

pedroigor added a commit to pedroigor/quarkus that referenced this issue Apr 10, 2020
@pedroigor
Copy link
Contributor

@stuartwdouglas Do you think that makes sense to have a configuration to enable the body handler, as suggested by @tpenakov ?

pedroigor added a commit to pedroigor/quarkus that referenced this issue Apr 10, 2020
@tpenakov
Copy link
Author

Thank you @pedroigor !
I assume that this will go live in 1.3.3.Final or 1.4.0.Final?

Just for the info - I've created a simple re-read-http-request-body Quarkus extension and now I can comment the row:

quarkus.keycloak.policy-enforcer.claim-information-point.claims.claim-from-body={request.body}

from the application.properties file.

@stuartwdouglas
Copy link
Member

I don't know about config to enable the body handler specifically, if it is needed but is not present then this is likely a bug on our side that we would want to fix.

iocanel pushed a commit to iocanel/quarkus that referenced this issue Apr 14, 2020
@tpenakov
Copy link
Author

Hi @stuartwdouglas and @pedroigor ,
I need a functionality to enable body re-read. Currently I am using this small extension which works fine for me. If you think that this might be useful - I can contribute with such functionality?

@tpenakov
Copy link
Author

tpenakov commented May 4, 2020

Hi @pedroigor ,
I've upgraded to 1.4.1 and still does not works for me.
You can try to run the initial test project (updated to 1.4.1.Final). It freezes after the line:

will try to read body for the second time

Created #9054

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants