Skip to content

Commit 842ec12

Browse files
committed
fix: SVG claim extraction and consent double-auth initiation
- Add HasVCIDocuments() to skip re-initiating OIDC/SAML auth when documents are already cached from callback - Add findValueByName fallback for SVG claims when JSONPath extraction fails due to credential_mapping mismatch - Add diagnostic logging: document data keys, mapping attributes, JSONPath map for debugging claim extraction issues - Fix pre-existing MDQ test compilation errors (missing signingCert param)
1 parent 13d95be commit 842ec12

File tree

9 files changed

+106
-51
lines changed

9 files changed

+106
-51
lines changed

internal/apigw/apiv1/handlers_issuer.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ func (c *Client) StoreVCIDocuments(ctx context.Context, sessionID string, docs m
2828
return nil
2929
}
3030

31+
// HasVCIDocuments checks whether documents have already been stored for the given VCI session.
32+
// Used by the consent endpoint to avoid re-initiating external auth when documents are already cached.
33+
func (c *Client) HasVCIDocuments(ctx context.Context, sessionID string) bool {
34+
docs, ok := c.cacheService.Document.Get(ctx, sessionID)
35+
return ok && len(docs) > 0
36+
}
37+
3138
// VCICredentialOffer implements OpenID4VCI credential offer endpoint
3239
// https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-credential-offer-endpoint
3340
func (c *Client) VCICredentialOffer(ctx context.Context, req *openid4vci.CredentialOfferParameters) (*openid4vci.CredentialOfferParameters, error) {

internal/apigw/apiv1/handlers_oidcrp.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,22 @@ func (c *Client) OIDCRPCallback(ctx context.Context, req *OIDCRPCallbackRequest,
141141
return nil, err
142142
}
143143

144+
// Log the mapping attributes and resulting document data for diagnostics.
145+
// This helps identify mismatches between credential_mappings and the VCTM claims.
146+
claimKeys := make([]string, 0, len(claims))
147+
for k := range claims {
148+
claimKeys = append(claimKeys, k)
149+
}
150+
mappingKeys := make([]string, 0, len(mapping.Attributes))
151+
for k := range mapping.Attributes {
152+
mappingKeys = append(mappingKeys, k)
153+
}
154+
144155
c.log.Info("OIDC authentication successful",
145156
"credential_type", session.CredentialType,
146157
"claims_count", len(claims),
158+
"claim_keys", claimKeys,
159+
"mapping_attributes", mappingKeys,
147160
"subject", authResp.IDToken.Subject)
148161

149162
// VCI mode: if the OIDC session was initiated from the OpenID4VCI consent flow,

internal/apigw/apiv1/handlers_users.go

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,15 +244,35 @@ func (c *Client) UserLookup(ctx context.Context, req *vcclient.UserLookupRequest
244244
return nil, fmt.Errorf("failed to extract claim values from document data: %w", err)
245245
}
246246

247-
c.log.Debug("extracted claim values",
248-
"extracted_count", len(claimValues),
249-
"requested_count", len(jsonPaths.Displayable),
250-
"claims", claimValues)
247+
if len(claimValues) == 0 && len(jsonPaths.Displayable) > 0 {
248+
// Log diagnostic info when JSONPath extraction finds nothing.
249+
// This typically means the credential_mappings don't produce the
250+
// claim keys expected by the VCTM (check svg_id / path alignment).
251+
docKeys := make([]string, 0, len(doc.DocumentData))
252+
for k := range doc.DocumentData {
253+
docKeys = append(docKeys, k)
254+
}
255+
c.log.Warn("no claims extracted: document data keys do not match VCTM JSONPaths",
256+
"document_data_keys", docKeys,
257+
"json_paths", jsonPaths.Displayable)
258+
} else {
259+
c.log.Debug("extracted claim values",
260+
"extracted_count", len(claimValues),
261+
"requested_count", len(jsonPaths.Displayable),
262+
"claims", claimValues)
263+
}
251264

252265
for _, claim := range req.VCTM.Claims {
253266
if claim.SVGID != "" {
254267
value, ok := claimValues[claim.SVGID].(string)
255268
if !ok {
269+
// JSONPath extraction missed this claim — try direct lookup as fallback
270+
if value := findValueByName(doc.DocumentData, claim.Path); value != "" {
271+
svgTemplateClaims[claim.SVGID] = vcclient.SVGClaim{
272+
Label: claim.Display[0].Label,
273+
Value: value,
274+
}
275+
}
256276
continue
257277
}
258278
svgTemplateClaims[claim.SVGID] = vcclient.SVGClaim{

internal/apigw/httpserver/api.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ type Apiv1 interface {
6969

7070
// VCI integration for external auth (SAML/OIDC)
7171
StoreVCIDocuments(ctx context.Context, sessionID string, docs map[string]*model.CompleteDocument) error
72+
HasVCIDocuments(ctx context.Context, sessionID string) bool
7273

7374
// misc endpoints
7475
Health(ctx context.Context, req *apiv1_status.StatusRequest) (*apiv1_status.StatusReply, error)

internal/apigw/httpserver/endpoints_oauth.go

Lines changed: 39 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -210,23 +210,28 @@ func (s *Service) endpointOAuthAuthorizationConsent(ctx context.Context, c *gin.
210210
return nil, err
211211
}
212212

213-
scope, _ := session.Get("scope").(string)
214-
if s.cfg.GetCredentialConstructor(scope) == nil {
215-
err := fmt.Errorf("scope %q not configured for credential issuance", scope)
216-
span.SetStatus(codes.Error, err.Error())
217-
return nil, err
213+
// Skip re-initiating auth if the callback already stored documents.
214+
if !s.apiv1.HasVCIDocuments(ctx, sessionID) {
215+
scope, _ := session.Get("scope").(string)
216+
if s.cfg.GetCredentialConstructor(scope) == nil {
217+
err := fmt.Errorf("scope %q not configured for credential issuance", scope)
218+
span.SetStatus(codes.Error, err.Error())
219+
return nil, err
220+
}
221+
222+
// Determine the IdP entity ID: use static IdP or default from config
223+
idpEntityID := s.samlSPService.GetStaticIDPEntityID()
224+
225+
authReq, err := s.samlSPService.InitiateAuthForVCI(ctx, idpEntityID, scope, sessionID)
226+
if err != nil {
227+
span.SetStatus(codes.Error, err.Error())
228+
return nil, err
229+
}
230+
231+
redirectURL = authReq.RedirectURL
232+
} else {
233+
s.log.Debug("SAML auth already completed, documents cached", "session_id", sessionID)
218234
}
219-
220-
// Determine the IdP entity ID: use static IdP or default from config
221-
idpEntityID := s.samlSPService.GetStaticIDPEntityID()
222-
223-
authReq, err := s.samlSPService.InitiateAuthForVCI(ctx, idpEntityID, scope, sessionID)
224-
if err != nil {
225-
span.SetStatus(codes.Error, err.Error())
226-
return nil, err
227-
}
228-
229-
redirectURL = authReq.RedirectURL
230235
}
231236

232237
if authMethod == model.AuthMethodOIDC {
@@ -236,20 +241,25 @@ func (s *Service) endpointOAuthAuthorizationConsent(ctx context.Context, c *gin.
236241
return nil, err
237242
}
238243

239-
scope, _ := session.Get("scope").(string)
240-
if s.cfg.GetCredentialConstructor(scope) == nil {
241-
err := fmt.Errorf("scope %q not configured for credential issuance", scope)
242-
span.SetStatus(codes.Error, err.Error())
243-
return nil, err
244-
}
245-
246-
authReq, err := s.oidcrpService.InitiateAuthForVCI(ctx, scope, sessionID)
247-
if err != nil {
248-
span.SetStatus(codes.Error, err.Error())
249-
return nil, err
244+
// Skip re-initiating auth if the callback already stored documents.
245+
if !s.apiv1.HasVCIDocuments(ctx, sessionID) {
246+
scope, _ := session.Get("scope").(string)
247+
if s.cfg.GetCredentialConstructor(scope) == nil {
248+
err := fmt.Errorf("scope %q not configured for credential issuance", scope)
249+
span.SetStatus(codes.Error, err.Error())
250+
return nil, err
251+
}
252+
253+
authReq, err := s.oidcrpService.InitiateAuthForVCI(ctx, scope, sessionID)
254+
if err != nil {
255+
span.SetStatus(codes.Error, err.Error())
256+
return nil, err
257+
}
258+
259+
redirectURL = authReq.AuthorizationURL
260+
} else {
261+
s.log.Debug("OIDC auth already completed, documents cached", "session_id", sessionID)
250262
}
251-
252-
redirectURL = authReq.AuthorizationURL
253263
}
254264

255265
c.HTML(http.StatusOK, "consent.html", gin.H{

internal/apigw/httpserver/endpoints_saml.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,14 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
184184
return nil, err
185185
}
186186

187+
claimKeys := make([]string, 0, len(claims))
188+
for k := range claims {
189+
claimKeys = append(claimKeys, k)
190+
}
187191
s.log.Info("SAML authentication successful",
188192
"credential_type", session.CredentialType,
189-
"claims_count", len(claims))
193+
"claims_count", len(claims),
194+
"claim_keys", claimKeys)
190195

191196
// VCI mode: if the SAML session was initiated from the OpenID4VCI consent flow,
192197
// store the transformed claims as a document in the VCI session cache and redirect

internal/apigw/integration/oidc_integration_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,7 @@ func createTestOIDCRPConfig(op *mockOIDCProvider) *model.OIDCRPConfig {
495495
Enable: true,
496496
Registration: &model.OIDCRPRegistrationConfig{
497497
Preconfigured: &model.OIDCRPPreconfiguredConfig{
498-
Enable: true,
498+
Enable: true,
499499
ClientID: op.clientID,
500500
ClientSecret: op.clientSecret,
501501
},

internal/apigw/samlsp/mdq_static_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestNewStaticMDQClient_FromFile(t *testing.T) {
3030
require.NoError(t, err)
3131

3232
// Create static MDQ client from file
33-
client, err := NewStaticMDQClient(metadataPath, "https://static-idp.example.com/idp", false, log)
33+
client, err := NewStaticMDQClient(metadataPath, "https://static-idp.example.com/idp", false, nil, log)
3434
require.NoError(t, err)
3535
require.NotNil(t, client)
3636

@@ -62,7 +62,7 @@ func TestNewStaticMDQClient_FromURL(t *testing.T) {
6262
require.NoError(t, err)
6363

6464
// Create static MDQ client from URL
65-
client, err := NewStaticMDQClient(server.URL+"/metadata", "https://static-idp.example.com/idp", true, log)
65+
client, err := NewStaticMDQClient(server.URL+"/metadata", "https://static-idp.example.com/idp", true, nil, log)
6666
require.NoError(t, err)
6767
require.NotNil(t, client)
6868

@@ -89,7 +89,7 @@ func TestStaticMDQClient_GetIDPMetadata_IgnoresEntityID(t *testing.T) {
8989
log, err := logger.New("test", "", false)
9090
require.NoError(t, err)
9191

92-
client, err := NewStaticMDQClient(metadataPath, "https://static-idp.example.com/idp", false, log)
92+
client, err := NewStaticMDQClient(metadataPath, "https://static-idp.example.com/idp", false, nil, log)
9393
require.NoError(t, err)
9494

9595
ctx := t.Context()
@@ -113,7 +113,7 @@ func TestNewStaticMDQClient_FileNotFound(t *testing.T) {
113113
require.NoError(t, err)
114114

115115
// Try to create client with non-existent file
116-
_, err = NewStaticMDQClient("/nonexistent/metadata.xml", "https://idp.example.com", false, log)
116+
_, err = NewStaticMDQClient("/nonexistent/metadata.xml", "https://idp.example.com", false, nil, log)
117117
require.Error(t, err)
118118
assert.Contains(t, err.Error(), "failed to read metadata file")
119119
}
@@ -127,7 +127,7 @@ func TestNewStaticMDQClient_InvalidXML(t *testing.T) {
127127
log, err := logger.New("test", "", false)
128128
require.NoError(t, err)
129129

130-
_, err = NewStaticMDQClient(metadataPath, "https://idp.example.com", false, log)
130+
_, err = NewStaticMDQClient(metadataPath, "https://idp.example.com", false, nil, log)
131131
require.Error(t, err)
132132
assert.Contains(t, err.Error(), "failed to parse IdP metadata XML")
133133
}
@@ -147,7 +147,7 @@ func TestNewStaticMDQClient_NoIDPDescriptor(t *testing.T) {
147147
log, err := logger.New("test", "", false)
148148
require.NoError(t, err)
149149

150-
_, err = NewStaticMDQClient(metadataPath, "https://sp.example.com", false, log)
150+
_, err = NewStaticMDQClient(metadataPath, "https://sp.example.com", false, nil, log)
151151
require.Error(t, err)
152152
assert.Contains(t, err.Error(), "does not contain IdP SSO descriptor")
153153
}
@@ -157,7 +157,7 @@ func TestNewStaticMDQClient_URLFetchError(t *testing.T) {
157157
require.NoError(t, err)
158158

159159
// Try to fetch from invalid URL
160-
_, err = NewStaticMDQClient("http://nonexistent.invalid/metadata", "https://idp.example.com", true, log)
160+
_, err = NewStaticMDQClient("http://nonexistent.invalid/metadata", "https://idp.example.com", true, nil, log)
161161
require.Error(t, err)
162162
assert.Contains(t, err.Error(), "failed to fetch metadata from URL")
163163
}
@@ -173,7 +173,7 @@ func TestNewStaticMDQClient_EntityIDMismatch(t *testing.T) {
173173

174174
// Create client with different entityID than in metadata
175175
// Should succeed but log warning (not tested here, but functionality works)
176-
client, err := NewStaticMDQClient(metadataPath, "https://different-idp.example.com", false, log)
176+
client, err := NewStaticMDQClient(metadataPath, "https://different-idp.example.com", false, nil, log)
177177
require.NoError(t, err)
178178
require.NotNil(t, client)
179179

internal/apigw/samlsp/mdq_test.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func TestMDQClient_GetIDPMetadata_Success(t *testing.T) {
3333
log, err := logger.New("test", "", false)
3434
require.NoError(t, err)
3535

36-
client := NewMDQClient(server.URL, 3600, log)
36+
client := NewMDQClient(server.URL, 3600, nil, log)
3737

3838
ctx := t.Context()
3939
metadata, err := client.GetIDPMetadata(ctx, "https://idp.example.com/idp")
@@ -61,7 +61,7 @@ func TestMDQClient_GetIDPMetadata_Caching(t *testing.T) {
6161
log, err := logger.New("test", "", false)
6262
require.NoError(t, err)
6363

64-
client := NewMDQClient(server.URL, 3600, log)
64+
client := NewMDQClient(server.URL, 3600, nil, log)
6565

6666
ctx := t.Context()
6767
entityID := "https://idp.example.com/idp"
@@ -96,8 +96,7 @@ func TestMDQClient_GetIDPMetadata_CacheExpiration(t *testing.T) {
9696
log, err := logger.New("test", "", false)
9797
require.NoError(t, err)
9898

99-
// Very short cache TTL (1 second)
100-
client := NewMDQClient(server.URL, 1, log)
99+
client := NewMDQClient(server.URL, 1, nil, log)
101100

102101
ctx := t.Context()
103102
entityID := "https://idp.example.com/idp"
@@ -126,7 +125,7 @@ func TestMDQClient_GetIDPMetadata_HTTPError(t *testing.T) {
126125
log, err := logger.New("test", "", false)
127126
require.NoError(t, err)
128127

129-
client := NewMDQClient(server.URL, 3600, log)
128+
client := NewMDQClient(server.URL, 3600, nil, log)
130129

131130
ctx := t.Context()
132131
_, err = client.GetIDPMetadata(ctx, "https://nonexistent.idp.com")
@@ -145,7 +144,7 @@ func TestMDQClient_GetIDPMetadata_InvalidXML(t *testing.T) {
145144
log, err := logger.New("test", "", false)
146145
require.NoError(t, err)
147146

148-
client := NewMDQClient(server.URL, 3600, log)
147+
client := NewMDQClient(server.URL, 3600, nil, log)
149148

150149
ctx := t.Context()
151150
_, err = client.GetIDPMetadata(ctx, "https://idp.example.com/idp")
@@ -171,7 +170,7 @@ func TestMDQClient_GetIDPMetadata_NoIDPDescriptor(t *testing.T) {
171170
log, err := logger.New("test", "", false)
172171
require.NoError(t, err)
173172

174-
client := NewMDQClient(server.URL, 3600, log)
173+
client := NewMDQClient(server.URL, 3600, nil, log)
175174

176175
ctx := t.Context()
177176
_, err = client.GetIDPMetadata(ctx, "https://sp.example.com")
@@ -195,7 +194,7 @@ func TestMDQClient_MultipleConcurrentRequests(t *testing.T) {
195194
log, err := logger.New("test", "", false)
196195
require.NoError(t, err)
197196

198-
client := NewMDQClient(server.URL, 3600, log)
197+
client := NewMDQClient(server.URL, 3600, nil, log)
199198

200199
ctx := t.Context()
201200
entityID := "https://idp.example.com/idp"

0 commit comments

Comments
 (0)