-
Notifications
You must be signed in to change notification settings - Fork 31
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
Removed Provider For Apple Login In getOAuthSignInURL method #67
base: main
Are you sure you want to change the base?
Removed Provider For Apple Login In getOAuthSignInURL method #67
Conversation
Removed the default value for provider in the init method for OpenIDConnectCredentials
@@ -298,7 +298,6 @@ public struct UserIdentity: Codable, Hashable, Identifiable, Sendable { | |||
} | |||
|
|||
public enum Provider: String, Codable, CaseIterable, Sendable { | |||
case apple |
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.
Not sure I follow why the removal of apple
as a provider.
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.
Because we don't need it in the getOAuthSignInURL
method. So the users can know in the documentation on this branch (for which i will create a PR) that the only method for Apple Sign In is with the signInWithIdToken
method.
We don't want to offer Apple as a provider for getOAuthSignInURL
if it's not usable
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.
@ljubiknenad The way I think about this is that we should provide a way for the user to sign in using the getOAuthSignInURL
even if it makes more sense to use native sign in.
Also removing the apple
as a provider will throw an error when decoding a user that was signed in using apple, as apple isn't available on the enum.
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.
@GRSouza if we provide a way for the user to sign in using getOAuthSignInURL
, won't the developer of the app be rejected from Apple? Because in Apple's documents it says:
To support Sign in with Apple for iOS, macOS, tvOS, and watchOS apps, see Implementing User Authentication with Sign in with Apple. For website support, see Sign in with Apple JS, and use the Sign in with Apple REST API to communicate with Apple servers.
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 think to provide a feature-complete library and just warn the user that he shouldn't use the getOAuthSignInURL
flow to submit to App Store. It's not on us to decide how the user will implement that, we just provide the features and make recommendations for implementation.
What kind of change does this PR introduce?
Removed apple as a Provider from
getOAuthSignInURL
method as well as the experimental label onsignInWithIdToken
What is the current behavior?
signInWithIdToken
was labeled as experimentalWhat is the new behavior?
The experimental label on
signInWithIdToken
is removed. Also.apple
was removed as a Provider forgetOAuthSignInURL
method since we will not provide Apple Sign In with that method.Additional context