popomore / github-labels

Add github labels automatically
177 stars 20 forks source link

Allow two-factor authentication in token creation. #3

Closed henrahmagix closed 9 years ago

henrahmagix commented 10 years ago

Fixes #2

popomore commented 10 years ago

Thanks, I will look it soon.

kenany commented 9 years ago

@henrahmagix Just tried your branch but I'm not getting the code delivered to my phone.

henrahmagix commented 9 years ago

Thanks @KenanY! I haven't tested with SMS 2FA, only with an app (Google Authenticator). After an initial look around the GitHub docs, I haven't found how to trigger an SMS. I will continue to look =)

henrahmagix commented 9 years ago

@KenanY GitHub support have told me that if I receive the X-GitHub-OTP header, GitHub considers that a "sign-in attempt" and so will send an SMS to users who use that as their method of two-factor authentication.

In the case of you not receiving a code, I'd recommend ensuring you have it setup correctly. In the mean time I will try to test it myself, but at the moment I am abroad so it'll have to wait until I am home.

Extra reading:

kenany commented 9 years ago

Alright, I'll have to look into it more then. I'm sure my current setup works because I've used it with ghauth and for logging into GitHub in general. Maybe an SMS hiccup, I should try once more today.

kenany commented 9 years ago

I'm still suspecting that something is wrong with this code somewhere, trying to look into now. I can confirm that SMS authentication is setup properly for me with a ghauth script like so:

var ghauth = require('ghauth');

ghauth({configName: 'test'}, function(error, data) { });

I get the code pretty much instantly after running this script but not when running this github-labels branch. AFAICT from the code (trying to learn the flow; I have no experience with generators), this is what is happening:

  1. Figure out that user has 2FA (has2FA())
  2. Create headers for the authentication request (createHeaders())
    • Ask user for an 2FA code that github-labels hasn't requested GitHub for (get2FA())

After that, well, everything seems stuck.

if I receive the X-GitHub-OTP header

Sorry, you or GitHub? If you mean GitHub, no where in this code does that header get sent unless the user enters a 2FA code.

kenany commented 9 years ago

This works:

var GitHubApi = require('github');

var github = new GitHubApi({
    version: '3.0.0'
});

github.authenticate({
    type: 'basic',
    username: 'KenanY',
    password: '...'
});

github.authorization.create({
    scopes: ['gist'],
    note: '...',
    note_url: '...'
}, function(err, res) {
  if (err) {
    throw err;
  }
});

err is thrown of course because a code in the X-GitHub-OTP header is needed but I do receive the text message this way.

EDIT: So code isn't sent until authorization creation is attempted? That would explain why github-labels isn't sending me the code: it's waiting for me to input a code before creating the authorization.

henrahmagix commented 9 years ago

if I receive the X-GitHub-OTP header

Sorry, you or GitHub? If you mean GitHub, no where in this code does that header get sent unless the user enters a 2FA code.

My apologies for being confusing: I think I mistyped. From the GitHub support email:

If you receive this header, a code will be sent to the user in the case that they are using SMS, because it is effectively an attempt to sign in.

The X-GitHub-OTP header should be sent by GitHub in their error response when attempting to login with basic authentication (user:pass) when a 2FA code is required.

My thinking was that userRequires2FA() should trigger an SMS when github.authorization.getAll() is called. Unfortunately one can't check the response headers of a call made with the github package, but I was certain that the failed basic authentication (attempted in userRequires2FA()) was incurring a response with the X-GitHub-OTP header, as explained above.

@KenanY Since you have a working script (in your comment), could you change github.authorization.create to github.authorization.getAll and test that? That might inform us further as to the source of the problem.

henrahmagix commented 9 years ago

Since writing this pull-request, I moved to creating a simple library for CLI prompting a GitHub OAuth token. I don't mean to self-promote, but I would be so grateful @KenanY if you could test my library to see if an SMS is being triggered correctly. If that works, I can come back and update this to use my library. If not, we could move this issue over to my repo and once fixed use my library here.

https://www.npmjs.org/package/github-oauth-prompt

kenany commented 9 years ago

github.authorization.getAll does not seem to trigger the SMS. I guess you'd have to at least attempt an authorization creation.

henrahmagix commented 9 years ago

:+1: Parfait! Thanks @KenanY =)

or

popomore commented 9 years ago

Hey @henrahmagix @KenanY do you have any conclusion and update code? I'll merge this PR.

henrahmagix commented 9 years ago

@popomore Done! Ready to merge =)

popomore commented 9 years ago

Good

henrahmagix commented 9 years ago

Thanks!