diff --git a/CLAUDE.md b/CLAUDE.md index 3a5660e3..df3b7cb2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -511,12 +511,18 @@ kbagent feature user-add --project ALIAS --email EMAIL --feature NAME [--dry-run kbagent feature user-remove --project ALIAS --email EMAIL --feature NAME [--dry-run] [--yes] # token: scoped Storage tokens (Keboola single-bucket-write pattern; acting token needs canManageTokens; secret shown once). +kbagent token list --project NAME kbagent token create --project NAME --description DESC [--bucket-write BUCKET ...] [--bucket-read BUCKET ...] [--component-access ID ...] [--can-read-all-file-uploads] [--expires-in N] kbagent token delete --project NAME --token-id ID [--yes] kbagent token refresh --project NAME --token-id ID [--yes] -# SDK (importable Client(url,token)) now exposes create_scoped_token / delete_token / refresh_token / -# create_stream_source / get_stream_source / list_stream_sources / delete_stream_source: dicts on .raw, -# typed ScopedTokenResult / StreamSourceResult on the facade. See docs/sdk.md. +# `token list` (issue #599): GET /v2/storage/tokens -- the only way to see what already exists and to +# get the id `delete`/`refresh` need, without the web UI. Secrets are STRIPPED from every row: a +# project with the `force-decrypted-token` feature has the API embed live values in the listing, and +# echoing those would break the group's "revealed once, at mint" contract for every token at once. +# SDK (importable Client(url,token)) now exposes create_scoped_token / list_tokens / delete_token / +# refresh_token / create_stream_source / get_stream_source / list_stream_sources / +# delete_stream_source: dicts on .raw, typed ScopedTokenResult / TokenListEntryResult / +# StreamSourceResult on the facade. See docs/sdk.md. # permissions: session write/destructive firewall. The top-level --deny-writes / --deny-destructive # flags are the one-shot form; `permissions set` persists a policy (mode allow|deny + allow/deny patterns diff --git a/docs/sdk.md b/docs/sdk.md index e921ddb0..2d388156 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -175,9 +175,10 @@ print(res.imported_rows, res.warnings) Seven methods for the "provision an OTLP ingest endpoint, then mint a narrowly-scoped Storage token a device can hold" flow. They live on the facade and delegate straight to `KeboolaClient`; the token/stream ones return the typed models in [§5](#5-typed-result-models). -**Scoped Storage tokens** — mint, revoke, rotate: +**Scoped Storage tokens** — mint, list, revoke, rotate: - **`create_scoped_token(*, description, bucket_permissions=None, component_access=None, can_read_all_file_uploads=False, expires_in=None) -> ScopedTokenResult`** — `POST /v2/storage/tokens`. `bucket_permissions` is `{bucket_id: "read"|"write"}`; `expires_in` is seconds. The acting token must carry **`canManageTokens`** or the create 403s. +- **`list_tokens() -> list[TokenListEntryResult]`** — `GET /v2/storage/tokens` (`0.86.0+`). Where the `token_id` for `delete_token` / `refresh_token` comes from. **Secrets are stripped before validation**: a project carrying the `force-decrypted-token` feature has the API embed live values in the listing, and `create_scoped_token` is meant to be the only reveal. The acting token needs `canManageTokens`. - **`delete_token(token_id) -> None`** — `DELETE /v2/storage/tokens/{id}` (204, no body). Revokes. - **`refresh_token(token_id) -> ScopedTokenResult`** — `POST .../tokens/{id}/refresh`. Rotates the secret in place; the returned `.token` is the new secret. @@ -236,7 +237,7 @@ Escape hatch for endpoints the facade omits. See [§7](#7-clientraw-the-escape-h ## 5. Typed result models -`result_models.py` defines the **stable return shapes** (`JobResult`, `QueryResult`, `UploadTableResult`, `ConfigDetailResult`, `SyncPushResult`, `CloneResult`, and the `0.66.0+` device-enrollment pair `ScopedTokenResult` / `StreamSourceResult`), all re-exported from the package root. They exist so a downstream consumer types against a **semver-versioned contract** instead of an undocumented `dict[str, Any]` — a contract change then surfaces at *type-check* time, not at runtime against a customer build. +`result_models.py` defines the **stable return shapes** (`JobResult`, `QueryResult`, `UploadTableResult`, `ConfigDetailResult`, `SyncPushResult`, `CloneResult`, the `0.66.0+` device-enrollment pair `ScopedTokenResult` / `StreamSourceResult`, and `TokenListEntryResult` from `0.86.0+`), all re-exported from the package root. They exist so a downstream consumer types against a **semver-versioned contract** instead of an undocumented `dict[str, Any]` — a contract change then surfaces at *type-check* time, not at runtime against a customer build. Two design rules every model follows (`_ApiResultModel` base): @@ -257,7 +258,9 @@ The two device-enrollment models (`0.66.0+`) commit these named fields: - **`ScopedTokenResult`** — `id`, `token` (the one-time secret, see the gotcha in §4), `description`, `expires` (`str | None`), `can_read_all_file_uploads` (alias `canReadAllFileUploads`). - **`StreamSourceResult`** — `id`, `source_id`, `name`, `type`, `description`, `branch_id` (default `"default"`), `otlp_url` (ingest URL, secret in the path — unmasked), `otlp_secret`, `base_endpoint`, `sink_bucket_id` (`str | None`; the `in.c-otlp-` bucket to grant a device token write on). -> **The committed surface is `__all__`.** Anything exported from `keboola_agent_cli` (`Client`, `Files`, `FileEntry`, the six result models, `JobIdempotencyStore`, `__version__`) is public API under semver. Renaming or removing a named field, or tightening a type, is a breaking change. +`TokenListEntryResult` (`0.86.0+`) commits `id`, `description`, `created` (`str | None`), `expires` (`str | None`), `is_expired` (alias `isExpired`), `is_master_token` (alias `isMasterToken`). It has **no secret field by design** — see `list_tokens` above. + +> **The committed surface is `__all__`.** Anything exported from `keboola_agent_cli` (`Client`, `Files`, `FileEntry`, the result models, `JobIdempotencyStore`, `__version__`) is public API under semver. Renaming or removing a named field, or tightening a type, is a breaking change. --- diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 30b76e06..18be314d 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -169,6 +169,17 @@ read it when a trigger fires. Each `(X.Y.Z+)` tag is the version floor. list` flags them). No migration command -- you do the argv mapping. Tool->command map in `docs/mcp-migration.md`; recipe in gotchas.md. +**A write that failed with a 5xx** +- `POST`/`PATCH` is NOT retried on 5xx any more (0.86.0+); `retryable: false` + there is deliberate -- never wrap it in your own retry loop. The work may have + landed: check with `token list` / `job list` before repeating. The message + carries the `exceptionId` -- quote it when escalating. gotchas.md. + +**Finding an existing Storage token** +- `kbagent token list -p P` (0.86.0+) -- the only source of the `--token-id` + that `token delete`/`refresh` need. Secrets are stripped from every row, + `--json` included; do not route around it with `kbagent http get`. + **Upgrading kbagent itself** - `install_channel` in `kbagent --json version` => native binary; `kbagent update` REFUSES by design. Quote `upgrade_command` (choco/winget/brew/apt/dnf); diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index c50e205f..42fd015c 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -99,6 +99,7 @@ When working inside a git repository or project directory, run `kbagent init` (o | Enable a feature flag on a user | `kbagent feature user-add --project PROJECT --email EMAIL --feature FEATURE` | | Disable a feature flag on a user (destructive) | `kbagent feature user-remove --project PROJECT --email EMAIL --feature FEATURE` | | Mint a scoped Storage API token (secret shown once) | `kbagent token create --project PROJECT --description DESCRIPTION` | +| List the project's Storage API tokens (no secrets -- those are mint-only) | `kbagent token list --project PROJECT` | | Revoke a Storage API token immediately (destructive; only non-master tokens) | `kbagent token delete --project PROJECT --token-id TOKEN-ID` | | Rotate a token: generate a new value and invalidate the old one (secret shown once) | `kbagent token refresh --project PROJECT --token-id TOKEN-ID` | | Show the current PAYG credit balance for one or more projects | `kbagent billing credits` | diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index b4cd7b2d..da57bf02 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -200,6 +200,7 @@ Uses the per-project Storage token (no manage token). Control plane = `stream. 204). Destructive; confirms via prompt unless `--yes` or `--json`. - `token refresh --project NAME --token-id ID [--yes]` -- rotate a token's secret (POST `.../refresh`); the new secret is printed ONCE. Confirms unless `--yes` or `--json`. diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 598a3f97..fdb350f6 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3709,3 +3709,65 @@ mapping in favor of `changed_since: adaptive`, which tracks a assumption: here the empty state is the expensive, surprising path, and a seeded checkpoint is the conservative one. Do not assume "no state = safe default" when adaptive is involved. + +## A failed POST/PATCH is no longer retried -- and the error now says what to do (since v0.86.0) + +kbagent used to retry every request on `429/500/502/503/504`, regardless of +HTTP method. That is safe for a read and unsafe for a write, because a 5xx does +not mean "nothing happened": + +- **Keboola's token mint persists the token row before the step that can + throw.** `POST /v2/storage/tokens` saves the new token, generates its secret, + and only then resolves bucket permissions -- and that last block is outside + any transaction. A 500 raised there leaves a live token behind that the + caller never saw, so three attempts could leave three of them. This is what + [issue #599](https://github.com/keboola/cli/issues/599) reported from the + `europe-west3.gcp` stack. +- The same hazard applies to every other non-idempotent call: `job run`, + `job terminate`, `config new`, `flow new`, `config oauth-url` (which mints a + token of its own), `data-app deploy`. + +What changed: + +- **5xx is retried only on `GET/HEAD/OPTIONS/PUT/DELETE`.** A `POST`/`PATCH` + answered with a 5xx fails on the first attempt. +- **429 is still retried on every method** -- the server is stating it did not + process the request, so repeating it is safe. +- **Transport failures are split by what they prove.** A refused connection + (`ConnectError`) or a connect/pool timeout never delivered the request and is + still retried on any method. A read/write timeout means the request was sent + and the outcome is unknown, so it is not retried on a `POST`/`PATCH`. + +Two consequences for an agent reading an error: + +1. **`retryable: false` on a 500 from a write is deliberate, not a + misclassification.** Do not paper over it with a retry loop of your own. The + correct next step is to check what the server already did -- for a token + mint, `kbagent token list`; for a job, `kbagent job list`. +2. **A 5xx message now carries the `exceptionId`** the Keboola API returns + alongside its generic `"Application error."`, plus one of two hints: the + request was not retried because the method is not idempotent, or the same + 5xx survived all 3 attempts and is therefore an upstream incident. Quote the + `exceptionId` when escalating to Keboola support -- it is the only handle + that traces back to the actual server-side exception. + +## `token list` is the only way to see what exists -- and it never shows secrets (since v0.86.0) + +`kbagent token list --project P` (`GET /v2/storage/tokens`) closes the gap where +the `token` group could mint, revoke, and rotate but not enumerate -- so there +was no way to obtain the `--token-id` that `delete` / `refresh` require without +the web UI. + +- **Secrets are stripped from every row, including under `--json`.** On a + project carrying the `force-decrypted-token` feature the Storage API embeds + each token's live value in the listing. kbagent removes it at the service + layer (and in the SDK facade) before anything is rendered: `token create` / + `token refresh` are the only reveals, and a listing that dumped live values + would break that contract for every token in the project at once. Do not + reach for `kbagent http get` to work around this. +- **It needs `canManageTokens`, same as `create`.** A plain Storage token gets + a 403 -> `ACCESS_DENIED`. +- The master token appears in the listing with `isMasterToken: true` and cannot + be deleted -- the API refuses. +- SDK parity: `Client.list_tokens() -> list[TokenListEntryResult]`, secrets + stripped there too. diff --git a/src/keboola_agent_cli/__init__.py b/src/keboola_agent_cli/__init__.py index dadbbfda..9b00981d 100644 --- a/src/keboola_agent_cli/__init__.py +++ b/src/keboola_agent_cli/__init__.py @@ -12,6 +12,7 @@ ScopedTokenResult, StreamSourceResult, SyncPushResult, + TokenListEntryResult, UploadTableResult, ) from .services.job_idempotency_store import JobIdempotencyStore @@ -33,6 +34,7 @@ "ScopedTokenResult", "StreamSourceResult", "SyncPushResult", + "TokenListEntryResult", "UploadTableResult", "__version__", ] diff --git a/src/keboola_agent_cli/auth/auth_client.py b/src/keboola_agent_cli/auth/auth_client.py index 8b38e75f..7259194a 100644 --- a/src/keboola_agent_cli/auth/auth_client.py +++ b/src/keboola_agent_cli/auth/auth_client.py @@ -770,7 +770,14 @@ def _extract_error_message(response: httpx.Response) -> str: # Shared error mapping # ------------------------------------------------------------------ - def _raise_api_error(self, response: httpx.Response, base_url: str | None = None) -> None: + def _raise_api_error( + self, + response: httpx.Response, + base_url: str | None = None, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> None: """Escalate a 404 before falling back to the shared error mapping. `BaseHttpClient._do_request` calls this method for every @@ -778,10 +785,20 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None (rather than adding a check in each method) covers all of them at once. `poll_device_token` bypasses `_do_request` entirely and calls `_map_auth_error` directly for the same 404 case. + + `hint` / `retryable` are the base class's 5xx guidance; they are + forwarded untouched so an auth-endpoint 500 reads the same as any + other (issue #599). A 404 is escalated before they matter. """ - self._map_auth_error(response) + self._map_auth_error(response, hint=hint, retryable=retryable) - def _map_auth_error(self, response: httpx.Response) -> NoReturn: + def _map_auth_error( + self, + response: httpx.Response, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> NoReturn: """Map a failed auth-endpoint response, escalating 404 to a dedicated code. `login_password`/`verify_mfa_totp` never reach this method -- both @@ -812,7 +829,7 @@ def _map_auth_error(self, response: httpx.Response) -> NoReturn: error_code=ErrorCode.AUTH_NOT_SUPPORTED_ON_STACK, retryable=False, ) - super()._raise_api_error(response, self._base_url) + super()._raise_api_error(response, self._base_url, hint=hint, retryable=retryable) # BaseHttpClient._raise_api_error always raises; the static type # checker cannot see that across the base-class call, so make the # divergence explicit rather than relax this method's NoReturn type. diff --git a/src/keboola_agent_cli/client/tokens.py b/src/keboola_agent_cli/client/tokens.py index 623cb80d..71a70b2e 100644 --- a/src/keboola_agent_cli/client/tokens.py +++ b/src/keboola_agent_cli/client/tokens.py @@ -159,6 +159,28 @@ def create_scoped_token( response = self._request("POST", "/v2/storage/tokens", data=data) return response.json() + def list_tokens(self) -> list[dict[str, Any]]: + """List the project's Storage API tokens (``GET /v2/storage/tokens``). + + Returns the API's array verbatim -- one dict per token, carrying ``id``, + ``description``, ``created``, ``expires``, ``isExpired``, + ``isMasterToken``, the ``can*`` grants, ``bucketPermissions`` and (when + the token was minted by another token) ``creatorToken``. + + The acting token must carry ``canManageTokens``; the API answers 403 + otherwise (surfaced as ``ACCESS_DENIED``). + + A **secret is never listed here as a rule, but the API is not a + guarantee**: on a project carrying the ``force-decrypted-token`` + feature the response embeds each token's live value in a ``token`` + field. Callers that render this must strip it -- ``TokenService`` + does. Anything past the documented array shape (an envelope object, + say) degrades to an empty list rather than blowing up in the caller. + """ + response = self._request("GET", "/v2/storage/tokens") + payload = response.json() + return payload if isinstance(payload, list) else [] + def delete_token(self, token_id: str) -> None: """Revoke a Storage API token immediately (``DELETE /v2/storage/tokens/{id}``). diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index c77adc68..274d1380 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -788,6 +788,10 @@ ### Scoped Storage Tokens + kbagent token list --project NAME + List the project's Storage API tokens (id, description, created, expires, master flag, + creating token). Secrets are never listed -- `token create` is the only reveal. This is where + the --token-id for delete/refresh comes from. Acting token needs canManageTokens. kbagent token create --project NAME --description DESC [--bucket-write BUCKET ...] [--bucket-read BUCKET ...] [--component-access ID ...] [--can-read-all-file-uploads] [--expires-in N] Create a scoped Storage API token (Keboola single-bucket-write pattern). --bucket-write / --bucket-read (repeatable) grant per-bucket write/read; write wins when a bucket is on both. diff --git a/src/keboola_agent_cli/commands/token.py b/src/keboola_agent_cli/commands/token.py index a8c3b471..71792aeb 100644 --- a/src/keboola_agent_cli/commands/token.py +++ b/src/keboola_agent_cli/commands/token.py @@ -19,6 +19,7 @@ import typer from rich.console import Console from rich.panel import Panel +from rich.table import Table from ..errors import ConfigError, ErrorCode, KeboolaApiError from ._helpers import ( @@ -71,6 +72,43 @@ def _format_created_token(console: Console, data: dict[str, Any]) -> None: console.print(Panel("\n".join(lines), title="Scoped token created", expand=False)) +def _format_token_list(console: Console, data: dict[str, Any]) -> None: + """Render the project's tokens as a table -- never their secret values.""" + tokens = data.get("tokens") or [] + alias = data.get("alias", "") + if not tokens: + console.print( + f"No tokens visible in project [cyan]{alias}[/cyan]. " + "Mint one with [bold]kbagent token create[/bold]." + ) + return + table = Table(title=f"Storage API tokens -- {alias} ({len(tokens)})") + table.add_column("ID", style="bold cyan") + table.add_column("Description") + table.add_column("Created", style="dim") + table.add_column("Expires", style="dim") + table.add_column("Master", justify="center") + table.add_column("Created by", style="dim") + for token in tokens: + expires = token.get("expires") + if not expires: + expires_label = "never" + elif token.get("isExpired"): + expires_label = f"[red]{expires} (expired)[/red]" + else: + expires_label = str(expires) + creator = token.get("creatorToken") or {} + table.add_row( + str(token.get("id", "")), + str(token.get("description", "")), + str(token.get("created") or ""), + expires_label, + "yes" if token.get("isMasterToken") else "", + str(creator.get("description") or ""), + ) + console.print(table) + + def _format_deleted_token(console: Console, data: dict[str, Any]) -> None: """Render the outcome of a token delete.""" console.print( @@ -145,6 +183,26 @@ def token_create( formatter.output(result, _format_created_token) +@token_app.command("list") +def token_list( + ctx: typer.Context, + project: str = typer.Option(..., "--project", "-p", help="Project alias"), +) -> None: + """List the project's Storage API tokens (no secrets -- those are mint-only). + + Answers "what already exists" and hands you the token id that `token delete` + and `token refresh` require, without a detour through the web UI. The acting + project token must carry canManageTokens, same as `token create`. + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "token_service") + try: + result = service.list_tokens(alias=project) + except (ConfigError, KeboolaApiError) as exc: + _handle_errors(formatter, exc) + formatter.output(result, _format_token_list) + + @token_app.command("delete") def token_delete( ctx: typer.Context, diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index e65d562d..36ecbbcf 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -54,6 +54,22 @@ def _resolve_app_name() -> str: MAX_RETRIES: int = 3 BACKOFF_BASE: float = 1.0 # seconds; delays: 1s, 2s, 4s +# RFC 9110 idempotent methods -- the only ones a 5xx or a read timeout may be +# repeated on. A failed POST/PATCH can have taken effect server-side before the +# error surfaced: Keboola's own token mint persists the token row BEFORE the +# step that can throw, so retrying a 500 from `POST /v2/storage/tokens` can +# leave live credentials behind that the caller never sees (issue #599). +# 429 is exempt from this gate -- the server states it did not process the +# request, so repeating it is safe regardless of method. +RETRY_SAFE_METHODS: frozenset[str] = frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"}) + +# Cap on the server-supplied `exceptionId` echoed into a 5xx error message. +# The value is untrusted input rendered into a Rich-markup console, so it gets +# the same "bounded before it reaches a terminal" treatment as the API's own +# error text (MAX_API_ERROR_LENGTH). Real Keboola ids run ~70 chars +# ("com-keboola-gcp-europe-west3-connection-<32 hex>"), so this is generous. +MAX_EXCEPTION_ID_LENGTH: int = 128 + # --- HTTP Timeout --- DEFAULT_TIMEOUT: httpx.Timeout = httpx.Timeout(connect=5.0, read=30.0, write=10.0, pool=5.0) diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index 91b5262f..e49d3aa0 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -9,6 +9,7 @@ import logging import os import platform +import re import time from typing import Any, Self from urllib.parse import urlparse, urlunparse @@ -20,14 +21,22 @@ BACKOFF_BASE, ENV_CONVERSATION_ID, MAX_API_ERROR_LENGTH, + MAX_EXCEPTION_ID_LENGTH, MAX_RETRIES, MAX_RETRY_AFTER_SECONDS, + RETRY_SAFE_METHODS, RETRYABLE_STATUS_CODES, ) from .errors import ErrorCode, KeboolaApiError, mask_token logger = logging.getLogger(__name__) +# Everything a real Keboola `exceptionId` is made of. Anything else in that +# server-supplied field -- Rich markup brackets, newlines that would forge an +# extra log line (CWE-117), control characters -- is dropped before the id is +# interpolated into an error message. +_EXCEPTION_ID_DISALLOWED = re.compile(r"[^A-Za-z0-9._:-]+") + def build_user_agent() -> str: """Build the User-Agent that signs every Keboola API call. @@ -151,6 +160,13 @@ def _do_request( Retries on status codes 429, 500, 502, 503, 504 up to MAX_RETRIES times with exponential backoff (1s, 2s, 4s). + A 5xx (and a read/write timeout) is only repeated on an idempotent + method -- see ``RETRY_SAFE_METHODS``. Repeating a failed POST/PATCH can + duplicate server-side state the caller never gets to see, so those fail + on the first attempt with a message saying so (issue #599). A 429 and a + refused connection are repeated on every method: in both cases the + server provably did not process the request. + Args: method: HTTP method (GET, POST, etc.). path: URL path relative to base_url. @@ -168,6 +184,11 @@ def _do_request( http_client = client or self._client url_label = base_url or self._base_url last_response: httpx.Response | None = None + retry_safe = method.upper() in RETRY_SAFE_METHODS + # Counted separately from `attempt`: a 429 burns an attempt without + # being a server error, so using the attempt index would report "the + # same 5xx came back on N attempts" after seeing exactly one. + server_error_attempts = 0 for attempt in range(MAX_RETRIES): try: @@ -176,7 +197,15 @@ def _do_request( if response.status_code < 400: return response - if response.status_code in RETRYABLE_STATUS_CODES and attempt < MAX_RETRIES - 1: + if response.status_code >= 500: + server_error_attempts += 1 + # 429 is repeatable on any method; a 5xx only on an idempotent one. + may_repeat = retry_safe or response.status_code == 429 + if ( + response.status_code in RETRYABLE_STATUS_CODES + and may_repeat + and attempt < MAX_RETRIES - 1 + ): if response.status_code == 429: retry_after = response.headers.get("Retry-After") if retry_after: @@ -201,10 +230,23 @@ def _do_request( last_response = response continue - self._raise_api_error(response, url_label) + hint = self._server_error_hint(method, response.status_code, server_error_attempts) + self._raise_api_error( + response, + url_label, + hint=hint, + retryable=may_repeat and response.status_code in RETRYABLE_STATUS_CODES, + ) except httpx.TimeoutException as exc: - if attempt < MAX_RETRIES - 1: + # A connect/pool timeout never delivered the request, so it is + # repeatable on any method. A read/write timeout means the + # request WAS sent and the outcome is unknown -- only repeat it + # when the method is idempotent. + timeout_repeatable = retry_safe or isinstance( + exc, httpx.ConnectTimeout | httpx.PoolTimeout + ) + if timeout_repeatable and attempt < MAX_RETRIES - 1: delay = BACKOFF_BASE * (2**attempt) logger.debug( "Retry attempt %d/%d for %s %s (timeout), delay %.1fs", @@ -216,11 +258,15 @@ def _do_request( ) time.sleep(delay) continue + unsafe_note = "" if timeout_repeatable else f" {self._non_idempotent_note(method)}" raise KeboolaApiError( - message=f"Request timed out connecting to {url_label} (token: {self._masked_token})", + message=( + f"Request timed out connecting to {url_label} " + f"(token: {self._masked_token}){unsafe_note}" + ), status_code=0, error_code=ErrorCode.TIMEOUT, - retryable=True, + retryable=timeout_repeatable, ) from exc except httpx.ConnectError as exc: @@ -244,7 +290,13 @@ def _do_request( ) from exc if last_response is not None: - self._raise_api_error(last_response, url_label) + self._raise_api_error( + last_response, + url_label, + hint=self._server_error_hint( + method, last_response.status_code, server_error_attempts + ), + ) raise KeboolaApiError( message=f"Request failed after {MAX_RETRIES} retries to {url_label} (token: {self._masked_token})", @@ -253,7 +305,68 @@ def _do_request( retryable=True, ) - def _raise_api_error(self, response: httpx.Response, base_url: str | None = None) -> None: + @staticmethod + def _safe_exception_id(raw: object) -> str: + """Bound and de-fang the server-supplied ``exceptionId``. + + The id is untrusted input that ends up in a message rendered through + Rich with markup enabled (``OutputFormatter.error``), so it gets the + same "bounded before it reaches a terminal" treatment the API's own + error text gets from MAX_API_ERROR_LENGTH. Disallowed characters are + dropped rather than escaped: a real Keboola id contains none of them, + so this is lossless in practice, and support still gets a handle out + of a partially mangled value instead of nothing. + """ + if not isinstance(raw, str): + return "" + return _EXCEPTION_ID_DISALLOWED.sub("", raw)[:MAX_EXCEPTION_ID_LENGTH] + + @staticmethod + def _non_idempotent_note(method: str) -> str: + """One sentence naming the partial-effect risk of an unrepeated write.""" + return ( + f"{method.upper()} is not idempotent, so this request was not retried -- " + "the operation may already have taken effect server-side; verify the resource " + "state before trying again." + ) + + @classmethod + def _server_error_hint(cls, method: str, status: int, server_error_attempts: int) -> str | None: + """Return the actionable next step for a 5xx, or None if there isn't one. + + Two situations need two different answers (issue #599). A 5xx on a + non-idempotent write means the server may already have done the work, + so the next step is to check before repeating. A 5xx that survived + every retry is the opposite: an upstream incident nobody on this side + can fix, and the next step is to escalate with the exceptionId. A + generic "API error 500" said neither. + + The method gate is checked FIRST and `server_error_attempts` counts + only 5xx responses, because a POST can reach a second attempt via a + 429. Ordering it the other way round told the operator to escalate on + exactly the request where they most needed to go and check what had + already landed. + """ + if status < 500: + return None + if method.upper() not in RETRY_SAFE_METHODS: + return cls._non_idempotent_note(method) + if server_error_attempts > 1: + return ( + f"The same 5xx came back on all {server_error_attempts} attempts, which points " + "at an upstream Keboola incident rather than a caller mistake -- check " + "status.keboola.com and contact Keboola support, quoting the exceptionId above." + ) + return None + + def _raise_api_error( + self, + response: httpx.Response, + base_url: str | None = None, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> None: """Convert an HTTP error response into a KeboolaApiError. Parses the response body for error messages, truncates long messages @@ -263,6 +376,12 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None Args: response: The HTTP error response. base_url: Optional URL label for error messages. + hint: Optional actionable next step appended to a 5xx message + (see :meth:`_server_error_hint`). + retryable: Overrides the status-derived ``retryable`` flag. A 500 + on a POST is in RETRYABLE_STATUS_CODES but must NOT be + advertised as retryable -- kbagent deliberately did not repeat + it, and neither should the caller without checking first. Raises: KeboolaApiError: Always raised with appropriate error code and message. @@ -270,8 +389,15 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None status = response.status_code url_label = base_url or self._base_url + exception_id = "" try: body = response.json() + # Keboola answers a 5xx with a generic `error` ("Application + # error.") plus an `exceptionId` -- the ONLY handle Keboola support + # can trace the incident by. Dropping it, as this parser used to, + # left the operator with nothing to escalate (issue #599). + if isinstance(body, dict): + exception_id = self._safe_exception_id(body.get("exceptionId")) # Real Keboola APIs answer with one of these keys in priority # order. Two caveats: # 1. Keboola Metastore puts the HTTP status code into `error` @@ -327,10 +453,21 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None retryable=False, ) - retryable = status in RETRYABLE_STATUS_CODES + # Appended AFTER the truncation above so they always survive into the + # message the operator actually reads. That is safe only because each + # is bounded on its own: the hint is a kbagent-authored constant, and + # the id went through `_safe_exception_id`. Never append raw + # server-supplied text here -- the truncation is what keeps the + # console (Rich, markup enabled) from rendering it as markup. + suffix = f" [exceptionId: {exception_id}]" if exception_id else "" + if hint: + suffix += f" {hint}" raise KeboolaApiError( - message=f"API error {status} from {url_label} (token: {self._masked_token}): {api_message}", + message=( + f"API error {status} from {url_label} " + f"(token: {self._masked_token}): {api_message}{suffix}" + ), status_code=status, error_code=ErrorCode.API_ERROR, - retryable=retryable, + retryable=status in RETRYABLE_STATUS_CODES if retryable is None else retryable, ) diff --git a/src/keboola_agent_cli/lib.py b/src/keboola_agent_cli/lib.py index 46b0056a..147c69c8 100644 --- a/src/keboola_agent_cli/lib.py +++ b/src/keboola_agent_cli/lib.py @@ -57,6 +57,7 @@ QueryResult, ScopedTokenResult, StreamSourceResult, + TokenListEntryResult, UploadTableResult, ) from .services.job_idempotency_store import JobIdempotencyStore, run_idempotent_job @@ -567,6 +568,22 @@ def create_scoped_token( ) ) + def list_tokens(self) -> list[TokenListEntryResult]: + """List the project's Storage API tokens as typed entries. + + The counterpart to :meth:`create_scoped_token`: it answers "what did I + already mint" and hands back the ``id`` that :meth:`delete_token` / + :meth:`refresh_token` need. Secrets are stripped before validation -- + the mint is the only reveal (see :class:`TokenListEntryResult`). The + acting token must carry ``canManageTokens``. + """ + return [ + TokenListEntryResult.model_validate( + {key: value for key, value in token.items() if key != "token"} + ) + for token in self._client.list_tokens() + ] + def delete_token(self, token_id: str) -> None: """Revoke a Storage API token immediately (active per-device revocation).""" self._client.delete_token(token_id) diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 4efcfcf0..440b89d1 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -61,6 +61,7 @@ "stream.delete": "destructive", # Storage API scoped tokens: minting/rotating is a write (creates a # credential), revoking is destructive (a live token stops working). + "token.list": "read", "token.create": "write", "token.refresh": "write", "token.delete": "destructive", diff --git a/src/keboola_agent_cli/result_models.py b/src/keboola_agent_cli/result_models.py index f7f8543c..db535105 100644 --- a/src/keboola_agent_cli/result_models.py +++ b/src/keboola_agent_cli/result_models.py @@ -297,6 +297,36 @@ class ScopedTokenResult(_ApiResultModel): ) +class TokenListEntryResult(_ApiResultModel): + """One Storage API token as listed by :meth:`keboola_agent_cli.Client.list_tokens`. + + Deliberately carries **no** secret field. `create_scoped_token` is the one + and only reveal in this SDK; a listing that returned live values would + break that contract for every token in the project at once, so the facade + drops the field before validating even when the API includes it (projects + with the ``force-decrypted-token`` feature do). Everything else the API + reports -- ``bucketPermissions``, ``componentAccess``, the remaining + ``can*`` grants -- is preserved as model extras. + """ + + id: str = Field(default="", description="Token ID (use with delete_token / refresh_token).") + description: str = Field(default="", description="Human-readable token description.") + created: str | None = Field(default=None, description="ISO creation timestamp.") + expires: str | None = Field( + default=None, description="ISO expiry timestamp; None = never expires." + ) + is_expired: bool = Field( + default=False, + validation_alias=AliasChoices("isExpired", "is_expired"), + description="True once the token is past its expiry.", + ) + is_master_token: bool = Field( + default=False, + validation_alias=AliasChoices("isMasterToken", "is_master_token"), + description="True for the project's master token (cannot be deleted).", + ) + + class StreamSourceResult(_ApiResultModel): """A per-device Data Streams (OTLP) source (issue: device enrollment). diff --git a/src/keboola_agent_cli/server/routers/token.py b/src/keboola_agent_cli/server/routers/token.py index fdc5b740..d3546a08 100644 --- a/src/keboola_agent_cli/server/routers/token.py +++ b/src/keboola_agent_cli/server/routers/token.py @@ -32,6 +32,17 @@ class TokenIdBody(BaseModel): token_id: str +@router.get("/{project}/list", summary="List the project's Storage tokens") +def list_tokens( + project: str, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """List the project's tokens without their secrets. Mirrors + `kbagent token list`. The acting project token must have canManageTokens. + """ + return registry.token.list_tokens(alias=project) + + @router.post("/{project}/create", summary="Mint a scoped Storage token") def create_token( project: str, diff --git a/src/keboola_agent_cli/services/token_service.py b/src/keboola_agent_cli/services/token_service.py index 7d425349..e58e796a 100644 --- a/src/keboola_agent_cli/services/token_service.py +++ b/src/keboola_agent_cli/services/token_service.py @@ -96,6 +96,29 @@ def create_scoped_token( finally: client.close() + def list_tokens(self, *, alias: str) -> dict[str, Any]: + """List every Storage token in ``alias``'s project. + + Returns ``{"alias", "count", "tokens"}``. Each entry is the raw API + token dict **minus its ``token`` field**: a project with the + ``force-decrypted-token`` feature has the Storage API embed live secret + values in the listing, and echoing those would break the group's + "the secret is revealed once, at mint" contract for every token at + once. Everything else is passed through untouched. + + The acting token needs ``canManageTokens``, same as create/refresh. + """ + creds = self._resolve_project(alias) + client = self._client_factory(creds.stack_url, creds.token) + try: + tokens = [ + {key: value for key, value in token.items() if key != "token"} + for token in client.list_tokens() + ] + return {"alias": alias, "count": len(tokens), "tokens": tokens} + finally: + client.close() + def delete_token(self, *, alias: str, token_id: str) -> dict[str, Any]: """Revoke a token immediately in ``alias``'s project.""" creds = self._resolve_project(alias) diff --git a/tests/test_client.py b/tests/test_client.py index d43dcdcd..8e6fc3eb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -3092,14 +3092,14 @@ def test_kill_job_500_404_mismatch(self, httpx_mock) -> None: missing jobs. Client passes the error through; service layer distinguishes the two cases via a follow-up GET. """ - # HTTP 500 triggers retry path (MAX_RETRIES total attempts); stub each. - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url="https://queue.keboola.com/jobs/1/kill", - method="POST", - json={"error": "Internal Server Error occurred.", "code": 404}, - status_code=500, - ) + # A kill is a POST, so the 500 is NOT retried -- one stub, one attempt + # (issue #599). Retrying it would be a second terminate command. + httpx_mock.add_response( + url="https://queue.keboola.com/jobs/1/kill", + method="POST", + json={"error": "Internal Server Error occurred.", "code": 404}, + status_code=500, + ) with ( KeboolaClient(stack_url=_BASE, token=_TOKEN) as client, @@ -3108,6 +3108,7 @@ def test_kill_job_500_404_mismatch(self, httpx_mock) -> None: client.kill_job("1") assert exc_info.value.status_code == 500 + assert len(httpx_mock.get_requests()) == 1 def test_kill_job_url_encodes_job_id(self, httpx_mock) -> None: """kill_job() URL-encodes the job ID to prevent path injection.""" diff --git a/tests/test_client_device_enrollment.py b/tests/test_client_device_enrollment.py index 547f1c22..2ce6717b 100644 --- a/tests/test_client_device_enrollment.py +++ b/tests/test_client_device_enrollment.py @@ -166,6 +166,50 @@ def test_refresh_returns_new_token_dict(self, httpx_mock) -> None: assert str(request.url) == f"{STACK_URL}/v2/storage/tokens/9001/refresh" +class TestListTokens: + def test_list_returns_raw_array(self, httpx_mock) -> None: + """GET /v2/storage/tokens -> the API's array, passed through verbatim.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + method="GET", + json=[ + {"id": "9001", "description": "device 42", "isMasterToken": False}, + {"id": "1", "description": "master", "isMasterToken": True}, + ], + status_code=200, + ) + + client = _make_client() + try: + result = client.list_tokens() + finally: + client.close() + + assert [t["id"] for t in result] == ["9001", "1"] + request = httpx_mock.get_requests()[0] + assert request.method == "GET" + assert str(request.url) == f"{STACK_URL}/v2/storage/tokens" + + def test_non_list_body_returns_empty(self, httpx_mock) -> None: + """A non-array body cannot be iterated as tokens -- degrade to empty. + + Guards the caller from a TypeError if the endpoint ever answers with an + envelope object instead of the bare array it documents today. + """ + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + method="GET", + json={"unexpected": "envelope"}, + status_code=200, + ) + + client = _make_client() + try: + assert client.list_tokens() == [] + finally: + client.close() + + # ---------------------------------------------------------------------------- # Per-device stream-source lifecycle # ---------------------------------------------------------------------------- diff --git a/tests/test_dev_portal_client.py b/tests/test_dev_portal_client.py index d47ab880..c845b3e9 100644 --- a/tests/test_dev_portal_client.py +++ b/tests/test_dev_portal_client.py @@ -292,14 +292,14 @@ def test_upload_icon_presign_failure(self, httpx_mock, monkeypatch): url="https://apps-api.keboola.com/auth/login", json={"token": "Bearer abc"}, ) - # Add 3 responses (MAX_RETRIES=3) since 500 is retryable. - for _ in range(3): - httpx_mock.add_response( - method="POST", - url="https://apps-api.keboola.com/vendors/keboola/apps/keboola.ex-foo/icon", - status_code=500, - json={"error": "boom"}, - ) + # One response: an icon upload is a POST, and a POST is no longer + # retried on 500 (issue #599). + httpx_mock.add_response( + method="POST", + url="https://apps-api.keboola.com/vendors/keboola/apps/keboola.ex-foo/icon", + status_code=500, + json={"error": "boom"}, + ) # Suppress retry sleeps. import keboola_agent_cli.http_base as http_base_module diff --git a/tests/test_e2e.py b/tests/test_e2e.py index c136d1cf..10d994f6 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -12675,7 +12675,7 @@ def _first_bucket_id(self) -> str | None: # ------------------------------------------------------------------ def test_scoped_token_mint_rotate_revoke(self) -> None: - """`token create` (scoped, expiring) -> `token refresh` -> `token delete`. + """`token create` (scoped, expiring) -> `token list` -> `token refresh` -> `token delete`. Never prints the secret; deletes the token the moment its shape is verified so no live credential outlives the test. @@ -12725,13 +12725,26 @@ def test_scoped_token_mint_rotate_revoke(self) -> None: assert bool(data.get("token")), "minted token must reveal a non-empty secret once" assert data.get("alias") == self.alias - _step(2, "token refresh", "rotate the secret; id is stable, old value dies") + _step(2, "token list", "the minted token is visible, and no row carries a secret") + listed = self._run_ok("token", "list", "--project", self.alias)["data"] + assert listed.get("alias") == self.alias + assert listed.get("count") == len(listed.get("tokens") or []) + rows = {str(row.get("id")): row for row in listed.get("tokens") or []} + assert token_id in rows, "the freshly minted token must appear in the listing" + assert rows[token_id].get("description", "").endswith("e2e device-enrollment token") + # The whole point of the strip: `create` is the only reveal, so not one + # row -- not even another project token's -- may carry a value. + assert all("token" not in row for row in rows.values()), ( + "token list must never carry a secret value" + ) + + _step(3, "token refresh", "rotate the secret; id is stable, old value dies") refreshed = self._run_ok( "token", "refresh", "--project", self.alias, "--token-id", token_id, "--yes" )["data"] assert bool(refreshed.get("token")), "rotated token must reveal a new non-empty secret" - _step(3, "token delete", "revoke immediately -- no live credential left behind") + _step(4, "token delete", "revoke immediately -- no live credential left behind") deleted = self._run_ok( "token", "delete", "--project", self.alias, "--token-id", token_id, "--yes" )["data"] diff --git a/tests/test_http_base.py b/tests/test_http_base.py index 7f68471b..7cf97bff 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -7,7 +7,12 @@ import httpx import pytest -from keboola_agent_cli.constants import APP_NAME, MAX_API_ERROR_LENGTH, MAX_RETRIES +from keboola_agent_cli.constants import ( + APP_NAME, + MAX_API_ERROR_LENGTH, + MAX_EXCEPTION_ID_LENGTH, + MAX_RETRIES, +) from keboola_agent_cli.errors import KeboolaApiError from keboola_agent_cli.http_base import BaseHttpClient, build_user_agent @@ -222,6 +227,356 @@ def test_alternate_client_parameter(self, httpx_mock) -> None: base_client.close() +class TestNonIdempotentRetryPolicy: + """A 5xx/transport failure on a non-idempotent method must NOT be retried. + + ``POST`` creates server-side state. Keboola's own token mint persists the + token row *before* the step that can fail, so a blind retry of a failed + ``POST /v2/storage/tokens`` can silently leave two live credentials behind + (issue #599). Only the RFC 9110 idempotent methods are safe to repeat. + """ + + def _client(self) -> BaseHttpClient: + return BaseHttpClient( + base_url=STACK_URL, + token=TOKEN, + headers={"Authorization": f"Bearer {TOKEN}"}, + ) + + def test_post_not_retried_on_500(self, httpx_mock) -> None: + """A POST answered with 500 fails on the first attempt -- no second mint.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/v2/storage/tokens") + assert exc_info.value.status_code == 500 + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_not_retried_on_502(self, httpx_mock) -> None: + """The gate is the method, not the specific 5xx code.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=502, text="Bad Gateway") + + client = self._client() + try: + with pytest.raises(KeboolaApiError): + client._do_request("POST", "/test-path") + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_patch_not_retried_on_500(self, httpx_mock) -> None: + """PATCH is not idempotent either (RFC 9110), so it gets the same gate.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=500, text="boom") + + client = self._client() + try: + with pytest.raises(KeboolaApiError): + client._do_request("PATCH", "/test-path") + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_still_retried_on_429(self, httpx_mock) -> None: + """429 means the server refused to process it -- repeating is safe.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("POST", "/test-path") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_put_retried_on_500(self, httpx_mock) -> None: + """PUT is idempotent, so it keeps the retry safety net.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=500, text="boom") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("PUT", "/test-path") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_delete_retried_on_503(self, httpx_mock) -> None: + """DELETE is idempotent -- repeating a delete converges on the same state.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=503, text="unavailable") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=204) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("DELETE", "/test-path") + assert response.status_code == 204 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_post_not_retried_on_timeout(self, httpx_mock) -> None: + """A timed-out POST may already have taken effect -- never repeat it.""" + httpx_mock.add_exception(httpx.ReadTimeout("Read timed out"), url=f"{STACK_URL}/test-path") + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert exc_info.value.error_code == "TIMEOUT" + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_retried_on_connect_error(self, httpx_mock) -> None: + """A refused connection never reached the server, so a POST may repeat.""" + httpx_mock.add_exception(httpx.ConnectError("Connection refused"), url=f"{STACK_URL}/x") + httpx_mock.add_response(url=f"{STACK_URL}/x", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("POST", "/x") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + +class TestServerErrorGuidance: + """A 5xx must tell the operator what to do next (issue #599).""" + + def _client(self) -> BaseHttpClient: + return BaseHttpClient( + base_url=STACK_URL, + token=TOKEN, + headers={"Authorization": f"Bearer {TOKEN}"}, + ) + + def test_unretried_500_names_the_partial_effect_risk(self, httpx_mock) -> None: + """The POST hint must warn the operation may already have taken effect.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/v2/storage/tokens") + message = exc_info.value.message + assert "POST" in message + assert "not retried" in message + assert "may already have taken effect" in message + finally: + client.close() + + def test_exhausted_500_points_at_an_upstream_incident(self, httpx_mock) -> None: + """A 500 that survives every retry is an incident, not a caller mistake.""" + for _ in range(MAX_RETRIES): + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + message = exc_info.value.message + assert f"{MAX_RETRIES} attempts" in message + assert "upstream Keboola incident" in message + assert "support" in message + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exception_id_surfaced_for_support(self, httpx_mock) -> None: + """Keboola's exceptionId is the only handle support can trace -- keep it.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={ + "error": "Application error.", + "exceptionId": "kbc-eu-central-1-connection-abc123", + "message": "Please contact our support", + }, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "kbc-eu-central-1-connection-abc123" in exc_info.value.message + finally: + client.close() + + def test_rate_limited_then_500_on_a_post_warns_about_partial_effect(self, httpx_mock) -> None: + """A POST can reach attempt 2 via a 429 -- the 5xx there is still its first. + + The hint must be the partial-effect warning, not "upstream incident": + telling an operator to escalate instead of checking what already + landed is exactly how the duplicate this PR prevents gets created. + """ + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", status_code=500, json={"error": "Application error."} + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "may already have taken effect" in message + assert "upstream Keboola incident" not in message + assert exc_info.value.retryable is False + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exhausted_hint_counts_server_errors_not_total_attempts(self, httpx_mock) -> None: + """A 429 burned an attempt but was not a 5xx -- do not count it as one.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + for _ in range(MAX_RETRIES - 1): + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", status_code=500, json={"error": "boom"} + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + message = exc_info.value.message + assert "upstream Keboola incident" in message + assert f"{MAX_RETRIES - 1} attempts" in message + assert f"{MAX_RETRIES} attempts" not in message + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exception_id_is_length_capped(self, httpx_mock) -> None: + """An untrusted id must not reach the terminal unbounded.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error.", "exceptionId": "a" * 5000}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "a" * MAX_EXCEPTION_ID_LENGTH in exc_info.value.message + assert "a" * (MAX_EXCEPTION_ID_LENGTH + 1) not in exc_info.value.message + finally: + client.close() + + def test_exception_id_markup_and_newlines_stripped(self, httpx_mock) -> None: + """Human-mode errors render through Rich with markup ON (output.py). + + A server-supplied id carrying brackets would be interpreted as markup, + and newlines would let it forge extra log lines (CWE-117). Neither may + survive into the message. + """ + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={ + "error": "Application error.", + "exceptionId": "kbc-1[bold red]spoof[/bold red]\nError: fake\x07", + }, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "[bold red]" not in message + assert "\n" not in message + assert "\x07" not in message + # the legitimate characters survive so support still gets a handle + assert "kbc-1" in message + finally: + client.close() + + def test_non_string_exception_id_ignored(self, httpx_mock) -> None: + """A numeric/object `exceptionId` is not an id -- drop it silently.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error.", "exceptionId": {"nested": 1}}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "exceptionId" not in exc_info.value.message + finally: + client.close() + + def test_no_hint_appended_to_client_errors(self, httpx_mock) -> None: + """A 4xx is the caller's problem -- the incident hint would be misleading.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=400, + json={"error": "Bad request"}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "not retried" not in message + assert "upstream Keboola incident" not in message + finally: + client.close() + + class TestBaseHttpClientErrorSanitization: """Verify message truncation and error mapping in the base class.""" diff --git a/tests/test_lib_device_enrollment.py b/tests/test_lib_device_enrollment.py index b79c4416..e131ccbe 100644 --- a/tests/test_lib_device_enrollment.py +++ b/tests/test_lib_device_enrollment.py @@ -253,3 +253,41 @@ def test_delegates_and_returns_none(self) -> None: assert client.delete_stream_source("cam01-src", branch_id="default") is None mock_kc.delete_stream_source.assert_called_once_with("cam01-src", branch_id="default") + + +class TestListTokens: + def test_returns_typed_entries(self) -> None: + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [ + { + "id": "12345", + "description": "device 42", + "created": "2026-08-01T10:00:00+0200", + "expires": None, + "isExpired": False, + "isMasterToken": False, + } + ] + result = _make_client(mock_kc).list_tokens() + assert len(result) == 1 + entry = result[0] + assert entry.id == "12345" + assert entry.description == "device 42" + assert entry.expires is None + assert entry.is_expired is False + assert entry.is_master_token is False + mock_kc.list_tokens.assert_called_once_with() + + def test_secret_never_reaches_the_caller(self) -> None: + """`create_scoped_token` is the one and only secret reveal.""" + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [ + {"id": "1", "description": "master", "token": "1-liveSecretValue"} + ] + result = _make_client(mock_kc).list_tokens() + assert "liveSecretValue" not in str(result[0].model_dump()) + + def test_empty_project(self) -> None: + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [] + assert _make_client(mock_kc).list_tokens() == [] diff --git a/tests/test_metastore_client.py b/tests/test_metastore_client.py index e12fa653..be87cee4 100644 --- a/tests/test_metastore_client.py +++ b/tests/test_metastore_client.py @@ -11,7 +11,6 @@ import pytest -from keboola_agent_cli.constants import MAX_RETRIES from keboola_agent_cli.errors import ErrorCode, KeboolaApiError from keboola_agent_cli.metastore_client import ( SEMANTIC_TYPES, @@ -189,13 +188,16 @@ def test_duplicate_name_409_becomes_already_exists(self, httpx_mock, metastore_c assert excinfo.value.retryable is False def test_duplicate_name_500_becomes_already_exists(self, httpx_mock, metastore_client) -> None: - """Legacy / pre-fix metastore still returns 500 -- retain the workaround.""" - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", - status_code=500, - json={"error": "Failed to create meta object: duplicate name 'foo'"}, - ) + """Legacy / pre-fix metastore still returns 500 -- retain the workaround. + + A single response: ``post_item`` is a POST, which is no longer retried + on a 5xx (issue #599), so normalisation has to happen on attempt one. + """ + httpx_mock.add_response( + url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", + status_code=500, + json={"error": "Failed to create meta object: duplicate name 'foo'"}, + ) with pytest.raises(KeboolaApiError) as excinfo: metastore_client.post_item("semantic-metric", name="foo", data={"name": "foo"}) assert excinfo.value.error_code == ErrorCode.ALREADY_EXISTS @@ -206,12 +208,11 @@ def test_duplicate_name_500_becomes_already_exists(self, httpx_mock, metastore_c def test_unrelated_500_passes_through(self, httpx_mock, metastore_client) -> None: """A 500 without the magic phrase keeps its API_ERROR code.""" - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", - status_code=500, - json={"error": "some unrelated internal error"}, - ) + httpx_mock.add_response( + url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", + status_code=500, + json={"error": "some unrelated internal error"}, + ) with pytest.raises(KeboolaApiError) as excinfo: metastore_client.post_item("semantic-metric", name="foo", data={"name": "foo"}) assert excinfo.value.error_code != ErrorCode.ALREADY_EXISTS diff --git a/tests/test_token_cli.py b/tests/test_token_cli.py index 80a8d117..5850f36a 100644 --- a/tests/test_token_cli.py +++ b/tests/test_token_cli.py @@ -188,3 +188,76 @@ def test_api_error_exit_code(self, tmp_path: Path) -> None: ) assert result.exit_code != 0 assert json.loads(result.output)["status"] == "error" + + +class TestList: + def _svc_with(self, tokens: list[dict]) -> MagicMock: + svc = MagicMock() + svc.list_tokens.return_value = { + "alias": ALIAS, + "count": len(tokens), + "tokens": tokens, + } + return svc + + def test_list_json(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = self._svc_with( + [ + { + "id": "12345", + "description": "device enrollment", + "created": "2026-08-01T10:00:00+0200", + "expires": "2026-09-01T10:00:00+0200", + "isExpired": False, + "isMasterToken": False, + } + ] + ) + result = _invoke(config_dir, svc, ["--json", "token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert payload["data"]["count"] == 1 + assert payload["data"]["tokens"][0]["id"] == "12345" + svc.list_tokens.assert_called_once_with(alias=ALIAS) + + def test_list_human_renders_rows(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = self._svc_with( + [ + {"id": "1", "description": "master token", "isMasterToken": True}, + {"id": "2", "description": "device enrollment", "isMasterToken": False}, + ] + ) + result = _invoke(config_dir, svc, ["token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + assert "device enrollment" in result.stdout + assert "12345" not in result.stdout + + def test_list_human_empty(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + result = _invoke(config_dir, self._svc_with([]), ["token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + assert "No tokens" in result.stdout + + def test_list_api_error_exits_nonzero(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = MagicMock() + svc.list_tokens.side_effect = KeboolaApiError( + message="Access denied", status_code=403, error_code="ACCESS_DENIED" + ) + result = _invoke(config_dir, svc, ["--json", "token", "list", "--project", ALIAS]) + # ACCESS_DENIED is a general error (1) in this CLI's exit-code map; + # only INVALID_TOKEN / session failures are the auth class (3). + assert result.exit_code == 1 + payload = json.loads(result.output) + assert payload["status"] == "error" + assert payload["error"]["code"] == "ACCESS_DENIED" diff --git a/tests/test_token_service.py b/tests/test_token_service.py index 65ef44c9..2a56393b 100644 --- a/tests/test_token_service.py +++ b/tests/test_token_service.py @@ -141,3 +141,54 @@ def test_unknown_alias_raises(self, store, client_factory) -> None: factory, _ = client_factory with pytest.raises(ConfigError): _svc(store, factory).refresh_token(alias="nope", token_id="1") + + +class TestListTokens: + def test_returns_alias_and_tokens(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.return_value = [ + {"id": "1", "description": "master", "isMasterToken": True}, + {"id": "2", "description": "device", "isMasterToken": False}, + ] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert result["alias"] == ALIAS + assert result["count"] == 2 + assert [t["id"] for t in result["tokens"]] == ["1", "2"] + factory.assert_called_once_with(STACK_URL, TOKEN) + mock.close.assert_called_once() + + def test_secret_values_are_stripped(self, store, client_factory) -> None: + """A project with `force-decrypted-token` returns live secrets in the list. + + `token create` reveals a secret ONCE by design; a listing that dumped + every live token's value to stdout would break that contract wholesale. + """ + factory, mock = client_factory + mock.list_tokens.return_value = [ + {"id": "1", "description": "master", "token": "1-liveSecretValue"}, + {"id": "2", "description": "device"}, + ] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert "token" not in result["tokens"][0] + assert "liveSecretValue" not in str(result) + # everything else survives untouched + assert result["tokens"][0]["description"] == "master" + + def test_empty_list(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.return_value = [] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert result["tokens"] == [] + assert result["count"] == 0 + + def test_unknown_alias_raises(self, store, client_factory) -> None: + factory, _ = client_factory + with pytest.raises(ConfigError): + _svc(store, factory).list_tokens(alias="nope") + + def test_client_closed_when_listing_raises(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.side_effect = RuntimeError("boom") + with pytest.raises(RuntimeError): + _svc(store, factory).list_tokens(alias=ALIAS) + mock.close.assert_called_once()