Deprecate OidcSession#expiresIn and add new methods #27336
Conversation
|
Thanks @gastaldi :-) |
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcSession.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
oidc_session_expires_in_fix
branch
from
d3ca366
to
a30deb7
Compare
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcSession.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
oidc_session_expires_in_fix
branch
2 times, most recently
from
b52ece4
to
a9d4120
Compare
| Instant expiresIn(); | ||
|
|
||
| /** | ||
| * Return an {@linkplain Instant} representing the current session's expiration time | ||
| * which is a number of seconds from the epoch of 1970-01-01T0:0:0Z. |
There was a problem hiding this comment.
Is this javadoc correct? It looks like the javadoc of expiresIn? I thought we decided to display the actual expiration time here?
which is a number of seconds from the epoch of 1970-01-01T0:0:0Z looks weird.
There was a problem hiding this comment.
@gsmet It is correct, this is what the exp claim value is which is what this Instant captures. System.currentTime() also uses the same epoch as a base
There was a problem hiding this comment.
I don't understand really. Isn't this method supposed to return the real expiration time and not something weird? The doc seems to indicate it will be 1970 + 30 minutes (typically) instead of the real expiration time?
I thought it was the original issue we were trying to solve so I might miss something entirely.
There was a problem hiding this comment.
@gsmet It is just the way the expiration time is handled for JWT tokens, the epoch is a base. if we take from some other base then we won't be able to correctly compare it against the current time (which is also calculated from the epoch).
From the exp claim definition in the OIDC spec:
Expiration time on or after which the ID Token MUST NOT be accepted for processing. ... Its value is a JSON number representing the number of seconds from 1970-01-01T0:0:0Z".
The same for issuedAt time, etc.
The doc seems to indicate it will be 1970 + 30 minutes
It will be more than 30 mins, it will be more than the current time. If it is less than the current time then the session (token) has expired.
There was a problem hiding this comment.
@gsmet, if you'd like you can do mvn quarkus:dev in security-openid-connect-quickstart and go to the OIDC devUI card, login to KC as alice:alice and check the exp value in the displayed access or ID token...
There was a problem hiding this comment.
Removed the ref to the epoch time to avoid the confusion as agreed with Guillaume
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcSession.java
Outdated
Show resolved
Hide resolved
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcSession.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
oidc_session_expires_in_fix
branch
2 times, most recently
from
b81f652
to
6bdd606
Compare
sberyozkin
changed the title
This comment has been minimized.
This comment has been minimized.
sberyozkin
force-pushed
the
oidc_session_expires_in_fix
branch
from
6bdd606
to
5d5db12
Compare
sberyozkin
force-pushed
the
oidc_session_expires_in_fix
branch
from
5d5db12
to
4677e9e
Compare
gsmet
added
the
triage/waiting-for-ci
quarkus-bot
bot
added
kind/bugfix
and removed
triage/waiting-for-ci
Fixes #27122.
This PR:
Instantrepresentation of the token expiration timeTesting for a precise duration value is not easy, it is 3 secs (id token lifespan), but I'd like to avoid some random test failures, I think allowing for a ">1 && < 5" margin is reasonable, even for practical purposes.
@geoand Have a look please when you are back :-).