Skip to content
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

fix: ignore all unmarshal errors from locale #673

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

muir
Copy link
Contributor

@muir muir commented Oct 31, 2024

This changes unmarshal of Locale so that rather than ignoring just language.ValueError errors, it ignores all errors. Generally speaking, a bad locale value should not prevent users from authenticating.

Resolves #672

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.94%. Comparing base (0992c5f) to head (8475675).
Report is 101 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #673      +/-   ##
==========================================
+ Coverage   60.06%   60.94%   +0.88%     
==========================================
  Files          80       81       +1     
  Lines        6998     7454     +456     
==========================================
+ Hits         4203     4543     +340     
- Misses       2498     2601     +103     
- Partials      297      310      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@muhlemmer muhlemmer self-requested a review November 1, 2024 08:47
@muhlemmer muhlemmer changed the title [fix] ignore all unmarshal errors from locale fix: ignore all unmarshal errors from locale Nov 1, 2024
@muhlemmer
Copy link
Collaborator

I'm approving this fix as I understand the rationale that locale is not so important as failed authentications. However, I have a dislike of ignoring errors in general, as it may hamper debug efforts. If this ever backfires in complaints where people couldn't figure out why the Locale is missing (silent error) we might rollback this change.

I do recommend pressuring Okta to do their job and fix this on their side for the sake of long-term stability.

@muhlemmer muhlemmer merged commit fbf009f into zitadel:main Nov 1, 2024
7 checks passed
Copy link

github-actions bot commented Nov 1, 2024

🎉 This PR is included in version 3.32.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@muir
Copy link
Contributor Author

muir commented Nov 1, 2024

@muhlemmer We could preserve the raw value sent somewhere in the object and allow it to be interrogated. Would you like me to do that?

@muhlemmer
Copy link
Collaborator

The raw values should already be available in the Userinfo.Claims map. So no need to do that.

@muir
Copy link
Contributor Author

muir commented Nov 4, 2024

The raw values should already be available in the Userinfo.Claims map. So no need to do that.

@muhlemmer
Hmm, in that case I think the value may be ""

@muhlemmer
Copy link
Collaborator

Good one. Yes, with an empty string on the previous version I can reproduce. I was under the impression omitempty on the Userinfo field should catch that. 😕

I will open a seperate PR which reverts and tries to catch the empty string. Perhaps you can try that out instead, and if it works we can go that way.

muhlemmer added a commit that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug]: Invalid locale prevents userinfo lookup
3 participants