From 60453ca2cbcdac4243581a862d36f565f0a6767e Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Mon, 6 Jul 2026 20:46:30 -0400 Subject: [PATCH] fix(auth): honor config-defaulted --aspect-* flags, user default over repo, and migrate legacy credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes to the auth feature surfaced in review: - `_want` (deployment_flags.axl) gated capability injection on `is_explicit`, which is CLI-only, so a `config.axl` default like `aspect_remote_cache = True` was ignored in favor of the `--aspect-remote` umbrella. Honor a resolved-True granular flag regardless of source; a CLI `--aspect-remote-*=false` still opts out via `is_explicit`. (A config default of False still can't opt out of the umbrella — it's indistinguishable from the built-in default without config- explicit tracking; documented on `_want`.) - Default reconciliation across config layers only cleared the built-in seed's default, so a user `config.json` marking a different deployment default left both the repo and user entries default and `select_deployment` picked the earlier repo one. Replace `reconcile_seed_default` with `overlay_deployments`, which clears prior defaults whenever an overlaid layer introduces one, so a higher-precedence (user) layer's default wins. - The keyring backend never read the legacy `~/.aspect/credentials.json`, so users who logged in on a pre-keyring release appeared logged out after upgrade. When the keyring is empty, migrate the legacy file in (best-effort write- through to the keyring; the file's credentials are returned regardless). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../aspect/private/lib/deployment_flags.axl | 22 +++-- .../private/lib/deployment_flags_test.axl | 12 +++ crates/axl-runtime/src/engine/aspect/auth.rs | 93 +++++++++++-------- .../src/engine/aspect/credential_store.rs | 54 ++++++++++- 4 files changed, 134 insertions(+), 47 deletions(-) diff --git a/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags.axl b/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags.axl index 5c19491a1..3d52ecb78 100644 --- a/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags.axl +++ b/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags.axl @@ -78,15 +78,23 @@ def deployment_flag_args() -> dict: } def _want(ctx, name: str, umbrella: bool) -> (bool, bool): - """Resolve capability arg `name` to `(on, explicit)`: an explicitly-set - granular flag wins (and is explicit); otherwise it follows the - `--aspect-remote` umbrella (not explicit). - - The distinction drives error handling: an explicitly-requested capability the + """Resolve capability arg `name` to `(on, requested)`: a directly-set granular + flag wins over the `--aspect-remote` umbrella; otherwise the flag follows it. + + A flag is "directly set" when it was given on the CLI (`is_explicit`) or turned + on via a `config.axl` default. The granular flags default off, so a resolved + `True` covers the config-on case that `is_explicit` (CLI-only) misses; a CLI + `--aspect-remote-*=false` is caught by `is_explicit` so it still opts that + capability out of the umbrella. (A config default of `False` reads the same as + the built-in default, so config cannot opt a capability out of the umbrella — + only the CLI can.) + + `requested` drives error handling: a directly-requested capability the deployment can't serve is an error, whereas the umbrella silently skips a capability the deployment doesn't advertise ("use whatever it offers").""" - if ctx.args.is_explicit(name): - return (getattr(ctx.args, name), True) + value = getattr(ctx.args, name) + if ctx.args.is_explicit(name) or value: + return (value, True) return (umbrella, False) def deployment_endpoint_flags(ctx) -> DeploymentFlags: diff --git a/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags_test.axl b/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags_test.axl index c1bf01838..7f77cc992 100644 --- a/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags_test.axl +++ b/crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags_test.axl @@ -103,6 +103,17 @@ def _test_umbrella_reports_injected_capabilities(ctx): ("bes backend", "grpcs://bes.gcp"), ]) +def _test_config_default_granular_honored(ctx): + # A granular flag turned on via a config.axl default (value True, but NOT + # is_explicit — that's CLI-only) must be honored, not ignored in favor of the + # umbrella. Regression guard for the is_explicit-only gate. + got = deployment_endpoint_flags(_ctx( + _endpoints("gcp", cache = "remote.gcp", bes = "bes.gcp", exec = "remote.gcp"), + aspect_remote_cache = True, + )) + _eq("config-on cache → cache wired", got.base_flags, ["--remote_cache=grpcs://remote.gcp"]) + _eq("config-on cache → no bes (umbrella not set)", got.bes_backends, []) + def _test_join_and(ctx): _eq("empty", join_and([]), "") _eq("one", join_and(["remote cache"]), "remote cache") @@ -116,6 +127,7 @@ _UNIT_TESTS = [ _test_umbrella_skips_unadvertised_capability, _test_granular_cache_only, _test_explicit_false_overrides_umbrella, + _test_config_default_granular_honored, _test_umbrella_reports_injected_capabilities, _test_join_and, ] diff --git a/crates/axl-runtime/src/engine/aspect/auth.rs b/crates/axl-runtime/src/engine/aspect/auth.rs index 1955b4b7f..bf1219e98 100644 --- a/crates/axl-runtime/src/engine/aspect/auth.rs +++ b/crates/axl-runtime/src/engine/aspect/auth.rs @@ -160,41 +160,41 @@ fn load_config_file(path: &PathBuf) -> anyhow::Result> { /// `.aspect/config.json` (when `$ASPECT_WORKSPACE` names one), overlaid by the /// user's `~/.aspect/config.json`. Overlay is by `name` (a later entry with the /// same name replaces an earlier one), so a user can override the seed or a -/// repo-declared deployment. Exactly one entry stays `default` — [`select_deployment`] -/// re-derives the default rather than trusting the flag on every entry. +/// repo-declared deployment. Exactly one entry stays `default`, with a +/// higher-precedence layer's default winning — see [`overlay_deployments`]. +/// [`select_deployment`] then re-derives the default rather than trusting the +/// flag on every entry. fn load_deployments() -> anyhow::Result> { let mut merged: Vec = vec![default_deployment()]; - let overlay = |merged: &mut Vec, entries: Vec| { - for entry in entries { - if let Some(existing) = merged.iter_mut().find(|d| d.name == entry.name) { - *existing = entry; - } else { - merged.push(entry); - } - } - }; if let Some(repo_cfg) = repo_config_path() { - overlay(&mut merged, load_config_file(&repo_cfg)?); + overlay_deployments(&mut merged, load_config_file(&repo_cfg)?); } - overlay(&mut merged, load_config_file(&config_path()?)?); - reconcile_seed_default(&mut merged); + overlay_deployments(&mut merged, load_config_file(&config_path()?)?); Ok(merged) } -/// The seed is re-created `default = true` on every load, but a configured -/// deployment marked default must win. So clear the seed's default whenever any -/// configured deployment claims it — leaving the seed as default only when -/// nothing configured does (which is also the "logged out of the default" state). -fn reconcile_seed_default(deployments: &mut [Deployment]) { - let configured_default = deployments - .iter() - .any(|d| d.default && d.name != DEFAULT_DEPLOYMENT_NAME); - if configured_default { - if let Some(seed) = deployments - .iter_mut() - .find(|d| d.name == DEFAULT_DEPLOYMENT_NAME) - { - seed.default = false; +/// Overlay a config layer's `entries` onto `merged` (by `name`), preserving the +/// "exactly one default" invariant with the incoming layer winning. +/// +/// Each layer is written with at most one default (see [`upsert_deployment`]). +/// The built-in seed starts `default = true`, and a repo layer may mark its own +/// default, so without care a later user layer marking a different deployment +/// would leave two defaults — and [`select_deployment`] would pick the earlier +/// (repo) one, so the user could never override a repo default. So when this +/// layer introduces a default, clear the flag on every prior entry first; a layer +/// that declares no default leaves the existing one untouched. +fn overlay_deployments(merged: &mut Vec, entries: Vec) { + let layer_has_default = entries.iter().any(|d| d.default); + if layer_has_default { + for d in merged.iter_mut() { + d.default = false; + } + } + for entry in entries { + if let Some(existing) = merged.iter_mut().find(|d| d.name == entry.name) { + *existing = entry; + } else { + merged.push(entry); } } } @@ -2913,16 +2913,33 @@ mod tests { } #[test] - fn reconcile_seed_default_yields_to_configured_default() { - // Seed loads default=true; a configured default clears it. - let mut merged = vec![default_deployment(), dep("acme", true)]; - reconcile_seed_default(&mut merged); - assert!(!merged[0].default, "seed yields to configured default"); - assert!(merged[1].default); - - // No configured default → the seed stays default (the fallback state). - let mut none_configured = vec![default_deployment(), dep("acme", false)]; - reconcile_seed_default(&mut none_configured); + fn overlay_deployments_lets_a_layer_default_win() { + // Seed starts default=true; a layer that marks a configured default + // clears the seed's. + let mut merged = vec![default_deployment()]; + overlay_deployments(&mut merged, vec![dep("acme", true)]); + assert!(!merged[0].default, "seed yields to a configured default"); + assert!(merged.iter().find(|d| d.name == "acme").unwrap().default); + + // A later layer marking a DIFFERENT deployment default must win over the + // earlier layer's default — the repo+user override case. Both must not + // remain default, else select_deployment picks the earlier (repo) one. + overlay_deployments(&mut merged, vec![dep("emca", true)]); + assert!( + !merged.iter().find(|d| d.name == "acme").unwrap().default, + "the earlier layer's default is cleared" + ); + assert!(merged.iter().find(|d| d.name == "emca").unwrap().default); + assert_eq!(merged.iter().filter(|d| d.default).count(), 1); + + // A layer that declares no default leaves the existing default untouched. + overlay_deployments(&mut merged, vec![dep("third", false)]); + assert!(merged.iter().find(|d| d.name == "emca").unwrap().default); + assert_eq!(merged.iter().filter(|d| d.default).count(), 1); + + // No configured default anywhere → the seed stays the default fallback. + let mut none_configured = vec![default_deployment()]; + overlay_deployments(&mut none_configured, vec![dep("acme", false)]); assert!(none_configured[0].default, "seed is the default fallback"); } diff --git a/crates/axl-runtime/src/engine/aspect/credential_store.rs b/crates/axl-runtime/src/engine/aspect/credential_store.rs index 36f34a3b0..44bd4744b 100644 --- a/crates/axl-runtime/src/engine/aspect/credential_store.rs +++ b/crates/axl-runtime/src/engine/aspect/credential_store.rs @@ -75,9 +75,21 @@ impl CredentialStore { /// read. The file backend additionally treats an *unparseable* file as empty /// (see `file_load_all`): a legacy/corrupt file means "re-run login", not a /// hard error on every command. - pub(crate) fn load_all(&self) -> anyhow::Result> { + /// + /// When the keyring is empty, credentials from the legacy `credentials.json` + /// (written by releases before the keyring backend) are migrated in, so + /// upgrading users stay logged in — see [`migrate_legacy_file`]. + pub(crate) fn load_all( + &self, + ) -> anyhow::Result> { match self { - Self::Keyring => keyring_load_all(), + Self::Keyring => { + let map = keyring_load_all()?; + if map.is_empty() { + return migrate_legacy_file(); + } + Ok(map) + } Self::File(path) => file_load_all(path), } } @@ -123,6 +135,21 @@ fn keyring_load_all() -> anyhow::Result> } } +/// One-time migration of credentials written by pre-keyring releases: read the +/// legacy file at [`default_file_path`] via [`file_load_all`] and, if it holds +/// any, write them into the keyring so subsequent reads are keyring-native. +/// Returns whatever the file held (empty when there is nothing to migrate). The +/// keyring write is best-effort — a write failure still returns the file's +/// credentials so the user isn't spuriously logged out; the migration simply +/// retries on the next load. +fn migrate_legacy_file() -> anyhow::Result> { + let map: HashMap = file_load_all(&default_file_path()?)?; + if !map.is_empty() { + let _ = keyring_save_all(&map); + } + Ok(map) +} + fn keyring_save_all(map: &HashMap) -> anyhow::Result<()> { let entry = keyring_entry()?; if map.is_empty() { @@ -218,6 +245,29 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn legacy_file_is_read_for_migration() { + // `migrate_legacy_file` (keyring-empty fallback) reads the legacy file via + // `file_load_all`; this covers that read — a populated legacy file yields + // its credentials so an upgrading user with a reachable-but-empty keyring + // stays logged in, an absent one yields empty. The keyring write-through is + // best-effort and deliberately not exercised here (no hermetic keyring). + let dir = std::env::temp_dir().join(format!("aspect-cred-migrate-{}", std::process::id())); + let path = dir.join("credentials.json"); + + let empty: HashMap = file_load_all(&path).unwrap(); + assert!(empty.is_empty(), "absent legacy file → nothing to migrate"); + + let mut legacy = HashMap::new(); + legacy.insert("default".to_string(), "tok-legacy".to_string()); + CredentialStore::File(path.clone()).save_all(&legacy).unwrap(); + + let read: HashMap = file_load_all(&path).unwrap(); + assert_eq!(read, legacy); + + let _ = fs::remove_dir_all(&dir); + } + #[test] fn env_override_forces_file_backend() { // SAFETY: single-threaded test; var removed before returning.