fix: broken JSON round-tripping for custom claims · ory/hydra@b36b701 · GitHub
Skip to content

Commit b36b701

Browse files
alnralnr
committed
fix: broken JSON round-tripping for custom claims
Adding custom claims with numerical types (think JavaScript Number) previously did not round-trip through Hydra correctly. For example, passing UNIX timestamps in custom claims would end up as floating points in exponential notation in the final token. That, in turn, confused or broke downstream consumers of the token, including Kratos. Ref go-jose/go-jose#144
1 parent fa50e3e commit b36b701

7 files changed

Lines changed: 55 additions & 32 deletions

File tree

‎consent/strategy_oauth_test.go‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,7 @@ func TestStrategyLoginConsentNext(t *testing.T) {
289289

290290
subject := "aeneas-rekkas"
291291
c := createDefaultClient(t)
292+
now := 1723546027 // Unix timestamps must round-trip through Hydra without converting to floats or similar
292293
testhelpers.NewLoginConsentUI(t, reg.Config(),
293294
acceptLoginHandler(t, subject, &hydra.AcceptOAuth2LoginRequest{
294295
Remember: pointerx.Bool(true),
@@ -297,8 +298,14 @@ func TestStrategyLoginConsentNext(t *testing.T) {
297298
Remember: pointerx.Bool(true),
298299
GrantScope: []string{"openid"},
299300
Session: &hydra.AcceptOAuth2ConsentRequestSession{
300-
AccessToken: map[string]interface{}{"foo": "bar"},
301-
IdToken: map[string]interface{}{"bar": "baz"},
301+
AccessToken: map[string]interface{}{
302+
"foo": "bar",
303+
"ts1": now,
304+
},
305+
IdToken: map[string]interface{}{
306+
"bar": "baz",
307+
"ts2": now,
308+
},
302309
},
303310
}))
304311

@@ -314,12 +321,14 @@ func TestStrategyLoginConsentNext(t *testing.T) {
314321
require.NoError(t, err)
315322

316323
claims := testhelpers.IntrospectToken(t, conf, token.AccessToken, adminTS)
317-
assert.Equal(t, "bar", claims.Get("ext.foo").String(), "%s", claims.Raw)
324+
assert.Equalf(t, `"bar"`, claims.Get("ext.foo").Raw, "%s", claims.Raw) // Raw rather than .Int() or .Value() to verify the exact JSON payload
325+
assert.Equalf(t, "1723546027", claims.Get("ext.ts1").Raw, "%s", claims.Raw) // must round-trip as integer
318326

319327
idClaims := testhelpers.DecodeIDToken(t, token)
320-
assert.Equal(t, "baz", idClaims.Get("bar").String(), "%s", idClaims.Raw)
328+
assert.Equalf(t, `"baz"`, idClaims.Get("bar").Raw, "%s", idClaims.Raw) // Raw rather than .Int() or .Value() to verify the exact JSON payload
329+
assert.Equalf(t, "1723546027", idClaims.Get("ts2").Raw, "%s", idClaims.Raw) // must round-trip as integer
321330
sid = idClaims.Get("sid").String()
322-
assert.NotNil(t, sid)
331+
assert.NotEmpty(t, sid)
323332
}
324333

325334
t.Run("perform first flow", run)
@@ -334,21 +343,28 @@ func TestStrategyLoginConsentNext(t *testing.T) {
334343
assert.Empty(t, pointerx.StringR(res.Client.ClientSecret))
335344
return hydra.AcceptOAuth2LoginRequest{
336345
Subject: subject,
337-
Context: map[string]interface{}{"foo": "bar"},
346+
Context: map[string]interface{}{"xyz": "abc"},
338347
}
339348
}),
340-
checkAndAcceptConsentHandler(t, adminClient, func(t *testing.T, res *hydra.OAuth2ConsentRequest, err error) hydra.AcceptOAuth2ConsentRequest {
349+
checkAndAcceptConsentHandler(t, adminClient, func(t *testing.T, req *hydra.OAuth2ConsentRequest, err error) hydra.AcceptOAuth2ConsentRequest {
341350
require.NoError(t, err)
342-
assert.True(t, *res.Skip)
343-
assert.Equal(t, sid, *res.LoginSessionId)
344-
assert.Equal(t, subject, *res.Subject)
345-
assert.Empty(t, pointerx.StringR(res.Client.ClientSecret))
351+
assert.True(t, *req.Skip)
352+
assert.Equal(t, sid, *req.LoginSessionId)
353+
assert.Equal(t, subject, *req.Subject)
354+
assert.Empty(t, pointerx.StringR(req.Client.ClientSecret))
355+
assert.Equal(t, map[string]interface{}{"xyz": "abc"}, req.Context)
346356
return hydra.AcceptOAuth2ConsentRequest{
347357
Remember: pointerx.Bool(true),
348358
GrantScope: []string{"openid"},
349359
Session: &hydra.AcceptOAuth2ConsentRequestSession{
350-
AccessToken: map[string]interface{}{"foo": "bar"},
351-
IdToken: map[string]interface{}{"bar": "baz"},
360+
AccessToken: map[string]interface{}{
361+
"foo": "bar",
362+
"ts1": now,
363+
},
364+
IdToken: map[string]interface{}{
365+
"bar": "baz",
366+
"ts2": now,
367+
},
352368
},
353369
}
354370
}))

‎oauth2/.snapshots/TestUnmarshalSession-v1.11.8.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
"amr": [],
1818
"c_hash": "",
1919
"ext": {
20-
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d"
20+
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
21+
"timestamp": 1723546027
2122
}
2223
},
2324
"headers": {

‎oauth2/.snapshots/TestUnmarshalSession-v1.11.9.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
"amr": [],
1818
"c_hash": "",
1919
"ext": {
20-
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d"
20+
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
21+
"timestamp": 1723546027
2122
}
2223
},
2324
"headers": {

‎oauth2/fixtures/v1.11.8-session.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515
"AuthenticationMethodsReferences": [],
1616
"CodeHash": "",
1717
"Extra": {
18-
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d"
18+
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
19+
"timestamp": 1723546027
1920
}
2021
},
2122
"Headers": {

‎oauth2/fixtures/v1.11.9-session.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515
"amr": [],
1616
"c_hash": "",
1717
"ext": {
18-
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d"
18+
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
19+
"timestamp": 1723546027
1920
}
2021
},
2122
"headers": {

‎oauth2/session.go‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,21 @@
44
package oauth2
55

66
import (
7+
"bytes"
78
"context"
8-
"encoding/json"
99
"time"
1010

11+
jjson "github.com/go-jose/go-jose/v3/json"
12+
"github.com/mohae/deepcopy"
1113
"github.com/pkg/errors"
1214
"github.com/tidwall/gjson"
1315
"github.com/tidwall/sjson"
1416

15-
"github.com/mohae/deepcopy"
16-
1717
"github.com/ory/fosite"
1818
"github.com/ory/fosite/handler/openid"
1919
"github.com/ory/fosite/token/jwt"
2020
"github.com/ory/hydra/v2/driver/config"
2121
"github.com/ory/hydra/v2/flow"
22-
2322
"github.com/ory/x/logrusx"
2423
"github.com/ory/x/stringslice"
2524
)
@@ -60,33 +59,33 @@ func NewSessionWithCustomClaims(ctx context.Context, p *config.DefaultProvider,
6059
}
6160

6261
func (s *Session) GetJWTClaims() jwt.JWTClaimsContainer {
63-
//a slice of claims that are reserved and should not be overridden
64-
var reservedClaims = []string{"iss", "sub", "aud", "exp", "nbf", "iat", "jti", "client_id", "scp", "ext"}
62+
// a slice of claims that are reserved and should not be overridden
63+
reservedClaims := []string{"iss", "sub", "aud", "exp", "nbf", "iat", "jti", "client_id", "scp", "ext"}
6564

66-
//remove any reserved claims from the custom claims
65+
// remove any reserved claims from the custom claims
6766
allowedClaimsFromConfigWithoutReserved := stringslice.Filter(s.AllowedTopLevelClaims, func(s string) bool {
6867
return stringslice.Has(reservedClaims, s)
6968
})
7069

71-
//our new extra map which will be added to the jwt
72-
var topLevelExtraWithMirrorExt = map[string]interface{}{}
70+
// our new extra map which will be added to the jwt
71+
topLevelExtraWithMirrorExt := map[string]interface{}{}
7372

74-
//setting every allowed claim top level in jwt with respective value
73+
// setting every allowed claim top level in jwt with respective value
7574
for _, allowedClaim := range allowedClaimsFromConfigWithoutReserved {
7675
if cl, ok := s.Extra[allowedClaim]; ok {
7776
topLevelExtraWithMirrorExt[allowedClaim] = cl
7877
}
7978
}
8079

81-
//for every other claim that was already reserved and for mirroring, add original extra under "ext"
80+
// for every other claim that was already reserved and for mirroring, add original extra under "ext"
8281
if s.MirrorTopLevelClaims {
8382
topLevelExtraWithMirrorExt["ext"] = s.Extra
8483
}
8584

8685
claims := &jwt.JWTClaims{
8786
Subject: s.Subject,
8887
Issuer: s.DefaultSession.Claims.Issuer,
89-
//set our custom extra map as claims.Extra
88+
// set our custom extra map as claims.Extra
9089
Extra: topLevelExtraWithMirrorExt,
9190
ExpiresAt: s.GetExpiresAt(fosite.AccessToken),
9291
IssuedAt: time.Now(),
@@ -185,8 +184,11 @@ func (s *Session) UnmarshalJSON(original []byte) (err error) {
185184
}
186185
}
187186

187+
// https://github.com/go-jose/go-jose/issues/144
188+
dec := jjson.NewDecoder(bytes.NewReader(transformed))
189+
dec.SetNumberType(jjson.UnmarshalIntOrFloat)
188190
type t Session
189-
if err := json.Unmarshal(transformed, (*t)(s)); err != nil {
191+
if err := dec.Decode((*t)(s)); err != nil {
190192
return errors.WithStack(err)
191193
}
192194

‎oauth2/session_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ func TestUnmarshalSession(t *testing.T) {
4949
AuthenticationMethodsReferences: []string{},
5050
CodeHash: "",
5151
Extra: map[string]interface{}{
52-
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
52+
"sid": "177e1f44-a1e9-415c-bfa3-8b62280b182d",
53+
"timestamp": 1723546027,
5354
},
5455
},
5556
Headers: &jwt.Headers{Extra: map[string]interface{}{
@@ -85,7 +86,7 @@ func TestUnmarshalSession(t *testing.T) {
8586
snapshotx.SnapshotTExcept(t, &actual, nil)
8687
})
8788

88-
t.Run("v1.11.9", func(t *testing.T) {
89+
t.Run("v1.11.9" /* and later versions */, func(t *testing.T) {
8990
var actual Session
9091
require.NoError(t, json.Unmarshal(v1119Session, &actual))
9192
assertx.EqualAsJSON(t, expect, &actual)

0 commit comments

Comments
 (0)