Skip to content

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Jan 1, 2025

This PR follows #45302 and is a 2nd preparation PR to address #32109.

In #45302 I've only restructured OIDC DefaultTokenStateManager to make it easier to understand how ID, access, and refresh tokens are concatenated before being encryped as a single session cookie or as individual cookies.

The plan was, to follow #45302 with the introduction of JSON into DefaultTokenStateManager, to make it easier to manage more token properties. I've created a branch, see this commit. But after thinking about it, I decided to postpone it because:

  • For 3 tokens, and an extra property, and with the encryption, the session cookie gets about 40 extra characters
  • And if the encryption is disabled for some internal networks, the size would increase by about 30% because JSON has to be base64url-encoded, and with tokens being quite large, it can easily go over 4K in size

I then looked again at the code, and really, after #45302, it looked much easier to me just to add one more property, access token expires_in to the String. In fact, it is likely to be the last property we will add to the session cookie content.

So this PR only adds an access token expires_in property to the session cookie String payload, if it is available, or an empty string if not, and updates tests to confirm the access token expires_in property is available in the session cookie.

Next PR will have #32109 resolved.

@michalvavrik, @pedroigor FYI

@sberyozkin sberyozkin requested a review from gastaldi January 1, 2025 18:32
@quarkus-bot quarkus-bot bot added the area/oidc label Jan 1, 2025
@sberyozkin
Copy link
Member Author

sberyozkin commented Jan 1, 2025

The OIDC deployment test which uses Keycloak 19 (WildFly based distro) had to be tuned because the session cookie is already close to 4K there so now it is split into chunks with having an extra 2/3 characters for capturing and encrypting the expires in property. It will be reverted once I finalize moving all the tests to latest Keycloak based on Quarkus

@sberyozkin
Copy link
Member Author

I fixed a link to the branch commit where I tried JSON

Copy link

quarkus-bot bot commented Jan 1, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 2e34eef.

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


Flaky tests - Develocity

⚙️ Maven Tests - JDK 17

📦 integration-tests/devmode

io.quarkus.test.devui.DevUIGrpcSmokeTest.testTestService - History

  • Too many recursions, message not returned for id [752393715] - java.lang.RuntimeException
java.lang.RuntimeException: Too many recursions, message not returned for id [752393715]
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:164)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)

@sberyozkin sberyozkin merged commit 85d9208 into quarkusio:main Jan 2, 2025
23 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Jan 2, 2025
@sberyozkin sberyozkin deleted the oidc_session_cookie_access_token_expiry branch January 2, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants