Cannot use twitter or google accounts within the desktop runtime - Needs to be whitelisted on origins allowed

VERIFIED FIXED

Status

Firefox Graveyard
Webapp Runtime
--
major
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: jsmith, Assigned: Mardak)

Tracking

unspecified
Bug Flags:
in-moztrap -

Details

(Whiteboard: [topapps], [marketplace-beta=] [testday-20120504]])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Google and twitter accounts cannot be used within the desktop runtime currently due to the implementation of bug 707836. Logging in with these accounts within an app that redirected to login will send you directly to the browser, making login by these accounts, currently not possible within an application. We need temporarily whitelist the origins to allow account access to twitter and google.

In the long run, we really need to support origin whitelisting for apps, as this is repeatedly becoming problematic (we're putting band-aids on the real problem).
(Reporter)

Comment 1

5 years ago
Example Top
Whiteboard: [topapps]
(Reporter)

Comment 2

5 years ago
Example top app: GIS Cloud

Example Steps:

1. Install and launch GIS Cloud
2. Click sign in
3. Select the twitter or google account logo
4. Login with your creds for twitter or google account respectively

Expected:

After a successful login, you should redirect back to GIS Cloud.

Actual:

The browser starts up. Google accounts prompt for a login again, twitter indicates an error.
(Reporter)

Comment 3

5 years ago
For the marketplace beta release, I think we need to define what are allowed_origins for this release carefully, then allow them in the code for now.
Whiteboard: [topapps] → [topapps], [marketplace-beta?]
Mardak: is this as simple as adding two more origins to the list of allowed origins?

Jason: do you know the specific origins to which those apps are directing users?
Whiteboard: [topapps], [marketplace-beta?] → [topapps], [marketplace-beta+]
To give more information about the opening comment of the bug, this will be a bandaid fix for the Marketplace Beta release. We will need to put more time into developing a solution to this problem. The blocking bug for K9o is bug 749415
(Assignee)

Comment 6

5 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> Mardak: is this as simple as adding two more origins to the list of allowed
> origins?
Should be. At least for GIS Cloud, they seem to redirect to..

https://api.twitter.com
https://www.google.com -> https://accounts.google.com

However, there's currently no distinction between whitelisting for popups vs main page navigation.
(Assignee)

Comment 7

5 years ago
Seems like both www and accounts are needed for Google, but there's no good way to go back from the Google login flow without restarting the app.

Twitter has a cancel button, but I wasn't able to fully test if just api.twitter.com was necessary.

These two flow differ from the Persona and Facebook logins where they use a separate window whereas these use the main window.
(Reporter)

Updated

5 years ago
Blocks: 737571
(Assignee)

Comment 8

5 years ago
Created attachment 620203 [details] [diff] [review]
v1
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #620203 - Flags: review?(myk)
(Assignee)

Comment 9

5 years ago
This fix doesn't quite fix GIS Cloud twitter login because some reason it redirects to http://www.giscloud.com/app instead of https://www.giscloud.com/app

So that ends up opening the non-https page in the default browser. But this is probably something GIS Cloud needs to fix on their side.
Comment on attachment 620203 [details] [diff] [review]
v1

Looks good, the obvious fix, and works, provided apps integrate with the identity providers correctly.  We obviously really need a better long-term solution sooner than later, but this, like the similar fix we landed last week, is sufficient in the short term.
Attachment #620203 - Flags: review?(myk) → review+
I had a conversation with glandium on IRC about getting approval for this patch to land, concluding that it has the "automatic approval to land" granted to files that are "not part of the build on Windows" <https://wiki.mozilla.org/Tree_Rules#mozilla-central_.28Nightly_channel.29> because it is a JS-only change.

So I landed it with a=js-only, and glandium is going to get edmorley to update the Tree Rules page to explicitly specify a=js-only (and others, like a=chrome-only and a=npotb) in the set of automatic approvals.

https://tbpl.mozilla.org/?noignore=1&rev=b13bfc70bc44

If it landed before the build infrastructure started building Wednesday's nightlies, it'll appear in those nightlies (which may or may not include Windows, given the Windows build bustage that is responsible for the tree restrictions).  Otherwise, it'll appear in Thursday's nightlies.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
The change did land before the build infrastructure started building Wednesday's nightlies, so it'll appear in them.
(Reporter)

Comment 13

5 years ago
On the try build, I was successfully able to login with my google account on GIS Cloud. The twitter account works except for the issue specified, which is known with the app. I'll want to find a test case where twitter is used correctly to make sure it works as expected.
(Reporter)

Comment 14

5 years ago
Nope, this does not work always for twitter. Fails with lanyrd mobile, a top app, since it's using twitter.com as its origin, not api.twitter.com. If you try to login in lanyrd mobile (can be found on marketplace.mozilla.org or https://apps.mozillalabs.com/appdir/?db=db/apps-preview.json), it will redirect to twitter.com to ask for auth. Logging in will allow you access to lanyrd mobile within the browser, not the application itself.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

5 years ago
Yup. Looks like..

http://lanyrd.com ->
http://lanyrd.com/twitter/signin/ ->
https://twitter.com/oauth/authenticate
(Assignee)

Comment 16

5 years ago
Created attachment 620371 [details] [diff] [review]
followup v1

Potentially someone could incorrectly link to https://www.twitter.com too which would then redirect to https://twitter.com but only after being kicked to the default browser..
Attachment #620371 - Flags: review?(myk)
Comment on attachment 620371 [details] [diff] [review]
followup v1

Since inbound is closed (and wouldn't be guaranteed to merge into central in time for tomorrow's nightly even if open), please land this on central with r=myk, a=js-only.
Attachment #620371 - Flags: review?(myk) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1bc4679ac516
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 19

5 years ago
For a typical user, this fix will not work to use lanyrd mobile on an app, as the default is to use http://twitter.com, not https://twitter.com. For Ed, https was likely used since he logged in with a twitter account that always requires https. However, Ed's argument is valid that for our temporary solution for allowed_origins, allowing http is risky. I do want to see a verification that the https version works though for lanyrd mobile. Still trying to figure out how to verify this with https for twitter though (I've had no luck so far).
(Reporter)

Updated

5 years ago
Whiteboard: [topapps], [marketplace-beta+] → [topapps], [marketplace-beta=]
I can verify Bug 750568 is fixed, my twitter account didn't indicate any error.
Windows 7, nightly build 20120504

Updated

5 years ago
Whiteboard: [topapps], [marketplace-beta=] → [topapps], [marketplace-beta=] [testday-20120502]
(Assignee)

Comment 21

5 years ago
(In reply to Gabriela from comment #20)
> I can verify Bug 750568 is fixed, my twitter account didn't indicate any
> error.
Curious, which app did you use to test?
(In reply to Edward Lee :Mardak from comment #21)
> (In reply to Gabriela from comment #20)
> > I can verify Bug 750568 is fixed, my twitter account didn't indicate any
> > error.
> Curious, which app did you use to test?

I used GIS Cloud, as the example above.
(Reporter)

Comment 23

5 years ago
(In reply to Gabriela from comment #22)
> (In reply to Edward Lee :Mardak from comment #21)
> > (In reply to Gabriela from comment #20)
> > > I can verify Bug 750568 is fixed, my twitter account didn't indicate any
> > > error.
> > Curious, which app did you use to test?
> 
> I used GIS Cloud, as the example above.

Confirmed. Marking as verified.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Whiteboard: [topapps], [marketplace-beta=] [testday-20120502] → [topapps], [marketplace-beta=] [testday-20120504]]
(Reporter)

Updated

5 years ago
No longer blocks: 737571
Component: Desktop Runtime → Webapp Runtime
Product: Web Apps → Firefox
(Reporter)

Updated

5 years ago
Flags: in-moztrap-
(Reporter)

Updated

5 years ago
QA Contact: desktop-runtime → jsmith
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.