Skip to content

auth: write JSON credential format and migrate legacy plaintext#155

Merged
jrusso1020 merged 1 commit into
mainfrom
05-27-auth_write_json_and_migrate_legacy
May 28, 2026
Merged

auth: write JSON credential format and migrate legacy plaintext#155
jrusso1020 merged 1 commit into
mainfrom
05-27-auth_write_json_and_migrate_legacy

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

Description

Completes the credential-format round-trip started in #154. That PR
taught heygen-cli to read the new JSON format; this one makes it
write JSON too and auto-upgrade a legacy plaintext file in place.

Stacked on #154 (base = 05-26-auth_read_json_credential_format).

What changes:

  • FileCredentialStore.Save now writes { "api_key": "..." } (JSON,
    mode 0600, atomic temp+rename) instead of single-line plaintext. It
    reads the existing file first and preserves a co-located oauth
    block
    , so heygen auth login no longer clobbers an active OAuth
    session that hyperframes-CLI wrote. (Previously a plaintext overwrite
    would silently drop the oauth data.)
  • FileCredentialResolver.Resolve upgrades a legacy plaintext file to
    JSON on read
    , best-effort: it never changes the value returned and
    never fails the read if the write can't happen (read-only FS,
    concurrent writer). A read-only environment just leaves the file
    plaintext until the next successful write.
  • Shared file-format helpers — loadCredentialsFile (parse) and
    writeCredentialsFile (atomic 0600 write) — extracted to a new
    credentials_file.go so the store and resolver agree on one format.

Net effect: both CLIs now read and write the same
~/.heygen/credentials JSON file, and existing plaintext files migrate
forward automatically the first time either tool touches them.

Why a separate PR: #154 was scoped to read-side compat; this is a
write-side behavior change (with the OAuth-preservation data-safety
fix), so it gets its own focused review per our one-PR-per-scope
convention.

Testing

  • go test ./internal/auth/... green: new tests cover JSON write,
    OAuth-block preservation on api-key save, legacy→JSON upgrade on save,
    and legacy→JSON migration on read (with a re-resolve from the upgraded
    file).
  • Updated cmd/heygen/auth_test.go and internal/auth/file_store_test.go
    assertions from plaintext to JSON.
  • go vet + gofmt clean. Full go test ./... green.

Note: a few cmd/heygen tests (TestVideoList_AuthMissing,
TestVideoDownload_AuthRequired, TestAuthStatus_NoKey,
TestEnrichAuthHint_ExistingHint_Preserved) fail only when
HEYGEN_API_KEY is exported in the shell — they assume no ambient key.
They pass with the var unset and are unaffected by this change (they
fail the same way on main).

@jrusso1020 jrusso1020 force-pushed the 05-27-auth_write_json_and_migrate_legacy branch from 277c06a to 8c47844 Compare May 27, 2026 03:22
Copy link
Copy Markdown
Collaborator

@somanshreddy somanshreddy left a comment

Choose a reason for hiding this comment

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

Two review notes:

  1. Blocking: preserving the existing oauth block means heygen auth login can save a new API key but subsequent commands may still select the preserved OAuth access token first. The resolver prefers oauth.access_token over api_key, but the client still treats the resolved string as an API key and always sends it as x-api-key (internal/client/client.go:78). So a credentials file with both a valid API key and a fresh OAuth token will select the OAuth token, then send it in the API-key header instead of Authorization: Bearer ....

Unless the API explicitly accepts OAuth access tokens in x-api-key, this breaks auth for the shared JSON format. Until the HTTP client supports typed credentials/Bearer auth, heygen-cli should prefer api_key here or avoid preserving/selecting OAuth in a way that changes the effective credential.

  1. Non-blocking compatibility concern: FileCredentialResolver.Resolve() migrates legacy plaintext credentials to JSON as a side effect of any authenticated command, not only when the user explicitly updates auth state.

Example scenario:

  1. User has an older heygen-cli installed, with ~/.heygen/credentials containing plaintext: hg_abc123.
  2. User runs this PR's CLI once, e.g. heygen video list.
  3. Resolve() reads the plaintext key successfully, but also rewrites the file to { "api_key": "hg_abc123" }.
  4. User later rolls back to the previous heygen-cli version, runs an older binary from a script/CI image, or otherwise uses a client that only understands plaintext.
  5. That older client reads the whole JSON document as the API key and sends it as x-api-key, so auth fails.

