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

Failed userinfo unmarshal with AWS Cognito due to email_verified being text field. #137

Closed
jozuenoon opened this issue Oct 26, 2021 · 9 comments · Fixed by #139
Closed
Labels
enhancement New feature or request

Comments

@jozuenoon
Copy link
Contributor

jozuenoon commented Oct 26, 2021

Describe the bug
AWS Cognito returns userinfo with email_verified field as text.

"userinfo failed: failed to unmarshal response: json: cannot unmarshal string into Go struct field .email_verified of type bool..."

To Reproduce
Steps to reproduce the behavior:

  1. Configure AWS Cognito with the example from the examples folder.
  2. Try to log in.

Expected behavior
Should read userinfo.

@jozuenoon jozuenoon added the bug Something isn't working label Oct 26, 2021
@fforootd
Copy link
Member

HI @jozuenoon

Thanks for reporting this issue. Can you provide some more information about the error you receive?
For example the http response headers would we interesting to see.

CC @caos/zitadel

@jozuenoon
Copy link
Contributor Author

jozuenoon commented Oct 26, 2021

Hi! Response headers don't contain anything interesting I think. I can drop full error response:

userinfo failed: failed to unmarshal response: json: cannot unmarshal string into Go struct field .email_verified of type bool {"sub":"xxx","email_verified":"true","phone_number_verified":"true","phone_number":"xxx","email":"xxx","username":"xxx"}

I'm running callback with rp.UserinfoCallback(marshalUserinfo) from the example folder.
The issue is that the code unmarshals response body into this:

type userInfoEmail struct {
	Email         string `json:"email,omitempty"`
	EmailVerified bool   `json:"email_verified,omitempty"`
}

Replacing boolean with custom type with UnmarshalJSON() should do the trick I think (handling both bool and string).

@hifabienne
Copy link
Member

Hi @jozuenoon

We have implemented the library using the OpenID spec. The specs defines the email verified attribute as a boolean.
https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

In your request the field is a text and because of that an error is returned. In this case the error is correct.

We had a look at the aws docs and they refer to the standard claims docs of OpenID.
https://docs.aws.amazon.com/cognito/latest/developerguide/userinfo-endpoint.html
Did you have to configure something to get the claims? Is it possible that something is wrong there?

@jozuenoon
Copy link
Contributor Author

jozuenoon commented Oct 26, 2021

Hi!
Seems like what is returned in IDTokenClaims is correct so email_verified is boolean. However, the /userInfo endpoint returns it as a string.

It's an inconsistency on the AWS side. However, the problem seems to be recognized and ignored eg. https://forums.aws.amazon.com/thread.jspa?messageID=949441&#949441

Maybe it's possible to change this bug report into enhancement and gracefully handle boolean conversions?

@fforootd
Copy link
Member

Well that's a bummer with AWS 😢

Do you want to contribute a PR for this?

@fforootd fforootd added enhancement New feature or request and removed bug Something isn't working labels Oct 27, 2021
@fforootd
Copy link
Member

For reference other libs have to work around that issues as well

@jozuenoon
Copy link
Contributor Author

I also found a similar issue with Auth0 today: https://community.auth0.com/t/userinfo-endpoint-in-oidc-returns-updated-at-field-as-a-string/62754

The library can't unmarshal updated_at field but it's actually both userinfo and IDTokenClaims which are affected. I can for now work around this by removing profile from scope.

I will try to wrap some fixes up and PR next week 👍

@fforootd
Copy link
Member

It is astonishing how many spec. violations are in the wild 😁

Thanks for your time 👍

@jozuenoon
Copy link
Contributor Author

This PR is for Cognito #139. I will try to scratch something for Auth0 as well.

@fforootd fforootd linked a pull request Nov 2, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants