octokit / discussions

discussions and planning for Octokit clients
7 stars 5 forks source link

How to handle 2FA authentication? #11

Open gr2m opened 6 years ago

gr2m commented 6 years ago

context: https://github.com/octokit/node-github/issues/669

I’d love to hear the other teams’ opinions / thoughts on this @octokit/rb @octokit/net

ryangribble commented 6 years ago

What's the context where basic auth is being used on a 2fa enabled account?

Given the primary use is for automated systems they wouldn't have access to a rolling 2fa code they could provide anyway would they?

Using a personal access token is the way I would normally do this rather than basic username/password

gr2m commented 6 years ago

A usecase where it could be problematic is an app where users provide username/password, so authentication is basic. Users with 2FA enabled won’t be able to use it at all I guess?

Maybe we should discourage the usage of basic authentication altogether, as it won’t be supported with GraphQL anyway, as far as I can tell?

kytrinyx commented 6 years ago

A usecase where it could be problematic is an app where users provide username/password, so authentication is basic.

I think we should strongly discourage this. The only time I think it would be ok to use basic auth is if you're writing a one-off script for yourself... but even then I would encourage people to just get themselves a personal access token.

it won’t be supported with GraphQL anyway, as far as I can tell?

Ah. I'm double-checking that, but if so it makes even more sense to simply discourage the use of basic auth altogether.

kytrinyx commented 6 years ago

It looks like currently Basic Auth works with GraphQL, but it's undocumented, and I'm not sure that we have decided whether or not this is something that will be supported going forward.

colinrymer commented 6 years ago

I'd love to see 2FA supported - the existing v3 REST docs specifically describe using Basic Auth to generate personal access tokens programmatically[0] for non-website applications[1], which we want to do so that we can keep users inside a CLI tool we're building and not require that they switch to their browser, generate the token, and then copy/paste it into the tool.


0 - https://developer.github.com/v3/oauth_authorizations/#create-a-new-authorization 1 - "Note that OAuth2 tokens can be acquired programmatically, for applications that are not websites." - https://developer.github.com/v3/#authentication

kytrinyx commented 6 years ago

@colinrymer That's a good use case. (I had forgotten that) I use this REST endpoint fairly regularly with curl to create personal tokens.

Just to clarify: is this discussion about how to support 2FA from within the client wrappers?

If I were to try to use this from within Octokit.rb I would go straight to the client.post method.

colinrymer commented 6 years ago

@kytrinyx Not sure about the other wrappers, but this discussion seems to be started because of a request for 2FA support in the JS client (that's how I ended up here :grin:), as that client provides an authenticate function that supports basic auth credentials as arguments but doesn't include 2FA support, so what could be one simple function call becomes more complex. I have an example of that here - see lines 15-17.

kytrinyx commented 6 years ago

@colinrymer Ok, that makes sense, thanks.

My gut instinct is that we should have a way to support it with a single call. I don't mind if that means dropping down to some lower-level call in the library, as long as we document it.

gr2m commented 6 years ago

@colinrymer for the JavaScript Octokit specifically I'd keep the discussion at https://github.com/octokit/rest.js/issues/669. I'll address the issue as part of the next milestone.

As a general learning, the conceptional problem with the existing client.authenticate from the JavaScript REST Octokit is that it is synchronous. The 2FA authentication requires a on-time request to generate a token which should be used after, so it needs to be asynchronous.

See also my follow up at https://github.com/octokit/rest.js/issues/669#issuecomment-368184029