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

Implement OAuth Device Authorization flow #1522

Merged
merged 2 commits into from
Aug 25, 2020
Merged

Implement OAuth Device Authorization flow #1522

merged 2 commits into from
Aug 25, 2020

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Aug 13, 2020

Before, we implemented the OAuth app authorization flow which requires a callback URL. To provide such a URL, we had to spin up a local HTTP server, which was brittle and did not cover cases where a person might want to authenticate with a browser that runs on a different machine than the GitHub CLI process.

This implements the OAuth Device Authorization flow where the user is given a one-time code and asked to paste it in the browser flow. There is no callback URL, so we can avoid spinning up a local server, and the user may open a browser on any of their devices, as long as they provide the correct one-time code.

This is how it looks like:
Screen Shot 2020-08-13 at 19 28 37

Here is the web-based portion of the flow

Screen Shot 2020-08-13 at 19 15 37

Screen Shot 2020-08-13 at 19 16 03

Screen Shot 2020-08-13 at 19 16 20

Screen Shot 2020-08-13 at 19 16 25

The user has to then close their browser tab and return to the terminal:
Screen Shot 2020-08-13 at 19 30 52

If the Device Authorization flow is detected to be unavailable for the OAuth app (right now, it's specifically enabled for GitHub CLI) or for an older GitHub Enterprise instance, this falls back to the old app authentication flow.

Fixes #297, fixes #773

Before, we implemented the OAuth app authorization flow which requires a
callback URL. To provide such a URL, we had to spin up a local HTTP
server, which was brittle and did not cover cases where a person might
want to authenticate with a browser that runs on a different machine
than the GitHub CLI process.

This implements the OAuth Device Authorization flow where the user is
given a one-time code and asked to paste it in the browser flow. There
is no callback URL, so we can avoid spinning up a local server, and the
user may open a browser on any of their devices, as long as they provide
the correct one-time code.

If the Device Authorization flow is detected to be unavailable for the
OAuth app (right now, it's specifically enabled for GitHub CLI) or for
an older GitHub Enterprise instance, this falls back to the old app
authentication flow.
@mislav mislav requested a review from vilmibm August 13, 2020 17:33
@tierninho
Copy link
Contributor

tierninho commented Aug 13, 2020

Nice work!

  • Looks like we need a space here after h:
    Screen Shot 2020-08-13 at 8 49 34 AM

  • I noticed we used -h for host. Aren't we still accepting -h as an alternative for --help?

  • Also, when I re-auth using our ghe site, I run into a 403 error. The PAT works fine, but for the web login, should I use the old app authentication flow?

    gh auth login -h ghe.io
    - Logging into ghe.io
    ? How would you like to authenticate? Login with a web browser
    
    failed to authenticate via web browser: error: HTTP 403 (https://ghe.io/login/device/code)
    
  • Lastly, I created a one-time code, hit Ctl-C to exit, made another one-time code and then used the first one to authenticate. When I got back to CLI to complete the authentication, it hung as I used the first code. Should we recognize both so the user can proceed to ✓ Authentication complete. Press Enter to continue...?

I want to avoid falling back to the old OAuth flow for just any HTTP
4xx/5xx because other statuses should be allowed to surface a problem
with a request or the server.
Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rules 👍

We should be printing the URL out so that users can choose to abort and manually open it, but that can be a follow up

@mislav
Copy link
Contributor Author

mislav commented Aug 14, 2020

@tierninho Thanks for the thorough review!

  • I noticed we used -h for host. Aren't we still accepting -h as an alternative for --help?

We are, for commands that don't define their own -h flag. In general, the -h shorthand for --help is offered as convenience, but not a guarantee.

  • Also, when I re-auth using our ghe site, I run into a 403 error.

Ah, that's my bad. I just pushed a change that fixes logging in to GHE using the old flow! Thanks for the heads-up 🙇

  • I created a one-time code, hit Ctl-C to exit, made another one-time code and then used the first one to authenticate. When I got back to CLI to complete the authentication, it hung as I used the first code.

I can see how this is a confusing experience, but there's no way around it that I can think of. Because you've Ctrl-C'ed the original process, you cannot use that first one-time code to complete the authentication process. The server will accept it, but there is no client to finalize the flow with that code anymore.

Thanks for raising this, but I think it's an edge case which our users typically won't run into.

@tierninho
Copy link
Contributor

Thanks for the update. Confirming the 403 error is no longer there with the changes.

Was there supposed to be a space here:

fmt.Fprintf(opts.IO.ErrOut, "- gh config set -h%s git_protocol %s\n", hostname, gitProtocol)
in regards to:

90174653-0930e200-dd42-11ea-95f6-e8bbf95776d3

@mislav
Copy link
Contributor Author

mislav commented Aug 18, 2020

Was there supposed to be a space here:

@tierninho This wasn't introduced in this PR, but we can tweak it here!

@vilmibm Do you feel strongly about the gh config set line staying printed? On one hand, I can see how this way we are teaching users on how to use gh config. On the other hand, I think the line might cause confusion because people might interpret it as a command they need to run themselves, rather than as something that was ran for them.

If we keep it, should we expand it to a more readable gh config set --host ghe.io git_protocol https?

@vilmibm
Copy link
Contributor

vilmibm commented Aug 18, 2020

I'd like to keep it in; I'm in favor of it being more explicit.

@jsejcksn
Copy link

jsejcksn commented Sep 8, 2020

We should be printing the URL out so that users can choose to abort and manually open it, but that can be a follow up

https://github.com/login/device

That's the URL that should be used for a separate device, right?

@mislav
Copy link
Contributor Author

mislav commented Sep 9, 2020

@jsejcksn Yes. You can also ensure that the URL is printed by setting BROWSER=echo. But, gh will print the URL if it didn't manage to successful open a web browser anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants