Add OAuth device authorization login - #382
Open
ivankuznetsov wants to merge 1 commit into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/auth/auth.go">
<violation number="1" location="internal/auth/auth.go:171">
P3: `DeviceLoginOptions.log` is an exact copy of the existing `LoginOptions.log` (same Logger-or-stderr routing). Extract the shared routing into one helper and have both option types call it, so the two login flows can't drift.</violation>
<violation number="2" location="internal/auth/auth.go:270">
P2: When the device endpoint returns control characters in a verification URI or user code, `LoginDevice` writes them verbatim to the terminal. Sanitize server-provided values before passing these messages to the logger.</violation>
<violation number="3" location="internal/auth/auth.go:288">
P2: When a pending poll leaves less than `interval` before `expiresAt`, this sleeps the full interval and issues another request after the device code has expired. Cap the wait at the remaining lifetime and check expiry before polling again.</violation>
</file>
<file name="internal/auth/oauth_test.go">
<violation number="1" location="internal/auth/oauth_test.go:108">
P3: The new /token and /device handlers never assert the request method or Content-Type, and the /token case omits the client_id/install_id checks, unlike the sibling TestExchangeCodeRequest and TestRefreshOAuthTokenRequest which assert both method, encoding, and every form field. As written, the test passes even if the client used the wrong method or encoding, or dropped client_id/install_id on the token exchange, so it does not fully cover the PR's stated request-shapes intent.</violation>
</file>
<file name="internal/cmd/auth.go">
<violation number="1" location="internal/cmd/auth.go:73">
P2: The new conflict check counts `--no-browser` as a login method, so `hey auth login --token X --no-browser` and `--cookie C --no-browser` — both valid before this change, where the token/cookie branch ran and `--no-browser` was ignored — now fail with a usage error. `--no-browser` only modifies the browser flow; it is not a method. Count only token/cookie/device as methods, and reject `--device --no-browser` separately so the pinned test still passes.</violation>
<violation number="2" location="internal/cmd/auth.go:100">
P2: When the device endpoint grants an authorization lifetime longer than 16 minutes, this command cancels `LoginDevice` while the user code is still valid. Let the server-provided expiry and the caller's cancellation control the wait instead of imposing a shorter fixed deadline.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return errors.New("device authorization expired") | ||
| } | ||
| if !firstPoll { | ||
| if err := m.wait(ctx, interval); err != nil { |
Contributor
There was a problem hiding this comment.
P2: When a pending poll leaves less than interval before expiresAt, this sleeps the full interval and issues another request after the device code has expired. Cap the wait at the remaining lifetime and check expiry before polling again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/auth.go, line 288:
<comment>When a pending poll leaves less than `interval` before `expiresAt`, this sleeps the full interval and issues another request after the device code has expired. Cap the wait at the remaining lifetime and check expiry before polling again.</comment>
<file context>
@@ -228,6 +254,67 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) error {
+ return errors.New("device authorization expired")
+ }
+ if !firstPoll {
+ if err := m.wait(ctx, interval); err != nil {
+ return err
+ }
</file context>
| if err != nil { | ||
| return err | ||
| } | ||
| opts.log(fmt.Sprintf("Open %s and enter code: %s\n", authorization.VerificationURI, authorization.UserCode)) |
Contributor
There was a problem hiding this comment.
P2: When the device endpoint returns control characters in a verification URI or user code, LoginDevice writes them verbatim to the terminal. Sanitize server-provided values before passing these messages to the logger.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/auth.go, line 270:
<comment>When the device endpoint returns control characters in a verification URI or user code, `LoginDevice` writes them verbatim to the terminal. Sanitize server-provided values before passing these messages to the logger.</comment>
<file context>
@@ -228,6 +254,67 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) error {
+ if err != nil {
+ return err
+ }
+ opts.log(fmt.Sprintf("Open %s and enter code: %s\n", authorization.VerificationURI, authorization.UserCode))
+ if authorization.VerificationURIComplete != "" {
+ opts.log(fmt.Sprintf("Direct link: %s\n", authorization.VerificationURIComplete))
</file context>
| } | ||
|
|
||
| if device { | ||
| ctx, cancel := context.WithTimeout(cmd.Context(), 16*time.Minute) |
Contributor
There was a problem hiding this comment.
P2: When the device endpoint grants an authorization lifetime longer than 16 minutes, this command cancels LoginDevice while the user code is still valid. Let the server-provided expiry and the caller's cancellation control the wait instead of imposing a shorter fixed deadline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cmd/auth.go, line 100:
<comment>When the device endpoint grants an authorization lifetime longer than 16 minutes, this command cancels `LoginDevice` while the user code is still valid. Let the server-provided expiry and the caller's cancellation control the wait instead of imposing a shorter fixed deadline.</comment>
<file context>
@@ -85,6 +96,16 @@ Use --token or --cookie for non-interactive login.`,
}
+ if device {
+ ctx, cancel := context.WithTimeout(cmd.Context(), 16*time.Minute)
+ defer cancel()
+ if err := authMgr.LoginDevice(ctx, auth.DeviceLoginOptions{}); err != nil {
</file context>
Suggested change
| ctx, cancel := context.WithTimeout(cmd.Context(), 16*time.Minute) | |
| ctx, cancel := context.WithCancel(cmd.Context()) |
Comment on lines
+73
to
+79
| for _, active := range []bool{token != "", cookie != "", noBrowser, device} { | ||
| if active { | ||
| selected++ | ||
| } | ||
| } | ||
| if selected > 1 { | ||
| return apierr.ErrUsage("choose only one of --device, --no-browser, --token, or --cookie") |
Contributor
There was a problem hiding this comment.
P2: The new conflict check counts --no-browser as a login method, so hey auth login --token X --no-browser and --cookie C --no-browser — both valid before this change, where the token/cookie branch ran and --no-browser was ignored — now fail with a usage error. --no-browser only modifies the browser flow; it is not a method. Count only token/cookie/device as methods, and reject --device --no-browser separately so the pinned test still passes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cmd/auth.go, line 73:
<comment>The new conflict check counts `--no-browser` as a login method, so `hey auth login --token X --no-browser` and `--cookie C --no-browser` — both valid before this change, where the token/cookie branch ran and `--no-browser` was ignored — now fail with a usage error. `--no-browser` only modifies the browser flow; it is not a method. Count only token/cookie/device as methods, and reject `--device --no-browser` separately so the pinned test still passes.</comment>
<file context>
@@ -59,14 +60,24 @@ func buildLoginCommand(path string) *cobra.Command {
}, "\n"),
RunE: func(cmd *cobra.Command, args []string) error {
+ selected := 0
+ for _, active := range []bool{token != "", cookie != "", noBrowser, device} {
+ if active {
+ selected++
</file context>
Suggested change
| for _, active := range []bool{token != "", cookie != "", noBrowser, device} { | |
| if active { | |
| selected++ | |
| } | |
| } | |
| if selected > 1 { | |
| return apierr.ErrUsage("choose only one of --device, --no-browser, --token, or --cookie") | |
| selected := 0 | |
| for _, active := range []bool{token != "", cookie != "", device} { | |
| if active { | |
| selected++ | |
| } | |
| } | |
| if selected > 1 { | |
| return apierr.ErrUsage("choose only one of --device, --token, or --cookie") | |
| } | |
| if device && noBrowser { | |
| return apierr.ErrUsage("--no-browser cannot be combined with --device") | |
| } |
| _, _ = io.WriteString(w, `{"device_code":"device-secret","user_code":"ABCD-EFGH","verification_uri":"https://example.test/device","verification_uri_complete":"https://example.test/device?code=ABCD-EFGH","expires_in":600,"interval":5}`) | ||
| case "/token": | ||
| polls++ | ||
| if r.Form.Get("grant_type") != "urn:ietf:params:oauth:grant-type:device_code" || r.Form.Get("device_code") != "device-secret" { |
Contributor
There was a problem hiding this comment.
P3: The new /token and /device handlers never assert the request method or Content-Type, and the /token case omits the client_id/install_id checks, unlike the sibling TestExchangeCodeRequest and TestRefreshOAuthTokenRequest which assert both method, encoding, and every form field. As written, the test passes even if the client used the wrong method or encoding, or dropped client_id/install_id on the token exchange, so it does not fully cover the PR's stated request-shapes intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/oauth_test.go, line 108:
<comment>The new /token and /device handlers never assert the request method or Content-Type, and the /token case omits the client_id/install_id checks, unlike the sibling TestExchangeCodeRequest and TestRefreshOAuthTokenRequest which assert both method, encoding, and every form field. As written, the test passes even if the client used the wrong method or encoding, or dropped client_id/install_id on the token exchange, so it does not fully cover the PR's stated request-shapes intent.</comment>
<file context>
@@ -91,6 +91,51 @@ func TestRefreshOAuthTokenRequest(t *testing.T) {
+ _, _ = io.WriteString(w, `{"device_code":"device-secret","user_code":"ABCD-EFGH","verification_uri":"https://example.test/device","verification_uri_complete":"https://example.test/device?code=ABCD-EFGH","expires_in":600,"interval":5}`)
+ case "/token":
+ polls++
+ if r.Form.Get("grant_type") != "urn:ietf:params:oauth:grant-type:device_code" || r.Form.Get("device_code") != "device-secret" {
+ t.Errorf("token form = %v", r.Form)
+ }
</file context>
| Logger func(msg string) | ||
| } | ||
|
|
||
| func (o DeviceLoginOptions) log(msg string) { |
Contributor
There was a problem hiding this comment.
P3: DeviceLoginOptions.log is an exact copy of the existing LoginOptions.log (same Logger-or-stderr routing). Extract the shared routing into one helper and have both option types call it, so the two login flows can't drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/auth.go, line 171:
<comment>`DeviceLoginOptions.log` is an exact copy of the existing `LoginOptions.log` (same Logger-or-stderr routing). Extract the shared routing into one helper and have both option types call it, so the two login flows can't drift.</comment>
<file context>
@@ -150,6 +163,19 @@ type LoginOptions struct {
+ Logger func(msg string)
+}
+
+func (o DeviceLoginOptions) log(msg string) {
+ if o.Logger != nil {
+ o.Logger(msg)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hey auth login --deviceand the matchinghey login --deviceshortcutslow_downbackoff, expiry, cancellation, denial, and refresh-token persistenceCloses #381.
Server dependency
This PR implements and tests only the CLI side. It expects HEY to provide:
POST /oauth/device_authorizationsverification_uripage for user code entry/approvalPOST /oauth/tokensUntil those server/web pieces exist,
--devicewill return the device-authorization endpoint error. Endpoint names and response details can be adjusted to match the HEY implementation during review.Verification
make checkgo test ./internal/auth ./internal/cmdmake fmt-checkmake lintgit diff --checkSummary by cubic
Adds
hey auth login --device(and thehey login --deviceshortcut) for signing in on headless machines using the OAuth device authorization grant (RFC 8628). Users see a short user code to enter at a verification URL; the secret device code is never displayed.slow_down,expired_token, andaccess_denied; success saves access and refresh tokens.--devicewith--token,--cookie, or--no-browsernow fails with a usage error.POST /oauth/device_authorizationsand device-code grant support atPOST /oauth/tokens; until those exist,--devicereturns the endpoint error. Closes Support OAuth device authorization for headless CLI sign-in #381.Written for commit 98a1587. Summary will update on new commits.