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

Changed state and email fields to optional to match Apple's responses #432

Merged

Conversation

intonarumori
Copy link
Contributor

@intonarumori intonarumori commented Aug 23, 2024

In this PR we updated the model objects to match Apple's responses.

The Issue

We were getting errors with the library if we omitted the state parameter in our call to getIDCredential or we were hiding our email address in the Apple Sign-in dialog. The error was pretty obscure:

TypeError: Instance of '_TypeError': type '_TypeError' is not a subtype of type 'JSObject'

What was actually happening

  • We omitted the state parameter when calling getIDCredential and then hid the email in the Apple Sign-in dialog
  • The state and email fields were omitted in the JS response from Apple
  • toDart here could not parse the JS object because the state and email fields were defined as required fields and not optionals in UserI and AuthorizationI
  • An informative parsing error was raised:
TypeError: null: type 'Null' is not a subtype of type 'String'
  • The catch block is only accounting for errors coming from Apple when trying to convert the error to SignInErrorI here
  • This failed, so another error was raised:
TypeError: Instance of '_TypeError': type '_TypeError' is not a subtype of type 'JSObject'
  • This obscured the original informative error message

The solution

  • state field has been updated to be optional: state is defined as optional when calling getIDCredential here. When not passing it, Apple will omit it in their response, thus it should be an optional field on AuthorizationI.

  • email field has been updated to optional: In various circumstances (ex. when the user uses Apple sign-in with their email hidden) this field might be omitted in the response from Apple. Thus this should also be defined as optional in UserI.

Couple of questions remain

  • Should we do the same for name.firstName and name.lastName?
    So far we were not about to clear any of these fields so it seems they are always provided, however it's hard to know we really covered all edge cases.
  • Should we improve the code in the catch block here so if parsing errors in the future arise we get more informative error messages and can fix them quicker?

@intonarumori intonarumori changed the title Changed state and email fields to optional to match Apple's implementation Changed state and email fields to optional to match Apple's responses Aug 23, 2024
@tp tp self-assigned this Aug 27, 2024
@tp
Copy link
Collaborator

tp commented Aug 27, 2024

@intonarumori Thanks for the thorough investigation.

Surely I was always testing with a state parameter, so it might be that the types here are wrong.

I will verify this locally and if it fixes the state-less and/or "second login" flow as you describe, will prepare a release ASAP.

@tp
Copy link
Collaborator

tp commented Aug 27, 2024

Should we do the same for name.firstName and name.lastName?

I think that is already working fine because the whole name object is optional, and thus won't error when this is omitted.

@tp
Copy link
Collaborator

tp commented Aug 27, 2024

Just tested this on web without any requested scopes (no name or mail), and without passing a nonce or state either.

Before (on master) it failed, and with this fix it worked fine.

So thanks a ton @intonarumori!

@tp
Copy link
Collaborator

tp commented Aug 27, 2024

Should we improve the code in the catch block here so if parsing errors in the future arise we get more informative error messages and can fix them quicker?

Yes, that would be a great follow-up. The code should not assume that it's only that specific error type. Will have to check whether an is check would work on there, and if it doesn't match forward the "raw" error.

@tp tp merged commit d335c67 into aboutyou:master Aug 27, 2024
20 checks passed
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.

2 participants