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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Netflix OAuth Module #25

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

slickplaid
Copy link
Contributor

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

@@ -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) {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Owner

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.

@bnoguchi
Copy link
Owner

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 req.user. Can you please update your code in ./example/server.js to match the new example app additions? In particular, it comes down to using addUser. See the other modules configured in ./example/server.js for examples of what I mean.

@slickplaid
Copy link
Contributor Author

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,
slickplaid

@slickplaid
Copy link
Contributor Author

Something broke in the last merge... Let me fix it and I'll commit again here later today or tomorrow.

Error: Step rememberTokenSecret of `netflix` is promising: requestTokenSecret ; however, the step returns nothing. Fix the step by returning the expected values OR by returning a Promise that promises said values.

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.

2 participants