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 #5959

Closed
tpenakov opened this issue Dec 5, 2019 · 11 comments
Closed

Keycloak Claim Information Point - NPE when trying to read body #5959

tpenakov opened this issue Dec 5, 2019 · 11 comments
Assignees
Labels
Milestone

Comments

@tpenakov
Copy link

tpenakov commented Dec 5, 2019

When you try to read body through Claim Information Point the result is Null Pointer Exception.

When you add the following to your application.properties file:

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

And execute the post request against the endpoint /api/auth-entry

The result is:

ERROR: HTTP Request to /api/auth-entry failed, error id: d73bb86a-aa3e-40bc-9497-7683a11b1a92-1
java.lang.NullPointerException
	at io.quarkus.keycloak.pep.VertxHttpFacade$1.getInputStream(VertxHttpFacade.java:124)
	at org.keycloak.adapters.authorization.util.RequestPlaceHolderResolver.resolve(RequestPlaceHolderResolver.java:107)
	at org.keycloak.adapters.authorization.util.PlaceHolders.parsePlaceHolders(PlaceHolders.java:93)
	at org.keycloak.adapters.authorization.util.PlaceHolders.resolve(PlaceHolders.java:45)

Here is the Sample Project

You can create the quarkus realm from src/test/resources/keycloack/quarkus-realm.json file.

Then you can retrieve the token for user (alice/alice) as described here: https://quarkus.io/guides/security-keycloak-authorization

Then you have to put the token in org.otaibe.enforcer.claim.can.not.read.body.web.controller.RestControllerTest#TOKEN

If you execute the test org.otaibe.enforcer.claim.can.not.read.body.web.controller.RestControllerTest#testPostEndpoint

Then you will receive the NPE

@tpenakov tpenakov added the kind/bug Something isn't working label Dec 5, 2019
@gsmet
Copy link
Member

gsmet commented Dec 5, 2019

@sberyozkin @pedroigor any chance you could have a look at that one before tomorrow evening?

@pedroigor
Copy link
Contributor

@gsmet Looking ...

@pedroigor pedroigor self-assigned this Dec 5, 2019
@pedroigor
Copy link
Contributor

@gsmet @tpenakov I think I'll not be able to deliver this fix until tomorrow.

Although I managed to fix the original issue so that we can properly read the request body, it is not possible to read the body again by the application. For this one, I'm stuck and I don't have yet a clue how to allow the request inputstream to be read twice (by security handlers + later on by the application).

@sberyozkin @stuartwdouglas Any clue how we can achieve this? Is there some way to buffer the inputstream and/or use a markSupported one ? Or maybe prove a wrapper for vert.x request so that we can buffer the stream ....

@pedroigor
Copy link
Contributor

pedroigor commented Dec 6, 2019

Regarding the original problem (NPE when obtaining the body) the cause of the exception is that the body needs to be read first by a handler (blocking). The solution I found is to reuse the VertxInputStream from Resteasy extension.

@pedroigor
Copy link
Contributor

pedroigor commented Dec 6, 2019

I think I have a prototype that might work. But not sure whether or not it is the best way to achieve it. Here are the changes https://github.com/quarkusio/quarkus/compare/master...pedroigor:issue-5959?expand=1.

Basically, after reading the request from the security layer, we set a buffered stream in the routing context so that later it can be reused. There is a change to Resteasy that checks whether or not the buffered stream is set and if so, uses it instead of trying to read again from the connection.

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
@pedroigor
Copy link
Contributor

@gsmet I have tested changes from @stuartwdouglas and they will serve as a baseline for this fix. Once the PR is merged I'll apply other minor changes and we should be good.

@tpenakov
Copy link
Author

tpenakov commented Dec 6, 2019

@pedroigor - You are referring fix in Resteasy. Is this means that the fix will not going to work for Reactive Routes?

pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
pedroigor pushed a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
@gsmet gsmet added this to the 1.1.0 milestone Dec 6, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 6, 2019
@gsmet gsmet closed this as completed in 5cef24b Dec 6, 2019
gsmet added a commit that referenced this issue Dec 6, 2019
[fixes #5959] - Error when processing request body from policy enforcer
@pedroigor
Copy link
Contributor

@pedroigor - You are referring fix in Resteasy. Is this means that the fix will not going to work for Reactive Routes?

I was just talking about reusing some class to fix the issue, which is part of the PR that fix this issue.

AFAIK, you should also have it working for reactive routes as per changes from Stuart.

mmusgrov pushed a commit to mmusgrov/quarkus that referenced this issue Dec 13, 2019
Fixes quarkusio#5959

This change allows the body handler to be used for Undertow
and RESTEasy standalone. When the request is fully buffered
it can be consumed multiple times, which allows keycloak
to also process it.
mmusgrov pushed a commit to mmusgrov/quarkus that referenced this issue Dec 13, 2019
@tpenakov
Copy link
Author

I've just tested it with 1.1.1.Final and can confirm that is fixed.
Thank you!

@tpenakov
Copy link
Author

@pedroigor - I need your help here :)
I've tried to add a custom ClaimInformationPointProvider. The provider is dummy and just read the request body and log it to the console. It is updated in the same sample test project.
The provider class is: org.otaibe.enforcer.claim.can.not.read.body.cip.FhirClaimInformationPointProvider
I've figured out that the correct way to read the request body and then to be able to reuse it is through
httpFacade.getRequest().getInputStream(true)
It worked fine in the sample test project, but when I've tried to port the same code in the real project it stopped to work.
The reason was:
In the test project httpFacade.getRequest().getInputStream(true) returns ByteArrayInputStream, but on the real one it returns VertxInputStream. The first one allow multiple reads, but the second one doesn't.
After a while I've managed to make my real project to work too, but this was like a magic to me :)
If I add this line to the application.properties file the body is buffered and can be read correctly:

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

After commented the very same line in the sample test project it stopped to work too.

I understand that this workaround with adding the line to the application.properties file will do the trick for me, but it will be great if you help me to enforce the same behavior in the correct way?

Could you please help me with that?

@vamsikrishna-VK31
Copy link

vamsikrishna-VK31 commented Jun 6, 2023

When i was using CIP for api . Request is going twice to Claim information point provider
can i know y?

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