Skip to content

fix(cli): store finite dx-id credentials - #133

Open
GregHolmes wants to merge 2 commits into
mainfrom
gh/store-finite-dx-id-token
Open

GregHolmes wants to merge 2 commits into
mainfrom
gh/store-finite-dx-id-token

Conversation

@GregHolmes

Copy link
Copy Markdown
Contributor

Summary

  • Store dg_token returned once by the Phase 0 device flow as the CLI API key.
  • Persist dg_expires_in as the direct credential expiry.
  • Remove legacy JWT and refresh credentials from the keyring when storing the finite key.
  • Request standard OIDC scopes instead of admin during device authorization.

Validation

  • uv run pytest packages/deepctl-core/tests/unit/test_auth.py

@dg-coreylweathers dg-coreylweathers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_password for the old jwt.* and refresh-token.* entries sits in the same try as set_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 PasswordDeleteError when the entry doesn't exist, which is every first login. The except then 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 (ignore PasswordDeleteError) and keep the file fallback only for a failed set_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_token and no dg_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_token comes back, store nothing and print "Run dg update and dg login again". Minimal: release only alongside the server change.

Should-fix

  • [S1] An expired key gives a bare 401. Expected: a "run dg login" prompt once dg_token_expires_at has passed. Observed: get_api_key checks 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: pass at auth.py:188 swallows 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 a BREAKING 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 dg-coreylweathers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 login shows the user the code to enter and opens the verification page.
    • Observed: the server now returns a user_code alongside device_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 prints user_code, so the user lands on "Enter the code" with nothing to enter.
    • Fix: print user_code prominently (for example Enter code: ABCD-EFGH) and open the plain verification_uri returned by /device/code. Add a test asserting both.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants