diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b480f1f..990197e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,6 +103,33 @@ jobs: - name: Test run: cargo test --workspace --locked + # PR screenshots of the account dialog's refused-save messages (#320), from GPUI's Linux offscreen renderer: the + # only platform with one. The runner has no GPU, so Mesa's software Vulkan driver draws them. No window opens, + # and the fixture uses an in-memory key store, a temporary settings folder and no providers. + - name: Render account dialog captures + if: runner.os == 'Linux' + env: + CODEXBAR_CAPTURE_DIR: ${{ github.workspace }}/artifacts/captures + run: | + sudo apt-get install -y --no-install-recommends mesa-vulkan-drivers libvulkan1 + cargo test -p codexbar-app --bin codexbar --locked -- --ignored --exact captures::captures_refused_key_320 --nocapture + for name in refused-key-not-restored refused-key-not-restored-dark refused-new-key-not-removed refused-new-key-not-removed-dark; do + test -s "$CODEXBAR_CAPTURE_DIR/$name.png" || { echo "::error::$name.png was not rendered"; exit 1; } + done + + - name: Upload account dialog captures + if: runner.os == 'Linux' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: account-dialog-captures + path: | + artifacts/captures/refused-key-not-restored.png + artifacts/captures/refused-key-not-restored-dark.png + artifacts/captures/refused-new-key-not-removed.png + artifacts/captures/refused-new-key-not-removed-dark.png + if-no-files-found: error + retention-days: 14 + rust-metrics: runs-on: ubuntu-latest timeout-minutes: 45 diff --git a/crates/codexbar-app/src/captures.rs b/crates/codexbar-app/src/captures.rs index bddb820..7d9203b 100644 --- a/crates/codexbar-app/src/captures.rs +++ b/crates/codexbar-app/src/captures.rs @@ -1042,3 +1042,158 @@ fn captures() { } let _ = std::fs::remove_dir_all(&settings); } + +/// A store that refuses to undo the account dialog's key write (#320): it won't save `sk-old` again and deletes +/// nothing. +struct RefusesUndo(MemoryCredentialStore); + +impl codexbar_store::credentials::CredentialStore for RefusesUndo { + fn read(&self, account_id: &str) -> Result, codexbar_store::credentials::CredentialError> { + self.0.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), codexbar_store::credentials::CredentialError> { + if secret == "sk-old" { + return Err(codexbar_store::credentials::CredentialError { code: 5 }); + } + self.0.write(account_id, secret) + } + + fn delete(&self, _: &str) -> Result<(), codexbar_store::credentials::CredentialError> { + Err(codexbar_store::credentials::CredentialError { code: 5 }) + } +} + +/// #320: the account dialog when a refused settings save can't be undone in the credential store, Light and Dark: +/// an edited account whose previous key couldn't be put back, then a new account whose key couldn't be removed. +/// The settings are refused by a blank name; the dashboard's provider factory builds nothing, so no request is made. +#[test] +#[ignore = "writes screenshots; run with CODEXBAR_CAPTURE_DIR set"] +fn captures_refused_key_320() { + use codexbar_store::credentials::CredentialStore as _; + let Some(out) = std::env::var_os("CODEXBAR_CAPTURE_DIR").map(PathBuf::from) else { + return; + }; + std::fs::create_dir_all(&out).unwrap(); + let settings = std::env::temp_dir().join(format!("codexbar-captures-320-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&settings); + std::fs::create_dir_all(&settings).unwrap(); + std::fs::write(settings.join("settings.json"), SETTINGS).unwrap(); + + let text_system = gpui_kit::platform::current_platform(true).text_system(); + let mut cx = HeadlessAppContext::with_platform(text_system, Arc::new(crate::assets::Assets), || { + gpui_kit::platform::current_headless_renderer() + }); + let store = MemoryCredentialStore::default(); + store.write("or-1", "sk-old").unwrap(); + store.write("ms-1", "sk-ms-capture").unwrap(); + cx.update(|cx| { + gpui_kit::init(cx); + cx.set_reduce_motion(true); + theme::init(cx); + SettingsHub::init_with(cx, &settings, Arc::new(RefusesUndo(store))); + crate::prefs_hub::PrefsHub::init_in_memory(cx); + zoom::init(cx); + crate::notifications::Notifications::init(cx, Arc::new(RecordingNotifier::default()), false); + crate::package::Updates::init(cx, false); + crate::package::StartupSetting::init(cx); + }); + let mut dashboard = None; + let handle = cx + .open_window(size(px(1440.), px(960.)), |window, cx| { + let source = DataSource::Live { + history: Arc::new(std::sync::Mutex::new(codexbar_store::HistoryStore::in_memory( + chrono::Duration::days(30), + ))), + providers: Arc::new(|_| Vec::new()), + }; + let view = cx.new(|cx| Dashboard::new(source, window, cx)); + dashboard = Some(view.clone()); + cx.new(|cx| Root::new(view, window, cx)) + }) + .unwrap(); + let handle: AnyWindowHandle = handle.into(); + let dashboard = dashboard.unwrap(); + cx.run_until_parked(); + cx.update(|cx| dashboard.update(cx, |dashboard, cx| dashboard.show_view(DashboardView::Settings, cx))); + cx.run_until_parked(); + let openrouter = crate::catalog::PROVIDERS + .iter() + .position(|info| info.id == codexbar_store::settings::names::OPENROUTER) + .unwrap(); + for entry in ["0-1".to_owned(), format!("0-1-{openrouter}")] { + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.within("settings-sidebar").click(entry, cx) + }) + .unwrap(); + cx.run_until_parked(); + } + + // Each case: open the dialog, clear the name, enter a key and press Save. + let cases = [ + ( + "edit-or-1", + "sk-new", + "refused-key-not-restored", + "couldn't be put back", + ), + ( + "add-OpenRouter", + "sk-abandoned", + "refused-new-key-not-removed", + "couldn't be removed", + ), + ]; + for (open, key, name, says) in cases { + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.click(open, cx) + }) + .unwrap(); + cx.run_until_parked(); + for press in ["backspace", "tab", "tab"] { + cx.update_window(handle, |_, window, cx| window.press(press, cx)) + .unwrap(); + cx.run_until_parked(); + } + cx.update_window(handle, |_, window, cx| window.input(key, cx)).unwrap(); + cx.run_until_parked(); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.click("account-save", cx) + }) + .unwrap(); + cx.run_until_parked(); + let error = cx + .update_window(handle, |_, window, cx| { + window.render_frame(cx); + window + .try_find("account-error") + .and_then(|found| found.label().map(str::to_owned)) + }) + .unwrap() + .unwrap_or_default(); + assert!( + error.starts_with("Every account needs a name.") && error.contains(says), + "{name}: {error}" + ); + // Light and Dark chosen explicitly, so neither frame depends on what System resolves to. + for (appearance, frame) in [ + (theme::Appearance::Light, name.to_owned()), + (theme::Appearance::Dark, format!("{name}-dark")), + ] { + cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, appearance)); + cx.run_until_parked(); + save(&mut cx, handle, &out, &frame); + } + cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, theme::Appearance::System)); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.press("escape", cx) + }) + .unwrap(); + cx.run_until_parked(); + } + let _ = std::fs::remove_dir_all(&settings); +} diff --git a/crates/codexbar-app/src/settings_view.rs b/crates/codexbar-app/src/settings_view.rs index 27b3fcb..f32b959 100644 --- a/crates/codexbar-app/src/settings_view.rs +++ b/crates/codexbar-app/src/settings_view.rs @@ -29,7 +29,7 @@ use crate::catalog::{self, PROVIDERS, SecretSpec}; use crate::notifications::{Notifications, NotifierStatus}; use crate::prefs_hub::PrefsHub; use crate::settings_hub::{SettingsHub, describe_source}; -use codexbar_store::credentials::SecretSource; +use codexbar_store::credentials::{CredentialError, CredentialStore, SecretSource}; /// Refresh choices required by #91, in minutes; 0 is Off. const REFRESH_CHOICES: [u64; 6] = [0, 1, 5, 15, 30, 60]; @@ -936,26 +936,19 @@ fn save_account(form: &AccountForm, existing: Option, window: &mu && !secret.is_empty() && previous.as_ref().is_some_and(crate::opencode_sign_in::is_managed); - // The secret is written first: if Credential Manager fails, the account isn't saved without its key. An OpenCode - // account on OAuth keeps its sign-in there instead, so no cookie is written over it. - // The sign-in a cookie replaces, put back if the settings can't be saved after it. - let mut replaced_sign_in = None; + // The secret is written first: if Credential Manager fails, the account isn't saved without its key. What it + // replaced is kept, to put back if the settings then can't be saved (#320). An OpenCode account on OAuth keeps + // its sign-in there instead, so no cookie is written over it. + let mut replaced = None; if info.secret.is_some() && !secret.is_empty() && !opencode_oauth { let store = SettingsHub::global(cx).credentials(); - let written = if replaces_opencode_sign_in { - // One write that leaves the old sign-in readable until the cookie is in place, with no renewal writing - // at the same time. - codexbar_providers::opencode_console::exclusive(|| { - replaced_sign_in = codexbar_store::credentials::read_long(store.as_ref(), &record.id)?; - codexbar_store::credentials::write_long(store.as_ref(), &record.id, &secret) - }) - } else { - store.write(&record.id, &secret) - }; - if let Err(err) = written { - *form.error.borrow_mut() = Some(err.to_string().into()); - window.refresh(); - return false; + match ReplacedSecret::write(store.as_ref(), &record.id, &secret, replaces_opencode_sign_in) { + Ok(written) => replaced = Some(written), + Err(err) => { + *form.error.borrow_mut() = Some(err.to_string().into()); + window.refresh(); + return false; + } } } use crate::managed::Managed; @@ -1019,20 +1012,98 @@ fn save_account(form: &AccountForm, existing: Option, window: &mu true } Err(err) => { - // The account is still on OAuth, so it gets its sign-in back. - if let Some(previous) = replaced_sign_in { - let store = SettingsHub::global(cx).credentials(); - let _ = codexbar_providers::opencode_console::exclusive(|| { - codexbar_store::credentials::write_long(store.as_ref(), &record_id, &previous) - }); - } - *form.error.borrow_mut() = Some(err.to_string().into()); + let store = SettingsHub::global(cx).credentials(); + let message = ReplacedSecret::undo(replaced, store.as_ref(), &record_id, &secret, &err); + *form.error.borrow_mut() = Some(message.into()); window.refresh(); false } } } +/// What the account dialog's key write replaced under the account's id, so a settings save that is then refused can +/// undo it (#320). +struct ReplacedSecret { + /// The secret saved before, or `None` when the id held none. + before: Option, + /// Written with `write_long`: an OpenCode sign-in that may span several credential parts, with no renewal + /// writing at the same time. + long: bool, +} + +impl ReplacedSecret { + /// Saves `secret` under `id` after reading what it replaces. Nothing is written if that can't be read, since it + /// couldn't be put back. + fn write(store: &dyn CredentialStore, id: &str, secret: &str, long: bool) -> Result { + use codexbar_store::credentials::{read_long, write_long}; + let write = || { + let before = if long { read_long(store, id)? } else { store.read(id)? }; + if long { + write_long(store, id, secret) + } else { + store.write(id, secret) + }?; + Ok(Self { before, long }) + }; + if long { + codexbar_providers::opencode_console::exclusive(write) + } else { + write() + } + } + + /// Undoes the write after the settings save failed with `refused`, and says why the save failed: the account + /// keeps the secret it had, and an id that held none is left empty. A secret another writer saved under the id + /// since then is left alone. A failure to undo it is added to the message, never hidden. + fn undo( + replaced: Option, + store: &dyn CredentialStore, + id: &str, + written: &str, + refused: &dyn std::fmt::Display, + ) -> String { + let Some(replaced) = replaced else { + return refused.to_string(); + }; + let had_secret = replaced.before.is_some(); + match replaced.restore(store, id, written) { + Ok(()) => refused.to_string(), + Err(err) if had_secret => { + format!("{refused} The new key is still saved, and the previous one couldn't be put back: {err}") + } + Err(err) => format!( + "{refused} The key is still saved in {} and couldn't be removed: {err}", + codexbar_store::credentials::STORE_SHORT_NAME + ), + } + } + + fn restore(self, store: &dyn CredentialStore, id: &str, written: &str) -> Result<(), CredentialError> { + use codexbar_store::credentials::{delete_long, read_long, write_long}; + let restore = || { + let now = if self.long { + read_long(store, id)? + } else { + store.read(id)? + }; + if now.as_deref() != Some(written) { + return Ok(()); + } + match (&self.before, self.long) { + (Some(before), true) => write_long(store, id, before), + (Some(before), false) => store.write(id, before), + (None, true) => delete_long(store, id), + (None, false) => store.delete(id), + } + }; + if self.long { + codexbar_providers::opencode_console::exclusive(restore) + } else { + restore() + } + } +} + fn confirm_remove(record: AccountRecord, window: &mut Window, cx: &mut App) { let label = record.label.clone(); // Only accounts that hold a pasted secret have anything in Credential Manager to delete. An account that is its own diff --git a/crates/codexbar-app/src/ui_tests.rs b/crates/codexbar-app/src/ui_tests.rs index 8a5df5b..f1995b7 100644 --- a/crates/codexbar-app/src/ui_tests.rs +++ b/crates/codexbar-app/src/ui_tests.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; -use codexbar_store::credentials::MemoryCredentialStore; +use codexbar_store::credentials::{CredentialError, CredentialStore, MemoryCredentialStore}; use gpui_kit::component::Root; use gpui_kit::test::TestWindowExt as _; use gpui_kit::{ @@ -1230,7 +1230,7 @@ fn open_live_with( cx: &mut TestAppContext, settings: &TempSettings, providers: Vec>, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { let providers = providers .into_iter() @@ -1252,7 +1252,7 @@ fn open_live_dyn( cx: &mut TestAppContext, settings: &TempSettings, providers: Vec>, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { let factory: crate::dashboard::ProviderFactory = Arc::new(move |_| providers.clone()); open_live_factory(cx, settings, factory, store) @@ -1263,7 +1263,7 @@ fn open_live_factory( cx: &mut TestAppContext, settings: &TempSettings, factory: crate::dashboard::ProviderFactory, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { use codexbar_store::HistoryStore; cx.update(|cx| { @@ -2767,7 +2767,7 @@ fn smart_order_re_ranks_when_held_alerts_change(cx: &mut TestAppContext) { fn open_settings_page( cx: &mut TestAppContext, settings: &TempSettings, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, page: usize, ) -> AnyWindowHandle { let dashboard = open_live_with(cx, settings, vec![], store); @@ -3466,6 +3466,313 @@ fn secure_store_failures_are_shown_and_nothing_is_saved_without_its_key(cx: &mut ); } +// A key saved before a settings save that is then refused is put back (#320). + +/// A store that saves the first `writes` secrets, then refuses every write and delete, so putting a key back fails. +struct FailsAfter { + inner: MemoryCredentialStore, + writes: std::sync::atomic::AtomicUsize, +} + +impl FailsAfter { + fn new(writes: usize) -> Self { + Self { + inner: MemoryCredentialStore::default(), + writes: std::sync::atomic::AtomicUsize::new(writes), + } + } +} + +impl CredentialStore for FailsAfter { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + use std::sync::atomic::Ordering; + self.writes + .try_update(Ordering::SeqCst, Ordering::SeqCst, |left| left.checked_sub(1)) + .map_err(|_| CredentialError { code: 5 })?; + self.inner.write(account_id, secret) + } + + fn delete(&self, _: &str) -> Result<(), CredentialError> { + Err(CredentialError { code: 5 }) + } +} + +/// A store that records every id written, so a test can see what a refused save left behind. Clones share it. +#[derive(Clone, Default)] +struct RecordingStore { + inner: Arc, + written: Arc>>, +} + +impl CredentialStore for RecordingStore { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + self.written.lock().unwrap().push(account_id.to_owned()); + self.inner.write(account_id, secret) + } + + fn delete(&self, account_id: &str) -> Result<(), CredentialError> { + self.inner.delete(account_id) + } +} + +/// Opens an account's dialog, clears its name or types a new one, enters `key` and presses Save. +fn edit_openrouter(cx: &mut TestAppContext, handle: AnyWindowHandle, id: &str, name: Option<&str>, key: &str) { + click(cx, handle, format!("edit-{id}")); + match name { + Some(name) => type_text(cx, handle, name), + None => press(cx, handle, "backspace"), + } + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, key); + click(cx, handle, "account-save"); +} + +/// Holds the shared settings write lock, as a second CodexBar saving settings does, until dropped. +fn hold_settings_lock(settings: &TempSettings) -> std::fs::File { + let mut options = std::fs::OpenOptions::new(); + options.read(true).write(true).create(true).truncate(false); + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt as _; + options.share_mode(0); + } + let file = options.open(settings.0.join("settings.write.lock")).unwrap(); + #[cfg(not(windows))] + file.try_lock().unwrap(); + file +} + +#[gpui_kit::test] +fn a_refused_edit_keeps_the_accounts_saved_key(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-key", "{}"); + let handle = open_settings_page(cx, &settings, MemoryCredentialStore::default(), 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + let before = std::fs::read(settings.0.join("settings.json")).unwrap(); + + // Without a new key there is nothing to put back. + edit_openrouter(cx, handle, &id, None, ""); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + + // A blank name is refused after the key was written: the old key is put back. + edit_openrouter(cx, handle, &id, None, "sk-new"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert!(exists(cx, handle, "account-save"), "the dialog stays open"); + assert_eq!(std::fs::read(settings.0.join("settings.json")).unwrap(), before); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old"), "cancelling keeps it"); + + // Another CodexBar holding the settings lock refuses the save too. + let held = hold_settings_lock(&settings); + edit_openrouter(cx, handle, &id, Some("Personal"), "sk-new"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + assert_eq!(saved_settings(&settings).accounts()[0].label, "Work"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + drop(held); + + // Once the save goes through, the new key is the one saved. + edit_openrouter(cx, handle, &id, Some("Personal"), "sk-new"); + assert_eq!(saved_settings(&settings).accounts()[0].label, "Personal"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-new")); +} + +#[gpui_kit::test] +fn a_refused_new_account_leaves_no_key_behind(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-new", "{}"); + let store = RecordingStore::default(); + let handle = open_settings_page(cx, &settings, store.clone(), 1); + click(cx, handle, "add-OpenRouter"); + press(cx, handle, "backspace"); + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, "sk-abandoned"); + click(cx, handle, "account-save"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + press(cx, handle, "escape"); + assert!(saved_settings(&settings).accounts().is_empty()); + let written = store.written.lock().unwrap().clone(); + assert!(!written.is_empty(), "the key was written before the refusal"); + for id in written { + assert_eq!(store.inner.read(&id).unwrap(), None, "nothing is left under {id}"); + } +} + +#[gpui_kit::test] +fn a_key_that_cannot_be_put_back_is_reported(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-restore-fails", "{}"); + // The first key and the edit's key are saved; putting the first one back is refused. + let handle = open_settings_page(cx, &settings, FailsAfter::new(2), 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + edit_openrouter(cx, handle, &id, None, "sk-new"); + let store_error = CredentialError { code: 5 }.to_string(); + assert_eq!( + label(cx, handle, "account-error"), + Some(format!( + "Every account needs a name. The new key is still saved, and the previous one couldn't be put back: \ + {store_error}" + )) + ); + assert_eq!( + secret_of(cx, &id).as_deref(), + Some("sk-new"), + "the message says what is saved" + ); +} + +#[gpui_kit::test] +fn a_new_accounts_key_that_cannot_be_removed_is_reported(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-remove-fails", "{}"); + let handle = open_settings_page(cx, &settings, FailsAfter::new(1), 1); + click(cx, handle, "add-OpenRouter"); + press(cx, handle, "backspace"); + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, "sk-abandoned"); + click(cx, handle, "account-save"); + let store_error = CredentialError { code: 5 }.to_string(); + assert_eq!( + label(cx, handle, "account-error"), + Some(format!( + "Every account needs a name. The key is still saved in {} and couldn't be removed: {store_error}", + codexbar_store::credentials::STORE_SHORT_NAME + )) + ); +} + +/// A store where another writer saves `theirs` for an account right after this one saves `mine` there. +struct ReplacedMeanwhile { + inner: MemoryCredentialStore, + mine: &'static str, + theirs: &'static str, +} + +impl CredentialStore for ReplacedMeanwhile { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + let secret = if secret == self.mine { self.theirs } else { secret }; + self.inner.write(account_id, secret) + } + + fn delete(&self, account_id: &str) -> Result<(), CredentialError> { + self.inner.delete(account_id) + } +} + +#[gpui_kit::test] +fn a_key_another_writer_saved_meanwhile_is_not_rolled_back(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-newer", "{}"); + let store = ReplacedMeanwhile { + inner: MemoryCredentialStore::default(), + mine: "sk-mine", + theirs: "sk-theirs", + }; + let handle = open_settings_page(cx, &settings, store, 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + edit_openrouter(cx, handle, &id, None, "sk-mine"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert_eq!( + secret_of(cx, &id).as_deref(), + Some("sk-theirs"), + "the newer key is left alone" + ); +} + +#[gpui_kit::test] +fn a_refused_cookie_leaves_a_signed_out_opencode_account_without_one(cx: &mut TestAppContext) { + let settings = TempSettings::new( + "opencode-refused-cookie-signed-out", + r#"{ "accountConfigurationVersion": 1, "accounts": [ + { "id": "oc-1", "providerId": "OpenCodeGo", "displayLabel": "Team", "enabled": true, + "authenticationMethod": "OAuth", "workspaceId": "wrk_01ABC" } ] }"#, + ); + let handle = open_opencode_accounts(cx, &settings, &FakeOpenCode::default()); + let held = hold_settings_lock(&settings); + click(cx, handle, "edit-oc-1"); + choose_browser_session(cx, handle); + press(cx, handle, "tab"); + type_text(cx, handle, "Fe26.2**new-cookie"); + click(cx, handle, "account-save"); + assert_eq!( + label_of(cx, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + drop(held); + let stored = + cx.update(|cx| codexbar_store::credentials::read_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1")); + assert_eq!(stored.unwrap(), None, "the signed-out account keeps no cookie"); +} + +#[gpui_kit::test] +fn a_refused_cookie_gives_an_opencode_account_its_sign_in_back(cx: &mut TestAppContext) { + let settings = TempSettings::new( + "opencode-refused-cookie", + r#"{ "accountConfigurationVersion": 1, "accounts": [ + { "id": "oc-1", "providerId": "OpenCodeGo", "displayLabel": "Team", "enabled": true, + "authenticationMethod": "OAuth", "workspaceId": "wrk_01ABC", "externalAccountId": "usr_1" } ] }"#, + ); + let handle = open_opencode_accounts(cx, &settings, &FakeOpenCode::default()); + let sign_in = opencode_fixture::credential("wrk_01ABC", "usr_1").to_secret(); + cx.update(|cx| { + codexbar_store::credentials::write_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1", &sign_in) + }) + .unwrap(); + let held = hold_settings_lock(&settings); + click(cx, handle, "edit-oc-1"); + choose_browser_session(cx, handle); + press(cx, handle, "tab"); + type_text(cx, handle, "Fe26.2**new-cookie"); + click(cx, handle, "account-save"); + assert_eq!( + label_of(cx, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + drop(held); + assert_eq!( + saved_settings(&settings).accounts()[0].method, + codexbar_store::settings::AuthMethod::OAuth + ); + let stored = + cx.update(|cx| codexbar_store::credentials::read_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1")); + assert_eq!( + stored.unwrap().as_deref(), + Some(sign_in.as_str()), + "the OAuth sign-in is back" + ); +} + /// A provider that returns fixed accounts, such as a provider's real parser output. struct FixedProvider(Vec); diff --git a/crates/codexbar-store/src/credentials.rs b/crates/codexbar-store/src/credentials.rs index cee2356..e1da132 100644 --- a/crates/codexbar-store/src/credentials.rs +++ b/crates/codexbar-store/src/credentials.rs @@ -448,6 +448,22 @@ mod tests { assert_eq!(read_long(&store, "acct").unwrap(), None); } + #[test] + fn refused_memory_store_operations_leave_saved_secrets_unchanged() { + let mut store = MemoryCredentialStore::default(); + store.write("existing", "before").unwrap(); + store.fail = true; + + assert_eq!(store.read("existing").unwrap_err().code, 5); + assert_eq!(store.write("existing", "after").unwrap_err().code, 5); + assert_eq!(store.write("new", "abandoned").unwrap_err().code, 5); + assert_eq!(store.delete("existing").unwrap_err().code, 5); + + store.fail = false; + assert_eq!(store.read("existing").unwrap().as_deref(), Some("before")); + assert_eq!(store.read("new").unwrap(), None); + } + #[test] fn memory_store_add_replace_read_delete() { let store = MemoryCredentialStore::default(); diff --git a/scripts/tests/ci-workflow.test.cjs b/scripts/tests/ci-workflow.test.cjs index 99ee23c..7f0df31 100644 --- a/scripts/tests/ci-workflow.test.cjs +++ b/scripts/tests/ci-workflow.test.cjs @@ -83,6 +83,33 @@ test('every platform checks the production binary without dev-dependency feature assert.ok(steps.indexOf(step) < names.indexOf('Test')); }); +// The account dialog's screenshots (#320) come from the Linux rust job, after its tests, and must exist: a capture that +// renders nothing fails the job instead of uploading an empty artifact. +test('the Linux rust job renders and uploads the account dialog captures', () => { + const steps = workflow.jobs.rust.steps; + const names = steps.map(step => step.name); + const render = steps.find(step => step.name === 'Render account dialog captures'); + const upload = steps.find(step => step.name === 'Upload account dialog captures'); + assert.ok(render && upload); + assert.ok(names.indexOf('Test') < steps.indexOf(render) && steps.indexOf(render) < steps.indexOf(upload)); + for (const step of [render, upload]) { + assert.equal(step.if, "runner.os == 'Linux'"); + assert.equal(step['continue-on-error'], undefined); + } + assert.equal(render.env.CODEXBAR_CAPTURE_DIR, '${{ github.workspace }}/artifacts/captures'); + assert.match(render.run, + /^cargo test -p codexbar-app --bin codexbar --locked -- --ignored --exact captures::captures_refused_key_320 --nocapture$/m); + const frames = ['refused-key-not-restored', 'refused-key-not-restored-dark', 'refused-new-key-not-removed', + 'refused-new-key-not-removed-dark']; + for (const frame of frames) { + assert.ok(render.run.includes(frame), frame); + } + assert.match(render.run, /test -s "\$CODEXBAR_CAPTURE_DIR\/\$name\.png" \|\| \{ .*exit 1; \}/); + assert.match(upload.uses, /^actions\/upload-artifact@/); + assert.equal(upload.with['if-no-files-found'], 'error'); + assert.deepEqual(upload.with.path.trim().split('\n'), frames.map(frame => `artifacts/captures/${frame}.png`)); +}); + // Every action is pinned to a reviewed commit, with the release it came from in a trailing comment (#301). // A movable tag such as `@v4` could change what runs after review, so a new reference must also be a SHA. test('every action reference in CI and Package is a full commit SHA with its version', () => {