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: Add features missing for Apple provider #8189

Closed
wants to merge 9 commits into from

Conversation

ChrGrb
Copy link
Contributor

@ChrGrb ChrGrb commented Jul 31, 2023

☕️ Reasoning

Changes to routes/callback.ts

  • If the response_mode of authorization is set to 'form_post', the information will be returned in the body and not the query. Therefor the callback handler should check for the information there to pass it on to the handleOAuth function

Changes to providers/oauth.ts

  • The Apple provider returns the user information with the authorization response. There should be a way to get to this data in the provider configuration

Changes to providers/apple.ts

  • The apple token endpoint expects authentication data to be passed in the body. Therefor the token_endpoint_auth_method has to be set to 'client_secret_post'
  • wellKnown endpoint should be defined in the default provider
  • authorization url should be defined in the default provider
  • authorization params should be set correctly
  • token url should be defined in the default provider
  • The profile function should correctly handle the available data
  • The documentation should mention the necessary pkceCodeVerifier cookie

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes:

📌 Resources

@ChrGrb ChrGrb requested a review from ThangHuuVu as a code owner July 31, 2023 18:54
@vercel
Copy link

vercel bot commented Jul 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
auth-docs ❌ Failed (Inspect) Aug 8, 2023 4:28pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Aug 8, 2023 4:28pm

@vercel
Copy link

vercel bot commented Jul 31, 2023

@ChrGrb is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@balazsorban44
Copy link
Member

I'm hesitant about merging this yet, as we don't want to make exceptions for handling providers specifically in the core library. Apple is making this hard here, not adhering to the OAuth spec... User information should be in id_token, and not part of a query param. 😬

I recently figured out how to finally test an Apple provider (had to pay for it 👎) myself, so I'll think about it a bit more.

*Venting*: Apple really does not want people to use their provider. 😅. By far the most convoluted/worst documented provider, and not even free...

@ChrGrb
Copy link
Contributor Author

ChrGrb commented Aug 3, 2023

I 100% agree with what you say. Figuring out what is going wrong and why is a nightmare with the Apple provider and changing the core did feel off.

I did have 2 possible alternatives to this, that could be considered:

Make the provider usable with the current core functionality
By changing the authorization params to

scope: "",
response_mode: "query",

the response is returned in the url, instead of the body.
This would limit the functionality in so far that the call does neither provide an email, nor a name. But the general sign in works and the id_token is still returned.
The email and name could then be requested and stored in the database on first signIn.

Add the ability to provide a custom handleOAuth function in the provider configuration
This would move the bulk of changes to the Apple provider configuration (except for determining whether the data of the callback is in the body or query). On the other hand I assume that you have a good reason, why this is not yet an implemented feature.

Either way, if you need any assistance or get stuck, just hit me up. I know how hard it is to find documentation or information on this topic.

This article highlights some of the differences between OAuth2 and the Apple version pretty well. It also explains, why the pkce cookie does not arrive back with the callback as expected: https://www.bscotch.net/post/sign-in-with-apple-implementation-hurdles

@ChrGrb
Copy link
Contributor Author

ChrGrb commented Aug 3, 2023

Doing some more digging I found that Apple actually does adhere to the OIDC spec. This is the actual, expected behaviour for an OAuth2.0 form_post request. It is just that apparently no other provider in Auth.js uses this. https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html#FormPostResponseExample

This does, in my opinion, justify changing the core. It is practically speaking not changing it for a single provider, but instead for all potential future providers using the same form_post spec.

@balazsorban44
Copy link
Member

My ick is with specifically returning the user data in a user search param as s stringified JSON objoect. That is not part of the spec AFAICT.

Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert the last changes, it's not the correct way to handle this IMO.

@balazsorban44
Copy link
Member

(closed by accident, sorry).

Add the ability to provide a custom handleOAuth function in the provider configuration

I'm thinking of something similar.

See

if (provider.token?.conform) {
codeGrantResponse =
(await provider.token.conform(codeGrantResponse.clone())) ??
codeGrantResponse
}

Here, if a provider is not adhering to the spec, we let them call into this .conform method, signaling that this is really on the provider and not on us. Also warning for the users so they can let us know when a provider finally fixed their stuff.

An example is Twitch:

async conform(response) {
const body = await response.json()
if (response.ok) {
if (typeof body.scope === "string") {
console.warn(
"'scope' is a string. Redundant workaround, please open an issue."
)
} else if (Array.isArray(body.scope)) {
body.scope = body.scope.join(" ")
return new Response(JSON.stringify(body), response)
} else if ("scope" in body) {
delete body.scope
return new Response(JSON.stringify(body), response)
}
} else {
const { message: error_description, error } = body
if (typeof error !== "string") {
return new Response(
JSON.stringify({ error: "invalid_request", error_description }),
response
)
}
console.warn(
"Response has 'error'. Redundant workaround, please open an issue."
)
}
},
},

