feat(auth): opt-in OAuth2 authorization-code + PKCE login - #892
Draft
elatt wants to merge 4 commits into
Draft
Conversation
Deployments that front their own authorization server — a self-hosted inference stack running Ory Hydra, say — have no /account/developer-tools handler, and the existing flow's ?key=<token> hand-off puts the credential in a URL, where it lands in browser history. This adds a standard authorization-code flow with PKCE: the CLI holds the verifier, receives only a code on the loopback redirect, and exchanges it over POST, so the token arrives in a response body. It also gives the callback a real `state` to check, which the hand-off cannot provide. Off by default. `dr auth login` is unchanged unless asked, and issues no discovery request at all — deliberately, because some deployments serve an OIDC discovery document without a login service behind it, so the document's presence cannot be treated as evidence the flow works. Enable per-shell with DATAROBOT_OAUTH_ENABLED=true, or per-invocation with --oauth; --no-oauth forces the hand-off back on. Asked for but undiscoverable is an error, not a silent downgrade to a different kind of credential. Wait() still returns "the credential" in both modes, so callers need no changes. The OAuth branch in handleCallback is evaluated BEFORE the keyless-request check: an empty `key` is the port-reclaim interrupt sentinel, so an OAuth callback reaching it would abort the login rather than complete it. Callback failures travel on their own channel for the same reason — publishing an empty string would read as an interruption instead of an error. The redirect URI takes its hostname from the configured callback address and its port from the bound listener. listener.Addr() resolves localhost to 127.0.0.1, and servers match redirect_uri as an exact string, so the IP literal gets the request rejected before any login page appears. Verified against a live Hydra: discovery, PKCE authorize request, and Hydra accepting client/redirect_uri/scope and redirecting to its login provider.
|
👋 Thanks so much for contributing to the DataRobot community! As a quick heads-up on how our team handles reviews: if you're still iterating on Once everything is finalized and you're ready for feedback, just click "Ready for review" |
Code OwnershipCli Maintainers
Review requested from the teams above. Labels will be removed automatically upon approval. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2cc8b87. Configure here.
elatt
marked this pull request as draft
September 3, 2026 15:59
The OAuth flow already asked for offline_access but discarded the refresh token, so a deployment issuing short-lived access tokens sent the user through a browser every time one expired — which defeats the point of asking. The refresh token and token endpoint are now persisted with the profile, and a rejected access token is renewed in the background before falling back to an interactive login. `dr auth logout` clears both: dropping the access token while leaving a working refresh token on disk does not log anyone out. Only a JUDGED rejection triggers a renewal. A 404, 429, 5xx or transport error says nothing about the token, and spending a rotate-on-use refresh token against a server that never rejected anything would turn a transient outage into a forced re-login. The renewed token is then verified rather than trusted, so a server handing back a credential it will not accept cannot loop us silently. A rotated refresh token replaces the stored one, since servers that rotate invalidate the old value on use. A rejected refresh clears the stored material so later commands go straight to a browser instead of retrying something spent. Also drops vendor and deployment names from comments, help text and test fixtures: this describes a standard OAuth2 authorization server, and naming a particular one in a public repo is both noise and leakage.
…n tests Same behavior, ~270 fewer lines, to keep the diff reviewable. Real duplication removed rather than lines golfed: - One postToken for both grants; exchangeCode and postRefresh differed only in the form they send and what rejection means to the caller. - One newFlow builds the flow and its callback server; the two constructors differed only in the URL and the OAuth fields. - Dropped tokenResponse.TokenType and .ExpiresIn, declared and never read. - Merged the refresh tests into oauth_test.go and shared one driveFlow helper, which removes the goroutine boilerplate repeated in five tests. Tests cut as unlikely to fail or covered elsewhere: - OAuthKeysArePersistable and ClearOAuthState asserted a map literal and a two-line setter. - SurfacesFailures: the errCh-not-sentinel property is asserted by RejectsStateMismatch; error_description propagation is formatting. - KeylessCallbackStillInterrupts: paired with CodeCallbackIsNotSwallowedBySentinel, which covers the reordering from the direction that regressed. - OAuthRequested: its load-bearing case is asserted more meaningfully by GateOffIssuesNoDiscovery, which checks that no network call is made rather than a boolean. - Trimmed the status-code table to one case per branch. Kept every test that guards a bug actually hit or a property that would fail silently: the sentinel ordering, the redirect-URI hostname, strict discovery, and the judged-vs-unjudged refresh guard.
Four issues from CI and review on datarobot-oss#892. In OAuth mode handleCallback intercepted `code` and `error` and then fell through to the legacy `key` handler, so any local process could hit the loopback listener with ?key= during the login window and have that value stored as the credential — bypassing the `state` check the flow exists to provide. A key cannot authenticate this flow, so it is now refused with a 400 and published on neither channel: the login keeps waiting for the real callback, making it neither an injection nor a way to cancel someone else's login. Regression test covers both halves — the injected callback refused, the genuine one still completing. --no-oauth did not exist. The help text and a comment claimed cobra synthesises --no- variants from a bool flag; pflag does not, so the documented way to force the legacy hand-off failed as an unknown flag. Registered explicitly and marked mutually exclusive with --oauth. CI: internal/auth/oauth.go was missing the Apache header (Copyrights), and the test stub called ParseForm without bounding the body (gosec G120). Both fixed, and verified with the repo's own `task lint` and `task copyright` rather than a hand-picked package subset — linting one GOOS over selected packages is what let the gosec finding through.
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.
RATIONALE
dr auth loginonly supports the DataRobot SaaS hand-off: it opens/account/developer-tools?cliRedirect=trueand waits for that page to redirectback to
http://localhost:51164/?key=<token>. Deployments that front their ownOAuth2 authorization server have no such page, so login cannot complete at all.
This adds the standard native-app OAuth flow instead — authorization code with
PKCE and a loopback redirect, per RFC 8252.
It is the same pattern
gcloud auth login,az loginandvault login -method=oidcuse.Two benefits over the hand-off, aside from working at all:
code and exchanges it over POST, so the credential does not land in browser
history or
Refererheaders.stateparameter, where theexisting callback has neither.
Off by default; nothing changes for existing users. With the gate unset,
dr auth loginbehaves exactly as it does today and issues no discoveryrequest. That default is deliberate: some hosts serve
/.well-known/openid-configurationwithout supporting this flow, so thedocument's presence is not evidence it will work — auto-detection would commit
those users to a login that cannot finish.
CHANGES
--oauth/--no-oauthondr auth login, orDATAROBOT_OAUTH_ENABLED=truefor a shell. Flag beats env; default off.
/.well-known/openid-configuration, requiring 200 andparseable JSON with both endpoints — a single-page app answers 200 with HTML
for unknown paths and would otherwise look like success. Asked-for but
undiscoverable is an error, not a silent downgrade to the old flow.
port,
stateverified on callback.offline_accessis requested, and the refresh token andtoken endpoint are stored with the profile. A rejected access token is renewed
in the background before falling back to a browser.
dr auth logoutclearsboth — dropping the access token while leaving a working refresh token does
not log anyone out.
?key=path is untouched and remains the default.NOTES
Details worth a reviewer's eye:
handleCallbackis evaluated before the existingkeyless check, because an empty
keyis the port-reclaim interrupt sentinel —an OAuth callback reaching it would abort the login rather than complete it.
transport error says nothing about the token, and spending a rotate-on-use
refresh token against a server that never rejected anything would turn a
transient outage into a forced re-login.
its port from the bound listener:
listener.Addr()resolveslocalhostto127.0.0.1, and servers compareredirect_urias an exact string.TESTING
13 unit tests in
internal/authcovering strict discovery, PKCE/state, thecode exchange, refresh (success, rotation, rejection, nothing-stored), the
judged-vs-unjudged guard, that the gate makes no network call when unset, and
that an OAuth callback is not swallowed by the interrupt sentinel.
Also exercised end to end against a real authorization server: discovery, the
authorize request, and the server accepting the client, redirect URI, scope and
challenge.
Per the template — this is a forked PR, so the required Smoke Tests check needs
a maintainer's
/approve-smoke-testsor/skip-smoke-tests.