The new CLI can already read plaintext without migrating it, so read-time migration is not required for compatibility. If one-way migration is acceptable, this is fine, but it is worth making explicit. Otherwise, consider migrating only on explicit writes like heygen auth login, or behind a deliberate upgrade path.

@jrusso1020 jrusso1020 force-pushed the 05-27-auth_write_json_and_migrate_legacy branch 2 times, most recently from 112b17e to a4f902f Compare May 28, 2026 00:32
@jrusso1020
Copy link
Copy Markdown
Collaborator Author

Thanks @somanshreddy — both addressed. Force-pushed (signed/verified).

1. Blocking — OAuth token mis-sent as x-api-key. You're right; this was a real break. heygen-cli's client only sends x-api-key (no Bearer), so selecting an OAuth access_token would transmit it in the wrong header. Fixed in selectCredential:

  • Always prefer api_key — it's the only credential heygen-cli can actually send.
  • An OAuth-only file is now refused with an actionable error ("…holds an OAuth session, which heygen-cli can't use yet; run heygen auth login with an API key or set HEYGEN_API_KEY") instead of returning the token to be mis-sent.
  • The OAuth block is still parsed and preserved on write (so we don't clobber a hyperframes session) — just never selected. Dropped the now-unused expiry machinery (isOAuthExpired, skew const) since heygen-cli never acts on it.
  • Tests updated: OAuth-only (with/without expiry) → asserts a usable non-ErrNotConfigured error; api_key+oauth → asserts api_key wins.

2. Non-blocking — read-time migration rollback risk. Good catch on the mixed-version/rollback scenario. Switched to write-only migration: Resolve() is now side-effect-free and leaves a legacy plaintext file untouched; the upgrade to JSON happens only on an explicit write (heygen auth loginSave). A re-Resolve no longer rewrites the file. Added a regression asserting the read leaves the plaintext file byte-for-byte unchanged.

Also from a self-review pass on this PR: Save now refuses to overwrite a present-but-unparseable file (returns an error instead of silently discarding a possibly-recoverable OAuth block), and the empty-file resolver test now asserts the error is not ErrNotConfigured (so the chain surfaces it).

All internal/auth + cmd/heygen tests green, golangci-lint clean.

Copy link
Copy Markdown
Collaborator

@somanshreddy somanshreddy left a comment

Choose a reason for hiding this comment

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

Looks good after the updates. The final stacked diff now keeps Resolve() aligned with the current HTTP client behavior: api_key is the only selected credential, OAuth-only files fail with an actionable error, and preserved OAuth blocks are only round-tripped for the other CLI. The legacy plaintext read path is also side-effect-free now, with JSON migration limited to explicit writes.

Verified locally:

  • go test ./internal/auth/... ./internal/client/... ./cmd/heygen
  • go test ./...

Base automatically changed from 05-26-auth_read_json_credential_format to main May 28, 2026 20:57
heygen-cli now writes the shared ~/.heygen/credentials file in the JSON
format (matching hyperframes-CLI) instead of single-line plaintext, and
upgrades a legacy plaintext file to JSON in place on read.

- FileCredentialStore.Save writes `{ "api_key": "..." }` and preserves a
  co-located oauth block, so saving an API key no longer clobbers an
  active OAuth session written by hyperframes-CLI.
- FileCredentialResolver upgrades a legacy plaintext file to JSON on
  read (best-effort: never changes the returned value, never fails the
  read on a read-only FS / concurrent writer).
- Shared file-format helpers (parse + atomic 0600 write) extracted to
  credentials_file.go so the store and resolver agree on the format.

Read-side compat from the parent PR is unchanged; this completes the
round-trip so both CLIs read AND write the same format.
@jrusso1020 jrusso1020 force-pushed the 05-27-auth_write_json_and_migrate_legacy branch from a4f902f to a68e4b2 Compare May 28, 2026 20:58
@jrusso1020 jrusso1020 merged commit 382558e into main May 28, 2026
9 checks passed
@jrusso1020 jrusso1020 deleted the 05-27-auth_write_json_and_migrate_legacy branch May 28, 2026 21:11
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