fix(cli): stop deleting shared apiKey on device login; bind oauth grant to its host
apiKey is shared with sibling tools that read it from the same config file, so device login must not remove it. Precedence flips to a live OAuth token over apiKey; a dead grant now degrades to the saved key with a warning instead of aborting. The oauth block records the host it was minted against and is ignored (no use, no refresh) when base_url points elsewhere, so a staging grant is never sent to prod. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
0b43a16feb
commit
f506713f5a
|
|
@ -249,7 +249,10 @@ def _device_login(base_url: str) -> OAuthTokens:
|
|||
raise typer.Exit(1)
|
||||
|
||||
return OAuthTokens.from_response(
|
||||
tokens, client_id=endpoints.client_id, scope_fallback=endpoints.scope
|
||||
tokens,
|
||||
client_id=endpoints.client_id,
|
||||
scope_fallback=endpoints.scope,
|
||||
host=base_url,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -329,13 +332,16 @@ def _check_connection(base_url: str, api_key: str) -> None:
|
|||
|
||||
def _auth_mode_detail(config: CLIConfig) -> str:
|
||||
"""Human summary of which credential the CLI will use."""
|
||||
tokens = config.usable_oauth()
|
||||
if tokens is not None:
|
||||
if tokens.access_valid():
|
||||
secs = max(int(tokens.access_expires_at - time.time()), 0)
|
||||
return f"OAuth device token (expires in {secs // 60}m)"
|
||||
if config.api_key:
|
||||
return "API key (OAuth token expired)"
|
||||
return "OAuth device token (expired — will refresh)"
|
||||
if config.api_key:
|
||||
return "API key"
|
||||
if config.oauth and config.oauth.access_token:
|
||||
if config.oauth.access_valid():
|
||||
secs = max(int(config.oauth.access_expires_at - time.time()), 0)
|
||||
return f"OAuth device token (expires in {secs // 60}m)"
|
||||
return "OAuth device token (expired — will refresh)"
|
||||
return "missing — run `honcho init`"
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -55,36 +55,45 @@ def get_resolved_config():
|
|||
def maybe_refresh_token(config: CLIConfig) -> None:
|
||||
"""Refresh an expired OAuth access token in place and persist it.
|
||||
|
||||
No-op when a manual api_key is set or the access token is still valid. On
|
||||
refresh failure the grant is gone — prints a re-login hint and exits.
|
||||
No-op when there is no grant for the current host or the token is still
|
||||
valid. A dead grant degrades to the saved apiKey with a warning; exits
|
||||
only when nothing is left to authenticate with.
|
||||
"""
|
||||
if config.api_key: # manual key takes precedence; nothing to refresh
|
||||
return
|
||||
tokens = config.oauth
|
||||
tokens = config.usable_oauth()
|
||||
if tokens is None or tokens.access_valid():
|
||||
return
|
||||
if not tokens.refresh_token:
|
||||
print_error("SESSION_EXPIRED", "OAuth session expired. Run `honcho init` to log in again.")
|
||||
raise typer.Exit(1)
|
||||
|
||||
endpoints = oauth.resolve_endpoints(config.base_url)
|
||||
if tokens.client_id:
|
||||
endpoints = replace(endpoints, client_id=tokens.client_id)
|
||||
try:
|
||||
refreshed = oauth.refresh_access_token(endpoints, tokens.refresh_token)
|
||||
except oauth.OAuthFlowError:
|
||||
print_error("SESSION_EXPIRED", "OAuth session expired. Run `honcho init` to log in again.")
|
||||
raise typer.Exit(1)
|
||||
if tokens.refresh_token:
|
||||
endpoints = oauth.resolve_endpoints(config.base_url)
|
||||
if tokens.client_id:
|
||||
endpoints = replace(endpoints, client_id=tokens.client_id)
|
||||
try:
|
||||
refreshed = oauth.refresh_access_token(endpoints, tokens.refresh_token)
|
||||
except oauth.OAuthFlowError:
|
||||
refreshed = None
|
||||
if refreshed is not None:
|
||||
# rotation-safe: persist the (possibly new) refresh token before
|
||||
# it's reused; keep the old one if the server didn't rotate
|
||||
# (refresh_token is optional)
|
||||
config.oauth = OAuthTokens.from_response(
|
||||
refreshed,
|
||||
client_id=tokens.client_id,
|
||||
scope_fallback=tokens.scope,
|
||||
refresh_fallback=tokens.refresh_token,
|
||||
host=tokens.host,
|
||||
)
|
||||
config.save()
|
||||
return
|
||||
|
||||
# rotation-safe: persist the (possibly new) refresh token before it's reused;
|
||||
# keep the old one if the server didn't rotate (refresh_token is optional)
|
||||
config.oauth = OAuthTokens.from_response(
|
||||
refreshed,
|
||||
client_id=tokens.client_id,
|
||||
scope_fallback=tokens.scope,
|
||||
refresh_fallback=tokens.refresh_token,
|
||||
)
|
||||
config.save()
|
||||
if config.api_key:
|
||||
typer.echo(
|
||||
"OAuth session expired; using the saved API key. "
|
||||
"Run `honcho init` to log in again.",
|
||||
err=True,
|
||||
)
|
||||
return
|
||||
print_error("SESSION_EXPIRED", "OAuth session expired. Run `honcho init` to log in again.")
|
||||
raise typer.Exit(1)
|
||||
|
||||
|
||||
def get_client(*, require_workspace: bool = True):
|
||||
|
|
|
|||
|
|
@ -5,10 +5,14 @@ directory defaults to ``~/.honcho`` and can be relocated with `HONCHO_CONFIG_DIR
|
|||
|
||||
The CLI owns these top-level keys in that file:
|
||||
|
||||
apiKey -- Honcho admin JWT (manual auth)
|
||||
environmentUrl -- Honcho API URL (full URL, e.g. https://api.honcho.dev)
|
||||
oauth -- OAuth device-grant tokens (accessToken, refreshToken,
|
||||
accessExpiresAt, clientId, scope), written by device login
|
||||
accessExpiresAt, clientId, scope, host), written by
|
||||
device login
|
||||
|
||||
``apiKey`` (manual admin JWT) is shared with sibling tools: the CLI writes it
|
||||
on paste-key login and reads it as a fallback, but never deletes it. A live
|
||||
OAuth token takes precedence over ``apiKey`` for the CLI's own calls.
|
||||
|
||||
All other top-level keys (``hosts``, ``sessions``, ``saveMessages``,
|
||||
``sessionStrategy``, …) are written by sibling Honcho tools and are
|
||||
|
|
@ -82,6 +86,15 @@ class OAuthTokens:
|
|||
access_expires_at: float = 0.0 # epoch seconds
|
||||
client_id: str = ""
|
||||
scope: str = ""
|
||||
host: str = "" # base_url the grant was minted against
|
||||
|
||||
def matches_host(self, base_url: str) -> bool:
|
||||
"""True when the grant belongs to ``base_url``.
|
||||
|
||||
Tokens are host-scoped — a staging grant must not be sent to prod.
|
||||
Legacy blocks with no recorded host are trusted.
|
||||
"""
|
||||
return not self.host or self.host.rstrip("/") == base_url.rstrip("/")
|
||||
|
||||
def access_valid(self, skew: int = 60) -> bool:
|
||||
"""True while the access token is present and not within ``skew`` of expiry.
|
||||
|
|
@ -100,6 +113,7 @@ class OAuthTokens:
|
|||
client_id: str,
|
||||
scope_fallback: str = "",
|
||||
refresh_fallback: str = "",
|
||||
host: str = "",
|
||||
) -> OAuthTokens:
|
||||
"""Build persisted tokens from a token response.
|
||||
|
||||
|
|
@ -112,6 +126,7 @@ class OAuthTokens:
|
|||
access_expires_at=time.time() + resp.expires_in,
|
||||
client_id=client_id,
|
||||
scope=resp.scope or scope_fallback,
|
||||
host=host,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -131,12 +146,30 @@ class CLIConfig:
|
|||
session_id: str = ""
|
||||
oauth: OAuthTokens | None = None
|
||||
|
||||
def usable_oauth(self) -> OAuthTokens | None:
|
||||
"""The OAuth grant, if present and bound to the current host."""
|
||||
if (
|
||||
self.oauth
|
||||
and self.oauth.access_token
|
||||
and self.oauth.matches_host(self.base_url)
|
||||
):
|
||||
return self.oauth
|
||||
return None
|
||||
|
||||
def resolved_api_key(self) -> str:
|
||||
"""The key handed to the SDK: manual apiKey wins, else the OAuth token."""
|
||||
"""The key handed to the SDK: a live OAuth token wins, else apiKey.
|
||||
|
||||
An expired grant loses to a saved apiKey (a dead grant degrades to the
|
||||
shared key) but still wins over nothing, since the server is the final
|
||||
judge.
|
||||
"""
|
||||
tokens = self.usable_oauth()
|
||||
if tokens and tokens.access_valid():
|
||||
return tokens.access_token
|
||||
if self.api_key:
|
||||
return self.api_key
|
||||
if self.oauth and self.oauth.access_token:
|
||||
return self.oauth.access_token
|
||||
if tokens:
|
||||
return tokens.access_token
|
||||
return ""
|
||||
|
||||
@classmethod
|
||||
|
|
@ -166,6 +199,7 @@ class CLIConfig:
|
|||
access_expires_at=_coerce_epoch(oauth.get("accessExpiresAt")),
|
||||
client_id=str(oauth.get("clientId", "")),
|
||||
scope=str(oauth.get("scope", "")),
|
||||
host=str(oauth.get("host", "")),
|
||||
)
|
||||
|
||||
for fld_name, env_var in ENV_MAP.items():
|
||||
|
|
@ -181,10 +215,12 @@ class CLIConfig:
|
|||
return config
|
||||
|
||||
def save(self) -> None:
|
||||
"""Write ``apiKey`` + ``environmentUrl`` to config.json.
|
||||
"""Write ``environmentUrl`` + credentials to config.json.
|
||||
|
||||
Preserves unrelated top-level keys (``hosts``, ``sessions``,
|
||||
``saveMessages``, ``sessionStrategy``, …) that other tools write.
|
||||
``apiKey`` is written when set but never removed — sibling tools read
|
||||
it. The ``oauth`` block is CLI-owned and dropped when empty.
|
||||
"""
|
||||
CONFIG_DIR.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
|
|
@ -201,8 +237,6 @@ class CLIConfig:
|
|||
data["environmentUrl"] = self.base_url
|
||||
if self.api_key:
|
||||
data["apiKey"] = self.api_key
|
||||
else:
|
||||
data.pop("apiKey", None)
|
||||
|
||||
if self.oauth and self.oauth.access_token:
|
||||
data["oauth"] = {
|
||||
|
|
@ -211,6 +245,7 @@ class CLIConfig:
|
|||
"accessExpiresAt": self.oauth.access_expires_at,
|
||||
"clientId": self.oauth.client_id,
|
||||
"scope": self.oauth.scope,
|
||||
"host": self.oauth.host,
|
||||
}
|
||||
else:
|
||||
data.pop("oauth", None)
|
||||
|
|
|
|||
|
|
@ -44,12 +44,28 @@ def test_valid_token_is_not_refreshed(cfg_path):
|
|||
refresh.assert_not_called()
|
||||
|
||||
|
||||
def test_manual_key_short_circuits(cfg_path):
|
||||
def test_expired_token_refreshes_despite_manual_key(cfg_path):
|
||||
"""OAuth wins over apiKey now, so the grant is kept alive even with a key set."""
|
||||
config = _cfg(time.time() - 100)
|
||||
config.api_key = "manual"
|
||||
rotated = TokenResponse(
|
||||
access_token="new-at", refresh_token="new-rt", expires_in=3600, scope="write"
|
||||
)
|
||||
with patch("honcho_cli.oauth.refresh_access_token", return_value=rotated) as refresh:
|
||||
common.maybe_refresh_token(config)
|
||||
refresh.assert_called_once()
|
||||
assert config.resolved_api_key() == "new-at"
|
||||
|
||||
|
||||
def test_host_mismatch_skips_refresh(cfg_path):
|
||||
"""A grant minted for another host is ignored — no refresh, apiKey covers this one."""
|
||||
config = _cfg(time.time() - 100)
|
||||
config.oauth.host = "https://staging.example.com"
|
||||
config.api_key = "manual"
|
||||
with patch("honcho_cli.oauth.refresh_access_token") as refresh:
|
||||
common.maybe_refresh_token(config)
|
||||
refresh.assert_not_called()
|
||||
assert config.resolved_api_key() == "manual"
|
||||
|
||||
|
||||
def test_expired_token_refreshes_and_persists(cfg_path):
|
||||
|
|
@ -86,8 +102,25 @@ def test_refresh_failure_exits(cfg_path):
|
|||
common.maybe_refresh_token(config)
|
||||
|
||||
|
||||
def test_refresh_failure_falls_back_to_api_key(cfg_path):
|
||||
"""A dead grant degrades to the saved apiKey instead of aborting."""
|
||||
config = _cfg(time.time() - 100)
|
||||
config.api_key = "manual"
|
||||
with patch("honcho_cli.oauth.refresh_access_token", side_effect=OAuthFlowError("invalid_grant")):
|
||||
common.maybe_refresh_token(config) # must not raise
|
||||
assert config.resolved_api_key() == "manual"
|
||||
|
||||
|
||||
def test_missing_refresh_token_exits(cfg_path):
|
||||
config = _cfg(time.time() - 100)
|
||||
config.oauth.refresh_token = ""
|
||||
with pytest.raises(typer.Exit):
|
||||
common.maybe_refresh_token(config)
|
||||
|
||||
|
||||
def test_missing_refresh_token_falls_back_to_api_key(cfg_path):
|
||||
config = _cfg(time.time() - 100)
|
||||
config.oauth.refresh_token = ""
|
||||
config.api_key = "manual"
|
||||
common.maybe_refresh_token(config) # must not raise
|
||||
assert config.resolved_api_key() == "manual"
|
||||
|
|
|
|||
|
|
@ -167,7 +167,7 @@ class TestOAuth:
|
|||
def test_oauth_persists_camelcase_keys(self, cfg_path):
|
||||
CLIConfig(base_url="http://localhost:8000", oauth=self._tokens(1234)).save()
|
||||
on_disk = json.loads(cfg_path.read_text())["oauth"]
|
||||
assert set(on_disk) == {"accessToken", "refreshToken", "accessExpiresAt", "clientId", "scope"}
|
||||
assert set(on_disk) == {"accessToken", "refreshToken", "accessExpiresAt", "clientId", "scope", "host"}
|
||||
|
||||
def test_save_preserves_foreign_keys_with_oauth(self, cfg_path):
|
||||
cfg_path.write_text(json.dumps({"hosts": {"claude_code": {"peerName": "u"}}}))
|
||||
|
|
@ -181,16 +181,34 @@ class TestOAuth:
|
|||
CLIConfig(base_url="http://localhost:8000").save()
|
||||
assert "oauth" not in json.loads(cfg_path.read_text())
|
||||
|
||||
def test_stale_api_key_is_dropped(self, cfg_path):
|
||||
"""Device login must clear an old manual key, or it wins over the fresh token forever."""
|
||||
cfg_path.write_text(json.dumps({"apiKey": "old-manual-key"}))
|
||||
def test_api_key_preserved_on_device_login(self, cfg_path):
|
||||
"""apiKey is shared with sibling tools — device login must not delete it."""
|
||||
cfg_path.write_text(json.dumps({"apiKey": "shared-key"}))
|
||||
CLIConfig(base_url="http://localhost:8000", oauth=self._tokens(9999999999)).save()
|
||||
assert "apiKey" not in json.loads(cfg_path.read_text())
|
||||
on_disk = json.loads(cfg_path.read_text())
|
||||
assert on_disk["apiKey"] == "shared-key"
|
||||
assert on_disk["oauth"]["accessToken"] == "hch-at-x"
|
||||
|
||||
def test_resolved_api_key_prefers_manual_key(self, cfg_path):
|
||||
def test_resolved_api_key_prefers_live_oauth(self, cfg_path):
|
||||
cfg = CLIConfig(api_key="manual", oauth=self._tokens(9999999999))
|
||||
assert cfg.resolved_api_key() == "hch-at-x"
|
||||
|
||||
def test_resolved_api_key_expired_oauth_falls_back_to_api_key(self, cfg_path):
|
||||
cfg = CLIConfig(api_key="manual", oauth=self._tokens(time.time() - 100))
|
||||
assert cfg.resolved_api_key() == "manual"
|
||||
|
||||
def test_resolved_api_key_host_mismatch_falls_back_to_api_key(self, cfg_path):
|
||||
tokens = self._tokens(9999999999)
|
||||
tokens.host = "https://staging.example.com"
|
||||
cfg = CLIConfig(
|
||||
base_url="https://api.honcho.dev", api_key="manual", oauth=tokens
|
||||
)
|
||||
assert cfg.resolved_api_key() == "manual"
|
||||
|
||||
def test_resolved_api_key_expired_oauth_wins_over_nothing(self, cfg_path):
|
||||
cfg = CLIConfig(oauth=self._tokens(time.time() - 100))
|
||||
assert cfg.resolved_api_key() == "hch-at-x"
|
||||
|
||||
def test_resolved_api_key_falls_back_to_oauth(self, cfg_path):
|
||||
cfg = CLIConfig(oauth=self._tokens(9999999999))
|
||||
assert cfg.resolved_api_key() == "hch-at-x"
|
||||
|
|
@ -216,9 +234,23 @@ class TestOAuth:
|
|||
client_id="honcho-cli",
|
||||
scope_fallback="write",
|
||||
refresh_fallback="prior-rt",
|
||||
host="https://staging.example.com",
|
||||
)
|
||||
assert tokens.refresh_token == "prior-rt"
|
||||
assert tokens.scope == "write"
|
||||
assert tokens.host == "https://staging.example.com"
|
||||
|
||||
def test_host_round_trips_and_legacy_matches_all(self, cfg_path):
|
||||
tokens = self._tokens(9999999999)
|
||||
tokens.host = "https://staging.example.com"
|
||||
CLIConfig(base_url="https://staging.example.com", oauth=tokens).save()
|
||||
loaded = CLIConfig.load()
|
||||
assert loaded.oauth is not None
|
||||
assert loaded.oauth.host == "https://staging.example.com"
|
||||
# trailing-slash normalization + legacy blocks (no host) trust any host
|
||||
assert loaded.oauth.matches_host("https://staging.example.com/")
|
||||
assert not loaded.oauth.matches_host("https://api.honcho.dev")
|
||||
assert OAuthTokens(access_token="x").matches_host("https://anything.dev")
|
||||
|
||||
def test_redacted_masks_oauth_token(self):
|
||||
red = CLIConfig(oauth=self._tokens(1234)).redacted()
|
||||
|
|
|
|||
Loading…
Reference in New Issue