jaredhanson / passport-twitter

Twitter authentication strategy for Passport and Node.js.
https://www.passportjs.org/packages/passport-twitter/?utm_source=github&utm_medium=referral&utm_campaign=passport-twitter&utm_content=about
MIT License
468 stars 131 forks source link

Preferred way to integrate email requesting? #73

Open tayste5000 opened 8 years ago

tayste5000 commented 8 years ago

Twitter recently added permission to access the user's email (app needs to request special permissions and be whitelisted by twitter) through the endpoint:

.../account/verify_credentials.json?include_email=true

This doesn't work with the method in this repo because it automatically adds:

?user_id={users id}

which clashes with ?include_emails=true when you provide the previously mentioned URL as userProfileURL. I would like to submit a pull request that will make this work but am not sure what the preferred route would be.

I'm guessing we want to minimize the impact that any change would have on apps using this library who aren't trying to get emails. Perhaps we could do:

...

if (this._userProfileURL == ".../account/verify_credentials.json?include_email=true"){
    profileURL = this._userProfileURL
}
else{
    profileURL = this._userProfileURL + "?user_id" + params.user_id
}  
this._oauth.get(profileURL, token, tokenSecret, ...

but maybe there is a more elegant solution?

Also I would want to add:

...
profile.emails = [{value: json.email}];
....

to the profile parsing function (all the other passport-providers generate profile objects that follow this format)

Thoughts?

danielstjules commented 8 years ago

I'm just monkey-patching the method for now, but it would be great to see this change in passport-twitter!

davidwieler commented 8 years ago

Is this still an issue? I can't for the life of me get the email working with the Twitter strategy.

Edit:

I was running an older version that had this issue still. Simply updated the module and it worked fine.

Example Code:

// route for twitter authentication and login
app.get('/auth/twitter', passport.authenticate('twitter', { scope: ['include_email=true']}));
   // Strategy Portion
    passport.use(new TwitterStrategy({

        consumerKey     : configAuth.twitterAuth.consumerKey,
        consumerSecret  : configAuth.twitterAuth.consumerSecret,
        callbackURL     : configAuth.twitterAuth.callbackURL,
        passReqToCallback : true,
        includeEmail: true

    },
    function(req, token, tokenSecret, profile, done) {

        // make the code asynchronous
        process.nextTick(function() {
            console.log(profile.emails[0].value)

        }

    })

Would recommend closing this Issue.

davidwieler commented 8 years ago

@danielstjules,

Worked just fine for me. I editing my comment to show a working example, once the App was whitelisted by Twitter of course.

To get whitelisted, use this URL: https://support.twitter.com/forms/platform, select "I need access to special permissions", then enter "Email" into "Permissions Requested"

danielstjules commented 8 years ago

Nvm, fixed in 1.0.4 :)

cfjedimaster commented 7 years ago

So I'm trying like heck to get emails as well. When I add includeEmail:true to my strategy, and then do

 app.get('/auth/twitter', passport.authenticate('twitter', {scope:['include_email=true']}));

When I return from my Twitter auth, I get this in my console:

TypeError: Cannot read property '0' of undefined at Strategy._verify (c:\projects\snowball\app.js:50:24) at c:\projects\snowball\node_modules\passport-oauth1\lib\strategy.js:184:24 at c:\projects\snowball\node_modules\passport-twitter\lib\strategy.js:158:7 at passBackControl (c:\projects\snowball\node_modules\oauth\lib\oauth.js:390:11) at IncomingMessage. (c:\projects\snowball\node_modules\oauth\lib\oauth.js:409:9)

cfjedimaster commented 7 years ago

So it turns out it's related to requesting permission - as commented on above. Should this be documented though?

davidwieler commented 7 years ago

@cfjedimaster it really should be. This topic was opened about a year ago, and it still pops up from time to time