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

Apple sign in client_id typo #6858

Closed
chr4ss1 opened this issue Aug 12, 2020 · 8 comments · Fixed by #6861
Closed

Apple sign in client_id typo #6858

chr4ss1 opened this issue Aug 12, 2020 · 8 comments · Fixed by #6861
Labels
type:feature New feature or improvement of existing feature

Comments

@chr4ss1
Copy link

chr4ss1 commented Aug 12, 2020

In documentation https://docs.parseplatform.org/parse-server/guide/#apple-authdata it is mentioning that you can use client_id. That might have been true in the past, but now the correct way is to use clientId.

@mtrezza
Copy link
Member

mtrezza commented Aug 12, 2020

Thanks for reporting. This may be an issue with the docs and therefore belong to the docs repository, however before we transfer this, I want to take a closer look at the Apple auth adapter.

The test cases seem to use both clientId and client_id as parameters:

await apple.validateAuthData({}, { clientId: 'secret' });

await apple.validateAuthData({}, { client_id: ['secret'] });

@drdaz Since you worked on Sign-in with Apple on iOS recently, do you have any insight into the Apple auth adapter and why we have two different parameters in the test cases?

@drdaz
Copy link
Member

drdaz commented Aug 12, 2020

@drdaz Since you worked on Sign-in with Apple on iOS recently, do you have any insight into the Apple auth adapter and why we have two different parameters in the test cases?

I can't say that I do. But I took a look at the API history for the client config: https://developer.apple.com/documentation/sign_in_with_apple/clientconfigi?changes=latest_minor

The clientId name appears to have been consistent across releases.

@drdaz
Copy link
Member

drdaz commented Aug 12, 2020

@UnderratedDev As the committer, can you comment on this?

@drdaz
Copy link
Member

drdaz commented Aug 12, 2020

I've been sniffing around the history for the Apple auth adapter, and it looks like we did use the client_id name internally in earlier revisions. Looks like we tried to stop doing that here 8e0e485.

@UnderratedDev
Copy link
Contributor

There isn't one particular reason to change from client_id to clientId. Mainly it was to be consistent with the rest of the code, for instance, we use keyId, verifyIdToken. Most of the code uses the camel case javascript style. We should be using just clientId.

Those tests also need to be reworked as we don't have any parse credentials for apple. We have to input our own & generate our own tokens manually with a separate application.

Also, side note, I made a pr for p8 file support for apple sign but it needs better testing (I don't wish to put in my p8 file or personal credentials). If anyone could help, that would be appreciated. #6648 Hope that clears it all up.

@mtrezza
Copy link
Member

mtrezza commented Aug 13, 2020

I think there are some aspects to distinguish:

  • the Parse Server internal parameter syntax
  • the key name that is transmitted to Apple's auth endpoint to verify a token

The authentication can be done in different ways:

  • for the Apple Web Service Endpoint (REST API) the key name is client_id
  • for the Apple Sign in JS Framework it is clientId

It seems we are using neither directly, instead we use the JWT framework (which uses the Apple Web Service Endpoint) which has an audience parameter for which the value of the Parse Server auth adapter configuration clientId key is set:

jwtClaims = jwt.verify(token, signingKey, {
algorithms: algorithm,
// the audience can be checked against a string, a regular expression or a list of strings and/or regular expressions.
audience: clientId,
});

My conclusion would be:

  • @ChrisEelmaa is correct in that the docs need to updated to use clientId instead of client_id, because this has been missed in the docs during the syntax refactoring in 8e0e485 that @drdaz pointed out --> @ChrisEelmaa can you please open a new issue in the docs repository? Instead of transferring this issue to the docs, I would rather keep it here because the discussion became more Parse Server auth adapter centric. I opened the docs issue 📙 Update apple auth adapter key docs#761
  • the syntax refactoring in 8e0e485 missed several parameters in the test cases for the Apple auth, that should be refactored for consistency --> requires a PR assigned to this issue

@drdaz, @UnderratedDev would you both agree to the conclusion?

@mtrezza mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed and removed needs more info labels Aug 13, 2020
@UnderratedDev
Copy link
Contributor

@mtrezza yep, I do remember using the Apple Sign in JS Framework & they do use clientId. Good catch!

@drdaz
Copy link
Member

drdaz commented Aug 14, 2020

Yes... conclusion checks out. Nice.

@mtrezza mtrezza added enhancement and removed type:bug Impaired feature or lacking behavior that is likely assumed labels Aug 15, 2020
@mtrezza mtrezza reopened this Aug 25, 2020
@mtrezza mtrezza closed this as completed Aug 25, 2020
@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants