feat(auth): add kbagent auth login-password for unattended CI login - #565
Conversation
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Adds an unattended, CI-oriented authentication path to the CLI by introducing kbagent auth login-password, which performs a password-grant login (optionally resolving TOTP MFA) and then reuses the existing session persistence / introspection / project-registration tail shared with the browser/device login flow.
Changes:
- Introduces
auth login-passwordend-to-end (CLI command, AuthService logic, AuthClient endpoints, newAUTH_MFA_INVALIDerror code). - Adds stdlib-only RFC 6238 TOTP computation and corresponding unit tests.
- Updates docs, plugin surfaces, permissions registry, and bumps version to
0.81.0.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Locks version bump to 0.81.0. |
| pyproject.toml | Bumps project version to 0.81.0. |
| src/keboola_agent_cli/commands/auth.py | Adds the auth login-password CLI command and wires env vars + TOTP computation. |
| src/keboola_agent_cli/services/auth_service.py | Implements password-grant login flow and shares the finalize/persist logic via _finalize_login(). |
| src/keboola_agent_cli/auth/auth_client.py | Adds POST /v1/auth/login and POST /v1/auth/mfa client methods. |
| src/keboola_agent_cli/auth/models.py | Adds MfaChallengeResult wire model for MFA challenges from password login. |
| src/keboola_agent_cli/auth/totp.py | Adds RFC 6238 TOTP computation helper (stdlib-only). |
| src/keboola_agent_cli/constants.py | Adds env var names for login-password and new auth endpoint paths. |
| src/keboola_agent_cli/errors.py | Adds AUTH_MFA_INVALID error code and category mapping. |
| src/keboola_agent_cli/permissions.py | Registers auth.login-password as a write operation. |
| src/keboola_agent_cli/changelog.py | Adds a 0.81.0 changelog entry describing the new command. |
| src/keboola_agent_cli/commands/context.py | Updates embedded CLI context docs to include login-password guidance. |
| tests/test_cli_auth.py | Adds CLI-level tests for args/env wiring, TOTP forwarding, and error surfacing. |
| tests/test_auth_totp.py | Adds unit tests for RFC 6238 behavior and input normalization. |
| tests/test_auth_service.py | Adds service-level tests for no-MFA, TOTP MFA, missing-code, and unsupported MFA factor branches. |
| tests/test_auth_client.py | Adds client-level tests for request bodies, MFA challenge parsing, and 404 mapping. |
| docs/auth.md | Documents the new unattended auth flow and its security posture. |
| docs/error-codes.md | Documents the new AUTH_MFA_INVALID error code. |
| plugins/kbagent/skills/kbagent/SKILL.md | Adds the new command to the skill’s command table. |
| plugins/kbagent/skills/kbagent/references/commands-reference.md | Extends command reference docs to include login-password details. |
| plugins/kbagent/.claude-plugin/plugin.json | Bumps plugin version to 0.81.0. |
| .claude-plugin/marketplace.json | Bumps marketplace plugin version to 0.81.0. |
| CLAUDE.md | Updates the hand-maintained command list and auth guidance to include login-password. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/test_auth_totp.py:17
- The RFC 6238 test-vector docstring says the module “hardcodes 6 digits” and suggests checking the low-order 6 digits, but the test actually computes 8 digits and asserts the full 8-digit value. This is misleading and should be updated to match the test behavior.
"""RFC 6238 appendix B: seed 'GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ' (the base32
encoding of the ASCII string '12345678901234567890') at T=59s -> '94287082'
for SHA1/8-digit. This module hardcodes 6 digits, so check the low-order
6 digits of the same well-known reference value instead."""
src/keboola_agent_cli/services/auth_service.py:321
- This error message is surfaced to end users, but it tells them to “pass totp_code”, which is not a CLI option (the CLI provides
--totp-secretand computes the code). Adjust the message to be actionable for CLI users while still describing the service parameter.
if not totp_code:
raise ConfigError(
"This account requires a TOTP code to sign in -- pass totp_code."
)
src/keboola_agent_cli/auth/auth_client.py:276
- On stacks where the password-grant endpoint is disabled, this call will currently surface the shared 404 message (“Browser login is not enabled...”), which is inaccurate for
login_password. Consider rewriting the 404 message for this method so users get the correct remediation (auth loginor static token).
response = self._do_request(
"POST",
AUTH_LOGIN_PATH,
json={"grantType": "password", "email": email, "password": password},
)
zajca
left a comment
There was a problem hiding this comment.
Review: NEEDS-WORK
Reviewed against the real Connection implementation on master (d6bb758342), connection/docs/rfc/programmatic-auth, and this repo's binding conventions. The wire contract is essentially right; the problems are in error handling, TOTP timing, four unupdated agent-facing doc surfaces, and one factual claim in docs/auth.md.
A. Confirmed correct against Connection master
Worth recording, because most of the contract checks out — in particular the load-bearing assumption.
| CLI assumption | Verdict | Evidence |
|---|---|---|
POST /v1/auth/login, body {grantType,email,password} |
correct | AuthLoginAction.php:67,117; LoginRequest.php:30,72-95 |
allowExtraFields:false — no stray keys sent |
correct | LoginRequest.php:91 vs auth_client.py:275 |
MFA challenge arrives as HTTP 200 with mfaRequired in the body |
correct — this is the load-bearing one | AuthLoginAction.php:171 returns HTTP_OK for both LoginResponse and MfaChallengeResponse |
mfaRequired/mfaType/mfaToken/expiresIn/allowedMethods |
correct | MfaChallengeResponse.php:16-22,84-97 |
POST /v1/auth/mfa with {mfaToken,type:"totp",code} |
correct | MfaAction.php:49; MfaRequest.php:29-33; MfaType.php:16 |
| 6-digit code required | correct | server enforces /^\d{6}$/; totp.py:37 does zfill(6) |
Reusing CliTokenResponse for the response |
correct — byte-identical shapes | LoginResponse.php:59-74 vs CliLogin/CliTokenResponse.php:59-75 |
| 404 ⇒ feature flag off, same flag as PKCE/device | correct | both controllers gate on AuthFeatures::STACK_PROGRAMMATIC_AUTH |
mfa_type != "totp" ⇒ cannot proceed |
correct, not the bug it looks like | ProgrammaticSessionService.php:113-116 — an admin has exactly one primary factor, so allowedMethods is [totp,recovery_code] or [webauthn,recovery_code], never both |
On the RFC: the password grant is not in conflict with it. programmatic-auth.md:56 lists it in scope ("Password + TOTP/recovery-code MFA flow (grantType: password)") and use case 5 is explicitly "E2E tests — existing test suites need a non-browser path to obtain user-scoped tokens" (programmatic-auth.md:40). The endpoint is a sanctioned programmatic path, not UI-only.
Also fine: loc-check passes with room (commands/auth.py 458 code lines vs 800/1200; auth_service.py 564 vs 1000/1500; auth_client.py 410 vs 1000/1500), check_error_codes.py and check_command_sync.py pass, the 0.80.0 → 0.81.0 bump is right (v0.80.0 released 2026-08-03), and neither SESSION_UNSUPPORTED_FEATURES nor the sentinel guards need an entry.
B. Blockers (repo conventions)
B1 — No E2E test, and unlike auth login there is no exemption available here.
CLAUDE.md convention #16: "Every new CLI command MUST have a corresponding E2E test." tests/test_e2e_auth.py exists and was not touched. The 0.80.0 flows were legitimately exempt (test_e2e_auth.py:31,479-506 — real browser / documented manual verification). login-password is the one auth command that is fully non-interactive, so it is also the one that can and must be covered end to end. Suggest new env gates E2E_LOGIN_EMAIL / E2E_LOGIN_PASSWORD / E2E_LOGIN_TOTP_SECRET, skipping when absent, mirroring the existing E2E_API_TOKEN / E2E_URL pattern in that file. Cover: no-MFA login, TOTP login, wrong password, --register-projects.
B2 — Four mandated doc surfaces not updated; three of them now state something false.
scripts/check_command_sync.py:31-38 explicitly does not gate these, which is exactly why they were missed:
plugins/kbagent/agents/keboola-expert.md(CONTRIBUTING.md calls it "highest silent-drift risk") — line 155: "needs a human at the keyboard, no headless path"; lines 326-334 document programmatic auth as browser-only; the Rule 6 VERSION GATE (line 76) still says 0.80.0+ with no 0.81.0 entry. Net effect: thekeboola-expertsubagent will refuse a command that now exists.references/gotchas.md— heading line 14 and bullet lines 16-19: "there is no headless/unattended path." Line 167: "any headless/unattended runner should keep using a static Storage token." No new gotcha carrying the mandatory(since v0.81.0)tag. The error-code → exit-code table (line 1764) has noAUTH_MFA_INVALIDrow.plugins/kbagent/.claude-plugin/CLAUDE.md— lines 92-95: "for unattended contexts point them at a static Storage token."references/auth-workflow.md(14.7 KB) — untouched.
B3 — Undocumented REST-route skip.
CONTRIBUTING.md line 350 requires a 1:1 route per command or "Document any skip in the PR description with a one-line reason." There is no server/routers/auth.py and the PR description does not mention the skip. The skip is almost certainly correct here — exposing a password grant over serve would let whoever holds KBAGENT_SERVE_TOKEN submit arbitrary credentials — but it needs saying, ideally in docs/web-server.md too.
C. Code defects
C1 — except ValueError swallows pydantic validation errors and blames --totp-secret.
In commands/auth.py the try block wraps service.login_password(...) as well as the TOTP computation. pydantic_core.ValidationError is a ValueError subclass, so every model_validate failure inside login_password / verify_mfa_totp / introspect surfaces as CONFIG_ERROR: --totp-secret: N validation errors for CliTokenResponse …, exit code 5, for a problem that has nothing to do with the flag.
if totp_secret:
try:
totp_code = compute_totp_code(totp_secret)
except ValueError as exc:
_handle_errors(formatter, ConfigError(f"--totp-secret: {exc}"))
else:
totp_code = None
try:
result = service.login_password(...)
except (ConfigError, KeboolaApiError) as exc:
_handle_errors(formatter, exc)C2 — A wrong password reports Invalid or expired token (token: ****).
login_password adds no status-specific mapping, so failures fall through to http_base.py:306-320:
- 401 (
InvalidCredentialsException) →INVALID_TOKEN, with a message about a token that does not exist in this flow (mask_token("")gets interpolated into it). - 403 →
ACCESS_DENIED, flattening two distinct actionable cases:SsoRequiredException(SAML-enforced account, carries a recovery URL —PasswordGrantProcessor.php:103-108) andSuperAdminMfaRequiredException(same file, 130-137). - 404 → the inherited message "Browser login is not enabled on this Keboola stack yet" (
auth_client.py:_map_auth_error), for a command whose entire point is that it is not browser login.
This is the single most likely failure mode of the command and it currently misdiagnoses it. Suggest explicit mapping: 401 → "invalid email or password" (AUTH_FLOW_DENIED already exists); 403 → surface the server message and name kbagent auth login for the SSO case; generalize the 404 wording to "programmatic auth is not enabled on this stack".
C3 — The TOTP code is computed too early.
It is computed in the CLI layer before the /v1/auth/login round trip and carried into /v1/auth/mfa. _do_request retries 429/5xx and timeouts (3 attempts, read=30s, backoff 1s/2s — http_base.py:172-218, constants.py:53-55,58), so the gap can reach ~60-90 s. Server tolerance is ~90 s (TotpVerifier.php:12-13), so this is narrow rather than theoretical: two read timeouts on the login call (30 + 1 + 30 = 61 s) already push a slice-N code out of the accepted window.
Fix: pass the seed (or a Callable[[], str]) into AuthService.login_password and compute the code immediately before verify_mfa_totp. That also resolves C1 naturally.
C4 — Retrying /v1/auth/mfa replays an already-consumed TOTP slice.
TotpVerifier.verifyAndConsume records (adminId, timeSlice) via INSERT IGNORE and rejects any later submission of the same slice (AdminTotpUsedTimeSliceRepository.php:34-42, TotpVerifier.php:9-19,33-42). Two consequences:
- A 429/5xx retry inside
_do_requestresubmits the same code — guaranteed rejection, and it burns one ofMAX_CHALLENGE_ATTEMPTS(MfaChallengeRepository.php:66-72). - Two CI jobs calling
login-passwordwithin the same 30 s slice: the second one fails. A matrix build sharing one MFA-enabled service account will fail intermittently with an opaque "invalid MFA code". Nothing in the docs warns about this.
Fix: exclude verify_mfa_totp from the retry loop — the same deliberate exception poll_device_token and refresh already make (auth_client.py:14-19) — or retry with a freshly computed code. Document the concurrency constraint in docs/auth.md.
C5 — The rate-limit signal is discarded.
/v1/auth/login allows 5 failed attempts per email and 20 per IP per 15 minutes (RateLimitListener.php:37-40), and the server emits X-RateLimit-Limit / -Remaining / -Reset on every response from these endpoints specifically "so clients can self-throttle before tripping the lockout", reporting the most restrictive bucket because "this is what a CLI or UI countdown cares about" (same file, 114-122).
The CLI ignores all three headers and clamps Retry-After to 60 s (constants.py:105), so a 15-minute lockout becomes three pointless retries and a generic API error 429. On a 429 from login/MFA, read X-RateLimit-Reset and report when the account unlocks.
D. Security and design
D1 — The docs/auth.md:152 claim that the session behaves "identically" to a browser-login session is false.
For an MFA-enabled account the password flow goes through createSessionAfterMfa, which sets sudoVerifiedAt: new DateTimeImmutable() unconditionally (ProgrammaticSessionService.php:136-150). PKCE/device instead inherit $context->sudoVerifiedAt(), which is "fresh only for MFA admins whose sudo window has not expired; NULL otherwise" (same file, 162, 175, 208-209).
So login-password reliably produces a session with a live 3-hour sudo window (SudoService::SUDO_TIMEOUT_SECONDS = 10800), which auth login usually does not. Sudo gates exactly the account-takeover-shaped operations: PAT create/revoke, TOTP delete, WebAuthn delete/register, recovery-code regeneration, revoke-all-sessions. Connection's own code flags the risk: "a login MFA code re-submitted to the sudo step-up on the freshly issued session … unlocks 1-year PAT creation" (TotpVerifier.php:14-17).
The security note in the docs discusses blast radius but omits this escalation. Related: IntrospectResponse (auth/models.py:154-161) drops the sudoVerified field the API returns (TokenIntrospectProcessor.php:51) — surfacing it in auth status would make this visible.
D2 — KBC_LOGIN_PASSWORD cuts against two established rules in this repo.
Not necessarily wrong, but it should be a stated decision rather than an implicit reversal:
- CLAUDE.md convention #12 makes the manage token default-deny from env (
--allow-env-manage-tokento opt in) precisely because "any subprocess (including the AI agent itself) inherits the manage token via env." An account password is a longer-lived credential than a manage token — it cannot be revoked without a password change — yet it is env-readable by default here. docs/programmatic-auth-login-plan.md:735-737(§ risk 4) states the guardrail: credentials "never logged, printed, put on the command line, or exported to subprocess environments."--passwordputs one on the command line;KBC_LOGIN_PASSWORDexports one to the environment.
--password-stdin is the right primitive and fixes the command-line half. Consider deprecating --password in its favour — the pattern already exists in this repo (dev-portal identity add --password-stdin).
D3 — The RFC nominates PATs for CI/CD, and kbagent has no PAT support at all.
programmatic-auth.md:326, use-case table: "User in Settings UI | POST /v1/auth/pat | … | Long-lived, broad-scope tokens for CI/CD, scripts." PATs are shipped ("Phase 2: Personal Access Tokens — shipped", line 282) and PatCreateAction / PatExchangeAction are live on master. The key operational difference (programmatic-auth.md:429): a password change cascade-revokes sessions, but "user-created PATs survive — they are the credential the user explicitly persisted for automation, and revoking them on password change would silently break unrelated CI/CD pipelines." A pipeline built on login-password therefore breaks on every password rotation.
Meanwhile kbagent has zero handling for kbc_pat_* or /v1/auth/pat (grep on this branch is empty). This is not a defect in the code as written — the password grant is legitimate for RFC use case 5 — but the PR should say why the password grant was chosen over the PAT path that already exists and that the RFC designates for CI/CD. Otherwise someone asks in three months.
D4 — docs/programmatic-auth-login-plan.md (49.6 KB, the design authority CLAUDE.md points at) was not updated and contains no mention of a password grant.
E. Nits
totp.py:26normalizes whitespace and case but does not pad base32.b32decoderequires a length that is a multiple of 8, so a legitimate unpadded seed (e.g. 26 chars) is rejected as "not valid base32". Addcleaned += "=" * (-len(cleaned) % 8).totp.py:26strips" "but not"-"; some enrollment UIs hyphenate seeds.MfaChallengeResultdoes not modelwebauthn, which the server includes for U2F admins (MfaChallengeResponse.php:20). Harmless underextra="allow"; worth a docstring line.allowedMethodsalways includesrecovery_code, including for WebAuthn-only accounts. A future--recovery-codewould let those accounts use this command too — enhancement, not a gap.
Suggested landing order
- C1, C2 — smallest diffs, worst user-facing symptoms.
- C3 + C4 — one refactor (seed pushed into the service, no retry on
verify_mfa_totp). - B2 — the four doc surfaces; the false statements are the real damage.
- B1 — E2E test.
- D1 — fix the "identically" claim.
- B3, D2, D3, D4 — PR description / design-doc write-ups.
- C5, E — polish.
|
Thanks for the depth here — especially section A verifying the wire contract against Connection master line by line. Addressed everything below, following your suggested landing order, in 4 commits: a48a321 (C1/C2/C3/C4/C5/E), 2aa7e7d (B2/D1), 8bbe323 (B1), c8d2a54 (B3/D3/D4). C1 ( C2 (wrong password → "Invalid or expired token") — Fixed. C3 + C4 (TOTP computed too early / retry replays a consumed slice) — Fixed as one refactor. C5 (rate-limit signal discarded) — Fixed. A 429 from login/MFA now reads E (TOTP seed parsing nits) — Fixed. B2 (four doc surfaces, three now false) — Fixed all four: B1 (no E2E test) — Fixed. Added D1 ("identically" claim, sudo escalation) — Fixed the claim in B3 (undocumented REST-route skip) — Documented in D3 (PATs vs. password grant) and D4 (design doc) — Addressed together in a new §12 addendum to D2 ( Ready for another pass whenever you have time. |
zajca
left a comment
There was a problem hiding this comment.
Selected findings from the automated read-only review.
c99a169 to
ee06e07
Compare
|
Thanks — this round caught two real bugs the first round's fixes introduced (O001, O002/S001) plus a genuinely dead guard (O003). Cross-validated against a thermo-nuclear structural pass + ponytail-review before touching anything; both independently converged on O001–O004, and ponytail additionally caught O001 (login_password retries through the shared loop) — Fixed. O002 / S001 (no transport-error mapping on the bypassed calls — same bug, two reviewers) — Fixed as part of the same refactor. O003 ( O004 ( O005 ( S003 ( Bonus (ponytail-review, not in your findings): S002 / D2 (env-var gating for One unrelated thing surfaced while getting Ready for another pass. |
zajca
left a comment
There was a problem hiding this comment.
Approved as requested. The remaining low-severity finding was sent as a non-blocking review comment.
zajca
left a comment
There was a problem hiding this comment.
Approved as requested. The remaining low-severity finding could not yet be published because GitHub rejected the inline review request; it will need to be retried separately.
|
Reviewed by Sonnet. The new constant |
Password-grant login (email + password + optional stdlib-computed TOTP code) as the deliberate, CI-safe exception to the existing browser-only PKCE/device-code login policy -- for CI/CD service accounts that use email+password (non-SAML) auth and cannot open a browser. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…w traceback Review found compute_totp_code() ran outside the command's try/except, so a non-base32 or blank TOTP secret crashed with an uncaught binascii.Error instead of the structured error every other input parser in this codebase produces. totp.py now validates and raises ValueError on empty/malformed input; the CLI maps that to ConfigError. Also adds TOTP period-boundary rotation and malformed/empty-secret test coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Fixed a stale test docstring claiming "this module hardcodes 6 digits" after the RFC 6238 test already exercised digits=8. - Split the auth_client.py "PKCE" section header: login_password/ verify_mfa_totp are a distinct password-grant+MFA section, not PKCE. - Fixed a ConfigError message that told a CLI user to "pass totp_code" -- the public flag is --totp-secret; the code is computed internally. - Added --password-stdin (mirrors dev-portal identity add's existing pattern): --password is now optional, and login-password fails with a ConfigError (not a Typer usage error) if neither --password, --password-stdin, nor KBC_LOGIN_PASSWORD supplied a value. Regenerated SKILL.md's auto-generated decision table to match the now-optional flag. Added test coverage for --password-stdin (TTY and pipe paths) and the missing-password error path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ming/replay Wrong-password/MFA failures on `login-password` no longer surface the generic session-oriented "Invalid or expired token" wording -- 401/403/404/429 on /v1/auth/login and /v1/auth/mfa now get a dedicated mapping naming the actual cause (AUTH_FLOW_DENIED, SSO/admin-MFA 403, rate-limit reset time). The TOTP seed is now passed into AuthService.login_password and the code computed immediately before the MFA request rather than before the login round trip, closing the timing gap a retried login could open. verify_mfa_totp now bypasses the shared retry loop (same exception poll_device_token/refresh already make) so a 429/5xx retry can never resubmit an already-consumed TOTP slice. totp.py also pads/strips seeds the way real enrollment UIs hand them out.
…session-parity claim keboola-expert.md, gotchas.md, the plugin CLAUDE.md, and auth-workflow.md still told an agent that no headless auth path exists and to fall back to a static Storage token unconditionally -- login-password now IS that headless path, so those surfaces get their own version-gated section/bullets instead of contradicting the CLI. docs/auth.md's claim that a login-password session behaves "identically" to a browser-login session was also narrowed: for an MFA-enabled account it carries a live 3-hour sudo window that a browser session usually does not (createSessionAfterMfa stamps it unconditionally), and two CI jobs sharing one MFA account within the same 30s window will see the second login fail.
Unlike the PKCE/device flows, login-password needs no human and no browser, so it is the one auth command with no exemption from the "every new CLI command needs an E2E test" rule. Gated on E2E_URL_US_EAST4/E2E_LOGIN_EMAIL/ E2E_LOGIN_PASSWORD/E2E_LOGIN_TOTP_SECRET (skips cleanly when absent, same two-tier pattern as the existing session-credentials gate): a full login->status->logout cycle (exercising the TOTP path when the seed is configured), a wrong-password regression guard for the new error message, and --register-projects.
The auth command group has no server/routers/auth.py counterpart at all (pre-existing), but that skip was never written down per CONTRIBUTING.md's "document any skip in the PR description" rule -- and for login-password specifically it's a deliberate choice, not an oversight: exposing a password grant over `serve` would let whoever holds KBAGENT_SERVE_TOKEN submit arbitrary account credentials. Also adds a design-doc addendum explaining why the password grant was used here (RFC use case 5, E2E tests) rather than the already-shipped PAT flow the RFC nominates for CI/CD, and flags PAT support as a follow-up given a password rotation cascade-revokes this grant's sessions while a PAT survives it.
The E2E_LOGIN_* secrets and E2E_URL_US_EAST4 variable existed but were never forwarded into e2e.yml's "Run E2E suite" step, so TestLoginPasswordCommand would keep skipping on every scheduled/manual run despite credentials being configured.
The first live run against the real service account failed test_register_projects_registers_accessible_projects with AUTH_FLOW_DENIED "Invalid or expired TOTP code" -- it and test_login_succeeds_and_produces_a_live_session each called login-password independently, ~1.6s apart, well inside the server's ~30s TOTP time-slice window. The server had already consumed that slice's code for the first login and correctly rejected the second submission -- this is the exact C4 concurrency constraint the PR documents, self-inflicted by two tests sharing one account's TOTP secret back to back. Merged the two into test_login_and_register_projects_succeeds: one login-password call with --register-projects, one TOTP code consumed.
… dead-code guard login_password still went through the shared retry loop (unlike every other credential-minting call in this client), so a 429/5xx/timeout retried the password grant up to 3 times -- burning extra requests against the account's rate-limit bucket before X-RateLimit-Reset guidance was even read, and risking a duplicate live session on a post-processing timeout that _finalize_login never gets to record. It now bypasses _do_request the same way verify_mfa_totp already does. Both bypassed calls previously mapped only HTTP status failures; a network blip (httpx.TimeoutException/TransportError) escaped as a raw traceback with no --json error envelope. Extracted _post_refresh's try/except shape into a shared _request_bypassing_retry, reused by login_password, verify_mfa_totp, and refresh() itself. Deleted the response.request.url.path dispatch in _map_auth_error and the matching is_mfa introspection in _raise_password_login_error -- both relied on httpx.Response.request, which raises RuntimeError rather than returning None when unset (never true in production, but true for any bare httpx.Response(...) built in a test, per the existing pattern in test_client.py::TestExtractCloudErrorCode). Now that login_password calls _raise_password_login_error directly instead of routing through the shared _map_auth_error, both callers pass is_mfa explicitly -- they already know which endpoint they hit, so there was nothing to gain from re-deriving it from the transport object. Also deleted _extract_api_message, a narrower duplicate of the existing _extract_error_message on the same class (missing the description/detail/ errors fallback keys) -- the one call site now reuses the original.
… reject conflicting password sources _read_password_stdin() in commands/auth.py was a byte-for-byte duplicate of the one already in commands/dev_portal.py -- same docstring, same body, two independent private copies. Moved to commands/_helpers.py as read_password_stdin(), reused by both call sites in dev_portal.py and the one in auth.py; no behavior change for dev-portal. auth login-password also silently let --password-stdin override --password/KBC_LOGIN_PASSWORD with no error when both were supplied, inconsistent with this codebase's established mutually-exclusive-input pattern (_metadata_input.py's --text/--file/--stdin). Now raises ConfigError. dev-portal's identity add/edit keep their existing (separate, unaudited) behavior -- out of scope here.
… missing it commands/context.py's AGENT_CONTEXT, CLAUDE.md's "All CLI Commands" block, and commands-reference.md's login-password bullet all showed the synopsis without --password-stdin and never mentioned it in prose -- despite it being the flag the command's own --help text recommends specifically to keep the password out of shell history and process listings. keboola-expert.md, auth-workflow.md, and gotchas.md already documented it correctly.
… budget Main's own recent commits added to this file independently, re-tipping it over the limit after the last rebase's trim.
ee06e07 to
7445d76
Compare
… version Two review blockers, both in the plugin doc surfaces. The agent prompt has a hard 62000-byte budget. This branch measured itself against the main it forked from, but main has since grown (#565's auth docs, then a trim back to 61990 B), leaving 10 bytes free -- merging as-is landed at 62119 B. Git cannot see that kind of conflict: both sides edit different lines, the merge is clean, and the invariant breaks anyway. The new note is now the short form, and the matched_columns prose on the row above is condensed the way 7445d76 did it -- same facts, fewer bytes. Merged result: 61985 B. gotchas.md entries carry a `(since vX.Y.Z)` tag, not an issue number: a reader needs to know whether their installed kbagent has the behavior, and #569 does not answer that. This ships in 0.84.0.
Summary
Adds
kbagent auth login-password-- an unattended, CI-safe login path using email + password (+ optional TOTP code, computed by kbagent itself from a stored TOTP seed) to obtain a full session token via the Auth Service's password grant (POST /v1/auth/login, resolving MFA viaPOST /v1/auth/mfa).Why
kbagent auth login(PKCE / device-code) deliberately requires a human at a browser -- correct for interactive use, but a hard blocker for non-SAML CI/CD service accounts that only have email+password (and optionally a TOTP MFA seed) as credentials. This command is the explicit, narrow exception: same session-token machinery (auth.json, sentinel-token registration, introspect, etc.) asauth login, but reached without any browser step.KBC_LOGIN_EMAIL/KBC_LOGIN_PASSWORD/KBC_LOGIN_TOTP_SECRETenv vars mirror the existingKBC_TOKEN/KBC_STORAGE_API_URLheadless-injection convention.auth/totp.py) -- no new dependency, and the seed is what's stored/passed, never a pre-computed code.AUTH_MFA_INVALIDerror code pointing back atkbagent auth login, since those genuinely require a browser ceremony._finalize_login()was extracted out oflogin()so both flows share the exact same persist/revoke/introspect/register-projects tail; existinglogin()tests all pass unchanged, confirming the extraction is behavior-preserving.Review pass (code quality + OWASP/security, independent agents)
Security review found no material issues: password/TOTP seed are never logged, written to disk, or included in exception messages; session storage correctly reuses the existing 0600-permissioned
auth.jsonpath rather than adding a parallel one; no injection risk (all values go throughhttpxJSON bodies, never string-formatted into URLs/shell). One non-blocking suggestion (add--password-stdinas a future enhancement to close the residualps/shell-history exposure window) -- deferred, not required for this PR since the env-var path is already the documented primary usage.Quality review found one real bug, now fixed in this PR:
compute_totp_code(totp_secret)ran inline as a call argument, outside the command'stry/except (ConfigError, KeboolaApiError)block -- a malformed or blank--totp-secretcrashed with a rawbinascii.Errortraceback instead of the structured JSON error every other input parser in this codebase produces. Fixed by validating intotp.py(raisesValueErroron empty/malformed base32) and mapping that toConfigErrorat the CLI layer. Added test coverage for: TOTP code rotation across the 30s period boundary, empty/whitespace-only secret, malformed base32, and the CLI-levelConfigErrormapping.Change type
Impact analysis
auth login-password; new error codeAUTH_MFA_INVALID; new env varsKBC_LOGIN_EMAIL/KBC_LOGIN_PASSWORD/KBC_LOGIN_TOTP_SECRET.auth login/auth status/auth logout/auth register-projectsbehavior beyond the internal_finalize_login()extraction (behavior-preserving, verified by unchanged existing tests).permissions.pyOPERATION_REGISTRYgains"auth.login-password": "write".Test plan
tests/test_auth_totp.py-- RFC 6238 test vector, default digit count, determinism/rotation across the period boundary, empty/whitespace/malformed-secret error paths, seed formatting tolerancetests/test_auth_client.py::TestLoginPassword/TestVerifyMfaTotp-- request bodies, MFA-required branch, 404 mappingtests/test_auth_service.py::TestLoginPassword-- no-MFA path, TOTP-resolution path (call order), missing-code error, non-TOTP-factor errortests/test_cli_auth.py::TestLoginPassword-- arg/env-var wiring, computed TOTP forwarded, malformed-secret maps toConfigError(not a traceback),AUTH_MFA_INVALIDsurfaces in--json, password never appears in outputmake check(ruff, format, ty, skill-check, version-check, command-sync-check, changelog-check, check-error-codes, check-sentinel-guards, loc-check, full pytest suite): 5428 passed, 11 skipped, 0 failedDeployment / Rollback plan
Standard: merge to
main, ships in the nextkbagentrelease. No migrations, no state changes to existing installs. Rollback is a plain revert -- the new command/env vars/error code are additive only.🤖 Generated with Claude Code