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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle authentication type in Facebook #5694

Merged
merged 2 commits into from May 11, 2016

Conversation

nuvipannu
Copy link
Contributor

Allow using authType in Facebook login (https://developers.facebook.com/docs/facebook-login/reauthentication) 馃巿

@timbotnik
Copy link

Seems useful, thanks!

@IonicaBizau
Copy link

@nuvipannu 馃憤 鉁

@timbotnik Take a look at #5693 as well. 馃榿 While coding together with @nuvipannu, we discovered these limitations and thought it's a good thing for universe to fix them. For some reason the tests are failing, but doesn't seem to be related to our changes. 馃挮

@robertpitt
Copy link
Contributor

You will probably need to update the documentation to show the new option before this is accepted.

@IonicaBizau
Copy link

@robertpitt Where should I update the docs for these OAuth APIs? 馃挱

@IonicaBizau
Copy link

@robertpitt The relevant docs I found are these鈥撯揵ut they don't contain the specific facebook/twitter information.

@IonicaBizau
Copy link

@benjamn Can you please take a look at this pull request and #5693? 馃敂

@IonicaBizau IonicaBizau force-pushed the facebook-force-login branch 2 times, most recently from c7b6113 to 67d9cb7 Compare December 12, 2015 15:50
@JonathanHindi
Copy link

@benjamn What's pending for this to be merged?

@IonicaBizau
Copy link

@benjamn @JonathanHindi I'm pretty sad to see how much time it takes for a simple fix like this to get merged (actually, this is not even merged yet). 馃槥


// Handle authentication type (e.g. for force login you need authType: "reauthenticate")
if (options.authType) {
loginUrl = loginUrl + "&authType=" + options.authType;
Copy link
Contributor

@benjamn benjamn May 3, 2016

Choose a reason for hiding this comment

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

I think this should maybe be

loginUrl += "&authType=" + encodeURIComponent(options.authType);

to make sure the authType is properly URL-escaped?

Choose a reason for hiding this comment

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

@benjamn I don't think that's really needed, but just in case, I just pushed it.

@IonicaBizau
Copy link

@JonathanHindi @benjamn Any ideas why the tests are failing? 馃挜

Same happens in #6987.
I have the feeling it doesn't happen because of my (or @nuvipannu's) changes. 馃殌

@zol
Copy link
Contributor

zol commented May 10, 2016

Hey @nuvipannu and @JonathanHindi . Sorry it's taken us so long to look at this.We're now working through our PR backlog and merging whatever we can, this looks like a good candidate. Could you please rebase against the latest devel branch as we've recently fixed our CI infrastructure so tests should now be passing (as you said, it's unlikely this PR would have broken any test).

@zol zol self-assigned this May 10, 2016
@IonicaBizau
Copy link

IonicaBizau commented May 11, 2016

@zol Thanks for the feedback. I have just rebased and pushed. Waiting for the tests. 鈴

@IonicaBizau
Copy link

@zol @nuvipannu Tests are passing 馃帀 馃殌

@zol zol merged commit 87bf256 into meteor:devel May 11, 2016
zol pushed a commit that referenced this pull request May 11, 2016
@zol
Copy link
Contributor

zol commented May 11, 2016

Merged, thank you.

@IonicaBizau
Copy link

@nuvipannu Finally! Yay! 馃帀 馃榿 馃帄

@sahanDissanayake
Copy link

sahanDissanayake commented Jun 14, 2016

is this documented on the Guide http://docs.meteor.com/api/accounts.html#Meteor-loginWith ? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants