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

VERIFIED FIXED in 2.2 S2 (19dec)

Status

defect
--
major
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jmcf, Assigned: fabrice)

Tracking

unspecified
2.2 S2 (19dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v1.4 wontfix, b2g-v2.0 verified, b2g-v2.0M fixed, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [p=3], [fb:escalated][mike-e])

Attachments

(3 attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
Johan,

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

thanks
Flags: needinfo?(jlorenzo)
Reporter

Comment 2

5 years ago
Fabrice,

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

thanks!
Flags: needinfo?(fabrice)
Reporter

Updated

5 years ago
blocking-b2g: --- → 2.2?
Reporter

Comment 3

5 years ago
qawanted for branchcheck
Keywords: qawanted
Assignee

Comment 5

5 years ago
(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.
Flags: needinfo?(fabrice)
Reporter

Updated

5 years ago
Target Milestone: --- → 2.1 S9 (21Nov)
Assignee: nobody → ferjmoreno
Whiteboard: [p=3]
[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
Flags: needinfo?(jlorenzo)
I also checked with a 1.3 version currently sold by ZTE in France => Repro'd
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 6 fulfilled the qawanted request. Removing keyword.
Flags: needinfo?(jmitchell)
Keywords: qawanted
Flags: needinfo?(jmitchell)
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?
Flags: needinfo?(whuang)
Posted image 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.
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.
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?
Flags: needinfo?(dersoz)
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.
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(whuang)
Hi Wesley,
Flags: needinfo?(dersoz)
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.
Flags: needinfo?(whuang)
Thanks Didem.
I believe we need support from Facebook for this. 
NI? to Michael.
Flags: needinfo?(mellis)
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.
Flags: needinfo?(fabrice)

Comment 19

5 years ago
Bug details provided to Fb's Mobilepartnerfeedback.  Will update bug when feedback is received from their team.
Flags: needinfo?(mellis)

Updated

5 years ago
Whiteboard: [p=3] → [p=3] [fb:escalated]

Updated

5 years ago
Whiteboard: [p=3] [fb:escalated] → [p=3], [fb:escalated]
Assignee

Comment 20

5 years ago
(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.
Flags: needinfo?(fabrice)
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".
Flags: needinfo?(fabrice)
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."
Target Milestone: 2.1 S9 (21Nov) → 2.2 S2 (19dec)
Assignee

Comment 23

5 years ago
(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.
Assignee: ferjmoreno → fabrice
Flags: needinfo?(fabrice)
Assignee

Comment 24

5 years ago
Fernando, can you try this patch?
Attachment #8530863 - Flags: feedback?(ferjmoreno)
Flags: needinfo?(whuang)

Comment 25

5 years ago
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.
Status: NEW → ASSIGNED
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.
Attachment #8530863 - Flags: feedback?(ferjmoreno) → feedback+

Comment 27

5 years ago
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?
Flags: needinfo?(jmcf)
Whiteboard: [p=3], [fb:escalated] → [p=3], [fb:escalated][mike-e]
Assignee

Updated

5 years ago
Attachment #8530863 - Flags: review?(bzbarsky)
Assignee

Comment 28

5 years ago
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 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.
Attachment #8530863 - Flags: review?(bzbarsky) → review+
Reporter

Comment 31

5 years ago
Once this lands in Moz Central I will test it again. Keeping the ni for tracking purposes
Flags: needinfo?(jmcf)
Reporter

Updated

5 years ago
Flags: needinfo?(jmcf)
This is on m-c already http://hg.mozilla.org/mozilla-central/diff/48f4e72f94cb/docshell/base/nsDocShell.cpp
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Reporter

Comment 33

5 years ago
tested and working. I think we would need an uplift to v2.1 and ideally to v2.0 as well.
Flags: needinfo?(jmcf)
Reporter

Updated

5 years ago
Flags: needinfo?(oteo)
Flags: needinfo?(noemi.freiredecarlos)
Flags: needinfo?(fabrice)
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
Flags: needinfo?(oteo)
[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!
blocking-b2g: 2.2+ → 2.0?
Flags: needinfo?(noemi.freiredecarlos)
Assignee

Comment 36

5 years ago
You need to ask for approval on the patch, not blocking status on the branch.
Flags: needinfo?(fabrice)
ok, restoring blocking-b2g flag, we will ask for approval directly on the patch
blocking-b2g: 2.0? → 2.2+
Reporter

Comment 38

5 years ago
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
Attachment #8530863 - Flags: approval-mozilla-b2g32?
Reporter

Comment 39

5 years ago
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
Attachment #8530863 - Flags: approval-mozilla-b2g34?
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
Attachment #8530863 - Flags: approval-mozilla-b2g34?
Attachment #8530863 - Flags: approval-mozilla-b2g34+
Attachment #8530863 - Flags: approval-mozilla-b2g32?
Attachment #8530863 - Flags: approval-mozilla-b2g32+
Keywords: verifyme
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.
Assignee

Updated

4 years ago
Flags: needinfo?(fabrice)
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
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(jmercado)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.