-
-
Notifications
You must be signed in to change notification settings - Fork 70
Anonymous and Apple Login #53
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 76.22% 76.28% +0.06%
==========================================
Files 42 46 +4
Lines 3886 4170 +284
==========================================
+ Hits 2962 3181 +219
- Misses 924 989 +65
Continue to review full report at Codecov.
|
@TomWFox can you look over the docs when you get a chance? I'm pretty much done with them and I don't think I need to a add anymore methods to the login protocols |
@TomWFox we should probably add an improved version of my second bullet point above to the contributors guide (and maybe a snippet to the readme?):
I can see a few questions/issues occurring frequently:
I think the method above we have setup for adding support for basically any authentication the server supports leads to easier maintainability. Plus, this allows users the flexibility of using any version of their authentication framework they want, they simply just need to provide the minimum keys. |
…g sync calls for auths as they shouldn't be used anyways. Devs can use semaphores/etc if they want sync
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.
Mostly minor changes with one potential issue.
Getting closer to 1.0!
var user: SessionUser { get } | ||
|
||
/// Whether the session is restricted. | ||
var restricted: Bool { get } |
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 this could be an issue, see parse-community/parse-server#6612
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 see, I think making it "optional" should make it backwards/forwards compatible with Parse Server. What do you think?
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 can also remove it, developers just won't be able to see it in their Swift apps because it's never decoded
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 only potential problem I see with removing is it will show in their Parse Dashboard, but not on their swift apps...
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.
Making it optional seems like an acceptable option. I wonder whether the comment should be changed to say that the feature isn't implemented server side and/or link to the #6612
Sources/ParseSwift/Authentication/Protocols/ParseAuthenticatable.swift
Outdated
Show resolved
Hide resolved
Sources/ParseSwift/Authentication/Protocols/ParseAuthenticatable.swift
Outdated
Show resolved
Hide resolved
Sources/ParseSwift/Authentication/Protocols/ParseAuthenticatable.swift
Outdated
Show resolved
Hide resolved
Sources/ParseSwift/Authentication/Protocols/ParseAuthenticatable.swift
Outdated
Show resolved
Hide resolved
Sources/ParseSwift/Authentication/Protocols/ParseAuthenticatable.swift
Outdated
Show resolved
Hide resolved
Sources/ParseSwift/Authentication/Internal/ParseAnonymous.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Tom Fox <13188249+TomWFox@users.noreply.github.com>
Guessing at the issue with jazzy docs. I believe GitHub Actions may have recently bumped the default Xcode to 12.3 and I "think" jazzy and it's dependencies aren't playing nice. It looks like the docs are building fine with 12.2, so docs can stay on that version for now, while the rest of the builds continue on the latest Xcode. |
I think adding to the contributors guide would be a good idea, not sure about the readme. |
This PR adds 3rd party login ability via the
ParseAuthenticatable
protocol.ParseAnonymous
andParseApple
loginParseAuthenticatable
protocol for implementation by developers. Example: Facebook, Twitter, etc. should conform to protocol and PR's can be submitted to add them. The dependency of the the respective framework, i.e.import FBSDKCoreKit
shouldn't be part of the PR (this should be done by the developer in their respective apps). Instead the implementation should just ensure the necessaryAuthenticationKeys
are captured to properly authenticate the respective framework with a Parse Server. An example of this is ParseApple.swift. As a starting point for those who want to add more 3rd party methods, I recommend copyingParseApple.swift
and modifyingAuthenticationKeys
and helper methods. All of the Parse server-side 3rd party supported authentication methods are here along with their respective documentation.ParseSession
protocolUsage (same can be done for "apple", just need to pass in apple credentials to login):