-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
server: add expires_in field to the response of token endpoint #575
server: add expires_in field to the response of token endpoint #575
Conversation
@@ -537,6 +538,7 @@ func handleTokenFunc(srv OIDCServer) http.HandlerFunc { | |||
IDToken: jwt.Encode(), | |||
TokenType: "bearer", | |||
RefreshToken: refreshToken, | |||
ExpiresIn: int64(session.DefaultSessionValidityWindow.Seconds()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like assuming that we're always using the default validity window, since it ties it to a global variable rather than this server's configuration. Ideally, CodeToken
, ClientCredsToken
, and RefreshToken
would all return the JWT's expiry in it's function signature and the actual configuration of this value lives within those functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I'm going to implement an another way.
Fixed the codes. |
@@ -49,13 +49,13 @@ type OIDCServer interface { | |||
Login(oidc.Identity, string) (string, error) | |||
|
|||
// CodeToken exchanges a code for an ID token and a refresh token string on success. | |||
CodeToken(creds oidc.ClientCredentials, sessionKey string) (*jose.JWT, string, error) | |||
CodeToken(creds oidc.ClientCredentials, sessionKey string) (*jose.JWT, string, int64, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of int64, please return a time
package type to indicate the exact meaning of the returned value. For instance, you can return a time.Time
here then convert using int64(t.Sub(time.Now()))
to "expired in".
Edit: fixed the time math
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback! I'll fix that to use time.Time
.
Fixed the code to use time.Time. If you have any other concerns, please let me know. |
Thanks for the work. lgtm! |
This PR adds the expires_in field to the response of token endpoint.
Related with #574.