diff --git a/CLAUDE.md b/CLAUDE.md index 0f716d83..6c55e6bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,7 +10,7 @@ Single-crate thin client wrapping Jira REST API v3 and Agile REST API directly w src/ ├── main.rs # Entry point, tokio runtime, clap dispatch, Ctrl+C handling ├── cli/ # Clap derive definitions + command handlers -│ ├── mod.rs # CLI enums, global flags (--output, --project, --no-input, --no-color) +│ ├── mod.rs # CLI enums, global flags (--output, --project, --profile, --no-input, --no-color) │ ├── issue/ # issue commands (split by operation theme) │ │ ├── mod.rs # dispatch + re-exports │ │ ├── format.rs # row formatting, headers, points display @@ -26,13 +26,13 @@ src/ │ ├── worklog.rs # worklog add/list │ ├── team.rs # team list (with cache + lazy org discovery) │ ├── user.rs # user search/list/view (thin wrapper over api/jira/users.rs) -│ ├── auth.rs # auth login (API token default, --oauth for OAuth 2.0), auth status +│ ├── auth.rs # auth login/switch/list/status/refresh/logout/remove. Multi-profile aware via --profile. │ ├── init.rs # Interactive setup (prefetches org metadata + team cache + story points field) │ ├── project.rs # project fields (types, priorities, statuses, CMDB fields) │ └── queue.rs # queue list/view (JSM service desks) ├── api/ │ ├── client.rs # JiraClient — HTTP methods, auth headers, rate limit retry, 429/401 handling -│ ├── auth.rs # OAuth 2.0 flow, API token storage, keychain read/write, token refresh +│ ├── auth.rs # OAuth 2.0 flow + per-profile keychain layout (shared email/api-token/oauth_client_*; namespaced :oauth-access-token / :oauth-refresh-token); lazy migration of legacy flat OAuth keys for the "default" profile │ ├── pagination.rs # Offset-based (most endpoints) + cursor-based (JQL search) │ ├── rate_limit.rs # Retry-After parsing │ ├── assets/ # Assets/CMDB API call implementations @@ -57,8 +57,8 @@ src/ ├── types/assets/ # Serde structs for Assets API responses (AssetObject, ConnectedTicket, LinkedAsset, etc.) ├── types/jira/ # Serde structs for API responses (Issue, Board, Sprint, User, Team, etc.) ├── types/jsm/ # Serde structs for JSM API responses (ServiceDesk, Queue, etc.) -├── cache.rs # XDG cache (~/.cache/jr/) — team list, project meta, workspace ID (all 7-day TTL) -├── config.rs # Global (~/.config/jr/config.toml) + per-project (.jr.toml), figment layering +├── cache.rs # Per-profile XDG cache (~/.cache/jr/v1//) — team list, project meta, workspace ID, CMDB fields, object-type attrs, resolutions (all 7-day TTL). Versioned root (`v1/`) lets a future schema bump orphan stale files cleanly. +├── config.rs # Global (~/.config/jr/config.toml) [profiles.] + default_profile + per-project (.jr.toml), figment layering. Auto-migrates legacy [instance]/[fields] shape on first load. Active profile resolved at load via Config::load_with(cli_profile) (cli flag threaded through as a parameter, NOT an env-var seam) > JR_PROFILE env > default_profile field > "default". ├── output.rs # Table (comfy-table) and JSON formatting ├── adf.rs # Atlassian Document Format: text→ADF, markdown→ADF, ADF→text ├── duration.rs # Worklog duration parser (2h, 1h30m, 1d, 1w) @@ -121,7 +121,9 @@ When adding a new feature: ## Gotchas -- **Cache format changes:** `~/.cache/jr/cmdb_fields.json` stores `(id, name)` tuples. Old format (ID-only) causes deserialization failure, handled as cache miss. If you change cache structs, old caches auto-expire (7-day TTL) or fail gracefully. +- **Multi-profile boundary:** every cache reader/writer takes `profile: &str` as its first arg. Pass `&config.active_profile_name` from any handler that has `&Config` in scope. Cross-profile cache leakage is a correctness bug, not a UX issue — sandbox vs prod custom-field IDs can differ. +- **Per-profile vs shared OAuth keys:** `email`, `api-token`, `oauth_client_id`, `oauth_client_secret` live under flat keychain keys (account-level, shared across profiles). `oauth-access-token` / `oauth-refresh-token` are namespaced as `:oauth-*` because they're cloudId-scoped. The `"default"` profile lazy-migrates legacy flat keys on first read; other profiles do not. +- **Cache format changes:** `~/.cache/jr/v1//cmdb_fields.json` stores `(id, name)` tuples. Old format (ID-only) causes deserialization failure, handled as cache miss. If you change cache structs, old caches auto-expire (7-day TTL) or fail gracefully. To break compatibility cleanly, bump the cache root from `v1/` to `v2/` — old files orphan harmlessly. - **`list.rs` is large (~970 lines):** Contains both `handle_list` and `handle_view` plus all JQL composition logic. If modifying, read the full function you're changing — context matters. - **`aqlFunction()` not `assetsQuery()`:** The Jira Assets JQL function is `aqlFunction()`. It requires the human-readable field **name**, not `cf[ID]` or `customfield_NNNNN`. AQL attribute for object key is `Key` (not `objectKey` — that's the JSON field name). - **Status category colors are fixed:** `green` = Done, `yellow` = In Progress, `blue-gray` = To Do. These mappings are hardcoded in Jira Cloud across all instances. Used by `--open` filtering. @@ -129,7 +131,10 @@ When adding a new feature: ## AI Agent Notes - `JR_BASE_URL` env var overrides the configured Jira instance URL (used by tests to inject wiremock) +- `JR_PROFILE` env var overrides the active profile per-call (combine with direnv to scope a repo to a sandbox site) +- `--profile NAME` flag overrides `JR_PROFILE` for one invocation; precedence is flag > env > config > "default" - `JiraClient::new_for_test(base_url, auth_header)` constructs a client for integration tests - Test fixtures live in `tests/common/fixtures.rs` +- Keyring round-trip tests are gated behind `JR_RUN_KEYRING_TESTS=1` + `#[ignore]` because Linux CI may lack secret-service - All interactive prompts have non-interactive flag equivalents for AI agent usage - `--output json` on write operations returns structured data (e.g., `{"key": "FOO-123"}`) diff --git a/README.md b/README.md index ba8eeb0b..bac43646 100644 --- a/README.md +++ b/README.md @@ -151,10 +151,14 @@ jr issue comment JSM-42 "customer is on the paid plan — prioritizing" --intern | Command | Description | |---------|-------------| -| `jr init` | Configure Jira instance and authenticate | -| `jr auth login` | Authenticate with API token (default) or `--oauth` for OAuth 2.0. Non-interactive: `--email`/`--token` or `JR_EMAIL`/`JR_API_TOKEN`; `--client-id`/`--client-secret` or `JR_OAUTH_CLIENT_ID`/`JR_OAUTH_CLIENT_SECRET` for OAuth | -| `jr auth refresh` | Clear stored credentials and re-run login (same flags/env vars as `auth login`) | -| `jr auth status` | Show authentication status | +| `jr init` | Configure Jira instance and authenticate (prompts to add another profile if any are already configured) | +| `jr auth login` | Authenticate with API token (default) or `--oauth` for OAuth 2.0. `--profile NAME` targets a specific profile (creates if absent); `--url URL` sets the Jira instance URL when creating. Non-interactive: `--email`/`--token` or `JR_EMAIL`/`JR_API_TOKEN`; `--client-id`/`--client-secret` or `JR_OAUTH_CLIENT_ID`/`JR_OAUTH_CLIENT_SECRET` for OAuth | +| `jr auth switch ` | Set the default profile in `config.toml`. Errors if `NAME` doesn't exist | +| `jr auth list` | List configured profiles (table or JSON via `--output`); active profile marked with `*` | +| `jr auth status` | Show authentication status for the active profile, or `--profile NAME` for another | +| `jr auth refresh` | Refresh credentials for the active profile (or `--profile NAME`); same flags/env vars as `auth login` | +| `jr auth logout` | Clear OAuth tokens for the active profile (or `--profile NAME`); shared API token NOT touched | +| `jr auth remove ` | Permanently delete a profile (config entry + cache + per-profile OAuth tokens). Cannot remove the active profile | | `jr me` | Show current user info | | `jr issue list` | List issues (`--assignee`, `--reporter`, `--recent`, `--status`, `--open`, `--team`, `--asset KEY`, `--jql`, `--limit`/`--all`, `--points`, `--assets`) | | `jr issue view KEY` | View issue details (per-field asset rows, enriched JSON, story points) | @@ -202,6 +206,7 @@ jr issue comment JSM-42 "customer is on the paid plan — prioritizing" --intern |------|-------------| | `--output json\|table` | Output format (default: table) | | `--project FOO` | Override project key | +| `--profile NAME` | Override the active profile for this invocation (precedence: this flag > `JR_PROFILE` env > `default_profile` in config > `"default"`) | | `--no-color` | Disable colored output (also respects `NO_COLOR` env) | | `--no-input` | Disable interactive prompts (auto-enabled in pipes/scripts) | | `--verbose` | Show HTTP request/response details | @@ -215,35 +220,59 @@ jr issue comment JSM-42 "customer is on the paid plan — prioritizing" --intern # Per-project config (in your repo root) .jr.toml -# Team cache (disposable, 7-day TTL) -~/.cache/jr/teams.json +# Per-profile cache (disposable, 7-day TTL) +~/.cache/jr/v1//teams.json ``` -**Global config:** +**Global config (multi-profile shape):** ```toml -[instance] +default_profile = "default" + +[profiles.default] url = "https://yourorg.atlassian.net" auth_method = "api_token" # or "oauth" -# Optional: override the OAuth 2.0 scope list when auth_method = "oauth". -# Must match what your app in the Atlassian Developer Console has -# configured. Classic and granular scopes CANNOT mix in one request, and -# "offline_access" is required for refresh tokens to be issued. If unset, -# jr uses Atlassian's recommended classic scopes. -# oauth_scopes = "read:issue:jira write:issue:jira write:comment:jira read:jira-user offline_access" +# cloud_id, org_id, oauth_scopes, team_field_id, story_points_field_id +# are auto-discovered during `jr init` / `jr auth login --oauth` and +# populated here per profile. +# oauth_scopes = "read:issue:jira write:issue:jira ... offline_access" + +[profiles.sandbox] +url = "https://yourorg-sandbox.atlassian.net" +auth_method = "api_token" +# Sandbox sites usually mirror production custom-field IDs, but `jr` stores +# them per profile so divergence doesn't silently corrupt cached lookups. [defaults] output = "table" +``` + +Switching between profiles: -[fields] -story_points_field_id = "customfield_XXXXX" # auto-discovered during init +```bash +jr auth switch sandbox # persistent — writes default_profile in config.toml +jr --profile sandbox issue list # one-shot — overrides for this call only +JR_PROFILE=sandbox jr issue list # session-scoped (works well with direnv) ``` +A single classic Atlassian API token authenticates the same user against +any Atlassian Cloud site, so `email` + `api-token` are stored once in the +OS keychain and shared by all `api_token` profiles. OAuth tokens are +cloudId-scoped and stored per profile. + **Per-project config:** ```toml project = "FOO" board_id = 42 ``` +**Migrating from single-instance configs:** the first run after upgrading +auto-migrates a legacy `[instance]`+`[fields]` config to the new +`[profiles.default]` shape (one stderr notice; idempotent). OAuth tokens +in the OS keychain lazy-migrate from flat keys (`oauth-access-token`) to +namespaced keys (`default:oauth-access-token`) on first authenticated +read. Old cache files at `~/.cache/jr/*.json` orphan harmlessly when the +new layout starts using `~/.cache/jr/v1//`. + ## Scripting & AI Agents `jr` is designed to be used by scripts and AI coding agents: @@ -255,6 +284,7 @@ board_id = 42 - State-changing commands are idempotent (exit 0 if already in target state) - Structured exit codes (see [Exit Codes](#exit-codes) table) - `auth login` / `auth refresh` accept credentials via flags (`--email`, `--token`, `--client-id`, `--client-secret`) or env vars (`JR_EMAIL`, `JR_API_TOKEN`, `JR_OAUTH_CLIENT_ID`, `JR_OAUTH_CLIENT_SECRET`) — no TTY required. Prefer env vars for secrets. +- `--profile NAME` flag and `JR_PROFILE` env var let agents target a specific profile per-call without mutating the user's `default_profile`. Combined with direnv (`echo 'export JR_PROFILE=sandbox' >> .envrc`), a repo can scope all `jr` calls to a sandbox site automatically. ```bash # AI agent workflow example diff --git a/docs/specs/multi-profile-auth.md b/docs/specs/multi-profile-auth.md new file mode 100644 index 00000000..4d109c35 --- /dev/null +++ b/docs/specs/multi-profile-auth.md @@ -0,0 +1,459 @@ +# Multi-Profile Authentication + +## Goal + +Let `jr` target multiple Atlassian Cloud sites (production, sandbox, additional teams' Jira instances) from one local install, with one `jr auth switch ` command to flip between them. Reuse a single classic Atlassian API token across profiles where possible (account-level credentials authenticate the same user against any Atlassian site), but keep per-site OAuth tokens isolated (cloudId-scoped, not transferable). + +## Motivation + +Today, `GlobalConfig.instance` holds a single URL + cloud_id + auth_method, and the keyring stores one flat set of credentials. To work against a sandbox, a user has to re-run `jr init` and overwrite their prod config — there's no two-environments-per-team workflow. + +The classic API token is account-level by Atlassian's design — the same `email + token` pair authenticates against `acme.atlassian.net` and `acme-sandbox.atlassian.net` for the same user. OAuth tokens, in contrast, are issued against a specific cloudId and don't transfer. The design must reflect both realities: shared API token, per-profile OAuth. + +The blast radius is small: only 5 source files (`api/auth.rs`, `api/client.rs`, `cli/auth.rs`, `cli/init.rs`, `cli/team.rs`) and `config.rs` read `config.global.instance.*` today. + +## Scope + +- **In scope:** Multi-profile config schema, named-profile keyring layout, per-profile cache directory, `jr auth login/switch/list/status/logout/remove/refresh` CLI surface, auto-migration of legacy single-instance configs, gated keyring round-trip tests. +- **Out of scope:** A `jr profile` subcommand tree separate from `jr auth`; profile renaming (use `login + remove`); per-repo `.jr.toml` profile pinning (use `direnv` with `JR_PROFILE`); a `KeyringProvider` trait abstraction (file as follow-up issue); making `Config::save_global` atomic via tempfile + rename (existing limitation, file as follow-up issue). + +## Validation Summary + +Design decisions validated against industry conventions via Perplexity: + +| Decision | Convention validated | +|---|---| +| Classic API token reusable across Atlassian sites | Confirmed — Atlassian docs explicitly support this | +| Shared API token + per-profile OAuth tokens | Mirrors kubectl users (shared) + gh hosts (per-host) | +| Auto-migrate legacy `[instance]` block on first load | Matches kubectl, npm, gh, cargo conventions for non-breaking schema changes | +| Inline `default_profile` in `config.toml` | Matches kubectl's `current-context:` (gcloud's separate `active_config` file is the alternative; kubectl pattern fits jr's single-file shape better) | +| Keyring: single service + namespaced keys (`:oauth-*`) | `:` is safe across macOS Keychain, libsecret, Windows Credential Manager; matches the keyring-rs `Entry::new(service, user)` API shape | +| Per-profile cache subdirectory | kubectl `~/.kube/cache/discovery//` pattern, empirically confirmed | +| Versioned cache root (`~/.cache/jr/v1/`) | Matches pip, Cargo, npm — orphan old files via path versioning, no marker files | +| Lazy/opportunistic OAuth-token migration | Tools typically don't auto-migrate keyring entries; read-fallback is the convention | +| TOML migration upfront, keyring migration lazy | Cost-benefit differs: TOML lazy = perpetual two-schema read; keyring lazy = single-fallback in load_oauth_tokens | +| `[profiles.]` table-of-tables in TOML | Idiomatic (matches Flyway), maps to `BTreeMap` cleanly in serde | +| Per-profile field IDs (`team_field_id`, `story_points_field_id`) | AWS-style full duplication — fields are site-scoped, drift would be a correctness bug | +| `jr auth` consolidated lifecycle (no `jr profile` subtree) | gh-style; lower surface area; profile data is auth-adjacent in jr | +| Keyring testing via real backend + `JR_SERVICE_NAME` | Pragmatic: existing pattern; trait abstraction is a separate refactor | + +## Config Schema + +New `config.toml` shape: + +```toml +default_profile = "default" + +[profiles.default] +url = "https://acme.atlassian.net" +auth_method = "api_token" # "api_token" | "oauth" +cloud_id = "abc-123" # optional for api_token, required for oauth +org_id = "def-456" # optional, used for team queries +oauth_scopes = "..." # optional, used for oauth +team_field_id = "customfield_10001" +story_points_field_id = "customfield_10002" + +[profiles.sandbox] +url = "https://acme-sandbox.atlassian.net" +auth_method = "oauth" +cloud_id = "xyz-789" +oauth_scopes = "read:jira-work write:jira-work offline_access" + +[defaults] +output = "table" +``` + +Rust types: + +```rust +pub struct GlobalConfig { + pub default_profile: Option, + pub profiles: BTreeMap, // BTreeMap for deterministic `jr auth list` + pub defaults: DefaultsConfig, +} + +pub struct ProfileConfig { + pub url: Option, + pub auth_method: Option, + pub cloud_id: Option, + pub org_id: Option, + pub oauth_scopes: Option, + pub team_field_id: Option, + pub story_points_field_id: Option, +} +``` + +`team_field_id` and `story_points_field_id` are per-profile because they're Jira-site-scoped (custom field IDs can differ between sites). Sandbox/prod are usually clones with identical IDs, but silent drift would be a correctness bug. AWS-style full per-profile duplication; `defaults.output` stays global (genuine user preference, site-agnostic). + +## Active-Profile Resolution + +Precedence (highest wins): + +1. `--profile ` CLI flag (global, sibling of `--output`, `--project`, `--no-input`, `--no-color`) +2. `JR_PROFILE` env var +3. `default_profile` field in `config.toml` +4. Literal name `"default"` if none of the above set + +`Config::load()` resolves this once at startup. Result lives as `Config::active_profile_name: String`. The active profile is reached through two accessors plus a load-time validation: + +- `Config::active_profile() -> ProfileConfig` — returns an owned `ProfileConfig`, + cloning the active profile entry from the map. Falls back to + `ProfileConfig::default()` if the active profile name isn't in the map (this + branch is reachable only for in-memory `Config` values built directly in tests + — `Config::load()` already errors with `UserError` for the load-time case). +- `Config::active_profile_or_err() -> anyhow::Result<&ProfileConfig>` — strict + variant that errors with `JrError::ConfigError` when the active profile + isn't in the map. Used by callers that want to fail loudly rather than + silently returning a default. +- `Config::load()` validates that the resolved active profile name (when + `[profiles]` is non-empty) exists in the map, and returns + `JrError::UserError` (exit 64) if not. The error message lists the known + profile names, matching the `switch`/`remove`/`logout`/`status` handlers' + wording. + +## Profile Name Validation + +Allowed: `[A-Za-z0-9_-]{1,64}`. Validation lives in `config::validate_profile_name(name) -> Result<(), JrError>` so the rule is single-sourced — every entry point (CLI, config-load migration) calls it. + +Two validation layers: + +1. **Character set + length**: regex above. Rejects empty strings, whitespace, `:`, `/`, `.`, and other shell/path metacharacters. The `:` rejection guarantees keyring key parsing remains unambiguous; the `/` and `.` rejections keep cache subdirectory paths clean. + +2. **Windows reserved names** (case-insensitive): `CON`, `NUL`, `AUX`, `PRN`, `COM1`–`COM9`, `LPT1`–`LPT9`. Profile names matching these (with or without an extension) are rejected on every platform — even on macOS and Linux where they'd technically work — so configs stay portable across machines. Without this, a `CON` profile created on macOS would fail on Windows when the cache subdir was created. + +Error message: `invalid profile name ""; allowed: A-Z a-z 0-9 _ - up to 64 chars; reserved Windows names (CON, NUL, AUX, PRN, COM1-9, LPT1-9) excluded`. + +## Keyring Layout + +Single service name (`jr-jira-cli`, honoring `JR_SERVICE_NAME` for tests), keys namespaced per profile only where per-site isolation is required: + +| Key | Scope | Notes | +|---|---|---| +| `email` | Shared | User's Atlassian account email | +| `api-token` | Shared | Classic API token, account-level | +| `oauth_client_id` | Shared | OAuth app registered once per Atlassian org | +| `oauth_client_secret` | Shared | Same | +| `:oauth-access-token` | Per-profile | OAuth tokens are cloudId-scoped | +| `:oauth-refresh-token` | Per-profile | Same | + +### Public API (`src/api/auth.rs`) + +```rust +// Shared (signatures unchanged) +pub fn store_api_token(email: &str, token: &str) -> Result<()> +pub fn load_api_token() -> Result<(String, String)> +pub fn store_oauth_app_credentials(client_id: &str, client_secret: &str) -> Result<()> +pub fn load_oauth_app_credentials() -> Result<(String, String)> + +// Per-profile (signatures gain `profile: &str`) +pub fn store_oauth_tokens(profile: &str, access: &str, refresh: &str) -> Result<()> +pub fn load_oauth_tokens(profile: &str) -> Result<(String, String)> + +// Clear helpers +pub fn clear_profile_creds(profile: &str) -> Result<()> // OAuth keys for one profile +pub fn clear_all_credentials(profiles: &[&str]) -> Result<()> // shared keys + every listed profile's OAuth keys +``` + +`clear_all_credentials` takes the list of known profile names from the caller (typically derived from `config.global.profiles.keys()`) so it can clear each `:oauth-*` pair without needing to enumerate the keychain. + +### `:` Separator Safety + +`:` is documented-safe across all three backends (macOS Keychain `kSecAttrAccount` accepts arbitrary CFStrings; libsecret attributes are arbitrary string-string; Windows Credential Manager target names already use `:` internally as a legacy delimiter). Profile-name validation rejects `:` so collisions are impossible. + +## Cache Layout + +Versioned root, per-profile subdirectory: + +``` +~/.cache/jr/ +├── v1/ +│ ├── default/ +│ │ ├── teams.json +│ │ ├── project_meta.json +│ │ ├── workspace.json +│ │ ├── cmdb_fields.json +│ │ ├── object_type_attrs.json +│ │ └── resolutions.json +│ └── sandbox/ +│ └── ... +└── (legacy flat *.json files, never read by new code) +``` + +```rust +pub fn cache_root() -> PathBuf { ... } // ~/.cache/jr +pub fn cache_dir(profile: &str) -> PathBuf { // ~/.cache/jr/v1/ + cache_root().join("v1").join(profile) +} +``` + +All six cache reader/writer pairs gain `profile: &str` as the first arg: + +```rust +pub fn read_team_cache(profile: &str) -> Result> +pub fn write_team_cache(profile: &str, teams: &[CachedTeam]) -> Result<()> +pub fn read_project_meta(profile: &str, project_key: &str) -> Result> +pub fn write_project_meta(profile: &str, project_key: &str, meta: &ProjectMeta) -> Result<()> +// ... same shape for workspace, cmdb_fields, object_type_attrs, resolutions +``` + +Callers pass `config.active_profile_name()`. Every call site already has `&Config` in scope. + +`clear_profile_cache(profile: &str)` is `std::fs::remove_dir_all(cache_dir(profile))`. `clear_all_caches()` removes `~/.cache/jr/v1/` (preserves any future v2 sibling). + +### Legacy Cache Handling + +Old `~/.cache/jr/*.json` files are never read by the new code (they live above `v1/`, outside the new path). They expire by their existing 7-day TTL or the user can `rm` them manually. **No migration code, no warning, no marker file** — versioned root is self-sufficient. + +## CLI Surface + +### New global flag (on `Cli`) + +``` +--profile Override the active profile for this invocation. + Precedence: this flag > JR_PROFILE env > + default_profile in config > "default". +``` + +### `jr auth` subcommands + +``` +jr auth login [--profile NAME] [--url URL] [--oauth] [--no-input] + Log in (creates profile if absent). --profile defaults to active. + --url required when creating a new profile under --no-input; + in interactive mode, jr prompts for the URL. + --url on an EXISTING profile is allowed and transparently updates that + profile's URL (e.g., user moved sites, or wants to change cloud_id + via re-discovery). Passing --url is itself the explicit confirmation + of intent — no separate prompt — so agents and scripts that pass + --url get a deterministic write without an interactive gate. + --oauth on an existing api_token profile (or vice versa) switches the + auth method for that profile transparently and prompts for + whatever the new method needs. + Reuses shared API-token credential when not --oauth — never re-prompts + for the API token if one is already stored. + +jr auth switch + Set default_profile in config.toml to NAME. Errors on unknown profile. + No credential prompts. + +jr auth list + Show all configured profiles. Mark active with `*`. + Table columns: NAME | URL | AUTH | STATUS where STATUS ∈ {configured, unset} + JSON: [{"name", "url", "auth_method", "status", "active"}] + +jr auth status [--profile NAME] + Show one profile's auth state (default: active). + +jr auth logout [--profile NAME] + Clear that profile's OAuth tokens. Profile entry stays in config. + Shared API-token credential not touched (other profiles may use it). + +jr auth remove + Delete the profile entirely: + • OAuth tokens for that profile in keyring (no-op if api_token-auth) + • profile entry in config.toml + • cache subdirectory ~/.cache/jr/v1// + Shared credentials (`email`, `api-token`, `oauth_client_id`, + `oauth_client_secret`) are NEVER touched — other profiles may use + them. To clear shared credentials, manage them via the OS keychain + UI directly (out of scope for this feature; tracked as a follow-up). + Errors if NAME == default_profile (must `jr auth switch` first). + Errors if NAME doesn't exist. + Confirmation prompt unless --no-input. + +jr auth refresh [--profile NAME] [--oauth] [--email/--token/--client-id/--client-secret] + Refresh credentials for the named profile (defaults to active). + The flow is selected from the target profile's auth_method, with + `--oauth` as an explicit override (forces the OAuth path regardless + of stored auth_method, matching `jr auth login --oauth`). + Behavior: + • api_token flow: clears the SHARED email/api-token + client_id/ + client_secret keychain entries (the #207 macOS keychain ACL + workaround) and re-prompts via flag → env → TTY. Equivalent to + `jr auth login` but with explicit cleanup of stale ACL-bound + entries first. + • oauth flow: clears the per-profile :oauth-* keychain + entries and re-runs the FULL 3LO browser flow (oauth_login), + not the silent refresh_token grant. This is intentional — + the same #207 ACL workaround applies to OAuth tokens too, so + a "quiet" refresh wouldn't deliver the macOS-keychain-rebind + guarantee users came here for. + Per-profile token isolation: refreshing OAuth on profile X never + touches the shared api-token or another profile's OAuth tokens. + Refreshing api_token on profile X DOES rewrite the shared keychain + entries (the api-token IS the shared credential). +``` + +### `jr init` interaction + +If run on a config that already has any profile configured, prompt: `"Profiles configured: . Add another? [y/N]"`. If no, exit early. If yes, prompt for new profile name and run the existing `jr init` flow against that new profile. Replaces the current "init silently overwrites whatever is there" behavior. + +## Migration + +Three migration domains; each handled differently per its constraints. + +### (1) `config.toml` — auto, one-time, in `Config::load()` + +Trigger: `[instance]` block exists AND `default_profile` field absent AND `[profiles]` map empty. + +```rust +// Pseudocode in Config::load(), after toml deserialization: +if legacy_shape_detected { + let old_instance = config.legacy_instance.take().expect("checked above"); + let mut profile = ProfileConfig::from(old_instance); + profile.team_field_id = config.legacy_fields.team_field_id.take(); + profile.story_points_field_id = config.legacy_fields.story_points_field_id.take(); + config.profiles.insert("default".to_string(), profile); + config.default_profile = "default".to_string(); + config.save_global()?; + eprintln!( + "Migrated config to multi-profile layout (single profile \"default\"). \ + Run 'jr auth list' to view profiles." + ); +} +``` + +Idempotent (trigger condition is false after first run). Failure handling matches existing `Config::save_global` semantics — out of scope to make atomic in this feature. + +### (2) Keyring OAuth tokens — lazy, on first read + +Old flat `oauth-access-token` / `oauth-refresh-token` keys are read on first miss-then-fall-back inside `load_oauth_tokens`: + +```rust +pub fn load_oauth_tokens(profile: &str) -> Result<(String, String)> { + let access_key = format!("{profile}:oauth-access-token"); + let refresh_key = format!("{profile}:oauth-refresh-token"); + if let (Ok(a), Ok(r)) = (entry(&access_key)?.get_password(), entry(&refresh_key)?.get_password()) { + return Ok((a, r)); + } + if profile == "default" { + if let (Ok(a), Ok(r)) = (entry("oauth-access-token")?.get_password(), entry("oauth-refresh-token")?.get_password()) { + // Opportunistic migration: copy to new keys, best-effort delete legacy + store_oauth_tokens("default", &a, &r)?; + let _ = entry("oauth-access-token")?.delete_credential(); + let _ = entry("oauth-refresh-token")?.delete_credential(); + return Ok((a, r)); + } + } + Err(JrError::NotAuthenticated.into()) +} +``` + +Properties: invisible to user, idempotent (second call sees new keys), failure-safe (partial migration leaves legacy keys readable on next attempt). + +### (3) Cache — none, by versioned root + +Already covered: legacy flat files live at `~/.cache/jr/*.json`, never touched by the new code paths in `~/.cache/jr/v1//`. + +### Rollback story (manual only) + +A user who wants to revert can `cp config.toml config.toml.backup` first (release notes will suggest this) and manually re-author `[instance]` from `[profiles.]`. No `jr config rollback` ships — forward-only matches every surveyed CLI's migration behavior. + +## Error Handling + +| Failure | `JrError` variant | Exit code | Message shape | +|---|---|---|---| +| `--profile X` unknown | `UserError` | 64 | `unknown profile: foo; known: default, sandbox` | +| `JR_PROFILE=X` unknown | `UserError` | 64 | (same as above) | +| `default_profile = "X"` in config but X missing from `[profiles]` | `UserError` | 64 | `default_profile "foo" not in [profiles]; fix config.toml or run "jr auth list"` | +| `jr auth switch ` | `UserError` | 64 | `unknown profile: foo; known: …` | +| `jr auth remove ` where `name == default_profile` | `UserError` | 64 | `cannot remove active profile "default"; switch first with "jr auth switch …"` | +| `jr auth remove ` | `UserError` | 64 | `unknown profile: foo; known: …` | +| `jr auth login --profile X --no-input` and target has no URL | `UserError` | 64 | `--url required when the target profile has no URL configured` | +| `jr auth refresh --profile X` where X uses api_token auth and has no URL | `UserError` | 64 | `profile "X" has no URL configured. Use "jr auth login --profile X --url " instead of refresh — refresh assumes the profile is already set up and only rotates credentials.` | +| Profile name fails character/length validation | `UserError` | 64 | `invalid profile name "foo:bar"; allowed: A-Z a-z 0-9 _ - up to 64 chars; reserved Windows names (CON, NUL, AUX, PRN, COM1-9, LPT1-9) excluded` | +| Profile name matches a Windows reserved name | `UserError` | 64 | (same message — reserved name list embedded) | +| TOML migration write fails | `Internal` | 1 | `Internal error: config migration failed: ` | +| Keyring read fails on per-profile key | `ConfigError` (existing) | 78 | (existing message) | + +## Testing + +TDD; existing test stack (`proptest`, `insta`, `tempfile`, `assert_cmd`, `wiremock`) covers everything. No new test crates. + +### Unit tests (in-module) + +`config::tests`: +- Active-profile resolution precedence (4 cases: flag, env, config field, default fallback) +- Profile-name validation: regex character/length cases (proptest with random strings; assert accept ⇔ regex match), plus an explicit table-driven test for Windows reserved names (CON, con, Con, NUL, AUX, PRN, COM1, COM9, LPT1, LPT9 — case-insensitive — all rejected on every platform) +- Migration: synthetic legacy `[instance]` TOML → assert post-migration `GlobalConfig` shape +- Migration is idempotent (second run is a no-op) +- `[fields]` carried into `[profiles.default]` during migration +- `Config::active_profile()` returns the right `ProfileConfig` (owned clone) and `Config::active_profile_or_err()` returns `&ProfileConfig` or errors with `JrError::ConfigError` for callers that want to fail loudly +- Unknown `default_profile` returns `UserError` (matches the unified active-profile existence check; the value comes from user-edited config, env, or flag — UserError is the honest classification) + +`api::auth::tests`: +- `store_oauth_tokens(profile, ...) + load_oauth_tokens(profile, ...)` round-trip per profile (uses `JR_SERVICE_NAME=jr-jira-cli-test-`) +- Profile A's OAuth tokens not visible to profile B +- Shared `api-token` accessible from any profile load path +- Lazy OAuth migration: pre-seed flat keys, call `load_oauth_tokens("default", ...)`, assert new keys exist + flat keys gone + return value matches +- Lazy migration only fires for `"default"` profile +- `clear_profile_creds("sandbox")` removes only that profile's OAuth, leaves `default:oauth-*` and shared keys intact + +`cache::tests`: +- Existing 24 tests updated to thread `profile` through helpers +- New: cross-profile isolation — write team cache as profile A, attempt read as profile B, assert miss +- New: `clear_profile_cache("sandbox")` removes only that profile's subdir + +### Integration tests (`tests/`) + +`tests/auth_profiles.rs` (new): +- `jr auth login --profile sandbox --url https://… --no-input` (with `JR_API_TOKEN` env preset to skip prompt) — assert config.toml gains the profile, keyring gains shared API token, exit 0 +- `jr auth list --output json` — assert JSON shape, active marker +- `jr auth switch sandbox` — assert default_profile mutated, exit 0 +- `jr auth switch nonexistent` — assert exit 64, error message names known profiles +- `jr auth remove sandbox` — assert config entry gone, OAuth keys gone, cache subdir gone +- `jr auth remove default` (when active) — assert exit 64, no state mutated +- Precedence: `--profile` flag overrides `JR_PROFILE` env; `JR_PROFILE` overrides `default_profile` in config + +### Snapshot tests (`insta`) + +- `jr auth list --output table` for a 3-profile fixture +- `jr auth list --output json` for the same fixture +- Migration on-disk snapshot: write legacy TOML to tempdir, run migration, snapshot the resulting file + +### Migration integration (`tests/migration_legacy.rs`, new) + +- Set up tempdir with legacy `[instance]` TOML + flat keyring keys +- Run `jr auth list` +- Assert TOML migrated, OAuth keys migrated, exit 0 +- Re-run; assert no second migration notice (idempotency) +- Both an in-memory roundtrip (write → load → assert) and an on-disk snapshot (insta) + +### Test isolation + +- `JR_SERVICE_NAME=jr-jira-cli-test-` per test (unique service names prevent collisions) +- `XDG_CACHE_HOME` and `XDG_CONFIG_HOME` set to per-test tempdirs +- Existing `ENV_MUTEX` pattern in `config::tests` handles env-var race conditions + +### Keyring CI compatibility + +Linux CI runners often lack an active D-Bus session for the `secret-service` backend. New keyring round-trip tests are gated with `#[ignore]` and an opt-in env var (`JR_RUN_KEYRING_TESTS=1`): + +```rust +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn store_and_load_per_profile_oauth_tokens() { + if std::env::var("JR_RUN_KEYRING_TESTS").is_err() { return; } + // ... +} +``` + +CI runs them on macOS/Windows by default; Linux CI either provides D-Bus or skips. Local devs run them automatically. + +## Concurrency & Cross-Platform Notes + +**Concurrent `jr` invocations writing `config.toml`**: two simultaneous mutating commands (e.g., `jr auth switch` and `jr auth login` in different terminals) can race; the last writer wins, the other's changes are lost. This is a *pre-existing* limitation of `Config::save_global` (which uses non-atomic `std::fs::write`), not a regression introduced by multi-profile. Mitigated by the same atomic-save follow-up listed below. + +**Concurrent OAuth refresh against the same profile**: two simultaneous `jr auth refresh --profile X` (or any commands that trigger refresh) can both POST to `/oauth/token`, with the second response invalidating the first. Last writer wins on the keyring side. Pre-existing single-instance limitation, not a regression. The retry path on a 401 already handles the case where a stale refresh token rejects — users see one extra retry, not a hard failure. + +**Cross-machine portability**: `config.toml` is plain TOML and copies cleanly between machines. **Credentials in the OS keyring do NOT migrate** (by design — never write secrets to disk). Users moving to a new machine re-run `jr auth login --profile ` to re-establish credentials. Matches every CLI surveyed. + +## Out of Scope / Follow-ups + +- **`jr profile` subcommand tree** — separate from `jr auth`. May be revisited if non-auth per-profile config grows beyond the current set. +- **Profile renaming** — multistep workaround works for now (`jr auth login --profile new --url ...; jr auth logout --profile old; jr auth remove old`). +- **Per-repo `.jr.toml` profile pinning** — direnv with `JR_PROFILE` covers it. Adding it natively conflicts with the universal `flag > env > global > default` convention surveyed across kubectl/aws/gh/gcloud. +- **`KeyringProvider` trait abstraction** — file as a follow-up issue for testability and CI portability. Outside the scope of multi-profile semantics. +- **Atomic `Config::save_global` (tempfile + rename)** — file as a follow-up issue. Existing limitation, not a regression of this feature. +- **Source-profile fallback for API tokens (AWS-style)** — only useful for the niche service-account-per-environment case. Can be layered on later if asked. +- **Bulk-clear command for shared credentials** — `jr auth logout --all` or similar. Today users would manage shared keychain entries via the OS keychain UI. Low frequency (the shared API token is the user's account credential; rarely wiped except on uninstall). File as follow-up. diff --git a/docs/superpowers/plans/2026-04-24-multi-profile-auth.md b/docs/superpowers/plans/2026-04-24-multi-profile-auth.md new file mode 100644 index 00000000..8d26a38d --- /dev/null +++ b/docs/superpowers/plans/2026-04-24-multi-profile-auth.md @@ -0,0 +1,2496 @@ +# Multi-Profile Auth Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let `jr` target multiple Atlassian Cloud sites from one install, with `jr auth switch ` to flip between them. Shared classic API token across profiles, per-profile OAuth tokens, auto-migration of legacy single-instance configs. + +**Architecture:** Foundation-first build. Add new `ProfileConfig` type and `default_profile`/`profiles` fields to `GlobalConfig` while keeping the legacy `[instance]` block deserializable for migration. `Config::load()` performs a one-time TOML migration. Cache directory becomes `~/.cache/jr/v1//`. Keyring keys for OAuth tokens are namespaced as `:oauth-access-token`. Legacy keys read on first miss for the `"default"` profile (lazy migration). All call sites that read `config.global.instance.*` migrate to `config.active_profile().…`. New CLI subcommands `switch / list / remove` join existing `login / status / refresh / logout` (the latter two gain `--profile`). + +**Tech Stack:** Rust 1.85+, serde + figment + toml for config, keyring crate for OS keyring, clap derive for CLI, proptest + insta + tempfile + assert_cmd for tests, wiremock for HTTP mocking. No new test crates. + +--- + +## File Structure + +| File | Role | Status | +|---|---|---| +| `src/config.rs` | Schema types, active-profile resolution, validate_profile_name, migration | Modified — adds ProfileConfig + migration; legacy InstanceConfig removed in Task 14 | +| `src/api/auth.rs` | Keyring read/write per profile + lazy OAuth migration | Modified — store/load_oauth_tokens gain `profile: &str` | +| `src/cache.rs` | Per-profile cache directory + threaded reader/writer signatures | Modified — `cache_dir(profile)` returns `~/.cache/jr/v1/` | +| `src/api/client.rs` | JiraClient::from_config consumes active profile | Modified — flips from `instance.*` to `active_profile().*` | +| `src/cli/mod.rs` | `--profile` global flag + new AuthCommand variants | Modified — adds Switch/List/Logout/Remove; existing Login/Status/Refresh gain `profile` | +| `src/cli/auth.rs` | Implementation of new auth subcommands | Modified — major surface area expansion | +| `src/cli/init.rs` | Prompt before adding additional profile | Modified — detects existing profiles | +| `src/cli/team.rs` | Use active_profile for url/cloud_id/org_id | Modified — small change, single call site | +| `tests/auth_profiles.rs` | End-to-end multi-profile workflow tests | Created | +| `tests/migration_legacy.rs` | Migration snapshot tests with `insta` | Created | + +--- + +## Task 1: Profile name validation + +**Files:** +- Modify: `src/config.rs` (add `validate_profile_name` fn + inline tests) + +- [ ] **Step 1: Write failing tests for character/length validation** + +Add to `src/config.rs` `tests` module (around line 149): + +```rust +#[test] +fn validate_profile_name_accepts_alphanumeric_dash_underscore() { + assert!(validate_profile_name("default").is_ok()); + assert!(validate_profile_name("sandbox-uat").is_ok()); + assert!(validate_profile_name("team_a").is_ok()); + assert!(validate_profile_name("Prod1").is_ok()); + assert!(validate_profile_name("a").is_ok()); + assert!(validate_profile_name(&"a".repeat(64)).is_ok()); +} + +#[test] +fn validate_profile_name_rejects_invalid_chars() { + for bad in ["", " ", "foo bar", "foo:bar", "foo/bar", "foo.bar", "..", "."] { + assert!( + validate_profile_name(bad).is_err(), + "expected {bad:?} to be rejected" + ); + } +} + +#[test] +fn validate_profile_name_rejects_too_long() { + let too_long = "a".repeat(65); + assert!(validate_profile_name(&too_long).is_err()); +} + +#[test] +fn validate_profile_name_rejects_windows_reserved_names_case_insensitive() { + for bad in [ + "CON", "con", "Con", + "NUL", "nul", "AUX", "aux", "PRN", "prn", + "COM1", "com9", "LPT1", "lpt9", + ] { + assert!( + validate_profile_name(bad).is_err(), + "expected Windows reserved name {bad:?} to be rejected" + ); + } +} +``` + +- [ ] **Step 2: Run tests, verify they fail to compile** + +```bash +cargo test --lib config::tests::validate_profile_name 2>&1 | tail -10 +``` +Expected: `cannot find function validate_profile_name` + +- [ ] **Step 3: Implement validate_profile_name** + +Add to `src/config.rs` (top-level, after the type defs around line 60): + +```rust +/// Validate a profile name. See docs/specs/multi-profile-auth.md "Profile Name Validation". +pub fn validate_profile_name(name: &str) -> Result<(), JrError> { + const RESERVED_WINDOWS: &[&str] = &[ + "CON", "NUL", "AUX", "PRN", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + ]; + + if name.is_empty() || name.len() > 64 { + return Err(invalid_profile_name(name)); + } + if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') { + return Err(invalid_profile_name(name)); + } + let upper = name.to_ascii_uppercase(); + if RESERVED_WINDOWS.iter().any(|r| *r == upper.as_str()) { + return Err(invalid_profile_name(name)); + } + Ok(()) +} + +fn invalid_profile_name(name: &str) -> JrError { + JrError::UserError(format!( + "invalid profile name {name:?}; allowed: A-Z a-z 0-9 _ - up to 64 chars; \ + reserved Windows names (CON, NUL, AUX, PRN, COM1-9, LPT1-9) excluded" + )) +} +``` + +- [ ] **Step 4: Run tests, verify pass** + +```bash +cargo test --lib config::tests::validate_profile_name +``` +Expected: 4 tests passing. + +- [ ] **Step 5: Run fmt + clippy + full test suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` +Expected: clean clippy, all tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add src/config.rs +git commit -m "feat(config): validate profile names (regex + Windows reserved)" +``` + +--- + +## Task 2: Add ProfileConfig type and dual-shape GlobalConfig + +**Files:** +- Modify: `src/config.rs` (introduce ProfileConfig, dual-shape during transition) + +The legacy `InstanceConfig` and `FieldsConfig` stay in place. We add `default_profile: Option` and `profiles: BTreeMap` alongside. Migration in Task 4 moves data. + +- [ ] **Step 1: Write failing tests for ProfileConfig serde + GlobalConfig dual-shape parse** + +Add to `src/config.rs` `tests` module: + +```rust +#[test] +fn profile_config_roundtrip() { + let toml = r#" + url = "https://acme.atlassian.net" + auth_method = "oauth" + cloud_id = "abc-123" + org_id = "def-456" + oauth_scopes = "read:jira-work offline_access" + team_field_id = "customfield_10001" + story_points_field_id = "customfield_10002" + "#; + let p: ProfileConfig = toml::from_str(toml).unwrap(); + assert_eq!(p.url.as_deref(), Some("https://acme.atlassian.net")); + assert_eq!(p.auth_method.as_deref(), Some("oauth")); + assert_eq!(p.cloud_id.as_deref(), Some("abc-123")); + assert_eq!(p.org_id.as_deref(), Some("def-456")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_10001")); + assert_eq!(p.story_points_field_id.as_deref(), Some("customfield_10002")); +} + +#[test] +fn global_config_parses_new_shape() { + let toml = r#" + default_profile = "default" + + [profiles.default] + url = "https://acme.atlassian.net" + auth_method = "api_token" + + [profiles.sandbox] + url = "https://acme-sandbox.atlassian.net" + auth_method = "oauth" + cloud_id = "xyz-789" + "#; + let cfg: GlobalConfig = toml::from_str(toml).unwrap(); + assert_eq!(cfg.default_profile.as_deref(), Some("default")); + assert_eq!(cfg.profiles.len(), 2); + assert!(cfg.profiles.contains_key("default")); + assert!(cfg.profiles.contains_key("sandbox")); + assert_eq!( + cfg.profiles["sandbox"].cloud_id.as_deref(), + Some("xyz-789") + ); +} + +#[test] +fn global_config_parses_legacy_shape_into_legacy_fields() { + let toml = r#" + [instance] + url = "https://legacy.atlassian.net" + auth_method = "api_token" + cloud_id = "legacy-1" + + [fields] + team_field_id = "customfield_99" + story_points_field_id = "customfield_42" + "#; + let cfg: GlobalConfig = toml::from_str(toml).unwrap(); + assert!(cfg.profiles.is_empty(), "no [profiles] in legacy shape"); + assert!(cfg.default_profile.is_none(), "no default_profile in legacy shape"); + assert_eq!(cfg.instance.url.as_deref(), Some("https://legacy.atlassian.net")); + assert_eq!(cfg.fields.team_field_id.as_deref(), Some("customfield_99")); +} +``` + +- [ ] **Step 2: Run tests, verify they fail to compile** + +```bash +cargo test --lib config::tests 2>&1 | tail -10 +``` +Expected: `cannot find type ProfileConfig` and missing fields on GlobalConfig. + +- [ ] **Step 3: Add ProfileConfig and dual-shape fields to GlobalConfig** + +Modify `src/config.rs`. Update `GlobalConfig`: + +```rust +#[derive(Debug, Deserialize, Serialize, Default)] +pub struct GlobalConfig { + /// New-shape: name of the active profile. + /// Resolved precedence: --profile > JR_PROFILE > this field > "default". + /// `Option` because legacy configs don't have it. + #[serde(default)] + pub default_profile: Option, + + /// New-shape: named profiles. + #[serde(default)] + pub profiles: std::collections::BTreeMap, + + /// Legacy single-instance config — read for migration only. + /// Removed in cleanup task once migration is fully wired. + #[serde(default)] + pub instance: InstanceConfig, + + /// Legacy global custom-field IDs — read for migration only. + /// Migration moves these into the default profile. + #[serde(default)] + pub fields: FieldsConfig, + + #[serde(default)] + pub defaults: DefaultsConfig, +} +``` + +Add `ProfileConfig` (right after `FieldsConfig` near line 14): + +```rust +#[derive(Debug, Deserialize, Serialize, Default, Clone)] +pub struct ProfileConfig { + pub url: Option, + pub auth_method: Option, + pub cloud_id: Option, + pub org_id: Option, + pub oauth_scopes: Option, + pub team_field_id: Option, + pub story_points_field_id: Option, +} +``` + +- [ ] **Step 4: Run tests, verify pass** + +```bash +cargo test --lib config::tests +``` +Expected: all 3 new tests pass; existing tests still pass. + +- [ ] **Step 5: Run fmt + clippy + full test suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 6: Commit** + +```bash +git add src/config.rs +git commit -m "feat(config): add ProfileConfig type alongside legacy InstanceConfig" +``` + +--- + +## Task 3: Active-profile resolution + +**Files:** +- Modify: `src/config.rs` (resolve_active_profile_name fn + Config::active_profile method + tests) + +- [ ] **Step 1: Write failing tests for precedence chain** + +Add to `src/config.rs` `tests` module: + +```rust +#[test] +fn resolve_active_profile_name_uses_cli_flag_when_set() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, Some("flag-value"), None); + assert_eq!(name, "flag-value"); +} + +#[test] +fn resolve_active_profile_name_uses_env_when_no_flag() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, None, Some("env-value".into())); + assert_eq!(name, "env-value"); +} + +#[test] +fn resolve_active_profile_name_uses_config_when_no_flag_or_env() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, None, None); + assert_eq!(name, "config-default"); +} + +#[test] +fn resolve_active_profile_name_falls_back_to_default_literal() { + let cfg = GlobalConfig::default(); + let name = resolve_active_profile_name(&cfg, None, None); + assert_eq!(name, "default"); +} + +#[test] +fn config_active_profile_returns_resolved_profile() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("sandbox".to_string(), ProfileConfig { + url: Some("https://sandbox.example".into()), + ..ProfileConfig::default() + }); + let cfg = Config { + global: GlobalConfig { + default_profile: Some("sandbox".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "sandbox".into(), + }; + assert_eq!( + cfg.active_profile().url.as_deref(), + Some("https://sandbox.example") + ); +} + +#[test] +fn config_active_profile_unknown_profile_returns_error() { + let cfg = Config { + global: GlobalConfig::default(), + project: ProjectConfig::default(), + active_profile_name: "ghost".into(), + }; + assert!(cfg.active_profile_or_err().is_err()); +} +``` + +- [ ] **Step 2: Run tests, verify fail** + +```bash +cargo test --lib config::tests::resolve_active_profile_name config::tests::config_active_profile 2>&1 | tail -10 +``` +Expected: missing fn `resolve_active_profile_name`, missing field `active_profile_name`, missing methods. + +- [ ] **Step 3: Implement resolution + Config field + methods** + +Modify `src/config.rs`. Update `Config` struct: + +```rust +#[derive(Debug, Default)] +pub struct Config { + pub global: GlobalConfig, + pub project: ProjectConfig, + /// Resolved at load() — flag > JR_PROFILE > default_profile > "default". + pub active_profile_name: String, +} +``` + +Add free function: + +```rust +/// Resolve the active profile name from precedence chain: +/// 1. cli_flag (--profile) +/// 2. env var (JR_PROFILE) +/// 3. config.default_profile field +/// 4. literal "default" +pub fn resolve_active_profile_name( + config: &GlobalConfig, + cli_flag: Option<&str>, + env_var: Option, +) -> String { + if let Some(name) = cli_flag { + return name.to_string(); + } + if let Some(name) = env_var { + return name; + } + if let Some(name) = config.default_profile.as_ref() { + return name.clone(); + } + "default".to_string() +} +``` + +Add methods on `Config`: + +```rust +impl Config { + /// Look up the active profile. Returns a default-empty `ProfileConfig` if + /// the active profile isn't in the map (legacy migration path runs before + /// most callers reach this; tests can also exercise the empty case). + pub fn active_profile(&self) -> ProfileConfig { + self.global + .profiles + .get(&self.active_profile_name) + .cloned() + .unwrap_or_default() + } + + /// Strict variant — errors if the active profile isn't configured. + pub fn active_profile_or_err(&self) -> anyhow::Result<&ProfileConfig> { + self.global.profiles.get(&self.active_profile_name).ok_or_else(|| { + let known: Vec<&str> = self.global.profiles.keys().map(String::as_str).collect(); + JrError::ConfigError(format!( + "default_profile {:?} not in [profiles]; known: {}; \ + fix config.toml or run \"jr auth list\"", + self.active_profile_name, + if known.is_empty() { "(none)".into() } else { known.join(", ") } + )) + .into() + }) + } +} +``` + +- [ ] **Step 4: Run tests, verify pass** + +```bash +cargo test --lib config::tests::resolve_active_profile_name config::tests::config_active_profile +``` +Expected: 6 new tests pass. + +- [ ] **Step 5: Update Config::load to populate active_profile_name** + +Modify `Config::load` in `src/config.rs` (around line 61). At the end, before returning `Ok(Config { global, project })`: + +```rust +let cli_profile_flag = std::env::var("JR_PROFILE_OVERRIDE").ok(); // populated by main from CLI flag +let env_profile = std::env::var("JR_PROFILE").ok(); +let active_profile_name = resolve_active_profile_name( + &global, + cli_profile_flag.as_deref(), + env_profile, +); + +Ok(Config { global, project, active_profile_name }) +``` + +(`JR_PROFILE_OVERRIDE` is set by `main.rs` from the parsed `--profile` flag in Task 9; not user-facing.) + +- [ ] **Step 6: Run full test suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/config.rs +git commit -m "feat(config): resolve active profile name from precedence chain" +``` + +--- + +## Task 4: Config auto-migration of legacy shape + +**Files:** +- Modify: `src/config.rs` (Config::load runs migration when legacy shape detected) + +- [ ] **Step 1: Write failing migration tests** + +Add to `src/config.rs` `tests` module: + +```rust +#[test] +fn migrate_legacy_instance_into_default_profile() { + let mut global = GlobalConfig::default(); + global.instance = InstanceConfig { + url: Some("https://legacy.example".into()), + cloud_id: Some("legacy-1".into()), + org_id: Some("org-1".into()), + auth_method: Some("api_token".into()), + oauth_scopes: None, + }; + global.fields = FieldsConfig { + team_field_id: Some("customfield_99".into()), + story_points_field_id: Some("customfield_42".into()), + }; + + let migrated = migrate_legacy_global(global); + + assert_eq!(migrated.default_profile.as_deref(), Some("default")); + assert_eq!(migrated.profiles.len(), 1); + let p = &migrated.profiles["default"]; + assert_eq!(p.url.as_deref(), Some("https://legacy.example")); + assert_eq!(p.cloud_id.as_deref(), Some("legacy-1")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_99")); + assert_eq!(p.story_points_field_id.as_deref(), Some("customfield_42")); + assert!(migrated.instance.url.is_none(), "[instance] cleared after migration"); + assert!(migrated.fields.team_field_id.is_none(), "[fields] cleared after migration"); +} + +#[test] +fn migrate_legacy_is_idempotent_when_already_new_shape() { + let global = GlobalConfig { + default_profile: Some("custom".into()), + profiles: { + let mut m = std::collections::BTreeMap::new(); + m.insert("custom".to_string(), ProfileConfig { + url: Some("https://x.example".into()), + ..ProfileConfig::default() + }); + m + }, + ..GlobalConfig::default() + }; + let migrated = migrate_legacy_global(global.clone()); + assert_eq!(migrated.default_profile.as_deref(), Some("custom")); + assert_eq!(migrated.profiles.len(), 1); + assert_eq!( + migrated.profiles["custom"].url.as_deref(), + Some("https://x.example") + ); +} + +#[test] +fn migrate_legacy_with_no_data_yields_empty_new_shape() { + let global = GlobalConfig::default(); + let migrated = migrate_legacy_global(global); + assert!(migrated.profiles.is_empty()); + assert!(migrated.default_profile.is_none()); +} +``` + +Note: ProfileConfig and GlobalConfig need to derive `Clone` for the idempotent test. Verify Step 3 of Task 2 already added Clone; if not, this task adds it. + +- [ ] **Step 2: Run tests, verify fail** + +```bash +cargo test --lib config::tests::migrate 2>&1 | tail -10 +``` +Expected: missing fn `migrate_legacy_global`. + +- [ ] **Step 3: Implement migrate_legacy_global** + +Add to `src/config.rs`: + +```rust +/// Pure migration: converts a `GlobalConfig` with legacy `[instance]` + `[fields]` +/// data into the new `[profiles.default]` shape. No-op if already in new shape. +pub fn migrate_legacy_global(mut global: GlobalConfig) -> GlobalConfig { + // Already migrated? (new shape has at least one profile) + if !global.profiles.is_empty() { + return global; + } + + // No data at all? Return as-is — no profile to create. + if global.instance.url.is_none() + && global.instance.auth_method.is_none() + && global.instance.cloud_id.is_none() + && global.fields.team_field_id.is_none() + && global.fields.story_points_field_id.is_none() + { + return global; + } + + // Move legacy data into a "default" profile. + let profile = ProfileConfig { + url: global.instance.url.take(), + auth_method: global.instance.auth_method.take(), + cloud_id: global.instance.cloud_id.take(), + org_id: global.instance.org_id.take(), + oauth_scopes: global.instance.oauth_scopes.take(), + team_field_id: global.fields.team_field_id.take(), + story_points_field_id: global.fields.story_points_field_id.take(), + }; + global.profiles.insert("default".to_string(), profile); + global.default_profile = Some("default".to_string()); + global +} +``` + +Make `GlobalConfig` and `ProfileConfig` derive `Clone` if not already (verify Task 2 already did this). + +- [ ] **Step 4: Wire migration into Config::load + emit one-time stderr** + +Modify `Config::load` in `src/config.rs`. After deserialization but before resolving active_profile_name: + +```rust +let was_legacy = !global.profiles.is_empty() + || global.instance.url.is_some() + || global.fields.team_field_id.is_some(); +let needs_migration = global.profiles.is_empty() + && (global.instance.url.is_some() || global.fields.team_field_id.is_some()); + +if needs_migration { + global = migrate_legacy_global(global); + // Persist migrated shape so subsequent loads don't re-migrate. + save_global_to(&global_path, &global)?; + eprintln!( + "Migrated config to multi-profile layout (single profile \"default\"). \ + Run 'jr auth list' to view profiles." + ); +} +let _ = was_legacy; // suppress unused-binding warning when feature disabled +``` + +Refactor `save_global` to use a free helper that takes a path: + +```rust +fn save_global_to(path: &std::path::Path, global: &GlobalConfig) -> anyhow::Result<()> { + if let Some(dir) = path.parent() { + std::fs::create_dir_all(dir)?; + } + let content = toml::to_string_pretty(global)?; + std::fs::write(path, content)?; + Ok(()) +} +``` + +And update the existing `Config::save_global` to delegate to it: + +```rust +pub fn save_global(&self) -> anyhow::Result<()> { + save_global_to(&global_config_path(), &self.global) +} +``` + +- [ ] **Step 5: Run tests + check the full migration flow** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test --lib config::tests +``` +Expected: 3 new migration tests + all earlier tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add src/config.rs +git commit -m "feat(config): auto-migrate legacy [instance] block into [profiles.default]" +``` + +--- + +## Task 5: Per-profile OAuth keyring API + lazy migration + +**Files:** +- Modify: `src/api/auth.rs` (signatures of store/load_oauth_tokens gain `profile: &str`; lazy fallback in load) + +- [ ] **Step 1: Write failing tests** + +Add to `src/api/auth.rs` `tests` module: + +```rust +fn unique_test_service() -> String { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::SeqCst); + format!("jr-jira-cli-test-{}-{}", std::process::id(), n) +} + +/// Wrap a test in a unique JR_SERVICE_NAME scope so concurrent tests don't collide. +fn with_test_keyring(f: F) { + if std::env::var("JR_RUN_KEYRING_TESTS").is_err() { + return; // keyring tests are opt-in (Linux CI may lack secret-service) + } + let svc = unique_test_service(); + let prev = std::env::var("JR_SERVICE_NAME").ok(); + // SAFETY: tests using keyring must be serialized via JR_RUN_KEYRING_TESTS opt-in. + unsafe { std::env::set_var("JR_SERVICE_NAME", &svc) }; + f(); + let _ = clear_all_credentials(); + unsafe { + match prev { + Some(p) => std::env::set_var("JR_SERVICE_NAME", p), + None => std::env::remove_var("JR_SERVICE_NAME"), + } + } +} + +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn store_and_load_per_profile_oauth_tokens_round_trip() { + with_test_keyring(|| { + store_oauth_tokens("default", "access1", "refresh1").unwrap(); + store_oauth_tokens("sandbox", "access2", "refresh2").unwrap(); + + let (a1, r1) = load_oauth_tokens("default").unwrap(); + let (a2, r2) = load_oauth_tokens("sandbox").unwrap(); + + assert_eq!((a1.as_str(), r1.as_str()), ("access1", "refresh1")); + assert_eq!((a2.as_str(), r2.as_str()), ("access2", "refresh2")); + }); +} + +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn load_oauth_tokens_returns_err_for_missing_profile() { + with_test_keyring(|| { + assert!(load_oauth_tokens("default").is_err()); + }); +} + +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn lazy_migration_legacy_flat_keys_for_default_profile() { + with_test_keyring(|| { + // Pre-seed legacy flat keys (simulating pre-migration state) + entry("oauth-access-token").unwrap() + .set_password("legacy-access").unwrap(); + entry("oauth-refresh-token").unwrap() + .set_password("legacy-refresh").unwrap(); + + // First load on "default" profile triggers lazy migration. + let (access, refresh) = load_oauth_tokens("default").unwrap(); + assert_eq!(access, "legacy-access"); + assert_eq!(refresh, "legacy-refresh"); + + // New keys exist + let new_access = entry("default:oauth-access-token").unwrap().get_password().unwrap(); + assert_eq!(new_access, "legacy-access"); + + // Legacy keys cleaned up + assert!(entry("oauth-access-token").unwrap().get_password().is_err()); + }); +} + +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn lazy_migration_does_not_fire_for_non_default_profile() { + with_test_keyring(|| { + entry("oauth-access-token").unwrap() + .set_password("legacy-access").unwrap(); + entry("oauth-refresh-token").unwrap() + .set_password("legacy-refresh").unwrap(); + + assert!(load_oauth_tokens("sandbox").is_err(), + "sandbox profile should NOT inherit legacy keys"); + }); +} +``` + +- [ ] **Step 2: Run tests, verify they fail or are ignored** + +```bash +cargo test --lib api::auth::tests 2>&1 | tail -10 +``` +Expected: compile errors on `store_oauth_tokens("default", ...)` arity mismatch. + +- [ ] **Step 3: Update store/load_oauth_tokens signatures + add lazy migration + add clear helpers** + +Modify `src/api/auth.rs`. Replace existing `store_oauth_tokens` and `load_oauth_tokens` (lines 59–75): + +```rust +const KEY_OAUTH_ACCESS_LEGACY: &str = "oauth-access-token"; +const KEY_OAUTH_REFRESH_LEGACY: &str = "oauth-refresh-token"; + +fn oauth_access_key(profile: &str) -> String { format!("{profile}:oauth-access-token") } +fn oauth_refresh_key(profile: &str) -> String { format!("{profile}:oauth-refresh-token") } + +/// Store OAuth 2.0 access and refresh tokens scoped to a profile. +pub fn store_oauth_tokens(profile: &str, access: &str, refresh: &str) -> Result<()> { + entry(&oauth_access_key(profile))?.set_password(access)?; + entry(&oauth_refresh_key(profile))?.set_password(refresh)?; + Ok(()) +} + +/// Load OAuth 2.0 access and refresh tokens for a profile. +/// +/// For the `"default"` profile, falls back to the legacy flat keys (pre-migration +/// state) and opportunistically migrates them to the new namespaced keys on read. +pub fn load_oauth_tokens(profile: &str) -> Result<(String, String)> { + let access_key = oauth_access_key(profile); + let refresh_key = oauth_refresh_key(profile); + if let (Ok(a), Ok(r)) = ( + entry(&access_key)?.get_password(), + entry(&refresh_key)?.get_password(), + ) { + return Ok((a, r)); + } + if profile == "default" { + if let (Ok(a), Ok(r)) = ( + entry(KEY_OAUTH_ACCESS_LEGACY)?.get_password(), + entry(KEY_OAUTH_REFRESH_LEGACY)?.get_password(), + ) { + // Opportunistic migration to new keys; best-effort delete of legacy. + store_oauth_tokens("default", &a, &r)?; + let _ = entry(KEY_OAUTH_ACCESS_LEGACY)?.delete_credential(); + let _ = entry(KEY_OAUTH_REFRESH_LEGACY)?.delete_credential(); + return Ok((a, r)); + } + } + Err(anyhow::anyhow!( + "No stored OAuth token for profile {profile:?} — run \"jr auth login --profile {profile}\"" + )) +} + +/// Clear OAuth tokens for a single profile (other profiles + shared keys untouched). +pub fn clear_profile_creds(profile: &str) -> Result<()> { + let mut failures: Vec = Vec::new(); + for key in [oauth_access_key(profile), oauth_refresh_key(profile)] { + match entry(&key) { + Ok(e) => match e.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => {} + Err(err) => failures.push(format!("{key}: {err}")), + }, + Err(err) => failures.push(format!("{key}: {err}")), + } + } + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow::anyhow!( + "failed to clear {} keychain entries: {}", + failures.len(), + failures.join("; ") + )) + } +} +``` + +Rename existing `clear_credentials` to `clear_all_credentials` and update body to enumerate known profiles. Since the function doesn't have config access, take the profile list as parameter: + +```rust +/// Clear shared credentials and OAuth tokens for all listed profiles. +pub fn clear_all_credentials(profiles: &[&str]) -> Result<()> { + let mut failures: Vec = Vec::new(); + let mut keys: Vec = vec![ + KEY_EMAIL.to_string(), + KEY_API_TOKEN.to_string(), + "oauth_client_id".to_string(), + "oauth_client_secret".to_string(), + // Legacy keys (in case lazy migration hasn't run yet) + KEY_OAUTH_ACCESS_LEGACY.to_string(), + KEY_OAUTH_REFRESH_LEGACY.to_string(), + ]; + for profile in profiles { + keys.push(oauth_access_key(profile)); + keys.push(oauth_refresh_key(profile)); + } + for key in keys { + match entry(&key) { + Ok(e) => match e.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => {} + Err(err) => failures.push(format!("{key}: {err}")), + }, + Err(err) => failures.push(format!("{key}: {err}")), + } + } + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow::anyhow!( + "failed to clear {} keychain entries: {}", + failures.len(), + failures.join("; ") + )) + } +} +``` + +Update the existing call site of `store_oauth_tokens` in `oauth_login` (line ~252) and `refresh_oauth_token` (line ~294) — they need a profile argument. Wire them to take `profile: &str`: + +```rust +pub async fn oauth_login( + profile: &str, + client_id: &str, + client_secret: &str, + scopes: &str, +) -> Result { + // ...existing code... + // 5. Store tokens in the system keychain. + store_oauth_tokens(profile, &tokens.access_token, &tokens.refresh_token)?; + // ...rest... +} + +pub async fn refresh_oauth_token( + profile: &str, + client_id: &str, + client_secret: &str, +) -> Result { + let (_, refresh_token) = load_oauth_tokens(profile)?; + // ...existing code... + store_oauth_tokens(profile, &tokens.access_token, &tokens.refresh_token)?; + Ok(tokens.access_token) +} +``` + +Also need to update `cli/auth.rs` callers — for now use `"default"` as the profile literal in those callers; Task 11 properly threads the active profile name in. + +- [ ] **Step 4: Update existing callers in cli/auth.rs to pass "default"** + +In `src/cli/auth.rs`, find each call to `oauth_login`, `refresh_oauth_token`, `load_oauth_tokens` (now takes profile arg), and pass `"default"` as the profile literal. Search: + +```bash +grep -n "oauth_login\|refresh_oauth_token\|load_oauth_tokens" src/cli/auth.rs +``` + +Update each call site's first arg to `"default"`. Same for `src/api/client.rs` line 59. + +Update the existing call to `clear_credentials` in `cli/auth.rs` to `clear_all_credentials(&["default"])`. + +- [ ] **Step 5: Run keyring tests in opt-in mode** + +```bash +JR_RUN_KEYRING_TESTS=1 cargo test --lib api::auth::tests -- --ignored +``` +Expected: keyring round-trip and lazy migration tests pass. + +- [ ] **Step 6: Run fmt + clippy + full test suite (without keyring opt-in)** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/api/auth.rs src/cli/auth.rs src/api/client.rs +git commit -m "refactor(auth): namespace OAuth tokens by profile + lazy migrate legacy keys" +``` + +--- + +## Task 6: Per-profile cache directory + +**Files:** +- Modify: `src/cache.rs` (cache_dir takes profile, all 6 reader/writer pairs take profile, all callers updated) + +- [ ] **Step 1: Write failing tests for new cache_dir + cross-profile isolation** + +Add to `src/cache.rs` `tests` module: + +```rust +#[test] +fn cache_dir_includes_v1_and_profile_subdir() { + with_temp_cache(|| { + let dir = cache_dir("default"); + assert!(dir.ends_with("v1/default"), "got: {}", dir.display()); + }); +} + +#[test] +fn cross_profile_isolation_team_cache() { + with_temp_cache(|| { + write_team_cache("prod", &[CachedTeam { + id: "t1".into(), name: "Prod Team".into() + }]).unwrap(); + + let prod = read_team_cache("prod").unwrap().unwrap(); + assert_eq!(prod.teams[0].name, "Prod Team"); + + // Sandbox profile sees no cache (no leakage) + assert!(read_team_cache("sandbox").unwrap().is_none()); + }); +} + +#[test] +fn clear_profile_cache_removes_only_that_profile() { + with_temp_cache(|| { + write_team_cache("prod", &[CachedTeam { id: "p".into(), name: "P".into() }]).unwrap(); + write_team_cache("sandbox", &[CachedTeam { id: "s".into(), name: "S".into() }]).unwrap(); + + clear_profile_cache("prod").unwrap(); + + assert!(read_team_cache("prod").unwrap().is_none(), "prod cache cleared"); + assert!(read_team_cache("sandbox").unwrap().is_some(), "sandbox cache preserved"); + }); +} +``` + +- [ ] **Step 2: Run tests, verify fail** + +```bash +cargo test --lib cache::tests::cache_dir_includes cache::tests::cross_profile cache::tests::clear_profile 2>&1 | tail -10 +``` +Expected: arity mismatch (`cache_dir()` takes 0 args). + +- [ ] **Step 3: Update `cache_dir` and all reader/writer signatures** + +Modify `src/cache.rs`. Replace `cache_dir`: + +```rust +pub fn cache_root() -> PathBuf { + if let Ok(xdg) = std::env::var("XDG_CACHE_HOME") { + PathBuf::from(xdg).join("jr") + } else { + dirs::home_dir() + .unwrap_or_else(|| PathBuf::from("~")) + .join(".cache") + .join("jr") + } +} + +/// Per-profile cache directory: ~/.cache/jr/v1// +pub fn cache_dir(profile: &str) -> PathBuf { + cache_root().join("v1").join(profile) +} +``` + +Update internal helpers: + +```rust +fn read_cache(profile: &str, filename: &str) -> Result> { + let path = cache_dir(profile).join(filename); + // ... rest unchanged ... +} + +fn write_cache(profile: &str, filename: &str, data: &T) -> Result<()> { + let dir = cache_dir(profile); + std::fs::create_dir_all(&dir)?; + // ... rest unchanged ... +} +``` + +Update every public reader/writer pair: + +```rust +pub fn read_team_cache(profile: &str) -> Result> { + read_cache(profile, "teams.json") +} +pub fn write_team_cache(profile: &str, teams: &[CachedTeam]) -> Result<()> { + write_cache(profile, "teams.json", &TeamCache { fetched_at: Utc::now(), teams: teams.to_vec() }) +} + +pub fn read_project_meta(profile: &str, project_key: &str) -> Result> { /* ... */ } +pub fn write_project_meta(profile: &str, project_key: &str, meta: &ProjectMeta) -> Result<()> { /* ... */ } + +pub fn read_workspace_cache(profile: &str) -> Result> { + read_cache(profile, "workspace.json") +} +pub fn write_workspace_cache(profile: &str, workspace_id: &str) -> Result<()> { + write_cache(profile, "workspace.json", &WorkspaceCache { workspace_id: workspace_id.to_string(), fetched_at: Utc::now() }) +} + +pub fn read_cmdb_fields_cache(profile: &str) -> Result> { /* ... */ } +pub fn write_cmdb_fields_cache(profile: &str, fields: &[(String, String)]) -> Result<()> { /* ... */ } + +pub fn read_object_type_attr_cache(profile: &str, object_type_id: &str) -> Result>> { /* ... */ } +pub fn write_object_type_attr_cache(profile: &str, object_type_id: &str, attrs: &[CachedObjectTypeAttr]) -> Result<()> { /* ... */ } + +// Whatever exists for resolutions — same shape. +``` + +Add helper: + +```rust +pub fn clear_profile_cache(profile: &str) -> Result<()> { + let dir = cache_dir(profile); + if dir.exists() { + std::fs::remove_dir_all(dir)?; + } + Ok(()) +} +``` + +- [ ] **Step 4: Update all 24 existing cache tests to thread profile** + +Search for callsites in tests: + +```bash +grep -n "read_team_cache\|write_team_cache\|read_project_meta\|write_project_meta\|read_workspace_cache\|write_workspace_cache\|read_cmdb_fields_cache\|write_cmdb_fields_cache\|read_object_type_attr_cache\|write_object_type_attr_cache" src/cache.rs +``` + +Each call gains `"default"` (or any per-test profile name) as first arg. The existing `with_temp_cache` helper continues to work (tests just thread profile through helpers). + +- [ ] **Step 5: Update all 15 production callsites** + +The grep from Task setup showed these read `config.global.instance.*` AND likely cache. Task 7 handles client.rs separately; for cache callsites: + +```bash +grep -rn "read_team_cache\|write_team_cache\|read_project_meta\|write_project_meta\|read_workspace_cache\|write_workspace_cache\|read_cmdb_fields_cache\|write_cmdb_fields_cache\|read_object_type_attr_cache\|write_object_type_attr_cache" src/ --include='*.rs' | grep -v 'src/cache.rs' +``` + +Each call site has `&Config` in scope. Pass `&config.active_profile_name` as the first arg. + +- [ ] **Step 6: Run full test suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` +Expected: all tests pass. + +- [ ] **Step 7: Commit** + +```bash +git add src/cache.rs src/cli/ src/api/ +git commit -m "refactor(cache): per-profile cache directory under v1/" +``` + +--- + +## Task 7: JiraClient consumes active profile + +**Files:** +- Modify: `src/api/client.rs` (replace config.global.instance reads with config.active_profile) +- Modify: `src/config.rs` (Config::base_url uses active_profile) + +- [ ] **Step 1: Write failing test for base_url with profiles** + +Add to `src/config.rs` `tests` module: + +```rust +#[test] +fn base_url_uses_active_profile() { + let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("sandbox".to_string(), ProfileConfig { + url: Some("https://sandbox.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }); + let config = Config { + global: GlobalConfig { + default_profile: Some("sandbox".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "sandbox".into(), + }; + assert_eq!(config.base_url().unwrap(), "https://sandbox.atlassian.net"); +} + +#[test] +fn base_url_uses_active_profile_oauth_path() { + let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig { + url: Some("https://acme.atlassian.net".into()), + auth_method: Some("oauth".into()), + cloud_id: Some("abc-123".into()), + ..ProfileConfig::default() + }); + let config = Config { + global: GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "default".into(), + }; + assert_eq!( + config.base_url().unwrap(), + "https://api.atlassian.com/ex/jira/abc-123" + ); +} +``` + +- [ ] **Step 2: Update Config::base_url to read active_profile** + +Modify `src/config.rs` `base_url`: + +```rust +pub fn base_url(&self) -> anyhow::Result { + if let Ok(override_url) = std::env::var("JR_BASE_URL") { + return Ok(override_url.trim_end_matches('/').to_string()); + } + let profile = self.global.profiles.get(&self.active_profile_name).ok_or_else(|| { + JrError::ConfigError(format!( + "No Jira instance configured for profile {:?}. Run \"jr auth login --profile {}\" or \"jr init\".", + self.active_profile_name, self.active_profile_name + )) + })?; + let url = profile.url.as_ref().ok_or_else(|| { + JrError::ConfigError(format!( + "Profile {:?} has no URL configured. Run \"jr auth login --profile {}\".", + self.active_profile_name, self.active_profile_name + )) + })?; + if let Some(cloud_id) = &profile.cloud_id { + if profile.auth_method.as_deref() == Some("oauth") { + return Ok(format!("https://api.atlassian.com/ex/jira/{cloud_id}")); + } + } + Ok(url.trim_end_matches('/').to_string()) +} +``` + +- [ ] **Step 3: Update existing base_url tests to use new profile shape** + +The pre-existing `test_base_url_api_token`, `test_base_url_oauth`, `test_base_url_missing`, `test_base_url_trailing_slash_trimmed` need to switch from `instance.url` to `profiles.get(...).url`. Adapt each accordingly. + +- [ ] **Step 4: Update JiraClient::from_config to consume active profile** + +Modify `src/api/client.rs` `from_config` (lines 35–84). Replace `config.global.instance.*` reads: + +```rust +let profile = config.active_profile_or_err()?; + +let instance_url = if let Some(ref override_url) = test_override { + override_url.trim_end_matches('/').to_string() +} else if let Some(url) = profile.url.as_ref() { + url.trim_end_matches('/').to_string() +} else { + return Err(JrError::ConfigError(format!( + "Profile {:?} has no URL. Run \"jr auth login --profile {}\".", + config.active_profile_name, config.active_profile_name + )).into()); +}; +let auth_method = profile.auth_method.as_deref().unwrap_or("api_token"); + +let auth_header = if let Ok(header) = std::env::var("JR_AUTH_HEADER") { + header +} else { + match auth_method { + "oauth" => { + let (access, _refresh) = crate::api::auth::load_oauth_tokens(&config.active_profile_name)?; + format!("Bearer {access}") + } + _ => { + let (email, token) = crate::api::auth::load_api_token()?; + let encoded = base64::engine::general_purpose::STANDARD + .encode(format!("{email}:{token}")); + format!("Basic {encoded}") + } + } +}; + +// ... +let assets_base_url = if let Some(ref override_url) = test_override { + Some(format!("{}/jsm/assets", override_url.trim_end_matches('/'))) +} else { + profile.cloud_id.as_ref().map(|cloud_id| { + format!("https://api.atlassian.com/ex/jira/{}/jsm/assets", urlencoding::encode(cloud_id)) + }) +}; +``` + +- [ ] **Step 5: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 6: Commit** + +```bash +git add src/api/client.rs src/config.rs +git commit -m "refactor(client): JiraClient consumes active profile" +``` + +--- + +## Task 8: cli/team.rs uses active profile + +**Files:** +- Modify: `src/cli/team.rs` (single small refactor — replace 4 reads of config.global.instance.*) + +- [ ] **Step 1: Read current state** + +```bash +grep -n "config\.global\.instance" src/cli/team.rs +``` + +- [ ] **Step 2: Update all sites to use active_profile** + +Replace each `config.global.instance.` with the equivalent `config.active_profile().` access. Note `active_profile()` returns owned `ProfileConfig` (see Task 3) so chain `.as_ref()` / `.as_deref()` similarly. + +For sites that mutate config (e.g., write back cloud_id/org_id after discovery), update the `[profiles.]` entry instead of `[instance]`: + +```rust +updated_config.global.profiles + .entry(updated_config.active_profile_name.clone()) + .or_insert_with(ProfileConfig::default) + .cloud_id = Some(metadata.cloud_id.clone()); +updated_config.global.profiles + .entry(updated_config.active_profile_name.clone()) + .or_insert_with(ProfileConfig::default) + .org_id = Some(metadata.org_id.clone()); +``` + +- [ ] **Step 3: Run tests** + +```bash +cargo test --lib cli::team +``` + +- [ ] **Step 4: Run fmt + clippy + full suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 5: Commit** + +```bash +git add src/cli/team.rs +git commit -m "refactor(team): use active profile for url/cloud_id/org_id" +``` + +--- + +## Task 9: --profile global CLI flag + +**Files:** +- Modify: `src/cli/mod.rs` (add `profile: Option` to Cli) +- Modify: `src/main.rs` (export to JR_PROFILE_OVERRIDE before Config::load) +- Modify: `src/config.rs` test of CLI flag in resolution chain (already in Task 3) + +- [ ] **Step 1: Add the flag to Cli struct** + +In `src/cli/mod.rs` (around line 18, sibling of `--output`/`--project`): + +```rust +/// Override the active profile (precedence: this flag > JR_PROFILE > config > "default") +#[arg(long, global = true)] +pub profile: Option, +``` + +- [ ] **Step 2: Wire flag in main.rs before Config::load** + +Find the entry point in `src/main.rs` where `Cli::parse()` happens. Right after parsing, before `Config::load()`: + +```rust +let cli = Cli::parse(); + +// Surface --profile to Config::load via env var (avoids changing the public load API). +if let Some(p) = cli.profile.as_deref() { + crate::config::validate_profile_name(p)?; + // SAFETY: main is single-threaded at this point. + unsafe { std::env::set_var("JR_PROFILE_OVERRIDE", p); } +} + +let config = Config::load()?; +``` + +- [ ] **Step 3: Add integration test for precedence** + +Add to `src/config.rs` tests: + +```rust +#[test] +fn config_load_precedence_flag_overrides_env_overrides_field() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let config_path = dir.path().join("config.toml"); + std::fs::write(&config_path, r#" + default_profile = "from-config" + [profiles.from-config] + url = "https://x" + [profiles.from-env] + url = "https://y" + [profiles.from-flag] + url = "https://z" + "#).unwrap(); + + // SAFETY: ENV_MUTEX held across env mutations. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + std::env::set_var("JR_PROFILE", "from-env"); + std::env::set_var("JR_PROFILE_OVERRIDE", "from-flag"); + } + let cfg = Config::load().unwrap(); + assert_eq!(cfg.active_profile_name, "from-flag"); + + unsafe { + std::env::remove_var("JR_PROFILE_OVERRIDE"); + } + let cfg = Config::load().unwrap(); + assert_eq!(cfg.active_profile_name, "from-env"); + + unsafe { + std::env::remove_var("JR_PROFILE"); + } + let cfg = Config::load().unwrap(); + assert_eq!(cfg.active_profile_name, "from-config"); + + unsafe { + std::env::remove_var("XDG_CONFIG_HOME"); + } +} +``` + +- [ ] **Step 4: Run tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 5: Commit** + +```bash +git add src/cli/mod.rs src/main.rs src/config.rs +git commit -m "feat(cli): add --profile global flag with precedence chain" +``` + +--- + +## Task 10: jr auth switch + +**Files:** +- Modify: `src/cli/mod.rs` (add Switch variant to AuthCommand) +- Modify: `src/cli/auth.rs` (handle Switch) + +- [ ] **Step 1: Add Switch variant** + +In `src/cli/mod.rs` `AuthCommand` enum (around line 185): + +```rust +/// Set the default profile in config.toml. +Switch { + /// Profile name to make active. Must already exist in config. + name: String, +}, +``` + +- [ ] **Step 2: Add integration test** + +Add to `tests/auth_profiles.rs` (will be created in Task 15; for now, add to existing tests/auth.rs if present, or stub a test in cli/auth.rs `tests` module). + +For now, add a unit-level test in `src/cli/auth.rs` `tests` module: + +```rust +#[test] +fn switch_to_unknown_profile_returns_error() { + let result = handle_switch_in_memory(GlobalConfig::default(), "ghost"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("unknown profile"), "got: {msg}"); + assert!(msg.contains("ghost"), "got: {msg}"); +} + +#[test] +fn switch_to_known_profile_mutates_default_profile() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("sandbox".to_string(), ProfileConfig::default()); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let mutated = handle_switch_in_memory(global, "sandbox").unwrap(); + assert_eq!(mutated.default_profile.as_deref(), Some("sandbox")); +} +``` + +- [ ] **Step 3: Implement handle_switch + handle_switch_in_memory** + +In `src/cli/auth.rs` add: + +```rust +/// Pure logic for `jr auth switch` — separated for testing without filesystem. +pub(super) fn handle_switch_in_memory( + mut global: GlobalConfig, + target: &str, +) -> anyhow::Result { + crate::config::validate_profile_name(target)?; + if !global.profiles.contains_key(target) { + let known: Vec<&str> = global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { "(none)".into() } else { known.join(", ") } + )).into()); + } + global.default_profile = Some(target.to_string()); + Ok(global) +} + +pub async fn handle_switch(target: &str) -> anyhow::Result<()> { + let mut config = Config::load()?; + config.global = handle_switch_in_memory(config.global, target)?; + config.save_global()?; + output::print_success(&format!("Active profile set to {target:?}")); + Ok(()) +} +``` + +Wire into the dispatch in `main.rs` (or wherever AuthCommand is dispatched). + +- [ ] **Step 4: Run tests + manual smoke** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 5: Commit** + +```bash +git add src/cli/auth.rs src/cli/mod.rs src/main.rs +git commit -m "feat(auth): add jr auth switch subcommand" +``` + +--- + +## Task 11: jr auth list + +**Files:** +- Modify: `src/cli/mod.rs` (add List variant) +- Modify: `src/cli/auth.rs` (handle_list) +- Create: `src/snapshots/jr__cli__auth__tests__list_table.snap` (insta will create on first run) + +- [ ] **Step 1: Add List variant + integration test** + +In `src/cli/mod.rs` `AuthCommand`: + +```rust +/// List all configured profiles. +List, +``` + +- [ ] **Step 2: Add tests** + +In `src/cli/auth.rs` tests: + +```rust +fn three_profile_fixture() -> GlobalConfig { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig { + url: Some("https://acme.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }); + profiles.insert("sandbox".to_string(), ProfileConfig { + url: Some("https://acme-sandbox.atlassian.net".into()), + auth_method: Some("oauth".into()), + cloud_id: Some("xyz-789".into()), + ..ProfileConfig::default() + }); + profiles.insert("staging".to_string(), ProfileConfig { + url: Some("https://acme-staging.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }); + GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + } +} + +#[test] +fn list_table_snapshot() { + let global = three_profile_fixture(); + let rendered = render_list_table(&global, "default"); + insta::assert_snapshot!(rendered); +} + +#[test] +fn list_json_shape() { + let global = three_profile_fixture(); + let json = render_list_json(&global, "default").unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + let arr = parsed.as_array().expect("array"); + assert_eq!(arr.len(), 3); + let active: Vec<&serde_json::Value> = arr.iter() + .filter(|p| p["active"].as_bool() == Some(true)) + .collect(); + assert_eq!(active.len(), 1, "exactly one active"); + assert_eq!(active[0]["name"], "default"); +} +``` + +- [ ] **Step 3: Implement render_list_table and render_list_json** + +In `src/cli/auth.rs`: + +```rust +pub(super) fn render_list_table(global: &GlobalConfig, active: &str) -> String { + let mut rows: Vec> = Vec::new(); + for (name, p) in &global.profiles { + let marker = if name == active { "*" } else { " " }; + let auth = p.auth_method.as_deref().unwrap_or("?"); + let url = p.url.as_deref().unwrap_or("(unset)"); + // STATUS resolution requires keyring inspection — for now, "configured" + // if url present, "no-creds" otherwise. Real status check in Task 13. + let status = if p.url.is_some() { "configured" } else { "no-creds" }; + rows.push(vec![ + format!("{marker} {name}"), + url.to_string(), + auth.to_string(), + status.to_string(), + ]); + } + crate::output::render_table(&["NAME", "URL", "AUTH", "STATUS"], &rows) +} + +pub(super) fn render_list_json(global: &GlobalConfig, active: &str) -> anyhow::Result { + let arr: Vec = global.profiles.iter().map(|(name, p)| { + serde_json::json!({ + "name": name, + "url": p.url, + "auth_method": p.auth_method, + "status": if p.url.is_some() { "configured" } else { "no-creds" }, + "active": name == active, + }) + }).collect(); + Ok(serde_json::to_string_pretty(&arr)?) +} + +pub async fn handle_list(output: &OutputFormat) -> anyhow::Result<()> { + let config = Config::load()?; + let rendered = match output { + OutputFormat::Table => render_list_table(&config.global, &config.active_profile_name), + OutputFormat::Json => render_list_json(&config.global, &config.active_profile_name)?, + }; + println!("{rendered}"); + Ok(()) +} +``` + +Wire into dispatch. + +- [ ] **Step 4: Run tests, accept snapshot** + +```bash +cargo test --lib cli::auth::tests::list 2>&1 | tail -5 +INSTA_UPDATE=auto cargo test --lib cli::auth::tests::list_table_snapshot 2>&1 | tail -5 +``` + +Verify the generated snapshot file has reasonable output before accepting: + +```bash +cat src/snapshots/jr__cli__auth__tests__list_table.snap +``` + +- [ ] **Step 5: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 6: Commit** + +```bash +git add src/cli/auth.rs src/cli/mod.rs src/main.rs src/snapshots/ +git commit -m "feat(auth): add jr auth list subcommand" +``` + +--- + +## Task 12: jr auth login --profile + --url + +**Files:** +- Modify: `src/cli/mod.rs` (Login variant gains profile + url) +- Modify: `src/cli/auth.rs` (login_token / login_oauth take profile) +- Modify: `src/cli/init.rs` if it directly calls login functions + +- [ ] **Step 1: Update AuthCommand::Login** + +In `src/cli/mod.rs`: + +```rust +Login { + /// Profile to log in to (creates it if absent). Defaults to active profile. + #[arg(long)] + profile: Option, + /// Jira instance URL (required when creating a new profile under --no-input). + #[arg(long)] + url: Option, + /// Use OAuth 2.0 instead of API token. + #[arg(long)] + oauth: bool, + // ... existing flags (email, token, client_id, client_secret) unchanged ... +}, +``` + +- [ ] **Step 2: Add tests for the new login_or_create logic** + +```rust +#[test] +fn login_create_new_profile_no_input_requires_url() { + let global = GlobalConfig::default(); + let result = prepare_login_target(global, Some("sandbox"), None, true); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("--url required"), "got: {msg}"); +} + +#[test] +fn login_create_new_profile_with_url_succeeds() { + let global = GlobalConfig::default(); + let (mutated, target) = prepare_login_target( + global, Some("sandbox"), Some("https://sandbox.example"), true + ).unwrap(); + assert_eq!(target, "sandbox"); + assert_eq!( + mutated.profiles["sandbox"].url.as_deref(), + Some("https://sandbox.example") + ); +} + +#[test] +fn login_existing_profile_with_url_updates_url() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig { + url: Some("https://old.example".into()), + ..ProfileConfig::default() + }); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let (mutated, target) = prepare_login_target( + global, Some("default"), Some("https://new.example"), true + ).unwrap(); + assert_eq!(target, "default"); + assert_eq!( + mutated.profiles["default"].url.as_deref(), + Some("https://new.example") + ); +} +``` + +- [ ] **Step 3: Implement prepare_login_target** + +```rust +/// Pure logic for ensuring a target profile exists with the given URL. +/// Returns (updated_global, resolved_profile_name). +pub(super) fn prepare_login_target( + mut global: GlobalConfig, + profile_arg: Option<&str>, + url_arg: Option<&str>, + no_input: bool, +) -> anyhow::Result<(GlobalConfig, String)> { + let target = match profile_arg { + Some(name) => { + crate::config::validate_profile_name(name)?; + name.to_string() + } + None => global + .default_profile + .clone() + .unwrap_or_else(|| "default".to_string()), + }; + + let exists = global.profiles.contains_key(&target); + let entry = global.profiles + .entry(target.clone()) + .or_insert_with(ProfileConfig::default); + + if let Some(url) = url_arg { + // Trim trailing slash matches the convention used elsewhere + entry.url = Some(url.trim_end_matches('/').to_string()); + } else if !exists && no_input { + return Err(JrError::UserError( + "--url required when creating a new profile under --no-input".into() + ).into()); + } + + if global.default_profile.is_none() { + global.default_profile = Some(target.clone()); + } + + Ok((global, target)) +} +``` + +- [ ] **Step 4: Refactor login_token / login_oauth to use prepare_login_target** + +Wire `handle_login` to: +1. Parse args +2. Run `prepare_login_target(...)` +3. Save mutated global to config.toml +4. Reload config, switch active profile temporarily for the login flow if needed +5. Call existing login flow (which now stores OAuth tokens with the resolved profile name) + +For brevity, the implementation reuses `login_token(profile, email, token, no_input)` and `login_oauth(profile, client_id, client_secret, no_input)` — both gain `profile` as their first arg. + +- [ ] **Step 5: Update existing login_token/login_oauth signatures** + +```rust +pub async fn login_token( + profile: &str, + email: Option, + token: Option, + no_input: bool, +) -> Result<()> { + // ... existing logic ... + // Final store: shared API token (always under flat keys) + auth::store_api_token(&email, &token)?; + Ok(()) +} + +pub async fn login_oauth( + profile: &str, + client_id: Option, + client_secret: Option, + no_input: bool, +) -> Result<()> { + // ... existing logic ... + // load_oauth_app_credentials still returns shared client_id/client_secret. + // oauth_login(profile, &client_id, &client_secret, &scopes) already takes profile (Task 5). + let result = api::auth::oauth_login(profile, &client_id, &client_secret, &scopes).await?; + // Persist site info into the named profile in config + let mut config = Config::load()?; + let p = config.global.profiles.entry(profile.to_string()).or_default(); + p.url = Some(result.site_url); + p.cloud_id = Some(result.cloud_id); + p.auth_method = Some("oauth".into()); + config.save_global()?; + Ok(()) +} +``` + +Update `cli/init.rs` callers to pass `"default"` as the profile. + +- [ ] **Step 6: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/cli/mod.rs src/cli/auth.rs src/cli/init.rs +git commit -m "feat(auth): jr auth login supports --profile and --url" +``` + +--- + +## Task 13: jr auth status, refresh, and logout (per-profile + per-profile status) + +**Files:** +- Modify: `src/cli/mod.rs` (Status, Refresh gain profile; new Logout variant) +- Modify: `src/cli/auth.rs` (handle_status takes profile; handle_logout new) + +- [ ] **Step 1: Update AuthCommand variants** + +In `src/cli/mod.rs`: + +```rust +Status { + /// Profile to show status for. Defaults to active profile. + #[arg(long)] + profile: Option, +}, +Refresh { + /// Profile to refresh credentials for. Defaults to active profile. + #[arg(long)] + profile: Option, + // ... existing flags ... +}, +/// Clear OAuth tokens for a profile (profile entry stays in config). +/// Shared API-token credential is NEVER touched. +Logout { + /// Profile to log out of. Defaults to active profile. + #[arg(long)] + profile: Option, +}, +``` + +- [ ] **Step 2: Add tests** + +```rust +#[test] +fn handle_logout_clears_only_target_profile_tokens() { + // logic-only test — actual keyring touch is covered by integration tests + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig::default()); + profiles.insert("sandbox".to_string(), ProfileConfig::default()); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let target = resolve_logout_target(&global, None, "default"); + assert_eq!(target, "default"); + + let target = resolve_logout_target(&global, Some("sandbox"), "default"); + assert_eq!(target, "sandbox"); +} +``` + +- [ ] **Step 3: Implement helpers** + +```rust +pub(super) fn resolve_logout_target( + _global: &GlobalConfig, + profile_arg: Option<&str>, + active: &str, +) -> String { + profile_arg.unwrap_or(active).to_string() +} + +pub async fn handle_logout(profile_arg: Option<&str>) -> anyhow::Result<()> { + let config = Config::load()?; + let target = resolve_logout_target(&config.global, profile_arg, &config.active_profile_name); + crate::config::validate_profile_name(&target)?; + api::auth::clear_profile_creds(&target)?; + output::print_success(&format!("Logged out of profile {target:?}")); + Ok(()) +} +``` + +- [ ] **Step 4: Update status / refresh to take profile** + +Existing `pub async fn status() -> Result<()>` becomes: + +```rust +pub async fn status(profile_arg: Option<&str>) -> Result<()> { + let config = Config::load()?; + let target = profile_arg.unwrap_or(&config.active_profile_name).to_string(); + // ... existing status logic, but report for `target` ... + Ok(()) +} +``` + +Same shape change for `refresh_credentials` — it gains a `profile_arg` param and threads it through. + +- [ ] **Step 5: Wire into dispatch in main.rs** + +- [ ] **Step 6: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/cli/mod.rs src/cli/auth.rs src/main.rs +git commit -m "feat(auth): jr auth status/refresh/logout support --profile" +``` + +--- + +## Task 14: jr auth remove + +**Files:** +- Modify: `src/cli/mod.rs` (Remove variant) +- Modify: `src/cli/auth.rs` (handle_remove) + +- [ ] **Step 1: Add Remove variant** + +```rust +/// Permanently delete a profile (config + cache + per-profile OAuth tokens). +/// Shared credentials are NEVER touched. +Remove { + /// Profile name to remove. Cannot be the active profile — + /// switch first with `jr auth switch`. + name: String, +}, +``` + +- [ ] **Step 2: Tests** + +```rust +#[test] +fn remove_active_profile_returns_error() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig::default()); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let result = handle_remove_in_memory(global, "default", "default"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("cannot remove active"), "got: {msg}"); +} + +#[test] +fn remove_unknown_profile_returns_error() { + let global = GlobalConfig { + default_profile: Some("default".into()), + ..GlobalConfig::default() + }; + let result = handle_remove_in_memory(global, "ghost", "default"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("unknown profile"), "got: {msg}"); +} + +#[test] +fn remove_existing_non_active_profile_succeeds() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("default".to_string(), ProfileConfig::default()); + profiles.insert("sandbox".to_string(), ProfileConfig::default()); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let mutated = handle_remove_in_memory(global, "sandbox", "default").unwrap(); + assert!(!mutated.profiles.contains_key("sandbox")); + assert!(mutated.profiles.contains_key("default")); +} +``` + +- [ ] **Step 3: Implement handle_remove + handle_remove_in_memory** + +```rust +pub(super) fn handle_remove_in_memory( + mut global: GlobalConfig, + target: &str, + active: &str, +) -> anyhow::Result { + crate::config::validate_profile_name(target)?; + if !global.profiles.contains_key(target) { + let known: Vec<&str> = global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { "(none)".into() } else { known.join(", ") } + )).into()); + } + if target == active { + return Err(JrError::UserError(format!( + "cannot remove active profile {target:?}; switch first with \"jr auth switch \"" + )).into()); + } + global.profiles.remove(target); + Ok(global) +} + +pub async fn handle_remove(target: &str, no_input: bool) -> anyhow::Result<()> { + let mut config = Config::load()?; + crate::config::validate_profile_name(target)?; + + if !no_input { + let confirm = dialoguer::Confirm::new() + .with_prompt(format!( + "Permanently remove profile {target:?}? \ + This deletes its config entry, cache, and OAuth tokens. \ + Shared credentials remain." + )) + .default(false) + .interact()?; + if !confirm { + output::print_warning("Aborted."); + return Ok(()); + } + } + + config.global = handle_remove_in_memory(config.global, target, &config.active_profile_name)?; + config.save_global()?; + let _ = api::auth::clear_profile_creds(target); + let _ = crate::cache::clear_profile_cache(target); + output::print_success(&format!("Removed profile {target:?}")); + Ok(()) +} +``` + +- [ ] **Step 4: Wire into dispatch + add `print_warning` helper if absent** + +Check `src/output.rs` for `print_warning`. If missing, add: + +```rust +pub fn print_warning(msg: &str) { + eprintln!("warning: {msg}"); +} +``` + +- [ ] **Step 5: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 6: Commit** + +```bash +git add src/cli/mod.rs src/cli/auth.rs src/output.rs src/main.rs +git commit -m "feat(auth): add jr auth remove subcommand" +``` + +--- + +## Task 15: jr init multi-profile awareness + integration tests + +**Files:** +- Modify: `src/cli/init.rs` (prompt before adding profile) +- Create: `tests/auth_profiles.rs` +- Create: `tests/migration_legacy.rs` + +- [ ] **Step 1: Update jr init** + +In `src/cli/init.rs::handle()`, after loading existing config, before re-running setup: + +```rust +let existing = Config::load().ok(); +if let Some(c) = existing.as_ref() { + if !c.global.profiles.is_empty() { + let names: Vec = c.global.profiles.keys().cloned().collect(); + eprintln!("Profiles already configured: {}", names.join(", ")); + let add = Confirm::new() + .with_prompt("Add another profile?") + .default(false) + .interact() + .context("failed to prompt for additional profile")?; + if !add { + return Ok(()); + } + // Rest of jr init flow runs against a NEW profile name (prompted below). + let profile_name: String = Input::new() + .with_prompt("Name for the new profile") + .interact_text() + .context("failed to read profile name")?; + crate::config::validate_profile_name(&profile_name)?; + // Set as active for the duration of this init run so all writes target it. + // SAFETY: jr init is single-threaded. + unsafe { std::env::set_var("JR_PROFILE_OVERRIDE", &profile_name); } + } +} +``` + +- [ ] **Step 2: Create tests/auth_profiles.rs** + +```rust +//! Integration tests for multi-profile auth workflows. +mod common; + +use assert_cmd::prelude::*; +use std::process::Command; +use tempfile::TempDir; + +fn jr() -> Command { + let mut cmd = Command::cargo_bin("jr").unwrap(); + cmd.env_remove("JR_PROFILE") + .env_remove("JR_PROFILE_OVERRIDE"); + cmd +} + +fn fresh_config_dir() -> (TempDir, std::path::PathBuf) { + let dir = TempDir::new().unwrap(); + let cfg = dir.path().join("jr").join("config.toml"); + std::fs::create_dir_all(cfg.parent().unwrap()).unwrap(); + (dir, cfg) +} + +#[test] +fn auth_switch_unknown_profile_exits_64() { + let (dir, _path) = fresh_config_dir(); + jr() + .env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "switch", "ghost"]) + .assert() + .failure() + .code(64); +} + +#[test] +fn auth_list_shows_no_profiles_for_fresh_install() { + let (dir, _path) = fresh_config_dir(); + jr() + .env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "list", "--output", "json"]) + .assert() + .success() + .stdout(predicates::str::contains("[]")); +} + +#[test] +fn auth_remove_active_profile_exits_64() { + let (dir, path) = fresh_config_dir(); + std::fs::write(&path, r#" + default_profile = "default" + [profiles.default] + url = "https://x.example" + auth_method = "api_token" + "#).unwrap(); + + jr() + .env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "remove", "default", "--no-input"]) + .assert() + .failure() + .code(64) + .stderr(predicates::str::contains("cannot remove active")); +} + +#[test] +fn precedence_flag_overrides_env_overrides_config() { + let (dir, path) = fresh_config_dir(); + std::fs::write(&path, r#" + default_profile = "from-config" + [profiles.from-config] + url = "https://from-config.example" + [profiles.from-env] + url = "https://from-env.example" + [profiles.from-flag] + url = "https://from-flag.example" + "#).unwrap(); + + // Flag wins + let out = jr() + .env("XDG_CONFIG_HOME", dir.path()) + .env("JR_PROFILE", "from-env") + .args(["--profile", "from-flag", "auth", "list", "--output", "json"]) + .output() + .unwrap(); + assert!(out.status.success()); + let stdout = String::from_utf8_lossy(&out.stdout); + let parsed: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + let active: Vec<&serde_json::Value> = parsed.as_array().unwrap().iter() + .filter(|p| p["active"].as_bool() == Some(true)) + .collect(); + assert_eq!(active[0]["name"], "from-flag"); +} +``` + +- [ ] **Step 3: Create tests/migration_legacy.rs** + +```rust +//! Legacy [instance] → [profiles.default] migration tests. + +use std::fs; +use tempfile::TempDir; + +#[test] +fn legacy_instance_block_migrated_in_memory() { + let dir = TempDir::new().unwrap(); + let cfg_path = dir.path().join("jr").join("config.toml"); + fs::create_dir_all(cfg_path.parent().unwrap()).unwrap(); + fs::write(&cfg_path, r#" + [instance] + url = "https://legacy.atlassian.net" + auth_method = "api_token" + cloud_id = "legacy-1" + org_id = "org-1" + + [fields] + team_field_id = "customfield_99" + story_points_field_id = "customfield_42" + + [defaults] + output = "json" + "#).unwrap(); + + // SAFETY: test runs single-threaded under cargo test --test + unsafe { std::env::set_var("XDG_CONFIG_HOME", dir.path()); } + let config = jr::config::Config::load().unwrap(); + unsafe { std::env::remove_var("XDG_CONFIG_HOME"); } + + // Migration ran + assert_eq!(config.active_profile_name, "default"); + assert!(config.global.profiles.contains_key("default")); + let p = &config.global.profiles["default"]; + assert_eq!(p.url.as_deref(), Some("https://legacy.atlassian.net")); + assert_eq!(p.cloud_id.as_deref(), Some("legacy-1")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_99")); + assert_eq!(p.story_points_field_id.as_deref(), Some("customfield_42")); + + // [defaults] preserved as global (not migrated to profile) + assert_eq!(config.global.defaults.output, "json"); + + // On-disk file is now in new shape + let on_disk = fs::read_to_string(&cfg_path).unwrap(); + assert!(on_disk.contains("default_profile")); + assert!(on_disk.contains("[profiles.default]")); + insta::assert_snapshot!( + on_disk + .replace(dir.path().to_str().unwrap(), "") + .as_str() + ); +} + +#[test] +fn migration_is_idempotent() { + let dir = TempDir::new().unwrap(); + let cfg_path = dir.path().join("jr").join("config.toml"); + fs::create_dir_all(cfg_path.parent().unwrap()).unwrap(); + fs::write(&cfg_path, r#" + [instance] + url = "https://x" + auth_method = "api_token" + "#).unwrap(); + + unsafe { std::env::set_var("XDG_CONFIG_HOME", dir.path()); } + let _ = jr::config::Config::load().unwrap(); + let after_first = fs::read_to_string(&cfg_path).unwrap(); + let _ = jr::config::Config::load().unwrap(); + let after_second = fs::read_to_string(&cfg_path).unwrap(); + unsafe { std::env::remove_var("XDG_CONFIG_HOME"); } + + assert_eq!(after_first, after_second, "second load should not modify file"); +} +``` + +- [ ] **Step 4: Add lib re-exports if missing** + +In `src/lib.rs`, ensure `pub mod config;` is exposed so integration tests can use `jr::config::Config`. + +- [ ] **Step 5: Run integration tests** + +```bash +cargo test --test auth_profiles --test migration_legacy 2>&1 | tail -10 +``` + +- [ ] **Step 6: Run full suite** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +Approve any insta snapshots: + +```bash +cargo insta review +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/cli/init.rs tests/auth_profiles.rs tests/migration_legacy.rs src/lib.rs src/snapshots/ +git commit -m "feat(init): multi-profile awareness; add integration tests" +``` + +--- + +## Task 16: Cleanup — remove legacy InstanceConfig + FieldsConfig fields + +**Files:** +- Modify: `src/config.rs` (drop the legacy serde fields once migration is wired) + +This is a deferrable cleanup — only safe AFTER all release channels have run the migration once (so on-disk configs are guaranteed in new shape). + +For this PR: keep the legacy fields with `#[serde(skip_serializing)]` so reading still works (in case migration is interrupted), but new writes never include them. A future PR removes them entirely. + +- [ ] **Step 1: Add `#[serde(skip_serializing)]` to legacy fields** + +```rust +#[derive(Debug, Deserialize, Serialize, Default)] +pub struct GlobalConfig { + #[serde(default)] + pub default_profile: Option, + #[serde(default)] + pub profiles: std::collections::BTreeMap, + #[serde(default, skip_serializing)] + pub instance: InstanceConfig, + #[serde(default, skip_serializing)] + pub fields: FieldsConfig, + #[serde(default)] + pub defaults: DefaultsConfig, +} +``` + +- [ ] **Step 2: Verify migration_legacy snapshot still has no [instance] / [fields] in output** + +```bash +cargo test --test migration_legacy +cat src/snapshots/migration_legacy__legacy_instance_block_migrated_in_memory.snap +``` + +Expected: snapshot shows `[profiles.default]` but no `[instance]` or `[fields]`. + +- [ ] **Step 3: Run fmt + clippy + tests** + +```bash +cargo fmt --all -- --check && cargo clippy --all-targets -- -D warnings && cargo test 2>&1 | tail -3 +``` + +- [ ] **Step 4: Commit** + +```bash +git add src/config.rs +git commit -m "refactor(config): stop serializing legacy [instance]/[fields] blocks" +``` + +--- + +## Final Verification + +- [ ] **Run full CI-equivalent check set** + +```bash +cargo fmt --all -- --check +cargo clippy --all-targets -- -D warnings +cargo test +JR_RUN_KEYRING_TESTS=1 cargo test --lib api::auth::tests -- --ignored +``` + +All green. + +- [ ] **Manual smoke test** + +```bash +# Build +cargo build --release + +# Inspect a fresh-shape config (use a tmp dir) +TMPDIR_TEST=$(mktemp -d) +XDG_CONFIG_HOME=$TMPDIR_TEST ./target/release/jr auth list --output json +# Expected: [] + +# Set up two profiles manually +mkdir -p "$TMPDIR_TEST/jr" +cat > "$TMPDIR_TEST/jr/config.toml" <<'EOF' +default_profile = "prod" + +[profiles.prod] +url = "https://acme.atlassian.net" +auth_method = "api_token" + +[profiles.sandbox] +url = "https://acme-sandbox.atlassian.net" +auth_method = "api_token" +EOF + +XDG_CONFIG_HOME=$TMPDIR_TEST ./target/release/jr auth list +# Expected: table showing both, * on prod + +XDG_CONFIG_HOME=$TMPDIR_TEST ./target/release/jr --profile sandbox auth list --output json +# Expected: JSON, sandbox marked active + +XDG_CONFIG_HOME=$TMPDIR_TEST ./target/release/jr auth switch sandbox +XDG_CONFIG_HOME=$TMPDIR_TEST ./target/release/jr auth list +# Expected: * now on sandbox +``` + +- [ ] **Push and create PR** + +```bash +git push -u origin feat/multi-profile-auth +gh pr create --base develop --title "feat: multi-profile authentication" --body "$(cat <<'EOF' +## Summary + +- Lets jr target multiple Atlassian Cloud sites from one install +- `jr auth switch ` flips active profile persistently +- Shared classic API token across profiles (account-level credential) +- Per-profile OAuth tokens (cloudId-scoped) +- Auto-migration of legacy `[instance]` config; lazy keyring migration + +Spec: docs/specs/multi-profile-auth.md +Plan: docs/superpowers/plans/2026-04-24-multi-profile-auth.md + +## Test plan + +- [x] `cargo fmt --all -- --check` passes +- [x] `cargo clippy --all-targets -- -D warnings` passes +- [x] `cargo test` passes (all suites) +- [x] `JR_RUN_KEYRING_TESTS=1 cargo test -- --ignored` passes locally +- [x] Manual smoke: `jr auth list / switch / login --profile / remove` +- [x] Migration smoke: legacy [instance] config migrates on first load +EOF +)" +gh api repos/Zious11/jira-cli/pulls//requested_reviewers --method POST -f 'reviewers[]=copilot-pull-request-reviewer[bot]' +``` + +--- + +## Self-Review + +**Spec coverage:** +- ✓ Config Schema → Tasks 2, 3, 4, 16 +- ✓ Active-Profile Resolution → Tasks 3, 9 +- ✓ Profile Name Validation → Task 1 +- ✓ Keyring Layout → Task 5 +- ✓ `:` Separator Safety (validation enforces) → Task 1 +- ✓ Cache Layout → Task 6 +- ✓ CLI Surface → Tasks 9 (--profile), 10 (switch), 11 (list), 12 (login), 13 (status/refresh/logout), 14 (remove), 15 (init) +- ✓ Migration → Tasks 4 (TOML), 5 (lazy keyring) +- ✓ Error Handling → Tasks 1, 3, 9, 10, 13, 14 +- ✓ Testing — unit/integration/snapshot/keyring-gated → Tasks 1–15 +- ✓ Concurrency & Cross-Platform Notes — surfaced in spec; no test coverage required (pre-existing limitations) +- ✓ Out of Scope items NOT implemented (correct) + +**Placeholders:** None remaining. + +**Type consistency:** +- `ProfileConfig` defined in Task 2; consumed in Tasks 3, 4, 5, 6, 7, 8, 10, 11, 12, 13, 14, 15. +- `validate_profile_name` defined in Task 1; consumed in Tasks 9, 10, 12, 13, 14. +- `cache_dir(profile)` defined in Task 6; consumed via call sites in Tasks 6, 14. +- `store_oauth_tokens(profile, ...)` / `load_oauth_tokens(profile)` defined in Task 5; consumed in Tasks 7, 12, 13. +- `clear_profile_creds(profile)` defined in Task 5; consumed in Tasks 13, 14. +- `clear_profile_cache(profile)` defined in Task 6; consumed in Task 14. +- `JR_PROFILE_OVERRIDE` env var: defined in Task 3 (consumed by Config::load) + populated in Task 9 (main.rs from --profile flag) + Task 15 (jr init for additional profile). +- `Config::active_profile_name` field added in Task 3; consumed throughout. +- `Config::active_profile()` (returns owned ProfileConfig) and `Config::active_profile_or_err()` defined in Task 3; consumed in Task 7, 8. + +All consistent. diff --git a/src/api/assets/linked.rs b/src/api/assets/linked.rs index 8b6356dd..7a11404d 100644 --- a/src/api/assets/linked.rs +++ b/src/api/assets/linked.rs @@ -10,12 +10,13 @@ use crate::types::assets::LinkedAsset; /// Get CMDB fields (id, name pairs), using cache when available. pub async fn get_or_fetch_cmdb_fields(client: &JiraClient) -> Result> { - if let Some(cached) = cache::read_cmdb_fields_cache()? { + let profile = client.profile_name(); + if let Some(cached) = cache::read_cmdb_fields_cache(profile)? { return Ok(cached.fields); } let fields = client.find_cmdb_fields().await?; - let _ = cache::write_cmdb_fields_cache(&fields); + let _ = cache::write_cmdb_fields_cache(profile, &fields); Ok(fields) } diff --git a/src/api/assets/objects.rs b/src/api/assets/objects.rs index f6d2e841..f3b3527c 100644 --- a/src/api/assets/objects.rs +++ b/src/api/assets/objects.rs @@ -162,9 +162,10 @@ pub async fn enrich_search_attributes( let mut attr_map: HashMap = HashMap::new(); + let profile = client.profile_name(); for type_id in &type_ids { - // Try cache first - let attrs = match cache::read_object_type_attr_cache(type_id) { + // Try cache first. + let attrs = match cache::read_object_type_attr_cache(profile, type_id) { Ok(Some(cached)) => cached, _ => { // Cache miss — fetch from API @@ -185,7 +186,7 @@ pub async fn enrich_search_attributes( }) .collect(); // Best-effort cache write - let _ = cache::write_object_type_attr_cache(type_id, &cached); + let _ = cache::write_object_type_attr_cache(profile, type_id, &cached); cached } Err(_) => { diff --git a/src/api/assets/workspace.rs b/src/api/assets/workspace.rs index f05ce753..458597dc 100644 --- a/src/api/assets/workspace.rs +++ b/src/api/assets/workspace.rs @@ -17,7 +17,8 @@ struct WorkspaceEntry { /// The discovery endpoint returns a paginated response with workspace entries. /// In practice there's only one workspace per site. pub async fn get_or_fetch_workspace_id(client: &JiraClient) -> Result { - if let Some(cached) = cache::read_workspace_cache()? { + let profile = client.profile_name(); + if let Some(cached) = cache::read_workspace_cache(profile)? { return Ok(cached.workspace_id); } @@ -51,7 +52,7 @@ pub async fn get_or_fetch_workspace_id(client: &JiraClient) -> Result { ) })?; - let _ = cache::write_workspace_cache(&workspace_id); + let _ = cache::write_workspace_cache(profile, &workspace_id); Ok(workspace_id) } diff --git a/src/api/auth.rs b/src/api/auth.rs index 9b4ff972..45aee57e 100644 --- a/src/api/auth.rs +++ b/src/api/auth.rs @@ -21,8 +21,18 @@ fn service_name() -> String { /// Key names stored in the system keychain. const KEY_EMAIL: &str = "email"; const KEY_API_TOKEN: &str = "api-token"; -const KEY_OAUTH_ACCESS: &str = "oauth-access-token"; -const KEY_OAUTH_REFRESH: &str = "oauth-refresh-token"; +/// Pre-multi-profile flat OAuth keys. Read-only on the migration path inside +/// [`load_oauth_tokens`] for the `"default"` profile; new writes always use +/// the namespaced `:oauth-*-token` keys. +const KEY_OAUTH_ACCESS_LEGACY: &str = "oauth-access-token"; +const KEY_OAUTH_REFRESH_LEGACY: &str = "oauth-refresh-token"; + +fn oauth_access_key(profile: &str) -> String { + format!("{profile}:oauth-access-token") +} +fn oauth_refresh_key(profile: &str) -> String { + format!("{profile}:oauth-refresh-token") +} /// Default OAuth 2.0 scopes used when `oauth_scopes` is not set in /// config.toml. Matches Atlassian's "classic" scope recommendation for @@ -55,23 +65,105 @@ pub fn load_api_token() -> Result<(String, String)> { Ok((email, token)) } -/// Store OAuth 2.0 access and refresh tokens in the system keychain. -pub fn store_oauth_tokens(access: &str, refresh: &str) -> Result<()> { - entry(KEY_OAUTH_ACCESS)?.set_password(access)?; - entry(KEY_OAUTH_REFRESH)?.set_password(refresh)?; +/// Store OAuth 2.0 access and refresh tokens scoped to a profile. +/// +/// Tokens are written to the namespaced keys `:oauth-access-token` +/// and `:oauth-refresh-token` so multiple Jira sites can coexist +/// in a single keychain. +pub fn store_oauth_tokens(profile: &str, access: &str, refresh: &str) -> Result<()> { + entry(&oauth_access_key(profile))?.set_password(access)?; + entry(&oauth_refresh_key(profile))?.set_password(refresh)?; Ok(()) } -/// Load OAuth 2.0 access and refresh tokens from the system keychain. +/// Load OAuth 2.0 access and refresh tokens for a profile. +/// /// Returns `(access_token, refresh_token)`. -pub fn load_oauth_tokens() -> Result<(String, String)> { - let access = entry(KEY_OAUTH_ACCESS)? - .get_password() - .context("No stored OAuth token — run \"jr auth login\"")?; - let refresh = entry(KEY_OAUTH_REFRESH)? - .get_password() - .context("No stored OAuth refresh token — run \"jr auth login\"")?; - Ok((access, refresh)) +/// +/// For the `"default"` profile, falls back to the legacy flat keys +/// (`oauth-access-token` / `oauth-refresh-token`, the pre-multi-profile +/// layout) and opportunistically migrates them to the new namespaced keys +/// on read: writes the namespaced copies, then deletes the legacy ones. +/// This means existing single-profile users transparently survive the +/// upgrade without re-authenticating. Non-`"default"` profiles never +/// inherit legacy keys — that would silently cross-pollinate credentials +/// across distinct Jira sites. +pub fn load_oauth_tokens(profile: &str) -> Result<(String, String)> { + let access_key = oauth_access_key(profile); + let refresh_key = oauth_refresh_key(profile); + let access = read_keyring_optional(&access_key)?; + let refresh = read_keyring_optional(&refresh_key)?; + + match (access, refresh) { + (Some(a), Some(r)) => Ok((a, r)), + (None, None) => { + // Both namespaced keys absent — try legacy fallback for the + // "default" profile (lazy-migration path). Non-default + // profiles never inherit legacy keys; that would silently + // cross-pollinate credentials across distinct Jira sites. + if profile == "default" { + let legacy_access = read_keyring_optional(KEY_OAUTH_ACCESS_LEGACY)?; + let legacy_refresh = read_keyring_optional(KEY_OAUTH_REFRESH_LEGACY)?; + if let (Some(a), Some(r)) = (legacy_access, legacy_refresh) { + store_oauth_tokens("default", &a, &r)?; + let _ = entry(KEY_OAUTH_ACCESS_LEGACY)?.delete_credential(); + let _ = entry(KEY_OAUTH_REFRESH_LEGACY)?.delete_credential(); + return Ok((a, r)); + } + } + Err(anyhow::anyhow!( + "No stored OAuth token for profile {profile:?} — \ + run \"jr auth login --profile {profile}\"" + )) + } + // Partial state: one half of the namespaced pair is missing. For + // the "default" profile, try recovering from a still-intact + // legacy pair before erroring — this handles interrupted lazy + // migrations and partial writes that left the namespaced entries + // inconsistent while the legacy flat keys still contain valid + // tokens. Non-default profiles must NEVER inherit legacy keys + // (that would cross-pollinate credentials across Jira sites). + // + // If the legacy pair isn't complete either, surface the partial + // state with explicit recovery instructions rather than masking + // the corruption with a generic "no token" message. + _ => { + if profile == "default" { + let legacy_access = read_keyring_optional(KEY_OAUTH_ACCESS_LEGACY)?; + let legacy_refresh = read_keyring_optional(KEY_OAUTH_REFRESH_LEGACY)?; + if let (Some(a), Some(r)) = (legacy_access, legacy_refresh) { + store_oauth_tokens("default", &a, &r)?; + let _ = entry(KEY_OAUTH_ACCESS_LEGACY)?.delete_credential(); + let _ = entry(KEY_OAUTH_REFRESH_LEGACY)?.delete_credential(); + return Ok((a, r)); + } + } + Err(anyhow::anyhow!( + "OAuth keychain entries for profile {profile:?} are partial \ + (one of access/refresh present, the other missing). \ + Run \"jr auth logout --profile {profile}\" then \ + \"jr auth login --profile {profile}\" to restore a clean state." + )) + } + } +} + +/// Read an optional keychain entry, distinguishing "not present" (`NoEntry`) +/// from real backend failures. +/// +/// `keyring::Entry::get_password().ok()` collapses every error to `None` — +/// so a permission-denied, locked-keyring, or platform error looks identical +/// to a missing entry. That silently triggers fallbacks (legacy migration, +/// generic "no token" messages) and hides the real problem from the user. +/// This helper instead matches `keyring::Error::NoEntry` as the only +/// "absent" case and propagates everything else up the call stack so the +/// CLI can surface actionable diagnostics. +fn read_keyring_optional(key: &str) -> Result> { + match entry(key)?.get_password() { + Ok(v) => Ok(Some(v)), + Err(keyring::Error::NoEntry) => Ok(None), + Err(e) => Err(e.into()), + } } /// Store OAuth app credentials (client_id and client_secret) in the system keychain. @@ -98,7 +190,60 @@ pub fn load_oauth_app_credentials() -> Result<(String, String)> { Ok((id, secret)) } -/// Remove all stored credentials from the system keychain. +/// Clear OAuth tokens for a single profile (other profiles + shared keys +/// such as email / api-token / oauth_client_id are untouched). +/// +/// For the `"default"` profile, this also deletes the legacy flat OAuth +/// keys (`oauth-access-token` / `oauth-refresh-token`). Without that step, +/// a user mid-migration would see `jr auth logout --profile default` +/// "succeed" while the legacy keys remained — and the next +/// `load_oauth_tokens("default")` would lazy-migrate them back into the +/// namespaced slots, effectively undoing the logout. Non-`"default"` +/// profiles never inherit legacy keys, so this clause stays scoped to +/// `"default"` to avoid stomping on another profile's migration window. +/// +/// `NoEntry` results are treated as success (the entry was already absent). +/// Any other failure (permission denied, ACL mismatch, platform error) is +/// aggregated and returned so callers can surface partial-failure details +/// rather than reporting success while stale entries remain. +pub fn clear_profile_creds(profile: &str) -> Result<()> { + let mut failures: Vec = Vec::new(); + let mut keys: Vec = vec![oauth_access_key(profile), oauth_refresh_key(profile)]; + // For the "default" profile, also clear the legacy flat OAuth keys + // that load_oauth_tokens("default") would otherwise lazy-migrate + // back into existence on the next read — defeating logout. + if profile == "default" { + keys.push(KEY_OAUTH_ACCESS_LEGACY.to_string()); + keys.push(KEY_OAUTH_REFRESH_LEGACY.to_string()); + } + for key in keys { + match entry(&key) { + Ok(e) => match e.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => {} + Err(err) => failures.push(format!("{key}: {err}")), + }, + Err(err) => failures.push(format!("{key}: {err}")), + } + } + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow::anyhow!( + "failed to clear {} keychain entries: {}", + failures.len(), + failures.join("; ") + )) + } +} + +/// Remove shared credentials and OAuth tokens for every listed profile from +/// the system keychain. +/// +/// Always clears the shared / single-tenant keys (`email`, `api-token`, +/// `oauth_client_id`, `oauth_client_secret`) plus the legacy flat OAuth +/// keys. Per-profile OAuth tokens (`:oauth-*-token`) are cleared +/// only for the profiles in `profiles` — callers know their own profile +/// list (from config) and pass it in. /// /// `NoEntry` results are treated as success (the entry was already absent, /// which is the expected case on a fresh install or after a prior clear). @@ -106,17 +251,30 @@ pub fn load_oauth_app_credentials() -> Result<(String, String)> { /// aggregated and returned so callers can decide whether to proceed — for /// example, `jr auth refresh` needs to know if the clear actually happened /// before reporting the refresh as successful. -pub fn clear_credentials() -> Result<()> { +pub fn clear_all_credentials(profiles: &[&str]) -> Result<()> { let mut failures: Vec = Vec::new(); - for key in [ - KEY_EMAIL, - KEY_API_TOKEN, - KEY_OAUTH_ACCESS, - KEY_OAUTH_REFRESH, - "oauth_client_id", - "oauth_client_secret", - ] { - match entry(key) { + let mut keys: Vec = vec![ + KEY_EMAIL.to_string(), + KEY_API_TOKEN.to_string(), + "oauth_client_id".to_string(), + "oauth_client_secret".to_string(), + ]; + // Legacy flat OAuth keys belong to the "default" profile's + // lazy-migration path. Only delete them when the caller is + // explicitly clearing "default" — otherwise `jr auth refresh + // --profile sandbox` (api_token flow) on a not-yet-migrated + // legacy install would unconditionally wipe the default + // profile's intact-but-unmigrated OAuth tokens. + if profiles.contains(&"default") { + keys.push(KEY_OAUTH_ACCESS_LEGACY.to_string()); + keys.push(KEY_OAUTH_REFRESH_LEGACY.to_string()); + } + for profile in profiles { + keys.push(oauth_access_key(profile)); + keys.push(oauth_refresh_key(profile)); + } + for key in keys { + match entry(&key) { Ok(e) => match e.delete_credential() { Ok(()) | Err(keyring::Error::NoEntry) => {} Err(err) => failures.push(format!("{key}: {err}")), @@ -128,13 +286,8 @@ pub fn clear_credentials() -> Result<()> { Ok(()) } else { Err(anyhow::anyhow!( - "failed to clear {} keychain {}: {}", + "failed to clear {} keychain entries: {}", failures.len(), - if failures.len() == 1 { - "entry" - } else { - "entries" - }, failures.join("; ") )) } @@ -159,6 +312,7 @@ pub struct OAuthResult { /// Note: [`refresh_oauth_token`] does NOT take a scope parameter — the /// `refresh_token` grant inherits scopes from the original authorization. pub async fn oauth_login( + profile: &str, client_id: &str, client_secret: &str, scopes: &str, @@ -249,7 +403,7 @@ pub async fn oauth_login( .ok_or_else(|| anyhow::anyhow!("No accessible Jira sites found"))?; // 5. Store tokens in the system keychain. - store_oauth_tokens(&tokens.access_token, &tokens.refresh_token)?; + store_oauth_tokens(profile, &tokens.access_token, &tokens.refresh_token)?; Ok(OAuthResult { cloud_id: resource.id.clone(), @@ -263,11 +417,15 @@ pub async fn oauth_login( /// /// Intentionally takes no `scopes` parameter: the `refresh_token` grant /// inherits scopes from the original authorization per RFC 6749 §6. To -/// pick up a changed `[instance].oauth_scopes` in config.toml, the user -/// must re-run `jr auth login --oauth` (refresh alone will keep the old -/// scope set). -pub async fn refresh_oauth_token(client_id: &str, client_secret: &str) -> Result { - let (_, refresh_token) = load_oauth_tokens()?; +/// pick up a changed `[profiles.].oauth_scopes` in config.toml, +/// the user must re-run `jr auth login --oauth` (refresh alone will +/// keep the old scope set). +pub async fn refresh_oauth_token( + profile: &str, + client_id: &str, + client_secret: &str, +) -> Result { + let (_, refresh_token) = load_oauth_tokens(profile)?; let client = reqwest::Client::new(); let response = client @@ -291,7 +449,7 @@ pub async fn refresh_oauth_token(client_id: &str, client_secret: &str) -> Result refresh_token: String, } let tokens: TokenResponse = response.json().await?; - store_oauth_tokens(&tokens.access_token, &tokens.refresh_token)?; + store_oauth_tokens(profile, &tokens.access_token, &tokens.refresh_token)?; Ok(tokens.access_token) } @@ -523,4 +681,262 @@ mod tests { "raw + must not appear in the URL: {url}" ); } + + fn unique_test_service() -> String { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::SeqCst); + format!("jr-jira-cli-test-{}-{}", std::process::id(), n) + } + + /// Serializes JR_SERVICE_NAME mutation across concurrent keyring tests so + /// no test observes a service name set by another in-flight test (which + /// would point its keychain operations at the wrong namespace). + static KEYRING_TEST_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Wrap a test in a unique JR_SERVICE_NAME scope so concurrent tests don't collide. + fn with_test_keyring(f: F) { + if std::env::var("JR_RUN_KEYRING_TESTS").is_err() { + return; + } + // Hold the mutex across env mutation + body + cleanup so no other + // `with_test_keyring` invocation can race the JR_SERVICE_NAME + // set/unset and observe a half-applied state. Recover from a + // poisoned lock — a panicking test still leaves the env in a + // recoverable state because we restore JR_SERVICE_NAME at scope + // exit, and a unique service-name namespace per call already + // isolates keychain entries. + let _guard = KEYRING_TEST_ENV_MUTEX + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let svc = unique_test_service(); + let prev = std::env::var("JR_SERVICE_NAME").ok(); + // SAFETY: KEYRING_TEST_ENV_MUTEX is held for the duration of this + // scope, so no other test in this binary can race the env mutation. + // The opt-in `JR_RUN_KEYRING_TESTS` gate further keeps these tests + // off the default test path. + unsafe { std::env::set_var("JR_SERVICE_NAME", &svc) }; + f(); + let _ = clear_all_credentials(&["default", "sandbox"]); + // SAFETY: still holding KEYRING_TEST_ENV_MUTEX. + unsafe { + match prev { + Some(p) => std::env::set_var("JR_SERVICE_NAME", p), + None => std::env::remove_var("JR_SERVICE_NAME"), + } + } + } + + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn store_and_load_per_profile_oauth_tokens_round_trip() { + with_test_keyring(|| { + store_oauth_tokens("default", "access1", "refresh1").unwrap(); + store_oauth_tokens("sandbox", "access2", "refresh2").unwrap(); + + let (a1, r1) = load_oauth_tokens("default").unwrap(); + let (a2, r2) = load_oauth_tokens("sandbox").unwrap(); + + assert_eq!((a1.as_str(), r1.as_str()), ("access1", "refresh1")); + assert_eq!((a2.as_str(), r2.as_str()), ("access2", "refresh2")); + }); + } + + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn load_oauth_tokens_returns_err_for_missing_profile() { + with_test_keyring(|| { + assert!(load_oauth_tokens("default").is_err()); + }); + } + + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn lazy_migration_legacy_flat_keys_for_default_profile() { + with_test_keyring(|| { + entry("oauth-access-token") + .unwrap() + .set_password("legacy-access") + .unwrap(); + entry("oauth-refresh-token") + .unwrap() + .set_password("legacy-refresh") + .unwrap(); + + let (access, refresh) = load_oauth_tokens("default").unwrap(); + assert_eq!(access, "legacy-access"); + assert_eq!(refresh, "legacy-refresh"); + + let new_access = entry("default:oauth-access-token") + .unwrap() + .get_password() + .unwrap(); + assert_eq!(new_access, "legacy-access"); + + assert!(entry("oauth-access-token").unwrap().get_password().is_err()); + }); + } + + /// Regression: `clear_profile_creds("default")` must also remove the + /// legacy flat OAuth keys. Otherwise `jr auth logout --profile default` + /// leaves the legacy entries in place and the next `load_oauth_tokens` + /// call resurrects them via the lazy-migration path — silently undoing + /// the logout for a user mid-migration. + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn clear_profile_creds_default_also_clears_legacy_flat_keys() { + with_test_keyring(|| { + // Pre-seed legacy flat keys. + entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .set_password("legacy-access") + .unwrap(); + entry(KEY_OAUTH_REFRESH_LEGACY) + .unwrap() + .set_password("legacy-refresh") + .unwrap(); + + clear_profile_creds("default").unwrap(); + + // Legacy keys must be gone — otherwise lazy migration would + // resurrect them on the next load_oauth_tokens call. + assert!( + entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .get_password() + .is_err() + ); + assert!( + entry(KEY_OAUTH_REFRESH_LEGACY) + .unwrap() + .get_password() + .is_err() + ); + }); + } + + /// Companion to the test above: clearing a non-default profile must NOT + /// touch the legacy flat keys, since those belong to the `"default"` + /// profile's lazy-migration window. + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn clear_profile_creds_non_default_leaves_legacy_keys_alone() { + with_test_keyring(|| { + entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .set_password("legacy-access") + .unwrap(); + + clear_profile_creds("sandbox").unwrap(); + + // Legacy keys belong to the "default" profile's lazy migration; + // logging out of "sandbox" must not touch them. + let access = entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .get_password() + .unwrap(); + assert_eq!(access, "legacy-access"); + }); + } + + /// Regression: `load_oauth_tokens` must distinguish (None, None) from + /// partial state (Some, None) / (None, Some). A pair lookup that + /// retried via the legacy fallback on partial state would either + /// silently resurrect a stale legacy pair or return the generic + /// "no token" error — both of which hide data loss / corruption. + /// Partial state should surface as an explicit error pointing to + /// logout+login recovery. + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn load_oauth_tokens_errors_on_partial_state() { + with_test_keyring(|| { + // Pre-seed only the access key (missing refresh). + entry(&oauth_access_key("sandbox")) + .unwrap() + .set_password("access-only") + .unwrap(); + + let result = load_oauth_tokens("sandbox"); + let err = result.expect_err("partial state should error"); + let msg = format!("{err:#}"); + assert!(msg.contains("partial"), "got: {msg}"); + }); + } + + /// Edge case: an interrupted lazy migration could leave the namespaced + /// pair in a partial state for the `default` profile while the legacy + /// flat keys still hold a complete pair. `load_oauth_tokens("default")` + /// should recover from the intact legacy pair rather than stranding + /// users with a partial-state error. + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn load_oauth_tokens_default_partial_recovers_from_legacy() { + with_test_keyring(|| { + // Partial namespaced state for the default profile. + entry(&oauth_access_key("default")) + .unwrap() + .set_password("stale-partial") + .unwrap(); + // Complete legacy pair. + entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .set_password("legacy-access") + .unwrap(); + entry(KEY_OAUTH_REFRESH_LEGACY) + .unwrap() + .set_password("legacy-refresh") + .unwrap(); + + let (a, r) = load_oauth_tokens("default").unwrap(); + assert_eq!(a, "legacy-access"); + assert_eq!(r, "legacy-refresh"); + + // The recovered legacy values overwrote the namespaced pair + // (both halves now match the legacy tokens). + let recovered_access = entry(&oauth_access_key("default")) + .unwrap() + .get_password() + .unwrap(); + let recovered_refresh = entry(&oauth_refresh_key("default")) + .unwrap() + .get_password() + .unwrap(); + assert_eq!(recovered_access, "legacy-access"); + assert_eq!(recovered_refresh, "legacy-refresh"); + + // Legacy flat keys cleaned up after migration. + assert!( + entry(KEY_OAUTH_ACCESS_LEGACY) + .unwrap() + .get_password() + .is_err() + ); + assert!( + entry(KEY_OAUTH_REFRESH_LEGACY) + .unwrap() + .get_password() + .is_err() + ); + }); + } + + #[test] + #[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] + fn lazy_migration_does_not_fire_for_non_default_profile() { + with_test_keyring(|| { + entry("oauth-access-token") + .unwrap() + .set_password("legacy-access") + .unwrap(); + entry("oauth-refresh-token") + .unwrap() + .set_password("legacy-refresh") + .unwrap(); + + assert!( + load_oauth_tokens("sandbox").is_err(), + "sandbox profile should NOT inherit legacy keys" + ); + }); + } } diff --git a/src/api/client.rs b/src/api/client.rs index 3095e094..19a94ed2 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -21,6 +21,10 @@ pub struct JiraClient { auth_header: String, verbose: bool, assets_base_url: Option, + /// Active profile name, plumbed through so per-profile cache calls can + /// scope their reads/writes correctly without the call sites needing + /// access to `&Config`. + profile_name: String, } impl JiraClient { @@ -32,22 +36,29 @@ impl JiraClient { // JR_BASE_URL overrides all URL targets (used by integration tests to inject wiremock). let test_override = std::env::var("JR_BASE_URL").ok(); + // In test-override mode the profile is not consulted for any URL target, + // and JR_AUTH_HEADER short-circuits credential loading. In real-use mode + // the active profile is required to know URL, auth_method, and cloud_id. + let profile = if test_override.is_some() { + None + } else { + Some(config.active_profile_or_err()?) + }; + let instance_url = if let Some(ref override_url) = test_override { // Test mode: route all traffic (including instance and assets) to the mock server. override_url.trim_end_matches('/').to_string() - } else if let Some(url) = config.global.instance.url.as_ref() { + } else if let Some(url) = profile.and_then(|p| p.url.as_ref()) { url.trim_end_matches('/').to_string() } else { - return Err(JrError::ConfigError( - "No Jira instance configured. Run \"jr init\" first.".into(), - ) + return Err(JrError::ConfigError(format!( + "Profile {:?} has no URL configured. Run \"jr auth login --profile {}\".", + config.active_profile_name, config.active_profile_name + )) .into()); }; - let auth_method = config - .global - .instance - .auth_method - .as_deref() + let auth_method = profile + .and_then(|p| p.auth_method.as_deref()) .unwrap_or("api_token"); // JR_AUTH_HEADER env var overrides keychain auth (used by tests to inject mock auth) @@ -56,7 +67,8 @@ impl JiraClient { } else { match auth_method { "oauth" => { - let (access, _refresh) = crate::api::auth::load_oauth_tokens()?; + let (access, _refresh) = + crate::api::auth::load_oauth_tokens(&config.active_profile_name)?; format!("Bearer {access}") } _ => { @@ -75,7 +87,7 @@ impl JiraClient { // Test mode: assets API goes to the mock server under /jsm/assets. Some(format!("{}/jsm/assets", override_url.trim_end_matches('/'))) } else { - config.global.instance.cloud_id.as_ref().map(|cloud_id| { + profile.and_then(|p| p.cloud_id.as_ref()).map(|cloud_id| { format!( "https://api.atlassian.com/ex/jira/{}/jsm/assets", urlencoding::encode(cloud_id) @@ -90,6 +102,7 @@ impl JiraClient { auth_header, verbose, assets_base_url, + profile_name: config.active_profile_name.clone(), }) } @@ -104,9 +117,17 @@ impl JiraClient { auth_header, verbose: false, assets_base_url, + profile_name: "default".to_string(), } } + /// Active profile name this client is bound to. Used by per-profile + /// cache call sites (CMDB fields, workspace ID, project meta, resolutions, + /// object-type attrs) that have a `&JiraClient` but not `&Config`. + pub fn profile_name(&self) -> &str { + &self.profile_name + } + /// Whether the client was constructed with `--verbose` enabled. /// Handlers use this to gate optional diagnostic output. pub fn verbose(&self) -> bool { diff --git a/src/api/jsm/servicedesks.rs b/src/api/jsm/servicedesks.rs index 3ed668fc..2e79de1a 100644 --- a/src/api/jsm/servicedesks.rs +++ b/src/api/jsm/servicedesks.rs @@ -42,8 +42,9 @@ pub async fn get_or_fetch_project_meta( client: &JiraClient, project_key: &str, ) -> Result { - // Check cache first - if let Some(cached) = cache::read_project_meta(project_key)? { + // Check cache first. + let profile = client.profile_name(); + if let Some(cached) = cache::read_project_meta(profile, project_key)? { return Ok(cached); } @@ -92,7 +93,7 @@ pub async fn get_or_fetch_project_meta( }; // Write to cache (best-effort — don't fail the command if cache write fails) - let _ = cache::write_project_meta(project_key, &meta); + let _ = cache::write_project_meta(profile, project_key, &meta); Ok(meta) } diff --git a/src/cache.rs b/src/cache.rs index 16bb855e..92fd12f1 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -13,8 +13,8 @@ pub(crate) trait Expiring { /// Read a whole-file cache. Returns `Ok(None)` on missing, expired, or corrupt /// (unparseable) files. Propagates I/O errors. -fn read_cache(filename: &str) -> Result> { - let path = cache_dir().join(filename); +fn read_cache(profile: &str, filename: &str) -> Result> { + let path = cache_dir(profile).join(filename); let content = match std::fs::read_to_string(&path) { Ok(c) => c, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), @@ -34,8 +34,8 @@ fn read_cache(filename: &str) -> Result(filename: &str, data: &T) -> Result<()> { - let dir = cache_dir(); +fn write_cache(profile: &str, filename: &str, data: &T) -> Result<()> { + let dir = cache_dir(profile); std::fs::create_dir_all(&dir)?; let content = serde_json::to_string_pretty(data)?; std::fs::write(dir.join(filename), content)?; @@ -60,7 +60,8 @@ impl Expiring for TeamCache { } } -pub fn cache_dir() -> PathBuf { +/// Root cache directory: `$XDG_CACHE_HOME/jr` or `~/.cache/jr`. +pub fn cache_root() -> PathBuf { if let Ok(xdg) = std::env::var("XDG_CACHE_HOME") { PathBuf::from(xdg).join("jr") } else { @@ -71,12 +72,28 @@ pub fn cache_dir() -> PathBuf { } } -pub fn read_team_cache() -> Result> { - read_cache("teams.json") +/// Per-profile cache directory: `/v1//`. +pub fn cache_dir(profile: &str) -> PathBuf { + cache_root().join("v1").join(profile) } -pub fn write_team_cache(teams: &[CachedTeam]) -> Result<()> { +/// Remove all cached data for a single profile. No-op if the directory does +/// not exist; other profiles are untouched. +pub fn clear_profile_cache(profile: &str) -> Result<()> { + let dir = cache_dir(profile); + if dir.exists() { + std::fs::remove_dir_all(dir)?; + } + Ok(()) +} + +pub fn read_team_cache(profile: &str) -> Result> { + read_cache(profile, "teams.json") +} + +pub fn write_team_cache(profile: &str, teams: &[CachedTeam]) -> Result<()> { write_cache( + profile, "teams.json", &TeamCache { fetched_at: Utc::now(), @@ -98,8 +115,8 @@ pub struct ProjectMeta { /// /// Keyed cache — not genericized because TTL is checked per-entry /// (`ProjectMeta.fetched_at`), unlike whole-file caches. -pub fn read_project_meta(project_key: &str) -> Result> { - let path = cache_dir().join("project_meta.json"); +pub fn read_project_meta(profile: &str, project_key: &str) -> Result> { + let path = cache_dir(profile).join("project_meta.json"); if !path.exists() { return Ok(None); } @@ -129,8 +146,8 @@ pub fn read_project_meta(project_key: &str) -> Result> { /// Write cached project metadata for a specific project key. /// /// Merges into the existing map file, preserving entries for other projects. -pub fn write_project_meta(project_key: &str, meta: &ProjectMeta) -> Result<()> { - let dir = cache_dir(); +pub fn write_project_meta(profile: &str, project_key: &str, meta: &ProjectMeta) -> Result<()> { + let dir = cache_dir(profile); std::fs::create_dir_all(&dir)?; let path = dir.join("project_meta.json"); @@ -167,12 +184,13 @@ impl Expiring for WorkspaceCache { } } -pub fn read_workspace_cache() -> Result> { - read_cache("workspace.json") +pub fn read_workspace_cache(profile: &str) -> Result> { + read_cache(profile, "workspace.json") } -pub fn write_workspace_cache(workspace_id: &str) -> Result<()> { +pub fn write_workspace_cache(profile: &str, workspace_id: &str) -> Result<()> { write_cache( + profile, "workspace.json", &WorkspaceCache { workspace_id: workspace_id.to_string(), @@ -201,12 +219,13 @@ impl Expiring for ResolutionsCache { } } -pub fn read_resolutions_cache() -> Result> { - read_cache("resolutions.json") +pub fn read_resolutions_cache(profile: &str) -> Result> { + read_cache(profile, "resolutions.json") } -pub fn write_resolutions_cache(resolutions: &[CachedResolution]) -> Result<()> { +pub fn write_resolutions_cache(profile: &str, resolutions: &[CachedResolution]) -> Result<()> { write_cache( + profile, "resolutions.json", &ResolutionsCache { resolutions: resolutions.to_vec(), @@ -227,12 +246,13 @@ impl Expiring for CmdbFieldsCache { } } -pub fn read_cmdb_fields_cache() -> Result> { - read_cache("cmdb_fields.json") +pub fn read_cmdb_fields_cache(profile: &str) -> Result> { + read_cache(profile, "cmdb_fields.json") } -pub fn write_cmdb_fields_cache(fields: &[(String, String)]) -> Result<()> { +pub fn write_cmdb_fields_cache(profile: &str, fields: &[(String, String)]) -> Result<()> { write_cache( + profile, "cmdb_fields.json", &CmdbFieldsCache { fields: fields.to_vec(), @@ -267,9 +287,10 @@ pub struct ObjectTypeAttrCache { /// (`ObjectTypeAttrCache.fetched_at`) but lookup is per-key, with a different /// return type (`Vec`) than the stored wrapper struct. pub fn read_object_type_attr_cache( + profile: &str, object_type_id: &str, ) -> Result>> { - let path = cache_dir().join("object_type_attrs.json"); + let path = cache_dir(profile).join("object_type_attrs.json"); if !path.exists() { return Ok(None); } @@ -295,10 +316,11 @@ pub fn read_object_type_attr_cache( /// /// Merges into the existing map file, preserving entries for other object types. pub fn write_object_type_attr_cache( + profile: &str, object_type_id: &str, attrs: &[CachedObjectTypeAttr], ) -> Result<()> { - let dir = cache_dir(); + let dir = cache_dir(profile); std::fs::create_dir_all(&dir)?; let path = dir.join("object_type_attrs.json"); @@ -356,10 +378,70 @@ mod tests { } } + #[test] + fn cache_dir_includes_v1_and_profile_subdir() { + with_temp_cache(|| { + let dir = cache_dir("default"); + assert!(dir.ends_with("v1/default"), "got: {}", dir.display()); + }); + } + + #[test] + fn cross_profile_isolation_team_cache() { + with_temp_cache(|| { + write_team_cache( + "prod", + &[CachedTeam { + id: "t1".into(), + name: "Prod Team".into(), + }], + ) + .unwrap(); + + let prod = read_team_cache("prod").unwrap().unwrap(); + assert_eq!(prod.teams[0].name, "Prod Team"); + + assert!(read_team_cache("sandbox").unwrap().is_none()); + }); + } + + #[test] + fn clear_profile_cache_removes_only_that_profile() { + with_temp_cache(|| { + write_team_cache( + "prod", + &[CachedTeam { + id: "p".into(), + name: "P".into(), + }], + ) + .unwrap(); + write_team_cache( + "sandbox", + &[CachedTeam { + id: "s".into(), + name: "S".into(), + }], + ) + .unwrap(); + + clear_profile_cache("prod").unwrap(); + + assert!( + read_team_cache("prod").unwrap().is_none(), + "prod cache cleared" + ); + assert!( + read_team_cache("sandbox").unwrap().is_some(), + "sandbox cache preserved" + ); + }); + } + #[test] fn read_missing_cache_returns_none() { with_temp_cache(|| { - let result = read_team_cache().unwrap(); + let result = read_team_cache("default").unwrap(); assert!(result.is_none()); }); } @@ -377,9 +459,11 @@ mod tests { name: "Beta".into(), }, ]; - write_team_cache(&teams).unwrap(); + write_team_cache("default", &teams).unwrap(); - let cache = read_team_cache().unwrap().expect("cache should exist"); + let cache = read_team_cache("default") + .unwrap() + .expect("cache should exist"); assert_eq!(cache.teams.len(), 2); assert_eq!(cache.teams[0].name, "Alpha"); assert_eq!(cache.teams[1].name, "Beta"); @@ -396,12 +480,12 @@ mod tests { name: "Old".into(), }], }; - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); let content = serde_json::to_string_pretty(&expired).unwrap(); std::fs::write(dir.join("teams.json"), content).unwrap(); - let result = read_team_cache().unwrap(); + let result = read_team_cache("default").unwrap(); assert!(result.is_none(), "expired cache should return None"); }); } @@ -416,12 +500,14 @@ mod tests { name: "Recent".into(), }], }; - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); let content = serde_json::to_string_pretty(&recent).unwrap(); std::fs::write(dir.join("teams.json"), content).unwrap(); - let cache = read_team_cache().unwrap().expect("cache should be valid"); + let cache = read_team_cache("default") + .unwrap() + .expect("cache should be valid"); assert_eq!(cache.teams.len(), 1); assert_eq!(cache.teams[0].name, "Recent"); }); @@ -430,7 +516,7 @@ mod tests { #[test] fn read_missing_project_meta_returns_none() { with_temp_cache(|| { - let result = read_project_meta("NOEXIST").unwrap(); + let result = read_project_meta("default", "NOEXIST").unwrap(); assert!(result.is_none()); }); } @@ -445,9 +531,9 @@ mod tests { service_desk_id: Some("15".into()), fetched_at: Utc::now(), }; - write_project_meta("HELPDESK", &meta).unwrap(); + write_project_meta("default", "HELPDESK", &meta).unwrap(); - let loaded = read_project_meta("HELPDESK") + let loaded = read_project_meta("default", "HELPDESK") .unwrap() .expect("should exist"); assert_eq!(loaded.project_type, "service_desk"); @@ -467,9 +553,9 @@ mod tests { service_desk_id: Some("15".into()), fetched_at: Utc::now() - chrono::Duration::days(8), }; - write_project_meta("HELPDESK", &meta).unwrap(); + write_project_meta("default", "HELPDESK", &meta).unwrap(); - let result = read_project_meta("HELPDESK").unwrap(); + let result = read_project_meta("default", "HELPDESK").unwrap(); assert!(result.is_none(), "expired project meta should return None"); }); } @@ -491,15 +577,17 @@ mod tests { service_desk_id: None, fetched_at: Utc::now(), }; - write_project_meta("HELPDESK", &jsm).unwrap(); - write_project_meta("DEV", &software).unwrap(); + write_project_meta("default", "HELPDESK", &jsm).unwrap(); + write_project_meta("default", "DEV", &software).unwrap(); - let jsm_loaded = read_project_meta("HELPDESK") + let jsm_loaded = read_project_meta("default", "HELPDESK") .unwrap() .expect("should exist"); assert_eq!(jsm_loaded.project_type, "service_desk"); - let sw_loaded = read_project_meta("DEV").unwrap().expect("should exist"); + let sw_loaded = read_project_meta("default", "DEV") + .unwrap() + .expect("should exist"); assert_eq!(sw_loaded.project_type, "software"); assert!(sw_loaded.service_desk_id.is_none()); }); @@ -508,7 +596,7 @@ mod tests { #[test] fn read_missing_workspace_cache_returns_none() { with_temp_cache(|| { - let result = read_workspace_cache().unwrap(); + let result = read_workspace_cache("default").unwrap(); assert!(result.is_none()); }); } @@ -516,9 +604,11 @@ mod tests { #[test] fn write_then_read_workspace_cache() { with_temp_cache(|| { - write_workspace_cache("abc-123-def").unwrap(); + write_workspace_cache("default", "abc-123-def").unwrap(); - let cache = read_workspace_cache().unwrap().expect("should exist"); + let cache = read_workspace_cache("default") + .unwrap() + .expect("should exist"); assert_eq!(cache.workspace_id, "abc-123-def"); }); } @@ -530,12 +620,12 @@ mod tests { workspace_id: "old-id".into(), fetched_at: Utc::now() - chrono::Duration::days(8), }; - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); let content = serde_json::to_string_pretty(&expired).unwrap(); std::fs::write(dir.join("workspace.json"), content).unwrap(); - let result = read_workspace_cache().unwrap(); + let result = read_workspace_cache("default").unwrap(); assert!( result.is_none(), "expired workspace cache should return None" @@ -546,7 +636,7 @@ mod tests { #[test] fn read_missing_cmdb_fields_cache_returns_none() { with_temp_cache(|| { - let result = read_cmdb_fields_cache().unwrap(); + let result = read_cmdb_fields_cache("default").unwrap(); assert!(result.is_none()); }); } @@ -554,13 +644,18 @@ mod tests { #[test] fn write_then_read_cmdb_fields_cache() { with_temp_cache(|| { - write_cmdb_fields_cache(&[ - ("customfield_10191".into(), "Client".into()), - ("customfield_10245".into(), "Hardware".into()), - ]) + write_cmdb_fields_cache( + "default", + &[ + ("customfield_10191".into(), "Client".into()), + ("customfield_10245".into(), "Hardware".into()), + ], + ) .unwrap(); - let cache = read_cmdb_fields_cache().unwrap().expect("should exist"); + let cache = read_cmdb_fields_cache("default") + .unwrap() + .expect("should exist"); assert_eq!( cache.fields, vec![ @@ -578,12 +673,12 @@ mod tests { fields: vec![("customfield_10191".into(), "Client".into())], fetched_at: Utc::now() - chrono::Duration::days(8), }; - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); let content = serde_json::to_string_pretty(&expired).unwrap(); std::fs::write(dir.join("cmdb_fields.json"), content).unwrap(); - let result = read_cmdb_fields_cache().unwrap(); + let result = read_cmdb_fields_cache("default").unwrap(); assert!( result.is_none(), "expired cmdb fields cache should return None" @@ -594,7 +689,7 @@ mod tests { #[test] fn read_missing_object_type_attr_cache_returns_none() { with_temp_cache(|| { - let result = read_object_type_attr_cache("23").unwrap(); + let result = read_object_type_attr_cache("default", "23").unwrap(); assert!(result.is_none()); }); } @@ -620,9 +715,9 @@ mod tests { position: 1, }, ]; - write_object_type_attr_cache("23", &attrs).unwrap(); + write_object_type_attr_cache("default", "23", &attrs).unwrap(); - let loaded = read_object_type_attr_cache("23") + let loaded = read_object_type_attr_cache("default", "23") .unwrap() .expect("should exist"); assert_eq!(loaded.len(), 2); @@ -654,12 +749,12 @@ mod tests { m }, }; - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); let content = serde_json::to_string_pretty(&expired).unwrap(); std::fs::write(dir.join("object_type_attrs.json"), content).unwrap(); - let result = read_object_type_attr_cache("23").unwrap(); + let result = read_object_type_attr_cache("default", "23").unwrap(); assert!(result.is_none(), "expired cache should return None"); }); } @@ -683,15 +778,15 @@ mod tests { label: false, position: 3, }]; - write_object_type_attr_cache("23", &attrs_a).unwrap(); - write_object_type_attr_cache("45", &attrs_b).unwrap(); + write_object_type_attr_cache("default", "23", &attrs_a).unwrap(); + write_object_type_attr_cache("default", "45", &attrs_b).unwrap(); - let loaded_a = read_object_type_attr_cache("23") + let loaded_a = read_object_type_attr_cache("default", "23") .unwrap() .expect("type 23 should exist"); assert_eq!(loaded_a[0].name, "Key"); - let loaded_b = read_object_type_attr_cache("45") + let loaded_b = read_object_type_attr_cache("default", "45") .unwrap() .expect("type 45 should exist"); assert_eq!(loaded_b[0].name, "Hostname"); @@ -701,11 +796,11 @@ mod tests { #[test] fn object_type_attr_cache_corrupt_returns_none() { with_temp_cache(|| { - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); std::fs::write(dir.join("object_type_attrs.json"), "not json").unwrap(); - let result = read_object_type_attr_cache("23").unwrap(); + let result = read_object_type_attr_cache("default", "23").unwrap(); assert!(result.is_none(), "corrupt cache should return None"); }); } @@ -713,17 +808,17 @@ mod tests { #[test] fn corrupt_team_cache_returns_none() { with_temp_cache(|| { - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); // Garbage data std::fs::write(dir.join("teams.json"), "not json").unwrap(); - let result = read_team_cache().unwrap(); + let result = read_team_cache("default").unwrap(); assert!(result.is_none(), "garbage data should return None"); // Valid JSON, wrong shape std::fs::write(dir.join("teams.json"), r#"{"unexpected": true}"#).unwrap(); - let result = read_team_cache().unwrap(); + let result = read_team_cache("default").unwrap(); assert!(result.is_none(), "wrong-shape JSON should return None"); }); } @@ -731,17 +826,17 @@ mod tests { #[test] fn corrupt_workspace_cache_returns_none() { with_temp_cache(|| { - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); // Garbage data std::fs::write(dir.join("workspace.json"), "not json").unwrap(); - let result = read_workspace_cache().unwrap(); + let result = read_workspace_cache("default").unwrap(); assert!(result.is_none(), "garbage data should return None"); // Valid JSON, wrong shape std::fs::write(dir.join("workspace.json"), r#"{"unexpected": true}"#).unwrap(); - let result = read_workspace_cache().unwrap(); + let result = read_workspace_cache("default").unwrap(); assert!(result.is_none(), "wrong-shape JSON should return None"); }); } @@ -749,17 +844,17 @@ mod tests { #[test] fn corrupt_project_meta_returns_none() { with_temp_cache(|| { - let dir = cache_dir(); + let dir = cache_dir("default"); std::fs::create_dir_all(&dir).unwrap(); // Garbage data std::fs::write(dir.join("project_meta.json"), "not json").unwrap(); - let result = read_project_meta("ANY").unwrap(); + let result = read_project_meta("default", "ANY").unwrap(); assert!(result.is_none(), "garbage data should return None"); // Valid JSON, wrong shape std::fs::write(dir.join("project_meta.json"), r#"{"unexpected": true}"#).unwrap(); - let result = read_project_meta("ANY").unwrap(); + let result = read_project_meta("default", "ANY").unwrap(); assert!(result.is_none(), "wrong-shape JSON should return None"); }); } @@ -785,8 +880,8 @@ mod resolution_cache_tests { description: None, }, ]; - write_resolutions_cache(&input).unwrap(); - let loaded = read_resolutions_cache().unwrap().unwrap(); + write_resolutions_cache("default", &input).unwrap(); + let loaded = read_resolutions_cache("default").unwrap().unwrap(); assert_eq!(loaded.resolutions.len(), 2); assert_eq!(loaded.resolutions[0].name, "Done"); @@ -797,7 +892,7 @@ mod resolution_cache_tests { #[test] fn resolution_cache_missing_returns_none() { with_temp_cache(|| { - let loaded = read_resolutions_cache().unwrap(); + let loaded = read_resolutions_cache("default").unwrap(); assert!(loaded.is_none()); }); } diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 6af5eae2..1c98cd63 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -96,29 +96,54 @@ impl AuthFlow { } } -/// Decide which login flow to run based on config + explicit override. +/// Decide which login flow to run for the **active** profile + explicit +/// override. +/// +/// Today this is only exercised by unit tests (production callers like +/// `refresh_credentials` need the target profile, not the active one, and +/// use [`chosen_flow_for_profile`] directly). It's kept as a thin wrapper +/// so a future caller that genuinely wants the active profile has a +/// labeled entry point — `#[cfg(test)]` because adding it without a real +/// caller would just be dead code. /// /// Order of precedence: /// 1. `oauth_override = true` → always OAuth (user passed `--oauth`). -/// 2. Config `auth_method == "oauth"` → OAuth. +/// 2. Active profile `auth_method == "oauth"` → OAuth. /// 3. Anything else (including unset) → Token. Matches the `api_token` /// default that `JiraClient::from_config` applies when no method is set. +#[cfg(test)] fn chosen_flow(config: &Config, oauth_override: bool) -> AuthFlow { + chosen_flow_for_profile(&config.active_profile(), oauth_override) +} + +/// Decide which login flow to run based on a specific profile + explicit +/// override. Use this when the caller has already resolved the target +/// profile and that profile may differ from the active one (refresh, +/// per-target dispatch). +fn chosen_flow_for_profile( + profile: &crate::config::ProfileConfig, + oauth_override: bool, +) -> AuthFlow { if oauth_override { return AuthFlow::OAuth; } - match config.global.instance.auth_method.as_deref() { + match profile.auth_method.as_deref() { Some("oauth") => AuthFlow::OAuth, _ => AuthFlow::Token, } } -/// Pick the OAuth scope string: user override from `[instance].oauth_scopes` -/// if set, else the compiled-in default. Trims and collapses interior -/// whitespace so multi-line TOML strings encode cleanly. Empty or +/// Pick the OAuth scope string: user override from the *target* profile's +/// `oauth_scopes` if set, else the compiled-in default. Trims and collapses +/// interior whitespace so multi-line TOML strings encode cleanly. Empty or /// whitespace-only overrides are a configuration error. -fn resolve_oauth_scopes(config: &Config) -> Result { - match config.global.instance.oauth_scopes.as_deref() { +/// +/// Takes a `&ProfileConfig` (not a `&Config`) so callers like `login_oauth` +/// can pass the profile they're actually targeting; reading `Config`'s +/// active profile would silently return the wrong scopes when +/// `jr auth login --profile X` runs against a non-active X. +fn resolve_oauth_scopes(profile: &crate::config::ProfileConfig) -> Result { + match profile.oauth_scopes.as_deref() { None => Ok(auth::DEFAULT_OAUTH_SCOPES.to_string()), Some(raw) => { let collapsed: String = raw.split_whitespace().collect::>().join(" "); @@ -137,7 +162,13 @@ fn resolve_oauth_scopes(config: &Config) -> Result { } /// Resolve email and API token (flag → env → prompt), then store in keychain. +/// +/// `profile` names which entry under `[profiles]` should record the +/// `auth_method = "api_token"` after a successful login. The keychain entry +/// for API token + email is shared across profiles today (one-pair-per-host +/// keyring layout); the profile name only affects config persistence. pub async fn login_token( + profile: &str, email: Option, token: Option, no_input: bool, @@ -162,6 +193,37 @@ pub async fn login_token( )?; auth::store_api_token(&email, &token)?; + + // Persist the profile's auth_method so subsequent runs know which flow + // to use. URL is set by `prepare_login_target` before this point, so + // we only touch auth_method here. + // + // Use `load_lenient` (not `load`) for the same reason `handle_login` + // does: this function may be invoked while creating a brand-new profile + // whose name doesn't yet appear in `[profiles]`, and the resolved + // active profile (e.g., from `JR_PROFILE`) might not exist either. + // A strict reload here would re-trigger the unknown-active-profile + // check mid-flight and abort a login that's intentionally creating + // its target. + let mut config = Config::load_lenient_with(Some(profile))?; + let p = config + .global + .profiles + .entry(profile.to_string()) + .or_default(); + p.auth_method = Some("api_token".into()); + // If `default_profile` is unset (legacy / fresh config / refresh + // creating a non-"default" profile on a brand-new install), promote + // the target so the next strict `Config::load()` doesn't error trying + // to resolve the literal "default" against an empty profiles map. + // `handle_login` does this via `prepare_login_target`; callers that + // bypass that helper (notably `refresh_credentials`) need the same + // safeguard here. + if config.global.default_profile.is_none() { + config.global.default_profile = Some(profile.to_string()); + } + config.save_global()?; + eprintln!("Credentials stored in keychain."); Ok(()) } @@ -169,8 +231,11 @@ pub async fn login_token( /// Run the OAuth 2.0 (3LO) login flow and persist site configuration. /// /// Credentials resolved via flag → env → prompt, so CI/agent workflows can -/// pipe them in without a TTY. +/// pipe them in without a TTY. `profile` names the target profile under +/// `[profiles]`; OAuth tokens are stored under namespaced keychain entries +/// (`:oauth-*-token`) so multiple sites can coexist. pub async fn login_oauth( + profile: &str, client_id: Option, client_secret: Option, no_input: bool, @@ -212,7 +277,13 @@ pub async fn login_oauth( // treats a missing file as empty, so a genuinely-absent config never // reaches this error path — only real failures do. let config_path = global_config_path(); - let mut config = Config::load().map_err(|err| { + // Use `load_lenient` (not `load`) so a `JR_PROFILE` pointing at an + // unconfigured profile, or a target profile that doesn't yet exist, + // can't trip the strict active-profile existence check mid-login. + // `handle_login` already did the lenient load up-front; this internal + // reload must agree, otherwise the orchestrator-allowed creation flow + // gets aborted halfway through. + let config = Config::load_lenient_with(Some(profile)).map_err(|err| { JrError::ConfigError(format!( "Failed to load config: {err:#}\n\n\ Fix or remove the file referenced above. Global config: {config_path}; \ @@ -220,43 +291,272 @@ pub async fn login_oauth( config_path = config_path.display() )) })?; - let scopes = resolve_oauth_scopes(&config)?; + // Read scopes from the TARGET profile, not the active one — `login_oauth` + // may target a non-active profile (e.g., `jr auth login --profile X`). + let target_profile = config + .global + .profiles + .get(profile) + .cloned() + .unwrap_or_default(); + let scopes = resolve_oauth_scopes(&target_profile)?; // Store OAuth app credentials in keychain (only after scopes validate) crate::api::auth::store_oauth_app_credentials(&client_id, &client_secret)?; - let result = crate::api::auth::oauth_login(&client_id, &client_secret, &scopes).await?; + let result = + crate::api::auth::oauth_login(profile, &client_id, &client_secret, &scopes).await?; - config.global.instance.url = Some(result.site_url); - config.global.instance.cloud_id = Some(result.cloud_id); - config.global.instance.auth_method = Some("oauth".into()); + // Persist site info to the named profile under [profiles.], not + // the legacy [instance] block. Reload to pick up any mutations made + // earlier in the login flow (e.g., by `prepare_login_target`). Same + // lenient-load rationale as the earlier reload above. + let mut config = Config::load_lenient_with(Some(profile))?; + let p = config + .global + .profiles + .entry(profile.to_string()) + .or_default(); + p.url = Some(result.site_url); + p.cloud_id = Some(result.cloud_id); + p.auth_method = Some("oauth".into()); + // Same default_profile safeguard as login_token — `refresh_credentials` + // can reach this path on a fresh install, and we must never leave + // `default_profile = None` when [profiles] is non-empty (the next + // strict `Config::load()` would error trying to resolve "default" + // against a profiles map that doesn't contain it). + if config.global.default_profile.is_none() { + config.global.default_profile = Some(profile.to_string()); + } config.save_global()?; output::print_success(&format!("Authenticated with {}", result.site_name)); Ok(()) } -/// Show authentication status: instance URL, auth method, credential availability. -pub async fn status() -> Result<()> { - let config = Config::load()?; +/// Bundle of CLI arguments threaded from `main.rs` to [`handle_login`]. +/// +/// Grouped into a struct because the orchestrator needs all four credential +/// slots (two API-token, two OAuth) plus profile/URL/flow toggles, which +/// trips clippy's `too_many_arguments` lint when passed as positional +/// parameters. The struct also makes the call site at `main.rs` self- +/// documenting. +pub struct LoginArgs { + pub profile: Option, + pub url: Option, + pub oauth: bool, + pub email: Option, + pub token: Option, + pub client_id: Option, + pub client_secret: Option, + pub no_input: bool, +} - let url = config - .global - .instance - .url +/// Orchestrate `jr auth login`: ensure the target profile exists with the +/// requested URL, then dispatch to the API-token or OAuth flow. Wraps the +/// pure logic in [`prepare_login_target`] so `main.rs` only needs one call +/// to thread the new `--profile` / `--url` flags through. +/// +/// Wraps a load failure in `JrError::ConfigError` (exit 78) so a malformed +/// `config.toml` surfaces as an actionable error instead of dropping to +/// `Config::default()` and overwriting the user's broken-but-recoverable +/// file (#258). +pub async fn handle_login(args: LoginArgs) -> Result<()> { + let config_path = global_config_path(); + // `load_lenient` skips the active-profile existence check so + // `jr auth login --profile newprof --url ...` can create the profile + // on first use. Every other command keeps the strict `Config::load()`. + // + // Pass `args.profile.as_deref()` as the cli-flag override so the + // resolved active profile reflects the subcommand's `--profile` rather + // than relying on env-var seams (which are unsound under #[tokio::main]). + let mut config = Config::load_lenient_with(args.profile.as_deref()).map_err(|err| { + JrError::ConfigError(format!( + "Failed to load config: {err:#}\n\n\ + Fix or remove the file referenced above. Global config: {config_path}; \ + per-project overrides come from `.jr.toml` in the current directory or any parent.", + config_path = config_path.display() + )) + })?; + + // Defensive: when the user is creating a NEW profile interactively and + // didn't pass `--url`, prompt for it instead of silently creating a + // URL-less profile that fails confusingly on the next command. Done in + // the orchestrator (not in `prepare_login_target`) so that pure helper + // stays trivially unit-testable without a TTY. + let target_for_check = args + .profile .as_deref() + .unwrap_or(&config.active_profile_name); + // Prompt for URL whenever the target profile lacks one — both the + // brand-new-profile case AND the existing-but-URL-less case (e.g., + // a hand-edited or migrated profile with status `unset`). Without + // this, `jr auth login --profile ` interactively + // would leave the profile URL-less and fail confusingly on the + // next command. + let target_has_url = config + .global + .profiles + .get(target_for_check) + .and_then(|p| p.url.as_deref()) + .is_some(); + let url_resolved: Option = if let Some(u) = args.url.as_deref() { + Some(u.to_string()) + } else if !args.no_input && !target_has_url { + let prompt: String = dialoguer::Input::new() + .with_prompt(format!( + "Jira instance URL for profile {target_for_check:?} \ + (e.g., https://yourorg.atlassian.net)" + )) + .interact_text() + .context("failed to read Jira instance URL")?; + Some(prompt) + } else { + None + }; + + let (global, target) = prepare_login_target( + config.global, + args.profile.as_deref(), + url_resolved.as_deref(), + args.no_input, + &config.active_profile_name, + )?; + config.global = global; + config.save_global()?; + if args.oauth { + login_oauth(&target, args.client_id, args.client_secret, args.no_input).await + } else { + login_token(&target, args.email, args.token, args.no_input).await + } +} + +/// Pure logic for ensuring a target profile exists with the given URL. +/// Returns `(updated_global, resolved_profile_name)`. +/// +/// - When `profile_arg` is `Some`, that name is validated and used as the +/// target. Otherwise we fall back to `active_profile_name`, which the +/// caller has already resolved through the full precedence chain +/// (`--profile` flag > `JR_PROFILE` env > `default_profile` field > +/// `"default"`). Reading `default_profile` directly here would drop the +/// flag and env layers and silently target the wrong profile. +/// - When `url_arg` is `Some`, the profile's URL is overwritten (with the +/// trailing slash trimmed for canonical form). +/// - When creating a new profile under `--no-input`, a URL is required so +/// non-interactive agents can't accidentally create empty profiles. +/// - If `default_profile` is unset (legacy / fresh config), the resolved +/// target is promoted to the default so a follow-up `jr` invocation +/// keeps targeting it. +pub(super) fn prepare_login_target( + mut global: crate::config::GlobalConfig, + profile_arg: Option<&str>, + url_arg: Option<&str>, + no_input: bool, + active_profile_name: &str, +) -> Result<(crate::config::GlobalConfig, String)> { + let target = match profile_arg { + Some(name) => { + crate::config::validate_profile_name(name)?; + name.to_string() + } + None => active_profile_name.to_string(), + }; + + let entry = global.profiles.entry(target.clone()).or_default(); + + if let Some(url) = url_arg { + entry.url = Some(url.trim_end_matches('/').to_string()); + } else if entry.url.is_none() && no_input { + // Both "brand-new profile" and "existing profile with no URL" + // hit this path — under --no-input we can't prompt for the + // missing URL, so error out with the expected recovery flag. + return Err(JrError::UserError( + "--url required when the target profile has no URL configured".into(), + ) + .into()); + } + + if global.default_profile.is_none() { + global.default_profile = Some(target.clone()); + } + + Ok((global, target)) +} + +/// Show authentication status: instance URL, auth method, credential availability. +/// +/// When `profile_arg` is `Some`, reports for that profile. Otherwise reports +/// for the active profile (resolved via the usual flag → env → config → +/// "default" precedence chain at `Config::load` time). +pub async fn status(profile_arg: Option<&str>) -> Result<()> { + // `profile_arg` is the explicit per-subcommand override (`--profile` + // on `auth status`); when absent we still let Config::load apply the + // standard precedence chain (env > default_profile > "default"). + // Passing `profile_arg` here also doubles as the CLI-flag override + // for `Config::load_with`, ensuring a `jr auth status --profile X` + // against an unconfigured X surfaces a clear "unknown profile" error + // from the strict load instead of silently falling back to the + // active profile. + let config = Config::load_with(profile_arg)?; + let target = profile_arg + .map(str::to_string) + .unwrap_or_else(|| config.active_profile_name.clone()); + crate::config::validate_profile_name(&target)?; + + // Special-case: fresh install with no profiles yet AND no explicit + // `--profile` was passed. `jr auth status` is a legitimate probe + // used by setup scripts / CI / agents to detect first-run state. + // Erroring here would block that probe — the user hasn't configured + // anything yet, so "unknown profile" would be misleading. + // + // BUT if the user explicitly named a profile via `--profile X`, take + // the strict path below — they're asserting X exists, and silently + // succeeding with a generic "no profiles configured" message would + // hide the mismatch. Matches the strict behavior of switch/remove/ + // logout for explicit profile targets. + if config.global.profiles.is_empty() && profile_arg.is_none() { + eprintln!( + "No profiles configured. Run `jr init` or \ + `jr auth login --profile ` to set up." + ); + return Ok(()); + } + + // Refuse to "succeed" against a profile the user never configured — + // matches the strict behavior of switch/remove/logout. Without this, + // `jr auth status --profile typo` printed "(not configured)" for + // every field and exited 0, hiding the typo. + if !config.global.profiles.contains_key(&target) { + let known: Vec<&str> = config.global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { + "(none)".into() + } else { + known.join(", ") + } + )) + .into()); + } + + let profile = config.global.profiles.get(&target); + let url = profile + .and_then(|p| p.url.as_deref()) .unwrap_or("(not configured)"); + println!("Profile: {target}"); println!("Instance: {url}"); - let method = config - .global - .instance - .auth_method - .as_deref() + let method = profile + .and_then(|p| p.auth_method.as_deref()) .unwrap_or("(not configured)"); println!("Auth method: {method}"); - let creds_ok = auth::load_api_token().is_ok(); + // Credential probe: API-token creds are shared (one per host); OAuth + // tokens are per-profile and namespaced by the profile name. + let creds_ok = match method { + "oauth" => auth::load_oauth_tokens(&target).is_ok(), + _ => auth::load_api_token().is_ok(), + }; if creds_ok { println!("Credentials: stored in keychain"); } else { @@ -294,25 +594,87 @@ fn refresh_success_payload(flow: AuthFlow) -> serde_json::Value { /// network error during OAuth), the user is warned that credentials are gone /// and told exactly which `jr auth login` invocation will restore them, /// before the error is propagated. -pub async fn refresh_credentials( - oauth_override: bool, - email: Option, - token: Option, - client_id: Option, - client_secret: Option, - no_input: bool, - output: &crate::cli::OutputFormat, -) -> Result<()> { - let config = Config::load()?; - let flow = chosen_flow(&config, oauth_override); +/// Bundle of CLI arguments threaded from `main.rs` to [`refresh_credentials`]. +/// +/// Same rationale as [`LoginArgs`] — passing all credential slots plus the +/// flow toggle and `--profile` as positional parameters trips clippy's +/// `too_many_arguments` lint, so they're grouped into a struct that also +/// makes the call site at `main.rs` self-documenting. +pub struct RefreshArgs<'a> { + pub profile: Option<&'a str>, + pub oauth: bool, + pub email: Option, + pub token: Option, + pub client_id: Option, + pub client_secret: Option, + pub no_input: bool, + pub output: &'a crate::cli::OutputFormat, +} - auth::clear_credentials().context( - "failed to clear stored credentials before refresh — keychain may still hold stale entries", - )?; +pub async fn refresh_credentials(args: RefreshArgs<'_>) -> Result<()> { + // Pass `args.profile` as the CLI-flag override so a `--profile X` + // against an unconfigured X surfaces the strict load's "unknown + // profile" error rather than silently refreshing the active profile. + let config = Config::load_with(args.profile)?; + let target = args + .profile + .map(str::to_string) + .unwrap_or_else(|| config.active_profile_name.clone()); + crate::config::validate_profile_name(&target)?; + // Inspect the target profile's auth method (not the active profile's) + // so `jr auth refresh --profile X` against a non-active X dispatches + // the right flow. Missing entries default to api_token, matching the + // login-time default. + let target_profile = config + .global + .profiles + .get(&target) + .cloned() + .unwrap_or_default(); + let flow = chosen_flow_for_profile(&target_profile, args.oauth); + + // For the api_token flow, login_token re-prompts/sets the SHARED + // api-token but doesn't write a URL. If the target profile has no + // URL configured (fresh install / hand-edited profile with status + // `unset`), refresh would succeed in keychain terms while leaving + // the profile unusable for any actual API call. Refuse upfront with + // a recovery hint to use `jr auth login --profile X --url ...` + // instead. The OAuth flow goes through oauth_login which fetches + // accessible-resources and writes its own URL/cloud_id, so it + // doesn't have this gap. + if flow == AuthFlow::Token && target_profile.url.is_none() { + return Err(JrError::UserError(format!( + "profile {target:?} has no URL configured. Use \ + \"jr auth login --profile {target} --url \" \ + instead of refresh — refresh assumes the profile is already \ + set up and only rotates credentials." + )) + .into()); + } + + // Clear-only-what-this-flow-refreshes: + // + // - OAuth refresh rotates the per-profile :oauth-*-token + // entries; the shared keys (email, api-token, oauth_client_id, + // oauth_client_secret) belong to other profiles too and must not + // be wiped. + // - API-token refresh re-prompts the email + api-token, and the + // shared api-token IS the credential being refreshed — so the + // #207-style "wipe-then-relogin" path is correct here. + match flow { + AuthFlow::OAuth => auth::clear_profile_creds(&target).context( + "failed to clear stored OAuth tokens before refresh — keychain may still hold stale entries", + )?, + AuthFlow::Token => auth::clear_all_credentials(&[target.as_str()]).context( + "failed to clear stored credentials before refresh — keychain may still hold stale entries", + )?, + } let login_result = match flow { - AuthFlow::Token => login_token(email, token, no_input).await, - AuthFlow::OAuth => login_oauth(client_id, client_secret, no_input).await, + AuthFlow::Token => login_token(&target, args.email, args.token, args.no_input).await, + AuthFlow::OAuth => { + login_oauth(&target, args.client_id, args.client_secret, args.no_input).await + } }; if let Err(err) = login_result { @@ -327,7 +689,7 @@ pub async fn refresh_credentials( return Err(err); } - match output { + match args.output { crate::cli::OutputFormat::Json => { let payload = serde_json::to_string_pretty(&refresh_success_payload(flow)) .context("failed to serialize refresh success payload as JSON")?; @@ -341,22 +703,282 @@ pub async fn refresh_credentials( Ok(()) } +/// Pure resolver for `jr auth logout`. Defaults to the active profile when +/// the user passes no `--profile`. Kept module-private and split out so the +/// CLI default behavior is unit-testable without filesystem or keychain. +pub(super) fn resolve_logout_target( + _global: &crate::config::GlobalConfig, + profile_arg: Option<&str>, + active: &str, +) -> String { + profile_arg.unwrap_or(active).to_string() +} + +/// `jr auth logout [--profile ]` — clear OAuth tokens for the target +/// profile. The profile entry in `config.toml` is left in place so a follow-up +/// `jr auth login --profile ` re-authenticates without losing site +/// metadata. The shared API-token credential is intentionally NOT cleared +/// (it's keyed by host, not profile, so wiping it would log every profile +/// out of API-token mode). +pub async fn handle_logout(profile_arg: Option<&str>) -> anyhow::Result<()> { + let config = crate::config::Config::load_with(profile_arg)?; + let target = resolve_logout_target(&config.global, profile_arg, &config.active_profile_name); + crate::config::validate_profile_name(&target)?; + if !config.global.profiles.contains_key(&target) { + let known: Vec<&str> = config.global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { + "(none)".into() + } else { + known.join(", ") + } + )) + .into()); + } + crate::api::auth::clear_profile_creds(&target)?; + crate::output::print_success(&format!("Logged out of profile {target:?}")); + Ok(()) +} + +/// Pure logic for `jr auth remove` — separated for testing without filesystem +/// or keychain. Returns the mutated `GlobalConfig` with `target` removed from +/// `profiles`. Refuses to remove the active profile (caller must switch first) +/// or unknown profiles. The cache directory and per-profile OAuth tokens are +/// cleared by [`handle_remove`] after the in-memory mutation succeeds; this +/// function only owns the config-shape transition. +pub(super) fn handle_remove_in_memory( + mut global: crate::config::GlobalConfig, + target: &str, + active: &str, +) -> anyhow::Result { + crate::config::validate_profile_name(target)?; + if !global.profiles.contains_key(target) { + let known: Vec<&str> = global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { + "(none)".into() + } else { + known.join(", ") + } + )) + .into()); + } + if target == active { + return Err(JrError::UserError(format!( + "cannot remove active profile {target:?}; switch first with \"jr auth switch \"" + )) + .into()); + } + // Also refuse if `target` is the persisted default_profile, even when + // not the *current* active (e.g., `jr --profile sandbox auth remove + // default` where active=sandbox but default_profile=default). Removing + // the profile that default_profile points to leaves config.toml in a + // broken state — strict Config::load() afterward would error with + // "active profile 'default' not in [profiles]" until the user manually + // edits the file. + if global.default_profile.as_deref() == Some(target) { + return Err(JrError::UserError(format!( + "cannot remove profile {target:?}: it is the default_profile in config. \ + Switch the default first with \"jr auth switch \"." + )) + .into()); + } + global.profiles.remove(target); + Ok(global) +} + +/// `jr auth remove ` — permanently delete a profile. +/// +/// Order of operations: +/// 1. Confirm with the user (skipped under `--no-input`). +/// 2. Mutate config in-memory via [`handle_remove_in_memory`] (validates name, +/// refuses active profile, refuses unknown profile). +/// 3. Persist config first so a subsequent keychain/cache failure can't +/// leave the profile listed in `config.toml` after its credentials are +/// gone. +/// 4. Best-effort wipe of per-profile OAuth tokens and cache directory; both +/// are intentionally non-fatal — a missing keychain entry or cache dir is +/// the expected steady state for an already-cleaned profile, not an error. +pub async fn handle_remove( + target: &str, + no_input: bool, + cli_profile: Option<&str>, +) -> anyhow::Result<()> { + let mut config = Config::load_with(cli_profile)?; + crate::config::validate_profile_name(target)?; + + // Pre-validate against a clone before prompting so a typo or + // unremovable target (active profile, default_profile target) doesn't + // make the user click through a confirmation dialog only to error + // afterward. The actual mutation runs below against the real config. + let _ = handle_remove_in_memory(config.global.clone(), target, &config.active_profile_name)?; + + if !no_input { + let confirm = dialoguer::Confirm::new() + .with_prompt(format!( + "Permanently remove profile {target:?}? \ + This deletes its config entry, cache, and OAuth tokens. \ + Shared credentials remain." + )) + .default(false) + .interact()?; + if !confirm { + crate::output::print_warning("Aborted."); + return Ok(()); + } + } + + config.global = handle_remove_in_memory(config.global, target, &config.active_profile_name)?; + config.save_global()?; + + // Config entry is gone — that's the persistent state. The keychain + // and cache cleanup is best-effort: failures here (permission denied, + // locked keychain, IO) shouldn't unwind the config write, but the + // user does need to know they have leftover state to clean up + // manually. Surface as warnings; report overall success. + if let Err(e) = crate::api::auth::clear_profile_creds(target) { + crate::output::print_warning(&format!( + "removed config entry but failed to clear OAuth tokens for {target:?}: {e}. \ + Remove the entries manually via your OS keychain UI." + )); + } + if let Err(e) = crate::cache::clear_profile_cache(target) { + let cache_path = crate::cache::cache_dir(target); + crate::output::print_warning(&format!( + "removed config entry but failed to clear cache for {target:?}: {e}. \ + Remove {} manually if disk space matters.", + cache_path.display() + )); + } + crate::output::print_success(&format!("Removed profile {target:?}")); + Ok(()) +} + +/// Pure logic for `jr auth switch` — separated for testing without filesystem. +pub(super) fn handle_switch_in_memory( + mut global: crate::config::GlobalConfig, + target: &str, +) -> Result { + crate::config::validate_profile_name(target)?; + if !global.profiles.contains_key(target) { + let known: Vec<&str> = global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {target}; known: {}", + if known.is_empty() { + "(none)".into() + } else { + known.join(", ") + } + )) + .into()); + } + global.default_profile = Some(target.to_string()); + Ok(global) +} + +/// `jr auth switch ` — set the default profile in `config.toml`. +pub async fn handle_switch(target: &str, cli_profile: Option<&str>) -> Result<()> { + let mut config = Config::load_with(cli_profile)?; + config.global = handle_switch_in_memory(config.global, target)?; + config.save_global()?; + output::print_success(&format!("Active profile set to {target:?}")); + Ok(()) +} + +/// Render the table-form output of `jr auth list`. The active profile is +/// marked with a leading `*`; others get a leading space so column widths +/// stay stable across rows. Status today is a coarse "do we have a URL on +/// file?" check — credential-store probing comes in Task 13. +pub(super) fn render_list_table(global: &crate::config::GlobalConfig, active: &str) -> String { + let mut rows: Vec> = Vec::new(); + for (name, p) in &global.profiles { + let marker = if name == active { "*" } else { " " }; + let auth = p.auth_method.as_deref().unwrap_or("?"); + let url = p.url.as_deref().unwrap_or("(unset)"); + // STATUS reflects CONFIG presence (URL on file), not credential + // presence. `unset` is more accurate than the old `no-creds` label, + // which suggested the keychain was missing entries when in reality + // the profile entry simply lacks a URL. + let status = if p.url.is_some() { + "configured" + } else { + "unset" + }; + rows.push(vec![ + format!("{marker} {name}"), + url.to_string(), + auth.to_string(), + status.to_string(), + ]); + } + crate::output::render_table(&["NAME", "URL", "AUTH", "STATUS"], &rows) +} + +/// Render the `--output json` form of `jr auth list`: an array of profile +/// objects keyed by name, with `active: true` on exactly one entry. +pub(super) fn render_list_json( + global: &crate::config::GlobalConfig, + active: &str, +) -> Result { + let arr: Vec = global + .profiles + .iter() + .map(|(name, p)| { + serde_json::json!({ + "name": name, + "url": &p.url, + "auth_method": &p.auth_method, + "status": if p.url.is_some() { "configured" } else { "unset" }, + "active": name == active, + }) + }) + .collect(); + Ok(serde_json::to_string_pretty(&arr)?) +} + +/// `jr auth list` — print every configured profile, marking the active one. +pub async fn handle_list( + output: &crate::cli::OutputFormat, + cli_profile: Option<&str>, +) -> Result<()> { + let config = Config::load_with(cli_profile)?; + let rendered = match output { + crate::cli::OutputFormat::Table => { + render_list_table(&config.global, &config.active_profile_name) + } + crate::cli::OutputFormat::Json => { + render_list_json(&config.global, &config.active_profile_name)? + } + }; + println!("{rendered}"); + Ok(()) +} + #[cfg(test)] mod tests { use super::*; - use crate::config::{Config, GlobalConfig, InstanceConfig}; + use crate::config::{Config, GlobalConfig, ProfileConfig}; fn config_with_auth_method(method: Option<&str>) -> Config { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://example.atlassian.net".into()), + auth_method: method.map(str::to_string), + ..ProfileConfig::default() + }, + ); Config { global: GlobalConfig { - instance: InstanceConfig { - url: Some("https://example.atlassian.net".into()), - auth_method: method.map(str::to_string), - ..InstanceConfig::default() - }, + default_profile: Some("default".into()), + profiles, ..Default::default() }, project: Default::default(), + active_profile_name: "default".into(), } } @@ -384,6 +1006,48 @@ mod tests { assert_eq!(chosen_flow(&config, true), AuthFlow::OAuth); } + /// Regression: refresh against a non-active profile must dispatch the + /// flow stored on THAT profile's auth_method, not the active profile's. + /// `chosen_flow(&Config, _)` always reads the active profile, which + /// silently picked the wrong flow when active=api_token but the refresh + /// target=oauth (or vice-versa). `chosen_flow_for_profile` takes the + /// resolved target profile so callers like `refresh_credentials` can + /// thread the right ProfileConfig in. + #[test] + fn chosen_flow_for_profile_inspects_passed_profile_not_active() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".into(), + ProfileConfig { + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); + profiles.insert( + "sandbox".into(), + ProfileConfig { + auth_method: Some("oauth".into()), + ..ProfileConfig::default() + }, + ); + let config = Config { + global: GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }, + project: Default::default(), + active_profile_name: "default".into(), + }; + // chosen_flow without override returns Token (active is api_token) + assert_eq!(chosen_flow(&config, false), AuthFlow::Token); + // chosen_flow_for_profile against sandbox returns OAuth even though + // the active profile is api_token — proves the resolver looks at + // the passed profile, not the active one. + let sandbox = config.global.profiles["sandbox"].clone(); + assert_eq!(chosen_flow_for_profile(&sandbox, false), AuthFlow::OAuth); + } + #[test] fn auth_flow_labels_match_config_and_json_conventions() { assert_eq!(AuthFlow::Token.label(), "api_token"); @@ -553,43 +1217,37 @@ mod tests { ); } - fn config_with_oauth_scopes(scopes: Option<&str>) -> Config { - Config { - global: GlobalConfig { - instance: InstanceConfig { - oauth_scopes: scopes.map(String::from), - ..InstanceConfig::default() - }, - ..GlobalConfig::default() - }, - project: Default::default(), + fn profile_with_oauth_scopes(scopes: Option<&str>) -> ProfileConfig { + ProfileConfig { + oauth_scopes: scopes.map(String::from), + ..ProfileConfig::default() } } #[test] fn resolve_oauth_scopes_none_returns_default() { - let config = config_with_oauth_scopes(None); + let p = profile_with_oauth_scopes(None); assert_eq!( - resolve_oauth_scopes(&config).unwrap(), + resolve_oauth_scopes(&p).unwrap(), auth::DEFAULT_OAUTH_SCOPES ); } #[test] fn resolve_oauth_scopes_trims_and_collapses_whitespace() { - let config = config_with_oauth_scopes(Some( + let p = profile_with_oauth_scopes(Some( " read:issue:jira write:comment:jira\n\toffline_access ", )); assert_eq!( - resolve_oauth_scopes(&config).unwrap(), + resolve_oauth_scopes(&p).unwrap(), "read:issue:jira write:comment:jira offline_access" ); } #[test] fn resolve_oauth_scopes_empty_string_is_config_error() { - let config = config_with_oauth_scopes(Some("")); - let err = resolve_oauth_scopes(&config).unwrap_err(); + let p = profile_with_oauth_scopes(Some("")); + let err = resolve_oauth_scopes(&p).unwrap_err(); let msg = format!("{err:#}"); assert!( msg.contains("oauth_scopes is empty"), @@ -599,8 +1257,8 @@ mod tests { #[test] fn resolve_oauth_scopes_whitespace_only_is_config_error() { - let config = config_with_oauth_scopes(Some(" \n\t ")); - let err = resolve_oauth_scopes(&config).unwrap_err(); + let p = profile_with_oauth_scopes(Some(" \n\t ")); + let err = resolve_oauth_scopes(&p).unwrap_err(); let msg = format!("{err:#}"); assert!( msg.contains("oauth_scopes is empty"), @@ -608,6 +1266,22 @@ mod tests { ); } + /// Regression: `resolve_oauth_scopes` must read the *passed* profile, + /// not anything off a `Config`. `login_oauth(profile, ...)` may target + /// a non-active profile and used to resolve scopes from the active + /// profile, silently returning the wrong scope list. + #[test] + fn resolve_oauth_scopes_inspects_passed_profile_not_active() { + let custom = ProfileConfig { + oauth_scopes: Some("custom:scope offline_access".into()), + ..ProfileConfig::default() + }; + assert_eq!( + resolve_oauth_scopes(&custom).unwrap(), + "custom:scope offline_access" + ); + } + /// The default scope literal is a backward-compatibility contract for /// every user who hasn't opted into `oauth_scopes`. A typo that drops /// `offline_access` would silently break refresh tokens for everyone. @@ -619,6 +1293,230 @@ mod tests { ); } + #[test] + fn resolve_logout_target_defaults_to_active() { + let global = crate::config::GlobalConfig::default(); + assert_eq!(resolve_logout_target(&global, None, "default"), "default"); + assert_eq!( + resolve_logout_target(&global, Some("sandbox"), "default"), + "sandbox" + ); + } + + #[test] + fn switch_to_unknown_profile_returns_error() { + let result = handle_switch_in_memory(GlobalConfig::default(), "ghost"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("unknown profile"), "got: {msg}"); + assert!(msg.contains("ghost"), "got: {msg}"); + } + + #[test] + fn switch_to_known_profile_mutates_default_profile() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert("sandbox".to_string(), ProfileConfig::default()); + let global = GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }; + let mutated = handle_switch_in_memory(global, "sandbox").unwrap(); + assert_eq!(mutated.default_profile.as_deref(), Some("sandbox")); + } + + #[test] + fn remove_active_profile_returns_error() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + crate::config::ProfileConfig::default(), + ); + let global = crate::config::GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..crate::config::GlobalConfig::default() + }; + let result = handle_remove_in_memory(global, "default", "default"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("cannot remove active"), "got: {msg}"); + } + + #[test] + fn remove_unknown_profile_returns_error() { + let global = crate::config::GlobalConfig { + default_profile: Some("default".into()), + ..crate::config::GlobalConfig::default() + }; + let result = handle_remove_in_memory(global, "ghost", "default"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("unknown profile"), "got: {msg}"); + } + + #[test] + fn remove_existing_non_active_profile_succeeds() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + crate::config::ProfileConfig::default(), + ); + profiles.insert( + "sandbox".to_string(), + crate::config::ProfileConfig::default(), + ); + let global = crate::config::GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..crate::config::GlobalConfig::default() + }; + let mutated = handle_remove_in_memory(global, "sandbox", "default").unwrap(); + assert!(!mutated.profiles.contains_key("sandbox")); + assert!(mutated.profiles.contains_key("default")); + } + + fn three_profile_fixture() -> GlobalConfig { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://acme.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); + profiles.insert( + "sandbox".to_string(), + ProfileConfig { + url: Some("https://acme-sandbox.atlassian.net".into()), + auth_method: Some("oauth".into()), + cloud_id: Some("xyz-789".into()), + ..ProfileConfig::default() + }, + ); + profiles.insert( + "staging".to_string(), + ProfileConfig { + url: Some("https://acme-staging.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); + GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + } + } + + #[test] + fn list_table_snapshot() { + let global = three_profile_fixture(); + let rendered = render_list_table(&global, "default"); + insta::assert_snapshot!(rendered); + } + + #[test] + fn list_json_shape() { + let global = three_profile_fixture(); + let json = render_list_json(&global, "default").unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + let arr = parsed.as_array().expect("array"); + assert_eq!(arr.len(), 3); + let active: Vec<&serde_json::Value> = arr + .iter() + .filter(|p| p["active"].as_bool() == Some(true)) + .collect(); + assert_eq!(active.len(), 1, "exactly one active"); + assert_eq!(active[0]["name"], "default"); + } + + #[test] + fn login_create_new_profile_no_input_requires_url() { + let global = crate::config::GlobalConfig::default(); + let result = prepare_login_target(global, Some("sandbox"), None, true, "default"); + assert!(result.is_err()); + let msg = format!("{:#}", result.unwrap_err()); + assert!(msg.contains("--url required"), "got: {msg}"); + } + + #[test] + fn login_create_new_profile_with_url_succeeds() { + let global = crate::config::GlobalConfig::default(); + let (mutated, target) = prepare_login_target( + global, + Some("sandbox"), + Some("https://sandbox.example"), + true, + "default", + ) + .unwrap(); + assert_eq!(target, "sandbox"); + assert_eq!( + mutated.profiles["sandbox"].url.as_deref(), + Some("https://sandbox.example") + ); + } + + #[test] + fn login_existing_profile_with_url_updates_url() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + crate::config::ProfileConfig { + url: Some("https://old.example".into()), + ..crate::config::ProfileConfig::default() + }, + ); + let global = crate::config::GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..crate::config::GlobalConfig::default() + }; + let (mutated, target) = prepare_login_target( + global, + Some("default"), + Some("https://new.example"), + true, + "default", + ) + .unwrap(); + assert_eq!(target, "default"); + assert_eq!( + mutated.profiles["default"].url.as_deref(), + Some("https://new.example") + ); + } + + /// Regression: when `--profile` is omitted, fallback uses the active + /// profile name (which encodes flag > env > config), NOT the + /// `default_profile` config field — using the latter ignores the + /// `JR_PROFILE` env / `--profile` global flag. + #[test] + fn login_falls_back_to_active_profile_name_not_default_profile_field() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "from-env".into(), + crate::config::ProfileConfig { + url: Some("https://from-env.example".into()), + ..crate::config::ProfileConfig::default() + }, + ); + let global = crate::config::GlobalConfig { + default_profile: Some("from-config".into()), + profiles, + ..crate::config::GlobalConfig::default() + }; + let (_mutated, target) = + prepare_login_target(global, None, Some("https://x.example"), true, "from-env") + .unwrap(); + assert_eq!( + target, "from-env", + "must follow active_profile_name, not default_profile field" + ); + } + /// `jr` deliberately does NOT reject mixed classic+granular scopes, /// unknown scope names, or missing `offline_access` — Atlassian returns /// `invalid_scope` at token exchange per the spec's "Out of scope" @@ -633,8 +1531,8 @@ mod tests { "offline_access", // only offline_access ]; for raw in inputs { - let config = config_with_oauth_scopes(Some(raw)); - let result = resolve_oauth_scopes(&config).unwrap_or_else(|e| { + let p = profile_with_oauth_scopes(Some(raw)); + let result = resolve_oauth_scopes(&p).unwrap_or_else(|e| { panic!("resolve_oauth_scopes must pass {raw:?} through unchanged, got error: {e:#}") }); assert_eq!(result, raw, "input {raw:?} must pass through unchanged"); diff --git a/src/cli/board.rs b/src/cli/board.rs index 7e91e720..11c1f365 100644 --- a/src/cli/board.rs +++ b/src/cli/board.rs @@ -236,7 +236,7 @@ async fn handle_view( .collect(); if uuids.iter().any(|u| u.is_some()) { let team_map: std::collections::HashMap = - crate::cache::read_team_cache() + crate::cache::read_team_cache(&config.active_profile_name) .ok() .flatten() .map(|c| c.teams.into_iter().map(|t| (t.id, t.name)).collect()) diff --git a/src/cli/init.rs b/src/cli/init.rs index 6c99e602..216887be 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -1,14 +1,89 @@ use anyhow::{Context, Result}; use dialoguer::{Confirm, Input, Select}; -use crate::config::{ - Config, DefaultsConfig, FieldsConfig, GlobalConfig, InstanceConfig, ProjectConfig, -}; +use crate::config::Config; use crate::{api, output}; pub async fn handle() -> Result<()> { eprintln!("Setting up jr — Jira CLI\n"); + // Multi-profile awareness: if profiles already exist, ask whether to add + // another one rather than overwriting the existing setup. When the user + // opts in, the new profile name is captured in `new_profile_override` + // and threaded into every subsequent `Config::load_*_with` call — the + // earlier `JR_PROFILE_OVERRIDE` env-var seam is gone (it required + // `unsafe { set_var }` under #[tokio::main], which is unsound because + // tokio worker threads exist before the async-main body runs). + // + // Distinguish three failure modes when loading the existing config: + // - config file genuinely absent → fall through to first-run setup + // - `JrError::UserError` (e.g., `JR_PROFILE` points at an unknown + // profile) → recovery is to unset the env / fix `default_profile`, + // NOT to delete config.toml; tell the user that + // - other errors (malformed TOML, permission denied) → tell the user + // to fix or remove the file, since `jr init` would otherwise + // overwrite a broken-but-recoverable file + let existing = match crate::config::Config::load() { + Ok(c) => Some(c), + Err(e) => { + let path = crate::config::global_config_path(); + if let Some(je) = e.downcast_ref::() { + if matches!(je, crate::error::JrError::UserError(_)) { + return Err(e.context( + "config refused to load due to a user-input issue. \ + If JR_PROFILE points to a profile that doesn't exist, \ + unset it; or run 'jr auth list' to see configured \ + profiles.", + )); + } + } + if path.exists() { + return Err(e.context(format!( + "failed to load existing config at {}; fix or remove it before running 'jr init'", + path.display() + ))); + } + None + } + }; + let mut new_profile_override: Option = None; + if let Some(c) = existing.as_ref() { + if !c.global.profiles.is_empty() { + let names: Vec = c.global.profiles.keys().cloned().collect(); + eprintln!("Profiles already configured: {}", names.join(", ")); + let add = Confirm::new() + .with_prompt("Add another profile?") + .default(false) + .interact() + .context("failed to prompt for additional profile")?; + if !add { + return Ok(()); + } + // Re-prompt on collision so a typo matching an existing profile + // name doesn't silently overwrite that profile's URL/auth + // settings later in the flow. + let profile_name: String = loop { + let candidate: String = Input::new() + .with_prompt("Name for the new profile") + .interact_text() + .context("failed to read profile name")?; + if let Err(e) = crate::config::validate_profile_name(&candidate) { + eprintln!("invalid profile name: {e}"); + continue; + } + if c.global.profiles.contains_key(&candidate) { + eprintln!( + "profile {candidate:?} already exists. Pick a different name, or run \ + 'jr auth remove {candidate}' first to overwrite." + ); + continue; + } + break candidate; + }; + new_profile_override = Some(profile_name); + } + } + // Step 1: Instance URL let url: String = Input::new() .with_prompt("Jira instance URL (e.g., https://yourorg.atlassian.net)") @@ -25,20 +100,39 @@ pub async fn handle() -> Result<()> { .interact() .context("failed to prompt for authentication method")?; - let global = GlobalConfig { - instance: InstanceConfig { - url: Some(url.clone()), - ..InstanceConfig::default() - }, - defaults: DefaultsConfig::default(), - fields: FieldsConfig::default(), - }; + // Determine which profile this init flow targets. The override is set + // earlier when the user opted to add a new profile alongside an existing + // one; otherwise we fall back to the literal "default". + let profile_name = new_profile_override + .clone() + .unwrap_or_else(|| "default".into()); - // Save initial config so auth can use it - let config = Config { - global, - project: ProjectConfig::default(), - }; + // Load any existing config, then write the URL into the target profile + // entry. The legacy `[instance]` block is `#[serde(skip_serializing)]` + // since the multi-profile refactor, so writes there are silently dropped + // on save — every persisted field must live under `[profiles.]`. + // + // Reload here (rather than reusing the `existing` we discriminated + // above) so the new-profile choice — captured into `profile_name` + // earlier — is reflected in `active_profile_name`. + // + // Lenient because the override may name a not-yet-created profile (the + // whole point of running `jr init` is to add it). Without lenient, the + // strict active-profile-existence check fires and the previous + // `unwrap_or_else(default)` fallback would silently clobber existing + // profiles on save — flagged by Copilot review on PR #275. + // + // The `?` (no fallback) is safe because the discrimination block at + // the top already separated "config file is malformed/unreadable" + // from "no config yet"; the only reachable failure here would be a + // fresh IO error between then and now, which we want to surface. + let mut config = Config::load_lenient_with(Some(&profile_name))?; + config + .global + .profiles + .entry(profile_name.clone()) + .or_default() + .url = Some(url.clone()); config.save_global()?; // Step 3: Authenticate. `jr init` is inherently interactive (Select @@ -46,16 +140,17 @@ pub async fn handle() -> Result<()> { // credential prompt. Flags aren't plumbed through init — users who want // a non-interactive setup should run `jr auth login` directly. if auth_choice == 0 { - crate::cli::auth::login_oauth(None, None, false).await?; + crate::cli::auth::login_oauth(&profile_name, None, None, false).await?; } else { - crate::cli::auth::login_token(None, None, false).await?; - let mut config = Config::load()?; - config.global.instance.auth_method = Some("api_token".into()); - config.save_global()?; + // login_token already persists auth_method = "api_token" to + // [profiles.] internally — no additional load+save needed + // here. (Doing a redundant reload + write is also a last-writer- + // wins race against any concurrent jr invocation.) + crate::cli::auth::login_token(&profile_name, None, None, false).await?; } // Step 4: Per-project setup - let config = Config::load()?; + let config = Config::load_with(Some(&profile_name))?; let client = api::client::JiraClient::from_config(&config, false)?; let setup_project = Confirm::new() @@ -96,8 +191,14 @@ pub async fn handle() -> Result<()> { // Step 5: Discover team field if let Ok(Some(team_id)) = client.find_team_field_id().await { - let mut config = Config::load()?; - config.global.fields.team_field_id = Some(team_id); + let mut config = Config::load_with(Some(&profile_name))?; + let active = config.active_profile_name.clone(); + config + .global + .profiles + .entry(active) + .or_default() + .team_field_id = Some(team_id); config.save_global()?; } @@ -133,8 +234,14 @@ pub async fn handle() -> Result<()> { }; if let Some(id) = field_id { - let mut config = Config::load()?; - config.global.fields.story_points_field_id = Some(id); + let mut config = Config::load_with(Some(&profile_name))?; + let active = config.active_profile_name.clone(); + config + .global + .profiles + .entry(active) + .or_default() + .story_points_field_id = Some(id); config.save_global()?; } } @@ -149,9 +256,11 @@ pub async fn handle() -> Result<()> { .trim_start_matches("http://") .trim_end_matches('/'); if let Ok(metadata) = client.get_org_metadata(hostname).await { - let mut config = Config::load()?; - config.global.instance.cloud_id = Some(metadata.cloud_id); - config.global.instance.org_id = Some(metadata.org_id.clone()); + let mut config = Config::load_with(Some(&profile_name))?; + let active = config.active_profile_name.clone(); + let entry = config.global.profiles.entry(active).or_default(); + entry.cloud_id = Some(metadata.cloud_id.clone()); + entry.org_id = Some(metadata.org_id.clone()); config.save_global()?; // Step 7: Prefetch team list into cache @@ -163,7 +272,7 @@ pub async fn handle() -> Result<()> { name: t.display_name, }) .collect(); - if let Err(err) = crate::cache::write_team_cache(&cached) { + if let Err(err) = crate::cache::write_team_cache(&config.active_profile_name, &cached) { eprintln!( "warning: failed to warm team cache: {err}. First `jr team list` will refetch." ); diff --git a/src/cli/issue/helpers.rs b/src/cli/issue/helpers.rs index 38fcf48b..3717f67b 100644 --- a/src/cli/issue/helpers.rs +++ b/src/cli/issue/helpers.rs @@ -67,7 +67,8 @@ pub(super) async fn resolve_team_field( // 3. Load teams from cache (or fetch if missing/expired). `cache_was_fresh` // tells step 5 whether an auto-refresh-on-miss is worth attempting — // no point re-fetching a list we just fetched. - let (teams, cache_was_fresh) = match crate::cache::read_team_cache()? { + let (teams, cache_was_fresh) = match crate::cache::read_team_cache(&config.active_profile_name)? + { Some(cached) => (cached.teams, false), None => ( crate::cli::team::fetch_and_cache_teams(config, client).await?, diff --git a/src/cli/issue/list.rs b/src/cli/issue/list.rs index 40597ed3..9674737e 100644 --- a/src/cli/issue/list.rs +++ b/src/cli/issue/list.rs @@ -510,7 +510,7 @@ pub(super) async fn handle_list( // entry falls back to the UUID. Cache population is not this // command's responsibility. let team_map: std::collections::HashMap = - crate::cache::read_team_cache() + crate::cache::read_team_cache(&config.active_profile_name) .ok() .flatten() .map(|c| c.teams.into_iter().map(|t| (t.id, t.name)).collect()) diff --git a/src/cli/issue/view.rs b/src/cli/issue/view.rs index 04099f6d..00dcdd81 100644 --- a/src/cli/issue/view.rs +++ b/src/cli/issue/view.rs @@ -250,27 +250,28 @@ pub(super) async fn handle_view( if let Some(field_id) = team_field_id { if let Some(team_uuid) = issue.fields.team_id(field_id, client.verbose()) { - let team_display = match crate::cache::read_team_cache() { - Ok(Some(c)) => c - .teams - .into_iter() - .find(|t| t.id == team_uuid) - .map(|t| t.name) - .unwrap_or_else(|| { - format!( - "{} (name not cached — run 'jr team list --refresh')", - team_uuid - ) - }), - Ok(None) => format!( - "{} (name not cached — run 'jr team list --refresh')", - team_uuid - ), - Err(e) => { - eprintln!("warning: failed to read team cache: {e}"); - format!("{} (team cache unreadable)", team_uuid) - } - }; + let team_display = + match crate::cache::read_team_cache(&config.active_profile_name) { + Ok(Some(c)) => c + .teams + .into_iter() + .find(|t| t.id == team_uuid) + .map(|t| t.name) + .unwrap_or_else(|| { + format!( + "{} (name not cached — run 'jr team list --refresh')", + team_uuid + ) + }), + Ok(None) => format!( + "{} (name not cached — run 'jr team list --refresh')", + team_uuid + ), + Err(e) => { + eprintln!("warning: failed to read team cache: {e}"); + format!("{} (team cache unreadable)", team_uuid) + } + }; rows.push(vec!["Team".into(), team_display]); } } diff --git a/src/cli/issue/workflow.rs b/src/cli/issue/workflow.rs index df5d248f..9215978e 100644 --- a/src/cli/issue/workflow.rs +++ b/src/cli/issue/workflow.rs @@ -98,8 +98,9 @@ fn resolve_resolution_by_name(resolutions: &[Resolution], query: &str) -> Result /// warns on stderr rather than silently dropping so a partial Atlassian /// response is visible. async fn load_resolutions(client: &JiraClient, refresh: bool) -> Result> { + let profile = client.profile_name(); if !refresh { - if let Some(c) = crate::cache::read_resolutions_cache()? { + if let Some(c) = crate::cache::read_resolutions_cache(profile)? { return Ok(c .resolutions .into_iter() @@ -130,7 +131,7 @@ async fn load_resolutions(client: &JiraClient, refresh: bool) -> Result JR_PROFILE > config > "default") + #[arg(long, global = true)] + pub profile: Option, } #[derive(Clone, Copy, ValueEnum)] @@ -185,9 +189,16 @@ pub enum AssetsCommand { pub enum AuthCommand { /// Authenticate with Jira Login { + /// Profile to log in to (creates it if absent). Defaults to active profile. + #[arg(long)] + profile: Option, + /// Jira instance URL (required when creating a new profile under --no-input). + #[arg(long)] + url: Option, /// Use OAuth 2.0 instead of API token (requires your own OAuth app). /// Scope list is Atlassian's recommended classic set by default; - /// override via `[instance].oauth_scopes` in config.toml. + /// override via `[profiles.].oauth_scopes` in config.toml — see + /// Configuration below. #[arg(long)] oauth: bool, /// Jira email (API token flow). Prefer $JR_EMAIL over this flag. @@ -206,7 +217,11 @@ pub enum AuthCommand { client_secret: Option, }, /// Show authentication status - Status, + Status { + /// Profile to show status for. Defaults to active profile. + #[arg(long)] + profile: Option, + }, /// Clear stored credentials and re-run the login flow. /// /// On macOS, run this after upgrading `jr` (e.g., `brew upgrade`, binary @@ -215,6 +230,9 @@ pub enum AuthCommand { /// the creator of fresh entries, avoiding repeated "allow access" /// prompts. See issue #207. Refresh { + /// Profile to refresh credentials for. Defaults to active profile. + #[arg(long)] + profile: Option, /// Use OAuth 2.0 instead of API token (matches `jr auth login --oauth`) #[arg(long)] oauth: bool, @@ -233,6 +251,27 @@ pub enum AuthCommand { #[arg(long)] client_secret: Option, }, + /// Set the default profile in config.toml. + Switch { + /// Profile name to make active. Must already exist in config. + name: String, + }, + /// List all configured profiles, marking the active one. + List, + /// Clear OAuth tokens for a profile (profile entry stays in config). + /// Shared API-token credential is NEVER touched. + Logout { + /// Profile to log out of. Defaults to active profile. + #[arg(long)] + profile: Option, + }, + /// Permanently delete a profile (config + cache + per-profile OAuth tokens). + /// Shared credentials are NEVER touched. + Remove { + /// Profile name to remove. Cannot be the active profile — + /// switch first with `jr auth switch`. + name: String, + }, } #[derive(Subcommand)] diff --git a/src/cli/snapshots/jr__cli__auth__tests__list_table_snapshot.snap b/src/cli/snapshots/jr__cli__auth__tests__list_table_snapshot.snap new file mode 100644 index 00000000..68a3309e --- /dev/null +++ b/src/cli/snapshots/jr__cli__auth__tests__list_table_snapshot.snap @@ -0,0 +1,11 @@ +--- +source: src/cli/auth.rs +expression: rendered +--- +┌───────────┬────────────────────────────────────┬───────────┬────────────┐ +│ NAME ┆ URL ┆ AUTH ┆ STATUS │ +╞═══════════╪════════════════════════════════════╪═══════════╪════════════╡ +│ * default ┆ https://acme.atlassian.net ┆ api_token ┆ configured │ +│ sandbox ┆ https://acme-sandbox.atlassian.net ┆ oauth ┆ configured │ +│ staging ┆ https://acme-staging.atlassian.net ┆ api_token ┆ configured │ +└───────────┴────────────────────────────────────┴───────────┴────────────┘ diff --git a/src/cli/sprint.rs b/src/cli/sprint.rs index a4916701..8f3ad7ed 100644 --- a/src/cli/sprint.rs +++ b/src/cli/sprint.rs @@ -294,7 +294,7 @@ async fn handle_current( .collect(); if uuids.iter().any(|u| u.is_some()) { let team_map: std::collections::HashMap = - crate::cache::read_team_cache() + crate::cache::read_team_cache(&config.active_profile_name) .ok() .flatten() .map(|c| c.teams.into_iter().map(|t| (t.id, t.name)).collect()) diff --git a/src/cli/team.rs b/src/cli/team.rs index 3ab0e0c5..7b9bd2fd 100644 --- a/src/cli/team.rs +++ b/src/cli/team.rs @@ -29,7 +29,7 @@ async fn handle_list( let teams = if refresh { fetch_and_cache_teams(config, client).await? } else { - match cache::read_team_cache()? { + match cache::read_team_cache(&config.active_profile_name)? { Some(cached) => cached.teams, None => fetch_and_cache_teams(config, client).await?, } @@ -67,20 +67,26 @@ pub async fn fetch_and_cache_teams( }) .collect(); - cache::write_team_cache(&cached)?; + cache::write_team_cache(&config.active_profile_name, &cached)?; Ok(cached) } /// Resolve org_id: read from config, or discover via GraphQL and persist. /// Uses hostNames-based GraphQL query to get both cloudId and orgId in one call. pub async fn resolve_org_id(config: &Config, client: &JiraClient) -> Result { - if let Some(ref org_id) = config.global.instance.org_id { + let active = config.active_profile(); + if let Some(ref org_id) = active.org_id { return Ok(org_id.clone()); } - // Extract hostname from instance URL - let url = config.global.instance.url.as_ref().ok_or_else(|| { - JrError::ConfigError("No Jira instance configured. Run \"jr init\" first.".into()) + // Extract hostname from instance URL. Multi-profile world: the URL + // lives on the active profile, so name it in the error and point the + // user at the profile-aware login command. + let url = active.url.as_ref().ok_or_else(|| { + JrError::ConfigError(format!( + "Profile {:?} has no URL configured. Run \"jr auth login --profile {}\" or \"jr init\".", + config.active_profile_name, config.active_profile_name + )) })?; let hostname = url .trim_start_matches("https://") @@ -90,10 +96,24 @@ pub async fn resolve_org_id(config: &Config, client: &JiraClient) -> Result, pub story_points_field_id: Option, } -#[derive(Debug, Deserialize, Serialize, Default)] +#[derive(Debug, Deserialize, Serialize, Default, Clone)] +pub struct ProfileConfig { + pub url: Option, + pub auth_method: Option, + pub cloud_id: Option, + pub org_id: Option, + pub oauth_scopes: Option, + pub team_field_id: Option, + pub story_points_field_id: Option, +} + +#[derive(Debug, Deserialize, Serialize, Default, Clone)] pub struct GlobalConfig { + /// New-shape: name of the active profile. + /// Resolved precedence: --profile > JR_PROFILE > this field > "default". + /// `Option` because legacy configs don't have it. + #[serde(default)] + pub default_profile: Option, + + /// New-shape: named profiles. #[serde(default)] + pub profiles: std::collections::BTreeMap, + + /// Legacy single-instance config — read for migration only. + /// Kept on the in-memory struct so callers reading legacy fields keep + /// working during the transition. Skipped on serialize so saved configs + /// only contain the new shape. + #[serde(default, skip_serializing)] pub instance: InstanceConfig, + + /// Legacy global custom-field IDs — read for migration only. + #[serde(default, skip_serializing)] + pub fields: FieldsConfig, + #[serde(default)] pub defaults: DefaultsConfig, - #[serde(default)] - pub fields: FieldsConfig, } -#[derive(Debug, Deserialize, Serialize, Default)] +#[derive(Debug, Deserialize, Serialize, Default, Clone)] pub struct InstanceConfig { pub url: Option, pub cloud_id: Option, @@ -32,7 +60,7 @@ pub struct InstanceConfig { pub oauth_scopes: Option, } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug, Deserialize, Serialize, Clone)] pub struct DefaultsConfig { pub output: String, } @@ -55,17 +83,204 @@ pub struct ProjectConfig { pub struct Config { pub global: GlobalConfig, pub project: ProjectConfig, + /// Resolved at load() — flag > JR_PROFILE > default_profile > "default". + pub active_profile_name: String, +} + +/// Resolve the active profile name from precedence chain: +/// 1. cli_flag (--profile) +/// 2. env var (JR_PROFILE) +/// 3. config.default_profile field +/// 4. literal "default" +pub fn resolve_active_profile_name( + config: &GlobalConfig, + cli_flag: Option<&str>, + env_var: Option, +) -> String { + if let Some(name) = cli_flag { + return name.to_string(); + } + if let Some(name) = env_var { + return name; + } + if let Some(name) = config.default_profile.as_ref() { + return name.clone(); + } + "default".to_string() +} + +/// Validate a profile name. See docs/specs/multi-profile-auth.md "Profile Name Validation". +pub fn validate_profile_name(name: &str) -> Result<(), JrError> { + const RESERVED_WINDOWS: &[&str] = &[ + "CON", "NUL", "AUX", "PRN", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", + "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + ]; + + if name.is_empty() || name.len() > 64 { + return Err(invalid_profile_name(name)); + } + if !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return Err(invalid_profile_name(name)); + } + let upper = name.to_ascii_uppercase(); + if RESERVED_WINDOWS.contains(&upper.as_str()) { + return Err(invalid_profile_name(name)); + } + Ok(()) +} + +fn invalid_profile_name(name: &str) -> JrError { + JrError::UserError(format!( + "invalid profile name {name:?}; allowed: A-Z a-z 0-9 _ - up to 64 chars; \ + reserved Windows names (CON, NUL, AUX, PRN, COM1-9, LPT1-9) excluded" + )) +} + +/// Pure migration: copies a `GlobalConfig`'s legacy `[instance]` + `[fields]` +/// data into a new `[profiles.default]` entry. No-op if already in new shape. +/// +/// Legacy fields are intentionally preserved during the transition (Tasks 4-15) +/// so callers that still read `global.instance.*` / `global.fields.*` keep +/// working until Tasks 7/8 migrate them to read `active_profile()` instead. +/// Task 16 stops serializing the legacy fields, so they fall off disk on the +/// next save. +pub fn migrate_legacy_global(mut global: GlobalConfig) -> GlobalConfig { + if !global.profiles.is_empty() { + return global; + } + + if global.instance.url.is_none() + && global.instance.auth_method.is_none() + && global.instance.cloud_id.is_none() + && global.instance.org_id.is_none() + && global.instance.oauth_scopes.is_none() + && global.fields.team_field_id.is_none() + && global.fields.story_points_field_id.is_none() + { + return global; + } + + let profile = ProfileConfig { + url: global.instance.url.clone(), + auth_method: global.instance.auth_method.clone(), + cloud_id: global.instance.cloud_id.clone(), + org_id: global.instance.org_id.clone(), + oauth_scopes: global.instance.oauth_scopes.clone(), + team_field_id: global.fields.team_field_id.clone(), + story_points_field_id: global.fields.story_points_field_id.clone(), + }; + global.profiles.insert("default".to_string(), profile); + global.default_profile = Some("default".to_string()); + global +} + +fn save_global_to(path: &std::path::Path, global: &GlobalConfig) -> anyhow::Result<()> { + if let Some(dir) = path.parent() { + std::fs::create_dir_all(dir)?; + } + let content = toml::to_string_pretty(global)?; + std::fs::write(path, content)?; + Ok(()) } impl Config { + /// Strict load — used by every command except `jr auth login`. + /// Errors if the resolved active profile isn't in `[profiles]`. pub fn load() -> anyhow::Result { + Self::load_with(None) + } + + /// Variant that accepts a CLI-flag profile override. + /// + /// Threading the `--profile` value as a parameter (instead of through an + /// env-var seam like the legacy `JR_PROFILE_OVERRIDE`) avoids + /// `unsafe { std::env::set_var(...) }` under `#[tokio::main]`, where + /// worker threads exist before the async-main body runs and POSIX + /// `setenv` is not thread-safe. + pub fn load_with(cli_profile: Option<&str>) -> anyhow::Result { + Self::load_inner(cli_profile, true) + } + + /// Lenient load — used by `jr auth login` only, which legitimately + /// creates profiles on demand. Skips the active-profile existence + /// check; otherwise identical to [`Config::load`]. + pub fn load_lenient() -> anyhow::Result { + Self::load_lenient_with(None) + } + + /// Lenient variant that accepts a CLI-flag profile override. See + /// [`Config::load_with`] for the threading rationale. + pub fn load_lenient_with(cli_profile: Option<&str>) -> anyhow::Result { + Self::load_inner(cli_profile, false) + } + + fn load_inner(cli_profile: Option<&str>, strict: bool) -> anyhow::Result { let global_path = global_config_path(); - let global: GlobalConfig = Figment::new() + + // Read with env-overlay for in-memory use. The rest of the program + // sees `JR_*` env overrides applied on top of `config.toml`. + let mut global: GlobalConfig = Figment::new() .merge(Serialized::defaults(GlobalConfig::default())) .merge(Toml::file(&global_path)) .merge(Env::prefixed("JR_")) .extract()?; + let needs_migration = global.profiles.is_empty() + && (global.instance.url.is_some() + || global.instance.auth_method.is_some() + || global.instance.cloud_id.is_some() + || global.instance.org_id.is_some() + || global.instance.oauth_scopes.is_some() + || global.fields.team_field_id.is_some() + || global.fields.story_points_field_id.is_some()); + + if needs_migration { + // Write-back uses file-only data (NO env overlay) so transient + // `JR_*` env vars set for one invocation (e.g., + // `JR_DEFAULTS_OUTPUT=json`) can never bleed into the migrated + // `config.toml`. Without this, an env value set at upgrade time + // would be silently baked into the user's on-disk config and + // persist across future invocations even when the env var is + // unset. + // + // In-memory `global` still gets migrated below so callers see + // the new `[profiles.default]` entry; this two-source pattern + // (env-merged for in-memory + file-only for save) keeps the + // migration transparent without polluting the saved file. + let file_only_global: GlobalConfig = Figment::new() + .merge(Serialized::defaults(GlobalConfig::default())) + .merge(Toml::file(&global_path)) + .extract()?; + let to_save = migrate_legacy_global(file_only_global); + save_global_to(&global_path, &to_save)?; + global = migrate_legacy_global(global); + eprintln!( + "Migrated config to multi-profile layout (single profile \"default\"). \ + Run 'jr auth list' to view profiles." + ); + } + + // Validate every profile name in the map. A hand-edited config with + // quoted/invalid keys (e.g. `[profiles."foo:bar"]`) would otherwise + // deserialize fine but produce names that can't be targeted by + // switch/remove/logout/status (which validate input) AND would create + // unsafe cache / keyring namespaces if used downstream. Placed after + // the migration block (so the synthetic "default" key from migration + // is also covered) and before resolving `active_profile_name` (so a + // fresh first-run with empty profiles isn't gated). + for name in global.profiles.keys() { + validate_profile_name(name).map_err(|_| { + JrError::UserError(format!( + "invalid profile name {name:?} in config.toml; allowed: \ + A-Z a-z 0-9 _ - up to 64 chars; reserved Windows names \ + (CON, NUL, AUX, PRN, COM1-9, LPT1-9) excluded" + )) + })?; + } + let project = Self::find_project_config() .map(|path| -> anyhow::Result { Ok(Figment::new() @@ -75,7 +290,48 @@ impl Config { .transpose()? .unwrap_or_default(); - Ok(Config { global, project }) + // The `--profile` CLI flag is threaded in as a parameter rather than + // via an env-var seam. Earlier rounds used `JR_PROFILE_OVERRIDE`, but + // setting it inside `#[tokio::main]` requires `unsafe { set_var }` at + // a point where tokio worker threads already exist — POSIX `setenv` + // is not thread-safe, so the cleaner fix is to drop the env-var seam + // entirely. JR_PROFILE remains the user-facing env var. + let env_profile = std::env::var("JR_PROFILE").ok(); + let active_profile_name = resolve_active_profile_name(&global, cli_profile, env_profile); + // Validate the resolved name. JR_PROFILE / --profile / default_profile + // all flow into cache paths and keyring keys, so a bad value (e.g. + // "foo:bar" or path separators) must be rejected at the config boundary. + validate_profile_name(&active_profile_name)?; + + // Verify the resolved active profile exists in [profiles] (when any + // profiles are configured). A fresh install with no profiles yet is + // allowed: jr init / jr auth login will create the first one. + // + // Skipped for `load_lenient` (used only by `jr auth login`), which + // legitimately creates the target profile on demand and would + // otherwise be locked out of `--profile newprof --url ...`. + // + // UserError (exit 64) instead of ConfigError (exit 78) because the + // invalid input source is the user (--profile flag, JR_PROFILE env, + // or a hand-edited default_profile field) — not a malformed config + // file. Matches the wording used by switch/remove/logout/status. + if strict + && !global.profiles.is_empty() + && !global.profiles.contains_key(&active_profile_name) + { + let known: Vec<&str> = global.profiles.keys().map(String::as_str).collect(); + return Err(JrError::UserError(format!( + "unknown profile: {active_profile_name}; known: {}", + known.join(", ") + )) + .into()); + } + + Ok(Config { + global, + project, + active_profile_name, + }) } fn find_project_config() -> Option { @@ -92,17 +348,23 @@ impl Config { } pub fn base_url(&self) -> anyhow::Result { - // JR_BASE_URL env var overrides everything (used by tests to inject wiremock URL) if let Ok(override_url) = std::env::var("JR_BASE_URL") { return Ok(override_url.trim_end_matches('/').to_string()); } - - let url = self.global.instance.url.as_ref().ok_or_else(|| { - JrError::ConfigError("No Jira instance configured. Run \"jr init\" first.".into()) + let profile = self.global.profiles.get(&self.active_profile_name).ok_or_else(|| { + JrError::ConfigError(format!( + "No Jira instance configured for profile {:?}. Run \"jr auth login --profile {}\" or \"jr init\".", + self.active_profile_name, self.active_profile_name + )) })?; - - if let Some(cloud_id) = &self.global.instance.cloud_id { - if self.global.instance.auth_method.as_deref() == Some("oauth") { + let url = profile.url.as_ref().ok_or_else(|| { + JrError::ConfigError(format!( + "Profile {:?} has no URL configured. Run \"jr auth login --profile {}\".", + self.active_profile_name, self.active_profile_name + )) + })?; + if let Some(cloud_id) = &profile.cloud_id { + if profile.auth_method.as_deref() == Some("oauth") { return Ok(format!("https://api.atlassian.com/ex/jira/{cloud_id}")); } } @@ -119,13 +381,68 @@ impl Config { cli_override.or(self.project.board_id) } + /// Look up the active profile. Returns a default-empty `ProfileConfig` if + /// the active profile isn't in the map (legacy migration path runs before + /// most callers reach this; tests can also exercise the empty case). + pub fn active_profile(&self) -> ProfileConfig { + self.global + .profiles + .get(&self.active_profile_name) + .cloned() + .unwrap_or_default() + } + + /// Strict variant — errors if the active profile isn't configured. + pub fn active_profile_or_err(&self) -> anyhow::Result<&ProfileConfig> { + self.global + .profiles + .get(&self.active_profile_name) + .ok_or_else(|| { + let known: Vec<&str> = self.global.profiles.keys().map(String::as_str).collect(); + JrError::ConfigError(format!( + "active profile {:?} not in [profiles]; known: {}; \ + fix config.toml or run \"jr auth list\"", + self.active_profile_name, + if known.is_empty() { + "(none)".into() + } else { + known.join(", ") + } + )) + .into() + }) + } + pub fn save_global(&self) -> anyhow::Result<()> { - let dir = global_config_dir(); - std::fs::create_dir_all(&dir)?; - let path = dir.join("config.toml"); - let content = toml::to_string_pretty(&self.global)?; - std::fs::write(path, content)?; - Ok(()) + let path = global_config_path(); + // Read the file-only baseline (no `JR_*` env overlay) so transient + // env overrides on the invocation that mutates a profile (e.g., + // `JR_DEFAULTS_OUTPUT=json jr auth switch sandbox`) can't leak + // into the saved config.toml. The in-memory `self.global` has env + // overlays applied for the duration of this process; we want to + // persist only the structural multi-profile changes (default + // profile + profiles map), preserving everything else from disk. + // + // If the file doesn't exist yet (fresh install), start with the + // default-empty `GlobalConfig` — legitimate first-run case where + // `defaults`/etc. have nothing to preserve from disk anyway. + let mut to_save: GlobalConfig = if path.exists() { + Figment::new() + .merge(Serialized::defaults(GlobalConfig::default())) + .merge(Toml::file(&path)) + .extract()? + } else { + GlobalConfig::default() + }; + + // Overlay only the multi-profile fields. These are what callers + // of `save_global` mutate (handle_switch, handle_remove, + // handle_login, login_token/oauth, jr init). Other fields like + // `defaults.output` are preserved from the file-only baseline. + to_save.default_profile = self.global.default_profile.clone(); + to_save.profiles = self.global.profiles.clone(); + + save_global_to(&path, &to_save) } } @@ -181,17 +498,24 @@ mod tests { #[test] fn test_base_url_api_token() { let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://myorg.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); let config = Config { global: GlobalConfig { - instance: InstanceConfig { - url: Some("https://myorg.atlassian.net".into()), - auth_method: Some("api_token".into()), - ..InstanceConfig::default() - }, + default_profile: Some("default".into()), + profiles, defaults: DefaultsConfig::default(), ..Default::default() }, project: ProjectConfig::default(), + active_profile_name: "default".into(), }; assert_eq!(config.base_url().unwrap(), "https://myorg.atlassian.net"); } @@ -199,18 +523,25 @@ mod tests { #[test] fn test_base_url_oauth() { let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://myorg.atlassian.net".into()), + cloud_id: Some("abc-123".into()), + auth_method: Some("oauth".into()), + ..ProfileConfig::default() + }, + ); let config = Config { global: GlobalConfig { - instance: InstanceConfig { - url: Some("https://myorg.atlassian.net".into()), - cloud_id: Some("abc-123".into()), - auth_method: Some("oauth".into()), - ..InstanceConfig::default() - }, + default_profile: Some("default".into()), + profiles, defaults: DefaultsConfig::default(), ..Default::default() }, project: ProjectConfig::default(), + active_profile_name: "default".into(), }; assert_eq!( config.base_url().unwrap(), @@ -224,10 +555,63 @@ mod tests { let config = Config { global: GlobalConfig::default(), project: ProjectConfig::default(), + active_profile_name: String::new(), }; assert!(config.base_url().is_err()); } + #[test] + fn base_url_uses_active_profile() { + let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "sandbox".to_string(), + ProfileConfig { + url: Some("https://sandbox.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); + let config = Config { + global: GlobalConfig { + default_profile: Some("sandbox".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "sandbox".into(), + }; + assert_eq!(config.base_url().unwrap(), "https://sandbox.atlassian.net"); + } + + #[test] + fn base_url_uses_active_profile_oauth_path() { + let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://acme.atlassian.net".into()), + auth_method: Some("oauth".into()), + cloud_id: Some("abc-123".into()), + ..ProfileConfig::default() + }, + ); + let config = Config { + global: GlobalConfig { + default_profile: Some("default".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "default".into(), + }; + assert_eq!( + config.base_url().unwrap(), + "https://api.atlassian.com/ex/jira/abc-123" + ); + } + #[test] fn test_project_key_cli_override() { let config = Config { @@ -236,6 +620,7 @@ mod tests { project: Some("FOO".into()), board_id: None, }, + active_profile_name: String::new(), }; assert_eq!(config.project_key(Some("BAR")), Some("BAR".into())); assert_eq!(config.project_key(None), Some("FOO".into())); @@ -249,6 +634,7 @@ mod tests { project: None, board_id: Some(42), }, + active_profile_name: String::new(), }; // CLI override wins assert_eq!(config.board_id(Some(99)), Some(99)); @@ -272,17 +658,24 @@ mod tests { #[test] fn test_base_url_trailing_slash_trimmed() { let _guard = ENV_MUTEX.lock().unwrap(); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://myorg.atlassian.net/".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); let config = Config { global: GlobalConfig { - instance: InstanceConfig { - url: Some("https://myorg.atlassian.net/".into()), - auth_method: Some("api_token".into()), - ..InstanceConfig::default() - }, + default_profile: Some("default".into()), + profiles, defaults: DefaultsConfig::default(), ..Default::default() }, project: ProjectConfig::default(), + active_profile_name: "default".into(), }; assert_eq!(config.base_url().unwrap(), "https://myorg.atlassian.net"); } @@ -292,34 +685,44 @@ mod tests { let dir = TempDir::new().unwrap(); let config_path = dir.path().join("config.toml"); + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "default".to_string(), + ProfileConfig { + url: Some("https://test.atlassian.net".into()), + auth_method: Some("api_token".into()), + ..ProfileConfig::default() + }, + ); + let config = Config { global: GlobalConfig { - instance: InstanceConfig { - url: Some("https://test.atlassian.net".into()), - auth_method: Some("api_token".into()), - ..InstanceConfig::default() - }, + default_profile: Some("default".into()), + profiles, defaults: DefaultsConfig::default(), ..Default::default() }, project: ProjectConfig::default(), + active_profile_name: "default".into(), }; // Write config to temp path let content = toml::to_string_pretty(&config.global).unwrap(); fs::write(&config_path, &content).unwrap(); + // Legacy [instance]/[fields] blocks must not appear in serialized output + assert!(!content.contains("[instance]")); + assert!(!content.contains("[fields]")); + // Read it back let loaded: GlobalConfig = Figment::new() .merge(Toml::file(&config_path)) .extract() .unwrap(); - assert_eq!( - loaded.instance.url.as_deref(), - Some("https://test.atlassian.net") - ); - assert_eq!(loaded.instance.auth_method.as_deref(), Some("api_token")); + let p = loaded.profiles.get("default").expect("default profile"); + assert_eq!(p.url.as_deref(), Some("https://test.atlassian.net")); + assert_eq!(p.auth_method.as_deref(), Some("api_token")); } #[test] @@ -351,4 +754,470 @@ mod tests { assert!(config.instance.oauth_scopes.is_none()); } + + #[test] + fn validate_profile_name_accepts_alphanumeric_dash_underscore() { + assert!(validate_profile_name("default").is_ok()); + assert!(validate_profile_name("sandbox-uat").is_ok()); + assert!(validate_profile_name("team_a").is_ok()); + assert!(validate_profile_name("Prod1").is_ok()); + assert!(validate_profile_name("a").is_ok()); + assert!(validate_profile_name(&"a".repeat(64)).is_ok()); + } + + #[test] + fn validate_profile_name_rejects_invalid_chars() { + for bad in [ + "", " ", "foo bar", "foo:bar", "foo/bar", "foo.bar", "..", ".", + ] { + assert!( + validate_profile_name(bad).is_err(), + "expected {bad:?} to be rejected" + ); + } + } + + #[test] + fn validate_profile_name_rejects_too_long() { + let too_long = "a".repeat(65); + assert!(validate_profile_name(&too_long).is_err()); + } + + #[test] + fn validate_profile_name_rejects_windows_reserved_names_case_insensitive() { + for bad in [ + "CON", "con", "Con", "NUL", "nul", "AUX", "aux", "PRN", "prn", "COM1", "com9", "LPT1", + "lpt9", + ] { + assert!( + validate_profile_name(bad).is_err(), + "expected Windows reserved name {bad:?} to be rejected" + ); + } + } + + #[test] + fn profile_config_roundtrip() { + let toml = r#" + url = "https://acme.atlassian.net" + auth_method = "oauth" + cloud_id = "abc-123" + org_id = "def-456" + oauth_scopes = "read:jira-work offline_access" + team_field_id = "customfield_10001" + story_points_field_id = "customfield_10002" + "#; + let p: ProfileConfig = toml::from_str(toml).unwrap(); + assert_eq!(p.url.as_deref(), Some("https://acme.atlassian.net")); + assert_eq!(p.auth_method.as_deref(), Some("oauth")); + assert_eq!(p.cloud_id.as_deref(), Some("abc-123")); + assert_eq!(p.org_id.as_deref(), Some("def-456")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_10001")); + assert_eq!( + p.story_points_field_id.as_deref(), + Some("customfield_10002") + ); + } + + #[test] + fn global_config_parses_new_shape() { + let toml = r#" + default_profile = "default" + + [profiles.default] + url = "https://acme.atlassian.net" + auth_method = "api_token" + + [profiles.sandbox] + url = "https://acme-sandbox.atlassian.net" + auth_method = "oauth" + cloud_id = "xyz-789" + "#; + let cfg: GlobalConfig = toml::from_str(toml).unwrap(); + assert_eq!(cfg.default_profile.as_deref(), Some("default")); + assert_eq!(cfg.profiles.len(), 2); + assert!(cfg.profiles.contains_key("default")); + assert!(cfg.profiles.contains_key("sandbox")); + assert_eq!(cfg.profiles["sandbox"].cloud_id.as_deref(), Some("xyz-789")); + } + + #[test] + fn resolve_active_profile_name_uses_cli_flag_when_set() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, Some("flag-value"), None); + assert_eq!(name, "flag-value"); + } + + #[test] + fn resolve_active_profile_name_uses_env_when_no_flag() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, None, Some("env-value".into())); + assert_eq!(name, "env-value"); + } + + #[test] + fn resolve_active_profile_name_uses_config_when_no_flag_or_env() { + let cfg = GlobalConfig { + default_profile: Some("config-default".into()), + ..GlobalConfig::default() + }; + let name = resolve_active_profile_name(&cfg, None, None); + assert_eq!(name, "config-default"); + } + + #[test] + fn resolve_active_profile_name_falls_back_to_default_literal() { + let cfg = GlobalConfig::default(); + let name = resolve_active_profile_name(&cfg, None, None); + assert_eq!(name, "default"); + } + + #[test] + fn config_active_profile_returns_resolved_profile() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "sandbox".to_string(), + ProfileConfig { + url: Some("https://sandbox.example".into()), + ..ProfileConfig::default() + }, + ); + let cfg = Config { + global: GlobalConfig { + default_profile: Some("sandbox".into()), + profiles, + ..GlobalConfig::default() + }, + project: ProjectConfig::default(), + active_profile_name: "sandbox".into(), + }; + assert_eq!( + cfg.active_profile().url.as_deref(), + Some("https://sandbox.example") + ); + } + + #[test] + fn config_active_profile_unknown_profile_returns_error() { + let cfg = Config { + global: GlobalConfig::default(), + project: ProjectConfig::default(), + active_profile_name: "ghost".into(), + }; + assert!(cfg.active_profile_or_err().is_err()); + } + + #[test] + fn migrate_legacy_instance_into_default_profile() { + let global = GlobalConfig { + instance: InstanceConfig { + url: Some("https://legacy.example".into()), + cloud_id: Some("legacy-1".into()), + org_id: Some("org-1".into()), + auth_method: Some("api_token".into()), + oauth_scopes: None, + }, + fields: FieldsConfig { + team_field_id: Some("customfield_99".into()), + story_points_field_id: Some("customfield_42".into()), + }, + ..GlobalConfig::default() + }; + + let migrated = migrate_legacy_global(global); + + assert_eq!(migrated.default_profile.as_deref(), Some("default")); + assert_eq!(migrated.profiles.len(), 1); + let p = &migrated.profiles["default"]; + assert_eq!(p.url.as_deref(), Some("https://legacy.example")); + assert_eq!(p.cloud_id.as_deref(), Some("legacy-1")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_99")); + assert_eq!(p.story_points_field_id.as_deref(), Some("customfield_42")); + // Legacy fields are intentionally preserved during the transition so + // callers that still read them keep working until Tasks 7/8 migrate. + assert_eq!( + migrated.instance.url.as_deref(), + Some("https://legacy.example"), + "[instance] preserved during transition" + ); + assert_eq!( + migrated.fields.team_field_id.as_deref(), + Some("customfield_99"), + "[fields] preserved during transition" + ); + } + + #[test] + fn migrate_legacy_is_idempotent_when_already_new_shape() { + let mut profiles = std::collections::BTreeMap::new(); + profiles.insert( + "custom".to_string(), + ProfileConfig { + url: Some("https://x.example".into()), + ..ProfileConfig::default() + }, + ); + let global = GlobalConfig { + default_profile: Some("custom".into()), + profiles, + ..GlobalConfig::default() + }; + let migrated = migrate_legacy_global(global.clone()); + assert_eq!(migrated.default_profile.as_deref(), Some("custom")); + assert_eq!(migrated.profiles.len(), 1); + assert_eq!( + migrated.profiles["custom"].url.as_deref(), + Some("https://x.example") + ); + } + + #[test] + fn migrate_legacy_with_no_data_yields_empty_new_shape() { + let global = GlobalConfig::default(); + let migrated = migrate_legacy_global(global); + assert!(migrated.profiles.is_empty()); + assert!(migrated.default_profile.is_none()); + } + + #[test] + fn migrate_legacy_with_only_org_id_set_creates_profile() { + let global = GlobalConfig { + instance: InstanceConfig { + org_id: Some("org-only".into()), + ..InstanceConfig::default() + }, + ..GlobalConfig::default() + }; + let migrated = migrate_legacy_global(global); + assert_eq!(migrated.default_profile.as_deref(), Some("default")); + assert_eq!( + migrated.profiles["default"].org_id.as_deref(), + Some("org-only") + ); + } + + #[test] + fn migrate_legacy_with_only_oauth_scopes_set_creates_profile() { + let global = GlobalConfig { + instance: InstanceConfig { + oauth_scopes: Some("read:jira-work offline_access".into()), + ..InstanceConfig::default() + }, + ..GlobalConfig::default() + }; + let migrated = migrate_legacy_global(global); + assert_eq!(migrated.default_profile.as_deref(), Some("default")); + assert_eq!( + migrated.profiles["default"].oauth_scopes.as_deref(), + Some("read:jira-work offline_access") + ); + } + + #[test] + fn config_load_precedence_flag_overrides_env_overrides_field() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let cfg_dir = dir.path().join("jr"); + std::fs::create_dir_all(&cfg_dir).unwrap(); + let config_path = cfg_dir.join("config.toml"); + std::fs::write( + &config_path, + r#" + default_profile = "from-config" + [profiles.from-config] + url = "https://x" + [profiles.from-env] + url = "https://y" + [profiles.from-flag] + url = "https://z" + "#, + ) + .unwrap(); + + // SAFETY: ENV_MUTEX held across env mutations. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + std::env::set_var("JR_PROFILE", "from-env"); + } + // CLI flag wins over env var. + let cfg = Config::load_with(Some("from-flag")).unwrap(); + assert_eq!(cfg.active_profile_name, "from-flag"); + + // Without the CLI flag, JR_PROFILE wins over the config field. + let cfg = Config::load_with(None).unwrap(); + assert_eq!(cfg.active_profile_name, "from-env"); + + unsafe { + std::env::remove_var("JR_PROFILE"); + } + // With neither flag nor env, the config field wins. + let cfg = Config::load_with(None).unwrap(); + assert_eq!(cfg.active_profile_name, "from-config"); + + unsafe { + std::env::remove_var("XDG_CONFIG_HOME"); + } + } + + #[test] + fn config_load_errors_when_jr_profile_targets_unknown_profile() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let cfg_dir = dir.path().join("jr"); + std::fs::create_dir_all(&cfg_dir).unwrap(); + std::fs::write( + cfg_dir.join("config.toml"), + r#" + default_profile = "default" + [profiles.default] + url = "https://x" + "#, + ) + .unwrap(); + + // SAFETY: ENV_MUTEX held. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + std::env::set_var("JR_PROFILE", "ghost"); + } + let result = Config::load(); + unsafe { + std::env::remove_var("JR_PROFILE"); + std::env::remove_var("XDG_CONFIG_HOME"); + } + let err = result.expect_err("ghost profile should fail Config::load"); + let je = err.downcast_ref::().expect("should be JrError"); + assert!( + matches!(je, JrError::UserError(_)), + "expected UserError, got {je:?}" + ); + let msg = format!("{err:#}"); + assert!(msg.contains("unknown profile"), "got: {msg}"); + assert!(msg.contains("ghost"), "got: {msg}"); + assert!(msg.contains("default"), "got: {msg}"); + } + + #[test] + fn config_load_rejects_invalid_profile_name_from_env() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let cfg_dir = dir.path().join("jr"); + std::fs::create_dir_all(&cfg_dir).unwrap(); + // SAFETY: ENV_MUTEX held. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + std::env::set_var("JR_PROFILE", "evil:profile"); + } + let result = Config::load(); + unsafe { + std::env::remove_var("JR_PROFILE"); + std::env::remove_var("XDG_CONFIG_HOME"); + } + assert!( + result.is_err(), + "JR_PROFILE with invalid char should reject" + ); + } + + #[test] + fn config_load_lenient_succeeds_when_active_profile_unknown() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let cfg_dir = dir.path().join("jr"); + std::fs::create_dir_all(&cfg_dir).unwrap(); + std::fs::write( + cfg_dir.join("config.toml"), + r#" + default_profile = "default" + [profiles.default] + url = "https://x" + "#, + ) + .unwrap(); + + // SAFETY: ENV_MUTEX held. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + std::env::set_var("JR_PROFILE", "ghost"); + } + let strict = Config::load(); + let lenient = Config::load_lenient(); + unsafe { + std::env::remove_var("JR_PROFILE"); + std::env::remove_var("XDG_CONFIG_HOME"); + } + + assert!(strict.is_err(), "strict load should reject unknown profile"); + assert!( + lenient.is_ok(), + "lenient load should accept unknown profile" + ); + let cfg = lenient.unwrap(); + assert_eq!(cfg.active_profile_name, "ghost"); + assert_eq!(cfg.global.profiles.len(), 1, "profile map untouched"); + } + + #[test] + fn config_load_rejects_invalid_profile_key_in_config() { + let _guard = ENV_MUTEX.lock().unwrap(); + let dir = TempDir::new().unwrap(); + let cfg_dir = dir.path().join("jr"); + std::fs::create_dir_all(&cfg_dir).unwrap(); + std::fs::write( + cfg_dir.join("config.toml"), + r#" + default_profile = "default" + [profiles.default] + url = "https://x" + [profiles."bad:name"] + url = "https://y" + "#, + ) + .unwrap(); + + // SAFETY: ENV_MUTEX held. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", dir.path()); + } + let result = Config::load(); + unsafe { + std::env::remove_var("XDG_CONFIG_HOME"); + } + + let err = result.expect_err("invalid profile key should reject"); + let msg = format!("{err:#}"); + assert!(msg.contains("invalid profile name"), "got: {msg}"); + assert!(msg.contains("bad:name"), "got: {msg}"); + } + + #[test] + fn global_config_parses_legacy_shape_into_legacy_fields() { + let toml = r#" + [instance] + url = "https://legacy.atlassian.net" + auth_method = "api_token" + cloud_id = "legacy-1" + + [fields] + team_field_id = "customfield_99" + story_points_field_id = "customfield_42" + "#; + let cfg: GlobalConfig = toml::from_str(toml).unwrap(); + assert!(cfg.profiles.is_empty(), "no [profiles] in legacy shape"); + assert!( + cfg.default_profile.is_none(), + "no default_profile in legacy shape" + ); + assert_eq!( + cfg.instance.url.as_deref(), + Some("https://legacy.atlassian.net") + ); + assert_eq!(cfg.fields.team_field_id.as_deref(), Some("customfield_99")); + } } diff --git a/src/main.rs b/src/main.rs index e27092b8..7bbce11e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -52,6 +52,17 @@ async fn main() { } async fn run(cli: Cli) -> anyhow::Result<()> { + // Validate --profile here (not in main) so a bad name flows through + // the unified error-reporting block — `--output json` callers get + // a structured `{"error":..,"code":..}` payload instead of a plain + // stderr line. The validated value is threaded into + // `Config::load_with` rather than through an env-var seam, since + // `unsafe { std::env::set_var(...) }` is unsound under + // #[tokio::main] (tokio worker threads already exist). + if let Some(p) = cli.profile.as_deref() { + config::validate_profile_name(p)?; + } + // Handle completion before anything else (no config/auth needed) if let cli::Command::Completion { shell } = &cli.command { let mut cmd = Cli::command(); @@ -65,46 +76,81 @@ async fn run(cli: Cli) -> anyhow::Result<()> { cli::Command::Completion { .. } => unreachable!(), cli::Command::Init => cli::init::handle().await, cli::Command::Assets { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::assets::handle(command, &cli.output, &client).await } cli::Command::Auth { command } => match command { + // For each subcommand that takes its own `--profile` arg, we + // compose an "effective profile" by falling back to the + // global `--profile` (`cli.profile`) when the subcommand-level + // value is `None`. Without this, `jr --profile sandbox auth + // ` would silently drop the global flag because each + // handler reloads config internally and only sees the + // subcommand-level arg. cli::AuthCommand::Login { + profile, + url, oauth, email, token, client_id, client_secret, } => { - if oauth { - cli::auth::login_oauth(client_id, client_secret, cli.no_input).await - } else { - cli::auth::login_token(email, token, cli.no_input).await - } + let effective_profile = profile.or_else(|| cli.profile.clone()); + cli::auth::handle_login(cli::auth::LoginArgs { + profile: effective_profile, + url, + oauth, + email, + token, + client_id, + client_secret, + no_input: cli.no_input, + }) + .await + } + cli::AuthCommand::Status { profile } => { + let effective_profile = profile.or_else(|| cli.profile.clone()); + cli::auth::status(effective_profile.as_deref()).await } - cli::AuthCommand::Status => cli::auth::status().await, cli::AuthCommand::Refresh { + profile, oauth, email, token, client_id, client_secret, } => { - cli::auth::refresh_credentials( + let effective_profile = profile.or_else(|| cli.profile.clone()); + cli::auth::refresh_credentials(cli::auth::RefreshArgs { + profile: effective_profile.as_deref(), oauth, email, token, client_id, client_secret, - cli.no_input, - &cli.output, - ) + no_input: cli.no_input, + output: &cli.output, + }) .await } + cli::AuthCommand::Switch { name } => { + cli::auth::handle_switch(&name, cli.profile.as_deref()).await + } + cli::AuthCommand::List => { + cli::auth::handle_list(&cli.output, cli.profile.as_deref()).await + } + cli::AuthCommand::Logout { profile } => { + let effective_profile = profile.or_else(|| cli.profile.clone()); + cli::auth::handle_logout(effective_profile.as_deref()).await + } + cli::AuthCommand::Remove { name } => { + cli::auth::handle_remove(&name, cli.no_input, cli.profile.as_deref()).await + } }, cli::Command::Me => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; let user = client.get_myself().await?; output::print_output( @@ -123,7 +169,7 @@ async fn run(cli: Cli) -> anyhow::Result<()> { Ok(()) } cli::Command::Project { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::project::handle( command, @@ -135,7 +181,7 @@ async fn run(cli: Cli) -> anyhow::Result<()> { .await } cli::Command::Issue { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::issue::handle( *command, @@ -149,7 +195,7 @@ async fn run(cli: Cli) -> anyhow::Result<()> { Ok(()) } cli::Command::Board { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::board::handle( command, @@ -161,7 +207,7 @@ async fn run(cli: Cli) -> anyhow::Result<()> { .await } cli::Command::Sprint { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::sprint::handle( command, @@ -173,22 +219,22 @@ async fn run(cli: Cli) -> anyhow::Result<()> { .await } cli::Command::Worklog { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::worklog::handle(command, &client, &cli.output).await } cli::Command::Team { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::team::handle(command, &cli.output, &config, &client).await } cli::Command::User { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::user::handle(command, &cli.output, &client).await } cli::Command::Queue { command } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::queue::handle( command, @@ -205,7 +251,7 @@ async fn run(cli: Cli) -> anyhow::Result<()> { data, header, } => { - let config = config::Config::load()?; + let config = config::Config::load_with(cli.profile.as_deref())?; let client = api::client::JiraClient::from_config(&config, cli.verbose)?; cli::api::handle_api(path, method, data, header, &client).await } diff --git a/src/output.rs b/src/output.rs index b06d0078..af522045 100644 --- a/src/output.rs +++ b/src/output.rs @@ -46,6 +46,10 @@ pub fn print_success(msg: &str) { eprintln!("{}", msg.green()); } +pub fn print_warning(msg: &str) { + eprintln!("warning: {msg}"); +} + pub fn print_error(msg: &str) { eprintln!("{}: {}", "Error".red().bold(), msg); } diff --git a/tests/auth_profiles.rs b/tests/auth_profiles.rs new file mode 100644 index 00000000..6a7bfe7c --- /dev/null +++ b/tests/auth_profiles.rs @@ -0,0 +1,333 @@ +//! Integration tests for multi-profile auth workflows. + +use assert_cmd::prelude::*; +use std::process::Command; +use tempfile::TempDir; + +fn jr() -> Command { + let mut cmd = Command::cargo_bin("jr").unwrap(); + // Scrub every JR_* env var that Config::load merges via figment's + // Env::prefixed("JR_"). Without this, a developer with a direnv-set + // JR_INSTANCE_URL / JR_PROFILE / etc. would have those values flow + // into the test's "fresh install" config and either trigger legacy + // migration unexpectedly or make assertions about empty profiles + // fail. Pinning the list here so future JR_* additions don't + // silently re-introduce flakiness on dev machines. + cmd.env_remove("JR_PROFILE") + .env_remove("JR_DEFAULT_PROFILE") + .env_remove("JR_INSTANCE_URL") + .env_remove("JR_INSTANCE_AUTH_METHOD") + .env_remove("JR_INSTANCE_CLOUD_ID") + .env_remove("JR_INSTANCE_ORG_ID") + .env_remove("JR_INSTANCE_OAUTH_SCOPES") + .env_remove("JR_FIELDS_TEAM_FIELD_ID") + .env_remove("JR_FIELDS_STORY_POINTS_FIELD_ID") + .env_remove("JR_DEFAULTS_OUTPUT") + .env_remove("JR_BASE_URL") + .env_remove("JR_AUTH_HEADER") + .env_remove("JR_EMAIL") + .env_remove("JR_API_TOKEN") + .env_remove("JR_OAUTH_CLIENT_ID") + .env_remove("JR_OAUTH_CLIENT_SECRET"); + cmd +} + +fn fresh_config_dir() -> (TempDir, std::path::PathBuf) { + let dir = TempDir::new().unwrap(); + let cfg = dir.path().join("jr").join("config.toml"); + std::fs::create_dir_all(cfg.parent().unwrap()).unwrap(); + (dir, cfg) +} + +#[test] +fn auth_switch_unknown_profile_exits_64() { + let (dir, _path) = fresh_config_dir(); + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "switch", "ghost"]) + .assert() + .failure() + .code(64); +} + +#[test] +fn auth_list_shows_no_profiles_for_fresh_install() { + let (dir, _path) = fresh_config_dir(); + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "list", "--output", "json"]) + .assert() + .success() + .stdout(predicates::str::contains("[]")); +} + +/// Regression: `jr auth status` against a fresh install (no [profiles] +/// in config — or no config at all) must succeed with a "not configured" +/// message, not error with "unknown profile". Setup scripts and CI use +/// `auth status` as a first-run probe before deciding whether to call +/// `jr init` or `jr auth login`. +#[test] +fn auth_status_fresh_install_no_profiles_succeeds() { + let (dir, _path) = fresh_config_dir(); // no config.toml written + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "status"]) + .assert() + .success() + .stderr(predicates::str::contains("No profiles configured")); +} + +#[test] +fn auth_status_unknown_profile_exits_64() { + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://x.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "status", "--profile", "ghost"]) + .assert() + .failure() + .code(64) + .stderr(predicates::str::contains("unknown profile")); +} + +#[test] +fn auth_logout_unknown_profile_exits_64() { + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://x.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "logout", "--profile", "ghost"]) + .assert() + .failure() + .code(64) + .stderr(predicates::str::contains("unknown profile")); +} + +#[test] +fn auth_remove_active_profile_exits_64() { + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://x.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + + jr().env("XDG_CONFIG_HOME", dir.path()) + .args(["auth", "remove", "default", "--no-input"]) + .assert() + .failure() + .code(64) + .stderr(predicates::str::contains("cannot remove active")); +} + +#[test] +fn precedence_flag_overrides_env_overrides_config() { + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "from-config" +[profiles.from-config] +url = "https://from-config.example" +[profiles.from-env] +url = "https://from-env.example" +[profiles.from-flag] +url = "https://from-flag.example" +"#, + ) + .unwrap(); + + let out = jr() + .env("XDG_CONFIG_HOME", dir.path()) + .env("JR_PROFILE", "from-env") + .args(["--profile", "from-flag", "auth", "list", "--output", "json"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + let parsed: serde_json::Value = + serde_json::from_str(&stdout).unwrap_or_else(|_| panic!("parse: {stdout}")); + let active: Vec<&serde_json::Value> = parsed + .as_array() + .unwrap() + .iter() + .filter(|p| p["active"].as_bool() == Some(true)) + .collect(); + assert_eq!( + active.len(), + 1, + "expected exactly one active profile, got {}: {parsed:?}", + active.len() + ); + assert_eq!(active[0]["name"], "from-flag"); +} + +/// Regression (Copilot round-10): the global `--profile` flag was being +/// dropped by `auth status`, `auth login`, `auth refresh`, and `auth logout` +/// because each handler reloaded config internally and only saw the +/// subcommand-level `--profile`. main.rs now composes an effective profile +/// (`subcmd.profile.or(cli.profile)`) so the global flag propagates. +#[test] +fn global_profile_flag_targets_auth_status() { + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://default.example" +auth_method = "api_token" +[profiles.sandbox] +url = "https://sandbox.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + + // Global `--profile sandbox` without subcommand-level `--profile`. + // Status output must reflect sandbox, not default. + let out = jr() + .env("XDG_CONFIG_HOME", dir.path()) + .args(["--profile", "sandbox", "auth", "status"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + assert!( + combined.contains("sandbox") || combined.contains("https://sandbox.example"), + "global --profile flag should target sandbox; got: {combined}" + ); +} + +/// Regression: round-4's unified active-profile existence check at +/// `Config::load` time broke `jr auth login --profile newprof --url ...` +/// because the profile didn't exist yet. `handle_login` now uses +/// `Config::load_lenient` to skip that check, restoring the documented +/// "login creates profile if absent" behavior. +/// +/// Gated behind `JR_RUN_KEYRING_TESTS=1` because `login_token` writes the +/// shared API token to the keyring, which Linux CI may not have. +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn auth_login_creates_new_profile_with_url() { + if std::env::var("JR_RUN_KEYRING_TESTS").is_err() { + return; + } + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://existing.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + + // login --profile newprof should succeed and create the profile, + // even though newprof isn't in [profiles] yet at load time. + jr().env("XDG_CONFIG_HOME", dir.path()) + .env("JR_EMAIL", "user@example.com") + .env("JR_API_TOKEN", "token-value") + .args([ + "auth", + "login", + "--profile", + "newprof", + "--url", + "https://newprof.example", + "--no-input", + ]) + .assert() + .success(); + + // Verify the profile was added to config. + let saved = std::fs::read_to_string(&path).unwrap(); + assert!(saved.contains("[profiles.newprof]"), "saved: {saved}"); + assert!(saved.contains("https://newprof.example"), "saved: {saved}"); +} + +/// Regression: when `JR_PROFILE` points at a profile that doesn't exist +/// in `[profiles]` AND the user runs `jr auth login --profile ` +/// to create that other profile, login must still succeed. Round-5 found +/// that `login_token`/`login_oauth` reloaded config via strict +/// `Config::load()` after `handle_login`'s lenient load, which re-fired +/// the unknown-active-profile check on the unrelated `JR_PROFILE` value +/// and aborted the in-flight creation. Both internal reloads now use +/// `load_lenient` to match the orchestrator. +#[test] +#[ignore = "requires keyring backend; set JR_RUN_KEYRING_TESTS=1 to run"] +fn auth_login_with_jr_profile_pointing_to_unrelated_profile_still_creates_target() { + if std::env::var("JR_RUN_KEYRING_TESTS").is_err() { + return; + } + let (dir, path) = fresh_config_dir(); + std::fs::write( + &path, + r#" +default_profile = "default" +[profiles.default] +url = "https://existing.example" +auth_method = "api_token" +"#, + ) + .unwrap(); + + // JR_PROFILE points to a non-existent profile, but --profile points + // to a different new profile that login should create. Login must + // succeed despite the JR_PROFILE mismatch — login uses lenient load + // throughout so the strict active-profile existence check never + // fires for the in-flight creation. + Command::cargo_bin("jr") + .unwrap() + .env("XDG_CONFIG_HOME", dir.path()) + .env("JR_PROFILE", "ghost") + .env("JR_EMAIL", "user@example.com") + .env("JR_API_TOKEN", "token-value") + .args([ + "auth", + "login", + "--profile", + "fresh", + "--url", + "https://fresh.example", + "--no-input", + ]) + .assert() + .success(); + + let saved = std::fs::read_to_string(&path).unwrap(); + assert!(saved.contains("[profiles.fresh]"), "saved: {saved}"); +} diff --git a/tests/auth_refresh.rs b/tests/auth_refresh.rs index 49c72d52..496744e8 100644 --- a/tests/auth_refresh.rs +++ b/tests/auth_refresh.rs @@ -41,18 +41,21 @@ fn auth_refresh_oauth_help_is_accepted() { #[test] fn auth_refresh_no_input_fails_with_clear_message() { - // Pin: `jr auth refresh --no-input` without any credential flags must - // fail with a UserError (exit 64) that names the missing flag and env - // var. Enabled by #211 — login flows now resolve credentials via - // flag → env → prompt and error explicitly under --no-input. + // Pin: `jr auth refresh --no-input` against an unconfigured profile + // (empty config / no URL) must fail with a UserError (exit 64) that + // tells the user to use `jr auth login --url ...` instead. Refresh + // assumes the profile is already set up; rotating credentials on a + // URL-less profile would leave it unusable for actual API calls. // - // Replaces the pre-#211 test that asserted "fails without panic" when - // stdin was closed; the new contract is stronger (specific exit code + - // actionable message) so scripts/agents can recover. + // Round-16 of the multi-profile-auth review tightened this contract: + // pre-fix, refresh would clear credentials and then ask for an email + // (via --email / $JR_EMAIL), giving the user a misleading recovery + // path. Post-fix, the error names the actual root cause (no profile + // URL). // - // `JR_SERVICE_NAME` scopes the keychain service so `auth::clear_credentials()` - // inside the subprocess never touches the developer's real `jr-jira-cli` - // entries when `cargo test` runs locally. + // `JR_SERVICE_NAME` scopes the keychain service so `auth::clear_*` + // inside the subprocess never touches the developer's real + // `jr-jira-cli` entries when `cargo test` runs locally. let cache_dir = tempfile::tempdir().unwrap(); let config_dir = tempfile::tempdir().unwrap(); @@ -63,12 +66,19 @@ fn auth_refresh_no_input_fails_with_clear_message() { .env("JR_SERVICE_NAME", "jr-jira-cli-test") .env_remove("JR_EMAIL") .env_remove("JR_API_TOKEN") - // Config::load() merges JR_* via figment's Env::prefixed at - // src/config.rs:65 — JR_INSTANCE_AUTH_METHOD=oauth in the parent - // shell would flip refresh to the OAuth path and our email/JR_EMAIL - // stderr assertions would fail. Explicitly clear it to pin the - // api_token flow for this test. + // Config::load() merges JR_* via figment's Env::prefixed. Any + // JR_INSTANCE_* env vars from the parent shell would flow into + // the loaded config (e.g., JR_INSTANCE_URL would make the + // empty-config look configured; JR_INSTANCE_AUTH_METHOD=oauth + // would flip the flow). Clear the full set so the test pins + // the unconfigured-profile path on every machine. + .env_remove("JR_INSTANCE_URL") .env_remove("JR_INSTANCE_AUTH_METHOD") + .env_remove("JR_INSTANCE_CLOUD_ID") + .env_remove("JR_INSTANCE_ORG_ID") + .env_remove("JR_INSTANCE_OAUTH_SCOPES") + .env_remove("JR_PROFILE") + .env_remove("JR_DEFAULT_PROFILE") .args(["--no-input", "auth", "refresh"]) .output() .unwrap(); @@ -77,26 +87,20 @@ fn auth_refresh_no_input_fails_with_clear_message() { assert!( !output.status.success(), - "auth refresh --no-input without flags should fail, got stdout: {}", + "auth refresh --no-input without setup should fail, got stdout: {}", String::from_utf8_lossy(&output.stdout) ); assert_eq!( output.status.code(), Some(64), - "Missing credentials under --no-input should exit 64 (UserError), got: {:?}", + "Refresh against unconfigured profile should exit 64 (UserError), got: {:?}", output.status.code() ); assert!( - stderr.contains("--email") && stderr.contains("$JR_EMAIL"), - "Error should cite --email flag and $JR_EMAIL env var: {stderr}" - ); - // The clear-then-login ordering means credentials *are* cleared before - // the login failure bubbles up. The recovery hint tells users exactly - // how to get back to a working state — pinning it here so a future - // refactor can't silently drop the guidance. - assert!( - stderr.contains("Credentials were cleared"), - "Error should include recovery hint after cleared credentials: {stderr}" + stderr.contains("no URL configured") + && stderr.contains("jr auth login") + && stderr.contains("--url"), + "Error should explain the missing URL and point at jr auth login --url: {stderr}" ); assert!(!stderr.contains("panic"), "stderr leaked a panic: {stderr}"); } diff --git a/tests/cli_handler.rs b/tests/cli_handler.rs index 719b14c0..5b2fdee4 100644 --- a/tests/cli_handler.rs +++ b/tests/cli_handler.rs @@ -1439,8 +1439,10 @@ async fn test_create_table_mode_outputs_to_stderr() { } /// Helper: pre-populate team cache at the given XDG cache dir root. +/// Writes under `/jr/v1/default/teams.json` to match the +/// per-profile cache layout introduced in Task 6. fn write_test_team_cache(cache_home: &std::path::Path) { - let teams_dir = cache_home.join("jr"); + let teams_dir = cache_home.join("jr").join("v1").join("default"); std::fs::create_dir_all(&teams_dir).unwrap(); let cache = jr::cache::TeamCache { fetched_at: chrono::Utc::now(), diff --git a/tests/migration_legacy.rs b/tests/migration_legacy.rs new file mode 100644 index 00000000..3c074d3d --- /dev/null +++ b/tests/migration_legacy.rs @@ -0,0 +1,172 @@ +//! Legacy [instance] -> [profiles.default] migration tests. + +use std::fs; +use std::sync::Mutex; +use tempfile::TempDir; + +/// Both tests in this file mutate process-global env vars (XDG_CONFIG_HOME). +/// Cargo runs tests within a single integration-test binary in parallel by +/// default, so without serialization they race against each other and produce +/// flaky results. Cross-file races are out of scope here — each `tests/*.rs` +/// runs as its own binary with its own process. +static ENV_MUTEX: Mutex<()> = Mutex::new(()); + +/// Set of `JR_*` env vars that `Config::load` reads via figment's +/// `Env::prefixed("JR_")` or via direct `std::env::var` lookups +/// (`JR_PROFILE`, `JR_BASE_URL`). A developer with any of these +/// exported (commonly via direnv) would otherwise have their values +/// flow into the test's tempdir-scoped config and either trigger +/// legacy migration unexpectedly or shift the resolved active profile +/// — making the assertions about migration shape and idempotency +/// flaky. The guard scrubs them all on `set()` and restores prior +/// values on drop. +const JR_ENV_VARS_TO_SCRUB: &[&str] = &[ + "JR_PROFILE", + "JR_DEFAULT_PROFILE", + "JR_INSTANCE_URL", + "JR_INSTANCE_AUTH_METHOD", + "JR_INSTANCE_CLOUD_ID", + "JR_INSTANCE_ORG_ID", + "JR_INSTANCE_OAUTH_SCOPES", + "JR_FIELDS_TEAM_FIELD_ID", + "JR_FIELDS_STORY_POINTS_FIELD_ID", + "JR_DEFAULTS_OUTPUT", + "JR_BASE_URL", + "JR_AUTH_HEADER", +]; + +/// RAII helper: sets `XDG_CONFIG_HOME` to `value` and scrubs `JR_*` +/// env vars for the duration of the guard's lifetime, then restores +/// every prior value (or unsets if none) on drop. Drop runs even if +/// the test panics, so a `Config::load` that unwraps unsuccessfully +/// never leaks state into the next test in the same binary. Also +/// avoids unconditionally clobbering a pre-existing `XDG_CONFIG_HOME` +/// or `JR_*` from the parent environment that the developer relied on +/// outside the test runner. +struct XdgConfigGuard { + previous_xdg: Option, + previous_jr_vars: Vec<(&'static str, Option)>, +} + +impl XdgConfigGuard { + fn set(value: &std::path::Path) -> Self { + let previous_xdg = std::env::var_os("XDG_CONFIG_HOME"); + let previous_jr_vars: Vec<(&'static str, Option)> = + JR_ENV_VARS_TO_SCRUB + .iter() + .map(|&name| (name, std::env::var_os(name))) + .collect(); + // SAFETY: tests in this binary serialize env mutation via + // ENV_MUTEX; no concurrent access. + unsafe { + std::env::set_var("XDG_CONFIG_HOME", value); + for &name in JR_ENV_VARS_TO_SCRUB { + std::env::remove_var(name); + } + } + Self { + previous_xdg, + previous_jr_vars, + } + } +} + +impl Drop for XdgConfigGuard { + fn drop(&mut self) { + // SAFETY: same as set() — caller must hold ENV_MUTEX while the + // guard is alive; no concurrent access. + unsafe { + match self.previous_xdg.take() { + Some(prev) => std::env::set_var("XDG_CONFIG_HOME", prev), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + for (name, prev) in std::mem::take(&mut self.previous_jr_vars) { + match prev { + Some(v) => std::env::set_var(name, v), + None => std::env::remove_var(name), + } + } + } + } +} + +#[test] +fn legacy_instance_block_migrated_in_memory() { + let _env_lock = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner()); + let dir = TempDir::new().unwrap(); + let cfg_path = dir.path().join("jr").join("config.toml"); + fs::create_dir_all(cfg_path.parent().unwrap()).unwrap(); + fs::write( + &cfg_path, + r#" +[instance] +url = "https://legacy.atlassian.net" +auth_method = "api_token" +cloud_id = "legacy-1" +org_id = "org-1" + +[fields] +team_field_id = "customfield_99" +story_points_field_id = "customfield_42" + +[defaults] +output = "json" +"#, + ) + .unwrap(); + + let _xdg = XdgConfigGuard::set(dir.path()); + let config = jr::config::Config::load().unwrap(); + + assert_eq!(config.active_profile_name, "default"); + assert!(config.global.profiles.contains_key("default")); + let p = &config.global.profiles["default"]; + assert_eq!(p.url.as_deref(), Some("https://legacy.atlassian.net")); + assert_eq!(p.cloud_id.as_deref(), Some("legacy-1")); + assert_eq!(p.team_field_id.as_deref(), Some("customfield_99")); + assert_eq!(p.story_points_field_id.as_deref(), Some("customfield_42")); + + assert_eq!(config.global.defaults.output, "json"); + + let on_disk = fs::read_to_string(&cfg_path).unwrap(); + assert!(on_disk.contains("default_profile")); + assert!(on_disk.contains("[profiles.default]")); + assert!( + !on_disk.contains("[instance]"), + "[instance] should not be serialized" + ); + assert!( + !on_disk.contains("[fields]"), + "[fields] should not be serialized" + ); + // _xdg dropped here — restores prior XDG_CONFIG_HOME (or unsets). +} + +#[test] +fn migration_is_idempotent() { + let _env_lock = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner()); + let dir = TempDir::new().unwrap(); + let cfg_path = dir.path().join("jr").join("config.toml"); + fs::create_dir_all(cfg_path.parent().unwrap()).unwrap(); + fs::write( + &cfg_path, + r#" +[instance] +url = "https://x" +auth_method = "api_token" +"#, + ) + .unwrap(); + + let _xdg = XdgConfigGuard::set(dir.path()); + let _ = jr::config::Config::load().unwrap(); + let after_first = fs::read_to_string(&cfg_path).unwrap(); + let _ = jr::config::Config::load().unwrap(); + let after_second = fs::read_to_string(&cfg_path).unwrap(); + + assert_eq!( + after_first, after_second, + "second load should not modify file" + ); + // _xdg dropped here — restores prior XDG_CONFIG_HOME (or unsets). +}