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

Preferred way to integrate email requesting? #73

Open
tayste5000 opened this issue Dec 24, 2015 · 7 comments
Open

Preferred way to integrate email requesting? #73

tayste5000 opened this issue Dec 24, 2015 · 7 comments

Comments

@tayste5000
Copy link

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
Copy link

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

@davidwieler
Copy link

davidwieler commented Jul 25, 2016

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
Copy link

@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
Copy link

Nvm, fixed in 1.0.4 :)

@cfjedimaster
Copy link

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
Copy link

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

@davidwieler
Copy link

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

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

No branches or pull requests

4 participants