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

fetches userId from the oauth_token (the part before '-') if manually… #14

Merged
merged 2 commits into from Oct 19, 2015

Conversation

webstersx
Copy link
Contributor

… specified -- user_id isn't in Twitter's own instructions for implementing twitter login: https://dev.twitter.com/web/sign-in/implementing

… specified -- user_id isn't in Twitter's own instructions for implementing twitter login: https://dev.twitter.com/web/sign-in/implementing
@webstersx
Copy link
Contributor Author

oops, typo.

should read as: if not manually specified

@ghaiklor
Copy link
Collaborator

@webstersx could you cover it with tests ?

And, does it make sense ? I mean, oauth-token doesn't contain user_id so why you need this ?

@webstersx
Copy link
Contributor Author

@ghaiklor my twitter 'oauth_token' contains my twitter user id

Am I just misunderstanding that the user_id is meant to be the user's id in my database for passport-twitter-token (rather than their twitter user id)? Edit: nope, see comment below

when I don't specify user_id in the body, passport causes a 500: InternalOAuthError: failed to fetch user profile

@webstersx
Copy link
Contributor Author

It looks as though when _skipExtendedUserProfile is false, A twitter user id is required as 'user_id'

Since the user_id is already in the oauth_token, my thinking is that it would make sense to just read it out of there instead of requiring another param

TwitterTokenStrategy.prototype.userProfile = function(token, tokenSecret, params, done) {
  if (!this._skipExtendedUserProfile) {
    this._oauth.get('https://api.twitter.com/1.1/users/show.json?user_id=' + params.user_id, token, tokenSecret, function (err, body, res) {

As long as the format of the oauth_token is always of the format {twitter user id}-{random characters} this should be okay -- happy to hear your thoughts

@ghaiklor
Copy link
Collaborator

@webstersx I'll be happy to merge this, but as I ask, cover it with tests, please.
Your code doesn't break backward compatibility, but it must be covered with tests.

@@ -55,7 +55,7 @@ export default class TwitterTokenStrategy extends OAuthStrategy {

let token = (req.body && req.body[this._oauthTokenField]) || (req.query && req.query[this._oauthTokenField]);
let tokenSecret = (req.body && req.body[this._oauthTokenSecretField]) || (req.query && req.query[this._oauthTokenSecretField]);
let userId = (req.body && req.body[this._userIdField]) || (req.query && req.query[this._userIdField]);
let userId = (req.body && req.body[this._userIdField]) || (req.query && req.query[this._userIdField]) || (token.substr(0, token.indexOf('-')));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can change to token.split('-')[0]. It more easily to read what it did.

@webstersx
Copy link
Contributor Author

@ghaiklor I'm having trouble writing the tests as I don't see where the existing userId portion is being tested :( (I'm also an iOS programmer by trade so there's that too!)

Can you offer any suggestions for how to accomplish this?

Cheers

@ghaiklor
Copy link
Collaborator

@webstersx I can add it myself though, but tomorrow. I will try not to forgot about this 👍

ghaiklor added a commit that referenced this pull request Oct 19, 2015
fetches userId from the oauth_token (the part before '-') if manually…
@ghaiklor ghaiklor merged commit 8302158 into drudge:master Oct 19, 2015
@ghaiklor
Copy link
Collaborator

@webstersx published under 1.1.0

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

2 participants