use token and algo from jwt header #6416

andrewking0207 · 2020-02-17T22:10:36Z

The appleid auth token endpoint https://appleid.apple.com/auth/keys returns three different keys. The current apple auth adapter implementation always selects the first key to verify the token. This works often but if the client did not encode using the first key then the verification will fail. This fix checks the header of the token to get the key ID used for the encoding and then selects the correct public key with which to perform verification of the token.

Also selects the encryption algorithm from the header in case Apple changes what they use in the future.

andrewking0207 · 2020-02-17T22:45:10Z

I'm not sure what to do about those failing tests. They don't seem related, I'm guessing they pass locally because I don't have an actual replicaset and redis cache?

codecov · 2020-02-17T23:34:31Z

Codecov Report

Merging #6416 into master will decrease coverage by 0.67%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6416      +/-   ##
==========================================
- Coverage   93.96%   93.29%   -0.68%     
==========================================
  Files         169      169              
  Lines       11801    11989     +188     
==========================================
+ Hits        11089    11185      +96     
- Misses        712      804      +92

Impacted Files	Coverage Δ
src/Adapters/Auth/apple.js	`100.00% <100.00%> (+12.90%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`90.02% <0.00%> (-6.70%)`	⬇️
src/RestWrite.js	`93.64% <0.00%> (-0.17%)`	⬇️
src/GraphQL/ParseGraphQLSchema.js	`97.70% <0.00%> (+0.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209d6f7...b3d353a. Read the comment docs.

dplewis

Thanks for the PR. I have a few suggestions.

At some point we should move from node-rsa to jwks-rsa. As mentioned in the apple documentation, to get the public keys, we need to deal properly with json web keys (jwks) to get the signin keys.

https://developer.apple.com/documentation/signinwithapplerestapi/fetch_apple_s_public_key_for_verifying_token_signature

Here is what jwks-rsa implementation looks like.

https://auth0.com/blog/implement-sign-in-with-apple-using-auth0-extensibility/

src/Adapters/Auth/apple.js

chriscborg · 2020-02-21T05:44:11Z

Hi, for those like myself who don’t have the skillset to contribute to this issue (apologies) is there a workaround from iOS or having this fixed is the only way? Because Apple are starting to enforce the iOS13 SDK which would unfortunately mandate the sign in with Apple. Thanks

dplewis · 2020-02-21T05:55:35Z

@chriscborg signin with Apple is already supported. https://docs.parseplatform.org/parse-server/guide/#apple-authdata

This PR is to improve the existing implementation.

Can you post a link about a mandate / require this?

chriscborg · 2020-02-21T09:10:06Z

@dplewis thanks for the reply. please find the link to the Apple review guidelines mandating the Sign In with Apple if the app uses other social logins https://developer.apple.com/app-store/review/guidelines/#sign-in-with-apple

I am having an issue when doing the sign in with Apple and I thought that it was related to this issue. When I attempt the login, some times, I'm getting the below error from the server. Sometimes it succeeds, sometimes it fails with the below.

error: Uncaught internal server error.invalid signature {"name":"JsonWebTokenError","stack":"JsonWebTokenError: invalid signature\n    at /app/node_modules/jsonwebtoken/verify.js:133:19\n    at getSecret (/app/node_modules/jsonwebtoken/verify.js:90:14)\n    at Object.module.exports [as verify] (/app/node_modules/jsonwebtoken/verify.js:94:10)\n    at verifyIdToken (/app/node_modules/parse-server/lib/Adapters/Auth/apple.js:48:25)\n    at processTicksAndRejections (internal/process/task_queues.js:97:5)\n    at async Promise.all (index 0)"}

JsonWebTokenError: invalid signature
at /app/node_modules/jsonwebtoken/verify.js:133:19
at getSecret (/app/node_modules/jsonwebtoken/verify.js:90:14)
at Object.module.exports [as verify] (/app/node_modules/jsonwebtoken/verify.js:94:10)
at verifyIdToken (/app/node_modules/parse-server/lib/Adapters/Auth/apple.js:48:25)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at async Promise.all (index 0)

Do you think this is related to this issue or is it something else?

dplewis · 2020-02-22T03:42:56Z

if the app uses other social logins

Ok I though you meant it was required in every app

I’m not sure if this will fix your issue. Feel free to pull down this branch fork and try it in your app. I left a comment above about another implementation for Sign In with Apple that may solve all our issues

chriscborg · 2020-02-23T10:10:23Z

@dplewis thanks for your suggestion. @andrewking0207 @dplewis I can confirm that after forking this branch, the issue mentioned above was resolved. I look forward to seeing this merged and released. I wish I could help so if there's a way let me know.

ashish-naik · 2020-03-01T07:44:13Z

I consistently get nil sessionToken from my server deployed on Heroku because of this i get error 206.

Could that be due to this issue? it works consistently on my local server.

I have tried searching answer for nil token everywhere but no luck.

If this fix will solve the problem then would be great.

markuswinkler · 2020-03-03T21:56:11Z

This fixes the problem for me as well. Please merge. :)

