Skip to content

Route auth status/list/revoke/logout through TokenForResource#1258

Merged
Soph merged 5 commits into
mainfrom
soph/auth-status-followup
May 28, 2026
Merged

Route auth status/list/revoke/logout through TokenForResource#1258
Soph merged 5 commits into
mainfrom
soph/auth-status-followup

Conversation

@khaong
Copy link
Copy Markdown
Contributor

@khaong khaong commented May 24, 2026

https://entire.io/gh/entireio/cli/trails/422

Stacked on top of #1239 (soph/auth-go). Once that merges, this PR's base will auto-rebase to main.

Summary

In v2 split-host deployments (ENTIRE_AUTH_BASE_URLENTIRE_API_BASE_URL), the keyring stores an auth-host-issued token. Sending that token directly to the data API's /api/v1/auth/tokens endpoint produces 401s because the audience is wrong; the user sees a misleading "Token in keychain ... is no longer valid" message.

This PR routes entire auth status / auth list / auth revoke / logout (server-side revoke) through auth.TokenForResource so they pick up a correctly-audienced bearer via the same RFC 8693 exchange that entire search, activity, recap, and dispatch already use. In v1 single-host the tokenmanager hits its same-host shortcut and returns the keyring token unchanged — so this is a no-op for v1 deployments.

What's in the diff

cmd/entire/cli/auth.go, logout.go

  • defaultListTokens, defaultRevokeTokenByID, defaultRevokeCurrentToken now resolve a data-API-scoped bearer internally via auth.TokenForResource(ctx, api.OriginOnly(api.BaseURL())).
  • The bug-prone token / callerToken parameters are dropped from authTokenLister, authTokenRevoker, and revokeCurrentFunc so the audience-mismatch mistake is structurally impossible at the call sites.
  • New helper resolveDataAPIToken centralises the resolution.
  • requireSecureBaseURL now also calls auth.EnableInsecureHTTP() when --insecure-http-auth is set, matching NewAuthenticatedAPIClient — otherwise the tokenmanager's HTTPS guard rejects non-loopback http:// resources even after the per-command TLS check is waived.

cmd/entire/cli/api_client.go

  • Wrap the not-logged-in case as fmt.Errorf("...: %w", auth.ErrNotLoggedIn) (was errors.New(...) which lost the sentinel). Callers can now errors.Is past the boundary.

cmd/entire/cli/activity_cmd.go

  • Only print "Not logged in. Run 'entire login' to authenticate." when errors.Is(err, auth.ErrNotLoggedIn). Real errors (STS rejections, network errors, malformed env config) surface verbatim via main.go instead of being papered over with a misleading login hint.

