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

Keep OAuth2 user info in the encrypted session cookie by default #39967

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Apr 9, 2024

Fixes #39553.

This PR (and a future follow up for #39556) is about improving the user experience when working with OAuth2 providers.

Here is the summary.

OIDC providers give ID token to represent a successful user authentication, so Quarkus keeps it in the encrypted session cookie and keeps verifying ID token's signature with JSON Web Keys fetched from the provider like Google.

However, OAuth2 providers like GitHub do not know what the ID token is and give the access token only (and possibly a refresh token). Quarkus must verify this access token somehow, so it requests a UserInfo representation using this access token from the OAuth2 provider like GitHub. This is correct, but it does so (with a remote call) even when the already authenticated user accesses Quarkus, on every request (as opposed to the OIDC provider authentication described above), thus, for all the social providers, a rate limiting failure is nearly inevitable.

We document that users should either enable a server side UserInfo cache or save it in the encrypted, internally generated ID token. However, this requires users set a property which is nearly always required, in all my demos with OAuth2 providers I have to enable the cache, even if my service endpoint does not need to access some OAuth2 provider specific API. This is not a good user/dev experience.

So this UserInfo cache for OAuth2 providers must really be enabled out of the box. The only question is which type of cache. The in-memory cache requires the extra resources and users should enable it themselves if they need it.
However, inlining the UserInfo in the internally generated and then encrypted ID token is free, the session cookie is created anyway, and this pseudo-cache is controlled by the session's life time.

So this is what this PR does, it saves UserInfo which acts as the OAuth2 access token verification confirmation, in the encrypted session cookie.
But - if the user has disabled the session encryption, or if the server-side UserInfo cache is enabled, then it is off, but out of the box it will go into a secured cookie and users will have fewer worries related to avoiding the rate limiting failures.

I've updated tests to check all of these variations

CC @Sanne

@sberyozkin sberyozkin requested review from FroMage and pedroigor April 9, 2024 15:09
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/oidc labels Apr 9, 2024
Copy link

quarkus-bot bot commented Apr 9, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 9a284c1.

✅ 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.

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

Copy link

quarkus-bot bot commented Apr 9, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9a284c1.

✅ 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.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great new default :)

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great new default :)

@sberyozkin
Copy link
Member Author

Hi @FroMage, thanks, I'll keep it open for today for Pedro @pedroigor to have a look if he'd like.
Fix for #39556 is on the way, with both of these fixes it should become closer to how it works for OIDC providers

@sberyozkin sberyozkin merged commit b8fa2e4 into quarkusio:main Apr 11, 2024
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 11, 2024
@sberyozkin sberyozkin deleted the oauth2_cache_userinfo_in_encrypted_session branch April 11, 2024 10:18
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Apr 11, 2024
Copy link

🙈 The PR is closed and the preview is expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation area/oidc kind/enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Consider enabling UserInfo cache by default for OAuth2 providers
2 participants