davimacedo · 2020-03-04T03:13:37Z

@andrewking0207 have you had the chance to look at @dplewis comments?

andrewking0207 · 2020-03-04T21:27:33Z

Yes, sorry for the delay. I'll get those changes implemented.

latest

latest from upstream

… test for coverage

dplewis

Thanks for the improvements! I added a few comments and the tests look good also.

spec/AuthenticationAdapters.spec.js

src/Adapters/Auth/apple.js

… validateAuthData

andrewking0207 · 2020-03-08T06:00:07Z

I think we should be all set here. I'm not sure why it's showing a code coverage change for RestWrite or MongoStorageAdapter, I didn't touch those. Let me know if you want me to add tests for those as part of this PR.

dplewis

Sorry for the constant reviews. Your is code beautiful. I’m just nitpicking.

dplewis · 2020-03-08T06:26:43Z

src/Adapters/Auth/apple.js

+    options.client_id,
+    options.cacheMaxEntries,
+    options.cacheMaxAge
+  );


A quick nit. Functions should have 2 parameters max. Just pass in options.

Also if I pass in { clientID: “something” } you default values would never be set. Use the or operator.

options.cacheMaxEntries || 5 ( if the first expression is false or undefined use the second) just a quick trick for setting defaults.

Good feedback thanks. I got that changed.

dplewis

LGTM! Thanks @andrewking0207 and everybody for testing this.

benpackard · 2020-03-11T20:32:57Z

Excited for this one, going to roll it out to all of my clients that use Sign In with Apple ASAP. Thanks for everyone's work on this!

chriscborg · 2020-03-12T07:02:15Z

Thank you @andrewking0207 and @dplewis for your work on this. This is going to be extremely helpful to a majority of our apps!

oallouch · 2020-03-12T08:48:46Z

Here too. Thx a lot.

mmimeault · 2020-03-19T19:38:27Z

Thank you for the fix! Works way better now.
But thank you for the initial version @dplewis ;)

benpackard · 2020-03-19T21:04:01Z

Any thoughts on when we might see a new release with this change? I have a couple of projects I’m eager to update but I know it’s a busy time for all.

* use token and algo from jwt header * change node-rsa out for jwks-rsa, reflect change in tests and add one test for coverage * remove superfluous cache, allow jwks cache parameters to be passed to validateAuthData * remove package lock * regenerate package lock * try fixing package-lock with copy from master * manual changes for merge conflict * whitespace * pass options as object * fix inconsistent variable name

use token and algo from jwt header

accb75e

andrewking0207 requested a review from dplewis February 17, 2020 22:10

dplewis suggested changes Feb 17, 2020

View reviewed changes

src/Adapters/Auth/apple.js Show resolved Hide resolved

src/Adapters/Auth/apple.js Show resolved Hide resolved

andrewking0207 added 4 commits March 4, 2020 16:30

Merge pull request #1 from parse-community/master

90cf87b

latest

Merge pull request #2 from andrewking0207/master

3a89c7a

latest from upstream

change node-rsa out for jwks-rsa, reflect change in tests and add one…

9a3e617

… test for coverage

Change node-rsa out for jwks-rsa, tests and test coverage

4e9e205

andrewking0207 requested a review from dplewis March 5, 2020 17:23

dplewis suggested changes Mar 6, 2020

View reviewed changes

spec/AuthenticationAdapters.spec.js Outdated Show resolved Hide resolved

