-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Task/p8 #6648
base: alpha
Are you sure you want to change the base?
Task/p8 #6648
Conversation
… we are doing all the checks in one place
…function which is better practise and we should be using it, however we can only test with real tokens and not use the spy function call as it skips it altogether
@TomWFox I have set placeholders for where to put config file (that we fill in from developer account panel) && also where to put the p8 file for future reference when we get an apple account. I used my own credentials to test :). |
@UnderratedDev awesome, thanks! I haven't got around to this yet as I realised I also needed to transfer my account into my name etc. I'm also wondering whether it's sensible to use to use my personal account. I could setup a Parse Community account, do you know if its possible to generate the credentials without paying the annual fee? |
You can create a free developer account (just Apple ID), This might be helpful: Memberships Also forgot to mention in description and wanted to give credit, I used this repo as a basis for requesting tokens from code: Apple Auth, I used it as a example 1 month and a bit ago (maybe 2 months). In the end, I rewrote my code entirely in my own way, but for reference, that repository contains other features which may be useful for other developers & also wanted to give credit :). |
Also, I was thinking about the way I did the config in the function, I will most likely change it so that it is passed into the options rather than as a file path. It's easier to check & then we don't have to check JSON parse errors. |
Uses config object instead of file now. No more invalid json for parse testing either. Ready to be reviewed :). |
Codecov Report
@@ Coverage Diff @@
## master #6648 +/- ##
==========================================
- Coverage 93.90% 93.61% -0.30%
==========================================
Files 169 169
Lines 12054 12630 +576
==========================================
+ Hits 11319 11823 +504
- Misses 735 807 +72
Continue to review full report at Codecov.
|
@dplewis when you have a chance, could you please take a look at this PR? |
Can you provide test cases and improve coverage? |
I did provide test cases in the Authentication spec file (p8 file, apple error, JWT errors, etc). I can take a look at increasing coverage though it shows coverage changed for files I didn't modify. I just modified the apple.js file so I will try to increase that file's coverage. |
Hey @dplewis, so I got a bit busy but recently made time to look at this again. Further test cases are hard to make since I would need to provide a p8 key, client id, etc. Also there are no test tokens provided from apple. That being said, I will stop working on this as I don't see a way to add more in the current state. I hope this gets merged, please let me know if there is anything specific I can do to help get this merged. |
|
Added functionality to generate tokens from requesting them from Apple using a code. Uses p8 file to sign token (claims) to get real token from apple for authentication. Also uses config file for web request tokens (contains info such as redirect uri). Audience & Issuer are verified in verify function with the rest of the verification which is nicer however almost impossible to test.
@TomWFox , hope this satisfies :)
#6523