feat(auth): interactive browser login for auth login, dark-launched behind --web / oauth_login#57
Merged
Merged
Conversation
On a TTY, `dnsimple auth login` opens the browser and runs the authorization-code flow with PKCE (RFC 7636) and a loopback redirect (RFC 8252), storing the issued bearer token. `--with-token` and non-TTY stdin keep the manual paste path unchanged. Implements #42.
A forged GET to the loopback callback could deliver an attacker-chosen error to the in-flight login without a valid state token, aborting the legitimate flow. Validate state first, regardless of the response shape (RFC 6749 section 10.12).
With default (zero) timeouts a stalled local connection could pin the listener until the flow deadline, and an unbounded Shutdown could hang the CLI on close. Set per-request timeouts (5s headers, 10s read/write) and wrap Shutdown in a 5s context.
…jection The OAuth error code and description come from the server and were printed verbatim. Strip control bytes so a malicious value cannot inject terminal escape sequences.
A 307 or 308 from the token endpoint would have Go re-send the POST body, which carries the authorization code and PKCE verifier, to the redirect target (RFC 6749 section 10.5). Set CheckRedirect to http.ErrUseLastResponse for the token POST only, and surface a 3xx with its Location header as a clear error.
…dbox OAuthTokenURL re-derived the API host from a sandbox bool, while the rest of the client uses cfg.BaseURL. Take a base URL string and pass cfg.BaseURL at the call site so the OAuth flow always tracks the same host as the API client, including under a BaseURL override.
A non-ErrServerClosed error from the loopback Serve was dropped, so await blocked until the 5-minute flow deadline before reporting a misleading timeout. Deliver the error through the result channel so the flow fails fast with the real cause.
A single DNSIMPLE_OAUTH_CLIENT_ID applied to both production and sandbox, so a value exported for sandbox testing could silently route into a later production login and fail with an opaque invalid_client. Split into DNSIMPLE_OAUTH_CLIENT_ID_PRODUCTION and DNSIMPLE_OAUTH_CLIENT_ID_SANDBOX, trimming surrounding whitespace.
…onse RFC 6749 section 5.1 makes token_type REQUIRED. Reject a non-bearer response, matched case-insensitively per the spec, instead of storing a token that every subsequent API call would reject with an opaque 401.
…t helper The OAuth branch carried its own copy of the term.IsTerminal check, identical to isInteractiveInput in confirm.go. Delegate to the shared helper so the two stay in lockstep; the isStdinTTY var remains as a test seam.
The post-await branch reported "timed out" for any non-nil ctx error, so a cancellation would be misreported as a timeout. Use errors.Is(ctx.Err(), context.DeadlineExceeded) so deadline expiry gets the friendly message and cancellation propagates verbatim.
Login discarded the state returned by the listener and never compared it to the value it generated. The listener already validates state, but re-check it before exchanging the code so a future weakening of that check cannot slip a wrong-state code through.
…L overrides For local development against a dnsimple-app checkout, DNSIMPLE_BASE_URL overrides the API host and DNSIMPLE_OAUTH_AUTHORIZE_URL overrides the authorize endpoint. Both Resolve and the pre-auth Load/SetSandbox path apply DNSIMPLE_BASE_URL, so every call (including the OAuth token exchange and whoami) targets the local host rather than production.
The DNSimple token endpoint requires the state recorded at authorize to be echoed back; omitting it fails the exchange with invalid_request.
…oken paste On a browser-login failure, acquireToken returns the error and tells the user to retry or pass --with-token, rather than dropping to a token paste prompt. Context cancellation and an unprovisioned build are reported the same way, and no context is stored when login fails.
The previous "Created context ... and set as active" line read like an internal status update. Lead with a plain success confirmation and the signed-in identity, then state which context is now active.
The interactive OAuth browser flow now ships off by default. On a terminal, `auth login` prompts for a pasted API token (the classic behaviour); the browser flow is opt-in per command with --web or persistently via the oauth_login config setting (DNSIMPLE_OAUTH_LOGIN). acquireToken takes a useOAuth argument (web || cfg.OAuthLogin); when it is false on a TTY it falls back to the token prompt instead of starting OAuth. --with-token still wins, non-TTY stdin is unchanged, and the not-provisioned error is preserved for the OAuth path. Flip the oauth_login default to true to roll the browser flow out to everyone.
…reToken The non-TTY and OAuth-disabled cases both fall back to the same pasted-token prompt, so merge them into a single switch case.
The only test reaching the OAuth branch through `auth login` opted in via --web, leaving the `cfg.OAuthLogin` half of `useOAuth := web || cfg.OAuthLogin` unguarded. Add a RunE-level test that enables the browser flow purely through the persistent config setting, so dropping the operand would fail a test.
Passing --web alongside --with-token, or on a non-interactive stdin, silently fell back to reading a pasted token with no feedback. Emit a warning in those cases. The check keys off the actual flag value so `--web=false` stays silent, and it ignores the persistent oauth_login toggle, which is meant to fall back to the prompt without noise on every non-interactive run.
The six-line comment narrated the whole rollout strategy and noted "reads via viper", which no sibling field documents. Collapse it; the rollout story lives in the CHANGELOG and README.
Drop the dark-launch / default-value narrative, which belongs to the caller that computes useOAuth (auth.go) and is already covered by tests; keep the description of what the function does.
Populate the public OAuth client identifiers so `auth login --web` (and the oauth_login toggle) start the browser flow without the per-environment env overrides. Public client IDs are not secrets, so embedding them in the binary is the intended provisioning step.
Reword the local-development comments to say "local API server" instead of naming the internal dnsimple-app repo. No behavior change.
Reference the PR and call out the OAuth/PKCE mechanism on the existing Unreleased entry.
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
1 participant
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.
Adds an interactive, browser-based login to
dnsimple auth loginand ships it as a dark launch so it can be rolled out gradually.auth loginusing OAuth 2.0 with PKCE and an RFC 8252 loopback redirect (a public client, no secret). On success the CLI stores the resulting credential as a named context, exactly like the token flow.auth loginkeeps prompting for a pasted API token. Users opt into the browser flow per command with--web, or persistently withoauth_login: truein the config file (orDNSIMPLE_OAUTH_LOGIN=1).--webemits a warning when it cannot be honored (combined with--with-token, or on a non-interactive stdin).📋 Deployment Pre/Post tasks
internal/config/oauth.go. Until then, the browser flow returns "not available in this build".dnsimple auth login(no flags) shows the "Paste your API token:" prompt and does not open a browser.DNSIMPLE_OAUTH_LOGIN=1 dnsimple auth loginruns the browser flow without--web.