@ChrGrb
Copy link
Contributor Author

ChrGrb commented Aug 8, 2023

I tried my best to implement your idea. I could not figure out a way on how to do it with only the existing conform methods, as the authorization.conform is afaict not being used.

What I did instead is create a provider.profileConform(profile, query) method that does, what the changes to the provider.profile(..) callback have done before. I feel like this is more readable, but in the end it still comes down to basically the same changes to the core.

Additionally I added the provider.token.url back in. While it is fetched from the issuer, Apple does not provide a userinfo endpoint, which then leads to an error. Defining it skips the discovery phase and the callback succeeds.

@hesselbom
Copy link

May I ask if there's any progress in merging this PR?

I'm currently using the fallback suggested here but with that the email is reported as null since it can't be part of the scope.

@leo3linbeck
Copy link

I can successfully sign into Apple, but this issue continues to be a problem for me since the failure to be able to get an email returned from Apple on a Vercel implementation makes the Apple provier unusable for my purposes.

After digging into it, it looks like the fix should be pretty simple: since Apple returns the state in the body of a POST (which one would expect for a POST, and is frankly better security practice) rather than as a GET query parameter (which is what the state check is looking for), the following approach should address the problem:

  1. In lib/actions/callback/index.ts:
      const { proxyRedirect, randomState } = handleState(
        query,
        body,
        provider,
        options.isOnRedirectProxy
      )

  1. In the handleState function in lib/actions/callback/oauth/checks.ts, the state can taken from query or body, depending on which is non falsy, and proceed accordingly.

I'd try to fix this myself, except I've never tried to submit a pull request and I'm on a tight time deadline on this project and the other providers (Google, Facebook, Auth0, and Github) all work fine so I can proceed without Apple. But given the ubiquity of iPhones, it would be great if Apple would return an email address in a Vercel implementation.

Love this library, and appreciate all of the hard work that has gone into what is an elegant solution to a tough problem.

Cheers,
L3

@tristan-warner-smith
Copy link

We've hit this issue ourselves and, short of modifying the core which I don't feel qualified to do, it doesn't seem we have any recourse on this other than to hope this PR exposing the form data vs query approach gets merged.

We'd really appreciate if this could keep moving forward 🙏

@xixixao xixixao mentioned this pull request Jul 25, 2024
3 tasks
@JamesWalkerGit
Copy link

JamesWalkerGit commented Sep 19, 2024

I've run into the same issue, it seems the Apple Auth simply doesn't work currently. I've tried 5.0.0-beta.20 and 5.0.0-beta.21 following the documentation and the issue is a server error that occurs after successfully logging in and authenticating on the Apple side then redirecting to the callback url which throws an error.

I've also tried some workarounds that were posted but they did not work either.

#6788 (comment)

@blinton-sc
Copy link

Is there a reason this wasn't merged? We are blocked due to this and so far none of the workarounds we've tried work.

It would be awesome if we could get an update here @ChrGrb @balazsorban44 @ThangHuuVu

@balazsorban44
Copy link
Member

Is there a reason this wasn't merged? We are blocked due to this and so far none of the workarounds we've tried work.

You are not blocked. You can copy-paste the code whenever you want and use something like pnpm patch, the source is... open.

I honestly just don't like that we would need a workaround like this, just for the sake of Apple...

My opinion is the same as #8189 (comment), Apple is one of the worst providers to work with, by far...

@JamesWalkerGit
Copy link

JamesWalkerGit commented Oct 2, 2024

@balazsorban44

Does this mean that Auth.js will not support Apple login, and that the documentation around Apple OAuth support needs to be removed?

Apple Documentation

For me it's a pretty big deal, Apple is one of the largest mobile device markets so to not have their login is a very large negative for me.

To be fair it clearly isn't following standards and would require more work to deal with the disappearing email, I may rethink using it altogether but it's an annoying gap for sure.

@balazsorban44
Copy link
Member

balazsorban44 commented Oct 6, 2024

Never said we don't want to support it. Just not the way it's presented in this PR.

Recently merged #11975 which might help resolve this. Maybe not. Maybe it's gonna be like adbe451

I've spent years pushing the standards, and removing workarounds from the core. I trialed Twitter OAuth 2 before it came out and gave them early feedback so it would be compliant. I talked LinkedIn into fixing their issuer.

Microsoft and Apple will be too much for me, I guess, and have to cave in and add hacks to the core to support them. And at the end, people are still mad at me for not shipping a solution quicker...

@JamesWalkerGit
Copy link

@balazsorban44 Ok awesome, thanks for the insight, I definitely appreciate your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core` providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants