parse-community / ParseFacebookUtils-Android

A utility library to authenticate ParseUsers with the Facebook SDK
http://docs.parseplatform.org/android/guide/#facebook-users
Other
53 stars 63 forks source link

prevent asymmetric facebook session management with flag #28

Open mitchymitch opened 7 years ago

mitchymitch commented 7 years ago

I was running into the following issue:

I was acquiring my facebook accessToken and then linking with ParseFacebookUtils.linkInBackground(...). Then, if I had already linked with that Facebook id, I was getting a 208 code. In that case, I then have to use ParseFacebookUtils.logInInBackground(...) to retrieve that old linked ParseUser. The problem is, logInInBackground will log me out of Facebook, even though I'm supplying my own accessToken.

Logic tells me, that if I'm supplying my own accessToken, its erroneous for it to assume its managing the lifecycle of my accessToken.

My Solution: I added a flag to track the cases (only 2) where the accessToken is acquired/expired externally to stop it from thinking it has to log you out.

Another Solution: Do we ever need to explicitly logout of Facebook? This could allow us to remove this flag.

flovilmart commented 7 years ago

@rogerhu what are your thoughts? @mitchymitch is a colleague of mine.

codecov[bot] commented 7 years ago

Codecov Report

Merging #28 into master will increase coverage by 0.38%. The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #28      +/-   ##
============================================
+ Coverage      57.3%   57.69%   +0.38%     
- Complexity       35       36       +1     
============================================
  Files             2        2              
  Lines           178      182       +4     
  Branches         18       19       +1     
============================================
+ Hits            102      105       +3     
- Misses           69       70       +1     
  Partials          7        7
Impacted Files Coverage Δ Complexity Δ
...ry/src/main/java/com/parse/FacebookController.java 78.87% <100%> (+0.3%) 15 <0> (+1) :arrow_up:
...ry/src/main/java/com/parse/ParseFacebookUtils.java 44.14% <75%> (+0.62%) 21 <1> (ø) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1b52507...460faed. Read the comment docs.

rogerhu commented 7 years ago

@flovilmart I'm inclined to say that setAuthData() shouldn't have the side effect of logging out... may add a deprecated warning if someone passes a null to indicate that this behavior has changed.

flovilmart commented 7 years ago

This will yield a lot a warnings. My main concern is that the current behavior is the same as on iOS, so I’m unsure about the Fox and the potential side effects on existing installations. Otherwise, that’s your call @Rogerhu.

rogerhu commented 7 years ago

Does the iOS logout too if authData is null?

rogerhu commented 7 years ago

you'll need to rebase anyways given the set of changes... my question is still how iOS handles authData null..

jyoon17 commented 7 years ago

@rogerhu Yes, it does. See this link.

I think the name setAuthData confuses us. Logging out is not a side effect of setAuthData. Since it is called when ParseUser.logOut is called, it is quite an intended behavior.

jyoon17 commented 7 years ago

@mitchymitch How did you get the user you provided to ParseFacebookUtils.linkInBackground? I am wondering why you need to call ParseFacebookUtils.logInInBackground to get the user you provided to linkInBackground. It's not important but I am just curious.

mitchymitch commented 7 years ago

@jyoon17 with ParseUser.enableAutomaticUser();

mitchymitch commented 7 years ago

Since it is called when ParseUser.logOut is called, it is quite an intended behavior.

@jyoon17 you're right - in a way. I don't think its intended behaviour though to log out of Facebook if I call linkInBackground(user) & then logInInBackground(token), since using logInInBackground(fbtoken) assumes YOU are the one responsible for the FB lifecycle. I'm curious how else people are using logInInBackground, since it seems pretty useless unless I'm not reading the code properly.