1094966 - [contacts] [facebook] First time a user logs into Facebook, the local redirection does not happen

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Jose Manuel Cantera]

STR:

Choose a Facebook account which has not previously authorized the Firefox OS Facebook Application. 

Go to contacts --> Settings --> Sync Facebook --> Log into Facebook --> Accept the permissions dialog --> The app shows a screen like the one attached. 

Actually the referred screen is the corresponding to the content served at https://www.facebook.com/connect/login_success.html

That means that the local redirection, the one configured in the manifest file 

"redirects":[{"from":"https://www.facebook.com/connect/login_success.html","to":"/shared/pages/import/redirects/redirect.html"}

is not happening.

After closing the window and clicking one more time on the sync FB switch the user can get access to her friend list. 

Please note that, this is not happening when the user has previously authorized the FB App. (That can be easily tested by logging out from FB, (switch off FB Sync in settings) In that case everything works as expected and the local redirection happens, going to the friend list.

Comment 1

[Jose Manuel Cantera]

Johan,

Please could you check if this reproduces on 2.0 and 2.1 ? 

thanks

Comment 2

[Jose Manuel Cantera]

Fabrice,

any idea of what could be happening with the local redirection ?

thanks!

Comment 3

[Jose Manuel Cantera]

Attachment: 2014-11-06-18-26-30.png

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

qawanted for branchcheck

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Jose Manuel Cantera from comment #2)
> Fabrice,
> 
> any idea of what could be happening with the local redirection ?
> 
> thanks!

So we load https://m.facebook.com/v1.0/dialog/oauth?redirect_uri=https%3A%2F%2Fwww.facebook.com%2Fconnect%2Flogin_success.html&state=friends&scope=friends_about_me and that redirects to https://www.facebook.com/connect/login_success.html but we don't detect the redirect. I'm not sure why yet.

Comment 6

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: 

Repro'd on 2.1, if I have an account which has not accepted the FFOS FB app, I arrive on the success page but I'm not redirected afterwards.
Gaia-Rev        0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/97487a2d1ee6
Build-ID        20141110001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.jlorenzo.20141002.104456
FW-Date         Thu Oct  2 10:45:09 CEST 2014
Bootloader      L1TC00011880

Same thing on 2.0
Gaia-Rev        d3e4da377ee448f9c25f908159480e867dfb13f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/7198906837e7
Build-ID        20141110000204
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.jlorenzo.20141002.104456
FW-Date         Thu Oct  2 10:45:09 CEST 2014
Bootloader      L1TC00011880

My buri on 1.4 is affected too:
Gaia-Rev        b06fee13c1d80bd2a463be1f2fb1d59169af9298
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/540ef440c9a9
Build-ID        20141013000203
Version         30.0
Device-Name     msm7627a
FW-Release      4.0.4
FW-Incremental  eng.tclxa.20131223.163538
FW-Date         Mon Dec 23 16:36:04 CST 2013

Comment 7

[Johan Lorenzo [:jlorenzo]]

I also checked with a 1.3 version currently sold by ZTE in France => Repro'd

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I've been looking into this today and for some reason nsDocShell::DoAppRedirectIfNeeded [1] is not being called as it should be. I'll keep digging.

[1] https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6501

Comment 9

[Pi Wei Cheng [:piwei] (inactive)]

Comment 6 fulfilled the qawanted request. Removing keyword.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Let's check if the partner can do something, Wesley do you know who can we contact on our partners team to try to talk with FB about what are they doing in that redirect?

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Requests

I believe the reason for this issue is that Facebook is not behaving in the same way depending if the user has previously allowed access to the app or not.
Attached you can see two different requests timelines. The one on the top is the timeline for the case where the user has not previously given access to the contacts app while the one on the bottom is for the other case. As you can see we are getting a 200 for https://www.facebook.com/connect/login_success.html while we should be getting a 302. That's the reason why we are not able to catch the redirect.

At this point I am not sure what else we can do on our side to fix this issue. You may argue that we could check if there are app redirections for every 200 responses too, but that would affect performance for every URI load so I don't think that's really an option. Any other idea would be really appreciated.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

I just did one more test by removing the app redirect from the app manifest and it seems that my previous assumption about getting a 302 was wrong. We are getting 200 in both cases. The only difference I could see so far is that for the case where we have no previous authorization we are getting a "referer" response header.

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Didem,
The problem is not on Facebook app but for the contact app with feature connecting to Facebook.
Do you happen to know who can guide us here?

Comment 14

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: from user experience perspective, it does impact the new users a lot.
we need engineering to figure out how we could get this fixed.

Comment 15

[Didem Ersoz [:didem]]

Hi Wesley,

Comment 16

[Didem Ersoz [:didem]]

Hi Wesley,

It looks like we need someone from Mozilla to help fix this issue, is that right? In that case, Francisco Jordano would know better who can help, and I see that he is already cc'ed here. 

If you still need someone from Facebook, let us know. I cc'ed Michael Ellis, who communicates bugs to Facebook. He can help report this to them, if we still think there is an issue on their side. 

I also cc'ed Harald to this bug who is the Partner Engineering point of contact for Facebook.

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Thanks Didem.
I believe we need support from Facebook for this. 
NI? to Michael.

Comment 18

[:Harald Kirschner :digitarald]

Fabrice, do we have a bug open to make "redirects" behave consistently for the various types of redirects we see on the web, including JS based?

Michael, the specific ask for FB is to re-enable 302 status on redirects on the OAuth redirect_uri.

Background is that we have a feature hard-coded to allow oauth flows to return to the app by hooking into 302 redirects and serving the URL that is being redirected to from within the app to extract the oauth tokens.

Comment 19

[Michael Ellis]

Bug details provided to Fb's Mobilepartnerfeedback.  Will update bug when feedback is received from their team.

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Harald Kirschner :digitarald from comment #18)
> Fabrice, do we have a bug open to make "redirects" behave consistently for
> the various types of redirects we see on the web, including JS based?
> 
> Michael, the specific ask for FB is to re-enable 302 status on redirects on
> the OAuth redirect_uri.
> 
> Background is that we have a feature hard-coded to allow oauth flows to
> return to the app by hooking into 302 redirects and serving the URL that is
> being redirected to from within the app to extract the oauth tokens.

By JS based redirect do you mean things like changing document.location? If so, I'm pretty sure we don't have support for that.

Comment 21

[:Harald Kirschner :digitarald]

I am saying that we should reconsider that otherwise stuff keeps breaking. We'll have a really hard time to argue with partners that we integrated with because they are not doing redirects that we deem "correct".

Comment 22

[:Harald Kirschner :digitarald]

To quote the oauth 2 spec:

"While the examples in this specification show the use of the HTTP 302 status code, any other method available via the user-agent to accomplish this redirection is allowed and is considered to be an implementation detail."

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Harald Kirschner :digitarald from comment #21)
> I am saying that we should reconsider that otherwise stuff keeps breaking.
> We'll have a really hard time to argue with partners that we integrated with
> because they are not doing redirects that we deem "correct".

Sure. That would still help to check if this is actually the way they redirect. Assigning to myself for now, but I won't look at it this week.

Comment 24

[[:fabrice] Fabrice Desré]

Attachment: redirect-nav.patch

Fernando, can you try this patch?

Comment 25

[Michael Ellis]

No feedback from Fb yet on this.  If we do not receive any feedback before the 16th, we have a meeting w/ them where we can hopefully reignite the conversation on these bugs.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8530863 [details] [diff] [review]
redirect-nav.patch

Review of attachment 8530863 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Fabrice! This fixes the issue for me.

Comment 27

[Michael Ellis]

Works for Fabrice, Works for me.

@ Jose Manuel - could you please take another look at this issue and let us know if it is fixed on your end?

Comment 28

[[:fabrice] Fabrice Desré]

Boris, until now we were only checking for apps redirect on http transport redirects, but some providers changed to redirect using document.location instead.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8530863 [details] [diff] [review]
redirect-nav.patch

Can you not remove the DoAppRedirectIfNeeded() call from the end of ForceRefreshURI, since that just calls into the code where you're adding this call?

r=me with that.

Comment 30

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/48f4e72f94cb

Comment 31

[Jose Manuel Cantera]

Once this lands in Moz Central I will test it again. Keeping the ni for tracking purposes

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

This is on m-c already http://hg.mozilla.org/mozilla-central/diff/48f4e72f94cb/docshell/base/nsDocShell.cpp

Comment 33

[Jose Manuel Cantera]

tested and working. I think we would need an uplift to v2.1 and ideally to v2.0 as well.

Comment 34

[Maria Angeles Oteo (:oteo)]

Not sure if we are quite late for 2.0 but at least we should try to uplift it and Fabrice ask for the approval in those branches

Comment 35

[Noemí Freire (:noemi)]

[Blocking Requested - why for this release]:asking for 2.0 approval since it impacts on the experience when a user logs in for the first time, Thanks!

Comment 36

[[:fabrice] Fabrice Desré]

You need to ask for approval on the patch, not blocking status on the branch.

Comment 37

[Noemí Freire (:noemi)]

ok, restoring blocking-b2g flag, we will ask for approval directly on the patch

Comment 38

[Jose Manuel Cantera]

Comment on attachment 8530863 [details] [diff] [review]
redirect-nav.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Local OAuth redirections 
User impact if declined: Very high. User can lose context and skip Facebook importation
Testing completed: Yes, but the reporter and by the owners of the patch
Risk to taking this patch (and alternatives if risky): It is a one liner patch
String or UUID changes made by this patch: None

Comment 39

[Jose Manuel Cantera]

Comment on attachment 8530863 [details] [diff] [review]
redirect-nav.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Local OAuth redirections 
User impact if declined: Very high. User can lose context and skip Facebook importation
Testing completed: Yes, by the reporter and by the owners of the patch
Risk to taking this patch (and alternatives if risky): It is a one liner patch. The alternative is to have a broken app. 
String or UUID changes made by this patch: None

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 8530863 [details] [diff] [review]
redirect-nav.patch

Spoke to :fabrice about this and this safe one, Just requesting qa verification once this lands on 2.0/2.1

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/67911f617573

Bug 1044490 only landed on Gecko 34, so b2g32 is going to need a branch patch for uplift.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2c2166d04399

Comment 43

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/2c2166d04399

Comment 44

[enrico.bertelletti]

Hello, I'm developing Facebook Login for a privileged app on FirefoxOS and I'm also experimenting this issue.

I see this fixed in the latest 2.0 preprelease builds for my Geeksphone Keon, but I see no fix will applied to older FXOS versions. Do you know for a workaround that could be applied for apps running on FXOS <2.0? 

As side notes for the FXOS Contact app Facebook integration: 
I read here some references to Facebook Open Graph API v1.0 and FQL.
- Facebook Open Graph API v1.0 support will be closed by Facebook on April 30, 2015 
https://developers.facebook.com/docs/graph-api
- Facebook Open Graph API v2.0 is the last version where FQL will be available. By August 7, 2016, FQL support will be closed 
https://developers.facebook.com/docs/reference/fql/
- Facebook Open Graph API v2.0 has removed the option to retrieve full friends list for users to Facebook Apps that are not games.
https://developers.facebook.com/docs/apps/faq#unable_full_friend_list
https://developers.facebook.com/bugs/1502515636638396

Comment 45

[Martin Shuman [:Marty]]

This issue is verified fixed on the latest Flame 2.2, 2.1, and 2.0 builds.
Using a Facebook account which has not yet authorized FirefoxOS, the user is able to successfully log in, and is properly redirected to a Facebook authorization permission prompt, and then a contact select screen.

Environmental Variables:
Device: Flame 2.2
BuildID: 20150908032505
Gaia: 335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gecko: 96170c571381
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20150724001207
Gaia: 9dba58d18006e921546cec62c76074ce81e16518
Gecko: 41e10c6740be
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 34.0 (2.1)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 2.0
BuildID: 20150723000207
Gaia: b16ba05481e577bc644ed8966f587a70fe2148e6
Gecko: 2e6f1d4deff9
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 32.0 (2.0)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0