Tests

  • auth_test.go and logout_test.go updated to match the new function-literal signatures. Assertions that checked "called with token X" are replaced with was-called booleans (the new contract is "implementation resolves its own bearer", so there's nothing to forward).

Test plan

  • go build ./... clean
  • mise run lint:go — 0 issues
  • mise run lint:gofmt clean
  • mise run lint:gomod clean
  • All TestRunAuth* / TestRunLogout* pass
  • End-to-end against us.auth.entire.io: entire auth status, auth list succeed (previously 401'd)

The same pre-existing TestExplainCmd_* failures from the base branch persist — environmental (.entire/settings.json enabled: false in this working tree), unrelated to this work.

Known gaps / follow-up work

Internal review during development surfaced several things that didn't make it into this PR's scope; happy to address in a follow-up:

  • STS-stage 401s aren't recognized by IsHTTPErrorStatusauth.go's 401 branch only fires for *api.HTTPError wrappers. auth-go/sts returns plain fmt.Errorf strings, so an STS-stage 401 (e.g. core token revoked, invalid_client) surfaces as a raw error rather than the friendly "Token in keychain is no longer valid. Run 'entire login' to re-authenticate." message. Worth either typing the STS error upstream or a string-match workaround here.
  • The self-revoke detection at auth.go:516 (list-after-revoke 401 → delete keychain) is pre-existing but mis-specified for v2 split-host — revoking an application API token by id doesn't invalidate the OAuth bearer chain, so the heuristic silently no-ops. Probably wants removal rather than papering over.
  • Other NewAuthenticatedAPIClient callers share the false-"authentication required" pattern that activity_cmd.go fixes — trail_cmd.go, trail_watch_cmd.go, dispatch_wizard.go, search_cmd.go. Sweep opportunity now that the sentinel-wrapping in api_client.go makes it trivial.
  • No direct unit test that defaultListTokens / defaultRevokeTokenByID / defaultRevokeCurrentToken actually route through TokenForResource. Covered e2e but a regression to "send raw keyring token" would pass go test ./.... The auth.SetManagerForTest seam exists in auth-go ready for use.
  • auth.EnableInsecureHTTP is sticky process-wide with no off switch — fine for one-shot CLI invocations, silent state leak across in-process subcommands (e.g. test harnesses, kubectl-style external command dispatcher).

🤖 Generated with Claude Code


Note

Medium Risk
Touches CLI authentication/token resolution and logout/token-revocation paths; mistakes could break auth flows or change which errors are surfaced to users, especially in split-host deployments.

Overview
Fixes split-host (ENTIRE_AUTH_BASE_URL != ENTIRE_API_BASE_URL) auth-token management by resolving a data-API-scoped bearer via auth.TokenForResource for entire auth status/list/revoke and server-side logout revocation, instead of forwarding the raw keyring token.

Refactors the token-management call sites to drop caller-supplied bearer parameters (making audience-mismatch harder to reintroduce), adds resolveDataAPIToken, and aligns --insecure-http-auth with the token manager via auth.EnableInsecureHTTP. Error handling is tightened so activity only shows the friendly login hint for auth.ErrNotLoggedIn, and NewAuthenticatedAPIClient preserves the ErrNotLoggedIn sentinel via wrapping.

Reviewed by Cursor Bugbot for commit 5824022. Configure here.

Copilot AI review requested due to automatic review settings May 24, 2026 05:08
@khaong khaong requested a review from a team as a code owner May 24, 2026 05:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes split-host (ENTIRE_AUTH_BASE_URLENTIRE_API_BASE_URL) auth-token management by ensuring auth status/list/revoke/logout obtain a data-API-audience bearer via auth.TokenForResource (RFC 8693 exchange) rather than sending the raw keyring token to the data API. It also improves error propagation by preserving the auth.ErrNotLoggedIn sentinel across NewAuthenticatedAPIClient, and refines activity to only show the “Not logged in” hint when that sentinel is actually present.

Changes:

  • Route auth token list/revoke/current-revoke through a centralized data-API token resolver (resolveDataAPIToken) and remove caller-supplied bearer parameters to prevent audience-mismatch regressions.
  • Preserve auth.ErrNotLoggedIn across NewAuthenticatedAPIClient and update activity to gate the friendly login hint on errors.Is(..., auth.ErrNotLoggedIn).
  • Update auth/logout unit tests to match the new function signatures (implementations now resolve their own bearer).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cmd/entire/cli/auth.go Adds resolveDataAPIToken, updates list/revoke flows to resolve their own data-API-scoped bearer, and relaxes tokenmanager HTTP guard when --insecure-http-auth is set.
cmd/entire/cli/logout.go Updates server-side revoke to internally resolve a data-API token and removes the caller-supplied bearer parameter.
cmd/entire/cli/api_client.go Wraps “not logged in” errors to preserve the auth.ErrNotLoggedIn sentinel for errors.Is checks.
cmd/entire/cli/activity_cmd.go Only prints the “Not logged in…” hint when the error is actually auth.ErrNotLoggedIn; otherwise returns the real error.
cmd/entire/cli/auth_test.go Updates injected lister/revoker function signatures in tests to match the new contracts.
cmd/entire/cli/logout_test.go Updates injected revoke function signature and assertions to match the new contract.

Comment thread cmd/entire/cli/api_client.go Outdated
Comment thread cmd/entire/cli/auth.go
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5824022. Configure here.

Comment thread cmd/entire/cli/activity_cmd.go
Comment thread cmd/entire/cli/auth.go
@khaong khaong force-pushed the soph/auth-status-followup branch from 5824022 to 357f576 Compare May 25, 2026 02:33
In v2 split-host deployments (ENTIRE_AUTH_BASE_URL ≠ ENTIRE_API_BASE_URL),
the keyring stores an auth-host-issued token. Sending that token directly
to the data API's /api/v1/auth/tokens endpoint produces 401s because the
audience is wrong; the user sees a misleading "Token in keychain ... is
no longer valid" message.

Three follow-on fixes:

cmd/entire/cli/auth.go, logout.go:
  defaultListTokens, defaultRevokeTokenByID, and defaultRevokeCurrentToken
  now resolve a data-API-scoped bearer internally via
  auth.TokenForResource(ctx, api.OriginOnly(api.BaseURL())) — RFC 8693
  exchange in split-host setups, same-host shortcut in single-host
  setups (so v1 single-host behavior is unchanged). The bug-prone
  `token` / `callerToken` parameters are dropped from authTokenLister,
  authTokenRevoker, and revokeCurrentFunc so the audience-mismatch
  mistake is structurally impossible at the call sites. New helper
  resolveDataAPIToken centralises the resolution.

  requireSecureBaseURL now also calls auth.EnableInsecureHTTP() when
  --insecure-http-auth is set, matching what NewAuthenticatedAPIClient
  already does — otherwise the tokenmanager's HTTPS guard would reject
  non-loopback http:// resources even after the per-command TLS check
  was waived.

cmd/entire/cli/api_client.go:
  Wrap the not-logged-in case as `fmt.Errorf("...: %w", auth.ErrNotLoggedIn)`
  so callers can still errors.Is past the boundary (was errors.New(...)
  which lost the sentinel).

cmd/entire/cli/activity_cmd.go:
  Only print "Not logged in. Run 'entire login' to authenticate." when
  errors.Is(err, auth.ErrNotLoggedIn). Any other error from
  NewAuthenticatedAPIClient (STS rejections, network errors, malformed
  env config) used to be silently re-mapped to that misleading login
  hint; now real errors surface verbatim via main.go.

End-to-end verified against us.auth.entire.io: entire login, entire
auth status, entire auth list, entire search, entire activity all
succeed. Pre-existing TestExplainCmd_* failures are environmental
(.entire/settings.json enabled:false in this working tree) and
unrelated.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@khaong khaong force-pushed the soph/auth-status-followup branch from 357f576 to c2d2cd6 Compare May 25, 2026 02:59
Base automatically changed from soph/auth-go to main May 27, 2026 07:16
Soph and others added 4 commits May 28, 2026 13:32
NewAuthenticatedAPIClient's "not logged in" branch was throwing away
the original tokenmanager error and wrapping the bare sentinel:

    return nil, fmt.Errorf("...: %w", auth.ErrNotLoggedIn)

Any context the manager attached — keyring backend message, expired-
token reason, store-load error chain — was dropped, with no
behavioural gain: the original err already wraps auth.ErrNotLoggedIn,
so errors.Is(err, auth.ErrNotLoggedIn) keeps working unchanged when
we wrap err directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 035b5f21e289
…tatus

Before this commit, runAuthStatus's friendly "Token in keychain for X
is no longer valid. Run 'entire login' to re-authenticate." branch
fired only on *api.HTTPError 401 from the data API. In split-host
setups (v2), two other failure modes leave the data API never being
called at all:

  - tokenmanager preflight: a stored core JWT whose `exp` claim is in
    the past surfaces as auth.ErrNotLoggedIn from TokenForResource —
    not a 401 — and the keyring read at the top of runAuthStatus
    still found a non-empty entry, so the "Not logged in" branch
    didn't catch it either. User saw a raw "validate token: ..."
    error.

  - STS rejection: when the auth host rejects the core token during
    RFC 8693 exchange (revoked, malformed, audience mismatch), auth-
    go's sts package wraps the response as
    `token exchange: status 4xx: <code>[: <desc>]`. No typed sentinel
    is exposed upstream, so detection has to be by prefix. The
    "status 4" anchor catches the entire 4xx range — every 4xx from
    STS is a credential problem the user has to fix with a re-login;
    5xx and transport errors flow through unchanged.

New helper isKeychainTokenRejected collapses all three shapes (data-
API 401, auth.ErrNotLoggedIn chain, sts 4xx prefix) into the single
re-login branch. Other shapes — network failures, malformed STS
response, manager construction errors — deliberately don't match so
the user still sees the real diagnostic.

Tested at two layers:
  - TestIsKeychainTokenRejected_AllShapes covers each error shape
    individually, including a deliberately-defeated string-only
    "wrapped ErrNotLoggedIn" case that confirms the substring fallback
    isn't accidentally catching sentinel cases.
  - TestRunAuthStatus_STSRejectionRendersInvalidMessage and
    TestRunAuthStatus_ExpiredCoreTokenRendersInvalidMessage pin the
    command-level UX — the friendly message actually fires on the new
    shapes, not just the helper in isolation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: e5ce5b8770df
The function-injection tests for runAuthStatus / runAuthList /
runAuthRevoke replace the authTokenLister / authTokenRevoker
contracts with fakes and never reach resolveDataAPIToken — which
is the helper that actually performs the audience-matching token
exchange this PR was written to fix. So the production path
(defaultListTokens etc. → resolveDataAPIToken → auth.TokenForResource)
had no direct coverage.

Two new tests install a real tokenmanager.Manager via
auth.SetManagerForTest and stub only the wire-level STS call via
tokenmanager.SetExchangeForTest:

  - TestResolveDataAPIToken_ScopesExchangeToDataAPIOrigin captures
    the Resource the manager hands to the exchange and asserts it
    is the api.OriginOnly(api.BaseURL()) origin — the bug this PR
    fixes was forwarding the wrong-audience token to the data API,
    so pinning the resource at the call boundary is the right
    assertion.

  - TestResolveDataAPIToken_WrapsManagerError covers the error path,
    asserting the "resolve API token" prefix and that the underlying
    manager error is preserved through %w.

Supporting machinery (authMemStore implementing tokenstore.Store,
saveCoreToken helper, newResolveTestManager wrapping tokenmanager.New
+ SetExchangeForTest) lives at the bottom of the test file. The
memStore duplicates auth-go's private tokenmanager_test.go memStore
rather than pulling in an internal test package.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 8421ebe8c196
Pre-PR runActivity treated every NewAuthenticatedAPIClient error as
"Not logged in" and silenced it via NewSilentError. The previous
commit (Route auth status/list/revoke/logout through TokenForResource)
correctly stopped doing that for real errors — STS rejection,
network failure, malformed config now surface verbatim instead of
hiding under a misleading login hint.

But it also stopped silencing context.Canceled. A user hitting
Ctrl+C during the keyring read or STS exchange now sees a noisy
"Error: context canceled" instead of a clean exit. The rest of the
codebase has a clear convention for this (clean.go, explain.go,
explain_export.go): errors.Is(err, context.Canceled) ||
errors.Is(err, context.DeadlineExceeded) → NewSilentError.

The new branch sits before the ErrNotLoggedIn check so cancellation
takes priority over the login hint — a context.Canceled wrapping
ErrNotLoggedIn (unlikely but possible during a slow keyring read)
should still exit silently rather than print a confusing
"re-authenticate" message at a user who chose to stop.

Tested via SetManagerForTest + a stub exchange that returns
context.Canceled — verifies SilentError wrap, preserved error
chain, and that errOut stays empty (no spurious login hint on
cancellation). Companion test pins the unchanged ErrNotLoggedIn
path so the new branch isn't accidentally consuming legitimate
"not logged in" cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 5c1b29dca799
@Soph Soph merged commit a21b3ef into main May 28, 2026
9 checks passed
@Soph Soph deleted the soph/auth-status-followup branch May 28, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants