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

Added support for session token and user authentication. #60

Merged
merged 3 commits into from Oct 26, 2016

Conversation

Projects
None yet
4 participants
@agordeev
Copy link
Contributor

agordeev commented Aug 21, 2016

Added using of sessionToken for both Connect and Subscribe messages. sessionToken is taken from PFUser.currentUser() directly and passes only if it's not nil.

Tested, works fine for me.

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 21, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added the GH Review: review-needed label Aug 21, 2016

@agordeev

This comment has been minimized.

Copy link
Contributor Author

agordeev commented Aug 21, 2016

Inspired by this issue: #40

@ghost ghost added the CLA Signed label Aug 21, 2016

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 21, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@richardjrossiii

This comment has been minimized.

Copy link
Contributor

richardjrossiii commented Sep 8, 2016

Sorry it's taken so long to get to this.

IIRC, the original reason I left out sending sessionToken, was that I couldn't come up with a good solution for logout occurring, or the current user changing in-between subscriptions.

One initial plan I had, was to have a single PFUser instance associated with a live query client, that could be passed on initialization (and the default client would use currentUser).

The issue with this, again, is logout. There's currently no way in the SDK to know when a user is logged out, and I had hoped that I would be able to invalidate all of the subscriptions when this occurred.

With that said, this is a great first step!

I've added a few more comments inline, that I'd like addressed before this gets merged.

@@ -23,7 +23,7 @@ public class Client: NSObject {
let clientKey: String?

var socket: SRWebSocket?
var userDisconnected = false
public var userDisconnected = false

This comment has been minimized.

@richardjrossiii

richardjrossiii Sep 8, 2016

Contributor

Why does this need to be made public - this only controls whether or not 'disconnect()' was explicitly called, and the person using the Client should know this already, no?

This comment has been minimized.

@agordeev

agordeev Oct 15, 2016

Author Contributor

I use this property in my own project to automatically reconnect to the socket after the Internet connection is available again. Otherwise I don't know any way to know whether the client is connected or not.

@@ -228,7 +227,7 @@ extension Client {
switch response {
case .Connected:
self.subscriptions.forEach {
self.sendOperationAsync(.Subscribe(requestId: $0.requestId, query: $0.query))
self.sendOperationAsync(.Subscribe(requestId: $0.requestId, query: $0.query, sessionToken: PFUser.currentUser()?.sessionToken))

This comment has been minimized.

@richardjrossiii

richardjrossiii Sep 8, 2016

Contributor

Since this is a loop, there's the weird off chance that another thread changes the current user between iterations of the loop - let's cache sessionToken outside the loop, so all of our queries get subscribed to the same user (should also improve perf. slightly).

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 15, 2016

@andrew8712 are you still up to take care of those nits so we can merge it and make it part of 1.1.1?

@agordeev

This comment has been minimized.

Copy link
Contributor Author

agordeev commented Oct 15, 2016

@flovilmart Looks like my fork is pretty outdated. Would you be able to merge it or you want me to merge the fork from master first? My project uses this fork, and it's not migrated to Swift 3, so I didn't update the fork too.

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 15, 2016

You'll probably need to rebase and update to swift 3.

I'll let @richardjrossiii do the final review and merge as he had some concerns

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 26, 2016

@andrew8712 we'll likely need to update to swift3, I could also add the support myself, but I don't like to take this good work for myself! So your call :)

agordeev added some commits Aug 21, 2016

Added support for session token and user authentication.
Added using of sessionToken for both Connect and Subscribe messages. sessionToken is taken from PFUser.currentUser() directly and passes only if it's not nil.

@agordeev agordeev force-pushed the agordeev:master branch from 0e186e5 to bafc28e Oct 26, 2016

@agordeev

This comment has been minimized.

Copy link
Contributor Author

agordeev commented Oct 26, 2016

@flovilmart Done. unfortunately, I couldn't test the changes on my project, as it's still not migrated to Swft 3.

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 26, 2016

Awesome! I'll have a look later today!

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 26, 2016

That's looking great! We probably want to add a notification to the iOS SDK to notify login/logout events and listen to that to automatically refresh the subscriptions.

@flovilmart flovilmart merged commit 7b32e74 into parse-community:master Oct 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agordeev

This comment has been minimized.

Copy link
Contributor Author

agordeev commented Oct 26, 2016

@flovilmart Agree, that case should be definitely handled.

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 26, 2016

@andrew8712 wanna make a PR on the iOS SDK to notify user logins/logout events?

@agordeev

This comment has been minimized.

Copy link
Contributor Author

agordeev commented Oct 26, 2016

@flovilmart Parse iOS SDK is out of my scope at this moment :)

@flovilmart

This comment has been minimized.

Copy link
Member

flovilmart commented Oct 26, 2016

alright I'Ll see what I can do then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment