From ff8e164e3f02986d993efe97ac03e288aadea7fe Mon Sep 17 00:00:00 2001 From: Leif Johansson Date: Mon, 24 Aug 2026 11:31:49 +0200 Subject: [PATCH] fix: release PID claims for the pid, pid_1_5 and pid_1_8 scopes These three scopes have been advertised in scopes_supported all along with no handler behind them in claimsForScopes. A request naming one - which is exactly what vc's apigw sends, one credential type at a time (Scopes: []string{session.CredentialType}) - matched none of profile/email/organisation/ehic, fell through to the backwards-compat block and released four claims: map[email:... family_name:... given_name:... name:...] No birthdate, no nationality, no place of birth, no issuing authority. Not enough to build a PID from, so the issuer rejects the credential request outright. Advertising a scope the OP has no claims for is the bug; a client has no way to discover that the scope is inert. ARF 1.5 and ARF 1.8 describe the same person under different claim NAMES, and apigw uses the OIDC claims verbatim as the credential's document data, so a claim under the other version's name simply isn't present to disclose and a verifier's DCQL query for it won't match. The two therefore get separate branches rather than one shared set: ARF 1.8 ARF 1.5 place_of_birth birth_place nationalities [] nationality (single) age_equal_or_over age_over_NN (flat booleans) The unversioned "pid" scope follows ARF 1.8, the current version. A request naming several of these is not meaningful - they are three spellings of one credential - so the switch lets the most specific win rather than having one overwrite the other's differently-named claims. All three are added to the backwards-compat guard for the same reason "ehic" was: otherwise the block below overwrites what the handler just set. Age attributes are derived from birthdate rather than stored per user, so they cannot go stale as the fixture data ages. Without them a verifier asking for age_over_18 (1.5) or age_equal_or_over.18 (1.8) finds nothing to match, since DCQL requires every requested claim to be present. The birthday comparison uses month/day, not day-of-year: in a leap year every date after February shifts by one day-of-year, which would report the wrong age for a full day around each such birthday. Tested: both vocabularies release their own names and none of the other's; the unversioned scope tracks 1.8; a user with no birthdate yields no age claims at all rather than empty ones; and the boundary case is checked on the day before, of, and after a birthday. The fallthrough regression test was confirmed to fail against the previous behaviour, reproducing the exact four-claim map above. --- cmd/op/main.go | 134 +++++++++++++++++++++++++++++++++++++++-- cmd/op/main_test.go | 143 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 4 deletions(-) diff --git a/cmd/op/main.go b/cmd/op/main.go index 9bd28b7..fd60b94 100644 --- a/cmd/op/main.go +++ b/cmd/op/main.go @@ -527,11 +527,44 @@ func claimsForScopes(user *users.User, scopeStr string) map[string]any { } } + // "pid" / "pid_1_8" / "pid_1_5" scopes → Person Identification Data. + // + // These three have been advertised in scopes_supported all along with no + // handler behind them, so a request naming one fell through to the + // backwards-compat block below and got four claims - given_name, + // family_name, name, email - with no birthdate, nationality, place of + // birth or issuing authority. That is not enough to build a PID from, so + // the issuer rejects the credential request outright. + // + // ARF 1.5 and ARF 1.8 describe the same person with different claim NAMES, + // and apigw uses the OIDC claims verbatim as the credential's document + // data - a claim under the other version's name simply isn't present to + // disclose, and a verifier's DCQL query for it won't match. So the two get + // separate branches rather than one shared set: + // + // ARF 1.8 ARF 1.5 + // place_of_birth birth_place + // nationalities [] nationality (single) + // age_equal_or_over age_over_NN (flat booleans) + // + // "pid" (the unversioned scope) follows ARF 1.8, the current version. + // A request naming several of these is not meaningful - they are three + // spellings of one credential - so the most specific one wins rather than + // letting one overwrite the other's differently-named claims. + switch { + case has("pid_1_5"): + addPIDClaims(claims, user, false) + case has("pid_1_8"), has("pid"): + addPIDClaims(claims, user, true) + } + // If no recognized scopes matched, return all claims (backwards compat). - // "ehic" counts as recognized: without it here, an ehic-only request (which - // is what apigw sends - Scopes: []string{session.CredentialType}) would - // fall through and overwrite the EHIC claims above with profile ones. - if !has("profile") && !has("email") && !has("organisation") && !has("ehic") { + // Every scope handled above must be listed here: otherwise a request for + // exactly that scope - which is what apigw sends, one credential type at a + // time - matches nothing, falls through, and overwrites the claims that + // handler just set with the generic profile ones. + if !has("profile") && !has("email") && !has("organisation") && !has("ehic") && + !has("pid") && !has("pid_1_5") && !has("pid_1_8") { claims["given_name"] = user.GivenName claims["family_name"] = user.FamilyName claims["name"] = user.Name @@ -541,6 +574,99 @@ func claimsForScopes(user *users.User, scopeStr string) map[string]any { return claims } +// addPIDClaims writes the Person Identification Data claims for one ARF +// version into claims. arf18 selects the ARF 1.8 vocabulary; false selects +// ARF 1.5. See the call site for why the two can't share one claim set. +// +// Only non-empty values are written, so a user carrying no PID data yields no +// PID claims rather than empty ones (same rule the ehic branch follows) - an +// empty string is a value the issuer would faithfully put in the credential. +func addPIDClaims(claims map[string]any, user *users.User, arf18 bool) { + if user.GivenName != "" { + claims["given_name"] = user.GivenName + } + if user.FamilyName != "" { + claims["family_name"] = user.FamilyName + } + if user.Birthdate != "" { + claims["birthdate"] = user.Birthdate + } + if user.IssuingAuthority != "" { + claims["issuing_authority"] = user.IssuingAuthority + } + if user.IssuingCountry != "" { + claims["issuing_country"] = user.IssuingCountry + } + + if arf18 { + if user.PlaceOfBirth != "" { + claims["place_of_birth"] = user.PlaceOfBirth + } + if len(user.Nationalities) > 0 { + claims["nationalities"] = user.Nationalities + } + } else { + if user.PlaceOfBirth != "" { + claims["birth_place"] = user.PlaceOfBirth + } + // ARF 1.5 carries a single nationality, not a list. + if len(user.Nationalities) > 0 { + claims["nationality"] = user.Nationalities[0] + } + } + + // Age attributes are derived from birthdate rather than stored per user, + // so they can't go stale as the fixture data ages. Without them a verifier + // asking for age_over_18 (ARF 1.5) or age_equal_or_over.18 (ARF 1.8) finds + // nothing to match - DCQL requires every requested claim to be present. + age, born, ok := ageInYears(user.Birthdate, time.Now()) + if !ok { + return + } + claims["age_in_years"] = age + claims["age_birth_year"] = born.Year() + + thresholds := []int{14, 16, 18, 21, 65} + if arf18 { + over := make(map[string]bool, len(thresholds)) + for _, t := range thresholds { + over[fmt.Sprintf("%d", t)] = age >= t + } + claims["age_equal_or_over"] = over + return + } + for _, t := range thresholds { + claims[fmt.Sprintf("age_over_%d", t)] = age >= t + } +} + +// pidBirthdateLayout is the full-date form RFC 7519 / OpenID Connect use for +// the birthdate claim, and the form users.yaml is written in. +const pidBirthdateLayout = "2006-01-02" + +// ageInYears returns the user's completed years of age at now, along with the +// parsed birthdate. ok is false if birthdate is empty, not a full date, or in +// the future - in which case no age attribute can be derived and none should +// be emitted. +// +// The birthday check compares month and day rather than day-of-year: in a leap +// year every date after February shifts by one day-of-year, which would report +// the wrong age for a whole day around each such birthday. +func ageInYears(birthdate string, now time.Time) (int, time.Time, bool) { + born, err := time.Parse(pidBirthdateLayout, birthdate) + if err != nil { + return 0, time.Time{}, false + } + age := now.Year() - born.Year() + if now.Month() < born.Month() || (now.Month() == born.Month() && now.Day() < born.Day()) { + age-- + } + if age < 0 { + return 0, time.Time{}, false + } + return age, born, true +} + // --- Helpers --- func verifyPKCE(verifier, challenge, method string) bool { diff --git a/cmd/op/main_test.go b/cmd/op/main_test.go index d18444a..5668d14 100644 --- a/cmd/op/main_test.go +++ b/cmd/op/main_test.go @@ -2,6 +2,7 @@ package main import ( "testing" + "time" "github.com/sirosfoundation/mini-oidc/internal/users" ) @@ -149,3 +150,145 @@ func TestClaimsForScopesEHICOmittedWhenNil(t *testing.T) { } } } + +func pidUser() *users.User { + u := testUser() + u.Birthdate = "1990-01-15" + u.PlaceOfBirth = "Stockholm" + u.Nationalities = []string{"SE", "NO"} + u.IssuingAuthority = "Swedish Tax Agency" + u.IssuingCountry = "SE" + return u +} + +// apigw requests the credential type as the sole scope +// (Scopes: []string{session.CredentialType}), so a bare "pid_1_8" is the shape +// that actually reaches the OP during issuance - not "profile". +func TestClaimsForScopesPIDARF18(t *testing.T) { + claims := claimsForScopes(pidUser(), "openid pid_1_8") + + if claims["birthdate"] != "1990-01-15" { + t.Fatalf("expected birthdate, got %v", claims) + } + if claims["place_of_birth"] != "Stockholm" { + t.Fatalf("expected ARF 1.8 place_of_birth, got %v", claims) + } + nats, ok := claims["nationalities"].([]string) + if !ok || len(nats) != 2 { + t.Fatalf("expected ARF 1.8 nationalities list, got %T %v", claims["nationalities"], claims["nationalities"]) + } + if claims["issuing_authority"] != "Swedish Tax Agency" || claims["issuing_country"] != "SE" { + t.Fatalf("expected issuing authority/country, got %v", claims) + } + + // ARF 1.5 spellings must NOT appear: a claim under the wrong version's + // name is not present to disclose, and pollutes the credential besides. + for _, k := range []string{"birth_place", "nationality", "age_over_18"} { + if _, ok := claims[k]; ok { + t.Fatalf("ARF 1.8 must not emit the 1.5 claim %q, got %v", k, claims) + } + } + + over, ok := claims["age_equal_or_over"].(map[string]bool) + if !ok { + t.Fatalf("expected age_equal_or_over map, got %T", claims["age_equal_or_over"]) + } + if !over["18"] { + t.Fatalf("a person born in 1990 is over 18, got %v", over) + } + if over["65"] { + t.Fatalf("a person born in 1990 is not yet 65, got %v", over) + } +} + +func TestClaimsForScopesPIDARF15(t *testing.T) { + claims := claimsForScopes(pidUser(), "openid pid_1_5") + + if claims["birth_place"] != "Stockholm" { + t.Fatalf("expected ARF 1.5 birth_place, got %v", claims) + } + if claims["nationality"] != "SE" { + t.Fatalf("expected ARF 1.5 single nationality, got %v", claims["nationality"]) + } + if claims["age_over_18"] != true || claims["age_over_65"] != false { + t.Fatalf("expected flat ARF 1.5 age booleans, got %v", claims) + } + + for _, k := range []string{"place_of_birth", "nationalities", "age_equal_or_over"} { + if _, ok := claims[k]; ok { + t.Fatalf("ARF 1.5 must not emit the 1.8 claim %q, got %v", k, claims) + } + } +} + +// The unversioned "pid" scope follows the current ARF version. +func TestClaimsForScopesPIDUnversionedFollowsARF18(t *testing.T) { + claims := claimsForScopes(pidUser(), "openid pid") + + if _, ok := claims["age_equal_or_over"]; !ok { + t.Fatalf("expected \"pid\" to use the ARF 1.8 vocabulary, got %v", claims) + } + if _, ok := claims["nationality"]; ok { + t.Fatalf("expected \"pid\" not to use ARF 1.5 spellings, got %v", claims) + } +} + +// Regression guard for the bug this fixes: these scopes were advertised in +// scopes_supported with no handler, so they fell through to the +// backwards-compat block and released four claims - nowhere near enough to +// build a PID, which made the issuer reject the whole credential request. +func TestClaimsForScopesPIDDoesNotFallThrough(t *testing.T) { + for _, scope := range []string{"pid", "pid_1_5", "pid_1_8"} { + claims := claimsForScopes(pidUser(), "openid "+scope) + if _, ok := claims["name"]; ok { + t.Fatalf("%s must not fall through to the backwards-compat claims, got %v", scope, claims) + } + if _, ok := claims["birthdate"]; !ok { + t.Fatalf("%s released no birthdate - the fallthrough bug is back: %v", scope, claims) + } + } +} + +func TestClaimsForScopesPIDOmitsAgeWithoutBirthdate(t *testing.T) { + u := pidUser() + u.Birthdate = "" + claims := claimsForScopes(u, "openid pid_1_8") + + for _, k := range []string{"birthdate", "age_equal_or_over", "age_in_years", "age_birth_year"} { + if _, ok := claims[k]; ok { + t.Fatalf("expected no %s for a user without a birthdate, got %v", k, claims) + } + } +} + +func TestAgeInYearsBirthdayBoundary(t *testing.T) { + // Day before, day of, and day after an 18th birthday. The day-of case is + // why this compares month/day rather than day-of-year: 2026 is not a leap + // year but 2008 was, so the two dates' day-of-year numbers differ. + for _, tc := range []struct { + now string + want int + }{ + {"2026-06-19", 17}, + {"2026-06-20", 18}, + {"2026-06-21", 18}, + } { + now, err := time.Parse("2006-01-02", tc.now) + if err != nil { + t.Fatal(err) + } + got, _, ok := ageInYears("2008-06-20", now) + if !ok || got != tc.want { + t.Fatalf("ageInYears at %s = %d (ok=%v), want %d", tc.now, got, ok, tc.want) + } + } +} + +func TestAgeInYearsRejectsUnparseableOrFuture(t *testing.T) { + now, _ := time.Parse("2006-01-02", "2026-06-20") + for _, bd := range []string{"", "not-a-date", "1990", "2030-01-01"} { + if _, _, ok := ageInYears(bd, now); ok { + t.Fatalf("expected ageInYears(%q) to report no derivable age", bd) + } + } +}