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

io.quarkus.oidc.OidcSession uses Instant to present duration #27122

Closed
jie-huang opened this issue Aug 3, 2022 · 6 comments · Fixed by #27336
Closed

io.quarkus.oidc.OidcSession uses Instant to present duration #27122

jie-huang opened this issue Aug 3, 2022 · 6 comments · Fixed by #27336
Labels
area/oidc kind/bug Something isn't working
Milestone

Comments

@jie-huang
Copy link

Describe the bug

In, io.quarkus.oidc.OidcSession (https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcSession.java),
it uses Instant to present duration.

    /**
     * Return an {@linkplain:Instant} indicating how long will it take for the current session to expire.
     *
     * @return
     */
    Instant expiresIn();

Based on Javadoc, it is an instantaneous point on the time-line. But the comment says "how long", it should be Duration.

When using it in app, it seems that the value is filled with Duration too.
e.g. One value I got is "expiresIn": "1970-01-01T00:23:05Z". I guess it means the session has 23 hours and 5 minutes to expire.

Personally, I prefer to use Instant but it should assigned the correct time value (a duration value is a little hard to predict the ending time without base time) and the comment should be updated.

Expected behavior

Using Instant but it should assigned the correct time value (a duration value is a little hard to predict the ending time without base time) and the comment should be updated.

Actual behavior

It uses Instant with the wrong value and comment is misleading.

How to Reproduce?

Just any endpoint with injection of OidcSession.

Output of uname -a or ver

Darwin Kernel Version 21.4.0

Output of java -version

openjdk version "18" 2022-03-2

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.8.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 7.4.1

Additional information

No response

@jie-huang jie-huang added the kind/bug Something isn't working label Aug 3, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 3, 2022

/cc @pedroigor, @sberyozkin

@quarkus-bot quarkus-bot bot added the area/oidc label Aug 3, 2022
@geoand
Copy link
Contributor

geoand commented Aug 4, 2022

I agree, this does not look correct.

However OidcSession is part of the public API, so we can just change it. We likely need to introduce a new method and deprecate the existing one.

@sberyozkin
Copy link
Member

sberyozkin commented Aug 8, 2022

ID token's expiry is a number of seconds since the epoch, see https://openid.net/specs/openid-connect-core-1_0.html#IDToken.
So this method says in how many seconds from the current time the session will expire, it is not meant to return the whole duration during which the token can be valid for, this instant value will be different every time the already authenticated user returns. The Java docs should indeed clarify it.

That expiry date shown in the description looks wrong to me.
Perhaps we can add a new method returning a precise Date when the session will expire

@jie-huang
Copy link
Author

After checking the implementation (https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcSessionImpl.java)

    @Override
    public Instant expiresIn() {
        final long nowSecs = System.currentTimeMillis() / 1000;
        return Instant.ofEpochSecond(idToken.getExpirationTime() - nowSecs);
    }

It returns actually a duration (from the meaning, not Java class) when the method is triggered.

@sberyozkin
Copy link
Member

Hmm. It does look wrong to me now, as indeed it returns a duration from the epoch, it was really meant to be idToken.getExpirationTime() - nowSecs only. What this method returns now is indeed of no much use, the idea was to give user a hint when, starting from now, the session will expire, in 30 mins or in 2 hours etc.

I'll fix it as proposed by Georgios

@sberyozkin
Copy link
Member

sberyozkin commented Aug 16, 2022

Actually, I'll keep this method undeprecated and just return an Instant representing the expiration time found in Id Token - which is exactly what Instant.ofEpochSecond is for, return Instant.ofEpochSecond(idToken.getExpirationTime()).
But indeed, will also add a method returning Duration capturing an idToken.getExpirationTime() - nowSecs interval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants