Repository navigation
Device code auth - #7093
Device code auth#7093maksim-zuev-db wants to merge 24 commits into
Conversation
Integration test reportCommit: 419aeb6
Top 3 slowest tests (at least 2 minutes):
|
fd59775 to
34ed39b
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The server answers a rate-limited device code token request with invalid_request too, but with HTTP 429. Entering the confirmation code again cannot fix that, so end the login for any error other than HTTP 400 with invalid_request. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The device code host check uses it to reject account and unified hosts, not to decide whether to prompt. Keep needsAccountIDPrompt as a local variable where setHostAndAccountId decides to prompt. Co-authored-by: Isaac <no-reply@databricks.com>
CheckDeviceCodeSupported fetched the OIDC endpoints with its own client and repeated the check DeviceCodeChallenge makes. Make it a PersistentAuth method that runs the same deviceCodeConfig as DeviceCodeChallenge, so the up-front check and the login cannot disagree. Co-authored-by: Isaac <no-reply@databricks.com>
Group the cases so that a failing test name points at the broken step: preconditions, the successful login (with a host, from a saved profile and with --scopes), unsupported hosts, an unallowed client, and missing or wrong confirmation codes. Only the nominal test prints the device code requests, because the requested scope is what it checks; the unsupported host test checks that no device authorization request is made. Co-authored-by: Isaac <no-reply@databricks.com>
751bb77 to
cbd3d8f
Compare
|
Commit: Regression test reportSelected 5 changed tests (limit=20, 0 not selected) 5 tests: regression=5 unreleased=0 coverage=0 failing=0
TestAccept/cmd/auth/login/device-code/client-not-allowed: branch ✅ | main (edebd84) ❌ | latest (v1.22.1) ❌main (edebd84): latest (v1.22.1): same output as main (edebd84) TestAccept/cmd/auth/login/device-code/nominal: branch ✅ | main (edebd84) ❌ | latest (v1.22.1) ❌main (edebd84): latest (v1.22.1): TestAccept/cmd/auth/login/device-code/precondition: branch ✅ | main (edebd84) ❌ | latest (v1.22.1) ❌main (edebd84): latest (v1.22.1): same output as main (edebd84) TestAccept/cmd/auth/login/device-code/unsupported-host: branch ✅ | main (edebd84) ❌ | latest (v1.22.1) ❌main (edebd84): latest (v1.22.1): same output as main (edebd84) TestAccept/cmd/auth/login/device-code/wrong-code: branch ✅ | main (edebd84) ❌ | latest (v1.22.1) ❌main (edebd84): latest (v1.22.1): |
The prompt's handling of a timed-out context is covered by unit tests, and keeping stdin open in the acceptance test cost 5 seconds per run. Co-authored-by: Isaac <no-reply@databricks.com>
8e66f05 to
556ea93
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
Main things I'd look at first:
- Invalid device code errors get retried as if the confirmation code was wrong (
device_code.goL117). DeviceAuthdoesn't carry the login context, so--timeoutand Ctrl-C don't stop a stalled request (L92).intervalis ignored, which can hit the per device code 429 and end an approved login (L102).
The rest are smaller correctness gaps, a few convention points (error wrapping, duplicated error parsing, shared test setup) and cleanups. No new dependencies, and skipping a .nextchanges entry makes sense to me while the flag is hidden and env gated.
Take the context as an argument instead of using the one stored in PersistentAuth, so that callers control the deadline and cancellation of the device code login. Co-authored-by: Isaac <no-reply@databricks.com>
oauth2.Config.DeviceAuth only uses the context to pick the HTTP client and builds the request without it, so --timeout and Ctrl-C did not stop a stalled device authorization request. Make the request directly, sharing the request and error handling with the token exchange, and keep parsing the response with oauth2.DeviceAuthResponse. Co-authored-by: Isaac <no-reply@databricks.com>
PersistentAuth only sends resource indicators on the browser authorization request, so a device code login with WithResources would silently return a token that is not scoped to them. The CLI already rejects --resource and resource-scoped profiles; reject it in the library too. Co-authored-by: Isaac <no-reply@databricks.com>
The arguments never carry a discovery URL before setHostAndAccountId runs, so the first isAccountOrUnifiedHost call only checked for a classic account host. Check for a classic account host, run host discovery, and check the discovered URL for a unified host. Co-authored-by: Isaac <no-reply@databricks.com>
The device code host check no longer calls it, so it is only used again to decide whether setHostAndAccountId prompts for an account ID. Co-authored-by: Isaac <no-reply@databricks.com>
Host discovery skips the discovery URL of a unified host whose metadata
has an {account_id} placeholder when no account ID is known. The device
code host check dropped the ?a= account ID from the host, so it treated
such a host as a workspace, while setHostAndAccountId later resolved it
as unified.
Co-authored-by: Isaac <no-reply@databricks.com>
Move the shared DATABRICKS_CLI_DEVICE_CODE setting and the home ignore into a parent test.toml. The precondition test unsets the variable for the case that checks the feature is disabled by default. Co-authored-by: Isaac <no-reply@databricks.com>
The workspace answers an unknown device code with HTTP 400 invalid_request, like a wrong confirmation code, so the CLI asks for the code again and the error never has the "token exchange:" prefix. Also document the error for too many token requests. Co-authored-by: Isaac <no-reply@databricks.com>
A blank line asked for the confirmation code again without limit, so anything that keeps sending newlines kept the login waiting until the timeout. Send it like any other code instead: the workspace rejects an empty confirmation code, and it counts as one of the 3 attempts. Co-authored-by: Isaac <no-reply@databricks.com>
The host was checked separately for every place it can come from: the flags, the profile picker and the profile. Split setHostAndAccountId into setHost and setAccountID and check the resolved host once in between, before setAccountID prompts for an account ID for account and unified hosts. This reuses the host discovery of setHost instead of running it again. A workspace without device code login enabled is now rejected by DeviceCodeChallenge, which runs without a prompt in between and fails before creating a device code, so drop CheckDeviceCodeSupported. An unsupported host given with --host now fails after the profile name prompt instead of before it. Co-authored-by: Isaac <no-reply@databricks.com>
oauthError rebuilt the error with %s, so errors.As could no longer find the *httpclient.HttpError or *oauth2.RetrieveError behind an error from DeviceCodeChallenge. Return an error type that shows the same message and unwraps to the original error. refresh() parsed the same RFC 6749 error response inline, with the same message format and the same %s issue. Use the shared helpers there too, and move them to error.go now that both login flows and the refresh use them. An error response that is not JSON now returns the original error instead of the JSON parsing error. Co-authored-by: Isaac <no-reply@databricks.com>
The workspace answers an unknown device code, e.g. one it discarded after the login expired, with HTTP 400 and invalid_request like a wrong confirmation code. Entering the code again cannot succeed, so fail right away. Only the error description tells the two apart; if it changes, the CLI falls back to asking for the code again. Co-authored-by: Isaac <no-reply@databricks.com>
simonfaltum
left a comment
There was a problem hiding this comment.
Looks good to me, approving. The earlier threads are addressed, and the replies on the ones still open make sense to me (interval, keeping the single-shot token request, echo and EOF on the prompt), so feel free to resolve them.
I left a few small comments inline. None of them are blockers, but please take a look at them before merging.
Pin what refresh() returns through the default ApiClient transport for an error response that is not JSON or has no error field. Before the error handling was shared, these gave a JSON parsing error and an empty "(error code: )" message. Co-authored-by: Isaac <no-reply@databricks.com>
A 200 response without verification_uri or user_code made the CLI print empty instructions and then wait for a confirmation code the user had no way to get, and a missing device_code was sent as device_code=. Fail before asking for the confirmation code if any of the three is empty. Co-authored-by: Isaac <no-reply@databricks.com>
validateDeviceCodeHost repeated the check in needsAccountIDPrompt. Move it into isAccountOrUnifiedHost and call it from both, so that the two cannot drift apart. Co-authored-by: Isaac <no-reply@databricks.com>
Changes
Implementation of device code auth flow. The backend implementation is still not finalized in production, so this change introduces
--use-device-codeflag as experimental, hidden underDATABRICKS_CLI_DEVICE_CODEenv variable.Why
The Device Code flow enables users to authenticate with the Databricks CLI on machines that don't have browser access (e.g. headless servers, SSH sessions, web terminals). Instead of the standard browser-redirect OAuth flow, the user authenticates on a separate device (e.g. their phone or laptop) by entering a short code.
Tests
auth loginhelpers, includingedge cases like expiry, retries and cancellation.
libs/testserver, which nowmodels Databricks' device code flow. Confirmation codes are piped through
stdin. The tests cover the successful login and each error scenario.