-
Notifications
You must be signed in to change notification settings - Fork 114
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: incorrect assignment of displayName
for Apple OAuth
#425
Conversation
🦋 Changeset detectedLatest commit: 7a63363 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Hi, I haven't tested this yet and I don't know how to setup hasura-auth to my local. but based from docs, I believe this should be the correct response |
Thanks a lot for the PR. To test locally you can run:
Then, if you have an nhost project using the cli you can temporarily set the auth version to When you manage to test it and verify it works send us a few screenshots capturing the result (or even a loom video) and we will merge. If you need help with the test don't hesitate to ping me on discord. |
@dbarrosop Will do. thanks! |
Here are my local tests with the following videos.
cb54599f-5839-4d11-89c9-2e4f3a21dbc8.mp4
c8ea1b69-6cb7-4144-bd5f-63ab2964d4f8.mp4
0042d6df-40ab-4514-884b-d67b75921718.mp4Please do final tests. I'm not too familiar with the whole project, I might break some stuff. Thank you |
The 7cc7422 commit fixes an issue where if an existing Apple user reauthenticates, it breaks. Here are the following behaviours:
Solution: I wrapped it in a try catch. Let me know if you want to have requested changes for this one. Test Video: bd55d775-c1a1-4892-b512-1d718b848263.mp4 |
displayName
displayName
for Apple OAuth
@totzk9 Thank you for the PR 🙌 - Will look into this now |
Thank you again @totzk9 for the PR 👍 . I've made some adjustments and added a changeset. |
src/routes/oauth/config.ts
Outdated
: displayName; | ||
} catch (error) { | ||
logger.warn( | ||
`Problem trying to parse user data from apple's response: ${error}` |
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.
nitpicking but I'd add , default to email
https://developer.apple.com/documentation/sign_in_with_apple/request_an_authorization_to_the_sign_in_with_apple_server#4066168
or
https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_js/incorporating_sign_in_with_apple_into_other_platforms#3332115