From 2879eb26fff1656be0d998001502945716b66dfc Mon Sep 17 00:00:00 2001 From: Keegan Donley Date: Fri, 23 Oct 2026 14:44:26 -0500 Subject: [PATCH 1/3] KZN-19166 - support multiple sessions using different credentials --- README.md | 114 ++++---- src/commands/admin.ts | 17 +- src/commands/auth.ts | 17 +- src/commands/block.ts | 16 ++ src/lib/authSelection.test.ts | 125 ++++++++- src/lib/authSelection.ts | 87 ++++++- src/lib/kizenAdmin.test.ts | 208 ++++++++++++++- src/lib/kizenAdmin.ts | 104 +++++--- src/lib/oauthCommands.test.ts | 378 ++++++++++++++++++++++++--- src/lib/oauthCommands.ts | 213 ++++++++++----- src/lib/oauthSession.test.ts | 170 ++++++++++-- src/lib/oauthSession.ts | 91 +++---- src/lib/pushHeadless.test.ts | 473 +++++++++++++++++++++++++++++----- src/lib/pushHeadless.ts | 91 +++++-- src/program.test.ts | 48 +++- 15 files changed, 1781 insertions(+), 371 deletions(-) diff --git a/README.md b/README.md index 3fc1e86..9e444d6 100644 --- a/README.md +++ b/README.md @@ -239,27 +239,29 @@ Packages the plugin in the current directory (same validation as `block export`) appbuilder block push [api_name] [flags] ``` -Interactive by default: pick the environment (only when you're signed in to several and pass no `--env`), the block, the surface (dashboard, homepage or chart group), the dashboard, then whether to update an existing custom code dashlet or create a new one, and confirm. Pushing to a production environment (`go`, `fmo`) asks you to type `y`. In a TTY, `--yes` skips the confirm prompt only when the headless write gate would apply, so `go` and `fmo` still ask for a typed `y` unless `--allow-production` is also passed; `--dry-run` shows the plan without writing. A dashlet edited in Kizen since the last push still prompts before overwriting unless `--force` is passed. +Interactive by default: pick the environment (only when you pass no `--env` and you're signed in to several, or the account you name is saved in several), the block, the surface (dashboard, homepage or chart group), the dashboard, then whether to update an existing custom code dashlet or create a new one, and confirm. Pushing to a production environment (`go`, `fmo`) asks you to type `y`. In a TTY, `--yes` skips the confirm prompt only when the headless write gate would apply, so `go` and `fmo` still ask for a typed `y` unless `--allow-production` is also passed; `--dry-run` shows the plan without writing. A dashlet edited in Kizen since the last push still prompts before overwriting unless `--force` is passed. It runs headless when stdin or stdout is not a TTY, or when `--json` is passed. `--yes` never selects the mode. Headless runs never prompt: anything that would be a question fails with `needs_choice` and lists the choices. -| Argument / flag | Default | Purpose | -| -------------------------- | ------------------------------- | ------------------------------------------------------------------------------------------- | -| `[api_name]` | the plugin's only block | Block to push. Required when the plugin has more than one block. | -| `-c, --credentials ` | the sign-in session | API key (deprecated): credentials JSON file to use instead of the session | -| `--profile ` | the sign-in session | API key (deprecated): stored credential profile to use instead of the session | -| `--dashboard ` | the remembered target | Dashboard, homepage or chart group to push to | -| `--dashlet ` | matched by push history or name | Existing custom code dashlet to update. Needs `--dashboard`; can't be used with `--create`. | -| `--create` | off | Create a new dashlet instead of updating one | -| `--dry-run` | off | Resolve everything and print the request without writing | -| `--yes` | off | Write without asking. Headless runs without it are dry runs. | -| `--allow-production` | off | Allow headless writes to `go` / `fmo` | -| `--force` | off | Overwrite a dashlet that was edited in Kizen since the last push | -| `--forget` | off | Clear the remembered target for this block before resolving | -| `--json` | off | Print a JSON result to stdout (forces headless); the human summary goes to stderr | -| `--env ` | the only signed-in environment | Environment of the session to use. Not with `-c` or `--profile`. | - -**Credentials.** Without `-c` or `--profile`, the push uses your [`appbuilder login`](#appbuilder-login-logout-whoami-and-auth-header) session for `--env`, or for the only environment you're signed in to. If you're signed in to several environments and pass no `--env`, the run is `needs_choice` with `choice: "environment"`. If you're signed out, it fails with `credentials_invalid` and tells you to run `appbuilder login`; it never falls back to an API key. API keys (deprecated) are used only with `-c ` or `--profile `, and never touch the session; `--env` with either is a `usage_error`. The credentials file must name a valid `environment` (`go`, `fmo`, `staging`, `integration`, `test1`); a missing or invalid one is refused with `credentials_invalid` rather than defaulting to production. Empty `apiKey`, `userId` or `businessId` are refused the same way. A `.kizenapp/config.json` `activeCredentialProfile` no longer selects an API key: the push uses the session and prints a one-line notice to stderr, so `--json` output stays clean. `credentialMode: "local"` doesn't affect block commands. +| Argument / flag | Default | Purpose | +| -------------------------- | ---------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- | +| `[api_name]` | the plugin's only block | Block to push. Required when the plugin has more than one block. | +| `-c, --credentials ` | the sign-in session | API key (deprecated): credentials JSON file to use instead of the session | +| `--profile ` | the sign-in session | API key (deprecated): stored credential profile to use instead of the session | +| `--dashboard ` | the remembered target | Dashboard, homepage or chart group to push to | +| `--dashlet ` | matched by push history or name | Existing custom code dashlet to update. Needs `--dashboard`; can't be used with `--create`. | +| `--create` | off | Create a new dashlet instead of updating one | +| `--dry-run` | off | Resolve everything and print the request without writing | +| `--yes` | off | Write without asking. Headless runs without it are dry runs. | +| `--allow-production` | off | Allow headless writes to `go` / `fmo` | +| `--force` | off | Overwrite a dashlet that was edited in Kizen since the last push | +| `--forget` | off | Clear the remembered target for this block before resolving | +| `--json` | off | Print a JSON result to stdout (forces headless); the human summary goes to stderr | +| `--env ` | the only signed-in environment, or with `--business` the only one where it's saved | Environment of the session to use. Not with `-c` or `--profile`. | +| `--business ` | `$APPBUILDER_BUSINESS`, else the current account | Saved account to use, matched as for [`login --business`](#saved-accounts). Not with `-c` or `--profile`. | +| `--user ` | `$APPBUILDER_USER` | With `--business`, the user id of that saved account. Not with `-c` or `--profile`. | + +**Credentials.** Without `-c` or `--profile`, the push uses your [`appbuilder login`](#appbuilder-login-logout-whoami-and-auth-header) session for `--env`, or for the only environment you're signed in to. If you're signed in to several environments and pass no `--env`, the run is `needs_choice` with `choice: "environment"`. It uses that environment's current account, unless `--business` (and `--user`), or `APPBUILDER_BUSINESS` (and `APPBUILDER_USER`), names another saved account; then the environment without `--env` is the only one where that name matches a saved account, whether or not any account there is current, and several such environments are `needs_choice` the same way (see [Saved accounts](#saved-accounts)). If you're signed out, it fails with `credentials_invalid` and tells you to run `appbuilder login`; it never falls back to an API key. API keys (deprecated) are used only with `-c ` or `--profile `, and never touch the session; `--env`, `--business` or `--user` with either is a `usage_error`, and `APPBUILDER_BUSINESS` and `APPBUILDER_USER` are ignored. The credentials file must name a valid `environment` (`go`, `fmo`, `staging`, `integration`, `test1`); a missing or invalid one is refused with `credentials_invalid` rather than defaulting to production. Empty `apiKey`, `userId` or `businessId` are refused the same way. A `.kizenapp/config.json` `activeCredentialProfile` no longer selects an API key: the push uses the session and prints a one-line notice to stderr, so `--json` output stays clean. `credentialMode: "local"` doesn't affect block commands. **Write gating (headless).** `--dry-run` is always a dry run. Without `--yes` the run is a dry run too, and stderr says `Dry run only: pass --yes to write.` With `--yes`, `go` and `fmo` also need `--allow-production` (else `production_requires_flag`). If the target dashlet was pushed from here before and its content has since been changed in Kizen, both dry runs and writes stop with `drift_detected` unless `--force` is passed. @@ -359,24 +361,24 @@ A failure exits 1: Optional failure fields: `choice` (`environment`, `block`, `dashboard` or `target`) and `choices` with `needs_choice`, `remembered_target_missing` and a `--dashlet` that is `not_found`; `issues` (the packager's validation issues) with `validation_failed`; `field` with `invalid_block`; `hint` with `auth_failed` and `forbidden`. Each choice's `args` are the exact CLI arguments that select it, to be appended to the command (dashboard choices also carry `surface` and `canEdit`). -| Code | Meaning | -| --------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | -| `needs_choice` | An environment, block, dashboard or target must be chosen; see `choice` and `choices` | -| `validation_failed` | The plugin failed validation | -| `invalid_block` | The plugin has no blocks, or the block would be rejected (`field` names the problem) | -| `credentials_invalid` | Not signed in (run `appbuilder login`), or the `-c`/`--profile` file's `environment` is missing or invalid | -| `auth_failed` | Kizen rejected the credentials | -| `forbidden` | The credentials can't access the dashboard | -| `not_found` | The dashboard, or the `--dashlet` on it, doesn't exist | -| `remembered_target_missing` | The remembered dashboard or dashlet is gone; nothing is created without `--create` | -| `drift_detected` | The dashlet was edited in Kizen since the last push; `--force` overwrites | -| `production_requires_flag` | `--yes` against `go` / `fmo` without `--allow-production` | -| `usage_error` | Conflicting flags (`--dashlet` with `--create`, `--dashlet` without `--dashboard`), or a non-custom-code `--dashlet` | -| `network_error` | Kizen couldn't be reached | -| `api_error` | Any other Kizen API failure | -| `local_error` | A local failure that isn't a validation error: no `kizen.json`, packaging, reading `.kizenapp/pushes.json`, or writing it for `--forget` | - -Sign-in sessions add these cases: not being signed in is `credentials_invalid`; several signed-in environments and no `--env` is `needs_choice` (`choice: "environment"`, each choice `{ value, label, args: ["--env", ] }`); a session that needs a new `appbuilder login` (expired, rejected or unreadable) is `auth_failed`; `--env` with `-c` or `--profile`, or an unknown `--env`, is `usage_error`; an unreachable token endpoint is `network_error`; and any other failure to refresh the session is `api_error`. +| Code | Meaning | +| --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `needs_choice` | An environment, block, dashboard or target must be chosen; see `choice` and `choices` | +| `validation_failed` | The plugin failed validation | +| `invalid_block` | The plugin has no blocks, or the block would be rejected (`field` names the problem) | +| `credentials_invalid` | Not signed in (run `appbuilder login`), or the `-c`/`--profile` file's `environment` is missing or invalid | +| `auth_failed` | Kizen rejected the credentials | +| `forbidden` | The credentials can't access the dashboard | +| `not_found` | The dashboard, or the `--dashlet` on it, doesn't exist | +| `remembered_target_missing` | The remembered dashboard or dashlet is gone; nothing is created without `--create` | +| `drift_detected` | The dashlet was edited in Kizen since the last push; `--force` overwrites | +| `production_requires_flag` | `--yes` against `go` / `fmo` without `--allow-production` | +| `usage_error` | Conflicting flags (`--dashlet` with `--create`, `--dashlet` without `--dashboard`), a non-custom-code `--dashlet`, or a `--business` (or `APPBUILDER_BUSINESS`) that matches no saved account, or several | +| `network_error` | Kizen couldn't be reached | +| `api_error` | Any other Kizen API failure | +| `local_error` | A local failure that isn't a validation error: no `kizen.json`, packaging, reading `.kizenapp/pushes.json`, or writing it for `--forget` | + +Sign-in sessions add these cases: not being signed in is `credentials_invalid`; several signed-in environments and no `--env` is `needs_choice` (`choice: "environment"`, each choice `{ value, label, args: ["--env", ] }`), and so is a `--business` (or `APPBUILDER_BUSINESS`) that matches saved accounts in several environments, whose retry commands keep `--business` and `--user`; a session that needs a new `appbuilder login` (expired, rejected or unreadable) is `auth_failed`; `--env`, `--business` or `--user` with `-c` or `--profile`, or an unknown `--env`, is `usage_error`; a `--business` (or `APPBUILDER_BUSINESS`) that matches no saved account, or several, is `usage_error` with a message that lists them, and so is `--user` without `--business` (or `APPBUILDER_USER` without `APPBUILDER_BUSINESS`); an unreachable token endpoint is `network_error`; and any other failure to refresh the session is `api_error`. ### `appbuilder block targets` @@ -393,8 +395,10 @@ The CLI honors `HTTP_PROXY`/`HTTPS_PROXY` and `NO_PROXY` on Node versions that s | `--object ` | — | List the chart groups of this custom object (or Contacts) | | `--json` | off | Print JSON instead of tables | | `--env ` | as for `block push` | Environment of the session to use. Not with `-c`/`--profile`. | +| `--business ` | as for `block push` | Saved account to use. Not with `-c`/`--profile`. | +| `--user ` | as for `block push` | With `--business`, the user id of that saved account. | -Credentials and `--env` work as for `block push` (see [`appbuilder login`](#appbuilder-login-logout-whoami-and-auth-header)). +Credentials, `--env`, `--business` and `--user` work as for `block push` (see [`appbuilder login`](#appbuilder-login-logout-whoami-and-auth-header)). ```json { @@ -455,18 +459,22 @@ Browser sign-in, the default for `block push` and `block targets`. API keys are ```sh appbuilder login [--env ] [--business [--user ]] appbuilder login [--env ] --list [--json] -appbuilder whoami [--env ] +appbuilder whoami [--env ] [--business [--user ]] appbuilder logout [--env ] [--all | --business [--user ]] appbuilder auth-header [--env ] [--business --user ] -appbuilder block push [api_name] [--env ] [flags] -appbuilder block targets [--env ] [flags] +appbuilder block push [api_name] [--env ] [--business [--user ]] [flags] +appbuilder block targets [--env ] [--business [--user ]] [flags] ``` -`login` opens Kizen's consent page in your browser and prints the URL in case the browser doesn't open. There, you pick the business the session is for. The CLI waits up to 5 minutes for the browser to return to `http://127.0.0.1:/callback`. It uses OAuth 2.0 authorization code with PKCE (S256). It only trusts the server's OAuth metadata when its `issuer` is exactly the environment's origin (for example `https://staging.kizen.com`). It registers itself with the server on first use (dynamic client registration), so you don't need an API key or a client secret. A request to the callback with the wrong `state` gets a `400` and is otherwise ignored, so the sign-in keeps waiting for the real callback. Each session belongs to one user and one business, which the CLI looks up with `/auth/bootstrap` right after sign-in. If that lookup fails, nothing is saved. Signing in again to the same account replaces its session. A `block push` or `block targets` that's already running, or `auth-header`, keeps to the account it started with: if you switch to another account meanwhile, it stops and asks you to run it again. Signing in again to the same account doesn't stop it. If the browser shows an error instead of the consent page, the saved app registration is probably stale. The sign-in then times out and clears that app registration, so running `appbuilder login` again registers afresh. +`login` opens Kizen's consent page in your browser and prints the URL in case the browser doesn't open. There, you pick the business the session is for. The CLI waits up to 5 minutes for the browser to return to `http://127.0.0.1:/callback`. It uses OAuth 2.0 authorization code with PKCE (S256). It only trusts the server's OAuth metadata when its `issuer` is exactly the environment's origin (for example `https://staging.kizen.com`). It registers itself with the server on first use (dynamic client registration), so you don't need an API key or a client secret. A request to the callback with the wrong `state` gets a `400` and is otherwise ignored, so the sign-in keeps waiting for the real callback. Each session belongs to one user and one business, which the CLI looks up with `/auth/bootstrap` right after sign-in. If that lookup fails, nothing is saved. Signing in again to the same account replaces its session. A `block push` or `block targets` that's already running keeps to the account it started with: switching to another account meanwhile doesn't affect it, and neither does signing in again to the same account. Signing out of that account stops it with a sign-in error. If the browser shows an error instead of the consent page, the saved app registration is probably stale. The sign-in then times out and clears that app registration, so running `appbuilder login` again registers afresh. #### Saved accounts -Every account you sign in to stays saved on this machine, and one of them is current for each environment. `whoami`, `auth-header`, `block push` and `block targets` always use the current one. To switch without the browser, which is how scripts and agents should do it: +Every account you sign in to stays saved on this machine, and one of them is current for each environment. `whoami`, `auth-header`, `admin`, `block push` and `block targets` use the current one unless you name another. + +To use another saved account for one command without switching, pass `--business ` (and `--user `) to `whoami`, `admin`, `block push` or `block targets`, or set `APPBUILDER_BUSINESS` (and `APPBUILDER_USER`) in its environment. The flags win over the variables, and the variables over the current account; an empty variable counts as unset. The value is matched as for `login --business` below, among the saved accounts of one environment. With `--env`, that environment. Without it, the only environment where the value matches a saved account; whether any account there is current doesn't matter. If it matches in several environments, the command fails, names them and asks for `--env`. If it matches nowhere, the environment is the only one with any saved accounts, current or not, and the command fails listing them; with saved accounts in several environments, pass `--env`. If it matches no saved account in the environment, or several, the command fails and lists them; it never falls back to the current account. Naming an account never changes which one is current. `auth-header` ignores the variables, so an MCP `headersHelper` always serves the current account. + +To switch without the browser, which is how scripts and agents should do it: ```sh appbuilder login --env staging --list # one line per saved account; the current one is marked @@ -483,16 +491,16 @@ When both stdin and stdout are terminals and you pass neither `--business` nor ` `logout` removes the current account's session and leaves the other saved accounts. `logout --business ` removes one saved account, matched the same way as for `login`; if it's the current account, you're signed out too. `logout --all` removes every saved account for the environment. When no environment has a current account, both `--business` and `--all` need `--env`. -| Command | `--env` default | What it does | -| ------------- | ------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `login` | `go` | Signs in with the browser, or switches to a saved account, and stores the session | -| `whoami` | the only environment you're signed in to | Calls `/auth/bootstrap` with the session and prints the user and business. Tokens are never printed. | -| `logout` | the only environment you're signed in to | Deletes the current account's session, one saved account (`--business`), or all of them (`--all`). Local only: the refresh tokens stay valid on Kizen. | -| `auth-header` | from the MCP server URL ([see below](#using-the-session-from-claude-code-mcp)) | Prints `{"Authorization":"Bearer "}` for a Claude Code MCP `headersHelper`. With `--business` and `--user` (used together), it serves only that saved account, and fails while another account is current; [`appbuilder admin`](#appbuilder-admin) passes them. | +| Command | `--env` default | What it does | +| ------------- | ------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `login` | `go` | Signs in with the browser, or switches to a saved account, and stores the session | +| `whoami` | the only environment you're signed in to; with `--business`, the only one where it's saved | Calls `/auth/bootstrap` with the current account's session, or the saved account `--business` names, and prints the user and business. Tokens are never printed. | +| `logout` | the only environment you're signed in to | Deletes the current account's session, one saved account (`--business`), or all of them (`--all`). Local only: the refresh tokens stay valid on Kizen. | +| `auth-header` | from the MCP server URL ([see below](#using-the-session-from-claude-code-mcp)) | Prints `{"Authorization":"Bearer "}` for a Claude Code MCP `headersHelper`. With `--business` and `--user` (used together, by id), it serves only that saved account, whether or not it's current; [`appbuilder admin`](#appbuilder-admin) passes them. | The production write gate (`--allow-production`) and every other flag of `block push` and `block targets` work the same with a session as with an API key. -Sessions are stored in `~/.kizenappbuilder/oauth/` (directory `0700`, files `0600`), separate from API-key profiles. Each saved account has its own file, `...json`, and `.json` names the current account. Access tokens last an hour and refresh automatically: when less than a minute is left, or once after a `401`. Refresh tokens rotate on each use. Each process makes one refresh at a time, and every file is written atomically. Concurrent `appbuilder` processes may each refresh, which is safe because Kizen returns the same token pair when a refresh token is reused within 5 minutes. Before saving, a refresh re-reads the session: it keeps the newer tokens if another process already refreshed or you signed in again to the same account, and it fails with a sign-in error if you signed out or switched to another account meanwhile. If the refresh token is rejected, the CLI keeps the tokens another process already refreshed to; otherwise it deletes that account's file, and the command asks you to run `appbuilder login` again (with `--env ` for an environment other than `go`). A refresh only ever writes or deletes its own account's file, and only `login` and `logout` change which account is current, so a refresh that finishes late can't undo a logout or a switch. +Sessions are stored in `~/.kizenappbuilder/oauth/` (directory `0700`, files `0600`), separate from API-key profiles. Each saved account has its own file, `...json`, and `.json` names the current account. Access tokens last an hour and refresh automatically: when less than a minute is left, or once after a `401`. Refresh tokens rotate on each use. Each process makes one refresh at a time, and every file is written atomically. Concurrent `appbuilder` processes may each refresh, which is safe because Kizen returns the same token pair when a refresh token is reused within 5 minutes. Before saving, a refresh re-reads the session: it keeps the newer tokens if another process already refreshed or you signed in again to the same account, and it fails with a sign-in error if you signed out of that account meanwhile. A switch to another account doesn't affect it. If the refresh token is rejected, the CLI keeps the tokens another process already refreshed to; otherwise it deletes that account's file, and the command asks you to run `appbuilder login` again (with `--env ` for an environment other than `go`). A refresh only ever writes or deletes its own account's file, and only `login` and `logout` change which account is current, so a refresh that finishes late can't undo a logout or a switch. A session file that is corrupt or can't be read isn't treated as "not signed in". Commands that use it report the file's path and ask you to run `appbuilder login` (with `--env ` for an environment other than `go`). `appbuilder logout --env ` signs you out either way. If the session file is the problem, logout removes it. If `.json` is the problem, logout removes that and leaves the saved accounts, and `logout --all` removes those too. `login --list` still lists a saved account whose file can't be read, by its ids. @@ -534,17 +542,17 @@ Claude Code gives the helper 10 seconds, so `auth-header` uses shorter timeouts Runs Kizen's admin CLI, `kizen`, signed in with your `appbuilder login` session. You don't install `kizen` yourself: you only need Python 3.12 or newer on `PATH`. ```sh -appbuilder admin [--env ] [--] +appbuilder admin [--env ] [--business [--user ]] [--] appbuilder admin init [--force] ``` -`--env` picks the session, and defaults to the only environment you're signed in to. The wrapper's own options (`--env`, `-h`/`--help`) come first and end at the first other argument or at `--`. Everything after that goes to `kizen` verbatim, so `appbuilder admin status --help` shows `kizen`'s help, and `appbuilder admin --help` shows this wrapper's. `init` is the wrapper's own subcommand only as the first argument; `appbuilder admin -- init` passes `init` to `kizen`. +`--env` picks the session, and defaults to the only environment you're signed in to. `kizen` runs as that environment's current account, unless `--business` (and `--user`), or `APPBUILDER_BUSINESS` (and `APPBUILDER_USER`), names another saved account; then `--env` defaults to the only environment where that name matches a saved account (see [Saved accounts](#saved-accounts)). The wrapper's own options (`--env`, `--business`, `--user`, `-h`/`--help`) come first and end at the first other argument or at `--`. Everything after that goes to `kizen` verbatim, so `appbuilder admin status --help` shows `kizen`'s help, and `appbuilder admin --help` shows this wrapper's. `init` is the wrapper's own subcommand only as the first argument; `appbuilder admin -- init` passes `init` to `kizen`. On first use, `appbuilder admin` installs `kizen` into a private virtual environment under `~/.kizenappbuilder/kizen/-/` and reuses it afterwards without touching the network. `appbuilder admin init` does that ahead of time, and needs no session. It uses the newest of `python3.14`, `python3.13`, `python3.12` and `python3` that is 3.12 or newer, downloads the `kizen` release this CLI version pins (currently 0.3.0) from its GitHub release, checks its sha256, and installs it and its dependencies from PyPI with `pip`. Progress goes to stderr; stdout is `kizen`'s. Each time it makes a new install, it removes installs for other `kizen` versions that have gone unused for 30 days. If the install is damaged (for example, the Python it was built with was removed), the command says so and changes nothing. `appbuilder admin init --force` deletes the install and installs it again. Don't run it while `appbuilder admin` is running, since a running `kizen` uses that install. -Under app-builder, `kizen` gets your session through `KIZEN_AUTH_HELPER`: it names `appbuilder auth-header --env --business --user `, which `kizen` runs when it needs a token, and `KIZEN_BASE_URL`, `KIZEN_BUSINESS_ID` and `KIZEN_USER_ID` identify the session. With `--business` and `--user`, `auth-header` serves only that account, and only while it's the environment's current one. `kizen` keeps the token for the life of its process and runs the helper again only after a `401`. So if you switch accounts with `appbuilder login` while `kizen` runs, its next token fetch (at startup, or after a `401`) fails, and you run the command again. A token it already holds is still its own account's, and stays in use until it expires. The token never appears in `kizen`'s environment or arguments. `KIZEN_PROFILE` and `CLAUDE_CODE_MCP_SERVER_URL` are removed from its environment. Under app-builder, `kizen` refuses `init`, `envs list` and `upgrade` (`upgrade --check` reports that app-builder manages the install). The command exits with `kizen`'s exit code. +Under app-builder, `kizen` gets your session through `KIZEN_AUTH_HELPER`: it names `appbuilder auth-header --env --business --user `, which `kizen` runs when it needs a token, and `KIZEN_BASE_URL`, `KIZEN_BUSINESS_ID` and `KIZEN_USER_ID` identify the session. With `--business` and `--user`, `auth-header` serves only that account, whether or not it's current, so switching accounts with `appbuilder login` while `kizen` runs doesn't affect it. `kizen` keeps the token for the life of its process and runs the helper again only after a `401`. If you sign out of that account while `kizen` runs, its next token fetch (at startup, or after a `401`) fails. The token never appears in `kizen`'s environment or arguments. `KIZEN_PROFILE` and `CLAUDE_CODE_MCP_SERVER_URL` are removed from its environment. Under app-builder, `kizen` refuses `init`, `envs list` and `upgrade` (`upgrade --check` reports that app-builder manages the install). The command exits with `kizen`'s exit code. To try a local `kizen` build, set `APPBUILDER_KIZEN_WHEEL` to the path of a `kizen_builder--….whl`. It's installed instead of the pinned release, with no download or hash check. @@ -563,6 +571,8 @@ Prod follows the same order minus step 3: `PLUGIN_WIZARD_URL`, then **`PLUGIN_WI **`CLAUDE_CODE_MCP_SERVER_URL`** is set by Claude Code when it runs `appbuilder auth-header`, which uses it to pick the environment (see [Using the session from Claude Code](#using-the-session-from-claude-code-mcp)). +**`APPBUILDER_BUSINESS`** and **`APPBUILDER_USER`** name a saved account for `whoami`, `admin`, `block push` and `block targets` to use instead of the current one, like `--business` and `--user`, which win over them. `APPBUILDER_USER` only applies with `APPBUILDER_BUSINESS`, and an empty value counts as unset. `auth-header` ignores them, and so do `block push` and `block targets` with `-c` or `--profile` (see [Saved accounts](#saved-accounts)). + **`APPBUILDER_KIZEN_WHEEL`** is the path of a local `kizen_builder--….whl` for `appbuilder admin` to install instead of the `kizen` release this CLI version pins, with no download or hash check (see [`appbuilder admin`](#appbuilder-admin)). ### The `.kizenapp/` directory diff --git a/src/commands/admin.ts b/src/commands/admin.ts index 8522c1a..5ee362c 100644 --- a/src/commands/admin.ts +++ b/src/commands/admin.ts @@ -63,8 +63,21 @@ export function adminCommand(program: Command): void { const admin = new RawArgsCommand(ADMIN) .copyInheritedSettings(program) .description("Run Kizen's admin CLI (kizen) with the signed-in session") - .usage('[--env ] [--] \n appbuilder admin init [--force]') - .option('--env ', 'environment to use (default: the only signed-in one)') + .usage( + '[--env ] [--business [--user ]] [--] \n appbuilder admin init [--force]', + ) + .option( + '--env ', + "environment to use (default: the only signed-in one, or with --business the only one where it's saved)", + ) + .option( + '--business ', + 'use this saved account, by business id or name (ignoring case), instead of the current one (default: $APPBUILDER_BUSINESS)', + ) + .option( + '--user ', + 'with --business, use the saved account of this user id (default: $APPBUILDER_USER)', + ) .allowExcessArguments() .addHelpText( 'after', diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 4b6b55a..a3c2389 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -54,7 +54,17 @@ const AUTH_COMMANDS: AuthCommandSpec[] = [ { name: 'whoami', description: 'Show the user and business of the stored session', - env: 'environment to check (default: the only signed-in one)', + env: "environment to check (default: the only signed-in one, or with --business the only one where it's saved)", + options: [ + [ + '--business ', + 'check this saved account, by business id or name (ignoring case), instead of the current one (default: $APPBUILDER_BUSINESS)', + ], + [ + '--user ', + 'with --business, check the saved account of this user id (default: $APPBUILDER_USER)', + ], + ], run: runWhoami, }, { @@ -63,10 +73,7 @@ const AUTH_COMMANDS: AuthCommandSpec[] = [ 'Print an Authorization header for the stored session as JSON (for a Claude Code MCP headersHelper)', env: 'environment to use; must match CLAUDE_CODE_MCP_SERVER_URL when that is set (default: the environment matching it, else the only signed-in one)', options: [ - [ - '--business ', - "with --user, use only this saved account, and fail if it isn't the environment's current one", - ], + ['--business ', 'with --user, use this saved account, current or not'], ['--user ', 'with --business, the user id of that account'], ], run: runAuthHeader, diff --git a/src/commands/block.ts b/src/commands/block.ts index f41627c..8c8467b 100644 --- a/src/commands/block.ts +++ b/src/commands/block.ts @@ -85,6 +85,14 @@ export function blockCommand(program: Command): void { .option('--forget', 'clear the remembered target for this block first') .option('--json', 'print a JSON result to stdout (always headless)') .option('--env ', 'environment of the `appbuilder login` session to use') + .option( + '--business ', + 'use this saved account, by business id or name (ignoring case), instead of the current one (default: $APPBUILDER_BUSINESS)', + ) + .option( + '--user ', + 'with --business, use the saved account of this user id (default: $APPBUILDER_USER)', + ) .action(async (apiName: string | undefined, options: PushCommandOptions) => { try { const deps = createDefaultPushDeps(process.cwd()); @@ -124,6 +132,14 @@ export function blockCommand(program: Command): void { .option('--object ', 'list the chart groups of this custom object') .option('--json', 'print a JSON result to stdout') .option('--env ', 'environment of the `appbuilder login` session to use') + .option( + '--business ', + 'use this saved account, by business id or name (ignoring case), instead of the current one (default: $APPBUILDER_BUSINESS)', + ) + .option( + '--user ', + 'with --business, use the saved account of this user id (default: $APPBUILDER_USER)', + ) .action(async (options: TargetsCommandOptions) => { try { await runTargetsHeadless(options, createDefaultPushDeps(process.cwd())); diff --git a/src/lib/authSelection.test.ts b/src/lib/authSelection.test.ts index d7ffeb3..dbfd041 100644 --- a/src/lib/authSelection.test.ts +++ b/src/lib/authSelection.test.ts @@ -1,7 +1,10 @@ import { describe, expect, it } from 'vitest'; import { + accountRequest, pinnedProfileNotice, selectAuth, + type AccountFlags, + type AccountRequestResult, type AuthFlags, type AuthSelectionResult, } from './authSelection.js'; @@ -38,7 +41,127 @@ describe('selectAuth', () => { }, ], ])('%s', (_label, flags, expected) => { - expect(selectAuth(flags)).toStrictEqual(expected); + expect(selectAuth(flags, {})).toStrictEqual(expected); + }); +}); + +describe('selectAuth and a named saved account', () => { + it.each<[string, AuthFlags, Record, AuthSelectionResult]>([ + [ + '--business with --env selects that saved account in that session', + { env: 'staging', business: 'Acme', user: 'u1' }, + {}, + { + ok: true, + selection: { + kind: 'oauth', + env: 'staging', + account: { business: 'Acme', user: 'u1', source: 'flags' }, + }, + }, + ], + [ + 'APPBUILDER_BUSINESS selects that saved account', + {}, + { APPBUILDER_BUSINESS: 'Acme' }, + { + ok: true, + selection: { + kind: 'oauth', + account: { business: 'Acme', user: undefined, source: 'env' }, + }, + }, + ], + [ + '--business with --profile is a usage error', + { profile: 'Demo', business: 'Acme' }, + {}, + { + ok: false, + message: + "--business selects a saved sign-in account, so it can't be combined with -c or --profile.", + }, + ], + [ + '--user with -c is a usage error', + { credentials: '/tmp/c.json', user: 'u1' }, + {}, + { + ok: false, + message: + "--user selects a saved sign-in account, so it can't be combined with -c or --profile.", + }, + ], + [ + '-c ignores APPBUILDER_BUSINESS and APPBUILDER_USER', + { credentials: '/tmp/c.json' }, + { APPBUILDER_BUSINESS: 'Acme', APPBUILDER_USER: 'u1' }, + { ok: true, selection: { kind: 'api-key', credentials: '/tmp/c.json' } }, + ], + [ + '--profile ignores a lone APPBUILDER_USER', + { profile: 'Demo' }, + { APPBUILDER_USER: 'u1' }, + { ok: true, selection: { kind: 'api-key', profile: 'Demo' } }, + ], + [ + 'the session with a lone APPBUILDER_USER is a usage error', + {}, + { APPBUILDER_USER: 'u1' }, + { ok: false, message: 'APPBUILDER_USER only applies with APPBUILDER_BUSINESS.' }, + ], + ])('%s', (_label, flags, processEnv, expected) => { + expect(selectAuth(flags, processEnv)).toStrictEqual(expected); + }); +}); + +describe('accountRequest', () => { + it.each<[string, AccountFlags, Record, AccountRequestResult]>([ + ['nothing names an account: the current one', {}, {}, { ok: true, request: undefined }], + [ + '--business alone', + { business: 'Acme' }, + {}, + { ok: true, request: { business: 'Acme', user: undefined, source: 'flags' } }, + ], + [ + 'the flags replace the variables as a pair', + { business: 'Acme' }, + { APPBUILDER_BUSINESS: 'Other', APPBUILDER_USER: 'u9' }, + { ok: true, request: { business: 'Acme', user: undefined, source: 'flags' } }, + ], + [ + 'the variables when no flag is passed', + {}, + { APPBUILDER_BUSINESS: 'Other', APPBUILDER_USER: 'u9' }, + { ok: true, request: { business: 'Other', user: 'u9', source: 'env' } }, + ], + [ + 'an empty APPBUILDER_BUSINESS is unset', + {}, + { APPBUILDER_BUSINESS: '' }, + { ok: true, request: undefined }, + ], + [ + 'an empty APPBUILDER_USER is unset', + {}, + { APPBUILDER_BUSINESS: 'Other', APPBUILDER_USER: '' }, + { ok: true, request: { business: 'Other', user: undefined, source: 'env' } }, + ], + [ + '--user without --business', + { user: 'u1' }, + { APPBUILDER_BUSINESS: 'Other' }, + { ok: false, message: '--user only applies with --business.' }, + ], + [ + 'APPBUILDER_USER without APPBUILDER_BUSINESS', + {}, + { APPBUILDER_BUSINESS: '', APPBUILDER_USER: 'u1' }, + { ok: false, message: 'APPBUILDER_USER only applies with APPBUILDER_BUSINESS.' }, + ], + ])('%s', (_label, flags, processEnv, expected) => { + expect(accountRequest(flags, processEnv)).toStrictEqual(expected); }); }); diff --git a/src/lib/authSelection.ts b/src/lib/authSelection.ts index 27c7960..6743166 100644 --- a/src/lib/authSelection.ts +++ b/src/lib/authSelection.ts @@ -6,17 +6,72 @@ import { quoteArg } from './shellArg.js'; -export interface AuthFlags { +/** `--business` / `--user`: a saved account to use instead of the current one. */ +export interface AccountFlags { + business?: string; + user?: string; +} + +export interface AuthFlags extends AccountFlags { credentials?: string; profile?: string; env?: string; } +/** The environment variables that name a saved account when the flags don't. */ +export const BUSINESS_ENV = 'APPBUILDER_BUSINESS'; +export const USER_ENV = 'APPBUILDER_USER'; + +/** + * A saved account a command names, by business id or name and optionally user id. `source` says + * where it came from, so an error can name what to change. + */ +export interface AccountRequest { + business: string; + user?: string | undefined; + source?: 'flags' | 'env' | undefined; +} + +export type AccountRequestResult = + | { ok: true; request: AccountRequest | undefined } + | { ok: false; message: string }; + +/** + * The saved account a command names: `--business` (and `--user`), else `APPBUILDER_BUSINESS` + * (and `APPBUILDER_USER`), else undefined for the current account. The flags replace the + * variables as a pair, and a user id without a business is a usage error. An empty variable + * counts as unset. + */ +export function accountRequest( + flags: AccountFlags, + processEnv: Readonly>, +): AccountRequestResult { + if (flags.business !== undefined) { + return { ok: true, request: { business: flags.business, user: flags.user, source: 'flags' } }; + } + + if (flags.user !== undefined) { + return { ok: false, message: '--user only applies with --business.' }; + } + + const business = processEnv[BUSINESS_ENV] || undefined; + const user = processEnv[USER_ENV] || undefined; + + if (business === undefined) { + return user === undefined + ? { ok: true, request: undefined } + : { ok: false, message: `${USER_ENV} only applies with ${BUSINESS_ENV}.` }; + } + + return { ok: true, request: { business, user, source: 'env' } }; +} + export type AuthSelection = /** Exactly one of the two is set; `-c` wins when both were passed. */ | { kind: 'api-key'; credentials: string; profile?: never } | { kind: 'api-key'; profile: string; credentials?: never } - | { kind: 'oauth'; env?: string }; + /** `account` names a saved account; without it, the current one. */ + | { kind: 'oauth'; env?: string; account?: AccountRequest }; /** `ok: false` is a usage error. */ export type AuthSelectionResult = @@ -26,7 +81,14 @@ export type AuthSelectionResult = const API_KEY_FLAGS_HINT = 'To use an API key instead (deprecated), pass --profile or -c .'; -export function selectAuth(flags: AuthFlags): AuthSelectionResult { +/** + * `processEnv` supplies `APPBUILDER_BUSINESS` / `APPBUILDER_USER` for the sign-in session; an API + * key ignores them. + */ +export function selectAuth( + flags: AuthFlags, + processEnv: Readonly>, +): AuthSelectionResult { const apiKey = flags.credentials !== undefined || flags.profile !== undefined; if (apiKey && flags.env !== undefined) { @@ -36,6 +98,13 @@ export function selectAuth(flags: AuthFlags): AuthSelectionResult { }; } + if (apiKey && (flags.business !== undefined || flags.user !== undefined)) { + return { + ok: false, + message: `${flags.business === undefined ? '--user' : '--business'} selects a saved sign-in account, so it can't be combined with -c or --profile.`, + }; + } + if (flags.credentials !== undefined) { return { ok: true, selection: { kind: 'api-key', credentials: flags.credentials } }; } @@ -44,9 +113,19 @@ export function selectAuth(flags: AuthFlags): AuthSelectionResult { return { ok: true, selection: { kind: 'api-key', profile: flags.profile } }; } + const account = accountRequest(flags, processEnv); + + if (!account.ok) { + return account; + } + return { ok: true, - selection: flags.env === undefined ? { kind: 'oauth' } : { kind: 'oauth', env: flags.env }, + selection: { + kind: 'oauth', + ...(flags.env !== undefined && { env: flags.env }), + ...(account.request !== undefined && { account: account.request }), + }, }; } diff --git a/src/lib/kizenAdmin.test.ts b/src/lib/kizenAdmin.test.ts index 8fa304a..159bb4f 100644 --- a/src/lib/kizenAdmin.test.ts +++ b/src/lib/kizenAdmin.test.ts @@ -1,6 +1,9 @@ import { execFileSync } from 'node:child_process'; import type { ChildProcess } from 'node:child_process'; import { EventEmitter } from 'node:events'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, expect, it, vi, type Mock } from 'vitest'; import type { Environment } from './credentials.js'; import { @@ -12,7 +15,13 @@ import { type SpawnKizen, } from './kizenAdmin.js'; import type { KizenInstall } from './kizenVenv.js'; -import type { OAuthSession } from './oauthStore.js'; +import { + createOAuthStore, + type OAuthAccount, + type OAuthSession, + type SavedAccount, +} from './oauthStore.js'; +import { ACCOUNT_A, ACCOUNT_B, oauthSession, signIn } from '../test/oauth.js'; const BUSINESS_ID = '11111111-1111-4111-8111-111111111111'; const USER_ID = '22222222-2222-4222-8222-222222222222'; @@ -48,6 +57,10 @@ interface FakeAdminDeps extends AdminDeps { store: { listSessions: Mock<() => Promise>; readSession: Mock<(environment: Environment) => Promise>; + readAccount: Mock< + (environment: Environment, account: OAuthAccount) => Promise + >; + listAccounts: Mock<(environment: Environment) => Promise>; }; ensureKizen: Mock; spawn: Mock; @@ -103,6 +116,8 @@ function fakeDeps( readSession: vi.fn((environment: Environment) => Promise.resolve(options.signedIn === false ? null : session(environment)), ), + readAccount: vi.fn(() => Promise.resolve(null)), + listAccounts: vi.fn(() => Promise.resolve([])), }, ensureKizen: vi.fn(() => Promise.resolve(INSTALL)), spawn, @@ -184,6 +199,30 @@ describe('parseAdminArgs', () => { ], [['--env'], { kind: 'error', message: "option '--env ' argument missing" }], [['--env='], { kind: 'error', message: "option '--env ' argument missing" }], + [ + ['--business', 'Acme', '--user', USER_ID, 'status'], + { kind: 'run', env: undefined, business: 'Acme', user: USER_ID, args: ['status'] }, + ], + [ + ['--user=u', '--env=go', '--business=b', '--', '--business', 'x'], + { kind: 'run', env: 'go', business: 'b', user: 'u', args: ['--business', 'x'] }, + ], + [['--business', 'b', 'init'], { kind: 'init', force: false }], + // After the first kizen argument, --business and --user are kizen's, verbatim. + [ + ['status', '--business', 'x', '--user', 'y'], + { kind: 'run', env: undefined, args: ['status', '--business', 'x', '--user', 'y'] }, + ], + [ + ['--business'], + { kind: 'error', message: "option '--business ' argument missing" }, + ], + [ + ['--business=', 'status'], + { kind: 'error', message: "option '--business ' argument missing" }, + ], + [['--user'], { kind: 'error', message: "option '--user ' argument missing" }], + [['--user='], { kind: 'error', message: "option '--user ' argument missing" }], ])('%j', (args, expected) => { expect(parseAdminArgs(args)).toStrictEqual(expected); }); @@ -543,3 +582,170 @@ describe('runAdmin', () => { expect(deps.ensureKizen).not.toHaveBeenCalled(); }); }); + +describe('runAdmin with a named saved account', () => { + /** + * A real store with account A current and B ("Bravo") saved but not current; `run` gets deps + * whose store is it. + */ + const withAccounts = async ( + run: ( + deps: AdminDeps & FakeAdminDeps, + stderr: string[], + store: ReturnType, + ) => Promise, + processEnv: Record = { PATH: '/usr/bin' }, + ): Promise => { + const dir = await mkdtemp(join(tmpdir(), 'appbuilder-admin-')); + + try { + const store = createOAuthStore(dir); + + await store.writeSession(oauthSession({ ...ACCOUNT_B, businessName: 'Bravo' })); + await signIn(store, oauthSession()); + + const { deps, stderr } = fakeDeps({ processEnv }); + + await run({ ...deps, store } as unknown as AdminDeps & FakeAdminDeps, stderr, store); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }; + + const boundTo = (account: OAuthAccount): Record => ({ + KIZEN_AUTH_HELPER: `/usr/local/bin/node /opt/appbuilder/dist/index.js auth-header --env staging --business ${account.businessId} --user ${account.userId}`, + KIZEN_BASE_URL: 'https://staging.kizen.com', + KIZEN_BUSINESS_ID: account.businessId, + KIZEN_USER_ID: account.userId, + }); + + it("--business binds kizen to that account while another is current, and later --business is kizen's", async () => { + await withAccounts(async (deps, stderr, store) => { + await runAdmin(['--business', 'bravo', '--', 'status', '--business', 'x'], deps); + + expect(stderr).toStrictEqual([]); + + const { args, options } = spawnCall(deps); + + expect(args).toStrictEqual([INSTALL.kizen, 'status', '--business', 'x']); + expect(options.env).toStrictEqual({ PATH: '/usr/bin', ...boundTo(ACCOUNT_B) }); + // The pointer never moves. + expect(await store.readSession('staging')).toMatchObject(ACCOUNT_A); + }); + }); + + it('without --env, --business finds its environment when nothing is current', async () => { + await withAccounts(async (deps, stderr, store) => { + await store.signOut('staging'); + + await runAdmin(['--business', 'bravo', 'status'], deps); + + expect(stderr).toStrictEqual([]); + expect(spawnCall(deps).options.env).toStrictEqual({ + PATH: '/usr/bin', + ...boundTo(ACCOUNT_B), + }); + expect(await store.listSessions()).toStrictEqual([]); + }); + }); + + it('with --env, --business binds kizen to that account when nothing is current', async () => { + await withAccounts(async (deps, stderr, store) => { + await store.signOut('staging'); + + await runAdmin(['--env', 'staging', '--business', 'bravo', 'status'], deps); + + expect(stderr).toStrictEqual([]); + expect(spawnCall(deps).options.env).toStrictEqual({ + PATH: '/usr/bin', + ...boundTo(ACCOUNT_B), + }); + expect(await store.listSessions()).toStrictEqual([]); + }); + }); + + it('without --env, a name saved in staging and go asks for --env', async () => { + await withAccounts(async (deps, stderr, store) => { + await store.writeSession( + oauthSession({ ...ACCOUNT_B, environment: 'go', businessName: 'Bravo' }), + ); + + await runAdmin(['--business', 'bravo', 'status'], deps); + + expect(stderr).toStrictEqual([ + 'appbuilder admin: --business bravo matches saved accounts in several environments; pass --env : go, staging\n', + ]); + expect(deps.ensureKizen).not.toHaveBeenCalled(); + expect(deps.spawn).not.toHaveBeenCalled(); + }); + }); + + it('--business after the first kizen argument leaves kizen on the current account', async () => { + await withAccounts(async (deps) => { + await runAdmin(['status', '--business', 'bravo'], deps); + + const { args, options } = spawnCall(deps); + + expect(args).toStrictEqual([INSTALL.kizen, 'status', '--business', 'bravo']); + expect(options.env).toMatchObject(boundTo(ACCOUNT_A)); + }); + }); + + it.each<[string, Record, string[], OAuthAccount]>([ + ['APPBUILDER_BUSINESS names it', { APPBUILDER_BUSINESS: 'Bravo' }, ['status'], ACCOUNT_B], + [ + '--business beats APPBUILDER_BUSINESS', + { APPBUILDER_BUSINESS: 'Bravo', APPBUILDER_USER: ACCOUNT_B.userId }, + ['--business', ACCOUNT_A.businessId, 'status'], + ACCOUNT_A, + ], + [ + 'an empty APPBUILDER_BUSINESS counts as unset', + { APPBUILDER_BUSINESS: '' }, + ['status'], + ACCOUNT_A, + ], + ])('%s', async (_name, env, args, expected) => { + await withAccounts( + async (deps) => { + await runAdmin(args, deps); + + expect(spawnCall(deps).options.env).toMatchObject(boundTo(expected)); + }, + { PATH: '/usr/bin', ...env }, + ); + }); + + it.each<[string, Record, string[], string]>([ + [ + 'a name no saved account matches', + {}, + ['--business', 'Nope', 'status'], + `appbuilder admin: No saved staging account matches --business Nope. Saved accounts:\n Acme A (${ACCOUNT_A.businessId}) ada@a.test\n Bravo (${ACCOUNT_B.businessId}) ada@a.test\n`, + ], + [ + 'APPBUILDER_USER without APPBUILDER_BUSINESS', + { APPBUILDER_USER: ACCOUNT_B.userId }, + ['status'], + 'appbuilder admin: APPBUILDER_USER only applies with APPBUILDER_BUSINESS.\n', + ], + [ + '--user without --business', + {}, + ['--user', ACCOUNT_B.userId, 'status'], + 'appbuilder admin: --user only applies with --business.\n', + ], + ])('%s: no provisioning, no spawn, exit 1', async (_name, env, args, line) => { + await withAccounts( + async (deps, stderr) => { + await runAdmin(args, deps); + + expect(stderr).toStrictEqual([line]); + expect(deps.ensureKizen).not.toHaveBeenCalled(); + expect(deps.spawn).not.toHaveBeenCalled(); + expect(deps.setExitCode).toHaveBeenCalledWith(1); + }, + { PATH: '/usr/bin', ...env }, + ); + }); +}); diff --git a/src/lib/kizenAdmin.ts b/src/lib/kizenAdmin.ts index 89d3ac1..58eddef 100644 --- a/src/lib/kizenAdmin.ts +++ b/src/lib/kizenAdmin.ts @@ -8,12 +8,17 @@ import type { ChildProcess, SpawnOptions } from 'node:child_process'; import { constants } from 'node:os'; import { apiOriginFor } from '../../shared/lib/kizenUrls.js'; +import type { AccountFlags } from './authSelection.js'; import type { Environment } from './credentials.js'; import { errorMessage } from './guards.js'; import type { KizenInstall } from './kizenVenv.js'; import { loginRequired } from './oauth.js'; -import { resolveSessionEnvironment } from './oauthSession.js'; -import type { OAuthAccount, OAuthStore } from './oauthStore.js'; +import { + resolveAccountEnvironment, + requireAccountRequest, + requireNamedAccount, +} from './oauthCommands.js'; +import type { OAuthAccount, OAuthSession, OAuthStore } from './oauthStore.js'; import { quoteArg } from './shellArg.js'; export const ADMIN_PREFIX = 'appbuilder admin'; @@ -23,44 +28,59 @@ export type AdminInvocation = | { kind: 'help' } | { kind: 'init'; force: boolean } | { kind: 'init-help' } - | { kind: 'run'; env: string | undefined; args: string[] } + | ({ kind: 'run'; env: string | undefined; args: string[] } & AccountFlags) | { kind: 'error'; message: string }; -const ENV_FLAG = '--env'; +/** The wrapper's options that take a value: the flag, and its usage for a missing argument. */ +const VALUE_FLAGS = { + env: ['--env', ''], + business: ['--business', ''], + user: ['--user', ''], +} as const; const HELP_FLAGS: readonly string[] = ['-h', '--help']; const FORCE_FLAG = '--force'; /** - * Wrapper options (`--env `, `--env=`, `-h`/`--help`) come first and end at the first - * other token or at `--`; everything after goes to kizen verbatim. `init` is the wrapper's own - * subcommand only as the first token after the wrapper options, never after `--`. + * Wrapper options (`--env `, `--business `, `--user `, each also as + * `--flag=`, and `-h`/`--help`) come first and end at the first other token or at `--`; + * everything after goes to kizen verbatim. `init` is the wrapper's own subcommand only as the + * first token after the wrapper options, never after `--`. */ export function parseAdminArgs(args: readonly string[]): AdminInvocation { - let env: string | undefined; + const values: { env?: string } & AccountFlags = {}; let index = 0; + const run = (rest: string[]): AdminInvocation => ({ + kind: 'run', + env: values.env, + ...(values.business !== undefined && { business: values.business }), + ...(values.user !== undefined && { user: values.user }), + args: rest, + }); - while (index < args.length) { + options: while (index < args.length) { const arg = args[index] ?? ''; if (arg === '--') { - return { kind: 'run', env, args: args.slice(index + 1) }; + return run(args.slice(index + 1)); } if (HELP_FLAGS.includes(arg)) { return { kind: 'help' }; } - if (arg === ENV_FLAG || arg.startsWith(`${ENV_FLAG}=`)) { - const value = arg === ENV_FLAG ? args[index + 1] : arg.slice(ENV_FLAG.length + 1); + for (const [key, [flag, usage]] of Object.entries(VALUE_FLAGS)) { + if (arg === flag || arg.startsWith(`${flag}=`)) { + const value = arg === flag ? args[index + 1] : arg.slice(flag.length + 1); - if (value === undefined || value === '') { - return { kind: 'error', message: "option '--env ' argument missing" }; - } + if (value === undefined || value === '') { + return { kind: 'error', message: `option '${flag} ${usage}' argument missing` }; + } - env = value; - index += arg === ENV_FLAG ? 2 : 1; + values[key as keyof typeof VALUE_FLAGS] = value; + index += arg === flag ? 2 : 1; - continue; + continue options; + } } break; @@ -89,17 +109,17 @@ export function parseAdminArgs(args: readonly string[]): AdminInvocation { }; } - return { kind: 'run', env, args: rest }; + return run(rest); } /** * The `KIZEN_AUTH_HELPER` command: this CLI's `auth-header --env --business --user `, - * bound to the account kizen was given its ids for, so after a switch to another account it fails - * rather than hand kizen that account's token. kizen caches the header for the life of its process - * and runs the helper again only after a 401 (`kizen_builder/auth_helper.py`), so a switch makes - * kizen's next token fetch fail (at startup, or after a 401), while a token it already holds, - * which is still its own account's, stays in use until it expires. Run by the same node and - * script, with each word POSIX-quoted so kizen's `shlex.split` gets them back exactly. + * bound to the account kizen was given its ids for. It serves that account from its own file + * whether or not it's current, so a switch to another account meanwhile neither stops kizen nor + * hands it the other account's token; a logout of that account makes the next token fetch fail. + * kizen caches the header for the life of its process and runs the helper again only after a 401 + * (`kizen_builder/auth_helper.py`). Run by the same node and script, with each word POSIX-quoted + * so kizen's `shlex.split` gets them back exactly. */ export const kizenAuthHelper = ( execPath: string, @@ -244,7 +264,7 @@ export function runKizen( } export interface AdminDeps { - store: Pick; + store: Pick; /** * The provisioner `admin init` and every `admin` run share (`ensureKizen`). `force` (only from * `admin init --force`) removes the install first. @@ -284,19 +304,36 @@ export async function runAdminInit(deps: AdminDeps, force = false): Promise { - const resolved = await resolveSessionEnvironment(env, () => deps.store.listSessions()); + const request = requireAccountRequest(invocation, deps.processEnv); + const resolved = await resolveAccountEnvironment(invocation.env, request, deps.store); if (!resolved.ok) { return { ok: false, message: resolved.message }; } const { environment } = resolved; - const session = await deps.store.readSession(environment); + let session: OAuthSession | null; + + if (request === undefined) { + session = await deps.store.readSession(environment); + } else { + const account = requireNamedAccount( + environment, + await deps.store.listAccounts(environment), + request, + ); + + session = await deps.store.readAccount(environment, account); + } if (session === null) { return { @@ -320,7 +357,10 @@ async function sessionEnv( }; } -/** `appbuilder admin [--env ] [--] ` and `appbuilder admin init`. */ +/** + * `appbuilder admin [--env ] [--business [--user ]] [--] ` + * and `appbuilder admin init`. + */ export async function runAdmin(args: readonly string[], deps: AdminDeps): Promise { const invocation = parseAdminArgs(args); @@ -349,7 +389,7 @@ export async function runAdmin(args: readonly string[], deps: AdminDeps): Promis let resolved: Awaited>; try { - resolved = await sessionEnv(invocation.env, deps); + resolved = await sessionEnv(invocation, deps); } catch (error) { fail(deps, errorMessage(error)); diff --git a/src/lib/oauthCommands.test.ts b/src/lib/oauthCommands.test.ts index 8378014..2b64b0d 100644 --- a/src/lib/oauthCommands.test.ts +++ b/src/lib/oauthCommands.test.ts @@ -279,6 +279,322 @@ describe('whoami', () => { expect(h.exitCodes).toStrictEqual([1, 1]); expect(h.fetchMock).not.toHaveBeenCalled(); }); + + describe('naming a saved account (--business / --user, APPBUILDER_BUSINESS / APPBUILDER_USER)', () => { + const B_ACCESS = 'secret-b-access'; + const B_SESSION = session('staging', { + ...ACCOUNT_B, + businessName: 'Bravo', + email: 'bea@b.test', + accessToken: B_ACCESS, + }); + const IDENTITY_A: KizenIdentity = { + ...ACCOUNT_A, + businessName: 'Acme A', + fullName: 'Ada A', + email: 'ada@a.test', + }; + const IDENTITY_B: KizenIdentity = { + ...ACCOUNT_B, + businessName: 'Bravo', + fullName: 'Bea B', + email: 'bea@b.test', + }; + /** Bootstrap answers for whichever account's token is presented. */ + const kizen = (_url: string, auth: string | undefined): Response => + auth === `Bearer ${ACCESS}` + ? jsonResponse(bootstrapFor(IDENTITY_A)) + : auth === `Bearer ${B_ACCESS}` + ? jsonResponse(bootstrapFor(IDENTITY_B)) + : jsonResponse({ detail: 'unexpected' }, 401); + + /** B saved, A current. */ + const savedBCurrentA = async (): Promise => { + await store.writeSession(B_SESSION); + await signIn(store, session('staging')); + }; + + const pointer = async (): Promise => store.readSession('staging'); + + it.each<[string, Record, Record, OAuthAccount]>([ + [ + '--business by name names a saved account that is not current', + {}, + { business: 'bravo' }, + ACCOUNT_B, + ], + [ + 'APPBUILDER_BUSINESS beats the current account', + { APPBUILDER_BUSINESS: BIZ_B }, + {}, + ACCOUNT_B, + ], + [ + '--business beats APPBUILDER_BUSINESS', + { APPBUILDER_BUSINESS: BIZ_B, APPBUILDER_USER: USER_B }, + { business: BIZ_A }, + ACCOUNT_A, + ], + [ + 'APPBUILDER_BUSINESS with APPBUILDER_USER', + { APPBUILDER_BUSINESS: 'Bravo', APPBUILDER_USER: USER_B }, + {}, + ACCOUNT_B, + ], + ])('%s', async (_name, processEnv, flags, expected) => { + await savedBCurrentA(); + + const h = harness(kizen, { processEnv }); + + await runWhoami({ env: 'staging', ...flags }, h.deps); + + expect(h.exitCodes).toStrictEqual([]); + expect(h.stdout.join('')).toContain( + `Business: ${expected === ACCOUNT_B ? 'Bravo' : 'Acme A'} (${expected.businessId})`, + ); + expect(h.providerOptions).toStrictEqual([{ account: expected }]); + // Naming an account never moves the pointer. + expect(await pointer()).toMatchObject(ACCOUNT_A); + }); + + it.each([ + ['without a name, the current account', {}], + ['an empty APPBUILDER_BUSINESS counts as unset', { APPBUILDER_BUSINESS: '' }], + ['an empty APPBUILDER_USER counts as unset', { APPBUILDER_USER: '' }], + ])('%s', async (_name, processEnv) => { + await savedBCurrentA(); + + const h = harness(kizen, { processEnv }); + + await runWhoami({ env: 'staging' }, h.deps); + + expect(h.exitCodes).toStrictEqual([]); + expect(h.stdout.join('')).toContain(`Business: Acme A (${BIZ_A})`); + expect(h.providerOptions).toStrictEqual([{}]); + }); + + it.each<[string, Record, Record, string]>([ + ['--user without --business', {}, { user: USER_B }, '--user only applies with --business.'], + [ + 'APPBUILDER_USER without APPBUILDER_BUSINESS', + { APPBUILDER_USER: USER_B }, + {}, + 'APPBUILDER_USER only applies with APPBUILDER_BUSINESS.', + ], + [ + 'APPBUILDER_USER with an empty APPBUILDER_BUSINESS', + { APPBUILDER_BUSINESS: '', APPBUILDER_USER: USER_B }, + {}, + 'APPBUILDER_USER only applies with APPBUILDER_BUSINESS.', + ], + ])('%s is a usage error', async (_name, processEnv, flags, message) => { + await savedBCurrentA(); + + const h = harness(kizen, { processEnv }); + + await runWhoami({ env: 'staging', ...flags }, h.deps); + + expect(h.stderr).toStrictEqual([`Error: ${message}\n`]); + expect(h.exitCodes).toStrictEqual([1]); + expect(h.stdout).toStrictEqual([]); + expect(h.fetchMock).not.toHaveBeenCalled(); + }); + + it('a name that matches no saved account lists them and never uses the current one', async () => { + await savedBCurrentA(); + + for (const [processEnv, flags, subject] of [ + [{}, { business: 'Nope' }, '--business Nope'], + [{ APPBUILDER_BUSINESS: 'Nope' }, {}, 'APPBUILDER_BUSINESS=Nope'], + ] as const) { + const h = harness(kizen, { processEnv }); + + await runWhoami({ env: 'staging', ...flags }, h.deps); + + expect(h.stderr).toStrictEqual([ + `Error: No saved staging account matches ${subject}. Saved accounts:\n Acme A (${BIZ_A}) ada@a.test\n Bravo (${BIZ_B}) bea@b.test\n`, + ]); + expect(h.exitCodes).toStrictEqual([1]); + expect(h.stdout).toStrictEqual([]); + expect(h.providerOptions).toStrictEqual([]); + expect(h.fetchMock).not.toHaveBeenCalled(); + } + }); + + it('a name that matches several saved users of one business asks for the user', async () => { + await store.writeSession(B_SESSION); + await store.writeSession(session('staging', { userId: USER_A2, email: 'zed@a.test' })); + await signIn(store, session('staging')); + + const byFlag = harness(kizen); + + await runWhoami({ env: 'staging', business: 'acme a' }, byFlag.deps); + + expect(byFlag.stderr).toStrictEqual([ + `Error: --business 'acme a' matches several saved staging users of the same business. Pass --user too, for example \`--business ${BIZ_A} --user ${USER_A}\`:\n --user ${USER_A}: Acme A (${BIZ_A}) ada@a.test\n --user ${USER_A2}: Acme A (${BIZ_A}) zed@a.test\n`, + ]); + + const byEnv = harness(kizen, { processEnv: { APPBUILDER_BUSINESS: BIZ_A } }); + + await runWhoami({ env: 'staging' }, byEnv.deps); + + expect(byEnv.stderr).toStrictEqual([ + `Error: APPBUILDER_BUSINESS=${BIZ_A} matches several saved staging users of the same business. Set APPBUILDER_USER too, for example \`APPBUILDER_BUSINESS=${BIZ_A} APPBUILDER_USER=${USER_A}\`:\n APPBUILDER_USER=${USER_A}: Acme A (${BIZ_A}) ada@a.test\n APPBUILDER_USER=${USER_A2}: Acme A (${BIZ_A}) zed@a.test\n`, + ]); + + for (const h of [byFlag, byEnv]) { + expect(h.exitCodes).toStrictEqual([1]); + expect(h.providerOptions).toStrictEqual([]); + expect(h.fetchMock).not.toHaveBeenCalled(); + } + }); + + it('a name that matches several businesses lists them', async () => { + await store.writeSession(session('staging', { ...ACCOUNT_B, businessName: 'Acme A' })); + await signIn(store, session('staging')); + + const h = harness(kizen); + + await runWhoami({ env: 'staging', business: 'Acme A' }, h.deps); + + expect(h.stderr.join('')).toContain( + "--business 'Acme A' matches several saved staging accounts. Pass one of these business ids instead:", + ); + expect(h.exitCodes).toStrictEqual([1]); + expect(h.fetchMock).not.toHaveBeenCalled(); + + const byEnv = harness(kizen, { processEnv: { APPBUILDER_BUSINESS: 'Acme A' } }); + + await runWhoami({ env: 'staging' }, byEnv.deps); + + expect(byEnv.stderr).toStrictEqual([ + `Error: APPBUILDER_BUSINESS='Acme A' matches several saved staging accounts. Set APPBUILDER_BUSINESS to one of these business ids instead:\n Acme A (${BIZ_A}) ada@a.test\n Acme A (${BIZ_B}) ada@a.test\n`, + ]); + expect(byEnv.fetchMock).not.toHaveBeenCalled(); + }); + + describe('without --env, the environment is the only one where the name is saved, current or not', () => { + /** B saved in `environment`. */ + const saveB = (environment: Environment): Promise => + store.writeSession({ ...B_SESSION, environment }); + + const run = async ( + flags: Record, + processEnv: Record = {}, + ): Promise => { + const h = harness(kizen, { processEnv }); + + await runWhoami(flags, h.deps); + + return h; + }; + + const expectServedB = (h: Harness, environment: Environment): void => { + expect(h.stderr).toStrictEqual([]); + expect(h.stdout.join('')).toContain(`Environment: ${environment}\n`); + expect(h.stdout.join('')).toContain(`Business: Bravo (${BIZ_B})`); + expect(h.providerOptions).toStrictEqual([{ account: ACCOUNT_B }]); + }; + + const expectRefused = (h: Harness, line: string): void => { + expect(h.stderr).toStrictEqual([`Error: ${line}\n`]); + expect(h.exitCodes).toStrictEqual([1]); + expect(h.providerOptions).toStrictEqual([]); + expect(h.fetchMock).not.toHaveBeenCalled(); + }; + + it('nothing current, saved only in staging: uses staging', async () => { + await savedBCurrentA(); + expect(await store.signOut('staging')).toBe(true); + expect(await store.listSessions()).toStrictEqual([]); + + expectServedB(await run({ business: 'Bravo' }), 'staging'); + expectServedB(await run({}, { APPBUILDER_BUSINESS: 'Bravo' }), 'staging'); + expect(await store.listSessions()).toStrictEqual([]); + }); + + it('current in staging, saved only in go: uses go', async () => { + await signIn(store, session('staging')); + await saveB('go'); + + expectServedB(await run({ business: 'Bravo' }), 'go'); + }); + + it('current in two environments, saved in one of them: uses that one', async () => { + await signIn(store, session('staging')); + await signIn(store, session('integration')); + await saveB('integration'); + + expectServedB(await run({ business: 'Bravo' }), 'integration'); + }); + + it('saved in staging and go: asks for --env, naming them', async () => { + await signIn(store, session('staging')); + await saveB('staging'); + await saveB('go'); + + const h = await run({ business: 'Bravo' }); + + expectRefused( + h, + '--business Bravo matches saved accounts in several environments; pass --env : go, staging', + ); + expect(h.stderr.join('')).not.toContain('signed-in'); + expectRefused( + await run({}, { APPBUILDER_BUSINESS: 'Bravo' }), + 'APPBUILDER_BUSINESS=Bravo matches saved accounts in several environments; pass --env : go, staging', + ); + }); + + it('a name that matches nowhere resolves the environment as without a name, then lists its accounts', async () => { + await savedBCurrentA(); + + expectRefused( + await run({ business: 'Nope' }), + `No saved staging account matches --business Nope. Saved accounts:\n Acme A (${BIZ_A}) ada@a.test\n Bravo (${BIZ_B}) bea@b.test`, + ); + }); + + it('a name that matches nowhere, with saved accounts in staging but none current, lists them', async () => { + await savedBCurrentA(); + expect(await store.signOut('staging')).toBe(true); + expect(await store.listSessions()).toStrictEqual([]); + + expectRefused( + await run({ business: 'Nope' }), + `No saved staging account matches --business Nope. Saved accounts:\n Bravo (${BIZ_B}) bea@b.test`, + ); + }); + + it('a name that matches nowhere, with saved accounts in several environments, asks for --env', async () => { + await saveB('staging'); + await saveB('go'); + + expectRefused( + await run({ business: 'Nope' }), + "You're signed in to several environments; pass --env : go, staging", + ); + }); + + it('a name that matches nowhere with nothing signed in is not signed in', async () => { + const h = await run({ business: 'Nope' }); + + expect(h.stderr.join('')).toContain("You're not signed in."); + expect(h.exitCodes).toStrictEqual([1]); + expect(h.fetchMock).not.toHaveBeenCalled(); + }); + + it("a corrupt pointer or account file in another environment doesn't stop it", async () => { + await signIn(store, session('staging')); + await saveB('staging'); + await signIn(store, session('integration')); + await writeFile(store.sessionPath('integration'), '{"businessId": "trunc'); + await writeFile(store.accountPath('integration', ACCOUNT_A), '{"accessToken": "tr'); + + expectServedB(await run({ business: 'Bravo' }), 'staging'); + }); + }); + }); }); describe('logout', () => { @@ -1677,7 +1993,7 @@ describe('auth-header', () => { ]); }); - it('fails closed, naming the mismatch, after a switch to another account', async () => { + it("serves that account's token while another account is current", async () => { await signIn(store, session('staging')); // Another terminal switches staging to account B while kizen still holds A's ids. await signIn(store, session('staging', { ...ACCOUNT_B, accessToken: B_ACCESS })); @@ -1686,41 +2002,18 @@ describe('auth-header', () => { await runAuthHeader(bound, h.deps); - expectHeaderFailure(h); - expect(h.stderr).toStrictEqual([ - `appbuilder auth-header: --business ${BIZ_A} --user ${USER_A} isn't the current staging account (the current account is business ${BIZ_B}, user ${USER_B}), so auth-header won't send its token. Run the command again.\n`, - ]); + expect(h.stdout).toStrictEqual([header(ACCESS)]); + expect(h.stderr).toStrictEqual([]); + expect(h.exitCodes).toStrictEqual([]); expect(h.all()).not.toContain(B_ACCESS); - // It fails before reading (or refreshing) any token. - expect(h.providerOptions).toStrictEqual([]); - expect(h.fetchMock).not.toHaveBeenCalled(); - }); - - it('fails closed after a switch that lands while the token is being read', async () => { - await signIn(store, session('staging')); - - const h = harness(noKizen); - const provider = h.deps.tokenProvider; - - h.deps.tokenProvider = (environment, providerOpts) => { - const inner = provider(environment, providerOpts); - - return { - ...inner, - getAccessToken: async () => { - const token = await inner.getAccessToken(); - - await signIn(store, session('staging', { ...ACCOUNT_B, accessToken: B_ACCESS })); - - return token; - }, - }; - }; - - await runAuthHeader(bound, h.deps); - - expectHeaderFailure(h, "isn't the current staging account (the current account is business"); - expect(h.providerOptions).toHaveLength(1); + expect(h.providerOptions).toStrictEqual([ + { requestTimeoutMs: AUTH_HEADER_REQUEST_TIMEOUT_MS, account: ACCOUNT_A }, + ]); + // B stays current. + expect(await store.readSession('staging')).toMatchObject({ + ...ACCOUNT_B, + accessToken: B_ACCESS, + }); }); it('fails closed when nobody is signed in to the environment any more', async () => { @@ -1734,18 +2027,17 @@ describe('auth-header', () => { expectHeaderFailure(h, 'appbuilder auth-header: '); }); - it('fails closed when the account is saved but not current', async () => { + it("fails closed once that account is signed out, even while another account's token is there", async () => { await signIn(store, session('staging')); await signIn(store, session('staging', { ...ACCOUNT_B, accessToken: B_ACCESS })); - - // A's file is still saved, so its token could be read: the pointer check is what refuses. - expect(await store.readAccount('staging', ACCOUNT_A)).not.toBeNull(); + await store.removeAccount('staging', ACCOUNT_A); const h = harness(noKizen); await runAuthHeader(bound, h.deps); - expectHeaderFailure(h, "isn't the current staging account"); + expectHeaderFailure(h, "appbuilder auth-header: You're not signed in to staging."); + expect(h.all()).not.toContain(B_ACCESS); }); it.each([[{ business: BIZ_A }], [{ user: USER_A }]])('refuses %j alone', async (partial) => { @@ -1762,11 +2054,13 @@ describe('auth-header', () => { expect(h.providerOptions).toStrictEqual([]); }); - it('without the flags, serves the current account as before, after a switch too', async () => { + it('without the flags, serves the current account as before, after a switch too, whatever APPBUILDER_BUSINESS says', async () => { await signIn(store, session('staging')); await signIn(store, session('staging', { ...ACCOUNT_B, accessToken: B_ACCESS })); - const h = harness(noKizen); + const h = harness(noKizen, { + processEnv: { APPBUILDER_BUSINESS: BIZ_A, APPBUILDER_USER: USER_A }, + }); await runAuthHeader({ env: 'staging' }, h.deps); diff --git a/src/lib/oauthCommands.ts b/src/lib/oauthCommands.ts index 9340188..6c92063 100644 --- a/src/lib/oauthCommands.ts +++ b/src/lib/oauthCommands.ts @@ -7,6 +7,13 @@ * environment has saved accounts. Otherwise it runs the browser sign-in. */ import { apiOriginFor } from '../../shared/lib/kizenUrls.js'; +import { + accountRequest, + BUSINESS_ENV, + USER_ENV, + type AccountFlags, + type AccountRequest, +} from './authSelection.js'; import { ENVIRONMENTS, parseEnvironment, type Environment } from './credentials.js'; import { errorMessage } from './guards.js'; import { createKizenClient, KizenApiError, type KizenIdentity } from './kizenClient.js'; @@ -20,6 +27,7 @@ import { type BearerTokenProvider, type Identify, type LoginDeps, + type SessionEnvironment, type TokenProviderOptions, } from './oauthSession.js'; import { @@ -123,12 +131,13 @@ function requireEnvironment(env: string): Environment { return environment; } -/** --env if given, else the only environment with a stored session; any problem is an error. */ +/** `resolveAccountEnvironment`, with any problem an error. */ async function sessionEnvironment( env: string | undefined, store: OAuthStore, + request?: AccountRequest, ): Promise { - const resolved = await resolveSessionEnvironment(env, () => store.listSessions()); + const resolved = await resolveAccountEnvironment(env, request, store); if (!resolved.ok) { throw new Error(resolved.message); @@ -241,11 +250,8 @@ type BusinessMatch = | { kind: 'one'; account: SavedAccount } | { kind: 'none' | 'several'; candidates: SavedAccount[] }; -/** What `--business` (and `--user`) ask for. */ -interface BusinessRequest { - business: string; - user?: string | undefined; -} +/** What `--business` (and `--user`), or `APPBUILDER_BUSINESS` (and `APPBUILDER_USER`), ask for. */ +type BusinessRequest = AccountRequest; /** * Whether a browser sign-in's `identity` is the account `--business` (and `--user`) asked for, by @@ -294,9 +300,16 @@ export function matchBusiness( const candidateLines = (candidates: readonly SavedAccount[]): string => candidates.map((candidate) => `\n ${accountSummary(candidate)}`).join(''); -/** `--business `, plus ` --user ` when given, shell-quoted, as the flags to repeat. */ -const requestFlags = ({ business, user }: BusinessRequest): string => - `--business ${quoteArg(business)}${user === undefined ? '' : ` --user ${quoteArg(user)}`}`; +/** How a request names its user id: `--user `, or `APPBUILDER_USER=`. */ +const userArg = (source: BusinessRequest['source'], user: string): string => + source === 'env' ? `${USER_ENV}=${quoteArg(user)}` : `--user ${quoteArg(user)}`; + +/** + * `--business `, plus ` --user ` when given, shell-quoted, as the flags to repeat; or the + * `APPBUILDER_BUSINESS=` (and `APPBUILDER_USER=`) assignments the request came from. + */ +const requestFlags = ({ business, user, source }: BusinessRequest): string => + `${source === 'env' ? `${BUSINESS_ENV}=${quoteArg(business)}` : `--business ${quoteArg(business)}`}${user === undefined ? '' : ` ${userArg(source, user)}`}`; /** * The one saved account `--business` names; several or none is a usage error listing them. Pass @@ -325,23 +338,122 @@ function requireBusiness( first !== undefined && match.candidates.every(({ account }) => account.businessId === first.account.businessId); - // Several users of one business: only --user can tell them apart. + // Several users of one business: only --user (or APPBUILDER_USER) can tell them apart. if (sameBusiness) { + const { source } = request; + throw new Error( - `${flags} matches several saved ${environment} users of the same business. Pass --user too, for example \`${requestFlags({ business: first.account.businessId, user: first.account.userId })}\`:${match.candidates + `${flags} matches several saved ${environment} users of the same business. ${source === 'env' ? `Set ${USER_ENV}` : 'Pass --user'} too, for example \`${requestFlags({ business: first.account.businessId, user: first.account.userId, source })}\`:${match.candidates .map( (candidate) => - `\n --user ${quoteArg(candidate.account.userId)}: ${accountSummary(candidate)}`, + `\n ${userArg(source, candidate.account.userId)}: ${accountSummary(candidate)}`, ) .join('')}`, ); } throw new Error( - `${flags} matches several saved ${environment} accounts. Pass one of these business ids instead:${candidateLines(match.candidates)}`, + `${flags} matches several saved ${environment} accounts. ${request.source === 'env' ? `Set ${BUSINESS_ENV} to` : 'Pass'} one of these business ids instead:${candidateLines(match.candidates)}`, + ); +} + +export type AccountEnvironment = + | { ok: true; environment: Environment } + | { + ok: false; + problem: Extract['problem']; + message: string; + /** The environments to choose among, for `several_sessions`. */ + environments: Environment[]; + }; + +/** + * The environment a command uses: --env if given; else, without a named account, the only + * signed-in one. With one, the only environment where the name matches a saved account, current + * or not; several are a choice. A name that matches nowhere resolves to the only environment with + * saved accounts, current or not, so the no-match error lists them. + */ +export async function resolveAccountEnvironment( + env: string | undefined, + request: AccountRequest | undefined, + store: Pick, +): Promise { + const resolveSignedIn = async ( + list: () => Promise, + ): Promise => { + let environments: Environment[] = []; + const resolved = await resolveSessionEnvironment(env, async () => { + environments = await list(); + + return environments; + }); + + return resolved.ok ? resolved : { ...resolved, environments }; + }; + + if (request === undefined || env !== undefined) { + return resolveSignedIn(() => store.listSessions()); + } + + const saved = await Promise.all( + ENVIRONMENTS.map(async (environment) => ({ + environment, + accounts: await store.listAccounts(environment), + })), + ); + const matching = saved + .filter( + ({ accounts }) => matchBusiness(accounts, request.business, request.user).kind !== 'none', + ) + .map(({ environment }) => environment); + const [only] = matching; + + if (matching.length === 1 && only !== undefined) { + return { ok: true, environment: only }; + } + + if (matching.length > 1) { + return { + ok: false, + problem: 'several_sessions', + message: `${requestFlags(request)} matches saved accounts in several environments; pass --env : ${matching.join(', ')}`, + environments: matching, + }; + } + + // Matches nowhere: the only environment with saved accounts, current or not, so the no-match + // error lists them. + return resolveSignedIn(() => + Promise.resolve( + saved.filter(({ accounts }) => accounts.length > 0).map(({ environment }) => environment), + ), ); } +/** + * The saved account `request` names among `accounts`, current or not; none or several is a usage + * error that lists them. Never falls back to the current account. + */ +export const requireNamedAccount = ( + environment: Environment, + accounts: readonly SavedAccount[], + request: AccountRequest, +): OAuthAccount => requireBusiness(environment, accounts, request).account; + +/** `accountRequest` for a command that throws its usage errors. */ +export function requireAccountRequest( + flags: AccountFlags, + processEnv: Readonly>, +): AccountRequest | undefined { + const resolved = accountRequest(flags, processEnv); + + if (!resolved.ok) { + throw new Error(resolved.message); + } + + return resolved.request; +} + function listAccounts( environment: Environment, accounts: readonly SavedAccount[], @@ -670,11 +782,23 @@ export const runLogout = guarded(async (options, deps) => ); }); -export const runWhoami = guarded(async (options, deps) => { - const environment = await sessionEnvironment(options.env, deps.store); +export interface WhoamiCommandOptions extends AuthCommandOptions, AccountFlags {} + +export const runWhoami = guarded(async (options, deps) => { + const request = requireAccountRequest(options, deps.processEnv); + const environment = await sessionEnvironment(options.env, deps.store, request); + const account = + request === undefined + ? undefined + : requireNamedAccount(environment, await deps.store.listAccounts(environment), request); deps.writeStdout( - identityLines(environment, await deps.identify(deps.tokenProvider(environment))), + identityLines( + environment, + await deps.identify( + deps.tokenProvider(environment, account === undefined ? undefined : { account }), + ), + ), ); }); @@ -747,49 +871,12 @@ function boundAccount(options: AuthHeaderCommandOptions): OAuthAccount | undefin return { businessId: business, userId: user }; } -/** - * The bound account's token. It comes from that account's own file, so it's never another - * account's; and the environment's current account must still be that one, so a switch to another - * account fails the helper (as it aborts any running command) instead of serving on. - */ -async function boundAccountToken( - environment: Environment, - account: OAuthAccount, - deps: AuthCommandDeps, -): Promise { - /** Throws unless `account` is the environment's current account. */ - const requireCurrent = async (): Promise => { - const current = await deps.store.readSession(environment); - - if (current === null || !sameAccount(current, account)) { - const now = - current === null - ? 'nobody is signed in' - : `the current account is business ${current.businessId}, user ${current.userId}`; - - throw new Error( - `--business ${account.businessId} --user ${account.userId} isn't the current ${environment} account (${now}), so auth-header won't send its token. Run the command again.`, - ); - } - }; - - // Before the token read, so an account that's no longer current fails without a refresh; and - // after it, so a switch during that read fails too. - await requireCurrent(); - - const { requestTimeoutMs } = deps.authHeaderTimeouts; - const token = await deps - .tokenProvider(environment, { requestTimeoutMs, account }) - .getAccessToken(); - - await requireCurrent(); - - return token; -} - /** * Claude Code `headersHelper`, and kizen's `KIZEN_AUTH_HELPER` (bound with `--business` and - * `--user`). Failures print one stderr line and nothing to stdout. + * `--user`). Bound, it serves that saved account from its own file, current or not, so it's never + * another account's token; unbound, the current account. It never reads `APPBUILDER_BUSINESS`, so + * a headersHelper always follows the current account. Failures print one stderr line and nothing + * to stdout. */ export const runAuthHeader: AuthCommand = async (options, deps) => { let line: string; @@ -798,10 +885,12 @@ export const runAuthHeader: AuthCommand = async (optio const account = boundAccount(options); const environment = await headerEnvironment(options.env, deps); const { requestTimeoutMs } = deps.authHeaderTimeouts; - const token = - account === undefined - ? await deps.tokenProvider(environment, { requestTimeoutMs }).getAccessToken() - : await boundAccountToken(environment, account, deps); + const token = await deps + .tokenProvider(environment, { + requestTimeoutMs, + ...(account !== undefined && { account }), + }) + .getAccessToken(); line = `${JSON.stringify({ Authorization: `Bearer ${token}` })}\n`; } catch (error) { diff --git a/src/lib/oauthSession.test.ts b/src/lib/oauthSession.test.ts index 347e5b5..eb3d8e0 100644 --- a/src/lib/oauthSession.test.ts +++ b/src/lib/oauthSession.test.ts @@ -213,7 +213,6 @@ const provider = ( ...overrides, }); -const SIGN_IN_CHANGED = 'The staging sign-in changed while this command was running. Run it again.'; const NOT_SIGNED_IN = "You're not signed in to staging. Run `appbuilder login --env staging` to sign in."; @@ -632,17 +631,29 @@ describe('createTokenProvider', () => { expect(await readdir(store.dir)).toStrictEqual([]); }); - it.each([ - [ - "a refresh that completes after a switch to another account doesn't overwrite it", - undefined, - ], - [ - 'a refresh that gets invalid_grant after a switch to another account does not delete it', - INVALID_GRANT, - ], - ])('%s', async (_name, respond) => { - const { pending, release, server } = await heldRefresh(respond); + it('a refresh that completes after a switch to another account saves only the pinned account', async () => { + const { pending, release, server } = await heldRefresh(); + + await signIn(createOAuthStore(dir), LOGIN_B); + release(); + + // The command keeps acting as A, and A's own file holds the new pair. + await expect(pending).resolves.toBe('access-1'); + expect(await store.readAccount('staging', ACCOUNT_A)).toMatchObject({ + ...ACCOUNT_A, + accessToken: 'access-1', + refreshToken: 'refresh-1', + }); + // B's file and the pointer are exactly as the switch left them. + expect(await store.readAccount('staging', ACCOUNT_B)).toStrictEqual(LOGIN_B); + expect(JSON.parse(await readFile(store.sessionPath('staging'), 'utf-8'))).toStrictEqual( + ACCOUNT_B, + ); + expect(server.tokenRequests()).toHaveLength(1); + }); + + it('a refresh that gets invalid_grant after a switch to another account deletes only the pinned account', async () => { + const { pending, release, server } = await heldRefresh(INVALID_GRANT); await signIn(createOAuthStore(dir), LOGIN_B); release(); @@ -650,8 +661,12 @@ describe('createTokenProvider', () => { const error = await rejectionOf(pending); expect(error.code).toBe('login_required'); - expect(error.message).toBe(SIGN_IN_CHANGED); + expect(error.message).toContain('is no longer valid'); + expect(await store.readAccount('staging', ACCOUNT_A)).toBeNull(); expect(await store.readSession('staging')).toStrictEqual(LOGIN_B); + expect(JSON.parse(await readFile(store.sessionPath('staging'), 'utf-8'))).toStrictEqual( + ACCOUNT_B, + ); expect(server.tokenRequests()).toHaveLength(1); }); @@ -941,40 +956,141 @@ describe('createTokenProvider', () => { ); }); - it('switching to another account aborts a running command with sign-in changed', async () => { + it('a switch to another account after the first read keeps serving the pinned account', async () => { await signIn(store, oauthSession()); const server = fakeAuthServer(); const tokens = provider(server); await expect(tokens.getAccessToken()).resolves.toBe('access-0'); + await signIn(store, LOGIN_B); - const newLogin = oauthSession({ ...ACCOUNT_B, accessToken: 'access-b' }); + // Still A's token, from A's own file, never B's. + await expect(tokens.getAccessToken()).resolves.toBe('access-0'); + // A 401 refreshes A, writing only A's file. + await expect(tokens.refreshAccessToken('access-0')).resolves.toBe('access-1'); + await expect(tokens.getAccessToken()).resolves.toBe('access-1'); + expect(server.tokenRequests().map((params) => params.get('refresh_token'))).toStrictEqual([ + 'refresh-0', + ]); + expect(await store.readAccount('staging', ACCOUNT_A)).toMatchObject({ + accessToken: 'access-1', + refreshToken: 'refresh-1', + }); + expect(await store.readSession('staging')).toStrictEqual(LOGIN_B); + // A provider created after the switch serves B. + await expect(provider(server).getAccessToken()).resolves.toBe('access-b'); + }); + + it('another user in the same business is another account to the pin', async () => { + await signIn(store, oauthSession()); + + const tokens = provider(fakeAuthServer()); - await signIn(store, newLogin); + await expect(tokens.getAccessToken()).resolves.toBe('access-0'); + await signIn(store, oauthSession({ userId: ACCOUNT_B.userId, accessToken: 'access-other' })); + + await expect(tokens.getAccessToken()).resolves.toBe('access-0'); + }); + + it('reads that start together before the first one settles all pin the same account', async () => { + await signIn(store, oauthSession()); + // The first pointer read finds A and is then held; any other pointer read waits and reads + // after the switch below, so it would find B. + const gate = deferred(); + let pointerReads = 0; + const counting: OAuthStore = { + ...store, + readSession: async (environment) => { + pointerReads += 1; + + if (pointerReads === 1) { + const session = await store.readSession(environment); + + await gate.promise; + + return session; + } + + await gate.promise; + + return store.readSession(environment); + }, + }; + const tokens = provider(fakeAuthServer(), { store: counting }); + const first = tokens.getAccessToken(); + const second = tokens.getAccessToken(); + + await vi.waitFor(() => { + expect(pointerReads).toBe(1); + }); + await signIn(store, LOGIN_B); + gate.resolve(); + + await expect(Promise.all([first, second])).resolves.toStrictEqual(['access-0', 'access-0']); + await expect(tokens.getAccessToken()).resolves.toBe('access-0'); + expect(pointerReads).toBe(1); + }); + + it('a first pointer read that throws pins nothing: a later read goes through the pointer', async () => { + await signIn(store, oauthSession()); + + let pointerReads = 0; + const flaky: OAuthStore = { + ...store, + readSession: (environment) => { + pointerReads += 1; + + return pointerReads === 1 + ? Promise.reject(new Error('EIO: transient read failure')) + : store.readSession(environment); + }, + }; + const tokens = provider(fakeAuthServer(), { store: flaky }); + + await expect(tokens.getAccessToken()).rejects.toThrow('EIO: transient read failure'); + // Meanwhile B becomes current: the next read pins B, not A. + await signIn(store, LOGIN_B); + await expect(tokens.getAccessToken()).resolves.toBe('access-b'); + expect(pointerReads).toBe(2); + // Pinned now: a switch back doesn't move it. + await signIn(store, oauthSession()); + await expect(tokens.getAccessToken()).resolves.toBe('access-b'); + expect(pointerReads).toBe(2); + }); + + it('a first pointer read that finds nobody signed in pins nothing: a later sign-in is served', async () => { + const tokens = provider(fakeAuthServer()); const error = await rejectionOf(tokens.getAccessToken()); expect(error.code).toBe('login_required'); - expect(error.message).toBe(SIGN_IN_CHANGED); - expect(error.message).not.toContain('access-b'); - // A 401 retry is refused the same way, without touching the new login. - expect((await rejectionOf(tokens.refreshAccessToken('access-0'))).code).toBe('login_required'); - expect(server.fetch).not.toHaveBeenCalled(); - expect(await store.readSession('staging')).toStrictEqual(newLogin); - // A provider created after the new login serves it. - await expect(provider(server).getAccessToken()).resolves.toBe('access-b'); + expect(error.message).toBe(NOT_SIGNED_IN); + + await signIn(store, LOGIN_B); + await expect(tokens.getAccessToken()).resolves.toBe('access-b'); + await signIn(store, oauthSession()); + await expect(tokens.getAccessToken()).resolves.toBe('access-b'); }); - it('another user in the same business is another account to the pin', async () => { + it('a logout of the pinned account stops the command even after a switch to another account', async () => { await signIn(store, oauthSession()); const tokens = provider(fakeAuthServer()); await expect(tokens.getAccessToken()).resolves.toBe('access-0'); - await signIn(store, oauthSession({ userId: ACCOUNT_B.userId, accessToken: 'access-other' })); + await signIn(store, LOGIN_B); + expect(await store.removeAccount('staging', ACCOUNT_A)).toStrictEqual({ + removed: true, + current: false, + }); - expect((await rejectionOf(tokens.getAccessToken())).message).toBe(SIGN_IN_CHANGED); + const error = await rejectionOf(tokens.getAccessToken()); + + expect(error.code).toBe('login_required'); + expect(error.message).toBe(NOT_SIGNED_IN); + expect((await rejectionOf(tokens.refreshAccessToken('access-0'))).message).toBe(NOT_SIGNED_IN); + expect(await store.readSession('staging')).toStrictEqual(LOGIN_B); }); it("re-login to the same account doesn't abort a running command", async () => { diff --git a/src/lib/oauthSession.ts b/src/lib/oauthSession.ts index 6cecdfb..dc70d3a 100644 --- a/src/lib/oauthSession.ts +++ b/src/lib/oauthSession.ts @@ -22,18 +22,10 @@ import { type TokenResponse, } from './oauth.js'; import type { KizenIdentity } from './kizenClient.js'; -import { - sameAccount, - type OAuthAccount, - type OAuthSession, - type OAuthStore, -} from './oauthStore.js'; +import type { OAuthAccount, OAuthSession, OAuthStore } from './oauthStore.js'; const REFRESH_LEEWAY_MS = 60_000; -/** Thrown when the pointer names another account than a provider is pinned to. */ -class SignInChangedError extends OAuthError {} - /** `/auth/bootstrap` through `bearer`: who its tokens act as. */ export type Identify = (bearer: BearerTokenProvider) => Promise; @@ -303,7 +295,8 @@ export interface TokenProviderOptions { fetch?: typeof fetch; /** * Serve this saved account, current or not, reading its own file instead of the pointer. Used - * to verify an account before switching to it; never moves the pointer. + * to verify an account before switching to it, and for a command that names a saved account; + * never moves the pointer. */ account?: OAuthAccount; } @@ -321,11 +314,12 @@ export interface TokenProviderDeps extends TokenProviderOptions { * anything. So each refresh re-reads the session before acting on a failure or writing, and keeps * a peer's rotation rather than overwriting it or signing out. * - * Pinned to the first session's account (business and user): a command resolves who it acts as - * once, so after a switch to another account every read throws rather than hand out its tokens. - * Signing in again to the same account doesn't: its new tokens are kept like a peer's rotation. - * Reads go through the store's pointer (or, with `account`, that account's file); writes and - * deletes touch only the pinned account's file, never the pointer. + * Pinned to one account (business and user): `account`, else the current account at the first + * read, which is the only read through the store's pointer. Every later read is of the pinned + * account's own file, so a switch to another account meanwhile doesn't change who the command acts + * as, and a logout of the pinned account makes the next read throw not-signed-in. Signing in again + * to the same account replaces its file, and its new tokens are kept like a peer's rotation. + * Writes and deletes touch only the pinned account's file, never the pointer. */ export function createTokenProvider( environment: Environment, @@ -333,30 +327,40 @@ export function createTokenProvider( ): BearerTokenProvider { let inflight: Promise | undefined; let pinned: OAuthAccount | undefined = deps.account; + /** The read through the pointer that pins, shared by reads that start before it settles. */ + let pinning: Promise | undefined; const fresh = (session: OAuthSession): boolean => session.expiresAt - deps.now() > REFRESH_LEEWAY_MS; - const readOrThrow = async (): Promise => { - const session = - deps.account === undefined - ? await deps.store.readSession(environment) - : await deps.store.readAccount(environment, deps.account); - + const orThrow = (session: OAuthSession | null): OAuthSession => { if (session === null) { throw notSignedIn(environment); } - pinned ??= { businessId: session.businessId, userId: session.userId }; + return session; + }; - if (!sameAccount(session, pinned)) { - throw new SignInChangedError( - 'login_required', - `The ${environment} sign-in changed while this command was running. Run it again.`, - ); + const readOrThrow = async (): Promise => { + if (pinned !== undefined) { + return orThrow(await deps.store.readAccount(environment, pinned)); } - return session; + // A failed first read pins nothing, so the next read goes through the pointer again. + pinning ??= deps.store + .readSession(environment) + .then((session) => { + const current = orThrow(session); + + pinned = { businessId: current.businessId, userId: current.userId }; + + return current; + }) + .finally(() => { + pinning = undefined; + }); + + return pinning; }; /** @@ -440,34 +444,15 @@ export function createTokenProvider( // Writes only this account's own file, never the pointer, so a logout or a switch to another // account can't be undone or replaced by it, whenever it lands. The re-read is for a - // same-account peer: a logout or a switch makes it throw, and a peer's rotation or a sign-in - // again to this account is kept (this command still uses the token it was issued). What's - // left is a peer writing between this re-read and the rename, which this write overwrites: - // a peer rotation holds the same pair within Kizen's 300s grace window (beyond it the cost is - // a re-login), and tokens from a sign-in again are replaced by this grant's, which Kizen + // same-account peer: a logout of this account makes it throw, and a peer's rotation or a + // sign-in again to this account is kept (this command still uses the token it was issued). + // What's left is a peer writing between this re-read and the rename, which this write + // overwrites: a peer rotation holds the same pair within Kizen's 300s grace window (beyond it + // the cost is a re-login), and tokens from a sign-in again are replaced by this grant's, which Kizen // still accepts. A write landing after a logout of this account leaves it saved but not // current. The deletes above are likewise scoped to this account's file. Vault entry: // cli-oauth-via-kizen-dcr-what-webapp-supports-the-app-builder-poc-design-and-its-gotchas - let rotated: boolean; - - try { - ({ rotated } = await reread()); - } catch (err) { - // A switch to another account landed while the request was out. The server has already - // spent this account's refresh token, so its file would be useless to switch back to: - // save the new pair there (its own file only, and only if no peer rotated it or a logout - // removed it), then abort as usual. - if (err instanceof SignInChangedError) { - const own = await deps.store.readAccount(environment, current).catch(() => null); - - if (own !== null && own.refreshToken === current.refreshToken) { - // Best effort: the error to report is the abort below. - await deps.store.writeSession(next).catch(() => undefined); - } - } - - throw err; - } + const { rotated } = await reread(); if (!rotated) { await deps.store.writeSession(next); diff --git a/src/lib/pushHeadless.test.ts b/src/lib/pushHeadless.test.ts index 759788a..0de1884 100644 --- a/src/lib/pushHeadless.test.ts +++ b/src/lib/pushHeadless.test.ts @@ -15,7 +15,7 @@ import type { ViewsByPlugin } from './exportBlock.js'; import { formatValidationIssues } from './formatValidationIssues.js'; import { BASE_URLS } from '../../shared/lib/kizenUrls.js'; import { KizenApiError } from './kizenClient.js'; -import { createOAuthStore, type OAuthSession } from './oauthStore.js'; +import { createOAuthStore, type OAuthSession, type SavedAccount } from './oauthStore.js'; import type { KizenClient, PushMapEntry, @@ -299,8 +299,9 @@ const oauthDeps = ( sessions: Environment[], fake: FakeClient = makeClient(), identity: typeof IDENTITY = IDENTITY, -): OAuthPushDeps & { listSessions: Mock; connect: Mock } => ({ +): OAuthPushDeps & { listSessions: Mock; listAccounts: Mock; connect: Mock } => ({ listSessions: vi.fn(() => Promise.resolve(sessions)), + listAccounts: vi.fn(() => Promise.resolve([])), connect: vi.fn(() => Promise.resolve({ identity, client: fake.client })), }); @@ -369,8 +370,10 @@ const makeHarness = (options: HarnessOptions = {}): Harness => { exitCodes.push(code); }, // Signed out: a test that leaves out -c / --profile must sign in explicitly. + processEnv: {}, oauth: { listSessions: vi.fn(() => Promise.resolve([])), + listAccounts: vi.fn(() => Promise.resolve([])), connect: vi.fn(() => Promise.reject(new Error('no OAuth session in this test'))), }, ...options.deps, @@ -2215,55 +2218,71 @@ describe('sign-in session credentials', () => { }); }); - it('push aborts without writing the push map when the login changes after identity lookup', async () => { + /** + * A Kizen API that serves either login, by token, recording every request. The bootstrap answer + * names the business of whichever token was presented; `onBootstrap` runs before it answers. + */ + const kizenFor = ( + onBootstrap: () => Promise = () => Promise.resolve(), + ): { + fetchMock: ReturnType>; + requests: { method: string; url: string; auth: string }[]; + } => { + const base = BASE_URLS.staging; + const businessFor: Record = { + 'Bearer access-a': 'oauth-biz-a', + 'Bearer access-b': 'oauth-biz-b', + }; + const requests: { method: string; url: string; auth: string }[] = []; + const fetchMock = vi.fn(async (input, init) => { + const url = input as string; + const method = init?.method ?? 'GET'; + const auth = new Headers(init?.headers).get('Authorization') ?? ''; + + requests.push({ method, url, auth }); + + if (url === `${base}/auth/bootstrap`) { + await onBootstrap(); + + return jsonResponse({ + team: { user: 'oauth-user' }, + business: { id: businessFor[auth], name: 'Acme' }, + }); + } + + if (url === `${base}/dashboards/D1` && method === 'GET') { + return jsonResponse(makeDashboard()); + } + + if (url === `${base}/dashboards/D1/dashlet` && method === 'POST') { + return jsonResponse({ + id: 'NEW', + name: pushKey(PLUGIN, BLOCK), + layout: null, + config: {}, + dashboard: 'D1', + } satisfies WireDashlet); + } + + return new Response('not found', { status: 404 }); + }); + + return { fetchMock, requests }; + }; + + const loginBFrom = (loginA: OAuthSession): OAuthSession => ({ + ...loginA, + ...ACCOUNT_B, + businessName: 'Bravo', + accessToken: 'access-b', + refreshToken: 'refresh-b', + }); + + it('push keeps acting as the account it started with when the login changes after identity lookup', async () => { await withStore(async (store, loginA) => { - const loginB = { - ...loginA, - ...ACCOUNT_B, - accessToken: 'access-b', - refreshToken: 'refresh-b', - }; - const base = BASE_URLS.staging; - const businessFor: Record = { - 'Bearer access-a': 'oauth-biz-a', - 'Bearer access-b': 'oauth-biz-b', - }; - const requests: { method: string; url: string; auth: string }[] = []; - // A Kizen API that serves either login: only the provider's pin can stop - // business B's token from writing a dashlet the push records under business A. - const fetchMock = vi.fn(async (input, init) => { - const url = input as string; - const method = init?.method ?? 'GET'; - const auth = new Headers(init?.headers).get('Authorization') ?? ''; - - requests.push({ method, url, auth }); - - if (url === `${base}/auth/bootstrap`) { - // Another terminal signs in to another business right after the lookup. - await signIn(store, loginB); - - return jsonResponse({ - team: { user: 'oauth-user' }, - business: { id: businessFor[auth], name: 'Acme' }, - }); - } - - if (url === `${base}/dashboards/D1` && method === 'GET') { - return jsonResponse(makeDashboard()); - } - - if (url === `${base}/dashboards/D1/dashlet` && method === 'POST') { - return jsonResponse({ - id: 'NEW', - name: pushKey(PLUGIN, BLOCK), - layout: null, - config: {}, - dashboard: 'D1', - } satisfies WireDashlet); - } - - return new Response('not found', { status: 404 }); - }); + const loginB = loginBFrom(loginA); + // Another terminal switches to another business right after the lookup. + const { fetchMock, requests } = kizenFor(() => signIn(store, loginB)); // The real wiring: one provider behind the identity lookup and every write. const harness = makeHarness({ deps: { oauth: createOAuthPushDeps(store, { fetch: fetchMock }) }, @@ -2275,24 +2294,348 @@ describe('sign-in session credentials', () => { harness.deps, ); + expect(harness.json()).toMatchObject({ ok: true, businessId: 'oauth-biz-a' }); + expect(harness.exitCodes).toStrictEqual([]); + // Every request, the write included, went out as login A; B's token never did. + expect(requests.length).toBeGreaterThan(1); + expect(requests.every(({ auth }) => auth === 'Bearer access-a')).toBe(true); + expect(harness.pushMap.entries).toMatchObject([ + { environment: 'staging', businessId: 'oauth-biz-a', dashletId: 'NEW' }, + ]); + // The switch stands. + expect(await store.readSession('staging')).toStrictEqual(loginB); + }); + }); + + it('push with --business uses that saved account while another is current, and never moves the pointer', async () => { + await withStore(async (store, loginA) => { + const loginB = loginBFrom(loginA); + + await store.writeSession(loginB); + + const { fetchMock, requests } = kizenFor(); + const harness = makeHarness({ + deps: { oauth: createOAuthPushDeps(store, { fetch: fetchMock }) }, + }); + + await runPushHeadless( + BLOCK, + { + json: true, + env: 'staging', + business: 'bravo', + dashboard: 'D1', + create: true, + yes: true, + }, + harness.deps, + ); + + expect(harness.json()).toMatchObject({ ok: true, businessId: 'oauth-biz-b' }); + expect(requests.length).toBeGreaterThan(1); + expect(requests.every(({ auth }) => auth === 'Bearer access-b')).toBe(true); + expect(await store.readSession('staging')).toStrictEqual(loginA); + }); + }); + + it('push --json with --business and no --env uses staging when nothing is current and it is saved only there', async () => { + await withStore(async (store, loginA) => { + await store.writeSession(loginBFrom(loginA)); + await store.signOut('staging'); + + const { fetchMock, requests } = kizenFor(); + const harness = makeHarness({ + deps: { oauth: createOAuthPushDeps(store, { fetch: fetchMock }) }, + }); + + await runPushHeadless( + BLOCK, + { json: true, business: 'bravo', dashboard: 'D1', create: true, yes: true }, + harness.deps, + ); + expect(harness.json()).toMatchObject({ + ok: true, + environment: 'staging', + businessId: 'oauth-biz-b', + }); + expect(requests.length).toBeGreaterThan(1); + expect(requests.every(({ auth }) => auth === 'Bearer access-b')).toBe(true); + expect(await store.listSessions()).toStrictEqual([]); + }); + }); + + it('targets with APPBUILDER_BUSINESS and no --env uses the only environment where it is saved', async () => { + await withStore(async (store, loginA) => { + await store.writeSession(loginBFrom(loginA)); + + const { fetchMock, requests } = kizenFor(); + const harness = makeHarness({ + deps: { + processEnv: { APPBUILDER_BUSINESS: 'Bravo' }, + oauth: createOAuthPushDeps(store, { fetch: fetchMock }), + }, + }); + + await runTargetsHeadless({ json: true }, harness.deps); + + // The session resolved to B in staging (the fake API has no listing endpoints). + expect(requests[0]).toStrictEqual({ + method: 'GET', + url: `${BASE_URLS.staging}/auth/bootstrap`, + auth: 'Bearer access-b', + }); + }); + }); + + it('push with --business saved in staging and go and no --env is needs_choice', async () => { + await withStore(async (store, loginA) => { + const loginB = loginBFrom(loginA); + + await store.writeSession(loginB); + await store.writeSession({ ...loginB, environment: 'go' }); + + const { fetchMock, requests } = kizenFor(); + const oauth = createOAuthPushDeps(store, { fetch: fetchMock }); + const connect = vi.spyOn(oauth, 'connect'); + const harness = makeHarness({ deps: { oauth } }); + + await runPushHeadless(BLOCK, { json: true, business: 'bravo' }, harness.deps); + + expect(harness.json()).toStrictEqual({ ok: false, - code: 'auth_failed', - message: 'The staging sign-in changed while this command was running. Run it again.', + code: 'needs_choice', + message: + '--business bravo matches saved accounts in several environments; pass --env : go, staging', + choice: 'environment', + choices: [ + { value: 'go', label: 'go', args: ['--env', 'go'] }, + { value: 'staging', label: 'staging', args: ['--env', 'staging'] }, + ], }); - expect(harness.exitCodes).not.toStrictEqual([]); - // The identity came from login A, and nothing after it went out at all. - expect(requests).toStrictEqual([ - { method: 'GET', url: `${base}/auth/bootstrap`, auth: 'Bearer access-a' }, - ]); - expect(harness.deps.createClient).not.toHaveBeenCalled(); - expect(harness.deps.writePushMap).not.toHaveBeenCalled(); - expect(harness.pushMap.entries).toStrictEqual([]); - expect(await store.readSession('staging')).toStrictEqual(loginB); + expect(connect).not.toHaveBeenCalled(); + expect(requests).toStrictEqual([]); + + const text = makeHarness({ deps: { oauth } }); + + await runPushHeadless(BLOCK, { business: 'bravo' }, text.deps); + + expect(text.stderr.join('')).toContain( + `appbuilder block push ${BLOCK} --env staging --business bravo`, + ); + expect(text.stderr.join('')).not.toContain('signed-in'); + }); + }); + + it('targets with APPBUILDER_BUSINESS uses that saved account', async () => { + await withStore(async (store, loginA) => { + await store.writeSession(loginBFrom(loginA)); + + const { fetchMock, requests } = kizenFor(); + const harness = makeHarness({ + deps: { + processEnv: { APPBUILDER_BUSINESS: ACCOUNT_B.businessId }, + oauth: createOAuthPushDeps(store, { fetch: fetchMock }), + }, + }); + + await runTargetsHeadless({ json: true, env: 'staging' }, harness.deps); + + expect(requests.length).toBeGreaterThan(0); + expect(requests.every(({ auth }) => auth === 'Bearer access-b')).toBe(true); + expect(await store.readSession('staging')).toStrictEqual(loginA); }); }); }); +describe('naming a saved account (--business / --user, APPBUILDER_BUSINESS / APPBUILDER_USER)', () => { + const BIZ_A = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; + const saved = (businessId: string, userId: string, businessName: string): SavedAccount => ({ + account: { businessId, userId }, + session: oauthSession({ businessId, userId, businessName, email: `${userId}@x.test` }), + current: false, + }); + const ACCOUNTS: SavedAccount[] = [ + saved(BIZ_A, 'a1a1a1a1-a1a1-4a1a-8a1a-a1a1a1a1a1a1', 'Acme'), + saved(ACCOUNT_B.businessId, ACCOUNT_B.userId, 'Bravo'), + ]; + + const oauthWithAccounts = (accounts: SavedAccount[] = ACCOUNTS): ReturnType => { + const oauth = oauthDeps(['staging']); + + // Saved in staging only. + oauth.listAccounts.mockImplementation((environment: Environment) => + Promise.resolve(environment === 'staging' ? accounts : []), + ); + + return oauth; + }; + + it.each<[string, Record, PushCommandOptions, string]>([ + [ + '--business beats APPBUILDER_BUSINESS', + { APPBUILDER_BUSINESS: 'Acme' }, + { business: 'Bravo' }, + ACCOUNT_B.businessId, + ], + [ + 'APPBUILDER_BUSINESS when no flag is passed', + { APPBUILDER_BUSINESS: 'bravo' }, + {}, + ACCOUNT_B.businessId, + ], + ['--business by id', {}, { business: BIZ_A }, BIZ_A], + ])('%s', async (_name, processEnv, flags, businessId) => { + const oauth = oauthWithAccounts(); + const { deps } = makeHarness({ deps: { oauth, processEnv } }); + const step = await resolveCredentials(flags, deps); + + expect(step.ok).toBe(true); + expect(oauth.listAccounts).toHaveBeenCalledWith('staging'); + expect(oauth.connect.mock.calls).toStrictEqual([ + ['staging', ACCOUNTS.find(({ account }) => account.businessId === businessId)?.account], + ]); + }); + + it('an empty APPBUILDER_BUSINESS counts as unset: the current account', async () => { + const oauth = oauthWithAccounts(); + const { deps } = makeHarness({ deps: { oauth, processEnv: { APPBUILDER_BUSINESS: '' } } }); + + expect((await resolveCredentials({}, deps)).ok).toBe(true); + expect(oauth.listAccounts).not.toHaveBeenCalled(); + expect(oauth.connect.mock.calls).toStrictEqual([['staging', undefined]]); + }); + + it.each<[string, Record, PushCommandOptions, string]>([ + [ + '--user without --business', + {}, + { user: ACCOUNT_B.userId }, + '--user only applies with --business.', + ], + [ + 'APPBUILDER_USER without APPBUILDER_BUSINESS', + { APPBUILDER_USER: ACCOUNT_B.userId }, + {}, + 'APPBUILDER_USER only applies with APPBUILDER_BUSINESS.', + ], + ])('%s is a usage error', async (_name, processEnv, flags, message) => { + const oauth = oauthWithAccounts(); + const { deps } = makeHarness({ deps: { oauth, processEnv } }); + + expect(await resolveCredentials(flags, deps)).toStrictEqual({ + ok: false, + code: 'usage_error', + message, + }); + expect(oauth.connect).not.toHaveBeenCalled(); + }); + + it.each<[string, PushCommandOptions, SavedAccount[], string, string]>([ + [ + 'no saved account matches', + { business: 'Nope' }, + ACCOUNTS, + 'No saved staging account matches --business Nope. Saved accounts:', + `Bravo (${ACCOUNT_B.businessId})`, + ], + [ + 'several saved accounts match', + { business: 'acme' }, + [ + ...ACCOUNTS, + saved( + 'cccccccc-cccc-4ccc-8ccc-cccccccccccc', + 'c1c1c1c1-c1c1-4c1c-8c1c-c1c1c1c1c1c1', + 'ACME', + ), + ], + '--business acme matches several saved staging accounts. Pass one of these business ids instead:', + 'ACME (cccccccc-cccc-4ccc-8ccc-cccccccccccc)', + ], + ])( + '%s: a usage_error listing them, never the current account', + async (_name, flags, accounts, message, listed) => { + const oauth = oauthWithAccounts(accounts); + const harness = makeHarness({ deps: { oauth } }); + + await runPushHeadless(BLOCK, { json: true, dashboard: 'D1', ...flags }, harness.deps); + + const result = harness.json() as { ok: boolean; code: string; message: string }; + + expect(result).toMatchObject({ ok: false, code: 'usage_error' }); + expect(result.message).toContain(message); + // The candidates: every saved account when none matches, the matches when several do. + expect(result.message).toContain(listed); + expect(oauth.connect).not.toHaveBeenCalled(); + expect(harness.deps.packageBlocks).not.toHaveBeenCalled(); + }, + ); + + it.each<[PushCommandOptions, string]>([ + [{ profile: 'Demo', business: 'Bravo' }, '--business'], + [{ credentials: '/tmp/c.json', business: 'Bravo' }, '--business'], + [{ profile: 'Demo', user: ACCOUNT_B.userId }, '--user'], + ])('%j with an API key is a usage error', async (flags, flag) => { + const oauth = oauthWithAccounts(); + const harness = makeHarness({ deps: { oauth } }); + + await runPushHeadless(BLOCK, { json: true, ...flags }, harness.deps); + + expect(harness.json()).toStrictEqual({ + ok: false, + code: 'usage_error', + message: `${flag} selects a saved sign-in account, so it can't be combined with -c or --profile.`, + }); + expect(oauth.connect).not.toHaveBeenCalled(); + expect(harness.deps.loadCredentialsDetailed).not.toHaveBeenCalled(); + }); + + it('an API key ignores APPBUILDER_BUSINESS and APPBUILDER_USER', async () => { + const oauth = oauthWithAccounts(); + const { deps } = makeHarness({ + deps: { oauth, processEnv: { APPBUILDER_BUSINESS: 'Nope', APPBUILDER_USER: 'nobody' } }, + }); + const step = await resolveCredentials(API_KEY, deps); + + expect(step).toMatchObject({ ok: true, value: { credentials: { businessId: 'biz' } } }); + expect(oauth.listAccounts).not.toHaveBeenCalled(); + expect(oauth.connect).not.toHaveBeenCalled(); + }); + + it('the environment choices keep --business and --user', async () => { + const oauth = oauthDeps(['staging', 'integration']); + + // Bravo is saved in both environments. + oauth.listAccounts.mockImplementation((environment: Environment) => + Promise.resolve(environment === 'staging' || environment === 'integration' ? ACCOUNTS : []), + ); + + const harness = makeHarness({ deps: { oauth } }); + + await runPushHeadless( + BLOCK, + { json: true, business: 'Bravo', user: ACCOUNT_B.userId }, + harness.deps, + ); + + expect(harness.json()).toMatchObject({ + ok: false, + code: 'needs_choice', + choice: 'environment', + choices: [{ value: 'staging' }, { value: 'integration' }], + }); + + const text = makeHarness({ deps: { oauth } }); + + await runPushHeadless(BLOCK, { business: 'Bravo', user: ACCOUNT_B.userId }, text.deps); + + expect(text.stderr.join('')).toContain( + `appbuilder block push ${BLOCK} --env staging --business Bravo --user ${ACCOUNT_B.userId}`, + ); + }); +}); + describe('the sign-in session is the default', () => { const NOT_SIGNED_IN = "You're not signed in. Run `appbuilder login` (add `--env ` to sign in to an environment other than go). To use an API key instead (deprecated), pass --profile or -c ."; @@ -2442,7 +2785,7 @@ describe('the sign-in session is the default', () => { environment: 'staging', businessId: 'oauth-biz', }); - expect(oauth.connect.mock.calls).toStrictEqual([['staging']]); + expect(oauth.connect.mock.calls).toStrictEqual([['staging', undefined]]); expectNoApiKeyReads(harness.deps); }); @@ -2495,7 +2838,7 @@ describe('the sign-in session is the default', () => { environment: 'staging', businessId: 'oauth-biz', }); - expect(oauth.connect.mock.calls).toStrictEqual([['staging']]); + expect(oauth.connect.mock.calls).toStrictEqual([['staging', undefined]]); expect(rerun.exitCodes).toStrictEqual([]); }); }); @@ -2526,7 +2869,7 @@ describe('the sign-in session is the default', () => { environment: 'staging', businessId: 'oauth-biz', }); - expect(oauth.connect).toHaveBeenCalledWith('staging'); + expect(oauth.connect).toHaveBeenCalledWith('staging', undefined); expect(fake.createDashlet).toHaveBeenCalledTimes(1); expect(harness.pushMap.entries).toMatchObject([ { environment: 'staging', businessId: 'oauth-biz' }, @@ -2642,7 +2985,7 @@ describe('the sign-in session is the default', () => { await vi.waitFor(() => { expect(phase()?.type).toBe('surface-select'); }); - expect(oauth.connect.mock.calls).toStrictEqual([['staging']]); + expect(oauth.connect.mock.calls).toStrictEqual([['staging', undefined]]); expect(state.session.credentials).toStrictEqual({ environment: 'staging', businessId: 'oauth-biz', diff --git a/src/lib/pushHeadless.ts b/src/lib/pushHeadless.ts index e394884..c51050d 100644 --- a/src/lib/pushHeadless.ts +++ b/src/lib/pushHeadless.ts @@ -61,10 +61,17 @@ import { notSignedInWithApiKeyHint, pinnedProfileNotice, selectAuth, + type AccountRequest, type AuthFlags, } from './authSelection.js'; -import { createDefaultTokenProvider, resolveSessionEnvironment } from './oauthSession.js'; -import { createOAuthStore, type OAuthStore } from './oauthStore.js'; +import { requireNamedAccount, resolveAccountEnvironment } from './oauthCommands.js'; +import { createDefaultTokenProvider } from './oauthSession.js'; +import { + createOAuthStore, + type OAuthAccount, + type OAuthStore, + type SavedAccount, +} from './oauthStore.js'; import { isClaudeSkillStale, STALE_CLAUDE_SKILL_WARNING } from './setupClaude.js'; import { quoteArg } from './shellArg.js'; @@ -122,6 +129,8 @@ export interface PushCommandOptions { forget?: boolean; json?: boolean; env?: string; + business?: string; + user?: string; } export interface TargetsCommandOptions { @@ -130,13 +139,23 @@ export interface TargetsCommandOptions { object?: string; json?: boolean; env?: string; + business?: string; + user?: string; } /** The stored `appbuilder login` sessions, which commands use unless -c or --profile is passed. */ export interface OAuthPushDeps { listSessions: () => Promise; - /** The identity and the client that resolved it, so later requests are pinned to that login. */ - connect: (environment: Environment) => Promise<{ identity: KizenIdentity; client: KizenClient }>; + /** The saved accounts `--business` / `APPBUILDER_BUSINESS` choose among. */ + listAccounts: (environment: Environment) => Promise; + /** + * The identity and the client that resolved it, so later requests are pinned to that login: + * `account` when given, else the current account at the first read. + */ + connect: ( + environment: Environment, + account?: OAuthAccount, + ) => Promise<{ identity: KizenIdentity; client: KizenClient }>; } export interface PushDeps { @@ -156,6 +175,8 @@ export interface PushDeps { writeStdout: (text: string) => void; writeStderr: (text: string) => void; setExitCode: (code: number) => void; + /** Where `APPBUILDER_BUSINESS` / `APPBUILDER_USER` are read from. */ + processEnv: Readonly>; oauth: OAuthPushDeps; } @@ -167,9 +188,11 @@ export function createOAuthPushDeps( return { listSessions: () => store.listSessions(), - connect: async (environment) => { + listAccounts: (environment) => store.listAccounts(environment), + connect: async (environment, account) => { const bearer = createDefaultTokenProvider(environment, store, { ...(fetchImpl && { fetch: fetchImpl }), + ...(account !== undefined && { account }), }); const client = createKizenClient({ bearer, ...(fetchImpl && { fetch: fetchImpl }) }); @@ -201,6 +224,7 @@ export function createDefaultPushDeps(cwd: string): PushDeps { setExitCode: (code) => { process.exitCode = code; }, + processEnv: process.env, // Constructing the store does no IO; a -c / --profile push never touches it. oauth: createOAuthPushDeps(createOAuthStore()), }; @@ -320,15 +344,10 @@ async function pinnedProfileNotices(deps: PushDeps): Promise { async function resolveSessionCredentials( env: string | undefined, + request: AccountRequest | undefined, deps: PushDeps, ): Promise> { - // Kept so the environment choices come from the same read that found several sessions. - let sessions: Environment[] = []; - const resolved = await resolveSessionEnvironment(env, async () => { - sessions = await deps.oauth.listSessions(); - - return sessions; - }); + const resolved = await resolveAccountEnvironment(env, request, deps.oauth); if (!resolved.ok) { switch (resolved.problem) { @@ -337,7 +356,7 @@ async function resolveSessionCredentials( case 'several_sessions': return failure('needs_choice', resolved.message, { choice: 'environment', - choices: sessions.map((environment) => ({ + choices: resolved.environments.map((environment) => ({ value: environment, label: environment, args: ['--env', environment], @@ -350,10 +369,29 @@ async function resolveSessionCredentials( const { environment } = resolved; + let account: OAuthAccount | undefined; + + if (request !== undefined) { + let accounts: SavedAccount[]; + + try { + accounts = await deps.oauth.listAccounts(environment); + } catch (error) { + return apiFailure(error); + } + + // A name that matches no saved account, or several, is a bad argument: never the current one. + try { + account = requireNamedAccount(environment, accounts, request); + } catch (error) { + return failure('usage_error', errorMessage(error)); + } + } + let connected: Awaited>; try { - connected = await deps.oauth.connect(environment); + connected = await deps.oauth.connect(environment, account); } catch (error) { return apiFailure(error); } @@ -367,14 +405,15 @@ async function resolveSessionCredentials( } /** - * The `appbuilder login` session by default (--env picks its environment); an API key - * only for -c or --profile, which never read the sessions. + * The `appbuilder login` session by default (--env picks its environment; --business / --user, or + * APPBUILDER_BUSINESS / APPBUILDER_USER, a saved account in it); an API key only for -c or + * --profile, which never read the sessions. */ export async function resolveCredentials( options: AuthFlags, deps: PushDeps, ): Promise> { - const selected = selectAuth(options); + const selected = selectAuth(options, deps.processEnv); if (!selected.ok) { return failure('usage_error', selected.message); @@ -383,7 +422,7 @@ export async function resolveCredentials( const { selection } = selected; if (selection.kind === 'oauth') { - return resolveSessionCredentials(selection.env, deps); + return resolveSessionCredentials(selection.env, selection.account, deps); } return selection.credentials === undefined @@ -1191,6 +1230,12 @@ function writeResult(output: Output, result: unknown, text: string, jsonText = t } } +/** `--business` / `--user` as passed, for the commands a choice prints. */ +const accountArgs = (options: AuthFlags): string[] => [ + ...(options.business === undefined ? [] : ['--business', options.business]), + ...(options.user === undefined ? [] : ['--user', options.user]), +]; + const credentialArgs = (options: AuthFlags): string[] => { if (options.credentials !== undefined) { return ['-c', options.credentials]; @@ -1200,7 +1245,7 @@ const credentialArgs = (options: AuthFlags): string[] => { return ['--profile', options.profile]; } - return options.env === undefined ? [] : ['--env', options.env]; + return [...(options.env === undefined ? [] : ['--env', options.env]), ...accountArgs(options)]; }; const writeNotices = (deps: PushDeps, notices: readonly string[]): void => { @@ -1227,7 +1272,7 @@ export async function runPushHeadless( let blockArgs = apiName === undefined ? [] : [apiName]; const choiceArgs: ChoiceArgs = (choice, kind) => { if (kind === 'environment') { - return [...blockArgs, ...choice.args]; + return [...blockArgs, ...choice.args, ...accountArgs(options)]; } if (kind === 'block') { @@ -1465,7 +1510,11 @@ export async function runTargetsHeadless( const output: Output = { json: options.json === true, deps }; const command = 'appbuilder block targets'; const objectArgs = options.object === undefined ? [] : ['--object', options.object]; - const choiceArgs: ChoiceArgs = (choice) => [...choice.args, ...objectArgs]; + const choiceArgs: ChoiceArgs = (choice) => [ + ...choice.args, + ...accountArgs(options), + ...objectArgs, + ]; const fail = (result: PushFailure): void => { writeFailure(output, result, command, choiceArgs); }; diff --git a/src/program.test.ts b/src/program.test.ts index 5cda2d8..8ff511a 100644 --- a/src/program.test.ts +++ b/src/program.test.ts @@ -41,6 +41,22 @@ vi.mock('./lib/kizenAdmin.js', async (importOriginal) => { businessId: '11111111-1111-4111-8111-111111111111', userId: '22222222-2222-4222-8222-222222222222', } as never), + readAccount: (_environment, account) => Promise.resolve(account as never), + listAccounts: (environment) => + Promise.resolve( + environment !== 'staging' + ? [] + : [ + { + account: { + businessId: '11111111-1111-4111-8111-111111111111', + userId: '22222222-2222-4222-8222-222222222222', + }, + session: undefined, + current: true, + }, + ], + ), }, ensureKizen: () => { admin.provisioned += 1; @@ -148,9 +164,9 @@ const COMMAND_OPTION_FLAGS: Record = { block: [], login: ['--env ', '--business ', '--user ', '--list', '--json'], logout: ['--env ', '--all', '--business ', '--user '], - whoami: ['--env '], + whoami: ['--env ', '--business ', '--user '], 'auth-header': ['--env ', '--business ', '--user '], - admin: ['--env '], + admin: ['--env ', '--business ', '--user '], }; const BLOCK_PUSH_FLAGS = [ @@ -166,6 +182,8 @@ const BLOCK_PUSH_FLAGS = [ '--forget', '--json', '--env ', + '--business ', + '--user ', ]; const BLOCK_TARGETS_FLAGS = [ @@ -174,6 +192,8 @@ const BLOCK_TARGETS_FLAGS = [ '--object ', '--json', '--env ', + '--business ', + '--user ', ]; function subcommand(program: Command, name: string): Command { @@ -397,6 +417,8 @@ describe('block command', () => { 'forget', 'json', 'env', + 'business', + 'user', ]); expect(attributeNames('targets')).toStrictEqual([ 'credentials', @@ -404,6 +426,8 @@ describe('block command', () => { 'object', 'json', 'env', + 'business', + 'user', ]); }); @@ -532,6 +556,20 @@ describe('admin command', () => { ['--env=staging', 'x', '--', 'y'], ['x', '--', 'y'], ], + [ + ['--business', '11111111-1111-4111-8111-111111111111', 'status', '--business', 'go'], + ['status', '--business', 'go'], + ], + [ + [ + '--user=22222222-2222-4222-8222-222222222222', + '--business=11111111-1111-4111-8111-111111111111', + '--', + '--user', + 'x', + ], + ['--user', 'x'], + ], [['--', '--help'], ['--help']], [['--', '--version'], ['--version']], [ @@ -570,7 +608,9 @@ describe('admin command', () => { expect(() => program.parse(['help', 'admin'], { from: 'user' })).toThrow( expect.objectContaining({ code: 'commander.help' }), ); - expect(out).toContain('Usage: appbuilder admin [--env ] [--] '); + expect(out).toContain( + 'Usage: appbuilder admin [--env ] [--business [--user ]] [--] ', + ); expect(out).toContain('--env '); expect(admin.provisioned).toBe(0); expect(admin.spawned).toStrictEqual([]); @@ -589,7 +629,7 @@ describe('admin command', () => { command.configureHelp({ helpWidth: 100 }); expect(command.helpInformation()).toContain( - 'Usage: appbuilder admin [--env ] [--] \n appbuilder admin init', + 'Usage: appbuilder admin [--env ] [--business [--user ]] [--] \n appbuilder admin init', ); }); }); From 25f2c8a7df63234b764b8e314a8f539532435d0e Mon Sep 17 00:00:00 2001 From: Keegan Donley Date: Fri, 23 Oct 2026 16:22:12 -0500 Subject: [PATCH 2/3] KZN-19166 - add more reliable locking mechanism to prevent logout race --- src/lib/oauthSession.test.ts | 115 +++++++++++- src/lib/oauthSession.ts | 40 ++-- src/lib/oauthStore.test.ts | 348 ++++++++++++++++++++++++++++++++++- src/lib/oauthStore.ts | 240 ++++++++++++++++++++++-- src/lib/pushHeadless.test.ts | 103 +++++++++++ src/lib/pushHeadless.ts | 38 ++-- src/test/oauth.ts | 14 +- 7 files changed, 845 insertions(+), 53 deletions(-) diff --git a/src/lib/oauthSession.test.ts b/src/lib/oauthSession.test.ts index eb3d8e0..26c02de 100644 --- a/src/lib/oauthSession.test.ts +++ b/src/lib/oauthSession.test.ts @@ -1,5 +1,6 @@ import { createHash } from 'node:crypto'; import { mkdtemp, readdir, readFile, rm, stat } from 'node:fs/promises'; +import type * as FsPromisesModule from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -37,6 +38,51 @@ import { const AUTHORIZE = `${ISSUER}/discovered/authorize`; const REGISTER = `${ISSUER}/discovered/register`; +/** + * `pauseRename`: the next `rename` onto a path it matches waits on `gate` first, a pause inside an + * atomic write just before it lands. `onCreateTaken` sees each exclusive create that finds its + * path taken: a waiter on a lock. + */ +const hooks = vi.hoisted(() => ({ + pauseRename: undefined as + | { matches: (to: string) => boolean; reached: () => void; gate: Promise } + | undefined, + onCreateTaken: undefined as ((path: string) => void) | undefined, +})); + +vi.mock('node:fs/promises', async (importOriginal) => { + const fs = await importOriginal(); + const rename = async (...args: Parameters): Promise => { + const pause = hooks.pauseRename; + + if (pause?.matches(String(args[1])) === true) { + hooks.pauseRename = undefined; + pause.reached(); + await pause.gate; + } + + return fs.rename(...args); + }; + + const open = async ( + ...args: Parameters + ): Promise => { + try { + return await fs.open(...args); + } catch (err) { + if (args[1] === 'wx' && (err as { code?: unknown }).code === 'EEXIST') { + hooks.onCreateTaken?.(String(args[0])); + } + + throw err; + } + }; + + const mocked = { ...fs, rename, open }; + + return { ...mocked, default: mocked }; +}); + let dir: string; let store: OAuthStore; @@ -46,6 +92,8 @@ beforeEach(async () => { }); afterEach(async () => { + hooks.pauseRename = undefined; + hooks.onCreateTaken = undefined; await rm(dir, { recursive: true, force: true }); }); @@ -787,7 +835,7 @@ describe('createTokenProvider', () => { /** Starts a refresh of login A through `held`'s store; resolves once it is held. */ const refreshUntilHeld = async ( - method: 'writeSession' | 'deleteSession', + method: 'commitRefresh' | 'deleteSession', respond?: Parameters[0], ): Promise<{ pending: Promise; release: () => void }> => { await signIn(store, oauthSession({ expiresAt: EXPIRING })); @@ -802,7 +850,7 @@ describe('createTokenProvider', () => { }; it("a refresh that writes after another account becomes current doesn't replace it", async () => { - const { pending, release } = await refreshUntilHeld('writeSession'); + const { pending, release } = await refreshUntilHeld('commitRefresh'); const b = await loginB(); release(); @@ -843,6 +891,69 @@ describe('createTokenProvider', () => { }); }); + describe('a logout while a refresh is paused just before its write lands', () => { + it.each([ + ['signOut', (logout: OAuthStore) => logout.signOut('staging')], + ['removeAccount', (logout: OAuthStore) => logout.removeAccount('staging', ACCOUNT_A)], + ['signOutAll', (logout: OAuthStore) => logout.signOutAll('staging')], + ])( + 'a logout (%s) of the account wins: its file stays gone and it can no longer be used', + async (_name, logout) => { + await signIn(store, oauthSession({ expiresAt: EXPIRING })); + + const accountFile = store.accountPath('staging', ACCOUNT_A); + const writing = deferred(); + const gate = deferred(); + const logoutWaits = deferred(); + + // Pause the refresh's final write after its last re-read, just before the rename. + hooks.pauseRename = { + matches: (to) => to === accountFile, + reached: () => { + writing.resolve(); + }, + gate: gate.promise, + }; + hooks.onCreateTaken = (path) => { + if (path.endsWith('.lock')) { + logoutWaits.resolve(); + } + }; + + const tokens = provider(fakeAuthServer()); + const pending = tokens.getAccessToken(); + + pending.catch(() => undefined); + await writing.promise; + + // Another terminal logs out of the account: it completes, or waits on the refresh's lock. + const loggingOut = logout(createOAuthStore(dir)); + + await Promise.race([loggingOut, logoutWaits.promise]); + gate.resolve(); + await loggingOut; + + // The command got the token it was issued before the logout... + await expect(pending).resolves.toBe('access-1'); + // ...but the account file stays gone, so nothing serves it from here on. + await expect(stat(accountFile)).rejects.toMatchObject({ code: 'ENOENT' }); + expect(await store.readAccount('staging', ACCOUNT_A)).toBeNull(); + expect(await store.readSession('staging')).toBeNull(); + + const error = await rejectionOf(tokens.getAccessToken()); + + expect(error.code).toBe('login_required'); + expect(error.message).toBe(NOT_SIGNED_IN); + expect( + (await rejectionOf(provider(fakeAuthServer(), { account: ACCOUNT_A }).getAccessToken())) + .message, + ).toBe(NOT_SIGNED_IN); + // No lock file or temp file is left behind. + expect(await readdir(store.dir)).toStrictEqual([]); + }, + ); + }); + describe('two processes refreshing at once', () => { const GRACE_MS = 300_000; diff --git a/src/lib/oauthSession.ts b/src/lib/oauthSession.ts index dc70d3a..734c1b4 100644 --- a/src/lib/oauthSession.ts +++ b/src/lib/oauthSession.ts @@ -309,10 +309,11 @@ export interface TokenProviderDeps extends TokenProviderOptions { /** * Refreshes with under a minute left or after a 401. In-process refreshes share one promise. - * Across processes nothing is locked: Kizen hands a refresh token presented again within its - * grace window the same pair, and refuses it with invalid_grant after that without revoking - * anything. So each refresh re-reads the session before acting on a failure or writing, and keeps - * a peer's rotation rather than overwriting it or signing out. + * Across processes the token request isn't locked: Kizen hands a refresh token presented again + * within its grace window the same pair, and refuses it with invalid_grant after that without + * revoking anything. So each refresh re-reads the session before acting on a failure, and writes + * through `commitRefresh`, which re-reads under the account's lock (the one logout's delete takes) + * and keeps a peer's rotation, or a logout, rather than overwriting it or signing out. * * Pinned to one account (business and user): `account`, else the current account at the first * read, which is the only read through the store's pointer. Every later read is of the pinned @@ -418,8 +419,9 @@ export function createTokenProvider( } // Only this account's file, so a logout or a switch to another account meanwhile is - // untouched. A same-account peer write (a refresh, or a sign-in again) landing between the - // re-read above and this unlink is lost; the cost is a re-login. + // untouched. The delete takes the account's lock, so it never lands inside a peer's + // commit, but doesn't re-check: a same-account peer write (a refresh, or a sign-in again) + // landing between the re-read above and this unlink is lost; the cost is a re-login. const deleted = await deps.store.deleteSession(environment, current); // Best effort, and only this session's client: a login meanwhile may have registered a @@ -442,20 +444,20 @@ export function createTokenProvider( const next = withTokens(current, tokens, deps.now()); - // Writes only this account's own file, never the pointer, so a logout or a switch to another - // account can't be undone or replaced by it, whenever it lands. The re-read is for a - // same-account peer: a logout of this account makes it throw, and a peer's rotation or a - // sign-in again to this account is kept (this command still uses the token it was issued). - // What's left is a peer writing between this re-read and the rename, which this write - // overwrites: a peer rotation holds the same pair within Kizen's 300s grace window (beyond it - // the cost is a re-login), and tokens from a sign-in again are replaced by this grant's, which Kizen - // still accepts. A write landing after a logout of this account leaves it saved but not - // current. The deletes above are likewise scoped to this account's file. Vault entry: + // Writes only this account's own file, never the pointer, so a switch to another account + // can't be undone or replaced by it, whenever it lands. The commit re-reads that file under + // the account's lock, which every logout's delete of it takes too (not across the request + // above): a logout of this account, before the commit or waiting on it, leaves the file gone, + // and this command signed out from its next read on. A peer's rotation or a sign-in again to + // this account is kept (this command still uses the token it was issued). What's left is a + // peer that writes without the lock, a sign-in again, landing between the commit's read and + // its rename, which this write overwrites: its tokens are replaced by this grant's, which + // Kizen still accepts. And when the lock isn't acquired in time, the commit goes ahead + // without it, racing as it would unlocked. The deletes above are likewise scoped to this + // account's file. Vault entry: // cli-oauth-via-kizen-dcr-what-webapp-supports-the-app-builder-poc-design-and-its-gotchas - const { rotated } = await reread(); - - if (!rotated) { - await deps.store.writeSession(next); + if ((await deps.store.commitRefresh(next, current.refreshToken)) === 'gone') { + throw notSignedIn(environment); } return next.accessToken; diff --git a/src/lib/oauthStore.test.ts b/src/lib/oauthStore.test.ts index c8cb188..910c316 100644 --- a/src/lib/oauthStore.test.ts +++ b/src/lib/oauthStore.test.ts @@ -6,8 +6,11 @@ import { join } from 'node:path'; import { afterAll, afterEach, describe, expect, it, vi } from 'vitest'; import { accountKey, + ACCOUNT_LOCK, createOAuthStore, SIGNOUT_MAX_AGE_MS, + withLockFile, + type LockFileOptions, type OAuthAccount, type OAuthSession, type OAuthStore, @@ -36,6 +39,10 @@ const hooks = vi.hoisted(() => ({ before?: boolean; } | undefined, + /** When set, the next `unlink` of a path it matches rejects with `error` instead. */ + failUnlink: undefined as { matches: (path: string) => boolean; error: Error } | undefined, + /** Called with the path whenever an exclusive create (`open(path, 'wx')`) finds it taken. */ + onCreateTaken: undefined as ((path: string) => void) | undefined, })); vi.mock('node:fs/promises', async (importOriginal) => { @@ -63,11 +70,40 @@ vi.mock('node:fs/promises', async (importOriginal) => { return result; }; - return { ...fs, readFile, default: { ...fs, readFile } }; + const unlink = async (...args: Parameters): Promise => { + const fail = hooks.failUnlink; + + if (fail !== undefined && typeof args[0] === 'string' && fail.matches(args[0])) { + hooks.failUnlink = undefined; + throw fail.error; + } + + return fs.unlink(...args); + }; + + const open = async ( + ...args: Parameters + ): Promise => { + try { + return await fs.open(...args); + } catch (err) { + if (args[1] === 'wx' && (err as { code?: unknown }).code === 'EEXIST') { + hooks.onCreateTaken?.(String(args[0])); + } + + throw err; + } + }; + + const mocked = { ...fs, readFile, unlink, open }; + + return { ...mocked, default: mocked }; }); afterEach(() => { hooks.pauseRead = undefined; + hooks.failUnlink = undefined; + hooks.onCreateTaken = undefined; }); /** Every store gets a base dir under this, so the real ~/.kizenappbuilder is never touched. */ @@ -549,3 +585,313 @@ describe('oauth store', () => { } }); }); + +describe('account lock', () => { + /** A refresh's next session for account A: what a commit of `refresh-0` would write. */ + const refreshed = oauthSession({ accessToken: 'access-1', refreshToken: 'refresh-1' }); + const lockOf = (store: OAuthStore, account: OAuthAccount = ACCOUNT_A): string => + join(store.dir, `staging.${accountKey(account)}.lock`); + /** Seconds, for utimes: a minute past the stale threshold. */ + const staleTime = (): number => (Date.now() - ACCOUNT_LOCK.staleMs - 60_000) / 1000; + const noLockFiles = async (store: OAuthStore): Promise => { + expect((await readdir(store.dir)).filter((name) => name.endsWith('.lock'))).toStrictEqual([]); + }; + + /** Options that never wait out a lock within a test's time, so only a release or takeover ends it. */ + const patient: LockFileOptions = { waitMs: 60_000, staleMs: ACCOUNT_LOCK.staleMs, pollMs: 5 }; + + it('the account lock is a separate file beside the account file, never mistaken for an account', () => { + const store = createOAuthStore(join(root, 'lock-name')); + + expect(lockOf(store)).toBe(store.accountPath('staging', ACCOUNT_A).replace(/\.json$/, '.lock')); + }); + + it('commitRefresh writes only while the file still holds the spent refresh token', async () => { + const store = createOAuthStore(join(root, 'commit-outcomes')); + const rotated = oauthSession({ accessToken: 'access-peer', refreshToken: 'refresh-peer' }); + + await signIn(store, oauthSession()); + expect(await store.commitRefresh(refreshed, 'refresh-0')).toBe('written'); + expect(await store.readAccount('staging', ACCOUNT_A)).toStrictEqual(refreshed); + + // A peer rotated past refresh-0 (here, to refresh-peer): its tokens are kept. + await store.writeSession(rotated); + expect(await store.commitRefresh(refreshed, 'refresh-0')).toBe('rotated'); + expect(await store.readAccount('staging', ACCOUNT_A)).toStrictEqual(rotated); + + // Logged out: nothing is written back. + expect(await store.removeAccount('staging', ACCOUNT_A)).toStrictEqual({ + removed: true, + current: true, + }); + expect(await store.commitRefresh(refreshed, 'refresh-peer')).toBe('gone'); + expect(await store.readAccount('staging', ACCOUNT_A)).toBeNull(); + expect(await readdir(store.dir)).toStrictEqual([]); + }); + + it('a peer rotation that lands while a commit waits on the lock is kept', async () => { + const store = createOAuthStore(join(root, 'commit-rotated-under-lock'), { lock: patient }); + const rotated = oauthSession({ accessToken: 'access-peer', refreshToken: 'refresh-peer' }); + const waiting = deferred(); + + await signIn(store, oauthSession()); + // A peer's commit holds the lock; ours waits on it. + await writeFile(lockOf(store), ''); + hooks.onCreateTaken = (path) => { + if (path === lockOf(store)) { + waiting.resolve(); + } + }; + + const committing = store.commitRefresh(refreshed, 'refresh-0'); + + await waiting.promise; + // The peer writes its rotation, then releases. + await store.writeSession(rotated); + await rm(lockOf(store)); + + expect(await committing).toBe('rotated'); + expect(await store.readAccount('staging', ACCOUNT_A)).toStrictEqual(rotated); + await noLockFiles(store); + }); + + it('no lock file remains after a commit or a logout, whether it succeeds or throws', async () => { + const store = createOAuthStore(join(root, 'lock-cleanup')); + + await signIn(store, oauthSession()); + expect(await store.commitRefresh(refreshed, 'refresh-0')).toBe('written'); + await noLockFiles(store); + + // The write throws once the temp file is open (as in the failed-write test above). + const poisoned = oauthSession({ + accessToken: { + toJSON: () => { + throw new Error('disk on fire'); + }, + } as unknown as string, + }); + + await expect(store.commitRefresh(poisoned, 'refresh-1')).rejects.toThrow('disk on fire'); + expect(await store.readAccount('staging', ACCOUNT_A)).toStrictEqual(refreshed); + await noLockFiles(store); + + // A logout whose unlink of the account file fails. + hooks.failUnlink = { + matches: (path) => path === store.accountPath('staging', ACCOUNT_A), + error: Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }), + }; + await expect(store.signOut('staging')).rejects.toThrow('EACCES'); + await noLockFiles(store); + + await store.setCurrent('staging', ACCOUNT_A); + await store.writeSession(oauthSession({ ...ACCOUNT_B })); + expect(await store.removeAccount('staging', ACCOUNT_B)).toStrictEqual({ + removed: true, + current: false, + }); + await noLockFiles(store); + expect(await store.deleteSession('staging', ACCOUNT_A)).toBe(true); + await noLockFiles(store); + await signIn(store, oauthSession()); + expect(await store.signOutAll('staging')).toStrictEqual({ signedIn: true, removed: 1 }); + expect(await store.signOut('staging')).toBe(false); + expect(await readdir(store.dir)).toStrictEqual([]); + }); + + it("a lock held past the wait doesn't stop a commit or a logout, and isn't removed by them", async () => { + const store = createOAuthStore(join(root, 'lock-timeout'), { + lock: { waitMs: 50, pollMs: 5 }, + }); + const foreign = lockOf(store); + + await signIn(store, oauthSession()); + // A live holder (fresh mtime) that never releases within the wait. + await writeFile(foreign, ''); + + expect(await store.commitRefresh(refreshed, 'refresh-0')).toBe('written'); + expect(await store.readAccount('staging', ACCOUNT_A)).toStrictEqual(refreshed); + expect(await store.signOut('staging')).toBe(true); + expect(await store.readAccount('staging', ACCOUNT_A)).toBeNull(); + + // Only the holder's own lock is left: nothing of ours. + expect(await readdir(store.dir)).toStrictEqual([`staging.${accountKey(ACCOUNT_A)}.lock`]); + + const run = vi.fn(() => Promise.resolve('ran')); + + expect( + await withLockFile(foreign, { ...ACCOUNT_LOCK, waitMs: 50, pollMs: 5 }, run), + ).toStrictEqual({ value: 'ran', held: false }); + expect(run).toHaveBeenCalledOnce(); + }); + + it('a stale lock file is taken over at once, not waited out', async () => { + const store = createOAuthStore(join(root, 'lock-stale'), { lock: patient }); + + await signIn(store, oauthSession()); + await writeFile(lockOf(store), ''); + await utimes(lockOf(store), staleTime(), staleTime()); + + const started = Date.now(); + const seen: boolean[] = []; + + const result = await withLockFile(lockOf(store), patient, async () => { + // Held: the file is ours, created fresh. + seen.push(Date.now() - (await stat(lockOf(store))).mtimeMs < ACCOUNT_LOCK.staleMs); + + return 'ran'; + }); + + expect(result).toStrictEqual({ value: 'ran', held: true }); + expect(seen).toStrictEqual([true]); + // `patient` waits a minute for a live lock; a takeover is immediate. + expect(Date.now() - started).toBeLessThan(5_000); + await noLockFiles(store); + + // Through the store too: a commit behind a stale lock writes. + await writeFile(lockOf(store), ''); + await utimes(lockOf(store), staleTime(), staleTime()); + expect(await store.commitRefresh(refreshed, 'refresh-0')).toBe('written'); + await noLockFiles(store); + }); + + it('a live lock is waited for, and the waiter goes ahead holding it once it is released', async () => { + const store = createOAuthStore(join(root, 'lock-wait'), { lock: patient }); + const path = lockOf(store); + const waiting = deferred(); + const run = vi.fn(async () => { + // The waiter's own lock, while it runs. + await stat(path); + + return 'ran'; + }); + + await mkdir(store.dir, { recursive: true }); + await writeFile(path, ''); + + let polls = 0; + + hooks.onCreateTaken = (taken) => { + if (taken === path && (polls += 1) === 3) { + waiting.resolve(); + } + }; + + const locked = withLockFile(path, patient, run); + + // It has found the lock taken three times, and hasn't run. + await waiting.promise; + expect(run).not.toHaveBeenCalled(); + await rm(path); + + expect(await locked).toStrictEqual({ value: 'ran', held: true }); + expect(run).toHaveBeenCalledOnce(); + await expect(stat(path)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it("a holder's release leaves a lock that replaced its own alone, whatever its inode", async () => { + const store = createOAuthStore(join(root, 'lock-successor')); + const path = lockOf(store); + const nonces: string[] = []; + + await mkdir(store.dir, { recursive: true }); + + const result = await withLockFile(path, ACCOUNT_LOCK, async () => { + nonces.push(await readFile(path, 'utf-8')); + // Taken over meanwhile (as stale, after a long pause): another holder's file now, which a + // file system may give the inode just freed. + await rm(path); + await writeFile(path, 'successor'); + + return 'ran'; + }); + + expect(result).toStrictEqual({ value: 'ran', held: true }); + // While held, the file holds this holder's random nonce. + expect(nonces).toHaveLength(1); + expect(nonces[0]).toMatch(/^[0-9a-f]{32}$/); + expect(await readFile(path, 'utf-8')).toBe('successor'); + }); + + it('a release leaves the lock alone when it holds another nonce, even in the same file', async () => { + const store = createOAuthStore(join(root, 'lock-other-nonce')); + const path = lockOf(store); + const other = 'f'.repeat(32); + + await mkdir(store.dir, { recursive: true }); + + const before = await withLockFile(path, ACCOUNT_LOCK, async () => { + const ino = (await stat(path)).ino; + + // Rewritten in place: same inode, so only the content tells it isn't this holder's. + await writeFile(path, other); + + return ino; + }); + + expect((await stat(path)).ino).toBe(before.value); + expect(await readFile(path, 'utf-8')).toBe(other); + + // A release of a file still holding its own nonce removes it. + await rm(path); + expect(await withLockFile(path, ACCOUNT_LOCK, () => Promise.resolve('ran'))).toStrictEqual({ + value: 'ran', + held: true, + }); + await expect(stat(path)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('a run that throws releases the lock and passes the error on', async () => { + const store = createOAuthStore(join(root, 'lock-throw')); + const path = lockOf(store); + + await mkdir(store.dir, { recursive: true }); + await expect( + withLockFile(path, ACCOUNT_LOCK, () => Promise.reject(new Error('boom'))), + ).rejects.toThrow('boom'); + await expect(stat(path)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('listing, login, switching and logout --all work with lock files present, and clean-up removes only stale ones', async () => { + const store = createOAuthStore(join(root, 'lock-present'), { + lock: { waitMs: 50, pollMs: 5 }, + }); + // A stale lock for A, a live one for B, and a stale one in another environment. + const goLock = join(store.dir, `go.${accountKey(ACCOUNT_A)}.lock`); + + await store.writeSession(sessionB()); + await signIn(store, oauthSession()); + await writeFile(lockOf(store, ACCOUNT_A), ''); + await writeFile(lockOf(store, ACCOUNT_B), ''); + await writeFile(goLock, ''); + + for (const path of [lockOf(store, ACCOUNT_A), goLock]) { + await utimes(path, staleTime(), staleTime()); + } + + expect( + (await store.listAccounts('staging')).map(({ account, current }) => ({ account, current })), + ).toStrictEqual([ + { account: ACCOUNT_A, current: true }, + { account: ACCOUNT_B, current: false }, + ]); + expect(await store.listAccounts('go')).toStrictEqual([]); + expect(await store.listSessions()).toStrictEqual(['staging']); + + // A switch cleans up: A's stale lock goes; B's live one and go's (another environment) stay. + await store.setCurrent('staging', ACCOUNT_B); + expect(await store.readSession('staging')).toStrictEqual(sessionB()); + expect( + (await readdir(store.dir)).filter((name) => name.endsWith('.lock')).sort(), + ).toStrictEqual( + [`go.${accountKey(ACCOUNT_A)}.lock`, `staging.${accountKey(ACCOUNT_B)}.lock`].sort(), + ); + + // A login again to A writes past nothing; logout --all counts accounts, not locks. + await signIn(store, oauthSession()); + expect(await store.readSession('staging')).toStrictEqual(oauthSession()); + expect(await store.signOutAll('staging')).toStrictEqual({ signedIn: true, removed: 2 }); + expect((await readdir(store.dir)).sort()).toStrictEqual( + [`go.${accountKey(ACCOUNT_A)}.lock`, `staging.${accountKey(ACCOUNT_B)}.lock`].sort(), + ); + }); +}); diff --git a/src/lib/oauthStore.ts b/src/lib/oauthStore.ts index c085147..c6d111e 100644 --- a/src/lib/oauthStore.ts +++ b/src/lib/oauthStore.ts @@ -6,22 +6,39 @@ * oauth/ 0700 * .json 0600 pointer: `{ "businessId", "userId" }`, the current account * ...json 0600 one saved account's session (tokens and labels) + * ...lock 0600 its holder's random nonce; exists only while held * clients.json 0600 DCR client_id cache, keyed by issuer * * `` and `` are the account's ids, lowercase UUIDs as Kizen issues them. Each * account file is a saved account; the pointer picks the current one. * - * Nothing is locked: every write is an atomic rename. A refresh only ever writes or deletes its - * own account's file, and only login, a switch and logout move the pointer, so a refresh racing a - * logout or a switch to another account can't undo it (see `createTokenProvider`). Accepted: a - * logout racing a sign-in to the same account can delete that sign-in's new session file, leaving - * the account signed out. + * Every write is an atomic rename. Only login, a switch and logout move the pointer, and a refresh + * only ever writes or deletes its own account's file, so it can't undo a logout of another account + * or a switch. Its write and a logout's unlink of that same file are serialized by the account's + * lock (`withLockFile`): the refresh writes only if, under the lock, the file is still there with + * the refresh token it spent (`commitRefresh`), so a refresh that finishes after a logout can't + * bring the account back. The lock is best effort, as in GitHub CLI's `refresh.lock`: a lock not + * acquired within `waitMs` is skipped, leaving that one operation to race as it would unlocked, + * rather than wedging the CLI. Nothing else is locked. Accepted: a logout racing a sign-in to the + * same account can delete that sign-in's new session file, leaving the account signed out. */ import { randomBytes } from 'node:crypto'; -import { chmod, mkdir, open, readdir, readFile, rename, rm, unlink } from 'node:fs/promises'; +import { + chmod, + mkdir, + open, + readdir, + readFile, + rename, + rm, + stat, + unlink, + type FileHandle, +} from 'node:fs/promises'; import { join } from 'node:path'; +import { setTimeout as sleep } from 'node:timers/promises'; import { ENVIRONMENTS, GLOBAL_CREDENTIALS_DIR, type Environment } from './credentials.js'; -import { ifExists, isRecord, nonEmptyString } from './guards.js'; +import { ifExists, isErrnoCode, isRecord, nonEmptyString } from './guards.js'; import { loginRequired, OAuthError } from './oauth.js'; /** Who a session acts as: a token is bound to one Kizen user in one business. */ @@ -71,11 +88,23 @@ export interface OAuthStore { listAccounts: (environment: Environment) => Promise; /** Writes this account's session file only, in place; never moves the pointer. */ writeSession: (session: OAuthSession) => Promise; + /** + * A refresh's write of `session`, under its account's lock: re-reads that account's own file and + * writes only if it's still there holding `spentRefreshToken`, the token the refresh presented. + * `'gone'`: the file was removed (a logout), so nothing is written and the account stays signed + * out. `'rotated'`: a peer stored newer tokens (another refresh, or a sign-in again), which are + * kept. Never moves the pointer. An unreadable file throws as `readAccount` does. + */ + commitRefresh: ( + session: OAuthSession, + spentRefreshToken: string, + ) => Promise<'written' | 'rotated' | 'gone'>; /** Points the environment at `account` (after `writeSession` of it), then cleans up. */ setCurrent: (environment: Environment, account: OAuthAccount) => Promise; /** - * Deletes this account's file only; never touches the pointer, so a pointer to it then reads as - * not signed in. Returns whether the file existed. + * Deletes this account's file only, under its lock (as every account-file delete here is, so a + * `commitRefresh` in progress finishes first and the delete wins); never touches the pointer, so + * a pointer to it then reads as not signed in. Returns whether the file existed. */ deleteSession: (environment: Environment, account: OAuthAccount) => Promise; /** @@ -120,6 +149,127 @@ const CLIENTS_FILE = 'clients.json'; */ export const SIGNOUT_MAX_AGE_MS = 10 * 60_000; +export interface LockFileOptions { + /** How long to wait for a live lock before going ahead without it. */ + waitMs: number; + /** A lock file older than this (by mtime) was left by a crash, and is taken over. */ + staleMs: number; + /** How often a waiter retries. */ + pollMs: number; +} + +/** + * An account lock is held for one read and one fsync'd atomic write, or one unlink: milliseconds. + * `waitMs` covers a slow disk while keeping `auth-header` inside Claude Code's 10s helper limit: a + * refresh takes the lock once (its commit or its delete), after a request of at most 5s. `staleMs` + * is far above any real hold, so only a lock whose holder died (or was stopped, or slept with the + * laptop) mid-hold is taken over; a process that resumes after that races as an unlocked one would. + */ +export const ACCOUNT_LOCK: LockFileOptions = { waitMs: 2_000, staleMs: 30_000, pollMs: 25 }; + +/** + * Creates `path` exclusively and writes a random nonce into it, waiting up to `waitMs` for a live + * holder and taking over a stale file. Resolves to the nonce, which tells this holder's file from + * a successor's on release (an inode number can be reused at once, and isn't reliable on Windows + * or FAT). Undefined when it can't: timed out, or any error but EEXIST (a missing directory too, + * where there's nothing yet to race over). The handle is closed at once, so the file can be + * unlinked on Windows while held. Others only stat a lock, by mtime, so one just created and + * still empty is live like any other. + * + * Best effort: two waiters that both judge one file stale can each remove it, the later removing + * the earlier's new lock, and then both hold it. That's the unlocked race again, for that pair. + */ +async function acquireLockFile( + path: string, + { waitMs, staleMs, pollMs }: LockFileOptions, +): Promise { + const deadline = Date.now() + waitMs; + + for (;;) { + let handle: FileHandle; + + try { + handle = await open(path, 'wx', 0o600); + } catch (err) { + if (!isErrnoCode(err, 'EEXIST')) { + return undefined; + } + + try { + const holder = await ifExists(stat(path)); + + // Released since the open: try again at once. + if (holder === undefined) { + continue; + } + + if (Date.now() - holder.mtimeMs > staleMs) { + await ifExists(unlink(path)); + continue; + } + } catch { + return undefined; + } + + if (Date.now() >= deadline) { + return undefined; + } + + await sleep(pollMs); + continue; + } + + let nonce: string | undefined = randomBytes(16).toString('hex'); + + try { + await handle.writeFile(nonce, 'utf-8'); + } catch { + // Without its nonce the release couldn't tell this file from a successor's. + nonce = undefined; + } + + try { + await handle.close(); + } catch { + nonce = undefined; + } + + if (nonce === undefined) { + await rm(path, { force: true }).catch(() => undefined); + } + + return nonce; + } +} + +/** + * Runs `run` holding the lock file `path` (see `acquireLockFile`), or without it once `waitMs` has + * passed, so a lock never wedges the CLI. `held` says which. The file exists only while held: it's + * removed whether `run` resolves or throws, and only if it still holds this call's nonce. + */ +export async function withLockFile( + path: string, + options: LockFileOptions, + run: () => Promise, +): Promise<{ value: T; held: boolean }> { + const lock = await acquireLockFile(path, options); + + try { + return { value: await run(), held: lock !== undefined }; + } finally { + if (lock !== undefined) { + try { + // Another nonce: taken over as stale meanwhile, so that file is the new holder's. + if ((await ifExists(readFile(path, 'utf-8'))) === lock) { + await ifExists(unlink(path)); + } + } catch { + // Best effort: a lock left behind is taken over once stale, or removed by `cleanUp`. + } + } + } +} + const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; const validId = (value: unknown): value is string => typeof value === 'string' && UUID.test(value); @@ -190,12 +340,31 @@ function parseSession(raw: unknown, environment: Environment): OAuthSession | nu }; } +export interface OAuthStoreOptions { + /** The account lock's timings; `ACCOUNT_LOCK` by default. */ + lock?: Partial; +} + /** @param baseDir the credentials directory; `oauth/` is created inside it. */ -export function createOAuthStore(baseDir: string = GLOBAL_CREDENTIALS_DIR): OAuthStore { +export function createOAuthStore( + baseDir: string = GLOBAL_CREDENTIALS_DIR, + options: OAuthStoreOptions = {}, +): OAuthStore { const dir = join(baseDir, 'oauth'); + const lockOptions: LockFileOptions = { ...ACCOUNT_LOCK, ...options.lock }; const sessionPath = (environment: Environment): string => join(dir, `${environment}.json`); const accountPath = (environment: Environment, account: OAuthAccount): string => join(dir, `${environment}.${accountKey(account)}.json`); + /** A stable name beside the account file, which is replaced by rename on every write. */ + const lockPath = (environment: Environment, account: OAuthAccount): string => + join(dir, `${environment}.${accountKey(account)}.lock`); + + /** `run` under the account's lock, or without it after `waitMs` (see `withLockFile`). */ + const underAccountLock = async ( + environment: Environment, + account: OAuthAccount, + run: () => Promise, + ): Promise => (await withLockFile(lockPath(environment, account), lockOptions, run)).value; const clientsPath = join(dir, CLIENTS_FILE); /** The account a file name in `dir` holds, for this environment; undefined for anything else. */ @@ -256,7 +425,8 @@ export function createOAuthStore(baseDir: string = GLOBAL_CREDENTIALS_DIR): OAut /** * Best effort, on login, switch and logout: deletes `.json.-.signout` pointers a * crash left mid-logout once they're old enough, by the time in their name (when the logout - * moved them aside). Only names matching that fixed pattern reach `join`. + * moved them aside), and account locks a crash left, once stale by their mtime (a lock is never + * renamed). Only names matching those fixed patterns reach `join`. */ const cleanUp = async (environment: Environment): Promise => { try { @@ -268,6 +438,20 @@ export function createOAuthStore(baseDir: string = GLOBAL_CREDENTIALS_DIR): OAut if (movedAt !== undefined && Date.now() - Number(movedAt) > SIGNOUT_MAX_AGE_MS) { await ifExists(unlink(join(dir, name))); } + + const locked = name.endsWith('.lock') + ? accountOfName(environment, `${name.slice(0, -'.lock'.length)}.json`) + : undefined; + + if (locked !== undefined) { + const path = lockPath(environment, locked); + const lock = await ifExists(stat(path)); + + // Read-then-unlink, as in a stale takeover; a lock this old has no live holder. + if (lock !== undefined && Date.now() - lock.mtimeMs > lockOptions.staleMs) { + await ifExists(unlink(path)); + } + } } } catch { // Best effort: a later login or logout cleans up again. @@ -377,9 +561,17 @@ export function createOAuthStore(baseDir: string = GLOBAL_CREDENTIALS_DIR): OAut } }; - /** Unlinks one account file; a failure other than "already gone" surfaces. */ - const unlinkAccount = async (environment: Environment, account: OAuthAccount): Promise => - (await ifExists(unlink(accountPath(environment, account)).then(() => true))) ?? false; + /** + * Unlinks one account file, under its lock so a `commitRefresh` in progress can't recreate it; + * a failure other than "already gone" surfaces. Every logout's delete comes through here. + */ + const unlinkAccount = (environment: Environment, account: OAuthAccount): Promise => + underAccountLock( + environment, + account, + async () => + (await ifExists(unlink(accountPath(environment, account)).then(() => true))) ?? false, + ); return { dir, @@ -442,6 +634,26 @@ export function createOAuthStore(baseDir: string = GLOBAL_CREDENTIALS_DIR): OAut await writeAtomic(accountPath(session.environment, session), session); }, + async commitRefresh(session, spentRefreshToken) { + const { environment } = session; + + return underAccountLock(environment, session, async () => { + const stored = await readAccount(environment, session); + + if (stored === null) { + return 'gone'; + } + + if (stored.refreshToken !== spentRefreshToken) { + return 'rotated'; + } + + await writeAtomic(accountPath(environment, session), session); + + return 'written'; + }); + }, + async setCurrent(environment, account) { const { businessId, userId } = account; diff --git a/src/lib/pushHeadless.test.ts b/src/lib/pushHeadless.test.ts index 0de1884..5863d8e 100644 --- a/src/lib/pushHeadless.test.ts +++ b/src/lib/pushHeadless.test.ts @@ -2634,6 +2634,109 @@ describe('naming a saved account (--business / --user, APPBUILDER_BUSINESS / APP `appbuilder block push ${BLOCK} --env staging --business Bravo --user ${ACCOUNT_B.userId}`, ); }); + + describe('the printed commands name the requested account as flags', () => { + const A_USER = 'a1a1a1a1-a1a1-4a1a-8a1a-a1a1a1a1a1a1'; + const run = ( + command: 'push' | 'targets', + flags: PushCommandOptions, + deps: PushDeps, + ): Promise => + command === 'push' ? runPushHeadless(BLOCK, flags, deps) : runTargetsHeadless(flags, deps); + + // A one-command `APPBUILDER_BUSINESS=… appbuilder …` doesn't persist, so a copied command + // needs the account as flags. + it.each<[string, 'push' | 'targets', Record, PushCommandOptions, string]>([ + [ + 'push after APPBUILDER_BUSINESS', + 'push', + { APPBUILDER_BUSINESS: 'Bravo' }, + {}, + `appbuilder block push ${BLOCK} --env staging --business Bravo\n`, + ], + [ + 'push after APPBUILDER_BUSINESS and APPBUILDER_USER', + 'push', + { APPBUILDER_BUSINESS: 'Bravo', APPBUILDER_USER: ACCOUNT_B.userId }, + {}, + `appbuilder block push ${BLOCK} --env staging --business Bravo --user ${ACCOUNT_B.userId}\n`, + ], + [ + 'push with --business over the variables', + 'push', + { APPBUILDER_BUSINESS: 'Acme', APPBUILDER_USER: A_USER }, + { business: 'Bravo' }, + `appbuilder block push ${BLOCK} --env staging --business Bravo\n`, + ], + [ + 'targets after APPBUILDER_BUSINESS', + 'targets', + { APPBUILDER_BUSINESS: 'Bravo' }, + {}, + 'appbuilder block targets --env staging --business Bravo\n', + ], + [ + 'targets after APPBUILDER_BUSINESS and APPBUILDER_USER', + 'targets', + { APPBUILDER_BUSINESS: 'Bravo', APPBUILDER_USER: ACCOUNT_B.userId }, + {}, + `appbuilder block targets --env staging --business Bravo --user ${ACCOUNT_B.userId}\n`, + ], + [ + 'targets with --business over the variables', + 'targets', + { APPBUILDER_BUSINESS: 'Acme', APPBUILDER_USER: A_USER }, + { business: 'Bravo' }, + 'appbuilder block targets --env staging --business Bravo\n', + ], + ])('environment choice: %s', async (_name, command, processEnv, flags, expected) => { + const oauth = oauthDeps(['staging', 'integration']); + + // Saved in both environments. + oauth.listAccounts.mockImplementation((environment: Environment) => + Promise.resolve(environment === 'staging' || environment === 'integration' ? ACCOUNTS : []), + ); + + const harness = makeHarness({ deps: { oauth, processEnv } }); + + await run(command, flags, harness.deps); + + const err = harness.stderr.join(''); + + expect(err).toContain('Choices:'); + expect(err).toContain(`staging → ${expected}`); + // Business and user come from one source: the variables' Acme user never joins Bravo. + expect(err).not.toContain('Acme'); + expect(err).not.toContain(A_USER); + expect(oauth.connect).not.toHaveBeenCalled(); + }); + + it('a dashboard choice after APPBUILDER_BUSINESS and APPBUILDER_USER', async () => { + const fake = makeClient({ + summaries: { generic_dashboard: [summary()] }, + dashboards: [makeDashboard({ dashlets: [customCodeDashlet('X1', { name: 'other' })] })], + }); + const oauth = oauthDeps(['staging'], fake); + + oauth.listAccounts.mockImplementation((environment: Environment) => + Promise.resolve(environment === 'staging' ? ACCOUNTS : []), + ); + + const harness = makeHarness({ + fake, + deps: { + oauth, + processEnv: { APPBUILDER_BUSINESS: 'Bravo', APPBUILDER_USER: ACCOUNT_B.userId }, + }, + }); + + await runPushHeadless(BLOCK, {}, harness.deps); + + expect(harness.stderr.join('')).toContain( + `Sales → appbuilder block push ${BLOCK} --business Bravo --user ${ACCOUNT_B.userId} --dashboard D1\n`, + ); + }); + }); }); describe('the sign-in session is the default', () => { diff --git a/src/lib/pushHeadless.ts b/src/lib/pushHeadless.ts index c51050d..120be83 100644 --- a/src/lib/pushHeadless.ts +++ b/src/lib/pushHeadless.ts @@ -58,6 +58,7 @@ import { type TargetMatch, } from './pushBlock.js'; import { + accountRequest, notSignedInWithApiKeyHint, pinnedProfileNotice, selectAuth, @@ -1230,13 +1231,25 @@ function writeResult(output: Output, result: unknown, text: string, jsonText = t } } -/** `--business` / `--user` as passed, for the commands a choice prints. */ -const accountArgs = (options: AuthFlags): string[] => [ - ...(options.business === undefined ? [] : ['--business', options.business]), - ...(options.user === undefined ? [] : ['--user', options.user]), -]; +/** + * The account the run asked for (`--business` / `--user`, else `APPBUILDER_BUSINESS` / + * `APPBUILDER_USER`), as flags for the commands a choice prints: a one-command variable + * assignment doesn't survive being copied. Empty when no account was named, or the request is + * invalid (that run already failed with the usage error). + */ +const accountArgs = (options: AuthFlags, processEnv: PushDeps['processEnv']): string[] => { + const result = accountRequest(options, processEnv); + + if (!result.ok || result.request === undefined) { + return []; + } -const credentialArgs = (options: AuthFlags): string[] => { + const { business, user } = result.request; + + return ['--business', business, ...(user === undefined ? [] : ['--user', user])]; +}; + +const credentialArgs = (options: AuthFlags, processEnv: PushDeps['processEnv']): string[] => { if (options.credentials !== undefined) { return ['-c', options.credentials]; } @@ -1245,7 +1258,10 @@ const credentialArgs = (options: AuthFlags): string[] => { return ['--profile', options.profile]; } - return [...(options.env === undefined ? [] : ['--env', options.env]), ...accountArgs(options)]; + return [ + ...(options.env === undefined ? [] : ['--env', options.env]), + ...accountArgs(options, processEnv), + ]; }; const writeNotices = (deps: PushDeps, notices: readonly string[]): void => { @@ -1272,14 +1288,14 @@ export async function runPushHeadless( let blockArgs = apiName === undefined ? [] : [apiName]; const choiceArgs: ChoiceArgs = (choice, kind) => { if (kind === 'environment') { - return [...blockArgs, ...choice.args, ...accountArgs(options)]; + return [...blockArgs, ...choice.args, ...accountArgs(options, deps.processEnv)]; } if (kind === 'block') { - return [...choice.args, ...credentialArgs(options)]; + return [...choice.args, ...credentialArgs(options, deps.processEnv)]; } - return [...blockArgs, ...credentialArgs(options), ...choice.args]; + return [...blockArgs, ...credentialArgs(options, deps.processEnv), ...choice.args]; }; const fail = (result: PushFailure): void => { @@ -1512,7 +1528,7 @@ export async function runTargetsHeadless( const objectArgs = options.object === undefined ? [] : ['--object', options.object]; const choiceArgs: ChoiceArgs = (choice) => [ ...choice.args, - ...accountArgs(options), + ...accountArgs(options, deps.processEnv), ...objectArgs, ]; const fail = (result: PushFailure): void => { diff --git a/src/test/oauth.ts b/src/test/oauth.ts index 8ef60a3..84dd51f 100644 --- a/src/test/oauth.ts +++ b/src/test/oauth.ts @@ -119,12 +119,13 @@ export interface HeldStore { } /** - * `store` with `method` held: a call waits for `release()` before it runs, so a test can land a - * logout or a switch to another account between a refresh's last check and its mutation. + * `store` with `method` held: a call waits for `release()` before it runs (for `commitRefresh`, + * before it takes the account lock), so a test can land a logout or a switch to another account + * between a refresh's last check and its mutation. */ export const holdingStore = ( store: OAuthStore, - method: 'writeSession' | 'deleteSession', + method: 'commitRefresh' | 'deleteSession', ): HeldStore => { const reached = deferred(); const released = deferred(); @@ -135,12 +136,13 @@ export const holdingStore = ( return { store: - method === 'writeSession' + method === 'commitRefresh' ? { ...store, - writeSession: async (session) => { + commitRefresh: async (session, spentRefreshToken) => { await hold(); - await store.writeSession(session); + + return store.commitRefresh(session, spentRefreshToken); }, } : { From 239931495e6f3b3b437d4cc30d122602603c3f37 Mon Sep 17 00:00:00 2001 From: Keegan Donley Date: Mon, 26 Oct 2026 15:37:56 -0500 Subject: [PATCH 3/3] KZN-19166 - add new --mcp option for setup --- README.md | 29 +-- src/__snapshots__/program.test.ts.snap | 4 +- src/commands/setupClaude.ts | 9 +- src/lib/setupClaude.test.ts | 246 +++++++++++++++++++++++-- src/lib/setupClaude.ts | 33 ++-- src/program.test.ts | 3 +- 6 files changed, 283 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 9e444d6..345ddce 100644 --- a/README.md +++ b/README.md @@ -122,25 +122,26 @@ As with `create`, the workflow only runs once it has reached the repository's de ### `appbuilder setup-claude` -Installs or refreshes the Claude Code skill for building custom blocks (`.claude/skills/kizen-custom-block/SKILL.md`) in the plugin in the current directory. `appbuilder create` writes the skill once; run `setup-claude` in an existing plugin to add it, or after upgrading the CLI to pick up the bundled version. +Installs or refreshes the Claude Code skill for building custom blocks (`.claude/skills/kizen-custom-block/SKILL.md`) in the plugin in the current directory. `appbuilder create` writes the skill once; run `setup-claude` in an existing plugin to add it, or after upgrading the CLI to pick up the bundled version. With `--mcp` it only registers the Kizen MCP server instead and writes no files, so it can run in any directory (see below). ```sh -appbuilder setup-claude [--dry-run] [--include-lib] [--mcp [--env ] [--force]] +appbuilder setup-claude [--dry-run] [--include-lib] +appbuilder setup-claude --mcp [--env ] [--force] [--dry-run] ``` -| Flag | Default | Purpose | -| --------------- | ------- | ------------------------------------------------------------ | -| `--dry-run` | off | Report what would change without writing files. | -| `--include-lib` | off | Also install the Kizen data helper library (`kizenData.js`). | -| `--mcp` | off | Also register the Kizen MCP server in Claude Code. | -| `--env ` | `go` | Environment of the MCP server to register. | -| `--force` | off | Replace an existing registration of that server. | +| Flag | Default | Purpose | +| --------------- | ------- | -------------------------------------------------------------------------------------------------------------- | +| `--dry-run` | off | Report what would change without writing files. | +| `--include-lib` | off | Also install the Kizen data helper library (`kizenData.js`). | +| `--mcp` | off | Only register the Kizen MCP server in Claude Code (user scope); writes no files, so it works in any directory. | +| `--env ` | `go` | Environment of the MCP server to register. | +| `--force` | off | Replace an existing registration of that server. | -`--env` and `--force` only apply with `--mcp`; see [Using the session from Claude Code](#using-the-session-from-claude-code-mcp). +`--env` and `--force` only apply with `--mcp`, and `--mcp` can't be combined with `--include-lib`; see [Using the session from Claude Code](#using-the-session-from-claude-code-mcp). It also installs the skill's design guide (`.claude/skills/kizen-custom-block/design.md`), which tells the agent how to make blocks match native dashlets. With `--include-lib`, it installs the Kizen data helper library that block scripts import to read records and apply the dashboard's date and team filters. The library goes under the `entry` directory from `kizen.json` (`/lib/kizenData.js`, one per entry in a multi-plugin `kizen.json`), and under `src/` when `kizen.json` has no usable `entry`. Without the flag, it refreshes only the libraries that already exist. `appbuilder create --include-lib` writes it to `src/lib/kizenData.js`. -It prints one line per file (`created`, `updated` or `unchanged`) and a one-line summary. These files are managed by the CLI, so local edits to them, including edits to `kizenData.js`, are replaced when they're `updated`. Don't edit `kizenData.js`; put your own helpers in another file under `src/lib/`. It only writes these files and never touches the `.github/` Copilot files or block code. It fails with exit code 1 when there's no `kizen.json` in the current directory. +It prints one line per file (`created`, `updated` or `unchanged`) and a one-line summary. These files are managed by the CLI, so local edits to them, including edits to `kizenData.js`, are replaced when they're `updated`. Don't edit `kizenData.js`; put your own helpers in another file under `src/lib/`. It only writes these files and never touches the `.github/` Copilot files or block code. Without `--mcp`, it fails with exit code 1 when there's no `kizen.json` in the current directory. `block push` adds the warning `Claude files managed by appbuilder are out of date; run appbuilder setup-claude` when the plugin has the skill and the skill or its design guide is missing or differs from the bundled one, or an existing `kizenData.js` differs from the bundled one. The warning never fails the push. @@ -508,14 +509,14 @@ A session file that is corrupt or can't be read isn't treated as "not signed in" Kizen's MCP server at `/api/mcp` accepts the same session. As a Claude Code `headersHelper`, `appbuilder auth-header` prints `{"Authorization":"Bearer "}` as one line on stdout, refreshing the token first if it's about to expire. -To set it up, run these from a plugin directory: +To set it up, run these from any directory: ```sh npx @kizenapps/cli setup-claude --mcp --env staging npx @kizenapps/cli login --env staging ``` -`setup-claude --mcp` installs the skill files as usual, then registers the server `kizen-` at user scope by running `claude mcp add-json` (`--dry-run` only prints that command). The helper is pinned to the version of the CLI you ran it with. For CLI version 1.16.0, for example, it's equivalent to this POSIX-shell command: +`setup-claude --mcp` only registers the server `kizen-` at user scope, by running `claude mcp add-json` (`--dry-run` only prints that command). It writes no files, in a plugin directory or anywhere else. The helper is pinned to the version of the CLI you ran it with. For CLI version 1.16.0, for example, it's equivalent to this POSIX-shell command: ```sh claude mcp add-json --scope user kizen-staging '{"type":"http","url":"https://staging.kizen.com/api/mcp","headersHelper":"npx -y @kizenapps/cli@1.16.0 auth-header"}' @@ -525,7 +526,7 @@ claude mcp add-json --scope user kizen-staging '{"type":"http","url":"https://st - `--force` only replaces the user-scope server. A server with the same name at local or project scope (for example, one you added by hand without `--scope user`) takes precedence over it in that project. Remove that one with `claude mcp remove kizen-` (run in the project) if you want the user-scope server there. - If the `claude` command isn't installed, it prints this command for you to run instead, and still succeeds. - If you aren't signed in to that environment yet, it reminds you to run `appbuilder login` (with `--env ` for an environment other than `go`). If the saved session can't be read, it prints that error, with the file's path, instead. Either way the server is still registered. -- If registering fails, the skill files stay installed and the command exits with code 1. +- If registering fails, the command exits with code 1. The helper runs through `npx`, which reuses its cached copy of the CLI (running `setup-claude` through `npx` fills that cache), so it starts in about a second. That works when Claude Code was started from a shell with `node` on `PATH`. If it wasn't (for example, with nvm and Claude Code launched outside a terminal), use absolute paths instead, from `which node` and `which appbuilder`: `"headersHelper":"/absolute/path/to/node /absolute/path/to/appbuilder auth-header"`. `appbuilder` starts with `#!/usr/bin/env node`, so the path to `appbuilder` alone isn't enough. Use the absolute paths too when testing a local or `pnpm link` build whose version isn't published, because `npx` can't fetch that version. diff --git a/src/__snapshots__/program.test.ts.snap b/src/__snapshots__/program.test.ts.snap index 192f0bf..e17500a 100644 --- a/src/__snapshots__/program.test.ts.snap +++ b/src/__snapshots__/program.test.ts.snap @@ -11,8 +11,8 @@ Options: Commands: create [options] Scaffold a new Kizen plugin project - setup-claude [options] Install or refresh the Claude Code skill for building - custom blocks + setup-claude [options] Install or refresh the Claude Code files in a plugin, + or with --mcp only register the Kizen MCP server setup-copilot [options] Add or refresh the Copilot code-review setup in an existing plugin repo build Bundle the plugin app into .kizenapp directory diff --git a/src/commands/setupClaude.ts b/src/commands/setupClaude.ts index 4078cd6..e54b94c 100644 --- a/src/commands/setupClaude.ts +++ b/src/commands/setupClaude.ts @@ -9,13 +9,18 @@ import { export function setupClaudeCommand(program: Command, version: string): void { program .command('setup-claude') - .description('Install or refresh the Claude Code skill for building custom blocks') + .description( + 'Install or refresh the Claude Code files in a plugin, or with --mcp only register the Kizen MCP server', + ) .option('--dry-run', 'report what would change without writing files') .option( '--include-lib', 'also install the Kizen data helper library (/lib/kizenData.js)', ) - .option('--mcp', 'also register the Kizen MCP server in Claude Code (user scope)') + .option( + '--mcp', + 'only register the Kizen MCP server in Claude Code (user scope); writes no files, so it works in any directory', + ) .option('--env ', 'environment of the MCP server to register (with --mcp; default: go)') .option('--force', 'replace an existing Kizen MCP server registration (with --mcp)') .action(async (options: SetupClaudeOptions) => { diff --git a/src/lib/setupClaude.test.ts b/src/lib/setupClaude.test.ts index 7d60405..fbf4b76 100644 --- a/src/lib/setupClaude.test.ts +++ b/src/lib/setupClaude.test.ts @@ -215,7 +215,7 @@ describe('runSetupClaude', () => { await runSetupClaude(dir, {}, deps); expect(errors).toStrictEqual([ - 'Error: kizen.json not found. Run this command from a plugin directory.', + 'Error: kizen.json not found. Run this command from a plugin directory, or pass --mcp to only register the MCP server.', ]); expect(exitCodes).toStrictEqual([1]); expect(logs).toStrictEqual([]); @@ -451,6 +451,14 @@ describe('setup-claude --mcp', () => { const ok = (stdout = ''): CommandResult => ({ code: 0, stdout, stderr: '' }); + /** --mcp never writes: no managed file or .claude directory appears. */ + const expectNoFiles = async (): Promise => { + await expect(stat(skillFile())).rejects.toMatchObject({ code: 'ENOENT' }); + await expect(stat(designFile())).rejects.toMatchObject({ code: 'ENOENT' }); + await expect(stat(libFile())).rejects.toMatchObject({ code: 'ENOENT' }); + await expect(stat(join(dir, '.claude'))).rejects.toMatchObject({ code: 'ENOENT' }); + }; + interface FakeClaude { run: ReturnType>; calls: () => string[][]; @@ -556,11 +564,11 @@ describe('setup-claude --mcp', () => { expect(claude.calls()).toStrictEqual([ADD_STAGING]); expect(JSON.parse(claude.calls()[0]?.[5] ?? '')).toStrictEqual(STAGING_CONFIG); - // The skill files were installed first. - expect(await readSkill()).toBe(BUNDLED); - expect(logs.at(-1)).toBe( + // Only the registration runs; nothing is written in the plugin. + await expectNoFiles(); + expect(logs).toStrictEqual([ 'Registered the MCP server kizen-staging (https://staging.kizen.com/api/mcp) in Claude Code at user scope.', - ); + ]); expect(errors).toStrictEqual([]); expect(exitCodes).toStrictEqual([]); }); @@ -631,7 +639,7 @@ describe('setup-claude --mcp', () => { "Error: couldn't register the MCP server kizen-staging: claude mcp remove --scope user exited 1: EACCES: permission denied, open ~/.claude.json", ]); expect(logs.some((line) => line.includes('Registered'))).toBe(false); - expect(await readSkill()).toBe(BUNDLED); + await expectNoFiles(); }); it('--force reports when the re-add fails after removing the old server', async () => { @@ -679,7 +687,7 @@ describe('setup-claude --mcp', () => { ]); expect(errors).toStrictEqual([]); expect(exitCodes).toStrictEqual([]); - expect(await readSkill()).toBe(BUNDLED); + await expectNoFiles(); }); it('setup-claude --mcp --dry-run runs nothing and prints the command', async () => { @@ -736,6 +744,223 @@ describe('setup-claude --mcp', () => { expect(signedIn.logs.some((line) => line.includes('appbuilder login'))).toBe(false); }); + it('--mcp in a plugin leaves out-of-date managed files untouched', async () => { + await writeFile(join(dir, 'kizen.json'), JSON.stringify({ entry: 'src' }), 'utf-8'); + await writeSkill('edited locally\n'); + await writeDesign('edited locally\n'); + await writeLib('export const mine = 1;\n'); + + for (const options of [ + { mcp: true, env: 'staging' }, + { mcp: true, env: 'staging', force: true }, + { mcp: true, env: 'staging', dryRun: true }, + ]) { + const claude = fakeClaude({ existsAt: 'user' }); + const fs = spyFs(); + const { deps, logs, exitCodes } = makeDeps(fs, { run: claude.run }); + + await runSetupClaude(dir, options, deps); + + expect(fs.writeFile).not.toHaveBeenCalled(); + expect( + logs.some((line) => + /^(created|updated|unchanged) |Claude files|nothing written/.test(line), + ), + ).toBe(false); + expect(exitCodes).toStrictEqual([]); + } + + expect(await readSkill()).toBe('edited locally\n'); + expect(await readDesign()).toBe('edited locally\n'); + expect(await readLib()).toBe('export const mine = 1;\n'); + }); + + it('--mcp with --include-lib is a usage error, in or out of a plugin', async () => { + for (const plugin of [false, true]) { + if (plugin) { + await inPlugin(); + } + + for (const options of [ + { mcp: true, includeLib: true }, + { mcp: true, includeLib: true, dryRun: true, env: 'staging' }, + ]) { + const claude = fakeClaude(); + const hasOAuthSession = vi.fn(() => Promise.resolve(true)); + const fs = spyFs(); + const { deps, logs, errors, exitCodes } = makeDeps(fs, { + run: claude.run, + hasOAuthSession, + }); + + await runSetupClaude(dir, options, deps); + + expect(errors).toStrictEqual([ + "Error: --mcp and --include-lib can't be combined. Run `appbuilder setup-claude --include-lib` and `appbuilder setup-claude --mcp` separately.", + ]); + expect(exitCodes).toStrictEqual([1]); + expect(logs).toStrictEqual([]); + expect(claude.run).not.toHaveBeenCalled(); + expect(hasOAuthSession).not.toHaveBeenCalled(); + expect(fs.writeFile).not.toHaveBeenCalled(); + await expectNoFiles(); + } + } + }); + + describe('without kizen.json', () => { + it('--mcp registers the server only, writing no files, and exits 0', async () => { + const claude = fakeClaude(); + const fs = spyFs(); + const { deps, logs, errors, exitCodes } = makeDeps(fs, { run: claude.run }); + + await runSetupClaude(dir, { mcp: true, env: 'staging' }, deps); + + expect(claude.calls()).toStrictEqual([ADD_STAGING]); + expect(logs).toStrictEqual([ + 'Registered the MCP server kizen-staging (https://staging.kizen.com/api/mcp) in Claude Code at user scope.', + ]); + expect(errors).toStrictEqual([]); + expect(exitCodes).toStrictEqual([]); + expect(fs.writeFile).not.toHaveBeenCalled(); + await expectNoFiles(); + }); + + it('--mcp registers go by default', async () => { + const claude = fakeClaude(); + + await runSetupClaude(dir, { mcp: true }, makeDeps(nodeClaudeFs, { run: claude.run }).deps); + + expect(claude.calls()[0]?.[4]).toBe('kizen-go'); + }); + + it('--mcp --dry-run runs nothing, writes nothing and prints the command', async () => { + const claude = fakeClaude(); + const hasOAuthSession = vi.fn(() => Promise.resolve(false)); + const { deps, logs, errors, exitCodes } = makeDeps(nodeClaudeFs, { + run: claude.run, + hasOAuthSession, + }); + + await runSetupClaude(dir, { mcp: true, env: 'staging', dryRun: true }, deps); + + expect(claude.run).not.toHaveBeenCalled(); + expect(hasOAuthSession).not.toHaveBeenCalled(); + expect(logs).toStrictEqual([ + `Dry run: would register the MCP server kizen-staging with: claude mcp add-json --scope user kizen-staging '${JSON.stringify(STAGING_CONFIG)}'`, + ]); + expect(errors).toStrictEqual([]); + expect(exitCodes).toStrictEqual([]); + await expectNoFiles(); + }); + + it('--mcp leaves an existing server alone without --force and replaces it with --force', async () => { + const kept = fakeClaude({ existsAt: 'user' }); + const keep = makeDeps(nodeClaudeFs, { run: kept.run }); + + await runSetupClaude(dir, { mcp: true, env: 'staging' }, keep.deps); + + expect(kept.calls()).toStrictEqual([ADD_STAGING]); + expect(keep.logs.at(-1)).toBe( + 'MCP server kizen-staging already exists in Claude Code; left unchanged. Pass --force to replace it.', + ); + expect(keep.exitCodes).toStrictEqual([]); + + const replaced = fakeClaude({ existsAt: 'user' }); + const replace = makeDeps(nodeClaudeFs, { run: replaced.run }); + + await runSetupClaude(dir, { mcp: true, env: 'staging', force: true }, replace.deps); + + expect(replaced.calls()).toStrictEqual([ADD_STAGING, REMOVE_STAGING, ADD_STAGING]); + expect(replace.logs.at(-1)).toContain('Registered the MCP server kizen-staging'); + expect(replace.exitCodes).toStrictEqual([]); + await expectNoFiles(); + }); + + it('--mcp prints the command when claude is not installed and still exits 0', async () => { + const claude = fakeClaude({ notInstalled: true }); + const { deps, logs, errors, exitCodes } = makeDeps(nodeClaudeFs, { run: claude.run }); + + await runSetupClaude(dir, { mcp: true, env: 'staging' }, deps); + + expect(logs).toStrictEqual([ + "Claude Code's `claude` command wasn't found. To register the MCP server, run (in a POSIX shell such as bash or zsh):", + ` claude mcp add-json --scope user kizen-staging '${JSON.stringify(STAGING_CONFIG)}'`, + ]); + expect(errors).toStrictEqual([]); + expect(exitCodes).toStrictEqual([]); + }); + + it('--mcp hints login when no session exists for the env', async () => { + const signedOut = vi.fn(() => Promise.resolve(false)); + const { deps, logs, exitCodes } = makeDeps(nodeClaudeFs, { + run: fakeClaude().run, + hasOAuthSession: signedOut, + }); + + await runSetupClaude(dir, { mcp: true, env: 'integration' }, deps); + + expect(signedOut).toHaveBeenCalledWith('integration'); + expect(logs.at(-1)).toBe( + "You're not signed in to integration yet. Run `appbuilder login --env integration` before using kizen-integration.", + ); + expect(exitCodes).toStrictEqual([]); + }); + + it('--mcp sets exit 1 when registering fails', async () => { + const claude = fakeClaude({ add: { code: 1, stdout: '', stderr: 'Invalid configuration' } }); + const { deps, errors, exitCodes } = makeDeps(nodeClaudeFs, { run: claude.run }); + + await runSetupClaude(dir, { mcp: true, env: 'staging' }, deps); + + expect(errors).toStrictEqual([ + "Error: couldn't register the MCP server kizen-staging: claude mcp add-json exited 1: Invalid configuration", + ]); + expect(exitCodes).toStrictEqual([1]); + }); + + it('--include-lib without --mcp fails before any side effect', async () => { + const claude = fakeClaude(); + const fs = spyFs(); + const { deps, logs, errors, exitCodes } = makeDeps(fs, { run: claude.run }); + + await runSetupClaude(dir, { includeLib: true }, deps); + + expect(errors).toStrictEqual([ + 'Error: kizen.json not found. Run this command from a plugin directory, or pass --mcp to only register the MCP server.', + ]); + expect(exitCodes).toStrictEqual([1]); + expect(logs).toStrictEqual([]); + expect(claude.run).not.toHaveBeenCalled(); + expect(fs.writeFile).not.toHaveBeenCalled(); + await expectNoFiles(); + }); + + it('without --mcp fails with exit 1, runs nothing and writes nothing', async () => { + const claude = fakeClaude(); + const { deps, logs, errors, exitCodes } = makeDeps(nodeClaudeFs, { run: claude.run }); + + await runSetupClaude(dir, { dryRun: true }, deps); + + expect(errors).toStrictEqual([ + 'Error: kizen.json not found. Run this command from a plugin directory, or pass --mcp to only register the MCP server.', + ]); + expect(exitCodes).toStrictEqual([1]); + expect(logs).toStrictEqual([]); + expect(claude.run).not.toHaveBeenCalled(); + await expectNoFiles(); + }); + + it('still rejects --env without --mcp as a usage error first', async () => { + const { deps, errors, exitCodes } = makeDeps(); + + await runSetupClaude(dir, { env: 'staging' }, deps); + + expect(errors).toStrictEqual(['Error: --env and --force only apply with --mcp.']); + expect(exitCodes).toStrictEqual([1]); + }); + }); + it('--env or --force without --mcp is a usage error', async () => { await inPlugin(); @@ -767,7 +992,7 @@ describe('setup-claude --mcp', () => { await expect(stat(skillFile())).rejects.toMatchObject({ code: 'ENOENT' }); }); - it('mcp step failure keeps installed files and sets exit 1', async () => { + it('mcp step failure sets exit 1 and writes nothing', async () => { await inPlugin(); // Even with --force: an add-json failure that isn't "already exists" never leads to remove. @@ -784,9 +1009,8 @@ describe('setup-claude --mcp', () => { "Error: couldn't register the MCP server kizen-staging: claude mcp add-json exited 1: Invalid configuration something went wrong", ]); expect(first.logs.some((line) => line.includes('left unchanged'))).toBe(false); - expect(await readSkill()).toBe(BUNDLED); - expect(await readDesign()).toBe(BUNDLED_DESIGN); expect(first.exitCodes).toStrictEqual([1]); + await expectNoFiles(); // A spawn error other than ENOENT is a failure too, not the "not installed" path. const broken = makeDeps(nodeClaudeFs, { @@ -797,7 +1021,7 @@ describe('setup-claude --mcp', () => { await runSetupClaude(dir, { mcp: true, env: 'staging' }, broken.deps); expect(broken.exitCodes).toStrictEqual([1]); expect(broken.errors[0]).toContain('EACCES'); - expect(await readSkill()).toBe(BUNDLED); + await expectNoFiles(); }); }); diff --git a/src/lib/setupClaude.ts b/src/lib/setupClaude.ts index 29753cc..a4e93af 100644 --- a/src/lib/setupClaude.ts +++ b/src/lib/setupClaude.ts @@ -228,6 +228,10 @@ function usageProblem(options: SetupClaudeOptions): string | undefined { return '--env and --force only apply with --mcp.'; } + if (options.mcp === true && options.includeLib === true) { + return "--mcp and --include-lib can't be combined. Run `appbuilder setup-claude --include-lib` and `appbuilder setup-claude --mcp` separately."; + } + return options.env === undefined ? undefined : parseEnvironment(options.env).message; } @@ -354,14 +358,30 @@ export async function runSetupClaude( return; } + const dryRun = options.dryRun === true; + + // --mcp only registers the user-scope server and never writes files, in or out of a plugin, + // so running it from any directory can't rewrite a repo's managed files. + if (options.mcp === true) { + await setupMcpServer( + (options.env === undefined ? undefined : parseEnvironment(options.env).environment) ?? + DEFAULT_LOGIN_ENVIRONMENT, + { dryRun, force: options.force === true }, + deps, + ); + + return; + } + if (!(await deps.fs.exists(join(dir, 'kizen.json')))) { - deps.error('Error: kizen.json not found. Run this command from a plugin directory.'); + deps.error( + 'Error: kizen.json not found. Run this command from a plugin directory, or pass --mcp to only register the MCP server.', + ); deps.setExitCode(1); return; } - const dryRun = options.dryRun === true; let plan: ManagedFilePlan[]; try { @@ -390,13 +410,4 @@ export async function runSetupClaude( ? `Dry run: ${counts}; nothing written (${managed}).` : `Claude files: ${counts} (${managed}).`, ); - - if (options.mcp === true) { - await setupMcpServer( - (options.env === undefined ? undefined : parseEnvironment(options.env).environment) ?? - DEFAULT_LOGIN_ENVIRONMENT, - { dryRun, force: options.force === true }, - deps, - ); - } } diff --git a/src/program.test.ts b/src/program.test.ts index 8ff511a..a8dc576 100644 --- a/src/program.test.ts +++ b/src/program.test.ts @@ -111,7 +111,8 @@ const COMMAND_NAMES = [ /** Every command's `--help` blurb, verbatim. */ const COMMAND_DESCRIPTIONS: Record = { create: 'Scaffold a new Kizen plugin project', - 'setup-claude': 'Install or refresh the Claude Code skill for building custom blocks', + 'setup-claude': + 'Install or refresh the Claude Code files in a plugin, or with --mcp only register the Kizen MCP server', 'setup-copilot': 'Add or refresh the Copilot code-review setup in an existing plugin repo', build: 'Bundle the plugin app into .kizenapp directory', dev: 'Start the plugin viewer dev server',