-
Notifications
You must be signed in to change notification settings - Fork 364
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
fix!: NumericDate parsing conformance #9
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,4 +10,13 @@ go: | |
- 1.5 | ||
- 1.6 | ||
- 1.7 | ||
- 1.8 | ||
- 1.9 | ||
- 1.10 | ||
- 1.11 | ||
- 1.12 | ||
- 1.13 | ||
- 1.14 | ||
- 1.15 | ||
- 1.16 | ||
- tip |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
package jwt | ||
|
||
import ( | ||
"testing" | ||
"time" | ||
) | ||
|
||
func Test_StandardClaims_VerifyExpiresAt_empty(t *testing.T) { | ||
c := StandardClaims{} | ||
if !c.VerifyExpiresAt(time.Now(), false) { | ||
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", true, false) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyExpiresAt_expired(t *testing.T) { | ||
c := StandardClaims{ | ||
ExpiresAt: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123, | ||
} | ||
if c.VerifyExpiresAt(time.Now(), true) { | ||
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", false, true) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyExpiresAt_not_expired(t *testing.T) { | ||
c := StandardClaims{ | ||
ExpiresAt: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123, | ||
} | ||
if !c.VerifyExpiresAt(time.Now(), true) { | ||
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", true, false) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyIssuedAt_empty(t *testing.T) { | ||
c := StandardClaims{} | ||
if !c.VerifyIssuedAt(time.Now(), false) { | ||
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", true, false) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyIssuedAt_expired(t *testing.T) { | ||
c := StandardClaims{ | ||
IssuedAt: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123, | ||
} | ||
if c.VerifyIssuedAt(time.Now(), true) { | ||
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", false, true) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyIssuedAt_past(t *testing.T) { | ||
c := StandardClaims{ | ||
IssuedAt: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123, | ||
} | ||
if !c.VerifyIssuedAt(time.Now(), true) { | ||
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", true, false) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyNotBefore_empty(t *testing.T) { | ||
c := StandardClaims{} | ||
if !c.VerifyNotBefore(time.Now(), false) { | ||
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", true, false) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyNotBefore_expired(t *testing.T) { | ||
c := StandardClaims{ | ||
NotBefore: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123, | ||
} | ||
if c.VerifyNotBefore(time.Now(), true) { | ||
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", false, true) | ||
} | ||
} | ||
|
||
func Test_StandardClaims_VerifyNotBefore_passed(t *testing.T) { | ||
c := StandardClaims{ | ||
NotBefore: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123, | ||
} | ||
if !c.VerifyNotBefore(time.Now(), true) { | ||
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", true, false) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,6 @@ import ( | |
"bytes" | ||
"crypto/rsa" | ||
"fmt" | ||
"github.com/dgrijalva/jwt-go" | ||
"github.com/dgrijalva/jwt-go/request" | ||
"io" | ||
"io/ioutil" | ||
"log" | ||
|
@@ -17,6 +15,9 @@ import ( | |
"net/url" | ||
"strings" | ||
"time" | ||
|
||
"github.com/dgrijalva/jwt-go" | ||
"github.com/dgrijalva/jwt-go/request" | ||
) | ||
|
||
// location of the files used for signing and verification | ||
|
@@ -150,7 +151,7 @@ func createToken(user string) (string, error) { | |
&jwt.StandardClaims{ | ||
// set the expire time | ||
// see http://tools.ietf.org/html/draft-ietf-oauth-json-web-token-20#section-4.1.4 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we are at: Changing this reference to https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.4 would be appropriate as well. That is btw the source of the "wrong" implementation. When the library was initially started, RFC7519 did not exist - only its drafts. The referenced draft actually stated that |
||
ExpiresAt: time.Now().Add(time.Minute * 1).Unix(), | ||
ExpiresAt: float64(time.Now().Add(time.Minute * 1).Unix()), | ||
}, | ||
"level1", | ||
CustomerInfo{user, "human"}, | ||
|
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 submitting a PR.
I believe one of the goals here is to maintain backwards compatibility with the upstream repo. I.e., make it a drop-in replacement.
Instead of changing the
StandardClaims
struct, would it suffice to add a new struct with an explanation and/or a deprecation notice?New users would be pushed towards the "correct struct", while existing users are unaffected but have the option to update their implementations?
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 would be one way to go. I'm feeling very urged to fix the current one but as we want to maintain the drop-in contract in order to ease the transition from the already deprecated and stale package we'll probably have to work around it this way until a new release is made in this one
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.
As you wish, but the current implementation is not valid according to the spec and fails hard in the wild (basically when interacting with any PHP project, because Lcobucci's lib is the most popular in this ecosystem).
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'd love to see this fixed as well. Although I think it's an easier migration story for users:
golang-jwt/jwt v1.x.x is backwards-compatible with dgrijalva/jwt-go (despite the incorrect implementation you pointed out)
and then we could consider further improvements for those already on /v1 that breaks compatibility with a /v2 release
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 fear, that this won't be completely possible anyway, because the cleanest fix to #6 (which we agreed on fixing before our first "release") is to adjust the claims struct, changing
Audience
fromstring
to[]string
. We might as well correct the float/int issue as well before releasingv1.x.x
.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.
Hehe, these are the trickiest tradeoffs. The breaking change is justified from a security and spec perspective, but keeping
StandardClaims
as-is maintains backwards-compatibility. The latter is something most Go folks have come to appreciate. Just want to make sure if we do breakStandardClaims
its worth it.What are your thoughts w.r.t adding a new struct
RegisteredClaims
while keepingStandardClaims
unchanged with a deprecation notice and comment suggesting against its use?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 will play around with this idea over the weekend. With that in mind I would then also probably choose to make the audience field directly a time Field instead of an into or float and have appropriate MarshalJSON functions that take care of the conversion. This feels more naturally and idiomatic to me than assigning the expiry in a float value.