-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
refactor: Generate claims in the same way #522
Conversation
@@ -87,9 +106,6 @@ func (c *IDTokenClaims) ToMap() map[string]interface{} { | |||
ret["amr"] = c.AuthenticationMethodsReference | |||
} | |||
|
|||
ret["iat"] = float64(c.IssuedAt.Unix()) |
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.
So iat
in ID tokens was always added but in access tokens only if set.
I made it so that all fields are not set if empty. I think this is cleaner. I do not think you ever want to issue a token with claim with timestamp set to 0.
@@ -75,7 +75,7 @@ func TestAssert(t *testing.T) { | |||
ToMapClaims().Valid()) | |||
assert.NotNil(t, (&JWTClaims{NotBefore: time.Now().UTC().Add(time.Hour)}). | |||
ToMapClaims().Valid()) | |||
assert.NotNil(t, (&JWTClaims{NotBefore: time.Now().UTC().Add(-time.Hour)}). | |||
assert.Nil(t, (&JWTClaims{NotBefore: time.Now().UTC().Add(-time.Hour)}). |
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.
This changes because ExpiredAt
is not 0 anymore.
@@ -49,16 +49,39 @@ type IDTokenClaims struct { | |||
// ToMap will transform the headers to a map structure |
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 have similar concerns about this as here.
How should we proceed with this PR? |
I will get back to this one shortly. |
Proposed changes
Just a bit of refactoring/code cleanup. Made sure that both OAuth JWT and OpenID JWT claims are generated in the same way. Also made order of field processing in the code the same so it is easier to compare.
Checklist
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.