-
Notifications
You must be signed in to change notification settings - Fork 447
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Netflix OAuth Module #25
base: master
Are you sure you want to change the base?
Conversation
@@ -81,7 +83,7 @@ everyModule.submodule('oauth') | |||
} | |||
|
|||
var p = this.Promise(); | |||
this.oauth.getOAuthRequestToken( function (err, token, tokenSecret, authUrl, params) { | |||
this.oauth.getOAuthRequestToken( function (err, token, tokenSecret, /* authUrl, */ params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the authUrl
variable is doing here. In the corresponding function, it doesn't have that variable passed to this function. I needed to use the params variable so that's why it's been corrected. I left it in commented just so you can see what I did in case it breaks something for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit 7cb7aa078613242d62a3, I have a conflict.
authUrl
variable doesn't seem to be in the parent oauth module. The callback on line 460 of node_modules/oauth/lib/oauth.js
is callback(null, oauth_token, oauth_token_secret, results );
which doesn't include authUrl
Just wanted to get your clarification on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that shouldn't be there. I just removed it in a commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the vigilant eyes.
I'll look at this today. I think there's room for re-factoring the extra auth query params because I have similar code in the oauth2 module. I'm formulating some ideas around module and step sequence mixins, so I think that is the approach to take for re-using additional query param code. In the meantime, I've updated the example app, so the way it stores users now supports the assumptions required for accessing |
Sure I can! There's definitely some room for refactoring (isn't there always?) ... I almost don't think that the application name is needed in the configuration since it's returned when you ask for the auth token. I almost didn't make this module since it seems the netflix oauth implementation is rather neglected by the developers. A lot of the images and stuff that should make logging in feel professional are missing and broken. But, on the other hand, it's a good case to force some flexibility into the code. I'll take a look at the example app now and get back with you if I have any questions. I might open an issue ticket to start developing an example of how to use the addUser/findOrCreateUser (what it expects and returns, required functions to save/append/delete data) with something other than mongoose, too. Thanks, |
Something broke in the last merge... Let me fix it and I'll commit again here later today or tomorrow.
|
I had to slightly modify several functions to make this one work.
Let me know if anything looks out of place or should be done differently.
Thanks,
slickplaid