-
-
Notifications
You must be signed in to change notification settings - Fork 963
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
feat: support native social sign using apple sdk #3476
Conversation
6ec19d3
to
6b580e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #3476 +/- ##
=======================================
Coverage 78.76% 78.76%
=======================================
Files 341 341
Lines 23707 23707
=======================================
Hits 18673 18673
Misses 3667 3667
Partials 1367 1367 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, we can really e2e test this, as there is no other provider that supports verifying an ID Token from the payload, for now.
I guess, we could make the generic provider support it and add a JWKS URL to the config? Then we could obtain an ID token from a Hydra instance and submit that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, please add three tests to strategy_test:
- Test login with an ID token, ensure claims end up properly
- Test registration with an ID token, ensure claims end up properly
- Test login/registration with an ID token where the Client ID is not matching (should fail)
- Test login/registration with an ID token where the nonce is not matching (should fail)
Please also make it possible to submit the nonce and validate the nonce as well. Fail the flow if the nonce is not set.
ec1ee7d
to
3f3b428
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nonce implementation does not look correct to me. Typically, a nonce is a plain string (like the state parameter) and you do a string equality match to verify if it is valid or not. The nonce_supported
flag defaults to false, which may be OK for apple, but is definitely not OK for any other provider, as this claim is not a standard claim (it is never mentioned here: https://openid.net/specs/openid-connect-core-1_0.html).
Additionally, the Apple documentation states:
A Boolean value that indicates whether the transaction is on a nonce-supported platform. If you send a nonce in the authorization request, but don’t see the nonce claim in the identity token, check this claim to determine how to proceed. If this claim returns true, treat nonce as mandatory and fail the transaction; otherwise, you can proceed treating the nonce as optional.
However, as far as I can see, we use this claim first to check whether we should validate. But the documentation states:
- If a nonce was sent (aka if we receive the nonce as part of the registration / login payload
- And the nonce is not available in the id_token
- Then (and only then) we check this claim
The current implementation however:
- Checks if the claim is true
- Proceeds with validation
My suggestion is to move this custom validation logic into the apple ID token verification:
- The apple provider validates the
nonce
claim: - If a nonce was sent as part of the payload
- And the id_token has a nonce -> we validate
- And the id_token has no nonce
* We check the nonce_supported claim
* If true: fail
* If false: proceed - The Google provider also validates the nonce (but always, because Google returns this always)
- The generic provider does not validate the nonce at all, it delegates the ID token validation to the specific provider (we can fix this later if desired).
Please don't merge this without another review from myself!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! Almost there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
9b97cc0
to
b8de37b
Compare
This PR adds support for social sign in via Apple using an ID token instead of the browser based OIDC flows. This allows integration of the SDK for Apple sign in iOS.
How this works:
Expo example (PR in repo following):
Related issue(s)
Docs PR: ory/docs#1528
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments