Repository navigation
fix(cli): store finite dx-id credentials - #133
GregHolmes wants to merge 2 commits into
Conversation
dg-coreylweathers
left a comment
There was a problem hiding this comment.
What this PR does: dg login stores the Deepgram API key that the device sign-in flow now returns, along with its expiry, and stops storing a short-lived token and refresh token.
Blocking
- [B1] The API key is written to a world-readable config file on a normal login.
keyring.delete_passwordfor the oldjwt.*andrefresh-token.*entries sits in the sametryasset_password(packages/deepctl-core/src/deepctl_core/auth.py:651-664).- Expected: the key is stored only in the system keyring.
- Observed: the macOS, Windows and SecretService/libsecret backends raise
PasswordDeleteErrorwhen the entry doesn't exist, which is every first login. Theexceptthen also writes the key to~/.config/deepctl/config.yaml(0644). The user sees "Could not store in keyring" and then "stored securely in system keyring". - Reproduced in Docker with a real keyring backend. The unit tests miss it because the mocked keyring never raises.
- Fix: give the deletes their own
try/except(ignorePasswordDeleteError) and keep the file fallback only for a failedset_password.
- [B2] The new CLI doesn't work with the current sign-in server.
- Expected: the CLI either stores a usable API key or tells the user what to do.
- Observed: against a server that still returns
refresh_tokenand nodg_token, the legacy branch (auth.py:681-697) stores the 15-minute JWT as the API key, and requests start failing a few minutes later. - Fix (lets the CLI ship first): if no
dg_tokencomes back, store nothing and print "Rundg updateanddg loginagain". Minimal: release only alongside the server change.
Should-fix
- [S1] An expired key gives a bare 401. Expected: a "run
dg login" prompt oncedg_token_expires_athas passed. Observed:get_api_keychecks the expiry only on the JWT path. - [S2] Upgraders with an old session get no message. Expected: a prompt to log in again. Observed: the refresh now returns 400, the
except: passatauth.py:188swallows it, and the expired cached key is used silently. The refresh and exchange code is dead and can go. - [S3] Release notes hide the change.
fix(cli):releases 0.3.4 under Bug Fixes. Because existing sessions need a re-login,fix(login)!:with aBREAKING CHANGE:footer gives 0.4.0 (bump-minor-pre-major).
Nits: device login drops a custom base_url; error copy names "dx-id" where users know "Deepgram sign-in"; the success message says "Finite API key"; the branch is one commit behind main; drop "Phase 0" from the PR body.
Gates (Docker, python 3.12 + uv): ruff format and check clean, mypy clean (121 files), 2207 passed / 12 skipped, build OK. A live dg login against the updated sign-in server stored dg_token, the 30-day expiry and project_id, and handled the authorization_pending poll.
Question: device-login keys get only usage:write and usage:read. Is that intended? If so, dg projects, dg keys, dg members and dg billing will return 403 for these users (not tested live).
dg-coreylweathers
left a comment
There was a problem hiding this comment.
Addendum: the device sign-in server now requires a typed code. This is another blocker to add to my review above.
- [B3] CLI login can't complete against the updated sign-in server.
- Expected:
dg loginshows the user the code to enter and opens the verification page. - Observed: the server now returns a
user_codealongsidedevice_code, and the verification page asks the user to type it. This PR still opens the old link built from the device code and never printsuser_code, so the user lands on "Enter the code" with nothing to enter. - Fix: print
user_codeprominently (for exampleEnter code: ABCD-EFGH) and open the plainverification_urireturned by/device/code. Add a test asserting both.
- Expected:
The response also carries per-client lifetimes now (one day for the CLI), not 30 days, so read dg_expires_in as returned and adjust the "30-day" copy if any remains.
Summary
dg_tokenreturned once by the Phase 0 device flow as the CLI API key.dg_expires_inas the direct credential expiry.adminduring device authorization.Validation
uv run pytest packages/deepctl-core/tests/unit/test_auth.py