diff --git a/README.md b/README.md index e6db320..600c9f6 100644 --- a/README.md +++ b/README.md @@ -6,22 +6,24 @@ JSON back. It also clones one organization's resources into another. ```bash curl -LsSf https://raw.githubusercontent.com/Zipstack/unstract-cli/main/install.sh | sh -export UNSTRACT_PLATFORM_KEY=... -unstract auth whoami # resolves and stores your organisation +unstract auth login # asks for your keys, checks them, stores them unstract docstudio deployment ls # what can I run? ``` +For an agent or CI, no prompts and no file — the environment is the profile: + +```bash +export UNSTRACT_ORG_ID=... UNSTRACT_DEPLOYMENT_KEY=... LLMWHISPERER_API_KEY=... +unstract -o json whisper extract ./doc.pdf +unstract -o json docstudio deployment run invoice-parser ./doc.pdf +``` + The installer fetches `uv` if it is missing and installs the CLI with it; `uv` brings its own Python, so nothing on the machine has to match. Already have `uv`? `uv tool install git+https://github.com/Zipstack/unstract-cli` is the same thing. Set `UNSTRACT_CLI_SOURCE` to install a branch or a local checkout instead. -`auth whoami` is the shortest way in: a platform key carries the organisation it -belongs to, so supplying the key is enough to discover `org_id` rather than -reading it out of a web-app URL. `config init` and `config doctor` are still -there for a profile you write by hand. - Or run it without installing: `uvx --from git+https://github.com/Zipstack/unstract-cli unstract --discover groups`. ## Output @@ -69,6 +71,26 @@ have to copy it: | 10 | the result was read but could not be saved — it is in `error.details` | | 130 | interrupted (128 + SIGINT) — the user stopped it, not a failure | +## Credentials + +Three keys, each for one job: + +- **LLMWhisperer key** — extracts text (`whisper …`). Minted in the LLMWhisperer + console. +- **Deployment key** — runs deployments (`deployment run`, `deployment status`). + Shown on the API deployment's own page in the Unstract UI; one minted under + **Settings → API Key Manager** covers every deployment in the organisation. +- **Platform key** — identifies the organisation and lists what is in it + (`auth whoami`, `deployment ls`). Minted by an organisation admin under + **Settings → Platform API Keys**. + +`auth login` takes whichever of the three you have, checks the two it can +(`whoami` for the platform key, the usage endpoint for the LLMWhisperer key; a +deployment key has nothing side-effect-free to call and is stored as given) and +writes them to one profile. Run it again to rotate a key. Without a terminal +pass them as flags — `--platform-key`, `--deployment-key`, `--llmwhisperer-key`, +any one of them `-` to read from stdin. + ## Configuration `~/.unstract/config.toml`, or a project-local `.unstract.toml` found by upward @@ -76,8 +98,8 @@ search, or `$UNSTRACT_CONFIG`, or `--config`. Every setting resolves **flag > env > profile > built-in default**, and the CLI is fully usable with no config file at all. The flag tier is the connection options on each product group — `unstract docstudio --base-url … --org-id … deployment run …`, and -`--base-url`/`--api-key` on `whisper` and on `auth` — which override the profile -for that one invocation without writing anything. +`--base-url`/`--api-key` on `whisper`, `--platform-key` on `auth` — which +override the profile for that one invocation without writing anything. ```toml default_profile = "cloud-us" @@ -90,66 +112,46 @@ api_key = "env:LLMWHISPERER_API_KEY" base_url = "https://us-central.unstract.com" org_id = "org_ABC123" api_key = "env:UNSTRACT_DEPLOYMENT_KEY" +platform_key = "env:UNSTRACT_PLATFORM_KEY" -[profiles.cloud-us.platform] -base_url = "https://us-central.unstract.com" - -[profiles.cloud-us.deployments.invoices] -api_name = "invoice-parser" +# Only for a deployment whose key differs from the one above. +[profiles.cloud-us.deployments."invoice-parser"] +api_key = "env:INVOICE_PARSER_KEY" ``` -One `api_key` on the `docstudio` block covers every alias under it: a key minted -under **Settings → API Key Manager** authenticates every API deployment in the -organisation, so an alias normally carries only its `api_name`. Give an alias its -own `api_key` when its deployment has a separate key of its own. - -An alias sits outside the flag tier for the settings it states itself. Where an -alias names its own `org_id` or `api_key`, those are the ones used and -`--org-id`/`--api-key` do not displace them — the flags fill in only what the -alias leaves to the profile. `--base-url` is not per-alias and always applies, -which is what points a profile's aliases at another host. - -Get an LLMWhisperer key from the LLMWhisperer console; a deployment key is shown -on the API deployment's own page in the Unstract UI, and an organisation-wide one -under Settings → API Key Manager. A **platform key** is minted by an -organisation admin under Settings → Platform API Keys. `config init` also writes -an `onprem-example` profile as a shape to copy for a self-hosted install — its -host is a placeholder, and only the *active* profile is ever resolved. - -The two Unstract keys are not interchangeable and neither replaces the other. A -deployment key runs deployments and cannot say which organisation it belongs -to; a platform key identifies the organisation and lists what is in it, and -cannot run a deployment. `auth whoami` and `deployment ls` take the platform key; -`deployment run` and `deployment status` take the deployment key; `auth whoami` -and `deployment ls` take the platform key. `org_id` lives on the `docstudio` -block either way — `auth whoami` writes the one it resolves there, because that -is where everything that needs it reads from. - -`config init` deliberately leaves `platform.api_key` out of the block above, so -that a caller who only holds a deployment key is not told a platform key is -missing. Add the line, or set `$UNSTRACT_PLATFORM_KEY`, when you have one. - -A credential can be written into the file literally, but `env:VAR_NAME` -indirection is what `config init` writes and what the examples use: the file -then records where a secret lives rather than the secret itself, and stays safe -to copy or commit. Either way the file is created `0600`, and `config doctor` -warns when its mode is wider than that. +`deployment run` and `deployment status` take the API name as `deployment ls` +prints it. The key for a run resolves `--api-key` > `$UNSTRACT_DEPLOYMENT_KEY` > +the deployment's own entry > the profile's `api_key`, so most profiles need no +`deployments` section at all; `config set docstudio api_key --deployment +` writes one. `org_id` lives on the `docstudio` block — `auth login` +and `auth whoami` write the one the platform key resolves there. `config init` +writes this shape minus `platform_key` and the `deployments` entry — both are +the exception, not the starting point — plus an `onprem-example` profile to +copy for a self-hosted install; only the *active* profile is ever resolved. + +`auth login` writes keys literally; `env:VAR_NAME` indirection is what +`config init` writes and what the example uses, so the file records where a +secret lives rather than the secret itself and stays safe to copy or commit. +Either way the file is created `0600`, and `config doctor` warns when its mode +is wider than that. `unstract config doctor` reports where each setting resolved from — including whether an `env:` reference is actually set in the current process — without -echoing any value. It exits non-zero when one of its own checks failed, so a -setup script can branch on it. - -A project-local `.unstract.toml` **found by upward search** may not supply -`api_key` or `base_url`. Those are ignored, with a warning; everything else in it -— profile selection, `org_id`, deployment aliases — applies as usual. A checkout -you did not write is not trusted to name the host your key is sent to. Name the -file explicitly (`--config` or `$UNSTRACT_CONFIG`) and it is honoured in full. - -What that protects is the key and the host, not the routing: `org_id`, -`api_name` and profile selection stay repo-controllable by design, so a -project file can still decide *which* deployment a command runs against on a -host you trust. Read one before you run inside a checkout you did not write. +echoing any value. `--probe` also checks the keys against the service and, with +a platform key, warns about a `deployments` entry the organisation no longer +has. It exits non-zero when one of its own checks failed, so a setup script can +branch on it. + +A project-local `.unstract.toml` **found by upward search** may not supply a +key or `base_url`. Those are ignored, with a warning; everything else in it — +profile selection, `org_id` — applies as usual. A checkout you did not write is +not trusted to name the host your key is sent to. Name the file explicitly +(`--config` or `$UNSTRACT_CONFIG`) and it is honoured in full. + +What that protects is the key and the host, not the routing: `org_id` and +profile selection stay repo-controllable by design, so a project file can still +decide *which* organisation a command runs against on a host you trust. Read +one before you run inside a checkout you did not write. `clone` is the exception, and it is an operator command: a human moving one organisation's resources into another, holding two admin Platform keys. It is @@ -157,10 +159,10 @@ not part of the document-processing path the rest of this CLI wraps, so an agent serving a user request should not reach for it unasked. It talks to two deployments at once, which no single profile describes, so it takes both endpoints as flags and both keys from `UNSTRACT_SRC_PLATFORM_KEY` / -`UNSTRACT_TGT_PLATFORM_KEY` — two keys for two organisations, so it reads neither -the `platform` profile block nor `$UNSTRACT_PLATFORM_KEY`. It exits 0 when nothing failed, which is not the -same as everything having moved: oversize and unsupported documents are skipped -by design, and `data.skipped` counts them. +`UNSTRACT_TGT_PLATFORM_KEY` — two keys for two organisations, so it reads +neither the profile's `platform_key` nor `$UNSTRACT_PLATFORM_KEY`. It exits 0 +when nothing failed, which is not the same as everything having moved: oversize +and unsupported documents are skipped by design, and `data.skipped` counts them. ## Development diff --git a/src/unstract_cli/app.py b/src/unstract_cli/app.py index e807818..51d4673 100644 --- a/src/unstract_cli/app.py +++ b/src/unstract_cli/app.py @@ -16,7 +16,7 @@ from unstract_cli.config import ( DOCSTUDIO, LLMWHISPERER, - PLATFORM, + SECRET_SETTINGS, ConfigError, ResolvedConfig, load_config, @@ -73,7 +73,7 @@ def override(self, product: str, values: dict[str, Any]) -> None: for key, value in values.items(): if value is None: continue - if key == "api_key": + if key in SECRET_SETTINGS: diagnostic( "warning: a key passed on the command line lands in shell " "history and in the process list. Prefer the environment " @@ -86,9 +86,13 @@ def override(self, product: str, values: dict[str, Any]) -> None: def secrets(self) -> list[str]: """Resolved credentials, for scrubbing anything on its way to a stream.""" out: list[str] = [] - for product in (LLMWHISPERER, DOCSTUDIO, PLATFORM): + for product, key in ( + (LLMWHISPERER, "api_key"), + (DOCSTUDIO, "api_key"), + (DOCSTUDIO, "platform_key"), + ): try: - if value := self.config.get(product, "api_key"): + if value := self.config.get(product, key): out.append(str(value)) except (ConfigError, CLIError): # A credential that cannot be resolved is one that cannot be @@ -201,20 +205,28 @@ def cli( ctx.exit(int(ExitCode.SUCCESS)) -def _connection_options(*, org_id: bool = False) -> Callable[[Any], Any]: +#: The connection flags a product group can carry, named after the setting +#: each one overrides. +_CONNECTION_FLAGS: dict[str, str] = { + "base_url": "Service URL to use.", + "api_key": "API key to use.", + "org_id": "Organisation to run against.", + "platform_key": "Platform key to use, for the commands that take one.", +} + + +def _connection_options(*settings: str) -> Callable[[Any], Any]: """The per-product connection settings, as flags. They sit on the product group rather than on each command: they say where to connect, which is the same question for every command underneath. """ options = [ - click.option("--base-url", default=None, help="Service URL to use."), - click.option("--api-key", default=None, help="API key to use."), - ] - if org_id: - options.append( - click.option("--org-id", default=None, help="Organisation to run against.") + click.option( + f"--{name.replace('_', '-')}", default=None, help=_CONNECTION_FLAGS[name] ) + for name in ("base_url", *settings) + ] def decorate(func: Any) -> Any: for option in reversed(options): @@ -225,7 +237,7 @@ def decorate(func: Any) -> Any: @cli.group("whisper") -@_connection_options() +@_connection_options("api_key") @pass_context def whisper_group(ctx: Context, **overrides: str | None) -> None: """Extract text and layout from documents with LLMWhisperer.""" @@ -233,7 +245,7 @@ def whisper_group(ctx: Context, **overrides: str | None) -> None: @cli.group("docstudio") -@_connection_options(org_id=True) +@_connection_options("api_key", "org_id", "platform_key") @click.option( "--transport-timeout", type=click.FloatRange(min=0), @@ -258,7 +270,7 @@ def deployment_group() -> None: @cli.group("auth") -@_connection_options() +@_connection_options("platform_key") @click.option( "--transport-timeout", type=float, @@ -270,14 +282,14 @@ def deployment_group() -> None: def auth_group( ctx: Context, transport_timeout: float | None, **overrides: str | None ) -> None: - """Identify the credential you are using. + """Sign in, and identify the credential you are using. Its flags configure the platform key, which is the credential that knows which organisation it belongs to. A deployment key does not: it authenticates against the deployment it was minted for and never reaches this endpoint. """ ctx.transport_timeout = transport_timeout - ctx.override(PLATFORM, overrides) + ctx.override(DOCSTUDIO, overrides) cli.add_command(config_group) diff --git a/src/unstract_cli/commands/config_cmd.py b/src/unstract_cli/commands/config_cmd.py index c3e90a8..f278e37 100644 --- a/src/unstract_cli/commands/config_cmd.py +++ b/src/unstract_cli/commands/config_cmd.py @@ -18,7 +18,6 @@ DOCSTUDIO, KEY_SOURCES, LLMWHISPERER, - PLATFORM, PRODUCTS, UNTRUSTED_PROJECT_KEYS, ConfigError, @@ -38,7 +37,11 @@ emit_result, resolve_format, ) -from unstract_cli.core.platform import platform_client +from unstract_cli.core.platform import organisation, platform_client + +#: The probe entry for docstudio's platform key. Named for the credential, +#: not the product: the deployment key sits beside it under `docstudio`. +PLATFORM_KEY = "platform" #: Keys whose value is never echoed back, even on explicit request: this output #: is as likely to land in a log or a transcript as on a screen. @@ -80,12 +83,17 @@ def config_group() -> None: """Local configuration management. These commands make no network calls.""" -@config_group.command("init", help="Create a starter config file with profile stubs.") +@config_group.command("init") @click.option( "--force", is_flag=True, default=False, help="Overwrite an existing config file." ) @click.pass_obj def config_init(obj: Any, force: bool) -> None: + """Create a starter config file with profile stubs. + + The stubs reference environment variables and hold no keys. To store your + keys and be done, run `unstract auth login` instead. + """ path = init_path() if path.exists() and not force: # Never prompt: state the situation and the exact flag that resolves it. @@ -108,7 +116,8 @@ def config_init(obj: Any, force: bool) -> None: "replaced_existing": replaced, "note": ( "Credentials use env: indirection, so this file holds no secrets. " - "Set the referenced environment variables to authenticate. " + KEY_SOURCES + "Set the referenced environment variables to authenticate, or run " + "`unstract auth login` to store keys in a profile. " + KEY_SOURCES ), }, _fmt(obj), @@ -173,8 +182,21 @@ def config_get(obj: Any, product: str, key: str) -> None: @click.argument("key") @click.argument("value") @click.option("--profile", "-p", "profile", default=None, help="Profile to write to.") +@click.option( + "--deployment", + default=None, + metavar="API_NAME", + help="Store a docstudio api_key for this one deployment only.", +) @click.pass_obj -def config_set(obj: Any, product: str, key: str, value: str, profile: str | None) -> None: +def config_set( + obj: Any, + product: str, + key: str, + value: str, + profile: str | None, + deployment: str | None, +) -> None: """Set a value in the config file. PRODUCT, KEY and VALUE are positional -- not flags. Writes to the active @@ -184,6 +206,7 @@ def config_set(obj: Any, product: str, key: str, value: str, profile: str | None Examples: unstract config set docstudio org_id org_ABC123 unstract config set llmwhisperer api_key 'env:LLMWHISPERER_API_KEY' + unstract config set docstudio api_key dk_... --deployment invoice-parser \b Prefer `env:VAR_NAME` for credentials: the file then records where the secret @@ -192,10 +215,21 @@ def config_set(obj: Any, product: str, key: str, value: str, profile: str | None """ _check_product(product) _check_key(product, key) + if deployment is not None and (product, key) != (DOCSTUDIO, "api_key"): + raise CLIError( + "--deployment only applies to `docstudio api_key`.", + ExitCode.USAGE, + hint="A deployment entry holds nothing but the key that runs it.", + ) cfg = _loaded(obj) name = profile or getattr(obj, "profile", None) or cfg.default_profile or "cloud-us" - cfg.profiles.setdefault(name, {}).setdefault(product, {})[key] = value + block = cfg.profiles.setdefault(name, {}) + if deployment is not None: + block = block.setdefault("deployments", {}).setdefault(deployment, {}) + else: + block = block.setdefault(product, {}) + block[key] = value if not cfg.default_profile: cfg.default_profile = name written = save_config(cfg) @@ -228,6 +262,7 @@ def config_set(obj: Any, product: str, key: str, value: str, profile: str | None "profile": name, "product": product, "key": key, + "deployment": deployment, "path": str(written), "warning": warning, }, @@ -236,7 +271,7 @@ def config_set(obj: Any, product: str, key: str, value: str, profile: str | None def _probe(resolved: ResolvedConfig) -> dict[str, Any]: - """Check each product's credentials against the service, where that is possible. + """Check each credential against the service, where that is possible. LLMWhisperer has a read-only usage endpoint, so its key can be verified for real, and so does the platform API -- `whoami` reads nothing but the key @@ -244,6 +279,9 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: call is an execution -- so its entry reports that the settings resolve and says plainly that nothing was verified. Claiming otherwise would be worse than not checking. + + Keyed by credential rather than by product: docstudio holds two keys that + are checked differently. """ out: dict[str, Any] = {} try: @@ -269,7 +307,7 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: with translated(endpoint="whoami"): identity = platform_client(resolved).whoami() except CLIError as exc: - out[PLATFORM] = { + out[PLATFORM_KEY] = { "checked": True, "ok": False, "detail": exc.message, @@ -279,9 +317,9 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: # Null, not False: a platform key is optional -- a caller holding only a # deployment key is the common case -- so an absent one is a report # rather than a failure, and must not decide this command's exit code. - out[PLATFORM] = {"checked": False, "ok": None, "detail": str(exc)} + out[PLATFORM_KEY] = {"checked": False, "ok": None, "detail": str(exc)} else: - out[PLATFORM] = { + out[PLATFORM_KEY] = { "checked": True, "ok": True, # The organisation is the reason to hold this key, so the probe @@ -310,6 +348,24 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: return out +def _stale_deployments(resolved: ResolvedConfig, names: list[str]) -> list[str]: + """Which deployment entries name a deployment the server no longer has. + + An API name is editable server-side, so an entry written under one can be + orphaned without anything local changing. Raises when the listing cannot + be asked: a check that was requested and did not run must not read as a + check that passed. + """ + org_id = organisation(resolved) + client = platform_client(resolved, org_id) + with translated(endpoint="api/deployment/"): + return [ + name + for name in names + if not client.list_deployments(org_id, api_name=name).get("results") + ] + + @config_group.command("doctor", help="Diagnose how each setting resolves.") @click.option( "--probe/--no-probe", @@ -326,7 +382,9 @@ def config_doctor(obj: Any, probe: bool) -> None: login profile the CLI never inherited being the classic trap. Resolution is answered offline. --probe adds the second question -- does the - resolved key work -- which needs the network, so it is opt-in. + resolved key work -- which needs the network, so it is opt-in. With a + platform key it also checks that every deployment entry still names a + deployment the organisation has. Exits 0 only when nothing it checked failed. A setting that is simply not configured is a report, not a failure; a setting that points somewhere and @@ -351,28 +409,28 @@ def config_doctor(obj: Any, probe: bool) -> None: products[product] = entry try: - aliases = list(resolved.deployment_aliases()) + deployments = list(resolved.deployment_names()) except ConfigError as exc: - aliases = [] + deployments = [] problems.append(str(exc)) - for alias in aliases: - # An alias carries a key of its own, so it is a second place a project - # file can name one -- and it falls back to the profile's key silently. - if detail := resolved.withheld_detail("deployments", alias, "api_key"): - problems.append(f"deployment alias {alias}: {detail}") + for api_name in deployments: + # A deployment entry is a second place a project file can name a key -- + # and a run falls back to the profile's key silently. + if detail := resolved.withheld_detail("deployments", api_name, "api_key"): + problems.append(f"deployment {api_name}: {detail}") try: - # Resolved the way a run resolves it: that an alias is *listed* says - # nothing about whether the settings behind it arrive. - resolved.deployment(alias) + # Resolved the way a run resolves it: that an entry is *listed* says + # nothing about whether the key behind it arrives. + resolved.deployment_key(api_name) except ConfigError as exc: - problems.append(f"deployment alias {alias}: {exc}") + problems.append(f"deployment {api_name}: {exc}") report: dict[str, Any] = { "active_profile": resolved.active_profile, "config_path": str(resolved.file.path), "config_exists": resolved.file.exists, "products": products, - "deployment_aliases": aliases, + "deployments": deployments, } if any( not entry["api_key"]["resolved"] @@ -389,6 +447,35 @@ def config_doctor(obj: Any, probe: bool) -> None: for name, result in report["probe"].items() if result["ok"] is False ] + notes = [] + if deployments and not report["probe"][PLATFORM_KEY]["checked"]: + # The flag was explicit, so the skip is said rather than silent. + notes.append( + "probe: deployment entries were not checked against the " + f"organisation -- no platform key resolves for profile " + f"{resolved.active_profile!r}." + ) + elif deployments: + try: + stale = _stale_deployments(resolved, deployments) + except (CLIError, ConfigError) as exc: + problems.append(f"probe deployments: {exc}") + else: + # A warning, not a problem: the entry is harmless until it is + # run, and the server is the only authority on what it is + # called now. + report["stale_deployments"] = stale + notes += [ + f"warning: no deployment is called {api_name!r} any more; " + "run `unstract docstudio deployment ls` for the current names." + for api_name in stale + ] + for note in notes: + diagnostic( + note, + quiet=getattr(obj, "quiet", False), + verbosity=getattr(obj, "verbosity", 0), + ) if problems: report["problems"] = problems diff --git a/src/unstract_cli/commands/docstudio_cmd.py b/src/unstract_cli/commands/docstudio_cmd.py index 1db9196..549218b 100644 --- a/src/unstract_cli/commands/docstudio_cmd.py +++ b/src/unstract_cli/commands/docstudio_cmd.py @@ -17,7 +17,7 @@ from unstract_cli.commands.common import finish, raw_fields, wait_options from unstract_cli.core.clients import ( deployment, - naming_aliases, + deployment_errors, raise_for_result, translated, translating, @@ -85,9 +85,9 @@ def run( ) -> None: """Run a deployment against one or more documents. - TARGET is a deployment alias or an API name. Name the documents as local - FILES, as --presigned-urls, or both. With --wait (the default) this polls - until the execution finishes and returns its result. + TARGET is the deployment's API name, as `deployment ls` prints it. Name the + documents as local FILES, as --presigned-urls, or both. With --wait (the + default) this polls until the execution finishes and returns its result. """ sent = requested(params) # Before the client is built: what the caller typed is wrong whatever the @@ -113,7 +113,7 @@ def run( ) if save: preflight(save) - with naming_aliases(ctx.config, target), translated(endpoint=client.api_url): + with deployment_errors(target), translated(endpoint=client.api_url): # Queued execution, so the request returns a handle instead of holding # the connection open for the length of the job. started = client.structure_file(list(files), timeout=0, **sent) @@ -265,7 +265,7 @@ def status( # Quoted rather than trusted: the id comes from the caller and would # otherwise be able to carry query syntax of its own. endpoint = f"{client.api_url}?execution_id={quote(execution_id, safe='')}" - with naming_aliases(ctx.config, target), translated(endpoint=client.api_url): + with deployment_errors(target), translated(endpoint=client.api_url): result = client.check_execution_status(endpoint, **requested(params)) if not result.get("pending"): raise_for_result(result, endpoint=client.api_url) diff --git a/src/unstract_cli/commands/platform_cmd.py b/src/unstract_cli/commands/platform_cmd.py index 11d206c..e7baf28 100644 --- a/src/unstract_cli/commands/platform_cmd.py +++ b/src/unstract_cli/commands/platform_cmd.py @@ -1,9 +1,10 @@ -"""`unstract auth whoami` and `unstract docstudio deployment ls`. +"""`unstract auth login`, `unstract auth whoami` and `unstract docstudio deployment ls`. -Both authenticate with a platform key rather than a deployment key. The two -credentials are not interchangeable and neither is going away: a deployment key -runs deployments and cannot describe the account, a platform key describes the -account and lists what is in it but cannot run anything. +`whoami` and `ls` authenticate with a platform key rather than a deployment +key. The two credentials are not interchangeable and neither is going away: a +deployment key runs deployments and cannot describe the account, a platform key +describes the account and lists what is in it but cannot run anything. `login` +stores either, and the LLMWhisperer key, into one profile. No OpenAPI spec is vendored for the platform API, so these declare their flags by hand rather than through `spec_options`. @@ -11,14 +12,25 @@ from __future__ import annotations +import re +import sys from typing import Any import click from unstract_cli.app import Context, auth_group, deployment_group, pass_context from unstract_cli.commands.common import finish -from unstract_cli.config import DOCSTUDIO, ConfigError, load_config, save_config -from unstract_cli.core.clients import translated +from unstract_cli.config import ( + DOCSTUDIO, + KEY_SOURCES, + LLMWHISPERER, + ConfigError, + ConfigFile, + ResolvedConfig, + load_config, + save_config, +) +from unstract_cli.core.clients import llmwhisperer, translated from unstract_cli.core.errors import CLIError, ExitCode from unstract_cli.core.output import diagnostic from unstract_cli.core.platform import organisation, platform_client @@ -45,35 +57,36 @@ def __init__(self, reason: str, hint: str) -> None: self.hint = hint -def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: - """Write the resolved organisation into the profile the run is using. - - The profile name comes from `ResolvedConfig.active_profile`, which is the - same flag > env > file-default ladder every read uses. Re-deriving it here - is what dropped the `$UNSTRACT_PROFILE` tier, so the organisation was - written into a profile no later command read. - - It lands on the docstudio block because that is where every consumer reads - it from -- deployment URLs and aliases both -- and a second copy under the - platform block would be one more thing to keep in agreement. - """ +def _writable_config() -> ConfigFile: + """The config file a command may write to, or a refusal saying why not.""" cfg = load_config() if cfg.is_project_local: # A `.unstract.toml` found by walking up from the working directory is # very likely committed. Rewriting it would replace a teammate's # `org_id` with this caller's, drop every comment (the file is # re-serialised, not patched) and narrow its mode to 0600 -- a dirty, - # mode-changed, semantically different tracked file, from a command - # named `whoami`. The config layer already declines to *trust* this - # file for credentials; declining to *write* it is the same judgement. + # mode-changed, semantically different tracked file. The config layer + # already declines to *trust* this file for credentials; declining to + # *write* it is the same judgement. raise SaveDeclinedError( f"the config at {cfg.path} is project-local", - hint="Nothing was written. Rerun with --no-save to silence this, " - "or store it elsewhere: `unstract --config config set " - f"docstudio org_id {org_id}`.", + hint="Nothing was written. Name the file to write instead: " + "`unstract --config ...`.", ) + return cfg + + +def _profile_to_write( + ctx: Context, cfg: ConfigFile, name: str | None = None, *, create: bool = False +) -> str: + """The profile a write lands in: the one named, else the one the run is using. - selected = ctx.config.active_profile or cfg.default_profile + The fallback is `ResolvedConfig.active_profile`, the same flag > env > + file-default ladder every read uses. Re-deriving it here is what dropped the + `$UNSTRACT_PROFILE` tier once, so the organisation was written into a + profile no later command read. + """ + selected = name or ctx.config.active_profile or cfg.default_profile if selected is None and cfg.exists and cfg.profiles: # Neither the caller nor the file named one, so the "cloud-us" literal # below is this function's own invention -- refusing under that name @@ -88,8 +101,8 @@ def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: f"(known profiles: {known}); rerun with `-p `" ) - name = selected or "cloud-us" - if cfg.exists and cfg.profiles and name not in cfg.profiles: + selected = selected or "cloud-us" + if not create and cfg.exists and cfg.profiles and selected not in cfg.profiles: # `setdefault` would create it. That is not a convenience: the profile # lookup raises "Profile not found" for a typo today, and materialising # the name silently disarms that check for every later command, which @@ -99,16 +112,296 @@ def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: # the identity back: the key was resolved, only the note-taking failed. known = ", ".join(sorted(cfg.profiles)) or "none" raise ConfigError( - f"profile {name!r} is not in {cfg.path} " + f"profile {selected!r} is not in {cfg.path} " f"(known profiles: {known}); create it with `config set` first" ) + return selected + +def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: + """Write the resolved organisation into the profile the run is using. + + It lands on the docstudio block because that is where every consumer reads + it from. + """ + cfg = _writable_config() + name = _profile_to_write(ctx, cfg) cfg.profiles.setdefault(name, {}).setdefault(DOCSTUDIO, {})["org_id"] = org_id if not cfg.default_profile: cfg.default_profile = name return {"profile": name, "path": str(save_config(cfg))} +# --------------------------------------------------------------------------- # +# auth login +# --------------------------------------------------------------------------- # + +#: Each credential `login` takes, in the order it asks for them: its flag, the +#: prompt a terminal sees, and where in a profile it is stored. +_CREDENTIALS = ( + ("platform", "--platform-key", "Platform key", (DOCSTUDIO, "platform_key")), + ("deployment", "--deployment-key", "Deployment key", (DOCSTUDIO, "api_key")), + ("llmwhisperer", "--llmwhisperer-key", "LLMWhisperer key", (LLMWHISPERER, "api_key")), +) + + +def _interactive() -> bool: + return sys.stdin.isatty() + + +def _prompt(text: str, **kwargs: Any) -> Any: + # Prompts go to stderr so `-o json` output on stdout stays parseable. + return click.prompt(text, err=True, **kwargs) + + +def _confirm(text: str, **kwargs: Any) -> bool: + return click.confirm(text, err=True, **kwargs) + + +def _keys_from_flags(given: dict[str, str | None]) -> dict[str, str | None]: + """Flag values as keys, with `-` read from stdin -- at most one of them.""" + from_stdin = [name for name, value in given.items() if value == "-"] + if len(from_stdin) > 1: + flags = ", ".join(flag for name, flag, *_ in _CREDENTIALS if name in from_stdin) + raise CLIError( + f"Only one key can be read from stdin, and {flags} each ask for it.", + ExitCode.USAGE, + hint="Pass the others as values.", + ) + keys = dict(given) + if from_stdin: + keys[from_stdin[0]] = sys.stdin.read().strip() + return keys + + +def _keys_from_prompts() -> dict[str, str | None]: + """One hidden, skippable prompt per credential, in the documented order.""" + keys: dict[str, str | None] = {} + for name, _flag, label, _setting in _CREDENTIALS: + keys[name] = ( + _prompt( + f"{label} (Enter to skip)", + hide_input=True, + default="", + show_default=False, + ).strip() + or None + ) + return keys + + +def _validation_config( + ctx: Context, cfg: ConfigFile, name: str, keys: dict[str, str | None] +) -> ResolvedConfig: + """The keys being stored, resolved as the profile they will land in. + + Validating through the ordinary config layer means the same base URL the + profile will run against is the one the keys are checked against. A + profile that does not exist yet resolves against nothing but the flags and + the environment, so a stranger's host cannot be the one that answers. + """ + overrides = dict(ctx.overrides) + for credential, _flag, _label, (product, key) in _CREDENTIALS: + if keys.get(credential): + overrides[f"{product}.{key}"] = keys[credential] + if name in cfg.profiles: + return ResolvedConfig(file=cfg, profile_name=name, overrides=overrides) + return ResolvedConfig(file=ConfigFile(), profile_name=None, overrides=overrides) + + +def _slug(text: str) -> str: + return re.sub(r"[^a-z0-9]+", "-", text.lower()).strip("-") + + +def _new_profile_name(cfg: ConfigFile, org_id: str, org_name: Any) -> str: + """Ask for the profile to hold this organisation, until the answer is safe. + + The profile named here is replaced, not merged into: its credentials were + never checked against this key's host. A typed name that already exists + therefore needs a confirmation of its own -- all the more when it belongs + to another organisation, or the guard's remedy would be the overwrite it + exists to prevent. + """ + suggested = _slug(str(org_name or "")) or org_id + while True: + name = _prompt("Profile name", default=suggested) + if name not in cfg.profiles: + return name + other = cfg.profiles[name].get(DOCSTUDIO, {}).get("org_id") + owner = f" and belongs to organisation {other}" if other else "" + if _confirm( + f"Profile {name!r} already exists{owner}. Replace it?", default=False + ): + return name + + +@auth_group.command("login") +@click.option("--profile", "-p", "profile", default=None, help="Profile to write.") +@click.option( + "--platform-key", + "platform", + default=None, + metavar="KEY", + help="The platform key to store; `-` reads it from stdin.", +) +@click.option( + "--deployment-key", + "deployment", + default=None, + metavar="KEY", + help="The deployment key to store; `-` reads it from stdin.", +) +@click.option( + "--llmwhisperer-key", + "llmwhisperer", + default=None, + metavar="KEY", + help="The LLMWhisperer key to store; `-` reads it from stdin.", +) +@click.option( + "--force", + is_flag=True, + default=False, + help="Overwrite a profile that belongs to a different organisation.", +) +@pass_context +def login(ctx: Context, profile: str | None, force: bool, **given: str | None) -> None: + """Store your keys in a profile, checking each one that can be checked. + + At a terminal this asks for each key in turn -- platform, deployment, + LLMWhisperer -- and Enter skips one; at least one is needed. Without a + terminal, pass the keys as flags, one of them as `-` to read it from stdin. + + \b + Examples: + unstract auth login + unstract auth login --profile staging + echo "$PLATFORM_KEY" | unstract auth login --platform-key - + unstract auth login --deployment-key "$DEPLOYMENT_KEY" --llmwhisperer-key - + + The platform key is checked with `whoami` and the organisation it resolves + is stored beside it; the LLMWhisperer key is checked against the usage + endpoint. A deployment key has no side-effect-free endpoint, so it is stored + as given and reported as unverified. Nothing is written until every check + has passed. Running it again replaces the keys given and keeps the rest. + + Exits 3 when a key is rejected and 2 when no key was given. + """ + if given["platform"] is None: + given["platform"] = ctx.overrides.get(f"{DOCSTUDIO}.platform_key") + if any(value is not None for value in given.values()): + keys = _keys_from_flags(given) + interactive = False + elif _interactive(): + keys = _keys_from_prompts() + interactive = True + else: + raise CLIError( + "No keys were given and stdin is not a terminal, so there is nothing " + "to ask for them with.", + ExitCode.USAGE, + hint="Pass --platform-key, --deployment-key or --llmwhisperer-key, as a " + "value or as `-` to read one of them from stdin.", + ) + if not any(keys.values()): + raise CLIError( + "No key was given; at least one is needed.", ExitCode.USAGE, hint=KEY_SOURCES + ) + + try: + cfg = _writable_config() + name = _profile_to_write(ctx, cfg, profile, create=True) + except SaveDeclinedError as exc: + raise CLIError( + f"Cannot store keys: {exc.reason}.", ExitCode.USAGE, hint=exc.hint + ) from exc + except ConfigError as exc: + raise CLIError(str(exc), ExitCode.USAGE) from exc + + resolved = _validation_config(ctx, cfg, name, keys) + timeout = getattr(ctx, "transport_timeout", None) + result: dict[str, Any] = {"profile": name, "path": None} + identity: dict[str, Any] = {} + if keys["platform"]: + with translated(endpoint="whoami"): + identity = platform_client(resolved, timeout=timeout).whoami() + result["organization_id"] = identity.get("organization_id") + result["organization_name"] = identity.get("organization_name") + if keys["llmwhisperer"]: + with translated(endpoint="get-usage-info"): + llmwhisperer(resolved).get_usage_info() + for credential, *_ in _CREDENTIALS: + if not keys[credential]: + result[credential] = "skipped" + elif credential == "deployment": + result[credential] = "stored" + else: + result[credential] = "verified" + if keys["deployment"]: + result["note"] = ( + "A deployment key has no side-effect-free endpoint to check it " + "against, so it was stored as given." + ) + + org_id = str(identity["organization_id"]) if identity.get("organization_id") else None + checked_as = name + existing = cfg.profiles.get(name, {}).get(DOCSTUDIO, {}).get("org_id") + if org_id and existing and existing != org_id: + # Silently overwriting would repoint every deployment entry in the + # profile at an organisation none of them belong to. + found = f"{org_id} ({identity.get('organization_name')})" + if not interactive and not force: + raise CLIError( + f"Profile {name!r} belongs to organisation {existing}, and this " + f"platform key belongs to {found}.", + ExitCode.USAGE, + hint=f"Pass --profile to write another profile, or --force " + f"to overwrite {name!r}.", + ) + if interactive and _confirm( + f"Profile {name!r} belongs to organisation {existing}; this key belongs " + f"to {found}. Create a new profile for it instead of overwriting?", + default=True, + ): + name = _new_profile_name(cfg, org_id, identity.get("organization_name")) + result["profile"] = name + cfg.profiles[name] = {} + + block = cfg.profiles.setdefault(name, {}) + for credential, _flag, _label, (product, key) in _CREDENTIALS: + if not keys[credential]: + continue + product_block = block.setdefault(product, {}) + product_block[key] = keys[credential] + # The key was checked against the host the run resolved -- a flag, or + # the profile the login started from. A profile that records any other + # host, or none, would send the key somewhere it was never checked. + if ( + name != checked_as + or "base_url" not in product_block + or ctx.overrides.get(f"{product}.base_url") + ): + product_block["base_url"] = resolved.get(product, "base_url") + if org_id: + block.setdefault(DOCSTUDIO, {})["org_id"] = org_id + if not cfg.default_profile: + cfg.default_profile = name + try: + result["path"] = str(save_config(cfg)) + except (OSError, ConfigError) as exc: + raise CLIError( + f"The keys were accepted but could not be written: {exc}", + ExitCode.SAVE_FAILED, + ) from exc + + diagnostic( + f"wrote profile {name!r} in {result['path']}", + quiet=ctx.quiet, + verbosity=ctx.verbosity, + ) + finish(ctx, result) + + @auth_group.command("whoami") @click.option( "--save/--no-save", @@ -224,9 +517,10 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: "`--api-key` on `docstudio` is a deployment key; " "`deployment ls` authenticates with a platform key.", ExitCode.USAGE, - hint="Drop the flag and set $UNSTRACT_PLATFORM_KEY, or add " - "`api_key` to the [profiles..platform] block. A deployment " - "key runs a deployment; a platform key describes the account.", + hint="Pass --platform-key instead, set $UNSTRACT_PLATFORM_KEY, or " + "add `platform_key` to the [profiles..docstudio] block. A " + "deployment key runs a deployment; a platform key describes the " + "account.", ) org_id = organisation(ctx.config) @@ -257,4 +551,4 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: ) -__all__ = ["SaveDeclinedError", "ls", "whoami"] +__all__ = ["SaveDeclinedError", "login", "ls", "whoami"] diff --git a/src/unstract_cli/config.py b/src/unstract_cli/config.py index 7a2ebdc..803f3e7 100644 --- a/src/unstract_cli/config.py +++ b/src/unstract_cli/config.py @@ -2,8 +2,7 @@ Two products with different hosts, different keys, and `org_id` as a URL *path segment* rather than a flag. Named profiles (kubectl/aws style) hold per-product -host, key and org, plus deployment aliases so a deployment can be named instead -of spelled out. +host, key and org, plus a key per deployment for the ones that need their own. The resolution chain -- **flag > env > profile > built-in default** -- is implemented once here and used by every parameter. It is never re-implemented @@ -32,16 +31,16 @@ LLMWHISPERER = "llmwhisperer" DOCSTUDIO = "docstudio" -PLATFORM = "platform" -PRODUCTS: tuple[str, ...] = (LLMWHISPERER, DOCSTUDIO, PLATFORM) +PRODUCTS: tuple[str, ...] = (LLMWHISPERER, DOCSTUDIO) + +#: Settings whose values are credentials: registered for scrubbing when +#: resolved, withheld from a discovered project file, never echoed. +SECRET_SETTINGS = frozenset({"api_key", "platform_key"}) #: Built-in defaults, lowest precedence. DEFAULT_BASE_URLS: dict[str, str] = { LLMWHISPERER: "https://llmwhisperer-api.us-central.unstract.com/api/v2", DOCSTUDIO: "https://us-central.unstract.com", - # The same host as docstudio: one deployment serves both the platform API - # and the deployments it manages. - PLATFORM: "https://us-central.unstract.com", } #: Environment variables per (product, setting), checked before the config file @@ -49,19 +48,16 @@ #: themselves read: an environment already set up for a client must not leave #: the CLI silently on its built-in default, which is production. #: -#: `platform` deliberately has no `org_id` of its own. A platform key carries -#: its organisation, and `auth whoami` writes the one it resolves to the -#: docstudio block -- the block everything else already reads. Two `org_id` -#: settings would mean two rows in `config doctor` that a user has to keep in -#: agreement by hand. +#: The platform key sits on the docstudio block beside the deployment key: one +#: deployment serves both the platform API and the deployments it manages, so +#: the two keys share a host and an organisation. ENV_VARS: dict[tuple[str, str], tuple[str, ...]] = { (LLMWHISPERER, "api_key"): ("LLMWHISPERER_API_KEY",), (LLMWHISPERER, "base_url"): ("LLMWHISPERER_BASE_URL", "LLMWHISPERER_BASE_URL_V2"), (DOCSTUDIO, "api_key"): ("UNSTRACT_DEPLOYMENT_KEY", "UNSTRACT_API_DEPLOYMENT_KEY"), (DOCSTUDIO, "base_url"): ("UNSTRACT_BASE_URL",), (DOCSTUDIO, "org_id"): ("UNSTRACT_ORG_ID",), - (PLATFORM, "api_key"): ("UNSTRACT_PLATFORM_KEY",), - (PLATFORM, "base_url"): ("UNSTRACT_BASE_URL",), + (DOCSTUDIO, "platform_key"): ("UNSTRACT_PLATFORM_KEY",), } @@ -81,10 +77,9 @@ def settings_for(product: str) -> tuple[str, ...]: """The settings a product actually has. - Products differ: `org_id` is a setting only for `docstudio` -- llmwhisperer - has no organisation, and `platform` reads docstudio's -- and reporting a - setting a user has no way to supply reads as a misconfiguration they cannot - fix. + Products differ: `org_id` and `platform_key` are settings only for + `docstudio` -- llmwhisperer has no organisation -- and reporting a setting a + user has no way to supply reads as a misconfiguration they cannot fix. """ return tuple(sorted(key for prod, key in ENV_VARS if prod == product)) @@ -202,7 +197,7 @@ def _deref(value: Any, *, allow_env: bool) -> Any: #: checkout the user did not write must not choose the host their key is sent #: to. Separately, and for every key, such a file may not name an environment #: variable to read either -- see `ResolvedConfig._env_refused`. -UNTRUSTED_PROJECT_KEYS = frozenset({"api_key", "base_url"}) +UNTRUSTED_PROJECT_KEYS = SECRET_SETTINGS | {"base_url"} @dataclass @@ -452,7 +447,7 @@ def unknown_settings(self, product: str) -> tuple[str, ...]: def get(self, product: str, key: str, default: Any = None) -> Any: """Resolve one setting: **flag > env > profile > built-in default**.""" value = self._resolve(product, key, default) - if key == "api_key": + if key in SECRET_SETTINGS: remember_secret(value) return value @@ -467,20 +462,13 @@ def get_explicit(self, product: str, key: str) -> Any: the caller's own choice as silence. """ value = self._explicit(product, key) - if key == "api_key": + if key in SECRET_SETTINGS: remember_secret(value) return value - def explicit_tiers(self, product: str, key: str) -> Iterator[Any]: + def _tiers(self, product: str, key: str) -> Iterator[Any]: """What each tier says, in order -- flag, env, profile -- unset as `None`. - For a setting two products share -- one deployment serves both, so - `base_url` is really one question asked twice -- picking a product first - and then walking its tiers inverts the precedence the whole config layer - promises: a profile value on the preferred product beats a *flag* on the - other. Walking tier by tier across both products keeps flag > env > - profile true regardless of which product a value was written under. - Lazy on purpose: reading the profile block resolves the profile name, which raises for one that does not exist. A caller answered by an earlier tier must not be failed by a later one it never consulted. @@ -495,7 +483,7 @@ def explicit_tiers(self, product: str, key: str) -> Iterator[Any]: def _explicit(self, product: str, key: str) -> Any: """The tiers a human supplied: flag, then environment, then profile.""" - return next((v for v in self.explicit_tiers(product, key) if v is not None), None) + return next((v for v in self._tiers(product, key) if v is not None), None) def _resolve(self, product: str, key: str, default: Any = None) -> Any: if (value := self._explicit(product, key)) is not None: @@ -550,66 +538,68 @@ def require(self, product: str, key: str) -> Any: hints.append(f"or add `{key}` to the [profiles..{product}] block") # `--api-key` exists but is not suggested: a secret on the command line # lands in shell history and in the process list. - if key != "api_key": + if key not in SECRET_SETTINGS: hints.append(f"or pass --{key.replace('_', '-')}") raise ConfigError( f"Missing required setting {product}.{key}. To fix: {'; '.join(hints)}." ) - def deployment(self, alias: str) -> dict[str, Any]: - """Resolve a deployment alias to its api_name, org and key. + def deployment_names(self) -> tuple[str, ...]: + """The deployments the active profile holds a key of its own for.""" + table = self._profile().get("deployments") + return tuple(sorted(table)) if isinstance(table, dict) else () - ``org_id`` and ``api_key`` are optional per alias and fall back to the - profile's Document Studio block, so the common case is one line per - deployment. + def deployment_key(self, api_name: str) -> Any: + """The key to run one deployment with, or ``None`` if nothing names one. + + **flag > env > per-deployment entry > profile key**. The entry is the + most specific value *within* the profile tier, not a tier of its own: a + file value that outranked the environment would let a stale entry hijack + a run the caller set up with ``$UNSTRACT_DEPLOYMENT_KEY``. """ - aliases = self._profile().get("deployments") - entry = aliases.get(alias) if isinstance(aliases, dict) else None - if not isinstance(entry, dict): - known = ( - ", ".join(sorted(aliases)) - if isinstance(aliases, dict) and aliases - else "none" - ) - raise ConfigError( - f"Deployment alias {alias!r} not found in profile " - f"{self.active_profile!r}. Known aliases: {known}." - ) - if not entry.get("api_name"): - raise ConfigError(f"Deployment alias {alias!r} has no `api_name`.") - api_key = self._alias_setting(alias, entry, "api_key") - remember_secret(api_key) - return { - "api_name": entry["api_name"], - "org_id": self._alias_setting(alias, entry, "org_id"), - "api_key": api_key, - } - - def _alias_setting(self, alias: str, entry: dict[str, Any], key: str) -> Any: - """One alias setting, falling back to the profile only where the alias is silent. + tiers = self._tiers(DOCSTUDIO, "api_key") + flag, env = next(tiers), next(tiers) + value = flag if flag is not None else env + if value is None: + value = self._entry_key(api_name) + if value is None: + value = next(tiers) + remember_secret(value) + return value + + def _entry_key(self, api_name: str) -> Any: + """The key the deployment's own entry names, if it names one. An ``env:`` reference that does not resolve is not silence. Falling back - there runs the deployment against the profile's organisation, with the - profile's key, and reports success. + there runs the deployment with the profile's key and reports success. """ - raw = entry.get(key) + table = self._profile().get("deployments") + entry = table.get(api_name) if isinstance(table, dict) else None + raw = entry.get("api_key") if isinstance(entry, dict) else None + # A discovered project file never gets this far: its keys are withheld + # at load time, so an `env:` reference seen here is always trusted. if isinstance(raw, str) and raw.startswith("env:"): - if value := _deref(raw, allow_env=self._env_allowed(raw)): + if value := _deref(raw, allow_env=True): return value - reason = ( - self._env_refusal_detail(raw) - if self._env_refused(raw) - else f"${raw[4:].strip()} is not set in this process's environment" - ) raise ConfigError( - f"Deployment alias {alias!r} sets {key} to {raw!r}, and {reason}." + f"Deployment {api_name!r} sets api_key to {raw!r}, and " + f"${raw[4:].strip()} is not set in this process's environment." ) - return raw or self.get(DOCSTUDIO, key) + return raw or None + + def deployment_key_sources(self, api_name: str) -> tuple[str, ...]: + """Every place a key for this deployment could have come from, in order. - def deployment_aliases(self) -> tuple[str, ...]: - """Names of the deployment aliases defined in the active profile.""" - aliases = self._profile().get("deployments") - return tuple(sorted(aliases)) if isinstance(aliases, dict) else () + Quoted when none of them did: a caller told only that a key is missing + has to guess which of four places the CLI looked in. + """ + profile = self.active_profile or "" + return ( + "--api-key", + f"${ENV_VARS[(DOCSTUDIO, 'api_key')][0]}", + f'[profiles.{profile}.deployments."{api_name}"] api_key', + f"[profiles.{profile}.docstudio] api_key", + ) def resolution_source(self, product: str, key: str) -> dict[str, Any]: """Report where a setting resolves from, without echoing a secret. @@ -659,7 +649,7 @@ def withheld_detail(self, *trail: str) -> str | None: Reporting only where a value came *from* would leave the user staring at a setting they can see in the file. Takes a trail rather than a - product/key pair so a deployment alias's own key -- nested a level deeper + product/key pair so a deployment entry's own key -- nested a level deeper -- is answerable too. """ if (self.active_profile, *trail) not in self.file.withheld: @@ -676,10 +666,11 @@ def starter_profiles() -> dict[str, dict[str, Any]]: Every credential uses ``env:`` indirection: the generated file is a map of where secrets live, never a copy of them. - One key on the product block, and aliases that carry only ``api_name``: a - key can cover every deployment in the organisation, so a key per alias is - the exception -- for an organisation whose deployments hold separate keys -- - rather than the shape to start from. + No ``platform_key`` and no ``deployments`` table on purpose: a platform key + is optional and an ``env:`` reference to an unset variable is a `config + doctor` problem, and one deployment key normally covers the organisation, + so a per-deployment key is the exception rather than the shape to start + from. """ return { "cloud-us": { @@ -692,12 +683,6 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, - # No `api_key` on purpose. A platform key is optional -- holding - # only a deployment key is the common case -- and an `env:` - # reference to an unset variable is a `config doctor` problem, - # which would exit 1 for every user who does not hold one. - PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}, - "deployments": {"example": {"api_name": "your-api-deployment-name"}}, }, "cloud-eu": { LLMWHISPERER: { @@ -717,8 +702,6 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, - # No `api_key` -- see the cloud-us block. - PLATFORM: {"base_url": "https://unstract.internal.example"}, }, } @@ -730,9 +713,9 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "HOME_CONFIG", "KEY_SOURCES", "LLMWHISPERER", - "PLATFORM", "PRODUCTS", "PROJECT_CONFIG_NAME", + "SECRET_SETTINGS", "UNTRUSTED_PROJECT_KEYS", "ConfigError", "ConfigFile", diff --git a/src/unstract_cli/core/clients.py b/src/unstract_cli/core/clients.py index 310158b..734fcfe 100644 --- a/src/unstract_cli/core/clients.py +++ b/src/unstract_cli/core/clients.py @@ -42,7 +42,13 @@ LLMWhispererClientV2, ) -from unstract_cli.config import DOCSTUDIO, LLMWHISPERER, ResolvedConfig +from unstract_cli.config import ( + DOCSTUDIO, + ENV_VARS, + KEY_SOURCES, + LLMWHISPERER, + ResolvedConfig, +) from unstract_cli.core.errors import CLIError, ExitCode, error_from_status from unstract_cli.core.params import find_operation @@ -76,37 +82,35 @@ def deployment_url(base_url: str, org_id: str, api_name: str) -> str: def deployment( config: ResolvedConfig, - target: str, + api_name: str, transport_timeout: float | None = DEFAULT_TRANSPORT_TIMEOUT, ) -> APIDeploymentsClient: - """Build a deployment client for an alias, or for a bare API name. + """Build a client for one deployment, named by its API name. - An alias carries its own organisation and key; a bare name falls back to the - profile's, so an unconfigured caller can still name a deployment directly. + Fails before any request when no key or organisation resolves: a credential + error from the server would name the wrong fault, and the caller is told + every place the CLI looked rather than only that it found nothing. """ - if target in config.deployment_aliases(): - entry = config.deployment(target) - api_name, org_id, api_key = ( - entry["api_name"], - entry["org_id"], - entry["api_key"], + org_id = config.get(DOCSTUDIO, "org_id") + if not org_id: + raise CLIError( + f"No organisation is configured to run {api_name!r} in.", + ExitCode.USAGE, + hint="Run `unstract auth login`, set $UNSTRACT_ORG_ID, or pass --org-id.", ) - else: - api_name = target - org_id = config.get(DOCSTUDIO, "org_id") - api_key = config.get(DOCSTUDIO, "api_key") - - missing = [ - name for name, value in (("org_id", org_id), ("api_key", api_key)) if not value - ] - if missing: + api_key = config.deployment_key(api_name) + if not api_key: + looked_in = ", ".join(config.deployment_key_sources(api_name)) raise CLIError( - f"Deployment {target!r} is missing {' and '.join(missing)}.", + f"No key resolves for deployment {api_name!r}. Looked in: {looked_in} " + "-- all unset.", ExitCode.USAGE, - hint=_alias_hint(config, target) - or ( - "Define the deployment as an alias in the active profile, or set " - "$UNSTRACT_ORG_ID and $UNSTRACT_DEPLOYMENT_KEY." + hint=( + f"Set ${ENV_VARS[(DOCSTUDIO, 'api_key')][0]}, or store a key: " + "`unstract config set docstudio api_key ` for one that covers " + "the organisation, or `unstract config set docstudio api_key " + f" --deployment {api_name}` for this deployment alone. " + f"{KEY_SOURCES}" ), ) @@ -118,34 +122,35 @@ def deployment( ) -def _alias_hint(config: ResolvedConfig, target: str) -> str | None: - """What to say when a target is not one of the aliases that are configured. - - A bare API name is a supported way to name a deployment, so a target that is - not an alias cannot be rejected outright. It can still be a misspelt one, - and a caller who has defined aliases is likelier to have meant one of them - than to have typed a raw name, so the ones that exist are worth naming. - """ - if not (aliases := config.deployment_aliases()) or target in aliases: - return None - return ( - f"{target!r} is not one of the deployment aliases in the active profile " - f"({', '.join(aliases)}), so it was sent as an API name." - ) - - @contextmanager -def naming_aliases(config: ResolvedConfig, target: str) -> Iterator[None]: - """Say which aliases exist when a bare API name is not found. +def deployment_errors(api_name: str) -> Iterator[None]: + """Say what a refusal means for *this* deployment, once the server answers. - Sending a misspelt alias as an API name is indistinguishable from sending a - real one until the service answers, so the correction belongs on the answer. + A rejected key and an unknown name are both indistinguishable from success + until the service answers, so the correction belongs on the answer: a key + that works elsewhere may not cover this deployment, and a name that was + valid may have been renamed since it was written down. """ try: yield except CLIError as exc: - if exc.exit_code is ExitCode.NOT_FOUND and (hint := _alias_hint(config, target)): - exc.hint = f"{exc.hint} {hint}" if exc.hint else hint + if exc.exit_code is ExitCode.AUTH: + exc.message = ( + f"The key supplied for deployment {api_name!r} does not authorize " + f"it: {exc.message}" + ) + exc.hint = ( + "This deployment may need a key of its own: " + "`unstract config set docstudio api_key --deployment " + f"{api_name}`." + ) + elif exc.exit_code is ExitCode.NOT_FOUND: + more = ( + "Run `unstract docstudio deployment ls` for the current API names; " + "`unstract config doctor --probe` reports profile entries the " + "organisation no longer has." + ) + exc.hint = f"{exc.hint} {more}" if exc.hint else more raise @@ -408,9 +413,9 @@ def raise_for_result(result: dict[str, Any], endpoint: str | None = None) -> Non "DEFAULT_TRANSPORT_TIMEOUT", "UNSENDABLE", "deployment", + "deployment_errors", "deployment_url", "llmwhisperer", - "naming_aliases", "raise_for_result", "translated", "translating", diff --git a/src/unstract_cli/core/platform.py b/src/unstract_cli/core/platform.py index 3f2ecae..a521d7f 100644 --- a/src/unstract_cli/core/platform.py +++ b/src/unstract_cli/core/platform.py @@ -1,61 +1,22 @@ -"""Where the platform API lives, and the client that talks to it. +"""The client that talks to the platform API. Both platform-key operations -- `whoami` and the deployment listing -- are published in the OpenAPI spec and generated into `unstract-client`, so this -module only resolves configuration and builds `PlatformKeyClient`. Earlier -revisions subclassed `unstract.clone.PlatformClient`, the org-cloning tool's -hand-written admin client, and hand-built the whoami URL; that bypassed the -generated surface entirely, which is why neither command was covered by the -spec-derived contract tests. +module only resolves configuration and builds `PlatformKeyClient`. + +The platform API is served by the same deployment as the API deployments it +manages, so the client reads docstudio's `base_url` and the `platform_key` +beside it: one host, two keys. """ from __future__ import annotations from unstract.api_deployments.client import PlatformKeyClient -from unstract_cli.config import ( - DOCSTUDIO, - PLATFORM, - ResolvedConfig, -) +from unstract_cli.config import DOCSTUDIO, ResolvedConfig from unstract_cli.core.errors import CLIError, ExitCode -def platform_base_url(config: ResolvedConfig) -> str: - """Where the platform API lives. - - One deployment serves both the platform API and the deployments it manages, - so a caller who has said where docstudio is has already said where this is. - Resolving `platform.base_url` alone would ignore that: a profile written - before the `platform` block existed, and every `docstudio --base-url`, would - silently fall through to the built-in cloud default and send the key there. - - The two products are walked **tier by tier**, not one product at a time. - Asking `platform` for all three tiers first would let a profile's - `platform.base_url` beat a `docstudio --base-url` flag -- and `config init` - writes `platform.base_url` into every profile it generates, so that would - silently ignore the flag for every generated config, which is the majority - of them. - - Each tier is read explicitly, stopping before the built-in defaults. - Comparing `get`'s answer against the default instead would read a caller who - named the SaaS host as one who named nothing -- the same `config init` - profiles again, from the other direction. - """ - for from_platform, from_docstudio in zip( - config.explicit_tiers(PLATFORM, "base_url"), - config.explicit_tiers(DOCSTUDIO, "base_url"), - strict=True, - ): - # Within one tier the platform block wins: it is the specific answer to - # this question, where docstudio's is the one inherited from the sibling. - if from_platform is not None: - return str(from_platform) - if from_docstudio is not None: - return str(from_docstudio) - return str(config.require(PLATFORM, "base_url")) - - def platform_client( config: ResolvedConfig, org_id: str | None = None, @@ -79,8 +40,8 @@ def platform_client( hint="Omit the flag to leave the connection unbounded.", ) return PlatformKeyClient( - base_url=platform_base_url(config), - api_key=config.require(PLATFORM, "api_key"), + base_url=config.require(DOCSTUDIO, "base_url"), + api_key=config.require(DOCSTUDIO, "platform_key"), transport_timeout=timeout, logging_level="ERROR", ) @@ -90,7 +51,7 @@ def organisation(config: ResolvedConfig) -> str: """The organisation to act inside, or a usage error naming how to get one. It lives on the docstudio block: a platform key resolves it, and everything - that consumes it -- deployment URLs, aliases -- reads it from there. + that consumes it -- deployment URLs above all -- reads it from there. """ if org_id := config.get(DOCSTUDIO, "org_id"): return str(org_id) @@ -104,8 +65,4 @@ def organisation(config: ResolvedConfig) -> str: ) -__all__ = [ - "organisation", - "platform_base_url", - "platform_client", -] +__all__ = ["organisation", "platform_client"] diff --git a/tests/test_cli.py b/tests/test_cli.py index d689277..147bbac 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -112,6 +112,31 @@ def test_set_refuses_a_setting_the_product_does_not_have(capsys, tmp_path, monke assert not (tmp_path / "c.toml").exists() +def test_set_can_store_a_key_for_one_deployment(capsys, tmp_path, monkeypatch): + """The entry is where a run looks after the environment and before the + profile's key, so the write lands under the API name, not the product.""" + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "c.toml")) + code, payload, _ = run( + capsys, "config", "set", "docstudio", "api_key", "dk-1", "--deployment", "inv" + ) + assert code == 0 and payload["data"]["deployment"] == "inv" + text = (tmp_path / "c.toml").read_text(encoding="utf-8") + assert "[profiles.cloud-us.deployments.inv]" in text + assert "[profiles.cloud-us.docstudio]" not in text + + +def test_set_refuses_to_store_anything_but_a_key_per_deployment( + capsys, tmp_path, monkeypatch +): + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "c.toml")) + code, payload, _ = run( + capsys, "config", "set", "docstudio", "org_id", "org_A", "--deployment", "inv" + ) + assert code == int(ExitCode.USAGE) + assert "--deployment" in payload["error"]["message"] + assert not (tmp_path / "c.toml").exists() + + def test_doctor_reports_a_setting_nothing_reads(capsys, write_config): write_config('default_profile = "p"\n\n[profiles.p.llmwhisperer]\norg_id = "org_A"\n') code, payload, _ = run(capsys, "config", "doctor") @@ -159,6 +184,21 @@ def test_doctor_reports_sources_without_leaking_values(capsys, monkeypatch): assert "super-secret-value" not in json.dumps(payload) +def test_doctor_reports_the_platform_key_beside_the_deployment_key(capsys, monkeypatch): + """Two keys on one block, each with its own row: a platform key is + optional, so its absence is a report and not a problem.""" + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-super-secret-value") + code, payload, _ = run(capsys, "config", "doctor") + assert code == 0 + docstudio = payload["data"]["products"]["docstudio"] + assert docstudio["platform_key"] == { + "resolved": True, + "source": "env:UNSTRACT_PLATFORM_KEY", + } + assert "platform" not in payload["data"]["products"] + assert "pk-super-secret-value" not in json.dumps(payload) + + def doctor(capsys, *args) -> str: """`config doctor` -- a command with no network -- and its raw stdout.""" main([*args, "config", "doctor"]) @@ -187,14 +227,17 @@ def test_the_default_is_a_table_in_a_terminal_and_in_a_pipe( def test_no_isatty_call_decides_a_format(self): """A format that depends on a terminal makes a script's output depend on - how it was launched.""" + how it was launched. `auth login` asks the question once, to decide + whether it may prompt for keys -- never what it prints in.""" source = Path(app.__file__).parent offenders = [ path.name for path in source.rglob("*.py") if "isatty" in path.read_text(encoding="utf-8") ] - assert offenders == [] + assert offenders == ["platform_cmd.py"] + text = (source / "commands" / "platform_cmd.py").read_text(encoding="utf-8") + assert text.count("isatty") == 1 and "sys.stdin.isatty()" in text def test_an_agent_environment_makes_json_the_default(self, capsys, monkeypatch): monkeypatch.setenv("CLAUDECODE", "1") diff --git a/tests/test_clients.py b/tests/test_clients.py index f0c8519..d815888 100644 --- a/tests/test_clients.py +++ b/tests/test_clients.py @@ -14,11 +14,12 @@ from unstract_cli.config import ResolvedConfig, load_config from unstract_cli.core.clients import ( deployment, + deployment_errors, deployment_url, raise_for_result, translated, ) -from unstract_cli.core.errors import CLIError, ExitCode +from unstract_cli.core.errors import CLIError, ExitCode, error_from_status def _translate(exc: Exception) -> CLIError: @@ -99,10 +100,8 @@ def test_a_clean_success_raises_nothing(): org_id = "org_profile" api_key = "profile-key" -[profiles.p.deployments.invoices] -api_name = "invoice-parser" -org_id = "org_alias" -api_key = "alias-key" +[profiles.p.deployments."invoice-parser"] +api_key = "entry-key" """ @@ -121,10 +120,10 @@ def test_a_deployment_url_is_built_from_the_route_the_spec_declares(): assert "//" not in url.removeprefix("https://") -def test_an_alias_is_built_from_its_own_organisation_and_key(tmp_path): - client = deployment(_config(tmp_path), "invoices") - assert client.api_url.endswith("/org_alias/invoice-parser/") - assert client.api_key == "alias-key" +def test_a_deployment_with_an_entry_runs_with_its_own_key(tmp_path): + client = deployment(_config(tmp_path), "invoice-parser") + assert client.api_url.endswith("/org_profile/invoice-parser/") + assert client.api_key == "entry-key" def test_a_deployment_client_is_built_with_a_socket_timeout_by_default(tmp_path): @@ -134,50 +133,92 @@ def test_a_deployment_client_is_built_with_a_socket_timeout_by_default(tmp_path) assert client.transport_timeout == 120.0 -def test_a_bare_api_name_falls_back_to_the_profile(tmp_path): +def test_a_deployment_without_an_entry_runs_with_the_profile_key(tmp_path): client = deployment(_config(tmp_path), "some-api") assert client.api_url.endswith("/org_profile/some-api/") assert client.api_key == "profile-key" -def test_a_deployment_with_nothing_configured_names_everything_missing(tmp_path): - """Both are required, and reporting one at a time costs a round trip each.""" - with pytest.raises(CLIError) as caught: - deployment(_config(tmp_path, 'default_profile = "p"\n[profiles.p]\n'), "some-api") - assert caught.value.exit_code is ExitCode.USAGE - assert "org_id" in caught.value.message and "api_key" in caught.value.message - - -def test_a_target_that_is_not_an_alias_is_told_which_ones_are(tmp_path): - """A bare API name is legal, so a misspelt alias cannot be rejected -- but - a caller who defined aliases most likely meant one of them.""" +@pytest.mark.parametrize( + "ablate, winner", + [ + ((), "flag-key"), + (("flag",), "env-key"), + (("flag", "env"), "entry-key"), + (("flag", "env", "entry"), "profile-key"), + ], +) +def test_the_key_for_a_run_resolves_flag_env_entry_profile( + tmp_path, monkeypatch, ablate, winner +): + """The per-deployment entry is the most specific value *within* the profile + tier, not a tier above the environment: a file value that beat + `$UNSTRACT_DEPLOYMENT_KEY` would let a stale entry hijack a run the caller + set up for CI. Every source set, then removed one at a time from the top.""" + text = CONFIG if "entry" not in ablate else CONFIG.split("[profiles.p.deployments")[0] + config = _config(tmp_path, text) + if "env" not in ablate: + monkeypatch.setenv("UNSTRACT_DEPLOYMENT_KEY", "env-key") + if "flag" not in ablate: + config.overrides = {"docstudio.api_key": "flag-key"} + + assert deployment(config, "invoice-parser").api_key == winner + + +def test_a_deployment_with_no_key_anywhere_says_where_it_looked(tmp_path): + """The chain walked, and the exact command for each remedy: a caller told + only that a key is missing has to guess which of four places was read.""" with pytest.raises(CLIError) as caught: deployment( - _config(tmp_path, CONFIG.replace('org_id = "org_profile"\n', "")), "invoic" + _config( + tmp_path, + 'default_profile = "p"\n[profiles.p.docstudio]\norg_id = "org_X"\n', + ), + "invoice-parser", ) - assert "invoices" in (caught.value.hint or "") - - -def test_a_flag_fills_in_what_an_alias_leaves_out_and_no_more(tmp_path): - """The precedence the README states: an alias owns the settings it names, - and the connection flags reach only the ones it leaves to the profile.""" - config = _config( - tmp_path, - CONFIG + '\n[profiles.p.deployments.plain]\napi_name = "plain-api"\n', - ) - config.overrides = { - "docstudio.org_id": "org_flag", - "docstudio.api_key": "flag-key", - "docstudio.base_url": "https://flag-host", - } - - stated = deployment(config, "invoices") - assert stated.api_key == "alias-key" - assert "/org_alias/" in stated.api_url - - silent = deployment(config, "plain") - assert silent.api_key == "flag-key" - assert "/org_flag/" in silent.api_url - - # base_url is not a per-alias setting, so the flag reaches both. - assert stated.api_url.startswith("https://flag-host") + error = caught.value + assert error.exit_code is ExitCode.USAGE + assert "invoice-parser" in error.message + for source in ( + "--api-key", + "$UNSTRACT_DEPLOYMENT_KEY", + '[profiles.p.deployments."invoice-parser"] api_key', + "[profiles.p.docstudio] api_key", + ): + assert source in error.message, source + assert "config set docstudio api_key --deployment invoice-parser" in error.hint + assert "API Key Manager" in error.hint + + +def test_a_deployment_with_no_organisation_fails_before_the_key_is_read(tmp_path): + with pytest.raises(CLIError) as caught: + deployment(_config(tmp_path, 'default_profile = "p"\n[profiles.p]\n'), "some-api") + assert caught.value.exit_code is ExitCode.USAGE + assert "organisation" in caught.value.message + assert "auth login" in caught.value.hint + + +def test_a_rejected_key_is_reported_against_the_deployment(): + """The server's refusal is where "this deployment may need its own key" + becomes true, so that is where it is said -- with the command that stores + one.""" + with pytest.raises(CLIError) as caught, deployment_errors("invoice-parser"): + raise error_from_status(401, "Unauthorized") + error = caught.value + assert error.exit_code is ExitCode.AUTH + assert "invoice-parser" in error.message and "does not authorize" in error.message + assert "config set docstudio api_key --deployment invoice-parser" in error.hint + + +def test_an_unknown_deployment_is_pointed_at_the_listing(): + """An API name is editable server-side, so a name that was right can stop + being right without anything local changing.""" + with pytest.raises(CLIError) as caught, deployment_errors("invoice-parser"): + raise error_from_status(404, "Not found") + assert "deployment ls" in caught.value.hint + + +def test_other_failures_pass_through_the_deployment_context_untouched(): + with pytest.raises(CLIError) as caught, deployment_errors("invoice-parser"): + raise error_from_status(500, "boom") + assert caught.value.message == "boom" diff --git a/tests/test_commands.py b/tests/test_commands.py index 7f8685a..a187343 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -27,7 +27,7 @@ from unstract_cli.__main__ import main from unstract_cli.app import command_tree from unstract_cli.commands import clone_cmd, docstudio_cmd, platform_cmd, whisper_cmd -from unstract_cli.config import LLMWHISPERER, PLATFORM +from unstract_cli.config import DOCSTUDIO, LLMWHISPERER from unstract_cli.core.errors import CLIError, ExitCode @@ -148,7 +148,7 @@ def build(config, org_id=None, *, timeout=None): # factory has to do it too or the seam hides a production path. # The signature tracks the real `platform_client` deliberately: a # fixture that drifts from it passes while testing nothing. - client.built_with["api_key"] = config.get(PLATFORM, "api_key") + client.built_with["api_key"] = config.get(DOCSTUDIO, "platform_key") client.built_with["org_id"] = org_id client.built_with["timeout"] = timeout return client @@ -186,7 +186,7 @@ def test_the_v1_commands_are_registered(): "run", "status", } - assert set(tree["auth"]["commands"]) == {"whoami"} + assert set(tree["auth"]["commands"]) == {"login", "whoami"} # --------------------------------------------------------------------------- # @@ -704,24 +704,44 @@ def test_a_queued_run_reports_the_handle_it_started(capsys, deployment_client, t assert capsys.readouterr().out.strip() == "e-1" -def test_a_target_that_is_not_a_configured_alias_names_the_ones_that_are( - capsys, deployment_client, write_config +@pytest.mark.parametrize( + "argv", + [ + ("docstudio", "deployment", "status", "invoice-parser", "e-1"), + ("docstudio", "deployment", "run", "invoice-parser", "DOC", "--no-wait"), + ], + ids=["status", "run"], +) +def test_a_rejected_key_names_the_deployment_and_how_to_give_it_its_own( + capsys, deployment_client, tmp_path, argv ): - """A misspelt alias is sent as an API name and comes back not-found, which - says nothing about the aliases sitting in the profile.""" - write_config( - 'default_profile = "p"\n' - "[profiles.p.docstudio]\n" - 'org_id = "org"\n' - 'api_key = "k"\n' - "[profiles.p.deployments.invoices]\n" - 'api_name = "invoice-parser"\n' + """A 401 is the first moment "this deployment may need its own key" is + known to be true, so the hint that says so has to be reachable from both + commands that send one.""" + doc = tmp_path / "doc.pdf" + doc.write_bytes(b"%PDF") + deployment_client( + check_execution_status={"status_code": 401, "error": "Unauthorized"}, + structure_file={"status_code": 401, "error": "Unauthorized"}, ) + code, out, _ = run(capsys, *(str(doc) if a == "DOC" else a for a in argv)) + assert code == int(ExitCode.AUTH) + error = envelope(out)["error"] + assert "invoice-parser" in error["message"] + assert "does not authorize" in error["message"] + assert ( + "config set docstudio api_key --deployment invoice-parser" + in (error["hint"]) + ) + + +def test_an_unknown_api_name_is_pointed_at_the_listing(capsys, deployment_client): + """A misspelt or renamed API name comes back not-found, and the server is + the only authority on what the current names are.""" deployment_client(check_execution_status={"status_code": 404, "error": "not found"}) code, out, _ = run(capsys, "docstudio", "deployment", "status", "invoces", "e-1") assert code == int(ExitCode.NOT_FOUND) - hint = envelope(out)["error"]["hint"] - assert "invoces" in hint and "invoices" in hint + assert "deployment ls" in envelope(out)["error"]["hint"] def test_highlights_on_an_extraction_without_line_numbers_says_where_to_fix_it( @@ -1909,6 +1929,34 @@ def test_a_deployment_key_flag_is_refused_rather_than_ignored_by_ls( assert "dk-FROM-FLAG" not in json.dumps(envelope(out)) +@pytest.mark.parametrize( + "args", + [ + ("auth", "--platform-key", "pk-FROM-FLAG-0123", "whoami", "--no-save"), + ("docstudio", "--platform-key", "pk-FROM-FLAG-0123", "deployment", "ls"), + ], +) +def test_a_platform_key_flag_reaches_the_client_and_is_warned_about( + capsys, platform_client, monkeypatch, tmp_path, args +): + """Both groups that run platform-key commands take the key as a flag, and a + key on the command line gets the same shell-history warning as `--api-key`. + """ + _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\norg_id = "org_X"\n', + ) + client = platform_client(whoami=IDENTITY, list_deployments=_returns(_page())) + + code, out, err = run(capsys, *args) + + assert code == int(ExitCode.SUCCESS) + assert client.built_with["api_key"] == "pk-FROM-FLAG-0123" + assert "shell history" in err + assert "pk-FROM-FLAG-0123" not in out + + def test_the_platform_key_never_reaches_a_stream( capsys, platform_client, monkeypatch, tmp_path ): @@ -1981,6 +2029,452 @@ def test_the_platform_status_is_recovered_from_the_message( assert code == int(expected), message +# --------------------------------------------------------------------------- # +# auth login +# --------------------------------------------------------------------------- # + +PK, DK, LK = "pk-platform-000001", "dk-deployment-0001", "lk-whisperer-00001" + + +@pytest.fixture +def login_seams(monkeypatch, platform_client, tmp_path): + """Both client factories faked, prompts scripted, and a config file of our own. + + Returns a function that scripts the terminal: `answers` are what each prompt + returns in order, `confirm` what the yes/no questions return (one value for + all of them, or a list consumed in order), and `tty` whether stdin counts + as a terminal at all. + """ + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + monkeypatch.delenv("UNSTRACT_PLATFORM_KEY", raising=False) + state = {"prompts": [], "answers": []} + + def prompt(text, **kwargs): + state["prompts"].append(text) + return state["answers"].pop(0) + + def install(answers=(), *, confirm=False, tty=True, whoami=None, usage=None): + state["answers"], state["prompts"] = list(answers), [] + monkeypatch.setattr(platform_cmd, "_interactive", lambda: tty) + monkeypatch.setattr(platform_cmd, "_prompt", prompt) + confirms = list(confirm) if isinstance(confirm, list) else None + + def confirm_answer(text, **kwargs): + state["confirms"].append(text) + return confirms.pop(0) if confirms is not None else confirm + + state["confirms"] = [] + monkeypatch.setattr(platform_cmd, "_confirm", confirm_answer) + client = platform_client(whoami=whoami or IDENTITY) + build_platform = platform_cmd.platform_client + + def build_recording_host(config, org_id=None, *, timeout=None): + client.built_with["base_url"] = config.get(DOCSTUDIO, "base_url") + return build_platform(config, org_id, timeout=timeout) + + monkeypatch.setattr(platform_cmd, "platform_client", build_recording_host) + whisper = FakeWhisper(get_usage_info=usage if usage is not None else {"quota": 1}) + whisper.built_with = {} + + def build(config): + whisper.built_with["api_key"] = config.get(LLMWHISPERER, "api_key") + return whisper + + monkeypatch.setattr(platform_cmd, "llmwhisperer", build) + state["platform"], state["whisper"] = client, whisper + return state + + return install + + +def _written(tmp_path) -> str: + return (tmp_path / "config.toml").read_text(encoding="utf-8") + + +def test_login_asks_for_each_key_in_turn_and_stores_them_as_literals( + capsys, login_seams, tmp_path +): + """Interactive path: platform, deployment, LLMWhisperer, one hidden prompt + each. The two keys with a read-only endpoint are checked; the deployment + key is stored as given and said to be.""" + seams = login_seams([PK, DK, LK]) + + code, out, err = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert [p.split(" (")[0] for p in seams["prompts"]] == [ + "Platform key", + "Deployment key", + "LLMWhisperer key", + ] + assert seams["platform"].built_with["api_key"] == PK + assert seams["whisper"].built_with["api_key"] == LK + data = envelope(out)["data"] + assert data["profile"] == "cloud-us" + assert (data["platform"], data["deployment"], data["llmwhisperer"]) == ( + "verified", + "stored", + "verified", + ) + assert data["organization_id"] == "org_ABC123" + assert "stored as given" in data["note"] + text = _written(tmp_path) + assert f'platform_key = "{PK}"' in text + assert f'api_key = "{DK}"' in text + assert f'api_key = "{LK}"' in text + assert 'org_id = "org_ABC123"' in text + assert oct((tmp_path / "config.toml").stat().st_mode & 0o777) == "0o600" + # The keys reach the file and nowhere else. + for key in (PK, DK, LK): + assert key not in out and key not in err + + +def test_login_with_every_prompt_skipped_is_a_usage_error(capsys, login_seams, tmp_path): + login_seams(["", "", ""]) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.USAGE) + assert "at least one" in envelope(out)["error"]["message"] + assert not (tmp_path / "config.toml").exists() + + +def test_login_with_only_a_deployment_key_calls_nothing(capsys, login_seams, tmp_path): + seams = login_seams(["", DK, ""]) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert seams["platform"].calls == [] and seams["whisper"].calls == [] + data = envelope(out)["data"] + assert (data["platform"], data["deployment"], data["llmwhisperer"]) == ( + "skipped", + "stored", + "skipped", + ) + assert "org_id" not in _written(tmp_path) + + +def test_login_without_a_terminal_and_without_flags_fails_before_prompting( + capsys, login_seams, tmp_path +): + """A script that reaches a hidden prompt hangs; a script told which flags + to pass does not.""" + seams = login_seams([], tty=False) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.USAGE) + assert seams["prompts"] == [] + error = envelope(out)["error"] + assert "not a terminal" in error["message"] + assert "--platform-key" in error["hint"] and "--llmwhisperer-key" in error["hint"] + assert not (tmp_path / "config.toml").exists() + + +def test_login_flags_take_values_and_one_of_them_from_stdin( + capsys, login_seams, monkeypatch, tmp_path +): + """The non-interactive twin: zero prompts, even at a terminal.""" + import io + + seams = login_seams([], tty=True) + monkeypatch.setattr("sys.stdin", io.StringIO(f"{PK}\n")) + + code, out, _ = run( + capsys, "auth", "login", "--platform-key", "-", "--llmwhisperer-key", LK + ) + + assert code == int(ExitCode.SUCCESS) + assert seams["prompts"] == [] + assert seams["platform"].built_with["api_key"] == PK + assert seams["whisper"].built_with["api_key"] == LK + assert envelope(out)["data"]["deployment"] == "skipped" + assert f'platform_key = "{PK}"' in _written(tmp_path) + + +def test_login_reads_at_most_one_key_from_stdin(capsys, login_seams, tmp_path): + login_seams([]) + + code, out, _ = run( + capsys, "auth", "login", "--platform-key", "-", "--deployment-key", "-" + ) + + assert code == int(ExitCode.USAGE) + assert "stdin" in envelope(out)["error"]["message"] + assert not (tmp_path / "config.toml").exists() + + +def test_login_takes_the_platform_key_from_the_group_flag_too( + capsys, login_seams, tmp_path +): + seams = login_seams([]) + + code, _, _ = run(capsys, "auth", "--platform-key", PK, "login") + + assert code == int(ExitCode.SUCCESS) + assert seams["prompts"] == [] + assert f'platform_key = "{PK}"' in _written(tmp_path) + + +@pytest.mark.parametrize( + "rejected", + ["platform", "llmwhisperer"], +) +def test_login_writes_nothing_when_any_key_is_rejected( + capsys, login_seams, tmp_path, rejected +): + """Every check runs before the one write: a file holding one good key and + one bad one would report the bad one as configured.""" + kwargs = { + "whoami": PlatformClientError("whoami failed with 401: nope") + if rejected == "platform" + else None, + "usage": LLMWhispererClientException( + {"message": "bad key", "status_code": 401}, 401 + ) + if rejected == "llmwhisperer" + else None, + } + login_seams([PK, DK, LK], **kwargs) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.AUTH) + assert envelope(out)["ok"] is False + assert not (tmp_path / "config.toml").exists() + + +def test_login_again_replaces_the_keys_given_and_keeps_the_rest( + capsys, login_seams, tmp_path +): + """Rotation: the same profile, updated in place, and a same-organisation + re-run asks nothing.""" + login_seams([PK, DK, LK]) + run(capsys, "auth", "login") + seams = login_seams(["", "dk-rotated-000001", ""]) + + code, _, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert seams["prompts"][-1].startswith("LLMWhisperer") + text = _written(tmp_path) + assert 'api_key = "dk-rotated-000001"' in text and DK not in text + assert f'platform_key = "{PK}"' in text and f'api_key = "{LK}"' in text + assert text.count("[profiles.") == 2 + + +def test_login_refuses_to_repoint_a_profile_at_another_organisation( + capsys, login_seams, tmp_path +): + """Non-interactive: fail, name both organisations, name the way out.""" + login_seams([], whoami={**IDENTITY, "organization_id": "org_OLD"}) + run(capsys, "auth", "login", "--platform-key", PK) + login_seams([], whoami={**IDENTITY, "organization_id": "org_NEW"}) + + code, out, _ = run(capsys, "auth", "login", "--platform-key", PK) + + assert code == int(ExitCode.USAGE) + error = envelope(out)["error"] + assert "org_OLD" in error["message"] and "org_NEW" in error["message"] + assert "--force" in error["hint"] and "--profile" in error["hint"] + assert 'org_id = "org_OLD"' in _written(tmp_path) + + +def test_login_overwrites_the_organisation_only_when_forced( + capsys, login_seams, tmp_path +): + login_seams([], whoami={**IDENTITY, "organization_id": "org_OLD"}) + run(capsys, "auth", "login", "--platform-key", PK) + login_seams([], whoami={**IDENTITY, "organization_id": "org_NEW"}) + + code, _, _ = run(capsys, "auth", "login", "--platform-key", PK, "--force") + + assert code == int(ExitCode.SUCCESS) + assert 'org_id = "org_NEW"' in _written(tmp_path) + assert "org_OLD" not in _written(tmp_path) + + +def test_login_offers_a_new_profile_named_after_the_organisation( + capsys, login_seams, tmp_path +): + """Interactive: the profile the key belongs to is a new one, suggested + from the organisation's display name, and the old profile is untouched.""" + login_seams([PK, "", ""], whoami={**IDENTITY, "organization_id": "org_OLD"}) + run(capsys, "auth", "login") + seams = login_seams( + [PK, "", "", "beta-corp"], + confirm=True, + whoami={ + **IDENTITY, + "organization_id": "org_NEW", + "organization_name": "Beta Corp", + }, + ) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert envelope(out)["data"]["profile"] == "beta-corp" + text = _written(tmp_path) + assert "[profiles.beta-corp.docstudio]" in text and 'org_id = "org_NEW"' in text + assert 'org_id = "org_OLD"' in text + assert seams["prompts"][-1] == "Profile name" + + +def test_a_new_profile_offered_by_the_guard_keeps_the_host_the_key_was_checked_on( + capsys, login_seams, tmp_path +): + """The key was verified against the profile the login started from; a new + profile that does not record that host would send it to the built-in + default next time.""" + (tmp_path / "config.toml").write_text( + 'default_profile = "onprem"\n[profiles.onprem.docstudio]\n' + 'base_url = "https://onprem.example/"\norg_id = "org_OLD"\n', + encoding="utf-8", + ) + seams = login_seams( + [PK, "", "", "beta"], + confirm=True, + whoami={**IDENTITY, "organization_id": "org_NEW"}, + ) + + code, _, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert seams["platform"].built_with["base_url"] == "https://onprem.example/" + text = _written(tmp_path) + assert text.count('base_url = "https://onprem.example/"') == 2 + assert "[profiles.beta.docstudio]" in text + + +def test_a_profile_chosen_at_the_guard_is_replaced_not_merged_into( + capsys, login_seams, tmp_path +): + """The name typed at the guard may be an existing profile with a host and + keys of its own; none of those were checked against this key's host, so + the profile is confirmed and then rebuilt from what this login verified.""" + (tmp_path / "config.toml").write_text( + 'default_profile = "onprem"\n[profiles.onprem.docstudio]\n' + 'base_url = "https://onprem.example/"\norg_id = "org_OLD"\n' + '[profiles.beta.docstudio]\nbase_url = "https://stale.example/"\n' + 'api_key = "sk-stale"\n' + '[profiles.beta.deployments."invoice-parser"]\napi_key = "sk-stale-entry"\n', + encoding="utf-8", + ) + seams = login_seams( + [PK, "", "", "beta"], + confirm=True, + whoami={**IDENTITY, "organization_id": "org_NEW"}, + ) + + code, _, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert seams["confirms"][-1] == "Profile 'beta' already exists. Replace it?" + text = _written(tmp_path) + assert "stale" not in text + assert text.count('base_url = "https://onprem.example/"') == 2 + assert "[profiles.beta.docstudio]" in text and 'org_id = "org_NEW"' in text + + +def test_a_new_profile_name_that_belongs_to_a_third_organisation_is_confirmed( + capsys, login_seams, tmp_path +): + """The guard's own remedy must not be the overwrite it exists to prevent: + a typed name that is another organisation's profile is asked about again, + and declining re-prompts.""" + (tmp_path / "config.toml").write_text( + 'default_profile = "cloud-us"\n' + '[profiles.cloud-us.docstudio]\norg_id = "org_OLD"\n' + '[profiles.partner.docstudio]\norg_id = "org_PARTNER"\n', + encoding="utf-8", + ) + seams = login_seams( + [PK, "", "", "partner", "fresh"], + confirm=[True, False], + whoami={**IDENTITY, "organization_id": "org_NEW"}, + ) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert envelope(out)["data"]["profile"] == "fresh" + assert "partner" in seams["confirms"][1] and "org_PARTNER" in seams["confirms"][1] + text = _written(tmp_path) + assert 'org_id = "org_PARTNER"' in text and 'org_id = "org_OLD"' in text + assert "[profiles.fresh.docstudio]" in text and 'org_id = "org_NEW"' in text + + +def test_login_overwrites_when_a_new_profile_is_declined(capsys, login_seams, tmp_path): + login_seams([PK, "", ""], whoami={**IDENTITY, "organization_id": "org_OLD"}) + run(capsys, "auth", "login") + seams = login_seams( + [PK, "", ""], confirm=False, whoami={**IDENTITY, "organization_id": "org_NEW"} + ) + + code, out, _ = run(capsys, "auth", "login") + + assert code == int(ExitCode.SUCCESS) + assert envelope(out)["data"]["profile"] == "cloud-us" + assert len(seams["prompts"]) == 3 + assert 'org_id = "org_NEW"' in _written(tmp_path) + assert "org_OLD" not in _written(tmp_path) + + +def test_login_writes_the_profile_named_and_checks_against_its_own_host( + capsys, login_seams, tmp_path +): + """A profile that does not exist yet must not borrow the default profile's + host for the check: the key would be verified against a server the new + profile will never talk to.""" + (tmp_path / "config.toml").write_text( + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\n' + 'base_url = "https://elsewhere.example/"\norg_id = "org_X"\n', + encoding="utf-8", + ) + seams = login_seams([]) + + code, out, _ = run( + capsys, + "auth", + "--base-url", + "https://staging.example/", + "login", + "--profile", + "staging", + "--platform-key", + PK, + ) + + assert code == int(ExitCode.SUCCESS) + assert envelope(out)["data"]["profile"] == "staging" + assert seams["platform"].built_with["base_url"] == "https://staging.example/" + text = _written(tmp_path) + assert "[profiles.staging.docstudio]" in text + assert 'base_url = "https://staging.example/"' in text + assert 'org_id = "org_X"' in text # the other profile is untouched + + +def test_login_does_not_write_a_discovered_project_config( + capsys, login_seams, monkeypatch, tmp_path +): + project = tmp_path / "repo" + project.mkdir() + (project / ".unstract.toml").write_text( + "[profiles.team.docstudio]\n", encoding="utf-8" + ) + monkeypatch.chdir(project) + monkeypatch.delenv("UNSTRACT_CONFIG", raising=False) + login_seams([]) + + code, out, _ = run(capsys, "auth", "login", "--deployment-key", DK) + + assert code == int(ExitCode.USAGE) + assert "project-local" in envelope(out)["error"]["message"] + assert DK not in (project / ".unstract.toml").read_text() + + # --------------------------------------------------------------------------- # # --save: the flag that exists to protect a one-shot read # --------------------------------------------------------------------------- # diff --git a/tests/test_config.py b/tests/test_config.py index d9a5079..87ad678 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -38,13 +38,11 @@ org_id = "org_from_profile" api_key = "env:UNSTRACT_DEPLOYMENT_KEY" -[profiles.p.deployments.invoices] -api_name = "invoice-parser" +[profiles.p.deployments."invoice-parser"] +api_key = "env:INVOICE_KEY" -[profiles.p.deployments.receipts] -api_name = "receipt-parser" -org_id = "org_alias" -api_key = "alias-key" +[profiles.p.deployments."receipt-parser"] +api_key = "entry-key" """ @@ -151,28 +149,39 @@ def test_profile_selected_by_env_var(write_config, monkeypatch): assert resolved().get(DOCSTUDIO, "org_id") == "org_from_profile" -def test_deployment_alias_falls_back_to_the_product_block(write_config, monkeypatch): +def test_a_deployment_without_an_entry_uses_the_profile_key(write_config, monkeypatch): write_config(PROFILE_TOML) monkeypatch.setenv("UNSTRACT_DEPLOYMENT_KEY", "secret-value") - alias = resolved().deployment("invoices") - assert alias == { - "api_name": "invoice-parser", - "org_id": "org_from_profile", - "api_key": "secret-value", - } + assert resolved().deployment_key("some-api") == "secret-value" + + +def test_a_deployment_entry_key_beats_the_profile_key(write_config): + write_config(PROFILE_TOML.replace("env:UNSTRACT_DEPLOYMENT_KEY", "profile-key")) + assert resolved().deployment_key("receipt-parser") == "entry-key" + assert resolved().deployment_key("some-api") == "profile-key" -def test_deployment_alias_overrides_win(write_config): +def test_a_deployment_entry_may_point_at_the_environment(write_config, monkeypatch): write_config(PROFILE_TOML) - alias = resolved().deployment("receipts") - assert alias["org_id"] == "org_alias" - assert alias["api_key"] == "alias-key" + monkeypatch.setenv("INVOICE_KEY", "from-env") + assert resolved().deployment_key("invoice-parser") == "from-env" -def test_unknown_deployment_alias_lists_the_known_ones(write_config): +def test_a_deployment_entry_pointing_at_an_unset_variable_is_an_error(write_config): + """Silently running with the profile's key reports success for a run the + entry was written to keep separate.""" write_config(PROFILE_TOML) - with pytest.raises(ConfigError, match="invoices, receipts"): - resolved().deployment("nope") + with pytest.raises(ConfigError, match=r"INVOICE_KEY is not set"): + resolved().deployment_key("invoice-parser") + + +def test_deployment_names_are_the_entries_in_the_profile(write_config): + write_config(PROFILE_TOML) + assert resolved().deployment_names() == ("invoice-parser", "receipt-parser") + assert resolved(profile="p").deployment_names() == ( + "invoice-parser", + "receipt-parser", + ) def test_resolution_source_reports_the_winner(write_config, monkeypatch): @@ -339,10 +348,10 @@ def test_loose_permissions_warn_rather_than_fail(write_config): [profiles.p.docstudio] org_id = "org_from_project" +platform_key = "project-platform-key" -[profiles.p.deployments.invoices] -api_name = "invoice-parser" -api_key = "alias-literal-key" +[profiles.p.deployments."invoice-parser"] +api_key = "entry-literal-key" """ @@ -361,10 +370,12 @@ def test_a_discovered_project_config_supplies_no_key_and_no_host(tmp_path, monke assert cfg.get(LLMWHISPERER, "base_url") == DEFAULT_BASE_URLS[LLMWHISPERER] assert cfg.get(LLMWHISPERER, "api_key") is None - assert cfg.deployment("invoices")["api_key"] is None + # Both docstudio keys are credentials, whatever they are called. + assert cfg.get(DOCSTUDIO, "platform_key") is None + assert cfg.deployment_key("invoice-parser") is None # Everything the file is legitimately for still applies. assert cfg.get(DOCSTUDIO, "org_id") == "org_from_project" - assert cfg.deployment("invoices")["api_name"] == "invoice-parser" + assert cfg.deployment_names() == ("invoice-parser",) assert any(str(path) in w and "Ignoring" in w for w in cfg.file.warnings) assert cfg.resolution_source(LLMWHISPERER, "api_key")["detail"] @@ -376,6 +387,7 @@ def test_the_same_file_named_explicitly_is_honoured(tmp_path, monkeypatch): assert cfg.get(LLMWHISPERER, "base_url") == "https://elsewhere.example/api/v2" assert cfg.get(LLMWHISPERER, "api_key") == "project-literal-key" + assert cfg.get(DOCSTUDIO, "platform_key") == "project-platform-key" assert not any("Ignoring" in w for w in cfg.file.warnings) @@ -389,8 +401,8 @@ def test_writing_back_a_project_config_keeps_the_keys_it_withheld(tmp_path, monk reloaded = load_config() assert reloaded.profiles["p"]["docstudio"]["org_id"] == "org_edited" assert reloaded.profiles["p"]["llmwhisperer"]["api_key"] == "project-literal-key" - assert reloaded.profiles["p"]["deployments"]["invoices"]["api_key"] == ( - "alias-literal-key" + assert reloaded.profiles["p"]["deployments"]["invoice-parser"]["api_key"] == ( + "entry-literal-key" ) @@ -463,11 +475,11 @@ def test_a_write_through_a_symlink_fails_without_touching_its_target(tmp_path): assert victim.read_text(encoding="utf-8") == "keep = true\n" -def test_a_withheld_alias_key_is_reported_against_the_alias(tmp_path, monkeypatch): +def test_a_withheld_entry_key_is_reported_against_the_entry(tmp_path, monkeypatch): _plant_project_config(tmp_path, monkeypatch) cfg = resolved() - assert cfg.withheld_detail("deployments", "invoices", "api_key") - assert cfg.withheld_detail("deployments", "invoices", "org_id") is None + assert cfg.withheld_detail("deployments", "invoice-parser", "api_key") + assert cfg.withheld_detail("deployments", "receipt-parser", "api_key") is None def test_starter_profiles_hold_no_literal_secrets(): @@ -526,26 +538,6 @@ def test_doctor_reports_a_refused_env_reference_as_unresolved(tmp_path, monkeypa assert "may not choose which environment variable" in report["detail"] -def test_a_refused_alias_reference_names_the_trust_rule_not_a_missing_var( - tmp_path, monkeypatch -): - """Blaming an unset variable sends the user to export one that is set.""" - work = tmp_path / "work" - work.mkdir() - (work / PROJECT_CONFIG_NAME).write_text( - '[profiles.p.docstudio]\napi_key = "k"\n' - '[profiles.p.deployments.inv]\napi_name = "n"\norg_id = "env:MY_ORG"\n', - encoding="utf-8", - ) - monkeypatch.chdir(work) - monkeypatch.setenv("MY_ORG", "org_ABC") - - cfg = ResolvedConfig(file=load_config(), profile_name="p") - with pytest.raises(ConfigError) as caught: - cfg.deployment("inv") - assert "may not choose which environment variable" in str(caught.value) - - def test_an_override_is_only_read_under_the_key_it_is_written_with(tmp_path): """`resolution_source` and `get` have to look in the same place, or doctor reports a value resolved that the CLI never reads.""" diff --git a/tests/test_discover.py b/tests/test_discover.py index c4eca60..a57db64 100644 --- a/tests/test_discover.py +++ b/tests/test_discover.py @@ -15,7 +15,7 @@ from unstract_cli.__main__ import main from unstract_cli.app import cli from unstract_cli.commands import config_cmd -from unstract_cli.config import PLATFORM +from unstract_cli.config import DOCSTUDIO from unstract_cli.core.discover import discover, exit_codes from unstract_cli.core.errors import CLIError, ExitCode from unstract_cli.core.output import CONTRACT_VERSION @@ -193,20 +193,26 @@ def get_usage_info(self): @pytest.fixture def platform_probe_client(monkeypatch): - def install(reply=None): + def install(reply=None, live=()): class Fake: def whoami(self): if isinstance(reply, Exception): raise reply return reply or {} + def list_deployments(self, org_id, api_name=None): + if isinstance(live, Exception): + raise live + results = [{"api_name": n} for n in live if n == api_name] + return {"count": len(results), "results": results} + monkeypatch.setattr( config_cmd, "platform_client", - # Resolves the key like its sibling in test_commands, so the probe - # tests exercise the registration that feeds the scrubber. + # Requires the key like the real factory, so an absent one is the + # ConfigError the probe reports rather than a call on the fake. lambda config, org_id=None, *, timeout=None: ( - config.get(PLATFORM, "api_key"), + config.require(DOCSTUDIO, "platform_key"), Fake(), )[1], ) @@ -354,21 +360,20 @@ def test_success_publishes_no_error_code(): assert all(code for name, code in table.items() if name != "success") -def test_doctor_reports_an_alias_whose_settings_do_not_arrive( +def test_doctor_reports_a_deployment_entry_whose_key_does_not_arrive( capsys, write_config, monkeypatch ): - """A listed alias says nothing about whether the settings behind it - resolve, and the failure only shows up when a run is attempted.""" + """A listed entry says nothing about whether the key behind it resolves, + and a run would otherwise find out only from the server.""" write_config( """ default_profile = "p" [profiles.p.docstudio] api_key = "dk-configured-key" org_id = "org_ABC" - [profiles.p.deployments.invoices] - api_name = "invoice-parser" - [profiles.p.deployments.broken] - api_name = "no-key" + [profiles.p.deployments."invoice-parser"] + api_key = "dk-entry-key" + [profiles.p.deployments."broken"] api_key = "env:NOT_SET_ANYWHERE" """ ) @@ -379,10 +384,94 @@ def test_doctor_reports_an_alias_whose_settings_do_not_arrive( assert code != int(ExitCode.SUCCESS) report = payload["error"]["details"] - assert set(report["deployment_aliases"]) == {"invoices", "broken"} + assert set(report["deployments"]) == {"invoice-parser", "broken"} assert any("broken" in problem for problem in report["problems"]) +STALE_CONFIG = """ +default_profile = "p" +[profiles.p.docstudio] +org_id = "org_ABC" +[profiles.p.deployments."invoice-parser"] +api_key = "dk-1" +[profiles.p.deployments."renamed-since"] +api_key = "dk-2" +""" + + +def test_the_probe_reports_entries_the_organisation_no_longer_has( + capsys, write_config, probe_client, platform_probe_client, monkeypatch +): + """An orphaned entry is a warning, not a failure: it does no harm until it + is run, and the exit code is what setup scripts branch on.""" + write_config(STALE_CONFIG) + probe_client({"quota": 1}) + platform_probe_client({"organization_id": "org_ABC"}, live={"invoice-parser"}) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + + code = main(["-o", "json", "config", "doctor", "--probe"]) + out, err = capsys.readouterr() + + assert code == int(ExitCode.SUCCESS) + assert json.loads(out)["data"]["stale_deployments"] == ["renamed-since"] + assert "renamed-since" in err and "deployment ls" in err + + +def test_a_listing_that_fails_is_a_failed_check_not_a_clean_one( + capsys, write_config, probe_client, platform_probe_client, monkeypatch +): + """The check was asked for and did not run; exiting 0 would tell a setup + script the entries were verified.""" + write_config(STALE_CONFIG) + probe_client({"quota": 1}) + platform_probe_client( + {"organization_id": "org_ABC"}, + live=CLIError("listing timed out", ExitCode.TIMEOUT), + ) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + + code = main(["-o", "json", "config", "doctor", "--probe"]) + report = json.loads(capsys.readouterr().out)["error"]["details"] + + assert code == int(ExitCode.GENERIC) + assert "stale_deployments" not in report + assert any("listing timed out" in p for p in report["problems"]) + + +def test_the_probe_skips_the_entry_check_without_a_platform_key( + capsys, write_config, probe_client, platform_probe_client +): + """Only the platform key can answer the question, and most callers hold + none -- their entries are still theirs to keep.""" + write_config(STALE_CONFIG) + probe_client({"quota": 1}) + platform_probe_client(CLIError("must not be called")) + + code = main(["-o", "json", "config", "doctor", "--probe"]) + out, err = capsys.readouterr() + + assert code == int(ExitCode.SUCCESS) + assert "stale_deployments" not in json.loads(out)["data"] + assert "renamed-since" not in err + # --probe was explicit, so the skip is explained rather than silent. + assert "no platform key" in err + + +def test_doctor_asks_the_server_nothing_without_probe( + capsys, write_config, platform_probe_client, monkeypatch +): + write_config(STALE_CONFIG) + platform_probe_client(CLIError("must not be called")) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + + code = main(["-o", "json", "config", "doctor"]) + out, err = capsys.readouterr() + + assert code == int(ExitCode.SUCCESS) + assert "stale_deployments" not in json.loads(out)["data"] + assert "platform key" not in err # bare doctor is silent about the skip + + def test_a_malformed_config_file_is_a_usage_error(capsys, write_config): write_config("[profiles.p\nthis is not toml") diff --git a/tests/test_platform.py b/tests/test_platform.py index ea8f61d..b652fec 100644 --- a/tests/test_platform.py +++ b/tests/test_platform.py @@ -1,13 +1,9 @@ """Where the platform key is sent, and the one flag the CLI validates itself. The transport and the two operations are generated into `unstract-client` and -covered there; what is left here is CLI-owned: which host the key goes to when -several tiers and two products could each name one, and refusing a -`--transport-timeout` the connection layer would reject deep inside itself. - -The whoami URL and the identity body used to be tested here, because an earlier -revision hand-built both on a subclass of the clone tool's admin client. They -moved upstream with the operation. +covered there; what is left here is CLI-owned: which host and key the client +is built with, and refusing a `--transport-timeout` the connection layer would +reject deep inside itself. """ from __future__ import annotations @@ -17,12 +13,13 @@ from unstract_cli.config import ( DEFAULT_BASE_URLS, DOCSTUDIO, - PLATFORM, ConfigFile, ResolvedConfig, ) from unstract_cli.core.errors import CLIError, ExitCode -from unstract_cli.core.platform import platform_base_url, platform_client +from unstract_cli.core.platform import platform_client + +KEY = {"platform_key": "pk-000000000000"} def _resolved(profiles, **overrides): @@ -32,14 +29,11 @@ def _resolved(profiles, **overrides): ) -# --- the URL whoami builds ------------------------------------------------ - - def test_the_factory_threads_a_timeout() -> None: """`--transport-timeout` was accepted on `deployment ls` and ignored: the parent's own default is 60s, spent per page. """ - config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + config = _resolved({"p": {DOCSTUDIO: KEY}}) assert platform_client(config, timeout=3).transport_timeout == 3 @@ -47,110 +41,47 @@ def test_the_factory_threads_a_timeout() -> None: # --- which host the key is sent to ---------------------------------------- -def test_the_platform_host_follows_docstudio_when_unset() -> None: - """A profile written before the `platform` block existed names only - docstudio's host. Resolving `platform.base_url` alone fell through to the - built-in cloud default and sent the key there. +def test_the_platform_host_is_docstudios() -> None: + """One deployment serves both the platform API and the deployments it + manages, so the host a caller named for docstudio is the host for this. """ - config = _resolved( - {"p": {DOCSTUDIO: {"base_url": "https://onprem.example"}}}, - ) + config = _resolved({"p": {DOCSTUDIO: {**KEY, "base_url": "https://onprem.example"}}}) - assert platform_base_url(config) == "https://onprem.example" + assert platform_client(config).base_url == "https://onprem.example" def test_a_docstudio_base_url_flag_reaches_the_platform_call() -> None: - """`docstudio --base-url` records `docstudio.base_url`; `deployment ls` - reads the platform block. The flag was accepted and dropped. - """ - config = _resolved({"p": {}}, **{"docstudio.base_url": "https://flag.example"}) - - assert platform_base_url(config) == "https://flag.example" - - -def test_an_explicit_platform_host_still_wins() -> None: config = _resolved( - { - "p": { - DOCSTUDIO: {"base_url": "https://docstudio.example"}, - PLATFORM: {"base_url": "https://platform.example"}, - } - } - ) - - assert platform_base_url(config) == "https://platform.example" - - -def test_the_saas_default_is_honoured_when_the_caller_names_it() -> None: - """The first fix compared the resolved value against - `DEFAULT_BASE_URLS[PLATFORM]` to tell "unset" from "chosen". Those are the - same string, so a caller who named the SaaS host was read as having named - nothing and silently redirected to docstudio's -- the inverse of the defect - it fixed. `config init` writes that exact host into every profile, so this - is the common shape, not a corner of it. - """ - profile = _resolved( - { - "p": { - DOCSTUDIO: {"base_url": "https://onprem.example"}, - PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}, - } - } - ) - flag = _resolved( - {"p": {DOCSTUDIO: {"base_url": "https://onprem.example"}}}, - **{"platform.base_url": DEFAULT_BASE_URLS[PLATFORM]}, + {"p": {DOCSTUDIO: {**KEY, "base_url": "https://profile.example"}}}, + **{"docstudio.base_url": "https://flag.example"}, ) - assert platform_base_url(profile) == DEFAULT_BASE_URLS[PLATFORM] - assert platform_base_url(flag) == DEFAULT_BASE_URLS[PLATFORM] + assert platform_client(config).base_url == "https://flag.example" -def test_a_docstudio_flag_beats_a_platform_host_in_the_profile() -> None: - """Greptile, on PR #3. Walking `platform`'s three tiers before docstudio's - let a *profile* value beat a *flag*, inverting the precedence the config - layer promises everywhere else -- and `config init` writes - `platform.base_url` into every profile it generates, so the flag was ignored - for every generated config, not a corner case. - """ - config = _resolved( - {"p": {PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}}}, - **{"docstudio.base_url": "https://flag.example"}, +def test_the_built_in_default_is_the_last_resort() -> None: + assert ( + platform_client(_resolved({"p": {DOCSTUDIO: KEY}})).base_url + == (DEFAULT_BASE_URLS[DOCSTUDIO]) ) - assert platform_base_url(config) == "https://flag.example" - -def test_a_platform_flag_still_beats_a_docstudio_flag() -> None: - """Within one tier the specific product wins; across tiers it does not.""" +def test_the_platform_key_is_not_the_deployment_key() -> None: + """Both live on the docstudio block; the client must take the right one.""" config = _resolved( - {"p": {}}, - **{ - "platform.base_url": "https://platform-flag.example", - "docstudio.base_url": "https://docstudio-flag.example", - }, + {"p": {DOCSTUDIO: {**KEY, "api_key": "dk-deployment-key"}}}, ) - assert platform_base_url(config) == "https://platform-flag.example" + assert platform_client(config).api_key == KEY["platform_key"] -def test_an_environment_host_beats_a_profile_on_either_product() -> None: - """`$UNSTRACT_BASE_URL` maps to both products, and env outranks profile.""" - import os +def test_a_missing_platform_key_names_where_one_goes() -> None: + with pytest.raises(Exception, match="docstudio.platform_key") as caught: + platform_client(_resolved({"p": {}})) - config = _resolved({"p": {PLATFORM: {"base_url": "https://profile.example"}}}) - os.environ["UNSTRACT_BASE_URL"] = "https://env.example" - try: - assert platform_base_url(config) == "https://env.example" - finally: - del os.environ["UNSTRACT_BASE_URL"] - - -def test_the_built_in_default_is_the_last_resort_not_a_veto() -> None: - """Nobody named a host anywhere: the built-in default is still the answer. - `get_explicit` stopping before the defaults must not lose that. - """ - assert platform_base_url(_resolved({"p": {}})) == DEFAULT_BASE_URLS[PLATFORM] + assert "UNSTRACT_PLATFORM_KEY" in str(caught.value) + # A secret flag exists but is never suggested: it lands in shell history. + assert "--platform-key" not in str(caught.value) @pytest.mark.parametrize("value", [0, 0.0, -1]) @@ -159,7 +90,7 @@ def test_a_non_positive_timeout_is_refused_before_the_transport_sees_it(value) - an error that matches no arm in `__main__`. Refused at the flag instead, so the caller gets a usage error and an envelope. """ - config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + config = _resolved({"p": {DOCSTUDIO: KEY}}) with pytest.raises(CLIError) as caught: platform_client(config, timeout=value) @@ -172,7 +103,7 @@ def test_a_sub_second_timeout_survives_as_a_float() -> None: `--transport-timeout 0.5` as 0 -- the rejection above -- and silently rounded 1.9 down to 1. """ - config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + config = _resolved({"p": {DOCSTUDIO: KEY}}) assert platform_client(config, timeout=0.5).transport_timeout == 0.5 assert platform_client(config, timeout=1.9).transport_timeout == 1.9