src/Adapters/Auth/apple.js Outdated Show resolved Hide resolved

src/Adapters/Auth/apple.js Outdated Show resolved Hide resolved

src/Adapters/Auth/apple.js Outdated Show resolved Hide resolved

andrewking0207 added 3 commits March 6, 2020 01:14

remove superfluous cache, allow jwks cache parameters to be passed to…

ae16ab3

… validateAuthData

remove package lock

1027556

regenerate package lock

316b9d7

dplewis mentioned this pull request Mar 6, 2020

updated apple auth to have multiple client ids, & also more checks added #6394

Closed

andrewking0207 added 3 commits March 7, 2020 23:12

try fixing package-lock with copy from master

4643ef8

manual changes for merge conflict

de7e47c

whitespace

806a50b

andrewking0207 requested a review from dplewis March 8, 2020 06:00

dplewis suggested changes Mar 8, 2020

View reviewed changes

andrewking0207 added 2 commits March 8, 2020 14:10

pass options as object

8239070

fix inconsistent variable name

b3d353a

dplewis mentioned this pull request Mar 11, 2020

Apple Sign-In: Invalid Signature #6496

Closed

dplewis approved these changes Mar 11, 2020

View reviewed changes

dplewis merged commit 8e0e485 into parse-community:master Mar 11, 2020

ashish-naik mentioned this pull request Mar 15, 2020

Error 206 cannot modify user for new Facebook user #6511

Open

TomWFox mentioned this pull request Apr 7, 2020

Apple signin for allowing multiple client ids #6523

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use token and algo from jwt header #6416

use token and algo from jwt header #6416

andrewking0207 commented Feb 17, 2020 •

edited

Loading

andrewking0207 commented Feb 17, 2020

codecov bot commented Feb 17, 2020 •

edited

Loading

dplewis left a comment •

edited

Loading

chriscborg commented Feb 21, 2020

dplewis commented Feb 21, 2020

chriscborg commented Feb 21, 2020

dplewis commented Feb 22, 2020

chriscborg commented Feb 23, 2020

ashish-naik commented Mar 1, 2020

markuswinkler commented Mar 3, 2020

davimacedo commented Mar 4, 2020

andrewking0207 commented Mar 4, 2020

dplewis left a comment

andrewking0207 commented Mar 8, 2020

dplewis left a comment

dplewis Mar 8, 2020

andrewking0207 Mar 8, 2020

dplewis left a comment

benpackard commented Mar 11, 2020

chriscborg commented Mar 12, 2020

oallouch commented Mar 12, 2020

mmimeault commented Mar 19, 2020 •

edited

Loading

benpackard commented Mar 19, 2020

use token and algo from jwt header #6416

use token and algo from jwt header #6416

Conversation

andrewking0207 commented Feb 17, 2020 • edited Loading

andrewking0207 commented Feb 17, 2020

codecov bot commented Feb 17, 2020 • edited Loading

Codecov Report

dplewis left a comment • edited Loading

Choose a reason for hiding this comment

chriscborg commented Feb 21, 2020

dplewis commented Feb 21, 2020

chriscborg commented Feb 21, 2020

dplewis commented Feb 22, 2020

chriscborg commented Feb 23, 2020

ashish-naik commented Mar 1, 2020

markuswinkler commented Mar 3, 2020

davimacedo commented Mar 4, 2020

andrewking0207 commented Mar 4, 2020

dplewis left a comment

Choose a reason for hiding this comment

andrewking0207 commented Mar 8, 2020

dplewis left a comment

Choose a reason for hiding this comment

dplewis Mar 8, 2020

Choose a reason for hiding this comment

andrewking0207 Mar 8, 2020

Choose a reason for hiding this comment

dplewis left a comment

Choose a reason for hiding this comment

benpackard commented Mar 11, 2020

chriscborg commented Mar 12, 2020

oallouch commented Mar 12, 2020

mmimeault commented Mar 19, 2020 • edited Loading

benpackard commented Mar 19, 2020

andrewking0207 commented Feb 17, 2020 •

edited

Loading

codecov bot commented Feb 17, 2020 •

edited

Loading

dplewis left a comment •

edited

Loading

mmimeault commented Mar 19, 2020 •

edited

Loading