Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add interactive param to oauth provider #70

Merged
merged 7 commits into from
Jul 31, 2015
Merged

Add interactive param to oauth provider #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f1961a9
Add interactive param to oauth provider
dborkan Jun 29, 2015
e2ba258
Add timeout to close tab if !interactive
dborkan Jul 9, 2015
44ffd98
Fix fail timeout
dborkan Jul 9, 2015
7c8fb9c
Merge branch 'master' of https://github.com/freedomjs/freedom-for-fir…
dborkan Jul 10, 2015
dc3835a
Remove backup file
dborkan Jul 10, 2015
c9c6b52
Fix package.json
dborkan Jul 10, 2015
a047fa4
Change undefined check to use typeof
dborkan Jul 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions demo/aboutme/data/aboutme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ instance.on('start', function() {
"scope=" + encodeURIComponent("https://www.googleapis.com/auth/userinfo.profile") + "&" +
"redirect_uri=" + encodeURIComponent(obj.redirect) + "&" +
"state=" + encodeURIComponent(obj.state);
return oauth.launchAuthFlow(url, obj);
return oauth.launchAuthFlow(url, obj, true);
//instance.emit("oAuth", url);
}).then(function(responseUrl) {
var query = responseUrl.substr(responseUrl.indexOf('#') + 1),
Expand All @@ -33,7 +33,7 @@ instance.on('start', function() {
param = keys[i].substr(0, keys[i].indexOf('='));
params[param] = keys[i].substr(keys[i].indexOf('=') + 1);
}

// https://developers.google.com/api-client-library/javascript/features/cors
// claims that googleapis.com supports CORS Headers. This is a lie.
// However, undocumented everywere is the fact that the endpoint API does
Expand Down
29 changes: 26 additions & 3 deletions providers/oauth/oauth.httpobserver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,19 @@ FirefoxTabsAuth.prototype.initiateOAuth = function (redirectURIs, continuation)
return false;
};

FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, continuation) {
FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, interactive, continuation) {
"use strict";
var gBrowser = wm.getMostRecentWindow("navigator:browser").gBrowser,
listener,
complete,
fail,
tab,
previousTab = gBrowser.selectedTab;

if (typeof interactive === "undefined") {
interactive = true;
}

var httpRequestObserver = {
observe: function(subject, topic, data) {
if (topic == "http-on-modify-request") {
Expand All @@ -65,22 +70,40 @@ FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, continua
}
};

var hasCredentials = false;
complete = function (location) {
hasCredentials = true;
httpRequestObserver.unregister();
gBrowser.selectedTab = previousTab;
if (interactive) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment to freedom-for-chrome.
I don't believe this code matches either behavior that we want and/or the behavior of https://developer.chrome.com/apps/identity

It'll still open the tab, so I'm not sure what's the point of an interactive flag parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar response to the freedom-for-chrome pull request:

All of these changes are being done to make Facebook reconnect a better UX. Facebook, unlike Google, doesn't let us get refresh (long-lived) tokens, so there is no way for us to get a valid Facebook token without opening an oauth tab. However, in most cases where we need to reconnect a user to Facebook, the user is going to be still logged into Facebook in their browser and still granting uProxy permission - this means that the auth tab will just open, redirect to uproxy.org with the token, then uProxy will close the tab. It is fairly annoying right now when this happens and the tab is focused on (a common scenario for me is I'm logged into FB on uProxy, lock my computer, come back and start doing unrelated stuff in Chrome, then suddenly a tab opens and closes).

What we want for uProxy is for this tab to open in the background (not be focused on). We've had this briefly working in Chrome where we override the core.oauth provider.

In the case of Firefox specifically, I've tested these changes manually in uProxy and verified that when we set interactive=true (for initial sign in), the tab is focused on, but when its false (for reconnect) the tab isn't focused

gBrowser.selectedTab = previousTab;
}
// Need to wait until the tab has switched before removing it
setTimeout(function () {
gBrowser.removeTab(tab);
continuation(location);
}, 100);
};

fail = function () {
gBrowser.removeProgressListener(listener);
gBrowser.removeTab(tab);
continuation(undefined, 'Error in launchAuthFlow');
};

httpRequestObserver.register();

//Timeout before switching makes sure the http observer is installed.
setTimeout(function () {
tab = gBrowser.addTab(authUrl);
gBrowser.selectedTab = tab;
if (interactive) {
gBrowser.selectedTab = tab;
} else {
setTimeout(function () {
if (!hasCredentials) {
fail();
}
}, 5000);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to check the reasoning for why 5000ms is the appropriate timeout here - is this something that could potentially block the user for this long?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows time for the new tab to launch and be redirected from the Facebook URL to the direct URL we supply in the arguments. I picked 5 seconds because it seemed like a reasonable amount of time to wait for a slower connection. I could lower this if needed (that might make the integration tests easier if it were lowered below 3000ms), but I think it would be a pretty bad UX if the tab was closed too early causing the non-interactive login (reconnect in uProxy) to fail

}
}, 100);
};

Expand Down
29 changes: 26 additions & 3 deletions providers/oauth/oauth.progresslistener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,19 @@ FirefoxTabsAuth.prototype.initiateOAuth = function (redirectURIs, continuation)
return false;
};

FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, continuation) {
FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, interactive, continuation) {
"use strict";
var gBrowser = wm.getMostRecentWindow("navigator:browser").gBrowser,
listener,
complete,
fail,
tab,
previousTab = gBrowser.selectedTab;

if (typeof interactive === "undefined") {
interactive = true;
}

listener = {
QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener", "nsISupportsWeakReference"]),

Expand All @@ -65,22 +70,40 @@ FirefoxTabsAuth.prototype.launchAuthFlow = function (authUrl, stateObj, continua
}
};

var hasCredentials = false;
complete = function (location) {
hasCredentials = true;
gBrowser.removeProgressListener(listener);
gBrowser.selectedTab = previousTab;
if (interactive) {
gBrowser.selectedTab = previousTab;
}
// Need to wait until the tab has switched before removing it
setTimeout(function () {
gBrowser.removeTab(tab);
continuation(location);
}, 100);
};

fail = function () {
gBrowser.removeProgressListener(listener);
gBrowser.removeTab(tab);
continuation(undefined, 'Error in launchAuthFlow');
};

gBrowser.addProgressListener(listener);

//Timeout before switching makes sure the progress listener is installed.
setTimeout(function () {
tab = gBrowser.addTab(authUrl);
gBrowser.selectedTab = tab;
if (interactive) {
gBrowser.selectedTab = tab;
} else {
setTimeout(function () {
if (!hasCredentials) {
fail();
}
}, 5000);
}
}, 100);
};

Expand Down