mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-10 03:28:53 +00:00
fix(cli): say so when the keychain took a credential the file cannot name
Staging the token file can succeed and the replacement still fail afterwards, and that is the one save path where the keychain has already taken the new secret. It was reported as a save that kept nothing, which sends the user looking for a credential that is sitting in their keychain, and it claimed the previous login was untouched when the one keychain slot had just been written over. Give that path its own outcome and its own notice. The new secret stays where it is: the entry it replaced went the moment it landed, so no rollback brings that back, and removing the new one too would turn a login this machine may still be able to use into no login at all. The remaining `CredentialNotSaved` paths all leave both stores untouched, so the reassurance they carry is now true wherever it is printed.
This commit is contained in:
parent
ba637553f8
commit
c7da91d47f
3 changed files with 59 additions and 5 deletions
|
|
@ -45,14 +45,25 @@ from litellm.litellm_core_utils.private_json import (
|
|||
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class CredentialNotSaved:
|
||||
"""The credential was minted but no store would keep it, so this machine has none."""
|
||||
"""The credential was minted but no store would keep it, so this machine has none.
|
||||
|
||||
Nothing was touched on the way to this, so a login that already worked still does.
|
||||
"""
|
||||
|
||||
detail: str
|
||||
|
||||
|
||||
SecretSave: TypeAlias = SecretWrite | CredentialNotSaved
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class CredentialNotRecorded:
|
||||
"""The keychain took the credential, but the file that names it could not be replaced.
|
||||
|
||||
_UNREPLACEABLE_FILE: Final = "the staged file could not replace the one already there"
|
||||
The keychain holds one entry, so the secret that was there is already gone and no rollback
|
||||
brings it back. Removing the new one as well would only turn a login this machine may still
|
||||
be able to use into no login at all, so it stays, and the user is told what is where.
|
||||
"""
|
||||
|
||||
|
||||
SecretSave: TypeAlias = SecretWrite | CredentialNotSaved | CredentialNotRecorded
|
||||
|
||||
|
||||
class CliTokenRecord(BaseModel):
|
||||
|
|
@ -111,6 +122,10 @@ def save_cli_token(record: CliTokenRecord, *, vault: SecretVault = SYSTEM_KEYRIN
|
|||
half that a read-only or full directory refuses, so it is staged before the keychain is handed
|
||||
anything. A save that cannot land then leaves both stores exactly as it found them, which
|
||||
matters most when the login it failed to replace is still perfectly good.
|
||||
|
||||
Staging can still succeed and the replacement fail afterwards. That is the one case where the
|
||||
keychain has already taken the new secret, and it reports itself as such rather than claiming
|
||||
the previous login survived.
|
||||
"""
|
||||
staged: Final = _stage_token_file(_without_secret(record))
|
||||
if isinstance(staged, CredentialNotSaved):
|
||||
|
|
@ -121,7 +136,7 @@ def save_cli_token(record: CliTokenRecord, *, vault: SecretVault = SYSTEM_KEYRIN
|
|||
else vault.write(_encode_secret(record.base_url, record.key, record.jwt_token))
|
||||
)
|
||||
if isinstance(outcome, SecretStored):
|
||||
return outcome if _commit_token_file(staged) else CredentialNotSaved(_UNREPLACEABLE_FILE)
|
||||
return outcome if _commit_token_file(staged) else CredentialNotRecorded()
|
||||
discard_staged_json(staged)
|
||||
return _keep_the_secret_in_the_file(record, outcome)
|
||||
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ from litellm.litellm_core_utils.cli_keyring import (
|
|||
)
|
||||
from litellm.litellm_core_utils.cli_token_utils import (
|
||||
CliTokenRecord,
|
||||
CredentialNotRecorded,
|
||||
CredentialNotSaved,
|
||||
SecretSave,
|
||||
clear_cli_token,
|
||||
|
|
@ -126,6 +127,12 @@ def storage_notice(outcome: SecretSave) -> str:
|
|||
"Any login you already had is untouched. Run 'lite login' again once that path is "
|
||||
"writable, or 'lite logout' to clear whatever is stored now."
|
||||
)
|
||||
case CredentialNotRecorded():
|
||||
return (
|
||||
f"Signed in, and the credential is in your OS keychain, but {path} could not be "
|
||||
"replaced, so this machine may still be using your previous login. Run 'lite login' "
|
||||
"again once that path is writable, or 'lite logout' to clear both."
|
||||
)
|
||||
|
||||
|
||||
def keychain_unreadable_notice(vault: SecretVault) -> str:
|
||||
|
|
@ -754,7 +761,7 @@ def login(ctx: click.Context, config_claude: bool):
|
|||
click.echo("\nLogin successful!")
|
||||
click.echo(f"JWT Token: {api_key[:20]}...")
|
||||
click.echo(storage_notice(stored))
|
||||
if isinstance(stored, CredentialNotSaved):
|
||||
if isinstance(stored, (CredentialNotSaved, CredentialNotRecorded)):
|
||||
return
|
||||
click.echo("You can now use the CLI without specifying --api-key")
|
||||
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@ from litellm.litellm_core_utils.cli_keyring import (
|
|||
)
|
||||
from litellm.litellm_core_utils.cli_token_utils import (
|
||||
CliTokenRecord,
|
||||
CredentialNotRecorded,
|
||||
CredentialNotSaved,
|
||||
clear_cli_token,
|
||||
get_cli_token_file_path,
|
||||
|
|
@ -368,6 +369,37 @@ class TestSaveCliToken:
|
|||
assert json.loads(vault.blob)["key"] == "sk-in-use"
|
||||
assert load_cli_token(vault=vault).key == "sk-in-use"
|
||||
|
||||
def test_a_keychain_write_the_file_cannot_be_pointed_at_is_reported_as_that(
|
||||
self, isolated_home, secret_vault_factory
|
||||
):
|
||||
"""Staging the file can succeed and the replacement still fail, and that is the one path
|
||||
where the keychain already took the new secret. Reporting it as a save that kept nothing
|
||||
would send the user looking for a credential that is sitting in their keychain."""
|
||||
vault = secret_vault_factory()
|
||||
path = _token_file(isolated_home)
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.mkdir()
|
||||
|
||||
outcome = save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-new"), vault=vault)
|
||||
|
||||
assert isinstance(outcome, CredentialNotRecorded)
|
||||
assert json.loads(vault.blob)["key"] == "sk-new"
|
||||
|
||||
def test_the_credential_the_file_cannot_name_is_left_in_the_keychain(
|
||||
self, isolated_home, secret_vault_factory
|
||||
):
|
||||
"""The keychain holds one entry, so the secret that was there went the moment this one
|
||||
landed. Taking the new one back out would turn a login this machine may still be able to
|
||||
use into no login at all, and it cannot restore the old one either way."""
|
||||
vault = secret_vault_factory(blob=_blob(key="sk-in-use"))
|
||||
path = _token_file(isolated_home)
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.mkdir()
|
||||
|
||||
save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-new"), vault=vault)
|
||||
|
||||
assert vault.blob is not None
|
||||
|
||||
def test_a_failed_write_leaves_the_previous_credential_intact(self, isolated_home, secret_vault_factory, monkeypatch):
|
||||
path = _write_legacy_file(isolated_home)
|
||||
before = path.read_text()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue