Skip to content

Conversation

@cemalkilic
Copy link
Contributor

@cemalkilic cemalkilic commented Dec 1, 2025

Summary

This PR loosens the validation for the amr (Authentication Method Reference) claim in custom access token hooks to accept both array of strings and array of objects, instead of only array of objects.

  • Test Coverage: Added two new test cases:
    • Modify amr to be array of strings - Verifies that amr as an array of strings passes validation
    • Modify amr to be array of objects - Verifies that amr as an array of objects still works (backward compatibility)

Motivation

This change provides more flexibility for custom access token hooks. RFC-8176 requires amr to be array of strings.

Testing

All tests pass, including:

  • Existing custom access token tests
  • New test for amr as array of strings
  • New test for amr as array of objects

Backward Compatibility

Fully backward compatible - The change only adds support for an additional format. Existing hooks that return amr as an array of objects will continue to work without any changes.

@cemalkilic cemalkilic requested a review from a team as a code owner December 1, 2025 06:13
"items": {
"type": "object"
"anyOf": [
{"type": "string"},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Severity: HIGH

Type mismatch causes authentication failures: The schema now allows amr as array of strings, but the AccessTokenClaims struct expects []models.AMREntry (array of objects with method/timestamp fields). When tokens created by custom hooks with string-based amr claims are later parsed via ParseWithClaims(&AccessTokenClaims{}), Go's JSON unmarshaler will fail attempting to unmarshal strings into struct types, causing all authentication requests using these tokens to fail. The struct definition must be updated to support both formats before this schema change can be deployed.
Helpful? Add 👍 / 👎

Suggestion: The fix requires modifying the AccessTokenClaims struct at line 38 (not line 955 where the schema is). Create a custom AMR type that implements json.Unmarshaler to handle both string arrays and AMREntry object arrays. Steps: (1) Define a new type 'type AMRClaim []models.AMREntry' in internal/tokens/service.go, (2) Implement UnmarshalJSON method on AMRClaim that checks if each array element is a string or object and converts accordingly (strings can be converted to AMREntry{Method: string, Timestamp: time.Now().Unix()}), (3) Update line 38 to use 'AuthenticationMethodReference AMRClaim' instead of '[]models.AMREntry'. This ensures backward compatibility while supporting the new string format allowed by the schema at line 955.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 19813188853

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 68.269%

Totals Coverage Status
Change from base Build 19743508420: 0.0%
Covered Lines: 14441
Relevant Lines: 21153

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants