nhost / hasura-auth

Authentication for Hasura.
https://nhost.io
MIT License
376 stars 111 forks source link

fix: incorrect assignment of `displayName` for Apple OAuth #425

Closed totzk9 closed 9 months ago

totzk9 commented 10 months ago

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

changeset-bot[bot] commented 10 months ago

🦋 Changeset detected

Latest commit: 7a6336309078f2a95437885d18ccdebf44b46077

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ----------- | ----- | | hasura-auth | Patch |

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

totzk9 commented 10 months ago

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

dbarrosop commented 10 months ago

Thanks a lot for the PR.

To test locally you can run:

docker build -t nhost/hasura-auth:local .

Then, if you have an nhost project using the cli you can temporarily set the auth version to local to use this image.

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.

totzk9 commented 10 months ago

@dbarrosop Will do. thanks!

totzk9 commented 10 months ago

Here are my local tests with the following videos.

  1. Testing the current v0.21.2. The current implementation doesn't properly fetch the user's info from Apple

https://github.com/nhost/hasura-auth/assets/24382050/42dfbae7-2b0a-4e58-b5fd-83defa86e7f9

  1. I've added logs to see and compare the json data taken from jwt and profile params.

https://github.com/nhost/hasura-auth/assets/24382050/edd7e73c-dd3f-4e16-a286-ed7ad3e906b2

  1. I've encoded profile into a JSON object because it's a stringified json then used the user variable to assign it to the displayName field.

https://github.com/nhost/hasura-auth/assets/24382050/8d5e281a-495a-4c40-8a1b-d127da0d015a

Please do final tests. I'm not too familiar with the whole project, I might break some stuff.

Thank you

totzk9 commented 10 months ago

The 7cc7422 commit fixes an issue where if an existing Apple user reauthenticates, it breaks.

Here are the following behaviours:

  1. If new user signs up, then the profile param is a stringified Object thus we need to parse it using JSON.parse(profile)
  2. If user is already exists and reauthenticates, then the profile param is not a stringified Object causing the JSON.parse(profile) to throw an exception.

Solution: I wrapped it in a try catch. Let me know if you want to have requested changes for this one.

Test Video:

https://github.com/nhost/hasura-auth/assets/24382050/a4084ee0-61e9-462d-bcd2-2143dbe7c128

onehassan commented 9 months ago

@totzk9 Thank you for the PR 🙌 - Will look into this now

onehassan commented 9 months ago

Thank you again @totzk9 for the PR 👍 . I've made some adjustments and added a changeset. We will merge very soon!