feat: Thingiverse v2 auth/token manager

[devonjones]

Summary

First code PR of the Thingiverse publish/sync epic (openforge_catalog-4kx), implementing ticket openforge_catalog-7dg.

• openforge/thingiverse/auth.py — TokenManager:
  • login() via POST /v2/auth/login (the SPA's own flow — no OAuth redirect needed); raises TwoFactorRequired on HTTP 202, completed by login_2fa() on the same session (the 2FA exchange is cookie-bound)
  • Persists {access, refresh} JWTs to ~/.config/openforge/thingiverse_tokens.json (0600, dir 0700, overridable via THINGIVERSE_TOKEN_FILE); passwords are never stored
  • access_token() checks the JWT exp claim (60s leeway, undecodable = expired) and transparently rotates via POST /v2/auth/refresh; rejected refresh → NotLoggedIn with a clear re-login message
  • whoami() (GET /v2/users/me) to verify which account the tokens belong to — important given the v1 app token turned out to be scoped to the wrong account
• bin/tv_auth — click CLI: login (prompted credentials, 2FA prompt when needed), status (verifies tokens against the live API), logout. -v/-q verbosity ladder, diagnostics to stderr.
• tests/test_thingiverse_auth.py — 25 tests; fake session injected at our boundary (per the pack's mocking discipline), fake JWTs with real exp claims, token-file permission assertions, password-never-in-errors assertion.
• docs/thingiverse-api-v2.md — v2 API reference extracted from the official OpenAPI spec (71 operations, 43 schemas), documenting the auth flows this implements and the read surface later tickets use.

Test plan

• pytest tests/test_thingiverse_auth.py — 25/25 green
• ruff check . clean; full suite run (4 pre-existing test_incremental.py failures reproduce on clean test locally; CI is green there)
• Live verification: Devon runs bin/tv_auth login + bin/tv_auth status to confirm a Devon-scoped JWT round-trips against the real API

Beads: openforge_catalog-7dg (closed via bd close on merge). Unblocks gn2 (API client) together with the HAR ticket hnr.

🤖 Generated with Claude Code

[devonjones]

Code Review (Claude)

This review focuses on auth-logic correctness, API contract fidelity against
docs/thingiverse-api-v2.md, and house style. Complexity, error-handling
depth, test coverage, resource leaks, dead code, logging, and credentials
hygiene are covered by other reviewers and are intentionally not duplicated
here.

Critical Issues

1. bin/tv_auth is committed non-executable, breaking the documented usage.

$ git ls-files -s bin/tv_auth bin/db_update
100644 ... bin/tv_auth   <- no exec bit
100755 ... bin/db_update <- exec bit (every other bin/ tool)

Every other script in bin/ is mode 100755. tv_auth is 100644, so
bin/tv_auth login (as documented in the PR body and the module docstring)
will fail with "Permission denied" on a fresh checkout — it only works via
python bin/tv_auth .... This directly violates the Unix-tooling convention
in openforge/CLAUDE.md ("Command-line tools go in bin/ directory... use
standard Unix conventions"). Fix: chmod +x bin/tv_auth && git add --chmod=+x bin/tv_auth (or git update-index --chmod=+x bin/tv_auth) before
merge.

2. _jwt_exp crashes (uncaught AttributeError) on a structurally-valid
JWT whose payload segment decodes to non-object JSON, contradicting its own
contract and the "undecodable = expired" fail-safe design.

openforge/thingiverse/auth.py:52-70:

payload = json.loads(base64.urlsafe_b64decode(payload_b64))
exp = payload.get("exp")

only IndexError, ValueError, binascii.Error, json.JSONDecodeError are
caught. If the payload segment is valid JSON but not a dict (e.g. a JSON
array, string, or number — plausible for a hand-edited/corrupted token file,
or an API/format change), payload.get raises AttributeError, which is
not caught and propagates out of _token_expired() → access_token(),
crashing the whole auth path instead of the documented "can't be decoded ->
None -> treated as expired" behavior:

>>> _jwt_exp(f"{seg({'alg':'none'})}.{seg([1,2,3])}.sig")
AttributeError: 'list' object has no attribute 'get'

Since the token file (~/.config/openforge/thingiverse_tokens.json) is
user-editable, untrusted, unsigned input, this fail-safe gap is a real risk,
not just theoretical. Fix: add AttributeError to the except tuple (or
isinstance(payload, dict) check before .get).

Medium Issues

3. Refresh-token rotation is neither atomic nor crash-safe, and the file
has no cross-process locking — two concerns that compound.

_store_tokens() (auth.py:265-272) opens the target file directly with
O_TRUNC and writes in place:

fd = os.open(self.token_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)

If the process dies (OOM-kill, Ctrl-C, laptop sleep) between the server
accepting the refresh call (which very likely invalidates/rotates the old
refresh token server-side) and this write completing, the on-disk file is
left empty or truncated, and the previous refresh token — now already
consumed — is what a subsequent run would try to reuse. The result is a full
forced re-login, and depending on timing, a JSON-corrupt file rather than a
clean rollback. There's no temp-file + os.replace() for atomicity, and no
fcntl.flock/lockfile serializing concurrent access — so two concurrent
tv_auth/future-sync-tool invocations reading the same refresh token will
race: whichever POSTs to /v2/auth/refresh second gets a 401 on an
already-rotated token and is forced into NotLoggedIn, aborting whatever
batch operation triggered it. Given this is meant to back an unattended sync
tool (per the PR's stated epic), both the non-atomic write and the missing
lock are worth addressing before other tools build on TokenManager
(write-to-temp-then-rename is a small, cheap fix; locking can be a simple
advisory flock around refresh()/_store_tokens()).

4. The 2FA cookie-binding assumption is not established anywhere in
docs/thingiverse-api-v2.md, and is untested against a live 2FA account.

login_2fa()'s docstring and the module docstring assert "the 2FA exchange
is tied to the login attempt's session cookies," but nothing in the API doc
(or the OpenAPI-derived Authenticate2FARequest {code} schema) documents
cookie semantics — the doc is silent on transport beyond the JWT Bearer
scheme. The assumption is reasonable by elimination (the 2FA endpoint's
request body carries only code, so a cookie is the only remaining channel
for the server to know which login attempt the code belongs to), but it's
inference, not spec. The PR's test plan explicitly leaves live verification
unchecked, and doesn't call out that the 2FA path specifically needs
verification against an account that actually has 2FA enabled. If the
assumption is wrong, the failure mode is an opaque ThingiverseAuthError: 2FA login failed: HTTP 401 with no hint that the real problem is "must
reuse the same TokenManager/session as the initial login() call."
Recommend: (a) add a line to docs/thingiverse-api-v2.md's auth section
noting this is an inferred/observed behavior, not from the spec, and (b)
special-case a 401/403 from login_2fa() with a clearer message.

5. logout() only deletes the local token file; it never calls the API's
own logout endpoint. docs/thingiverse-api-v2.md documents GET /v2/auth/logout ("Logout, clear login cookies"). Local-only deletion means
the refresh token is never revoked server-side — it remains valid until it
naturally expires, so "logout" doesn't actually invalidate the session,
just the local copy. That may be an intentional/acceptable scope decision for
a CLI dotfile tool, but it's worth a one-line docstring caveat so future
callers (e.g. an eventual "revoke all sessions" feature) don't assume
logout() does more than it does.

Minor Issues

• auth.py:247 — jwt = body.get("jwt", body) correctly distinguishes
  AuthTokensResponse (has jwt key) from JwtTokenResponse (returned
  directly by refresh), matching the doc's two shapes. But if a response
  ever included an explicit "jwt": null or "jwt": {}, .get("jwt", body)
  would return that falsy value rather than falling back to body, and
  jwt.get("access") would raise AttributeError on None. Low
  probability given the doc's schemas, but same class of bug as #2 — an
  isinstance guard would make both spots equally robust.
• bin/tv_auth's status command writes its failure states
  ("Not logged in.", "Tokens stored but unusable: ...") via click.echo(...)
  to stdout, then sys.exit(1) — per the Unix-convention guidance in
  openforge/CLAUDE.md ("stderr for errors"), these should probably be
  click.echo(..., err=True) so a non-zero exit code is paired with stderr
  output, consistent with how login/logout already route failures
  through click.ClickException (which Click prints to stderr automatically).
• The refresh endpoint's 401 -> NotLoggedIn special-case (auth.py:192)
  assumes /v2/auth/refresh behaves like an authenticated route for its
  failure status code; the doc's only stated 401 semantics are for
  Authorization: Bearer routes in general, and /v2/auth/refresh takes
  the refresh token as a body param, not a Bearer header. Not necessarily
  wrong, just unconfirmed by the doc — worth a quick manual check when
  Devon does the live verification pass.

Positive Observations

• Login/2FA/refresh request payload key casing matches
  docs/thingiverse-api-v2.md exactly, including the (Thingiverse-side)
  inconsistency between usernameOrEmail (camelCase, login) and
  refresh_token (snake_case, refresh) — the code doesn't try to
  "normalize" this away, which is the right call.
• _jwt_exp's base64url padding math ("=" * (-len(payload_b64) % 4)) is
  correct for all four padding cases and is exercised well by the
  TestJwtHelpers tests.
• TokenManager accepting an injectable session is exactly the right
  seam for testing an HTTP-boundary class without a mocking framework, and
  matches how SessionService/IncrementalScanner do similar things
  elsewhere in the codebase — a class here is consistent with existing
  precedent for stateful resources (session + token file path), despite the
  general procedural-style preference in openforge/CLAUDE.md.
• Token file permissions (0600 file, 0700 dir) are set correctly and
  proactively via os.open/os.chmod rather than relying on umask.
• ruff check and ruff format --diff are clean on all four changed
  Python files; pytest tests/test_thingiverse_auth.py is 25/25 green.

Recommendations Summary

Must fix before merge:

• Set the executable bit on bin/tv_auth (Critical #1).
• Guard _jwt_exp against non-dict JSON payloads so malformed/corrupt
  tokens degrade to "expired" instead of crashing (Critical #2).

Should fix:

• Make _store_tokens atomic (write-temp + os.replace) and add a simple
  lock around refresh/persist to avoid the concurrent-invocation race
  (Medium #3).
• Document the 2FA cookie-binding assumption as inferred (not spec-backed)
  in docs/thingiverse-api-v2.md, and give login_2fa() failures a clearer
  error message (Medium #4).
• Add a one-line docstring caveat that logout() is local-only and doesn't
  revoke the refresh token server-side (Medium #5).

Nice to have:

• Guard the jwt.get(...) call similarly to #2 for the {"jwt": null} edge
  case (Minor).
• Route status's failure-path click.echo calls to stderr (Minor).
• Confirm refresh's 401 semantics during the live-verification pass (Minor).

Overall Assessment

Solid, well-tested first cut of the auth layer with good API-contract
fidelity to docs/thingiverse-api-v2.md; the two must-fix items are cheap
(a chmod and an except clause) but both are the kind of thing that bites
silently in production (a locked-out sync job, a script nobody can execute
directly), so worth closing before this becomes the foundation other
Thingiverse tickets build on.

[devonjones]

Response to Claude Review

Critical 1 (bin/tv_auth not executable): Fixed — exec bit set (git update-index --chmod=+x). Good catch; the chmod in the original workflow was silently skipped by a failed command chain.

Critical 2 (_jwt_exp AttributeError on non-dict payload): Fixed — explicit isinstance(payload, dict) guard returning None (→ treated as expired, per the documented contract). Regression test: test_non_dict_jwt_payload_treated_as_expired.

Medium 1 (non-atomic _store_tokens, no locking): Fixed (atomicity) — writes to a same-directory .tmp (0600) and swaps via os.replace, with cleanup-and-reraise on failure; tests cover both the no-leftover path and the crash path. Won't fix (locking) — this is a single-user local CLI; atomic replace eliminates corruption, and the residual concurrent-refresh race degrades to a spurious re-login, which isn't worth a lockfile protocol.

Medium 2 (2FA cookie-binding unverified): Acknowledged — it's an inference the public spec can't confirm; the constraint is documented on login_2fa() and the PR test plan includes live verification (bin/tv_auth login). If the live flow 401s, the fix lands within this epic — the HAR capture (openforge_catalog-hnr) will show the real mechanism.

Medium 3 (logout doesn't revoke server-side): Fixed — logout() now best-effort calls GET /v2/auth/logout with the bearer header before deleting the local file; API-unreachable and not-logged-in paths tested.

Minor 1 ("jwt": null AttributeError class): Fixed — body.get("jwt") or body plus an isinstance guard; both null and non-dict shapes raise the no-tokens error. Tests added.

Minor 2 (status failures on stdout): Fixed — failure messages now click.echo(..., err=True).

Minor 3 (refresh 401→NotLoggedIn mapping unconfirmed): Acknowledged — the spec documents 401 on the refresh endpoint; mapping it to NotLoggedIn (re-login required) is our semantic choice and gets validated in the live check.

[devonjones]

Verification of round-1 fixes (commit c47347d)

Ran pytest tests/test_thingiverse_auth.py tests/test_thingiverse_cli.py -q → 52 passed.
Ran ruff check openforge/thingiverse/ bin/tv_auth → clean.

1. bin/tv_auth exec bit — Confirmed: git ls-files -s bin/tv_auth shows 100755. ✅

2. _jwt_exp isinstance guard — Confirmed in auth.py: if not isinstance(payload, dict): return None before .get("exp"). Regression test test_non_dict_jwt_payload_treated_as_expired present and passing (list-payload JWT → _jwt_exp returns None, _token_expired → True). ✅

3. _store_tokens atomicity — Confirmed: writes to <file>.tmp (0600) in the same directory, os.replace() swap, except BaseException cleanup-and-reraise on failure. Tests: test_failed_store_cleans_up_tmp_and_reraises (forces os.replace to blow up, asserts no .tmp leftover and original file untouched) and test_store_leaves_no_tmp_file (happy path). Both pass. ✅

4. logout() best-effort server-side revocation — Confirmed: GET /v2/auth/logout with bearer header inside try/except catching (ThingiverseAuthError, requests.RequestException), local file always removed after. Tests cover success (test_logout_revokes_server_side_and_removes_file), unreachable API (test_logout_removes_file_when_revocation_fails, via ConnectionError), and not-logged-in noop (test_logout_without_file_is_noop, asserts zero session calls). All pass. Minor FYI (non-blocking): if the stored access token is expired at logout time, auth_header() will transparently trigger a refresh() (rewriting the token file) before the revoke call fires — harmless since the file is deleted right after, but that specific interleaving isn't exercised by a test.

5. jwt null/non-dict guards — Confirmed: jwt = body.get("jwt") or body plus isinstance(jwt, dict) guard, raising the same "succeeded but response had no tokens" error for both. test_null_jwt_in_login_body_raises and test_non_dict_jwt_in_login_body_raises both present and passing. ✅

6. status stderr routing — Confirmed: both failure branches in cli.py ("Not logged in." and "Tokens stored but unusable: ...") now use click.echo(..., err=True). (Note: CliRunner's default result.output merges stdout/stderr, so the existing tests assert content/exit-code but don't independently prove stream separation — verified by code inspection instead.)

Won't-fix / acknowledged rationale review:

• Locking won't-fix: reasonable — this is a single-user local dotfile CLI, and atomic os.replace already eliminates the corruption risk that mattered most; residual concurrent-refresh race degrades to a spurious re-login rather than data loss. Fine to defer until an actual concurrent caller (e.g. the sync engine) exists.
• 2FA cookie-binding / refresh-401 mapping acknowledged-pending-live-verification: reasonable — both are inferences from a partially-documented API that can only be confirmed against a real account; deferring to the live-verification pass called out in the PR test plan is the right call rather than blocking on speculation.

Scan of c47347d for new issues (CLI extraction + context manager): No new issues found. bin/tv_auth is a clean shim importing openforge.thingiverse.cli:cli. TokenManager.__enter__/__exit__/close() are straightforward and covered by test_context_manager_closes_session. The jwt = body.get("jwt") or body rewrite additionally guards the {"jwt": {}} edge case that the prior .get(key, default) form didn't, which is a strict improvement, not a regression.

Overall: LGTM. All 6 items verified fixed with passing regression tests; won't-fix/acknowledged items have sound rationale. No new issues introduced by the fix commit. Only the logout/refresh-interleaving note above is worth a mental bookmark, not a blocker.

[devonjones]

Live verification ✅

• bin/tv_auth login run interactively by Devon — succeeded
• bin/tv_auth status → Logged in as devonjones (id 47139) — token is Devon-scoped (not the stray v1 app-token account)
• Forced refresh() → HTTP 200, access token rotated, rotation persisted, whoami still authenticated

This closes out the two acknowledged review items: the login flow works end-to-end against the live API, and the refresh endpoint behaves as the code assumes.