-
Notifications
You must be signed in to change notification settings - Fork 192
fix: consistently handle string-valued boolean fields from non-compliant OIDC providers #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: consistently handle string-valued boolean fields from non-compliant OIDC providers #791
Conversation
f546b4f to
3bd4db0
Compare
Please don't use this string in your PR comment. It get's converted to the commit message later and it will trigger the semrel tool to create a new major release. |
|
Heyo @bashhack checking if you'll have some time to come back to this PR? |
|
@elinashoko - yeah! Been swamped on my end, but should have time by end of this weekend to wrap this up - thanks for being so patient all 🤝 |
751e5ae to
7ace312
Compare
|
@muhlemmer + @elinashoko - should be all set here, I think the end approach is a nice balance, thanks again! PR feedback implemented, and branch updated accordingly with Congrats on the 1.25+ support, too! 🎉 |
|
Thank you kindly @bashhack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more small comment and we are good to merge.
| // | ||
| // For broader historical context, see: | ||
| // - https://github.com/zitadel/oidc/pull/139 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to link the PR. If people want to find the source of a change, they can git blame.
| // | |
| // For broader historical context, see: | |
| // - https://github.com/zitadel/oidc/pull/139 |
|
@bashhack pls have a look when you have a chance, let's get this over the line! |
Background
👋 @muhlemmer - hope all is well with you and the team!
AWS Cognito (and potentially other providers) return
email_verifiedandphone_number_verifiedas strings ("true"/"false") instead of proper JSON booleans, violating the OIDC specification.AWS Documentation confirms this:
Source: https://docs.aws.amazon.com/cognito/latest/developerguide/userinfo-endpoint.html#get-userinfo-response-sample
The Problem
The
zitadel/oidclibrary currently handles this inconsistently:EmailVerifieduses the customBooltype (added in feat: Enable parsing email_verified from string. #139)PhoneNumberVerifieduses Go's standardboolThis forces developers to handle semantically identical fields differently:
Additionally, the existing
Bool.UnmarshalJSONimplementation meant that false values couldn't overwrite true.Solution
Applied
Booltype consistently to both fields and simplifiedBool.UnmarshalJSONusing a direct switch statement to:Updated tests to match codebase conventions, as well.
Impact
PhoneNumberVerifiedchanges frombooltoBool(type alias ofbool). Most consumer code should work as-is sinceBoolis just a type alias. Direct type assertions would need updating.Definition of Ready