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

github basic authentication does not support 2FA #669

Closed
jtrinklein opened this issue Dec 7, 2017 · 11 comments · Fixed by #1204
Closed

github basic authentication does not support 2FA #669

jtrinklein opened this issue Dec 7, 2017 · 11 comments · Fixed by #1204
Assignees
Labels
Status: Pinned A way to keep old or long lived issues around Type: Feature New feature or request

Comments

@jtrinklein
Copy link

jtrinklein commented Dec 7, 2017

If a user has 2FA enabled, the authenticate() call will fail. This example will reproduce the issue:

  1. Enable 2FA on your account
  2. Run the basic auth example
  3. Make any other call, for example request user info
github.authenticate({
  type: 'basic',
  username: process.env.USERNAME,
  password: process.env.PASSWORD
})

github.users.getForUser({username: 'jtrinklein'})

This will fail with the following error:

{
  message: '{"message":"Must specify two-factor authentication OTP code.","documentation_url":"https://developer.github.com/v3/auth#working-with-two-factor-authentication"}',
  code: 401,
  status: 'Unauthorized',
  headers:
  { server: 'GitHub.com',
     status: '401 Unauthorized',
     'x-github-otp': 'required; app',
     <snip>
  }
}

Github documentation for working with two factor authentication says:

For users with two-factor authentication enabled, Basic Authentication requires an extra step. When you attempt to authenticate with Basic Authentication, the server will respond with a 401 and an X-GitHub-OTP: required; :2fa-type header. This indicates that a two-factor authentication code is needed (in addition to the username and password). The :2fa-type in this header indicates whether the account receives its two-factor authentication codes via SMS or via an application.

In addition to the Basic Authentication credentials, you must send the user's authentication code (i.e., one-time password) in the X-GitHub-OTP header.

✏️ Forgot to specify you need to make an actual API request for the authentication failure to manifest.

@gr2m
Copy link
Contributor

gr2m commented Dec 8, 2017

Thanks @jtrinklein that’s definitely something we should cover, it won’t be easy with the current API ...

The first thought would be to add another param to github.authenticate so you could do

github.authenticate({
  type: 'basic',
  username: process.env.USERNAME,
  password: process.env.PASSWORD,
  code: twoFactorAuthCode
})

Would that help? Any other ideas?

I’ll ask the other Octokit maintainers how they handle it right now

@jtrinklein
Copy link
Author

That's exactly what I was thinking. I can't really see a better way around it at this time. It would definitely be enough for my use case anyway.

@jtrinklein
Copy link
Author

Actually I'm not sure that would work... After the first use the code would become invalid. It would need to do something like detect that code was provided, authenticate and generate an api key..?

@gr2m
Copy link
Contributor

gr2m commented Dec 9, 2017

Yeah this is not ideal. I’ll have to experiment with it. Creating an access token with the basic + 2FA code is what GitHub recommends to do, it’s currently not well covered by the APIs.

But you can pass custom headers to the API calls

const GitHub = require('.')
const client = new GitHub()

client.authenticate({
  type: 'basic',
  username,
  password
})

const {data: {token} = await client.authorization.create({
  note: 'funky',
  headers: {
    'x-github-otp': twoFactorCode
  }
})

client.authenticate({
  type: 'token',
  token
})

Does that work for you?

@jtrinklein
Copy link
Author

Yes, that will work. Thank you.

@gr2m gr2m self-assigned this Dec 26, 2017
@gr2m gr2m added the Status: Pinned A way to keep old or long lived issues around label Dec 26, 2017
@gr2m gr2m added the Type: Feature New feature or request label Feb 17, 2018
@gr2m gr2m added this to the v15.0.0 - Revise APIs milestone Feb 17, 2018
@gr2m
Copy link
Contributor

gr2m commented Feb 24, 2018

@colinrymer as a follow up to octokit/discussions#11 (comment)

The latest @octokit/rest already has internal APIs to register an asynchronous method that is run before requests are sent. See https://github.com/octokit/rest.js/tree/master/lib/plugins#usage for an example. I'll make that public with the next milestone, but the API might change slightly, so better pin the @octokit/rest version in your package.json if you use that.

client.authenticate() is already implemented using that API, see the implementation at https://github.com/octokit/rest.js/tree/master/lib/plugins/authentication. It just doesn’t take advantage of it yet. The before-request.js is synchronous right now but could as well be async returning a promise, which would allow us to implement something like

client.authenticate({
  type: '2fa',
  username,
  password
  twoFactorCode
})

On the next request it would get a token using client.authorization.create({}) and then probably set state.auth.type to 'token' and state.auth.token to the received token.

Does that make sense? I'd love folks to experiment with these APIs to see if the work well with common use cases.

@zeidlos
Copy link

zeidlos commented Mar 14, 2018

Hi @gr2m what can I help you to make this into a release asap? :)

@gr2m
Copy link
Contributor

gr2m commented Mar 14, 2018

@zeidlos thanks for your help!

The best thing to do right now would be to create a plugin that implements a new method such as .authenticate2fa so we can discuss the implementation and everyone is unblocked. Then use it in your apps/libraries to make sure it doesn’t break anything. Then we can discuss how to incorporate it into @octokit/rest core.

Sounds good?

@gr2m
Copy link
Contributor

gr2m commented Nov 28, 2018

The latest news on this is that I plan to add proper support for 2FA authentication with octokit.js. My current idea is this:

const clientWithAuth = new Octokit({
  auth: {
    username,
    password,
    async on2Fa () {
     // ask user for 2FA code here and resolve with the code
   }
})

Internally, username and password will always be used to create a token and then discarded. If the server responds that a 2FA code is required, the optional on2Fa() method is called in which you can add what ever logic you need to ask the user for the 2FA code.

Would that work for all your usecases?

@gr2m
Copy link
Contributor

gr2m commented Jan 17, 2019

Today I thought why wait for v17 if we can just do

octokit.authenticate({
  type: 'basic',
  username: 'yourusername',
  password: 'password'
  on2fa () {
    // must return or resolve with a two-factor code
  }
})

so here we go: https://github.com/octokit/rest.js#authentication 🎉

@octokitbot
Copy link
Collaborator

🎉 This issue has been resolved in version 16.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Pinned A way to keep old or long lived issues around Type: Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants