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

Introduce OidcResponseFilter #43283

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Sep 13, 2024

This PR introduces OidcResponseFilter to support filtering responses to all quarkus-oidc, quarkus-oidc-client and quarkus-oidc-client-registration calls. With OidcRequestFilter and OidcRedirectFilter already available, all the direct and indirect communication channels with the OIDC server can not be filtered.

I'll clean up and open for review early next week

Copy link

github-actions bot commented Sep 13, 2024

🙈 The PR is closed and the preview is expired.

@sberyozkin sberyozkin marked this pull request as ready for review September 15, 2024 15:41

This comment has been minimized.

@sberyozkin
Copy link
Member Author

sberyozkin commented Sep 15, 2024

The main design challenge from my point of view is whether to allow response filters modify the response body by returning an updated Buffer or not. I think it can be a bit too flexible and I'm not aware right now of any use cases which might require it. Nor I'm aware of other APIs supporting the response body modification (JAX-RS ClientResponseFilter, etc) .

I can only imagine a case where a non-standard property name is used in the response, Quarkus OIDC does not recognize it, and the the filter transforms the body. But it can be controlled at the configuration level if necessary, modifying OIDC responses can introduce an unexpected immediate or later time error in the system.

So, right now, OidcResponseFilter can help to do specialized logging and do all sort of actions given the response status code, headers and body (send events, record some response data values somewhere, etc)

Copy link

quarkus-bot bot commented Sep 15, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 8736dc6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Sep 15, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 8736dc6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin
Copy link
Member Author

The other thing I've realized is that the request context properties visible to the request filters must be visible to the response filters to make them more useful.

For example, both for quarkus-oidc and quarkus-oidc-client, the refresh token grant response will be exactly the same in structure to the main grant's response. With quarkus.oidc, the authorization code flow response will return the id, access and refresh tokens; and so will the refresh token grant.

So being able to check in the response filter which grant was used to return the current tokens is important.
It also gives access to the tenant id, and other relevant properties.

@sberyozkin
Copy link
Member Author

Overall though, the PR changes are simple, given the HttpResponse<Buffer>, get the Buffer, pass it, the headers, the status, and the request context properties to the response filters if any, and then use the original buffer to convert it to JSON or string as required, and this is really all to it.
Tests, docs, have been updated

@pedroigor, @gastaldi, please have a look when you get a chance

@sberyozkin
Copy link
Member Author

sberyozkin commented Sep 17, 2024

Hi Pedro @pedroigor, how does it look to you ? The way OidcResponseFilter is controlled and optionally bound to specific endpoints only is done exactly the same way we discussed with you and did for OidcRequestFilter

@sberyozkin
Copy link
Member Author

Hey @gastaldi @pedroigor, let me merge it now, it is indeed not really about the OIDC logic, so I believe George's approval is sufficient, thanks

@sberyozkin sberyozkin merged commit 0984ac2 into quarkusio:main Sep 19, 2024
41 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 19, 2024
@sberyozkin sberyozkin deleted the oidc-response-filter branch September 19, 2024 12:47
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 19, 2024
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 this pull request may close these issues.

Add OIDC response filters
2 participants