Skip to content

Commit 94176c9

Browse files
shiv-tyagishiv-tyagi
committed
broker: use /userinfo as fallback for claims not available in id token
Fixes [#1440](#1440). This uses the information retrieved from /userinfo endpoint as a fallback for claims not available in id token. This only affects the generic oidc broker, for ms entra id, we simple drop the information retrieved from /userinfo. I have verified both oidc and msentraid brokers with these changes, they both work fine. Signed-off-by: Shiv Tyagi <shivtyagi3015@gmail.com>
1 parent 1f7d986 commit 94176c9

7 files changed

Lines changed: 133 additions & 34 deletions

File tree

‎authd-oidc-brokers/internal/broker/broker.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -709,7 +709,7 @@ func (b *Broker) deviceAuth(ctx context.Context, session *session) (string, isAu
709709
return AuthDenied, unexpectedErrMsg("could not get provider metadata")
710710
}
711711

712-
authInfo.UserInfo, err = b.userInfoFromIDToken(ctx, session, rawIDToken)
712+
authInfo.UserInfo, err = b.userInfo(ctx, session, t)
713713
if err != nil {
714714
log.Errorf(context.Background(), "could not get user info: %s", err)
715715
return AuthDenied, errorMessageForDisplay(err, "Could not get user info")
@@ -1147,7 +1147,7 @@ func (b *Broker) refreshToken(ctx context.Context, session *session, oldToken *t
11471147
t.ProviderMetadata = oldToken.ProviderMetadata
11481148
t.DeviceRegistrationData = oldToken.DeviceRegistrationData
11491149

1150-
t.UserInfo, err = b.userInfoFromIDToken(ctx, session, rawIDToken)
1150+
t.UserInfo, err = b.userInfo(ctx, session, t.Token)
11511151
if err != nil {
11521152
return nil, err
11531153
}
@@ -1157,16 +1157,28 @@ func (b *Broker) refreshToken(ctx context.Context, session *session, oldToken *t
11571157
return t, nil
11581158
}
11591159

1160-
// userInfoFromIDToken verifies and parses the raw ID token and returns the user info from it.
1160+
// userInfo verifies and parses the raw ID token and returns the user info from it.
1161+
// It also fetches additional claims from the /userinfo endpoint as a fallback for fields not present in the ID token,
1162+
// with ID token claims taking precedence over userinfo claims.
11611163
// Note that verifying the ID token requires a working network connection to the provider's JWKs endpoint,
11621164
// so make sure to only call this function if the session is online.
1163-
func (b *Broker) userInfoFromIDToken(ctx context.Context, session *session, rawIDToken string) (info.User, error) {
1165+
func (b *Broker) userInfo(ctx context.Context, session *session, token *oauth2.Token) (info.User, error) {
1166+
rawIDToken, ok := token.Extra("id_token").(string)
1167+
if !ok {
1168+
return info.User{}, errors.New("token response does not contain an ID token")
1169+
}
1170+
11641171
idToken, err := session.oidcServer.Verifier(&b.oidcCfg).Verify(ctx, rawIDToken)
11651172
if err != nil {
11661173
return info.User{}, fmt.Errorf("could not verify token: %w", err)
11671174
}
11681175

1169-
userInfo, err := b.provider.GetUserInfo(idToken)
1176+
oidcUserInfo, err := session.oidcServer.UserInfo(ctx, oauth2.StaticTokenSource(token))
1177+
if err != nil {
1178+
log.Warningf(ctx, "could not fetch information from /userinfo endpoint: %v", err)
1179+
}
1180+
1181+
userInfo, err := b.provider.GetUserInfo(idToken, oidcUserInfo)
11701182
if err != nil {
11711183
return info.User{}, err
11721184
}

‎authd-oidc-brokers/internal/providers/genericprovider/genericprovider.go‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package genericprovider
33

44
import (
55
"context"
6+
"encoding/json"
67
"fmt"
78
"slices"
89
"strings"
@@ -48,8 +49,8 @@ func (p GenericProvider) GetMetadata(provider *oidc.Provider) (map[string]interf
4849
}
4950

5051
// GetUserInfo is a no-op when no specific provider is in use.
51-
func (p GenericProvider) GetUserInfo(idToken info.Claimer) (info.User, error) {
52-
userClaims, err := p.userClaims(idToken)
52+
func (p GenericProvider) GetUserInfo(idToken info.Claimer, oidcUserInfo info.Claimer) (info.User, error) {
53+
userClaims, err := p.userClaims(idToken, oidcUserInfo)
5354
if err != nil {
5455
return info.User{}, err
5556
}
@@ -109,13 +110,46 @@ type claims struct {
109110
EmailVerified bool `json:"email_verified"`
110111
}
111112

112-
// userClaims returns the user claims parsed from the ID token.
113-
func (p GenericProvider) userClaims(idToken info.Claimer) (claims, error) {
114-
var userClaims claims
115-
if err := idToken.Claims(&userClaims); err != nil {
116-
return claims{}, fmt.Errorf("failed to get ID token claims: %v", err)
113+
// userClaims returns the user claims parsed from idToken and oidcUserInfo.
114+
115+
func (p GenericProvider) userClaims(idToken info.Claimer, oidcUserInfo info.Claimer) (claims, error) {
116+
idMap := make(map[string]any)
117+
if err := idToken.Claims(&idMap); err != nil {
118+
return claims{}, fmt.Errorf("failed to get claims from ID token: %v", err)
119+
}
120+
121+
infoMap := make(map[string]any)
122+
if oidcUserInfo != nil {
123+
_ = oidcUserInfo.Claims(&infoMap) // ignore error, as the /userinfo endpoint is a fallback
124+
}
125+
126+
mergedClaims, err := mergeUserClaims(idMap, infoMap)
127+
if err != nil {
128+
return claims{}, err
129+
}
130+
return mergedClaims, nil
131+
}
132+
133+
// mergeUserClaims merges id token and /userinfo claims, giving preference to idToken values.
134+
func mergeUserClaims(idTokenClaims, userInfoClaims map[string]any) (claims, error) {
135+
merged := make(map[string]any, len(idTokenClaims)+len(userInfoClaims))
136+
for k, v := range userInfoClaims {
137+
merged[k] = v
138+
}
139+
for k, v := range idTokenClaims {
140+
merged[k] = v
141+
}
142+
143+
data, err := json.Marshal(merged)
144+
if err != nil {
145+
return claims{}, fmt.Errorf("failed to merge user claims: %v", err)
146+
}
147+
148+
var c claims
149+
if err := json.Unmarshal(data, &c); err != nil {
150+
return claims{}, fmt.Errorf("failed to parse merged user claims: %v", err)
117151
}
118-
return userClaims, nil
152+
return c, nil
119153
}
120154

121155
// IsTokenExpiredError returns true if the reason for the error is that the refresh token is expired.

‎authd-oidc-brokers/internal/providers/genericprovider/genericprovider_test.go‎

Lines changed: 68 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,14 @@ func TestGetUserInfo(t *testing.T) {
1616
t.Parallel()
1717

1818
tests := map[string]struct {
19-
claims map[string]interface{}
20-
wantUser info.User
21-
wantErr bool
22-
wantErrType error
19+
idTokenClaims map[string]interface{}
20+
userInfoClaims map[string]interface{}
21+
wantUser info.User
22+
wantErr bool
23+
wantErrType error
2324
}{
2425
"Successfully_get_user_info_with_all_fields": {
25-
claims: map[string]interface{}{
26+
idTokenClaims: map[string]interface{}{
2627
"sub": "sub123",
2728
"email": "user@example.com",
2829
"email_verified": true,
@@ -33,7 +34,7 @@ func TestGetUserInfo(t *testing.T) {
3334
wantUser: info.NewUser("user@example.com", "/home/user", "sub123", "/bin/bash", "Test User", nil),
3435
},
3536
"Successfully_get_user_info_with_minimal_fields": {
36-
claims: map[string]interface{}{
37+
idTokenClaims: map[string]interface{}{
3738
"sub": "sub123",
3839
"email": "user@example.com",
3940
"email_verified": true,
@@ -42,53 +43,103 @@ func TestGetUserInfo(t *testing.T) {
4243
},
4344

4445
"Error_when_sub_is_missing": {
45-
claims: map[string]interface{}{
46+
idTokenClaims: map[string]interface{}{
4647
"email": "user@example.com",
4748
"email_verified": false,
4849
},
4950
wantErr: true,
5051
},
5152
"Error_when_email_is_missing": {
52-
claims: map[string]interface{}{
53+
idTokenClaims: map[string]interface{}{
5354
"sub": "sub123",
5455
},
5556
wantErr: true,
5657
},
57-
"Error_when_email_verified_is_missing": {
58-
claims: map[string]interface{}{
58+
"Error_when_email_verified_is_missing_in_both": {
59+
idTokenClaims: map[string]interface{}{
5960
"sub": "sub123",
6061
"email": "user@example.com",
6162
},
6263
wantErr: true,
6364
wantErrType: &providerErrors.ForDisplayError{},
6465
},
6566
"Error_when_email_is_not_verified": {
66-
claims: map[string]interface{}{
67+
idTokenClaims: map[string]interface{}{
6768
"email": "user@example.com",
6869
"sub": "sub123",
6970
"email_verified": false,
7071
},
7172
wantErr: true,
7273
wantErrType: &providerErrors.ForDisplayError{},
7374
},
75+
"idToken_email_verified_overrides_userinfo_false": {
76+
idTokenClaims: map[string]interface{}{
77+
"sub": "sub123",
78+
"email": "user@example.com",
79+
"email_verified": true,
80+
},
81+
userInfoClaims: map[string]interface{}{
82+
"email_verified": false,
83+
},
84+
wantUser: info.NewUser("user@example.com", "", "sub123", "", "", nil),
85+
},
86+
"email_verified_missing_in_idToken_falls_back_to_userinfo": {
87+
idTokenClaims: map[string]interface{}{
88+
"sub": "sub123",
89+
"email": "user@example.com",
90+
},
91+
userInfoClaims: map[string]interface{}{
92+
"email_verified": true,
93+
},
94+
wantUser: info.NewUser("user@example.com", "", "sub123", "", "", nil),
95+
},
96+
"extra_fields_from_userinfo_used_when_absent_in_idToken": {
97+
idTokenClaims: map[string]interface{}{
98+
"sub": "sub123",
99+
"email": "user@example.com",
100+
"email_verified": true,
101+
},
102+
userInfoClaims: map[string]interface{}{
103+
"home": "/home/frominfo",
104+
"shell": "/bin/zsh",
105+
"name": "From UserInfo",
106+
},
107+
wantUser: info.NewUser("user@example.com", "/home/frominfo", "sub123", "/bin/zsh", "From UserInfo", nil),
108+
},
109+
"idToken_fields_take_precedence_over_userinfo": {
110+
idTokenClaims: map[string]interface{}{
111+
"sub": "sub123",
112+
"email": "user@example.com",
113+
"email_verified": true,
114+
"home": "/home/fromidtoken",
115+
"shell": "/bin/bash",
116+
"name": "From IDToken",
117+
},
118+
userInfoClaims: map[string]interface{}{
119+
"home": "/home/frominfo",
120+
"shell": "/bin/zsh",
121+
"name": "From UserInfo",
122+
},
123+
wantUser: info.NewUser("user@example.com", "/home/fromidtoken", "sub123", "/bin/bash", "From IDToken", nil),
124+
},
74125
}
75126

76127
for name, tc := range tests {
77128
t.Run(name, func(t *testing.T) {
78129
t.Parallel()
79130

80131
p := genericprovider.New()
81-
mockToken := &mockIDToken{claims: tc.claims}
132+
idToken := &mockIDToken{claims: tc.idTokenClaims}
133+
userInfo := &mockIDToken{claims: tc.userInfoClaims}
82134

83-
user, err := p.GetUserInfo(mockToken)
135+
user, err := p.GetUserInfo(idToken, userInfo)
84136
t.Logf("GetUserInfo error: %v", err)
85137

86138
if tc.wantErr {
87139
require.Error(t, err)
88-
return
89-
}
90-
if tc.wantErrType != nil {
91-
require.ErrorIs(t, err, tc.wantErrType)
140+
if tc.wantErrType != nil {
141+
require.ErrorAs(t, err, &tc.wantErrType)
142+
}
92143
return
93144
}
94145
require.NoError(t, err)

‎authd-oidc-brokers/internal/providers/msentraid/msentraid.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,9 @@ func (p *Provider) GetMetadata(provider *oidc.Provider) (map[string]interface{},
119119
}
120120

121121
// GetUserInfo returns the user info from the ID token.
122-
func (p *Provider) GetUserInfo(idToken info.Claimer) (info.User, error) {
122+
// The oidcUserInfo parameter is accepted for interface compatibility but is not used,
123+
// as MS Entra ID tokens are authoritative and contain all required claims.
124+
func (p *Provider) GetUserInfo(idToken info.Claimer, _ info.Claimer) (info.User, error) {
123125
var err error
124126

125127
userClaims, err := p.userClaims(idToken)

‎authd-oidc-brokers/internal/providers/msentraid/msentraid_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func TestGetUserInfo(t *testing.T) {
124124

125125
p := msentraid.New()
126126

127-
got, err := p.GetUserInfo(idToken)
127+
got, err := p.GetUserInfo(idToken, &testIDToken{claims: "{}"})
128128
if tc.wantErr {
129129
require.Error(t, err, "GetUserInfo should return an error")
130130
return

‎authd-oidc-brokers/internal/providers/providers.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ type Provider interface {
1717
GetExtraFields(token *oauth2.Token) map[string]interface{}
1818
GetMetadata(provider *oidc.Provider) (map[string]interface{}, error)
1919

20-
GetUserInfo(idToken info.Claimer) (info.User, error)
20+
GetUserInfo(idToken info.Claimer, oidcUserInfo info.Claimer) (info.User, error)
2121

2222
GetGroups(
2323
ctx context.Context,

‎authd-oidc-brokers/internal/testutils/provider.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ func (p *MockProvider) GetMetadata(provider *oidc.Provider) (map[string]interfac
404404
}
405405

406406
// GetUserInfo returns the user info parsed from the ID token.
407-
func (p *MockProvider) GetUserInfo(idToken info.Claimer) (info.User, error) {
407+
func (p *MockProvider) GetUserInfo(idToken info.Claimer, _ info.Claimer) (info.User, error) {
408408
userClaims, err := p.userClaims(idToken)
409409
if err != nil {
410410
return info.User{}, err

0 commit comments

Comments
 (0)