From 596deeb877b0ab26ac44a1790a3175fc85d548c9 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 10:20:01 +0100 Subject: [PATCH 1/7] Assistant: keep the ChatGPT sign-in's tokens in the OS keychain (#759) --- Cargo.lock | 1 + ROADMAP.md | 7 +- apps/rustgis-cli/src/main.rs | 25 +- apps/rustgis/Cargo.toml | 2 + apps/rustgis/src/main.rs | 6 +- crates/assistant/src/chatgpt.rs | 801 ++++++++++++++++++++++------ crates/assistant/src/tests.rs | 13 +- crates/ui-egui/src/assistantpane.rs | 98 +++- docs/architecture.md | 7 + docs/cli.md | 5 +- docs/user-guide/assistant.md | 23 +- 11 files changed, 791 insertions(+), 197 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 36362188c..f630c165f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4646,6 +4646,7 @@ dependencies = [ "objc2 0.6.5", "objc2-app-kit 0.3.2", "objc2-foundation 0.3.2", + "rustgis-assistant", "rustgis-engine", "rustgis-ui-egui", "serde_json", diff --git a/ROADMAP.md b/ROADMAP.md index db45e11cc..7e0ff53e3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -59,8 +59,8 @@ Correct, robust and recoverable, with the foundations other themes stand on. for a file that moved or was renamed. - Signed Windows builds (no SmartScreen warning), then in-place updates; settings an administrator can fix for a managed install (no AI Assistant, plugins or web services). -- Extend the OS keychain secret store to service connections (database passwords, S3 keys and - AI provider keys can already be remembered). +- Extend the OS keychain secret store to service connections (database passwords, S3 keys, + AI provider keys and the ChatGPT sign-in's tokens are already kept there). - Overlay tools beside Clip and Erase: intersect, union, identity and near _(open)_. - Spatial Join by distance, closest, centre in and largest overlap, with merge rules (sum, mean, first…) for one-to-one joins ([#851](https://github.com/opengeos/RustGIS/issues/851)). @@ -233,7 +233,8 @@ Open questions rather than plans; discuss before building. documentation built in. - The AI Assistant's skills, subagents and ACP clients; MCP over HTTP, an audited loopback relay and a file bridge, which let external AI clients drive the app. Database passwords, S3 - keys and AI provider keys optionally remembered in the desktop OS keychain. + keys and AI provider keys optionally remembered in the desktop OS keychain, where the ChatGPT + sign-in keeps its tokens. - Sandboxed WebAssembly plugins with a registry at plugins.rustgis.app; process plugins that run as programs (with Python environments RustGIS makes), in the background with progress and Cancel. diff --git a/apps/rustgis-cli/src/main.rs b/apps/rustgis-cli/src/main.rs index ce0ec841b..67b5d842e 100644 --- a/apps/rustgis-cli/src/main.rs +++ b/apps/rustgis-cli/src/main.rs @@ -12,6 +12,7 @@ //! [--provider anthropic|openai|chatgpt|gemini|openrouter|ollama|custom] [--model M] [--base-url URL] [--yes] [--save out.rgis] [--trace log.txt] //! rustgis-cli ask --models [--provider P] # list the provider's models //! rustgis-cli ask --accounts | --account CLIENT_ID # list or select a saved registration +//! rustgis-cli ask --remove-account CLIENT_ID # sign out and forget a saved registration //! rustgis-cli ask --login [--another-account] # sign in with ChatGPT to use your ChatGPT plan //! rustgis-cli ask --logout //! rustgis-cli mcp [--sample | --project p.rgis | --data f.geojson …] [--yes] # an MCP server on stdio @@ -42,7 +43,7 @@ use serde_json::{Value, json}; fn usage() -> ExitCode { eprintln!( - "usage:\n rustgis-cli commands | tools\n rustgis-cli plugins [install | uninstall | enable | disable | browse | get | prepare ] [--registry URL]\n rustgis-cli skills [add | remove | enable | disable ]\n rustgis-cli info \n rustgis-cli logo [--size N] [--canvas WxH --at X,Y (.bmp only)]\n rustgis-cli render [--basemap NAME] [--tiles URL] [--width N] [--height N] [--dpi N] [--worldview auto|default|china]\n rustgis-cli run [--sample] [--project P] [--data F]… --cmd 'id={{json}}'… [--save P] [--worldview auto|default|china]\n rustgis-cli ask [--sample] [--project P] [--data F]… \"request\" [--provider P] [--model M] [--base-url URL] [--yes] [--save P]\n rustgis-cli ask --models [--provider P]\n rustgis-cli ask --login [--another-account] [--account CLIENT_ID] | --logout\n rustgis-cli ask --accounts | --account CLIENT_ID\n rustgis-cli mcp [--sample] [--project P] [--data F]… [--yes] [--attach [PORT] [--token-file FILE] [--timeout SECONDS (5-600)]]\n rustgis-cli mcp-register [--client claude] [--name NAME] [--port PORT]" + "usage:\n rustgis-cli commands | tools\n rustgis-cli plugins [install | uninstall | enable | disable | browse | get | prepare ] [--registry URL]\n rustgis-cli skills [add | remove | enable | disable ]\n rustgis-cli info \n rustgis-cli logo [--size N] [--canvas WxH --at X,Y (.bmp only)]\n rustgis-cli render [--basemap NAME] [--tiles URL] [--width N] [--height N] [--dpi N] [--worldview auto|default|china]\n rustgis-cli run [--sample] [--project P] [--data F]… --cmd 'id={{json}}'… [--save P] [--worldview auto|default|china]\n rustgis-cli ask [--sample] [--project P] [--data F]… \"request\" [--provider P] [--model M] [--base-url URL] [--yes] [--save P]\n rustgis-cli ask --models [--provider P]\n rustgis-cli ask --login [--another-account] [--account CLIENT_ID] | --logout\n rustgis-cli ask --accounts | --account CLIENT_ID | --remove-account CLIENT_ID\n rustgis-cli mcp [--sample] [--project P] [--data F]… [--yes] [--attach [PORT] [--token-file FILE] [--timeout SECONDS (5-600)]]\n rustgis-cli mcp-register [--client claude] [--name NAME] [--port PORT]" ); ExitCode::from(2) } @@ -197,6 +198,7 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { "--registry", "--worldview", "--account", + "--remove-account", ]; let prompt = args .iter() @@ -235,6 +237,24 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { return Ok(()); } } + if let Some(id) = opt(args, "--remove-account") { + use rustgis_assistant::chatgpt; + let accounts = chatgpt::accounts()?; + let matches: Vec<_> = accounts.iter().filter(|c| c.client_id == id).collect(); + let [c] = matches.as_slice() else { + return Err("--remove-account needs an unambiguous client id from --accounts".into()); + }; + if let Some(r) = chatgpt::remove(&c.client_id, &c.subject)? + && let Err(e) = chatgpt::revoke(&r) + { + eprintln!( + "removed on this computer, but OpenAI didn't confirm the sign-out ({e}); disconnect RustGIS in ChatGPT's settings if needed" + ); + return Ok(()); + } + eprintln!("removed {}", c.label()); + return Ok(()); + } let mut cfg = Config::from_env(&provider); if let Some(m) = opt(args, "--model").map(str::to_string).or_else(|| std::env::var("RUSTGIS_ASSISTANT_MODEL").ok()) { cfg.model = m; @@ -270,6 +290,9 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { } let who = if c.email.is_empty() { "your ChatGPT account" } else { c.email.as_str() }; eprintln!("signed in as {who}; RustGIS now uses your ChatGPT plan (manage usage at {})", chatgpt::USAGE_URL); + if c.kept == chatgpt::Kept::Session { + eprintln!("this sign-in isn't remembered: the command line can't use the system keychain, so it lasts for this command only"); + } return Ok(()); } // A signed-in account's requests renew its tokens as needed. diff --git a/apps/rustgis/Cargo.toml b/apps/rustgis/Cargo.toml index 118511541..1c9a4a44e 100644 --- a/apps/rustgis/Cargo.toml +++ b/apps/rustgis/Cargo.toml @@ -13,6 +13,8 @@ workspace = true [dependencies] rustgis-engine = { workspace = true, features = ["geolibre"] } rustgis-ui-egui = { workspace = true } +# The ChatGPT sign-in, given the keychain at start-up. +rustgis-assistant = { workspace = true } eframe = { workspace = true, features = ["clipboard", "default_fonts", "links", "wgpu", "accesskit", "wayland", "x11"] } egui = { workspace = true } serde_json = { workspace = true } diff --git a/apps/rustgis/src/main.rs b/apps/rustgis/src/main.rs index d57c2cd65..36b9430c7 100644 --- a/apps/rustgis/src/main.rs +++ b/apps/rustgis/src/main.rs @@ -37,6 +37,10 @@ mod prefs { } pub fn services() -> Services { + let secrets: std::sync::Arc = + std::sync::Arc::new(crate::keychain::Keychain::default()); + // The ChatGPT sign-in's tokens go to the same keychain (its accounts stay in the file). + rustgis_assistant::chatgpt::set_secret_store(Some(secrets.clone())); Services { load_pref: Some(Box::new(|key: &str| read().get(key).and_then(Value::as_str).map(str::to_string))), save_pref: Some(Box::new(|key: &str, value: &str| { @@ -54,7 +58,7 @@ mod prefs { log::warn!("could not save preferences: {e}"); } })), - secrets: Some(std::sync::Arc::new(crate::keychain::Keychain::default())), + secrets: Some(secrets), ..Services::default() } } diff --git a/crates/assistant/src/chatgpt.rs b/crates/assistant/src/chatgpt.rs index b683032f1..56a2620e0 100644 --- a/crates/assistant/src/chatgpt.rs +++ b/crates/assistant/src/chatgpt.rs @@ -11,9 +11,11 @@ //! (`https://api.openai.com/v1`), renewed with its rotating refresh token, and revoked on sign-out. //! //! The pure parts (the authorization page, the token requests and replies, the credentials file's -//! shape, deciding when to renew) are plain data for the tests; the files, the id token's -//! signature and the callback server are desktop only. Tokens are never logged, and the `Debug` -//! form of [`Credentials`] hides them. +//! shape, deciding when to renew) are plain data for the tests; the files, the system keychain, +//! the id token's signature and the callback server are desktop only. The settings folder's +//! `chatgpt-auth.json` keeps the registrations; their tokens go to the system keychain +//! ([`set_secret_store`]). Tokens are never logged, and the `Debug` form of [`Credentials`] hides +//! them. use base64::Engine; use serde_json::{Value, json}; @@ -47,6 +49,21 @@ pub const REFRESH_MARGIN: i64 = 5 * 60; /// The clock skew allowed when checking an id token's expiry. pub const CLOCK_SKEW: i64 = 5 * 60; +/// Where a sign-in's tokens are kept. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum Kept { + /// There are none. + #[default] + Nowhere, + /// In the system keychain. + Keychain, + /// Still in the settings folder's file, from before the keychain: they move to the keychain + /// the next time it's available. + SettingsFile, + /// In memory only, for this session: the keychain wasn't available to remember them. + Session, +} + /// The registration of RustGIS for a ChatGPT account and, while signed in, its tokens. #[derive(Clone, Default, PartialEq, Eq)] pub struct Credentials { @@ -73,6 +90,11 @@ pub struct Credentials { /// Changes with each sign-in and sign-out (not renewal or selection): work begun before /// either is refused after it. pub session: String, + /// Changes with every change of the tokens; the tokens in the keychain must carry the same, + /// or they are out of date (empty when there are none to look for). + pub rev: String, + /// Where the tokens are kept (never saved). + pub kept: Kept, } impl std::fmt::Debug for Credentials { @@ -82,6 +104,7 @@ impl std::fmt::Debug for Credentials { .field("signed_in", &self.signed_in()) .field("scopes", &self.scopes) .field("saved_at", &self.saved_at) + .field("kept", &self.kept) .finish_non_exhaustive() } } @@ -93,6 +116,8 @@ pub struct Account { pub email: String, /// The id token's subject. pub subject: String, + /// Where the sign-in's tokens are kept. + pub kept: Kept, } /// The registration a request is for: its issued client id, the verified subject, and the @@ -140,13 +165,29 @@ impl Credentials { /// The verified identity for display. pub fn account(&self) -> Account { - Account { email: self.email.clone(), subject: self.subject.clone() } + Account { email: self.email.clone(), subject: self.subject.clone(), kept: self.kept } + } + + /// Whether there are any tokens (an id token alone counts). + pub fn has_tokens(&self) -> bool { + !(self.access_token.is_empty() && self.refresh_token.is_empty() && self.id_token.is_empty()) + } + + /// The registration without its tokens. + pub fn without_tokens(&self) -> Credentials { + Credentials { + id_token: String::new(), + access_token: String::new(), + refresh_token: String::new(), + kept: Kept::Nowhere, + ..self.clone() + } } - /// Reads a saved registration. + /// Reads a saved registration. Tokens are read too: older versions saved them in the file. pub fn from_json(v: &Value) -> Result { let field = |k: &str| v.get(k).and_then(Value::as_str).unwrap_or("").trim().to_string(); - let c = Credentials { + let mut c = Credentials { client_id: field("client_id"), subject: field("subject"), email: field("email"), @@ -158,29 +199,32 @@ impl Credentials { plan_declined: v.get("plan_declined").and_then(Value::as_bool).unwrap_or(false), saved_at: field("saved_at"), session: field("session"), + rev: field("rev"), + kept: Kept::Nowhere, }; if c.client_id.is_empty() || c.client_id == REGISTER { return Err("the sign-in has no registration".into()); } + if c.has_tokens() { + c.kept = Kept::SettingsFile; + } Ok(c) } - /// Serializes credentials; never log this value. + /// The registration as saved in the settings folder: never a token. pub fn to_json(&self) -> Value { json!({ "issuer": ISSUER, "client_id": self.client_id, "subject": self.subject, "email": self.email, - "id_token": self.id_token, - "access_token": self.access_token, - "refresh_token": self.refresh_token, "token_type": "Bearer", "expires_at": self.expires_at, "scopes": self.scopes, "plan_declined": self.plan_declined, "saved_at": self.saved_at, "session": self.session, + "rev": self.rev, }) } @@ -233,7 +277,7 @@ pub fn check_id_claims(claims: &Value, client_id: &str, nonce: &str, now: i64) - if s("sub").is_empty() { return Err("the id token names no account".into()); } - Ok(Account { email: s("email").to_string(), subject: s("sub").to_string() }) + Ok(Account { email: s("email").to_string(), subject: s("sub").to_string(), kept: Kept::Nowhere }) } /// What a token endpoint gave. @@ -334,6 +378,8 @@ pub fn signed_in(client_id: &str, account: &Account, g: &Grant, now: i64) -> Cre scopes, saved_at: rfc3339(now), session: String::new(), + rev: String::new(), + kept: Kept::Nowhere, } } @@ -602,13 +648,16 @@ mod native { use serde_json::Value; use super::{ - Account, Authorization, Binding, Credentials, DISCOVERY_URL, GrantError, PORT, REGISTER, authorize_url, callback_client_id, + Account, Authorization, Binding, Credentials, DISCOVERY_URL, GrantError, Kept, PORT, REGISTER, authorize_url, callback_client_id, check_id_claims, discovered, exchange_request, host_id_from, jwt_payload, parse_grant, query_params, refresh_request, renewed, revoke_request, valid_host_id, }; use crate::provider::{HttpRequest, HttpResponse}; use crate::{Error, Recovery, http}; use rustgis_engine::data::dirs::config_dir; + use rustgis_engine::data::secrets::{Secret, SecretStore}; + use std::collections::BTreeMap; + use std::sync::{LazyLock, MutexGuard}; /// The credentials' file name, in the app's settings folder. const FILE: &str = "chatgpt-auth.json"; @@ -644,29 +693,116 @@ mod native { Ok(file) } + // The store lock covers every read, compare and write of the file and of the keychain, so + // two processes never both use a refresh token that rotates. fn store(path: &Path) -> Result { file_lock(path, "store-lock") } + /// The system keychain, and the tokens it couldn't keep, by account: they last for this + /// session (this process), while the file's entry has the same `rev`. Tokens the keychain + /// keeps are read from it each time, under the store lock: another process may have rotated + /// them. + pub(crate) struct Vault { + store: Mutex>>, + held: Mutex>, + } + + struct Held { + rev: String, + id_token: String, + access_token: String, + refresh_token: String, + } + + impl Held { + fn of(c: &Credentials) -> Held { + Held { + rev: c.rev.clone(), + id_token: c.id_token.clone(), + access_token: c.access_token.clone(), + refresh_token: c.refresh_token.clone(), + } + } + + fn apply(&self, entry: &Credentials) -> Credentials { + Credentials { + id_token: self.id_token.clone(), + access_token: self.access_token.clone(), + refresh_token: self.refresh_token.clone(), + kept: Kept::Session, + ..entry.clone() + } + } + } + + impl Vault { + pub(crate) fn new(store: Option>) -> Vault { + Vault { store: Mutex::new(store), held: Mutex::default() } + } + + fn store(&self) -> Option> { + self.store.lock().unwrap_or_else(|e| e.into_inner()).clone() + } + + fn held(&self) -> MutexGuard<'_, BTreeMap<(String, String), Held>> { + self.held.lock().unwrap_or_else(|e| e.into_inner()) + } + } + + static VAULT: LazyLock = LazyLock::new(|| Vault::new(None)); + + fn vault() -> &'static Vault { + &VAULT + } + + /// Gives the system keychain that keeps the sign-in's tokens (the desktop app's). Without one + /// (the command line), a sign-in from before the keychain keeps working from the settings + /// folder, and a new one lasts for the session only. + pub fn set_secret_store(store: Option>) { + *vault().store.lock().unwrap_or_else(|e| e.into_inner()) = store; + } + + /// The keychain account of a registration's tokens. + fn secret_name(client_id: &str, subject: &str) -> String { + format!("chatgpt/{client_id}/{subject}") + } + + fn id(c: &Credentials) -> (String, String) { + (c.client_id.clone(), c.subject.clone()) + } + + fn secret(c: &Credentials) -> Secret { + Secret::new( + serde_json::json!({"rev": c.rev, "id_token": c.id_token, "access_token": c.access_token, "refresh_token": c.refresh_token}) + .to_string(), + ) + } + + fn keychain_error(what: &str, e: &rustgis_engine::data::secrets::SecretError) -> Error { + Error::new(Recovery::Auth, format!("{what}: {e}")) + } + #[derive(Default)] struct Accounts { + /// The registrations. Only those from before the keychain ([`Kept::SettingsFile`]) carry + /// tokens: the file's own. entries: Vec, active: Option<(String, String)>, } impl Accounts { fn selected(&self) -> Option<&Credentials> { - self.entries.iter().find(|c| self.active.as_ref() == Some(&(c.client_id.clone(), c.subject.clone()))) + self.entries.iter().find(|c| self.active.as_ref() == Some(&id(c))) } fn put(&mut self, c: &Credentials) { self.entries.retain(|old| old.client_id != c.client_id || old.subject != c.subject); self.entries.push(c.clone()); - self.active = Some((c.client_id.clone(), c.subject.clone())); + self.active = Some(id(c)); } } - // Callers hold the store lock for the entire read/compare/write transaction. fn read_accounts(path: &Path) -> Result { let text = match std::fs::read_to_string(path) { Ok(text) => text, @@ -688,44 +824,161 @@ mod native { fn write_accounts(path: &Path, accounts: &Accounts) -> Result<(), Error> { let active = accounts.active.as_ref().map(|(client, subject)| serde_json::json!({"client_id": client, "subject": subject})); + let entry = |c: &Credentials| { + let mut v = c.to_json(); + // Tokens from before the keychain stay until it takes them. + if c.kept == Kept::SettingsFile { + v["id_token"] = c.id_token.clone().into(); + v["access_token"] = c.access_token.clone().into(); + v["refresh_token"] = c.refresh_token.clone().into(); + } + v + }; write_private( path, - &serde_json::json!({"version": 2, "active": active, - "accounts": accounts.entries.iter().map(Credentials::to_json).collect::>()}) - .to_string(), + &serde_json::json!({"version": 2, "active": active, "accounts": accounts.entries.iter().map(entry).collect::>()}) + .to_string(), ) } - fn read_credentials(path: &Path) -> Result { - read_accounts(path)?.selected().cloned().ok_or_else(|| "not signed in to ChatGPT".into()) + /// A file entry with its tokens: from the file (from before the keychain, moved to the + /// keychain now if it takes them), this process, or the keychain. Tokens of another `rev` are + /// out of date: none. Under the store lock. + fn with_tokens(v: &Vault, path: &Path, accounts: &mut Accounts, entry: Credentials) -> Result { + if entry.kept == Kept::SettingsFile { + let Some(store) = v.store() else { return Ok(entry) }; + let moved = Credentials { rev: random_token()?, kept: Kept::Keychain, ..entry.clone() }; + if let Err(e) = store.save(&secret_name(&entry.client_id, &entry.subject), &secret(&moved)) { + log::warn!("the ChatGPT sign-in stays in the settings folder: {e}"); + return Ok(entry); + } + // Out of the file only now that the keychain has them. + let mut after = Accounts { entries: accounts.entries.clone(), active: accounts.active.clone() }; + if let Some(e) = after.entries.iter_mut().find(|e| id(e) == id(&moved)) { + *e = moved.without_tokens(); + } + if let Err(e) = write_accounts(path, &after) { + log::warn!("the ChatGPT sign-in stays in the settings folder: {e}"); + return Ok(entry); + } + *accounts = after; + return Ok(moved); + } + if entry.rev.is_empty() { + return Ok(entry); + } + if let Some(h) = v.held().get(&id(&entry)).filter(|h| h.rev == entry.rev) { + return Ok(h.apply(&entry)); + } + // Without a keychain, tokens kept elsewhere are another process's. + let Some(store) = v.store() else { return Ok(entry) }; + let saved = store + .load(&secret_name(&entry.client_id, &entry.subject)) + .map_err(|e| keychain_error("the ChatGPT sign-in can't be read", &e))?; + let Some(saved) = saved.and_then(|s| serde_json::from_str::(s.expose()).ok()) else { return Ok(entry) }; + let field = |k: &str| saved.get(k).and_then(Value::as_str).unwrap_or("").to_string(); + if field("rev") != entry.rev { + return Ok(entry); + } + Ok(Credentials { + id_token: field("id_token"), + access_token: field("access_token"), + refresh_token: field("refresh_token"), + kept: Kept::Keychain, + ..entry + }) + } + + /// Puts a registration and its tokens in `accounts` and selects it: the tokens go to the + /// keychain; to the file only for an entry whose tokens are still there (from before the + /// keychain) when the keychain refuses them; else to this session. Without tokens, any are + /// deleted. Gives the credentials as kept; the caller writes the file, under the store lock. + fn put_tokens(v: &Vault, accounts: &mut Accounts, c: &Credentials) -> Result { + let name = secret_name(&c.client_id, &c.subject); + let store = v.store(); + if !c.has_tokens() { + let entry = Credentials { rev: String::new(), ..c.without_tokens() }; + v.held().remove(&id(c)); + // The file's empty `rev` already makes any left in the keychain unusable. + if let Some(Err(e)) = store.map(|s| s.delete(&name)) { + log::warn!("the ChatGPT sign-in's tokens couldn't be deleted from the system keychain: {e}"); + } + accounts.put(&entry); + return Ok(entry); + } + let mut kept = Credentials { rev: random_token()?, kept: Kept::Keychain, ..c.clone() }; + // Only tokens renewed from the file's own may go back there; a new sign-in never does. + let in_file = c.kept == Kept::SettingsFile && accounts.entries.iter().any(|e| id(e) == id(c) && e.kept == Kept::SettingsFile); + match store.map(|s| s.save(&name, &secret(&kept))) { + Some(Ok(())) => {} + failed => { + if let Some(Err(e)) = failed { + log::warn!("the system keychain didn't keep the ChatGPT sign-in: {e}"); + } + kept.kept = if in_file { Kept::SettingsFile } else { Kept::Session }; + } + } + if kept.kept == Kept::Session { + v.held().insert(id(c), Held::of(&kept)); + } else { + v.held().remove(&id(c)); + } + accounts.put(&if kept.kept == Kept::SettingsFile { kept.clone() } else { kept.without_tokens() }); + Ok(kept) + } + + /// The selected registration with its tokens. Under the store lock. + fn selected_at(path: &Path, v: &Vault) -> Result, Error> { + let mut accounts = read_accounts(path)?; + let Some(entry) = accounts.selected().cloned() else { return Ok(None) }; + with_tokens(v, path, &mut accounts, entry).map(Some) } - /// The selected registration, including signed-out accounts. + /// Saves `c` and selects it. Under the store lock. + fn write_credentials(path: &Path, v: &Vault, c: &Credentials) -> Result { + let mut accounts = read_accounts(path)?; + let kept = put_tokens(v, &mut accounts, c)?; + write_accounts(path, &accounts)?; + Ok(kept) + } + + /// The selected registration, including signed-out accounts (without its tokens when the + /// keychain can't give them). pub fn load_any() -> Option { let path = auth_path()?; let _guard = store(&path).ok()?; - read_credentials(&path).ok() + load_any_at(&path, vault()) } - /// Saved registrations, including those signed out. + fn load_any_at(path: &Path, v: &Vault) -> Option { + match selected_at(path, v) { + Ok(c) => c, + Err(e) => { + log::warn!("{e}"); + read_accounts(path).ok()?.selected().map(Credentials::without_tokens) + } + } + } + + /// Saved registrations, including those signed out (without tokens). pub fn accounts() -> Result, Error> { let path = auth_path().ok_or("no settings folder")?; let _guard = store(&path)?; - Ok(read_accounts(&path)?.entries) + Ok(read_accounts(&path)?.entries.iter().map(Credentials::without_tokens).collect()) } /// Selects an existing registration without signing it in. pub fn select(client_id: &str, subject: &str) -> Result { - select_at(&auth_path().ok_or("no settings folder")?, client_id, subject) + select_at(&auth_path().ok_or("no settings folder")?, vault(), client_id, subject) } - fn select_at(path: &Path, client_id: &str, subject: &str) -> Result { + fn select_at(path: &Path, v: &Vault, client_id: &str, subject: &str) -> Result { let _guard = store(path)?; let mut accounts = read_accounts(path)?; accounts.active = Some((client_id.to_string(), subject.to_string())); - let c = accounts.selected().cloned().ok_or("the saved account wasn't found")?; + let entry = accounts.selected().cloned().ok_or("the saved account wasn't found")?; write_accounts(path, &accounts)?; - Ok(c) + with_tokens(v, path, &mut accounts, entry) } /// The account signed in on this computer with the plan's use allowed, if any. @@ -751,12 +1004,6 @@ mod native { Ok(()) } - fn write_credentials(path: &Path, c: &Credentials) -> Result<(), Error> { - let mut accounts = read_accounts(path)?; - accounts.put(c); - write_accounts(path, &accounts) - } - /// This computer's host id, created the first time. pub fn host_id() -> Result { let path = config_dir().ok_or("no settings folder")?.join(HOST_FILE); @@ -825,7 +1072,7 @@ mod native { /// Renews the tokens with `send` and keeps the result. An unusable refresh token is cleared /// (the registration stays for the next sign-in). - fn refresh(path: &Path, c: &Credentials, send: Send) -> Result { + fn refresh(path: &Path, v: &Vault, c: &Credentials, send: Send) -> Result { if c.refresh_token.is_empty() { return Err(Error::new(Recovery::SignIn, "the sign-in can't be renewed; sign in again")); } @@ -833,20 +1080,16 @@ mod native { let grant = parse_grant(&resp); let _guard = store(path)?; // A user can sign out or sign into another account while the grant is in flight. - if read_credentials(path).ok().as_ref() != Some(c) { + if selected_at(path, v).ok().flatten().as_ref() != Some(c) { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; try again")); } match grant { - Ok(g) => { - let fresh = renewed(c, &g, now()); - write_credentials(path, &fresh)?; - Ok(fresh) - } + Ok(g) => write_credentials(path, v, &renewed(c, &g, now())), Err(GrantError::Unusable) => { // Not a sign-out: the id token stays to skip choosing the account next time. let mut registration = c.registration(); registration.id_token = c.id_token.clone(); - write_credentials(path, ®istration)?; + write_credentials(path, v, ®istration)?; Err(Error::token(&resp, GrantError::Unusable)) } Err(e) => Err(Error::token(&resp, e)), @@ -861,21 +1104,18 @@ mod native { pub fn fresh(send: Send) -> Result { let path = auth_path().ok_or("no settings folder")?; let _guard = renewal_lock(&path)?; - fresh_at(&path, send) + fresh_at(&path, vault(), send) } - fn fresh_at(path: &Path, send: Send) -> Result { + fn fresh_at(path: &Path, v: &Vault, send: Send) -> Result { let c = { let _guard = store(path)?; - read_credentials(path) - .ok() - .filter(Credentials::signed_in) - .ok_or_else(|| Error::new(Recovery::SignIn, "not signed in to ChatGPT"))? + selected_at(path, v)?.filter(Credentials::signed_in).ok_or_else(|| Error::new(Recovery::SignIn, "not signed in to ChatGPT"))? }; if !c.plan_allowed() { return Err(Error::new(Recovery::Consent, "RustGIS isn't allowed to use your ChatGPT plan; sign in again and allow it")); } - if c.expires_soon(now()) { refresh(path, &c, send) } else { Ok(c) } + if c.expires_soon(now()) { refresh(path, v, &c, send) } else { Ok(c) } } /// Sends a request as `account` (blocking): refused if another account is now selected or it @@ -895,7 +1135,7 @@ mod native { if !super::api_url(&req.url) { return Err(Error::new(Recovery::Configuration, "a ChatGPT sign-in can only be used with the OpenAI API")); } - send_at(req, account, &auth_path().ok_or("no settings folder")?, &mut |req| { + send_at(req, account, &auth_path().ok_or("no settings folder")?, vault(), &mut |req| { if req.url == super::TOKEN_URL { http::send_timeout(req, Duration::from_secs(30)).map_err(Error::auth_context) } else if let Some(f) = on_chunk.as_mut() { @@ -906,17 +1146,17 @@ mod native { }) } - fn send_at(req: &HttpRequest, account: &Binding, path: &Path, transport: Send) -> Result { + fn send_at(req: &HttpRequest, account: &Binding, path: &Path, v: &Vault, transport: Send) -> Result { let c = { let _guard = renewal_lock(path)?; { let _store = store(path)?; // No saved sign-in at all reads as signed out. - check_account(account, &read_credentials(path).unwrap_or_default())?; + check_account(account, &selected_at(path, v)?.unwrap_or_default())?; } - fresh_at(path, transport)? + fresh_at(path, v, transport)? }; - check_final(path, account, &c)?; + check_final(path, v, account, &c)?; let resp = transport(&c.authorize(req.clone()))?; if resp.status != 401 { return Ok(resp); @@ -925,8 +1165,7 @@ mod native { let _guard = renewal_lock(path)?; let current = { let _store = store(path)?; - read_credentials(path) - .ok() + selected_at(path, v)? .filter(Credentials::signed_in) .ok_or_else(|| Error::new(Recovery::SignIn, "not signed in to ChatGPT"))? }; @@ -935,9 +1174,9 @@ mod native { if current.binding() != c.binding() { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT account changed; try again")); } - if current.access_token != c.access_token { current } else { refresh(path, ¤t, transport)? } + if current.access_token != c.access_token { current } else { refresh(path, v, ¤t, transport)? } }; - check_final(path, account, &c)?; + check_final(path, v, account, &c)?; transport(&c.authorize(req.clone())) } @@ -952,9 +1191,9 @@ mod native { Ok(()) } - fn check_final(path: &Path, account: &Binding, c: &Credentials) -> Result<(), Error> { + fn check_final(path: &Path, v: &Vault, account: &Binding, c: &Credentials) -> Result<(), Error> { let _guard = store(path)?; - let saved = read_credentials(path)?; + let saved = selected_at(path, v)?.unwrap_or_default(); check_account(account, &saved)?; if !c.plan_allowed() || !saved.plan_allowed() { return Err(Error::new(Recovery::Consent, "RustGIS isn't allowed to use your ChatGPT plan; sign in again and allow it")); @@ -977,12 +1216,12 @@ mod native { } } - /// Signs out on this computer at once: the tokens are cleared (the registration and host id + /// Signs out on this computer at once: the tokens are deleted (the registration and host id /// stay for the next sign-in). Revoke the session with what it gives ([`revoke`]), off the /// UI thread. pub fn sign_out() -> Result, Error> { match auth_path() { - Some(path) => sign_out_at(&path), + Some(path) => sign_out_at(&path, vault()), None => Ok(None), } } @@ -990,24 +1229,71 @@ mod native { /// Signs out only if the displayed registration is still selected. pub fn sign_out_selected(account: &Binding) -> Result, Error> { let path = auth_path().ok_or("no settings folder")?; - sign_out_checked(&path, Some(account)) + sign_out_checked(&path, vault(), Some(account)) } - fn sign_out_at(path: &Path) -> Result, Error> { - sign_out_checked(path, None) + fn sign_out_at(path: &Path, v: &Vault) -> Result, Error> { + sign_out_checked(path, v, None) } - fn sign_out_checked(path: &Path, expected: Option<&Binding>) -> Result, Error> { + fn sign_out_checked(path: &Path, v: &Vault, expected: Option<&Binding>) -> Result, Error> { let _guard = store(path)?; - let accounts = read_accounts(path)?; - let Some(c) = accounts.selected() else { return Ok(None) }; - if expected.is_some_and(|e| *e != c.binding()) { + let mut accounts = read_accounts(path)?; + let Some(entry) = accounts.selected().cloned() else { return Ok(None) }; + if expected.is_some_and(|e| *e != entry.binding()) { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; select the account again")); } + // The refresh token to revoke. A keychain that can't give it stops the sign-out: its + // tokens would stay there, unrevoked. + let c = with_tokens(v, path, &mut accounts, entry)?; + out_of_reach(v, &c)?; let mut registration = c.registration(); registration.session = random_token()?; - write_credentials(path, ®istration)?; - Ok((!c.refresh_token.is_empty()).then_some(Revocation { client_id: c.client_id.clone(), refresh_token: c.refresh_token.clone() })) + put_tokens(v, &mut accounts, ®istration)?; + write_accounts(path, &accounts)?; + Ok(revocation(&c)) + } + + fn revocation(c: &Credentials) -> Option { + (!c.refresh_token.is_empty()).then(|| Revocation { client_id: c.client_id.clone(), refresh_token: c.refresh_token.clone() }) + } + + /// Refuses to forget tokens kept in a keychain this process doesn't have (the command line's): + /// they would stay there, unrevoked. + fn out_of_reach(v: &Vault, c: &Credentials) -> Result<(), Error> { + if v.store().is_none() && !c.rev.is_empty() && !c.has_tokens() { + return Err(Error::new( + Recovery::Auth, + "this ChatGPT sign-in is kept in the system keychain, which isn't available here; sign out in the RustGIS app", + )); + } + Ok(()) + } + + /// Removes a saved registration, with its tokens (signing it out first when signed in). + /// Revoke the session with what it gives ([`revoke`]), off the UI thread. + pub fn remove(client_id: &str, subject: &str) -> Result, Error> { + remove_at(&auth_path().ok_or("no settings folder")?, vault(), client_id, subject) + } + + fn remove_at(path: &Path, v: &Vault, client_id: &str, subject: &str) -> Result, Error> { + let _guard = store(path)?; + let mut accounts = read_accounts(path)?; + let who = (client_id.to_string(), subject.to_string()); + let Some(entry) = accounts.entries.iter().find(|e| id(e) == who).cloned() else { return Ok(None) }; + let c = with_tokens(v, path, &mut accounts, entry)?; + out_of_reach(v, &c)?; + // The tokens go first: a registration whose tokens the keychain kept is never forgotten. + v.held().remove(&who); + if let Some(store) = v.store() { + store.delete(&secret_name(client_id, subject)).map_err(|e| keychain_error("the ChatGPT account can't be removed", &e))?; + } + accounts.entries.retain(|e| id(e) != who); + if accounts.active.as_ref() == Some(&who) { + accounts.active = None; + } + write_accounts(path, &accounts)?; + Ok(revocation(&c)) } /// Ends a signed-out session at OpenAI (blocking; retried a few times on network and server @@ -1216,7 +1502,7 @@ mod native { let result = exchanged.and_then(|(client_id, grant)| { let kept = finish(p, &client_id, &grant, &mut |req| auth_send(req, cancel, deadline)) .and_then(|c| check_attempt(cancel, deadline).map(|()| c)) - .and_then(|c| save_if_active(cancel, &c, &mut |c| save(c, p.expected.as_ref())).map(|()| c)); + .and_then(|c| save_if_active(cancel, &c, &mut |c| save(c, p.expected.as_ref()))); if kept.is_err() && !grant.refresh_token.is_empty() { let _ = revoke(&Revocation { client_id, refresh_token: grant.refresh_token.clone() }); } @@ -1295,29 +1581,32 @@ mod native { Ok(c) } - /// Keeps a verified sign-in, with plan permission recorded separately. - fn save(c: &Credentials, expected: Option<&Credentials>) -> Result<(), Error> { + /// Keeps a verified sign-in, with plan permission recorded separately. Gives it as kept. + fn save(c: &Credentials, expected: Option<&Credentials>) -> Result { let path = auth_path().ok_or("no settings folder")?; - save_at(&path, c, expected) + save_at(&path, vault(), c, expected) } - fn save_at(path: &Path, c: &Credentials, expected: Option<&Credentials>) -> Result<(), Error> { + fn save_at(path: &Path, v: &Vault, c: &Credentials, expected: Option<&Credentials>) -> Result { let _guard = store(path)?; - let current = read_accounts(path)?.selected().cloned(); - // Who, and whether signed in: a sign-out or another sign-in meanwhile is not undone. - let identity = |c: &Credentials| (c.binding(), c.signed_in()); - if current.as_ref().map(identity) != expected.map(identity) { + let mut accounts = read_accounts(path)?; + // Who, and whether it had tokens (from the file alone: a locked keychain still compares): + // a sign-out or another sign-in meanwhile is not undone. + let identity = |c: &Credentials| (c.binding(), !c.rev.is_empty() || c.kept == Kept::SettingsFile); + if accounts.selected().map(identity) != expected.map(identity) { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; select the account again")); } // Keep valid identity even when plan permission was declined. - write_credentials(path, c) + let kept = put_tokens(v, &mut accounts, c)?; + write_accounts(path, &accounts)?; + Ok(kept) } fn save_if_active( cancel: &Mutex, c: &Credentials, - persist: &mut dyn FnMut(&Credentials) -> Result<(), Error>, - ) -> Result<(), Error> { + persist: &mut dyn FnMut(&Credentials) -> Result, + ) -> Result { let cancelled = cancel.lock().map_err(|_| "the sign-in was cancelled")?; if *cancelled { return Err(Error::new(Recovery::Cancelled, "the sign-in was cancelled")); @@ -1390,6 +1679,7 @@ mod native { #[cfg(test)] mod tests { use super::*; + use rustgis_engine::data::secrets::{MemorySecrets, SecretError}; fn access(exp: i64) -> String { let payload = serde_json::json!({"client_id": "oaiapp_synthetic", "sub": "user-1", "exp": exp}); @@ -1409,9 +1699,50 @@ mod native { plan_declined: false, saved_at: String::new(), session: "session-1".into(), + rev: String::new(), + kept: Kept::Nowhere, } } + /// A process's vault, on a keychain of its own. + fn keychain() -> Vault { + Vault::new(Some(Arc::new(MemorySecrets::new()))) + } + + /// A process's vault, on a keychain other processes share. + fn sharing(store: &Arc) -> Vault { + Vault::new(Some(store.clone() as Arc)) + } + + fn failing() -> Vault { + Vault::new(Some(Arc::new(MemorySecrets::failing(SecretError::Denied)))) + } + + fn read_credentials(path: &Path, v: &Vault) -> Result { + selected_at(path, v)?.ok_or_else(|| "nothing selected".into()) + } + + /// The credentials without where, and as which revision, they are kept. + fn plain(c: Credentials) -> Credentials { + Credentials { rev: String::new(), kept: Kept::Nowhere, ..c } + } + + /// An entry as versions before the keychain saved it, tokens and all. + fn legacy_json(c: &Credentials) -> Value { + let mut v = c.to_json(); + v["id_token"] = c.id_token.clone().into(); + v["access_token"] = c.access_token.clone().into(); + v["refresh_token"] = c.refresh_token.clone().into(); + v + } + + fn file_has_tokens(path: &Path) -> bool { + let text = std::fs::read_to_string(path).unwrap(); + ["synthetic-id", "synthetic-refresh", "rotated", "e30."].iter().any(|t| text.contains(t)) + } + + const NAME: &str = "chatgpt/oaiapp_synthetic/user-1"; + fn grant(expires_at: i64, refresh: &str) -> HttpResponse { let body = serde_json::json!({"access_token": access(expires_at), "refresh_token": refresh, "expires_in": expires_at - now()}); HttpResponse { status: 200, body: body.to_string().into_bytes(), headers: vec![] } @@ -1426,10 +1757,151 @@ mod native { } } + #[test] + fn tokens_go_to_the_keychain_and_never_to_the_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + let v = sharing(&store); + let c = synthetic(now() + 100_000); + assert_eq!(write_credentials(&path, &v, &c).unwrap().kept, Kept::Keychain); + assert!(!file_has_tokens(&path)); + let json: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + let entry = &json["accounts"][0]; + for k in ["client_id", "subject", "email", "scopes", "expires_at"] { + assert!(!entry[k].is_null(), "{k} stays in the file"); + } + assert_eq!(json["active"]["subject"], "user-1"); + assert!(store.load(NAME).unwrap().unwrap().expose().contains("synthetic-refresh")); + // Another process, on the same keychain, reads the same sign-in. + let other = read_credentials(&path, &sharing(&store)).unwrap(); + assert_eq!(other.kept, Kept::Keychain); + assert_eq!(plain(other), c); + // Without the keychain's tokens, it is signed out; no error. + store.delete(NAME).unwrap(); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); + } + + #[test] + fn tokens_from_the_file_move_to_the_keychain_once_it_takes_them() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let old = synthetic(now() + 100_000); + let text = serde_json::json!({"version": 2, "active": {"client_id": old.client_id, "subject": old.subject}, + "accounts": [legacy_json(&old)]}) + .to_string(); + std::fs::write(&path, &text).unwrap(); + // Without a keychain, or with one that refuses, the file stays as it is and works. + for v in [Vault::new(None), failing()] { + let c = read_credentials(&path, &v).unwrap(); + assert_eq!((plain(c.clone()), c.kept), (old.clone(), Kept::SettingsFile)); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + // Renewing such a sign-in without the keychain keeps the rotated token in the file: + // the old one is used up. + std::fs::write(&path, legacy_json(&synthetic(0)).to_string()).unwrap(); + send_at(&test_request(), &old.binding(), &path, &failing(), &mut |req| { + Ok(if req.url == super::super::TOKEN_URL { + grant(now() + 100_000, "rotated") + } else { + HttpResponse { status: 200, body: vec![], headers: vec![] } + }) + }) + .unwrap(); + let renewed = read_credentials(&path, &Vault::new(None)).unwrap(); + assert_eq!((renewed.refresh_token.as_str(), renewed.kept), ("rotated", Kept::SettingsFile)); + // The keychain takes them: out of the file. + let store = Arc::new(MemorySecrets::new()); + let moved = read_credentials(&path, &sharing(&store)).unwrap(); + assert_eq!((moved.refresh_token.as_str(), moved.kept), ("rotated", Kept::Keychain)); + assert!(!file_has_tokens(&path)); + assert!(store.load(NAME).unwrap().unwrap().expose().contains("rotated")); + assert_eq!(read_credentials(&path, &sharing(&store)).unwrap().refresh_token, "rotated"); + } + + #[test] + fn a_sign_in_the_keychain_refuses_lasts_for_the_session_only() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + for (v, legacy) in [(Vault::new(None), false), (failing(), false), (Vault::new(None), true), (failing(), true)] { + let _ = std::fs::remove_file(&path); + if legacy { + // Signing in again to an account whose old tokens are still in the file. + std::fs::write(&path, legacy_json(&synthetic(now() + 100_000)).to_string()).unwrap(); + } + let expected = load_any_at(&path, &v); + let mut new = synthetic(now() + 100_000); + new.session = "session-2".into(); + new.refresh_token = "rotated".into(); + assert_eq!(save_at(&path, &v, &new, expected.as_ref()).unwrap().kept, Kept::Session); + assert!(!file_has_tokens(&path), "never written to the file instead"); + let c = read_credentials(&path, &v).unwrap(); + assert!(c.signed_in() && c.kept == Kept::Session && c.refresh_token == "rotated"); + // Another process (or the next run) isn't signed in. + assert!(!read_credentials(&path, &Vault::new(None)).unwrap().signed_in()); + } + } + + #[test] + fn sign_out_and_removal_delete_the_stored_tokens() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + let v = sharing(&store); + write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); + // Without the keychain (the command line), its tokens are out of reach: neither + // signing out nor removing forgets them unrevoked. + let none = Vault::new(None); + assert!(sign_out_at(&path, &none).is_err()); + assert!(remove_at(&path, &none, "oaiapp_synthetic", "user-1").is_err()); + assert!(read_credentials(&path, &v).unwrap().signed_in()); + let r = sign_out_at(&path, &v).unwrap().unwrap(); + assert_eq!(r.refresh_token, "synthetic-refresh"); + assert_eq!(store.load(NAME).unwrap(), None); + // Removing: the tokens and the registration. + let mut other = synthetic(now() + 100_000); + other.client_id = "oaiapp_other".into(); + write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); + write_credentials(&path, &v, &other).unwrap(); + // A keychain that refuses keeps the account: its tokens would be left behind. + assert!(remove_at(&path, &failing(), "oaiapp_synthetic", "user-1").is_err()); + assert_eq!(read_accounts(&path).unwrap().entries.len(), 2); + let r = remove_at(&path, &v, "oaiapp_synthetic", "user-1").unwrap().unwrap(); + assert_eq!(r.client_id, "oaiapp_synthetic"); + assert_eq!(store.load(NAME).unwrap(), None); + let accounts = read_accounts(&path).unwrap(); + assert_eq!(accounts.entries.iter().map(|e| e.client_id.as_str()).collect::>(), ["oaiapp_other"]); + assert_eq!(read_credentials(&path, &v).unwrap().client_id, "oaiapp_other", "the other account stays selected"); + remove_at(&path, &v, "oaiapp_other", "user-1").unwrap(); + assert!(read_accounts(&path).unwrap().selected().is_none()); + assert!(remove_at(&path, &v, "oaiapp_other", "user-1").unwrap().is_none()); + } + + #[test] + fn keychain_tokens_of_another_revision_are_never_used() { + // A renewal whose tokens the keychain took but whose file write failed: the rotated + // refresh token is the keychain's, the file still names the previous revision. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + let (first, second) = (sharing(&store), sharing(&store)); + write_credentials(&path, &first, &synthetic(0)).unwrap(); + assert!(read_credentials(&path, &second).unwrap().signed_in()); + let mut rotated = synthetic(now() + 100_000); + rotated.refresh_token = "rotated".into(); + put_tokens(&first, &mut read_accounts(&path).unwrap(), &rotated).unwrap(); + for v in [&first, &second] { + assert!(!read_credentials(&path, v).unwrap().signed_in()); + let e = send_at(&test_request(), &synthetic(0).binding(), &path, v, &mut |_| panic!("no token may be used")).unwrap_err(); + assert_eq!(e.recovery, Recovery::SignIn); + } + } + #[test] fn requests_for_another_registration_or_after_sign_out_are_refused() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); + let v = keychain(); for change in ["client", "subject", "signed-out"] { let mut c = synthetic(now() + 100_000); match change { @@ -1437,8 +1909,8 @@ mod native { "subject" => c.subject = "other-user".into(), _ => c = c.registration(), } - write_credentials(&path, &c).unwrap(); - let e = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| panic!("must refuse before any network I/O")) + write_credentials(&path, &v, &c).unwrap(); + let e = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| panic!("must refuse before any network I/O")) .unwrap_err(); assert!(e.to_string().contains(if change == "signed-out" { "not signed in" } else { "changed" }), "{change}: {e}"); } @@ -1448,42 +1920,44 @@ mod native { fn work_from_before_a_sign_out_is_refused_after_signing_in_again() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); + let v = keychain(); let old = synthetic(now() + 100_000); - write_credentials(&path, &old).unwrap(); + write_credentials(&path, &v, &old).unwrap(); // Another window signs out and back into the same registration. - sign_out_at(&path).unwrap(); + sign_out_at(&path, &v).unwrap(); let mut again = old.clone(); again.session = "session-2".into(); - write_credentials(&path, &again).unwrap(); - let e = send_at(&test_request(), &old.binding(), &path, &mut |_| panic!("must refuse before any network I/O")).unwrap_err(); + write_credentials(&path, &v, &again).unwrap(); + let e = send_at(&test_request(), &old.binding(), &path, &v, &mut |_| panic!("must refuse before any network I/O")).unwrap_err(); assert!(e.to_string().contains("changed"), "{e}"); // A sign-in begun before another process signs out is not kept, even when the account // was already signed out. - sign_out_at(&path).unwrap(); - let started = read_credentials(&path).unwrap(); - sign_out_at(&path).unwrap(); + sign_out_at(&path, &v).unwrap(); + let started = read_credentials(&path, &v).unwrap(); + sign_out_at(&path, &v).unwrap(); let mut late = old.clone(); late.session = "session-3".into(); - assert!(save_at(&path, &late, Some(&started)).is_err()); - assert!(!read_credentials(&path).unwrap().signed_in()); + assert!(save_at(&path, &v, &late, Some(&started)).is_err()); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); // Renewal and selection keep the session. - write_credentials(&path, &again).unwrap(); - assert_eq!(select_at(&path, &again.client_id, &again.subject).unwrap().binding(), again.binding()); + write_credentials(&path, &v, &again).unwrap(); + assert_eq!(select_at(&path, &v, &again.client_id, &again.subject).unwrap().binding(), again.binding()); } #[test] fn renewed_scope_is_checked_before_initial_send_and_retry() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); + let v = keychain(); for retry in [false, true] { - write_credentials(&path, &synthetic(if retry { now() + 100_000 } else { 0 })).unwrap(); + write_credentials(&path, &v, &synthetic(if retry { now() + 100_000 } else { 0 })).unwrap(); let mut inference = 0; - let e = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |req| { + let e = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |req| { if req.url == super::super::TOKEN_URL { let mut r = grant(now() + 100_000, "rotated"); - let mut v: Value = serde_json::from_slice(&r.body).unwrap(); - v["scope"] = serde_json::json!("openid email"); - r.body = v.to_string().into_bytes(); + let mut body: Value = serde_json::from_slice(&r.body).unwrap(); + body["scope"] = serde_json::json!("openid email"); + r.body = body.to_string().into_bytes(); return Ok(r); } inference += 1; @@ -1492,7 +1966,7 @@ mod native { .unwrap_err(); assert!(e.to_string().contains("plan")); assert_eq!(inference, usize::from(retry)); - assert!(!read_credentials(&path).unwrap().plan_allowed()); + assert!(!read_credentials(&path, &v).unwrap().plan_allowed()); } } @@ -1500,21 +1974,22 @@ mod native { fn separate_store_handles_serialize_compare_and_write() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); - write_credentials(&path, &synthetic(0)).unwrap(); + let v = keychain(); + write_credentials(&path, &v, &synthetic(0)).unwrap(); let held = store(&path).unwrap(); let other = std::fs::OpenOptions::new().read(true).write(true).open(path.with_extension("store-lock")).unwrap(); assert!(other.try_lock().is_err()); let (tx, rx) = std::sync::mpsc::channel(); std::thread::scope(|scope| { scope.spawn(|| { - let r = sign_out_at(&path); + let r = sign_out_at(&path, &v); tx.send(r).unwrap(); }); assert!(rx.recv_timeout(Duration::from_millis(50)).is_err()); drop(held); assert!(rx.recv_timeout(Duration::from_secs(2)).unwrap().is_ok()); }); - assert!(!read_credentials(&path).unwrap().signed_in()); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); assert!(other.try_lock().is_ok()); } @@ -1522,19 +1997,22 @@ mod native { fn independent_process_changes_survive_inflight_renewal_and_login() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + let (v, other) = (sharing(&store), sharing(&store)); for signout in [false, true] { let old = synthetic(0); - write_credentials(&path, &old).unwrap(); - let result = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| { + write_credentials(&path, &v, &old).unwrap(); + let result = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| { std::thread::scope(|scope| { scope .spawn(|| { if signout { - sign_out_at(&path).unwrap(); + sign_out_at(&path, &other).unwrap(); } else { + let current = read_credentials(&path, &other).unwrap(); let mut new = old.clone(); new.subject = "new-account".into(); - save_at(&path, &new, Some(&old)).unwrap(); + save_at(&path, &other, &new, Some(¤t)).unwrap(); } }) .join() @@ -1543,8 +2021,8 @@ mod native { Ok(grant(now() + 100_000, "late-refresh")) }); assert!(result.is_err()); - assert!(save_at(&path, &old, Some(&old)).is_err(), "late login must not undo the change"); - let current = read_credentials(&path).unwrap(); + assert!(save_at(&path, &v, &old, Some(&old)).is_err(), "late login must not undo the change"); + let current = read_credentials(&path, &v).unwrap(); assert_ne!(current.refresh_token, "late-refresh"); assert_eq!(current.signed_in(), !signout); } @@ -1554,21 +2032,22 @@ mod native { fn accounts_migrate_select_and_keep_distinct_registrations() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); + let v = keychain(); let a = synthetic(now() + 100_000); - std::fs::write(&path, a.to_json().to_string()).unwrap(); - assert_eq!(read_credentials(&path).unwrap(), a); + std::fs::write(&path, legacy_json(&a).to_string()).unwrap(); + let read = read_credentials(&path, &v).unwrap(); + assert_eq!(plain(read.clone()), a); let mut b = a.clone(); b.client_id = "oaiapp_workspace".into(); assert_ne!(a.label(), b.label()); - save_at(&path, &b, Some(&a)).unwrap(); - let selected = select_at(&path, &a.client_id, &a.subject).unwrap(); - assert_eq!(selected, a, "selecting changes nothing but the selection"); - assert_eq!(selected.client_id, a.client_id); - sign_out_at(&path).unwrap(); + save_at(&path, &v, &b, Some(&read)).unwrap(); + let selected = select_at(&path, &v, &a.client_id, &a.subject).unwrap(); + assert_eq!(plain(selected), a, "selecting changes nothing but the selection"); + sign_out_at(&path, &v).unwrap(); let accounts = read_accounts(&path).unwrap(); assert_eq!(accounts.entries.len(), 2); - assert!(!accounts.selected().unwrap().signed_in()); - let selected = select_at(&path, &b.client_id, &b.subject).unwrap(); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); + let selected = select_at(&path, &v, &b.client_id, &b.subject).unwrap(); assert!(selected.signed_in()); assert_eq!(selected.refresh_token, b.refresh_token); let json: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); @@ -1620,19 +2099,21 @@ mod native { #[test] fn simultaneous_401_responses_rotate_only_once() { + // Two processes: their own vaults, one keychain. let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); let far = now() + 100_000; - write_credentials(&path, &synthetic(far)).unwrap(); + let store = Arc::new(MemorySecrets::new()); + write_credentials(&path, &sharing(&store), &synthetic(far)).unwrap(); let barrier = Arc::new(std::sync::Barrier::new(2)); let rotations = Arc::new(std::sync::atomic::AtomicUsize::new(0)); std::thread::scope(|scope| { let workers: Vec<_> = (0..2) .map(|_| { - let (barrier, rotations, path) = (barrier.clone(), rotations.clone(), path.clone()); + let (barrier, rotations, path, v) = (barrier.clone(), rotations.clone(), path.clone(), sharing(&store)); scope.spawn(move || { let mut inference = 0; - send_at(&test_request(), &synthetic(0).binding(), &path, &mut |req| { + send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |req| { if req.url == super::super::TOKEN_URL { rotations.fetch_add(1, std::sync::atomic::Ordering::SeqCst); return Ok(grant(far + 1, "synthetic-rotated")); @@ -1656,31 +2137,32 @@ mod native { } }); assert_eq!(rotations.load(std::sync::atomic::Ordering::SeqCst), 1); - assert_eq!(read_credentials(&path).unwrap().refresh_token, "synthetic-rotated"); + assert_eq!(read_credentials(&path, &sharing(&store)).unwrap().refresh_token, "synthetic-rotated"); } #[test] fn late_401_does_not_restore_a_signed_out_or_different_account() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); + let v = keychain(); for switch_account in [false, true] { - write_credentials(&path, &synthetic(now() + 100_000)).unwrap(); + write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); let mut requests = 0; - let result = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| { + let result = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| { requests += 1; let _guard = store(&path).unwrap(); if switch_account { let mut other = synthetic(now() + 100_001); other.subject = "user-2".into(); - write_credentials(&path, &other).unwrap(); + write_credentials(&path, &v, &other).unwrap(); } else { - write_credentials(&path, &synthetic(0).registration()).unwrap(); + write_credentials(&path, &v, &synthetic(0).registration()).unwrap(); } Ok(HttpResponse { status: 401, body: vec![], headers: vec![] }) }); assert!(result.is_err()); assert_eq!(requests, 1, "a late response must not renew or resend"); - assert_eq!(read_credentials(&path).unwrap().signed_in(), switch_account); + assert_eq!(read_credentials(&path, &v).unwrap().signed_in(), switch_account); } } @@ -1688,51 +2170,56 @@ mod native { fn sign_out_during_refresh_does_not_wait_for_network_or_restore_tokens() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); - write_credentials(&path, &synthetic(0)).unwrap(); - let result = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |req| { + let v = keychain(); + write_credentials(&path, &v, &synthetic(0)).unwrap(); + let result = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |req| { assert_eq!(req.url, super::super::TOKEN_URL); // An independent handle can sign out while renewal waits for the network. - sign_out_at(&path).unwrap(); + sign_out_at(&path, &v).unwrap(); Ok(grant(now() + 100_000, "synthetic-rotated")) }); assert!(result.is_err()); - assert!(!read_credentials(&path).unwrap().signed_in()); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); } #[test] fn an_unusable_refresh_token_is_cleared_and_the_registration_kept() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); - write_credentials(&path, &synthetic(0)).unwrap(); - let e = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| { + let v = keychain(); + write_credentials(&path, &v, &synthetic(0)).unwrap(); + let e = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| { Ok(HttpResponse { status: 400, body: br#"{"error":"refresh_token_reused"}"#.to_vec(), headers: vec![] }) }) .unwrap_err(); assert!(e.to_string().contains("sign in again"), "{e}"); - let left = read_credentials(&path).unwrap(); + let left = read_credentials(&path, &v).unwrap(); assert!(!left.signed_in() && left.refresh_token.is_empty()); assert_eq!(left.client_id, "oaiapp_synthetic"); + // The id token stays, to skip choosing the account next time. + assert_eq!(left.id_token, "synthetic-id"); // A passing failure keeps the tokens. - write_credentials(&path, &synthetic(0)).unwrap(); + write_credentials(&path, &v, &synthetic(0)).unwrap(); assert!( - send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| Ok(HttpResponse { + send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| Ok(HttpResponse { status: 503, body: vec![], headers: vec![] })) .is_err() ); - assert!(read_credentials(&path).unwrap().signed_in()); + assert!(read_credentials(&path, &v).unwrap().signed_in()); } #[test] fn without_the_plan_nothing_is_sent() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); + let v = keychain(); let mut c = synthetic(now() + 100_000); c.scopes.retain(|s| s != super::super::PLAN_SCOPE); - write_credentials(&path, &c).unwrap(); - let e = send_at(&test_request(), &synthetic(0).binding(), &path, &mut |_| panic!("nothing may be sent")).unwrap_err(); + write_credentials(&path, &v, &c).unwrap(); + let e = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| panic!("nothing may be sent")).unwrap_err(); assert!(e.to_string().contains("plan"), "{e}"); } @@ -1740,16 +2227,17 @@ mod native { fn sign_out_keeps_the_registration_and_gives_the_session_to_revoke() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); - write_credentials(&path, &synthetic(now() + 100_000)).unwrap(); - let r = sign_out_at(&path).unwrap().unwrap(); + let v = keychain(); + write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); + let r = sign_out_at(&path, &v).unwrap().unwrap(); assert_eq!((r.client_id.as_str(), r.refresh_token.as_str()), ("oaiapp_synthetic", "synthetic-refresh")); assert!(!format!("{r:?}").contains("synthetic-refresh")); - let left = read_credentials(&path).unwrap(); + let left = read_credentials(&path, &v).unwrap(); assert!(!left.signed_in() && left.id_token.is_empty() && left.refresh_token.is_empty()); assert_eq!((left.client_id.as_str(), left.subject.as_str()), ("oaiapp_synthetic", "user-1")); // Again: nothing more to revoke. A missing file is no error. - assert!(sign_out_at(&path).unwrap().is_none()); - assert!(sign_out_at(&dir.path().join("missing.json")).unwrap().is_none()); + assert!(sign_out_at(&path, &v).unwrap().is_none()); + assert!(sign_out_at(&dir.path().join("missing.json"), &v).unwrap().is_none()); } #[test] @@ -1805,9 +2293,9 @@ mod native { fn cancellation_prevents_persistence() { let cancelled = Mutex::new(true); let mut writes = 0; - let result = save_if_active(&cancelled, &Credentials::default(), &mut |_| { + let result = save_if_active(&cancelled, &Credentials::default(), &mut |c| { writes += 1; - Ok(()) + Ok(c.clone()) }); assert!(result.is_err()); assert_eq!(writes, 0); @@ -1819,11 +2307,12 @@ mod native { fn credentials_file_replaces_atomically_and_cleans_failed_writes() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); + let v = keychain(); let first = synthetic(1); let next = synthetic(2); - write_credentials(&path, &first).unwrap(); - write_credentials(&path, &next).unwrap(); - assert_eq!(read_credentials(&path).unwrap(), next); + write_credentials(&path, &v, &first).unwrap(); + write_credentials(&path, &v, &next).unwrap(); + assert_eq!(plain(read_credentials(&path, &v).unwrap()), next); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -1831,12 +2320,12 @@ mod native { } let destination_dir = dir.path().join("directory"); std::fs::create_dir(&destination_dir).unwrap(); - assert!(write_credentials(&destination_dir, &next).is_err()); - assert_eq!(read_credentials(&path).unwrap(), next); - assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 2, "failed writes leave no token-bearing tempfiles"); + assert!(write_credentials(&destination_dir, &v, &next).is_err()); + assert_eq!(plain(read_credentials(&path, &v).unwrap()), next); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 2, "failed writes leave no tempfiles"); // A byte-order mark (Windows editors) is no reason to lose the sign-in. - std::fs::write(&path, format!("\u{feff}{}", next.to_json())).unwrap(); - assert_eq!(read_credentials(&path).unwrap(), next); + std::fs::write(&path, format!("\u{feff}{}", legacy_json(&next))).unwrap(); + assert_eq!(plain(read_credentials(&path, &v).unwrap()), next); } } } diff --git a/crates/assistant/src/tests.rs b/crates/assistant/src/tests.rs index 94b7df269..efe107a8e 100644 --- a/crates/assistant/src/tests.rs +++ b/crates/assistant/src/tests.rs @@ -1272,6 +1272,8 @@ fn chatgpt_credentials(expires_at: i64) -> chatgpt::Credentials { plan_declined: false, saved_at: "2026-10-14T00:00:00Z".into(), session: "session-1".into(), + rev: String::new(), + kept: chatgpt::Kept::Nowhere, } } @@ -1478,8 +1480,13 @@ fn responses_models_from_the_catalog() { fn chatgpt_credentials_file_shape() { let c = chatgpt_credentials(1_800_000_000); assert!(c.signed_in() && c.plan_allowed()); - assert_eq!(chatgpt::Credentials::from_json(&c.to_json()).unwrap(), c); - assert_eq!(c.to_json()["issuer"], chatgpt::ISSUER); + // The file keeps the registration and never a token. + let saved = c.to_json(); + for token in [&c.id_token, &c.access_token, &c.refresh_token] { + assert!(!saved.to_string().contains(token.as_str())); + } + assert_eq!(chatgpt::Credentials::from_json(&saved).unwrap(), c.without_tokens()); + assert_eq!(saved["issuer"], chatgpt::ISSUER); assert!(!c.expires_soon(1_800_000_000 - 3600)); assert!(c.expires_soon(1_800_000_000 - 60)); assert!(chatgpt::Credentials { expires_at: 0, ..c.clone() }.expires_soon(0), "an unknown expiry means renew"); @@ -1633,7 +1640,7 @@ fn chatgpt_token_requests_and_replies() { let g = chatgpt::parse_grant(&resp(json!({"access_token": "a2", "refresh_token": "r2", "id_token": "i2", "expires_in": 3600, "scope": "openid email chatgpt.tokens.use.direct"}))) .unwrap(); - let account = chatgpt::Account { email: "ann@example.com".into(), subject: "user-1".into() }; + let account = chatgpt::Account { email: "ann@example.com".into(), subject: "user-1".into(), ..Default::default() }; let signed = chatgpt::signed_in("oaiapp_synthetic", &account, &g, 1_000); assert_eq!((signed.expires_at, signed.plan_allowed(), signed.plan_declined), (4_600, true, false)); assert_eq!(signed.saved_at, "1970-01-01T00:16:40Z"); diff --git a/crates/ui-egui/src/assistantpane.rs b/crates/ui-egui/src/assistantpane.rs index f1858011d..f3a8a8005 100644 --- a/crates/ui-egui/src/assistantpane.rs +++ b/crates/ui-egui/src/assistantpane.rs @@ -942,9 +942,7 @@ fn provider_settings(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Co let preset = provider::preset(&cfg.provider); let mut notes = Vec::new(); if cfg.login() { - notes.push( - tl!("The sign-in is kept in the app's settings folder, for your user account only, and never in a project file.").to_string(), - ); + notes.push(sign_in_storage_note(app.assistant.account.as_ref().filter(|_| !cfg.key.is_empty()))); notes.push(tl!("Requests go to the OpenAI API and use your ChatGPT plan.").to_string()); } else { ui.checkbox(&mut app.assistant.remember_key, tl!("Remember the key on this computer")); @@ -973,6 +971,20 @@ fn provider_settings(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Co } } +/// Where the ChatGPT sign-in is kept, as truthfully as is known. +fn sign_in_storage_note(account: Option<&chatgpt::Account>) -> String { + match account.map(|a| a.kept) { + Some(chatgpt::Kept::SettingsFile) => tl!( + "The sign-in is still kept in the app's settings folder, readable by your user account: the system keychain is unavailable, so it hasn't been moved there yet." + ), + Some(chatgpt::Kept::Session) => tl!( + "The system keychain is unavailable, so this sign-in won't be remembered: it lasts until RustGIS closes." + ), + _ => tl!("The sign-in is kept in the system keychain, never in the app's settings or a project file."), + } + .to_string() +} + /// Describes actual storage, including a legacy key retained after a failed migration. fn key_storage_note(app: &RustGisApp, provider: &str) -> String { #[cfg(target_arch = "wasm32")] @@ -1072,38 +1084,37 @@ fn account_row(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Config) start_login(app, cfg, true); return; } + let selected = active.map(|c| (c.client_id.clone(), c.subject.clone())); + if let Some((client_id, subject)) = selected + && ui + .small_button(tl!("Remove account")) + .on_hover_text(tl!("Signs this account out and forgets it on this computer, with its saved sign-in.")) + .clicked() + { + match chatgpt::remove(&client_id, &subject) { + Ok(revocation) => { + revoke_later(app, revocation, ui.ctx()); + forget_sign_in(app, cfg); + cfg.client_id.clear(); + cfg.account.clear(); + cfg.session.clear(); + } + Err(e) => app.assistant.login_error = Some(e.auth_context()), + } + return; + } if !cfg.key.is_empty() { let who = app.assistant.account.as_ref().map(|a| a.email.clone()).filter(|e| !e.is_empty()); ui.label(RichText::new(who.unwrap_or_else(|| tl!("signed in").to_string())).small()); if ui.small_button(tl!("Sign out")).clicked() { match chatgpt::sign_out_selected(&cfg.binding()) { - Ok(revocation) => { - // Revoking the session at OpenAI is network I/O: off the frame. - if let Some(r) = revocation { - let slot: Slot> = Slot::default(); - let (out, ctx) = (slot.clone(), ui.ctx().clone()); - std::thread::spawn(move || { - let result = chatgpt::revoke(&r); - if let Ok(mut s) = out.lock() { - *s = Some(result); - } - ctx.request_repaint(); - }); - app.assistant.signout = Some(slot); - } - } + Ok(revocation) => revoke_later(app, revocation, ui.ctx()), Err(e) => { app.assistant.login_error = Some(e.auth_context()); return; } } - app.assistant.models_slot = None; - cfg.key.clear(); - app.assistant.account = None; - app.assistant.models.clear(); - app.assistant.catalog.clear(); - app.assistant.welcome = false; - app.assistant.accounts = None; + forget_sign_in(app, cfg); } if app.assistant.duplicate { ui.label( @@ -1119,6 +1130,34 @@ fn account_row(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Config) } } +/// Revokes a signed-out session at OpenAI: network I/O, off the frame. +#[cfg(not(target_arch = "wasm32"))] +fn revoke_later(app: &mut RustGisApp, revocation: Option, ctx: &egui::Context) { + let Some(r) = revocation else { return }; + let slot: Slot> = Slot::default(); + let (out, ctx) = (slot.clone(), ctx.clone()); + std::thread::spawn(move || { + let result = chatgpt::revoke(&r); + if let Ok(mut s) = out.lock() { + *s = Some(result); + } + ctx.request_repaint(); + }); + app.assistant.signout = Some(slot); +} + +/// After signing out or removing the account: no tokens, no catalog. +#[cfg(not(target_arch = "wasm32"))] +fn forget_sign_in(app: &mut RustGisApp, cfg: &mut Config) { + app.assistant.models_slot = None; + cfg.key.clear(); + app.assistant.account = None; + app.assistant.models.clear(); + app.assistant.catalog.clear(); + app.assistant.welcome = false; + app.assistant.accounts = None; +} + #[cfg(not(target_arch = "wasm32"))] fn waiting_for_browser(ui: &mut Ui, t: &Tokens, url: &str) -> (bool, bool) { ui.spinner(); @@ -2284,10 +2323,17 @@ mod tests { let b = chatgpt::Credentials { client_id: "test-workspace".into(), ..a.clone() }; let path = chatgpt::auth_path().unwrap(); std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + // As versions before the keychain saved them, with their tokens; this run has no keychain, + // so they stay there. + let saved = |c: &chatgpt::Credentials| { + let mut v = c.to_json(); + v["access_token"] = c.access_token.clone().into(); + v + }; std::fs::write( &path, serde_json::json!({"version":2, "active":{"client_id":a.client_id,"subject":a.subject}, - "accounts":[a.to_json(),b.to_json()]}) + "accounts":[saved(&a),saved(&b)]}) .to_string(), ) .unwrap(); diff --git a/docs/architecture.md b/docs/architecture.md index 5e5ca5809..dc66d8c8d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -74,6 +74,13 @@ browser and the CLI, where callers keep a secret for the session only, as before the requested bucket's explicit-grant status. Copy only that login and grant boolean, not the grant list; parse and validate addresses and prepare signatures outside the lock. Keychain calls and network I/O never run under the registry lock. +- **ChatGPT sign-in.** The desktop app gives its store to `chatgpt::set_secret_store` at start; + each registration's tokens are one secret, `chatgpt//`, carrying the `rev` + its entry in `chatgpt-auth.json` names (any other is out of date and never used). The file's + `store-lock` covers every keychain read and write too, so a rotating refresh token is never used + twice. Keychain tokens are read again for each use; only those it couldn't keep stay in memory, + for the session. Tokens from an earlier version's file leave it only after the keychain has + taken them. - **macOS.** The system asks again for access when the app's code signature changes, which happens with unsigned or re-signed builds. - **Tests.** Use `MemorySecrets`, and `MemorySecrets::failing(..)` for error paths. The desktop diff --git a/docs/cli.md b/docs/cli.md index c05b09e20..da475eeec 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -109,7 +109,10 @@ models and replacing the project are always declined here. `--trace log.txt` app and response body (never the headers, which carry the key) to a file; the bodies can contain your data and prompts. `--login` prints a browser sign-in URL so `--provider chatgpt` uses your ChatGPT plan. `--another-account` starts a new registration, `--accounts` lists saved registrations, -`--account ` selects one, and `--logout` signs out; see +`--account ` selects one, `--remove-account ` signs it out and forgets it, +and `--logout` signs out. The command line has no system keychain: it uses a sign-in an earlier +version saved in the settings folder, but not one the desktop app keeps in the keychain, and its +own sign-ins last for the command only. See [Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). ## Serving AI agents diff --git a/docs/user-guide/assistant.md b/docs/user-guide/assistant.md index b2e93b720..1cf3cb5bf 100644 --- a/docs/user-guide/assistant.md +++ b/docs/user-guide/assistant.md @@ -68,7 +68,8 @@ what RustGIS uses or disconnect it. **Sign out** clears local tokens and attempt remote session. If OpenAI does not confirm revocation, RustGIS says so; disconnect RustGIS in ChatGPT settings to ensure that remote access ends. The registration remains for signing in again. The account picker retains separate registrations, even when their emails match. **Use another -account** adds a registration; choose an existing one to reuse its issued client ID. From the command line: +account** adds a registration; choose an existing one to reuse its issued client ID. **Remove +account** signs the selected one out and forgets it. From the command line: ```sh rustgis-cli ask --login # continue with ChatGPT in the browser @@ -77,6 +78,7 @@ rustgis-cli ask --sample --provider chatgpt "Zoom to Tokyo" rustgis-cli ask --accounts # list saved registrations rustgis-cli ask --account oaiapp_... # select one by its issued client id rustgis-cli ask --login --account oaiapp_... # reauthorize that registration +rustgis-cli ask --remove-account oaiapp_... # sign out and forget it rustgis-cli ask --logout ``` @@ -285,11 +287,20 @@ is cleared only after the keychain key is read or the old key is saved successfu operation fails, the old preference is retained and the unencrypted-storage warning stays visible. In the web app, **Remember** still saves the key unencrypted in the browser's storage, with a note in settings. -A ChatGPT sign-in is kept in `chatgpt-auth.json` in the app's settings folder (owner-only on -macOS and Linux; on Windows, protected like the rest of your user profile), never in the -preferences or a project file, and its tokens are never logged; RustGIS renews them as they expire. -The ChatGPT provider's address is fixed, so only the OpenAI API receives them. Renewals are -serialized across RustGIS processes (a refresh token can be used only once). +A ChatGPT sign-in's tokens are kept in the system keychain, never in the preferences or a project +file, and never logged; RustGIS renews them as they expire. `chatgpt-auth.json` in the app's +settings folder keeps only the saved accounts (their client id, account, email, permissions, when +the tokens expire, and which one is selected), no token. A sign-in from an earlier version, whose +tokens are in that file, keeps working: they move to the keychain the first time they are used, +and leave the file only once the keychain has taken them. If the keychain is unavailable, they +stay in the file until it is, and the settings say so. A new sign-in the keychain can't take lasts +until RustGIS closes and is never written to the file; the settings say it won't be remembered. +**Sign out** revokes the session at OpenAI and deletes its tokens; **Remove account** also forgets +the account. The command line has no keychain (it uses a sign-in from an earlier version still in +the file), so its sign-ins last for the command only and it can't sign out an account whose tokens +are in the keychain. The ChatGPT provider's address is fixed, so only the OpenAI API receives the +tokens. Renewals are serialized across RustGIS processes (a refresh token can be used only once), +and so are the keychain's reads and writes. Plain HTTP is only used for servers on your computer or local network. In the web app, the browser calls the provider directly. Anthropic, OpenAI, Gemini and From 5c6366fe0258814bd59f0f12755b0b84e0bb7d80 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 10:20:46 +0100 Subject: [PATCH 2/7] docs(cli): --logout and --remove-account refuse keychain sign-ins --- docs/cli.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index da475eeec..0f918c3c7 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -112,8 +112,9 @@ plan. `--another-account` starts a new registration, `--accounts` lists saved re `--account ` selects one, `--remove-account ` signs it out and forgets it, and `--logout` signs out. The command line has no system keychain: it uses a sign-in an earlier version saved in the settings folder, but not one the desktop app keeps in the keychain, and its -own sign-ins last for the command only. See -[Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). +own sign-ins last for the command only. `--logout` and `--remove-account` refuse an account whose +tokens are in the keychain (sign out or remove it in the app), so they are never left there +unrevoked. See [Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). ## Serving AI agents From b8f4f77bb51f44e0d58943cb4bd19686de34329d Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 10:40:50 +0100 Subject: [PATCH 3/7] Assistant: read the ChatGPT sign-in off the UI thread --- apps/rustgis-cli/src/main.rs | 5 ++ crates/assistant/src/provider.rs | 21 +---- crates/ui-egui/src/assistantpane.rs | 126 +++++++++++++++++++--------- docs/architecture.md | 4 +- 4 files changed, 98 insertions(+), 58 deletions(-) diff --git a/apps/rustgis-cli/src/main.rs b/apps/rustgis-cli/src/main.rs index 67b5d842e..51f075422 100644 --- a/apps/rustgis-cli/src/main.rs +++ b/apps/rustgis-cli/src/main.rs @@ -256,6 +256,11 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { return Ok(()); } let mut cfg = Config::from_env(&provider); + if cfg.login() + && let Some(c) = rustgis_assistant::chatgpt::load_any() + { + cfg.sign_in(&c); + } if let Some(m) = opt(args, "--model").map(str::to_string).or_else(|| std::env::var("RUSTGIS_ASSISTANT_MODEL").ok()) { cfg.model = m; } diff --git a/crates/assistant/src/provider.rs b/crates/assistant/src/provider.rs index 3e61a1e1c..a91c10e76 100644 --- a/crates/assistant/src/provider.rs +++ b/crates/assistant/src/provider.rs @@ -165,8 +165,9 @@ impl std::fmt::Debug for Config { } impl Config { - /// The preset's defaults, with a key and base URL from the environment where set (for a - /// sign-in provider, the account signed in on this computer). + /// The preset's defaults, with a key and base URL from the environment where set. A sign-in + /// provider's account is not read here: its tokens are in the system keychain, which may block + /// (`chatgpt::load_any`, then [`Config::sign_in`]). pub fn from_env(provider: &str) -> Config { let p = preset(provider).unwrap_or(&PRESETS[0]); let env = |name: &str| std::env::var(name).ok().map(|v| v.trim().to_string()).filter(|v| !v.is_empty()); @@ -176,7 +177,7 @@ impl Config { if p.id == "ollama" && !base_url.trim_end_matches('/').ends_with("/v1") { base_url = format!("{}/v1", base_url.trim_end_matches('/')); } - let cfg = Config { + Config { provider: p.id.to_string(), base_url, model: p.model.to_string(), @@ -189,20 +190,6 @@ impl Config { .and_then(|s| s.parse().ok()) .filter(|n| *n > 0) .unwrap_or(crate::tools::DEFAULT_OUTPUT_TOKENS), - }; - #[cfg(not(target_arch = "wasm32"))] - { - let mut cfg = cfg; - if p.login - && let Some(t) = crate::chatgpt::load_any() - { - cfg.sign_in(&t); - } - cfg - } - #[cfg(target_arch = "wasm32")] - { - cfg } } diff --git a/crates/ui-egui/src/assistantpane.rs b/crates/ui-egui/src/assistantpane.rs index f3a8a8005..72d52633e 100644 --- a/crates/ui-egui/src/assistantpane.rs +++ b/crates/ui-egui/src/assistantpane.rs @@ -72,6 +72,10 @@ pub struct AssistantState { /// A sign-out's revocation at OpenAI, under way (desktop only). #[cfg(not(target_arch = "wasm32"))] signout: Option>>, + /// The ChatGPT sign-in, being read on a worker: its tokens are in the system keychain, which + /// may block or ask (desktop only). + #[cfg(not(target_arch = "wasm32"))] + sign_in_read: Option>>, /// The first sign-in's confirmation that the ChatGPT plan is in use, until dismissed. welcome: bool, /// A failed request to send again: its generation and when. @@ -175,7 +179,11 @@ fn ensure_loaded(app: &mut RustGisApp, ctx: &egui::Context) { cfg.model = m.trim().to_string(); } #[cfg(not(target_arch = "wasm32"))] - let loading = app.assistant.keys.loading(&provider); + if cfg.login() { + read_sign_in(app, ctx); + } + #[cfg(not(target_arch = "wasm32"))] + let loading = reading(app, &provider); #[cfg(target_arch = "wasm32")] let loading = false; if !loading { @@ -185,10 +193,6 @@ fn ensure_loaded(app: &mut RustGisApp, ctx: &egui::Context) { if loading { app.assistant.settings_pending = Some(provider); } - if cfg.login() && !cfg.key.is_empty() { - app.assistant.account = signed_in_account(); - } - app.assistant.catalog_pending = cfg.login() && !cfg.key.is_empty(); let mut agent = Agent::new(cfg, &app.session); app.assistant.local_agents = pref(app, "assistant_local_agents").as_deref() == Some("on"); #[cfg(not(target_arch = "wasm32"))] @@ -256,18 +260,56 @@ fn sync_skills(app: &mut RustGisApp, force: bool) { } } -/// The ChatGPT account signed in on this computer, as its verified id token named it. -fn signed_in_account() -> Option { - #[cfg(not(target_arch = "wasm32"))] - { - chatgpt::account() - } - #[cfg(target_arch = "wasm32")] - { - None +/// Starts reading the selected ChatGPT sign-in on a worker; [`poll_sign_in`] applies it. +#[cfg(not(target_arch = "wasm32"))] +fn read_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { + let slot: Slot> = Slot::default(); + let (out, ctx) = (slot.clone(), ctx.clone()); + std::thread::spawn(move || { + let c = chatgpt::load_any(); + if let Ok(mut s) = out.lock() { + *s = Some(c); + } + ctx.request_repaint(); + }); + app.assistant.sign_in_read = Some(slot); +} + +/// Applies a finished sign-in read to the sign-in provider, if it's still the one chosen: a new +/// sign-in loads its catalog; one that's gone (expired, revoked, signed out elsewhere) clears the +/// key and opens the settings to sign in again. +#[cfg(not(target_arch = "wasm32"))] +fn poll_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { + let Some(read) = app.assistant.sign_in_read.as_ref().and_then(|s| s.lock().ok().and_then(|mut s| s.take())) else { return }; + app.assistant.sign_in_read = None; + let Some(agent) = app.assistant.agent.as_mut().filter(|a| a.config.login()) else { return }; + let had = !agent.config.key.is_empty(); + match &read { + Some(c) => agent.config.sign_in(c), + None => agent.config.key.clear(), + } + let cfg = agent.config.clone(); + if !cfg.key.is_empty() { + app.assistant.account = read.map(|c| c.account()); + if !had { + list_models(app, &cfg, ctx); + } + } else { + app.assistant.account = None; + if had { + app.assistant.welcome = false; + app.assistant.settings = true; + app.assistant.accounts = None; + } } } +/// Whether the provider's key or sign-in is still being read (off the frame). +#[cfg(not(target_arch = "wasm32"))] +fn reading(app: &RustGisApp, provider: &str) -> bool { + app.assistant.keys.loading(provider) || (provider::preset(provider).is_some_and(|p| p.login) && app.assistant.sign_in_read.is_some()) +} + /// Persists ordinary settings; desktop secrets are queued for the keychain worker. fn save_config(app: &mut RustGisApp, c: &Config, ctx: &egui::Context) { save_pref(app, "assistant_provider", &c.provider); @@ -323,7 +365,7 @@ fn poll_keys(app: &mut RustGisApp, ctx: &egui::Context) { agent.config.key = key.to_string(); } if let Some(provider) = app.assistant.settings_pending.as_deref() - && !app.assistant.keys.loading(provider) + && !reading(app, provider) { let cfg = app.assistant.agent.as_ref().filter(|a| a.config.provider == provider).map(|a| a.config.clone()); app.assistant.settings_pending = None; @@ -446,22 +488,14 @@ fn signed_in(app: &mut RustGisApp, c: &chatgpt::Credentials, ctx: &egui::Context } /// After a failed request: when the sign-in on this computer no longer has tokens (they expired -/// or were revoked), the settings say so and offer to sign in again. -fn sync_sign_in(app: &mut RustGisApp) { +/// or were revoked), the settings say so and offer to sign in again (read off the frame). +fn sync_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { #[cfg(not(target_arch = "wasm32"))] - if let Some(a) = app.assistant.agent.as_mut() - && a.config.login() - && !a.config.key.is_empty() - && chatgpt::load().is_none() - { - a.config.key.clear(); - app.assistant.account = None; - app.assistant.welcome = false; - app.assistant.settings = true; - app.assistant.accounts = None; + if app.assistant.agent.as_ref().is_some_and(|a| a.config.login() && !a.config.key.is_empty()) { + read_sign_in(app, ctx); } #[cfg(target_arch = "wasm32")] - let _ = app; + let _ = (app, ctx); } fn take(slot: &Reply) -> Option> { @@ -515,6 +549,8 @@ fn with_agent(app: &mut RustGisApp, f: impl FnOnce(&mut Agent, &mut rustgis_engi pub fn poll(app: &mut RustGisApp, ctx: &egui::Context) { #[cfg(not(target_arch = "wasm32"))] poll_keys(app, ctx); + #[cfg(not(target_arch = "wasm32"))] + poll_sign_in(app, ctx); if app.assistant.catalog_pending && let Some(cfg) = app.assistant.agent.as_ref().map(|a| a.config.clone()) { @@ -556,7 +592,7 @@ pub fn poll(app: &mut RustGisApp, ctx: &egui::Context) { follow(app, step, ctx); } if failed { - sync_sign_in(app); + sync_sign_in(app, ctx); } } // Agent threads on this computer: what they did, asked and reported. @@ -609,7 +645,7 @@ pub fn poll(app: &mut RustGisApp, ctx: &egui::Context) { } Err(e) => { app.assistant.models_error = Some(e); - sync_sign_in(app); + sync_sign_in(app, ctx); } } } @@ -656,7 +692,7 @@ fn send(app: &mut RustGisApp, text: String, ctx: &egui::Context) { fn key_loading(app: &RustGisApp) -> bool { #[cfg(not(target_arch = "wasm32"))] { - app.assistant.agent.as_ref().is_some_and(|a| app.assistant.keys.loading(&a.config.provider)) + app.assistant.agent.as_ref().is_some_and(|a| reading(app, &a.config.provider)) } #[cfg(target_arch = "wasm32")] { @@ -765,6 +801,8 @@ fn composer_panel(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, busy: bool, ctx /// Edits provider settings and invalidates pending key reads as soon as a key changes. fn settings(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { + #[cfg(not(target_arch = "wasm32"))] + poll_sign_in(app, ui.ctx()); let Some(mut cfg) = app.assistant.agent.as_ref().map(|a| a.config.clone()) else { return }; let before = (cfg.clone(), app.assistant.remember_key); let open = !app.assistant.provider_closed; @@ -850,10 +888,9 @@ fn provider_settings(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Co app.assistant.catalog.clear(); app.assistant.models_error = None; app.assistant.login_error = None; - if cfg.login() && !cfg.key.is_empty() { - app.assistant.account = signed_in_account(); - let ctx = ui.ctx().clone(); - list_models(app, cfg, &ctx); + #[cfg(not(target_arch = "wasm32"))] + if cfg.login() { + read_sign_in(app, ui.ctx()); } } } @@ -2340,10 +2377,15 @@ mod tests { let mut app = RustGisApp::with_sample(); app.services.load_pref = Some(Box::new(|key| (key == "assistant_provider").then(|| "chatgpt".into()))); ensure_loaded(&mut app, &egui::Context::default()); - assert!(app.assistant.catalog_pending); + // The sign-in is read on a worker; the catalog request follows once it arrives. let ctx = egui::Context::default(); - poll(&mut app, &ctx); - assert!(app.assistant.models_slot.is_some() || app.assistant.models_error.is_some()); + let end = std::time::Instant::now() + std::time::Duration::from_secs(3); + while app.assistant.models_slot.is_none() && app.assistant.models_error.is_none() { + assert!(std::time::Instant::now() < end, "the sign-in read never finished"); + std::thread::sleep(std::time::Duration::from_millis(1)); + poll(&mut app, &ctx); + } + assert_eq!(app.assistant.account.as_ref().map(|a| a.kept), Some(chatgpt::Kept::SettingsFile)); app.run("ui.pane.assistant", serde_json::Value::Null).unwrap(); app.assistant.settings = true; // Draw settings without polling, so each click's catalog request remains observable. @@ -2372,8 +2414,12 @@ mod tests { h.get_by_value("Ollama (local)").click(); h.run_steps(4); h.get_by_label("ChatGPT (sign in)").click(); - h.run_steps(4); - assert!(h.state().assistant.models_slot.is_some()); + let end = std::time::Instant::now() + std::time::Duration::from_secs(3); + while h.state().assistant.models_slot.is_none() { + assert!(std::time::Instant::now() < end, "the sign-in read never finished"); + std::thread::sleep(std::time::Duration::from_millis(1)); + h.run_steps(1); + } h.state_mut().assistant.models_slot = None; h.get_by_value(&a.label()).click(); h.run_steps(4); diff --git a/docs/architecture.md b/docs/architecture.md index dc66d8c8d..b82bec098 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -80,7 +80,9 @@ browser and the CLI, where callers keep a secret for the session only, as before `store-lock` covers every keychain read and write too, so a rotating refresh token is never used twice. Keychain tokens are read again for each use; only those it couldn't keep stay in memory, for the session. Tokens from an earlier version's file leave it only after the keychain has - taken them. + taken them. `Config::from_env` never reads the sign-in: the Assistant reads it on a worker + (startup, switching to the provider, after a failed request) and requests wait for it; only + selecting, signing out and removing an account, user actions, read the keychain on the frame. - **macOS.** The system asks again for access when the app's code signature changes, which happens with unsigned or re-signed builds. - **Tests.** Use `MemorySecrets`, and `MemorySecrets::failing(..)` for error paths. The desktop From 7d7d46e36d961dcb46784b8b86368d88ae41fc2c Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 11:19:04 +0100 Subject: [PATCH 4/7] Assistant: keep only the ChatGPT refresh token in the keychain; fix review findings --- apps/rustgis-cli/src/main.rs | 9 +- crates/assistant/src/chatgpt.rs | 561 +++++++++++++++++++--------- crates/assistant/src/provider.rs | 5 +- crates/assistant/src/tests.rs | 4 +- crates/ui-egui/src/assistantpane.rs | 151 ++++++-- docs/architecture.md | 24 +- docs/cli.md | 7 +- docs/user-guide/assistant.md | 33 +- 8 files changed, 554 insertions(+), 240 deletions(-) diff --git a/apps/rustgis-cli/src/main.rs b/apps/rustgis-cli/src/main.rs index 51f075422..6910dd779 100644 --- a/apps/rustgis-cli/src/main.rs +++ b/apps/rustgis-cli/src/main.rs @@ -269,7 +269,14 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { } if args.iter().any(|a| a == "--logout") { use rustgis_assistant::chatgpt; - if let Some(r) = chatgpt::sign_out()? + let out = chatgpt::sign_out()?; + if let Some(e) = &out.unrevoked { + eprintln!( + "signed out on this computer, but OpenAI wasn't asked to end the session ({e}); disconnect RustGIS in ChatGPT's settings if needed" + ); + return Ok(()); + } + if let Some(r) = out.revocation && let Err(e) = chatgpt::revoke(&r) { eprintln!( diff --git a/crates/assistant/src/chatgpt.rs b/crates/assistant/src/chatgpt.rs index 56a2620e0..5f76bb32c 100644 --- a/crates/assistant/src/chatgpt.rs +++ b/crates/assistant/src/chatgpt.rs @@ -48,6 +48,9 @@ pub const USAGE_URL: &str = "https://chatgpt.com/settings/usage?tab=overview"; pub const REFRESH_MARGIN: i64 = 5 * 60; /// The clock skew allowed when checking an id token's expiry. pub const CLOCK_SKEW: i64 = 5 * 60; +/// A signed-in configuration's key: a marker, not a credential (`send` authorizes each request +/// with the current access token). +pub const SIGNED_IN: &str = "chatgpt-sign-in"; /// Where a sign-in's tokens are kept. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -55,7 +58,8 @@ pub enum Kept { /// There are none. #[default] Nowhere, - /// In the system keychain. + /// In the system keychain: the refresh token is there; the access and id tokens stay with + /// the process that got them (a process without one renews first). Keychain, /// Still in the settings folder's file, from before the keychain: they move to the keychain /// the next time it's available. @@ -90,9 +94,12 @@ pub struct Credentials { /// Changes with each sign-in and sign-out (not renewal or selection): work begun before /// either is refused after it. pub session: String, - /// Changes with every change of the tokens; the tokens in the keychain must carry the same, - /// or they are out of date (empty when there are none to look for). + /// Names the refresh token in the keychain: the keychain's must carry the same, or it is out + /// of date. Empty when the keychain holds no current one for this registration. pub rev: String, + /// The keychain may hold an item for this registration (set before each keychain write, + /// cleared once one is deleted), so removing it must delete that item first. + pub in_keychain: bool, /// Where the tokens are kept (never saved). pub kept: Kept, } @@ -135,9 +142,10 @@ impl Credentials { Binding { client_id: self.client_id.clone(), subject: self.subject.clone(), session: self.session.clone() } } - /// Whether there are tokens to send. + /// Whether there are tokens to use: an access token, or a refresh token here or in the + /// keychain to get one. pub fn signed_in(&self) -> bool { - !self.access_token.is_empty() + !self.access_token.is_empty() || !self.refresh_token.is_empty() || self.kept == Kept::Keychain } /// Whether the grant lets RustGIS use the plan. @@ -153,6 +161,7 @@ impl Credentials { email: self.email.clone(), plan_declined: self.plan_declined, session: self.session.clone(), + in_keychain: self.in_keychain, ..Default::default() } } @@ -168,9 +177,9 @@ impl Credentials { Account { email: self.email.clone(), subject: self.subject.clone(), kept: self.kept } } - /// Whether there are any tokens (an id token alone counts). + /// Whether there are tokens worth keeping (an access or refresh token). pub fn has_tokens(&self) -> bool { - !(self.access_token.is_empty() && self.refresh_token.is_empty() && self.id_token.is_empty()) + !(self.access_token.is_empty() && self.refresh_token.is_empty()) } /// The registration without its tokens. @@ -200,11 +209,13 @@ impl Credentials { saved_at: field("saved_at"), session: field("session"), rev: field("rev"), + in_keychain: v.get("keychain").and_then(Value::as_bool).unwrap_or(false), kept: Kept::Nowhere, }; if c.client_id.is_empty() || c.client_id == REGISTER { return Err("the sign-in has no registration".into()); } + // Older versions kept the tokens in the file. if c.has_tokens() { c.kept = Kept::SettingsFile; } @@ -225,6 +236,7 @@ impl Credentials { "saved_at": self.saved_at, "session": self.session, "rev": self.rev, + "keychain": self.in_keychain, }) } @@ -379,6 +391,7 @@ pub fn signed_in(client_id: &str, account: &Account, g: &Grant, now: i64) -> Cre saved_at: rfc3339(now), session: String::new(), rev: String::new(), + in_keychain: false, kept: Kept::Nowhere, } } @@ -693,35 +706,52 @@ mod native { Ok(file) } - // The store lock covers every read, compare and write of the file and of the keychain, so - // two processes never both use a refresh token that rotates. + // The store lock covers every read, compare and write of the file and of the keychain. Only + // a renewal, a sign-out and a removal read the keychain, under the store lock and (for a + // renewal) the renewal lock, so two processes never both use a refresh token that rotates. fn store(path: &Path) -> Result { file_lock(path, "store-lock") } - /// The system keychain, and the tokens it couldn't keep, by account: they last for this - /// session (this process), while the file's entry has the same `rev`. Tokens the keychain - /// keeps are read from it each time, under the store lock: another process may have rotated - /// them. + // Where a registration's tokens are, from its file entry (`rev`, `keychain`) and this + // process's `held` tokens: + // + // | file entry | refresh token | access and id tokens | Kept | + // |--------------------------------|------------------------|-----------------------|--------------| + // | tokens in it (older version) | the file | the file | SettingsFile | + // | `rev` set | keychain, same `rev` | held (same session) | Keychain | + // | no `rev`, held for the session | held | held | Session | + // | no `rev`, nothing held | none: signed out | none | Nowhere | + // + // `keychain` says an item may exist (set before the first write, cleared after a delete), so + // removal deletes it even when `rev` no longer names it. + + /// The system keychain, and the tokens this process holds by account (see the table above). pub(crate) struct Vault { store: Mutex>>, held: Mutex>, } + /// Tokens this process holds for one sign-in session. struct Held { - rev: String, + session: String, + kept: Kept, id_token: String, access_token: String, + /// Only for [`Kept::Session`]: the keychain keeps the others'. refresh_token: String, + expires_at: i64, } impl Held { fn of(c: &Credentials) -> Held { Held { - rev: c.rev.clone(), + session: c.session.clone(), + kept: c.kept, id_token: c.id_token.clone(), access_token: c.access_token.clone(), - refresh_token: c.refresh_token.clone(), + refresh_token: if c.kept == Kept::Session { c.refresh_token.clone() } else { String::new() }, + expires_at: c.expires_at, } } @@ -730,7 +760,8 @@ mod native { id_token: self.id_token.clone(), access_token: self.access_token.clone(), refresh_token: self.refresh_token.clone(), - kept: Kept::Session, + expires_at: self.expires_at, + kept: self.kept, ..entry.clone() } } @@ -756,14 +787,18 @@ mod native { &VAULT } - /// Gives the system keychain that keeps the sign-in's tokens (the desktop app's). Without one - /// (the command line), a sign-in from before the keychain keeps working from the settings - /// folder, and a new one lasts for the session only. + /// Gives the system keychain that keeps the sign-in's refresh tokens (the desktop app's). + /// Without one (the command line), a sign-in from before the keychain keeps working from the + /// settings folder, and a new one lasts for the session only. pub fn set_secret_store(store: Option>) { *vault().store.lock().unwrap_or_else(|e| e.into_inner()) = store; } - /// The keychain account of a registration's tokens. + /// Why a keychain sign-in can't be used, signed out or removed here. + const KEYCHAIN_ELSEWHERE: &str = + "this ChatGPT sign-in is kept in the system keychain, which isn't available here; use, sign out or remove it in the RustGIS app"; + + /// The keychain account of a registration's refresh token. fn secret_name(client_id: &str, subject: &str) -> String { format!("chatgpt/{client_id}/{subject}") } @@ -772,11 +807,10 @@ mod native { (c.client_id.clone(), c.subject.clone()) } + /// The keychain's value: the refresh token and the `rev` it is for. Kept small: Windows + /// Credential Manager takes at most 2560 bytes of UTF-16. fn secret(c: &Credentials) -> Secret { - Secret::new( - serde_json::json!({"rev": c.rev, "id_token": c.id_token, "access_token": c.access_token, "refresh_token": c.refresh_token}) - .to_string(), - ) + Secret::new(serde_json::json!({"rev": c.rev, "refresh_token": c.refresh_token}).to_string()) } fn keychain_error(what: &str, e: &rustgis_engine::data::secrets::SecretError) -> Error { @@ -841,13 +875,14 @@ mod native { ) } - /// A file entry with its tokens: from the file (from before the keychain, moved to the - /// keychain now if it takes them), this process, or the keychain. Tokens of another `rev` are - /// out of date: none. Under the store lock. + /// A file entry with the tokens this process can use (see the table above), without reading + /// the keychain: a keychain sign-in's refresh token is read when needed ([`load_refresh`]). + /// Tokens still in the file (from before the keychain) move to the keychain if it takes + /// them; the file changes only then. Under the store lock. fn with_tokens(v: &Vault, path: &Path, accounts: &mut Accounts, entry: Credentials) -> Result { if entry.kept == Kept::SettingsFile { let Some(store) = v.store() else { return Ok(entry) }; - let moved = Credentials { rev: random_token()?, kept: Kept::Keychain, ..entry.clone() }; + let moved = Credentials { rev: random_token()?, in_keychain: true, kept: Kept::Keychain, ..entry.clone() }; if let Err(e) = store.save(&secret_name(&entry.client_id, &entry.subject), &secret(&moved)) { log::warn!("the ChatGPT sign-in stays in the settings folder: {e}"); return Ok(entry); @@ -858,72 +893,107 @@ mod native { *e = moved.without_tokens(); } if let Err(e) = write_accounts(path, &after) { + // The keychain's copy is the file's own token; the next use moves it again. log::warn!("the ChatGPT sign-in stays in the settings folder: {e}"); return Ok(entry); } *accounts = after; + v.held().insert(id(&moved), Held::of(&moved)); return Ok(moved); } + let held = v.held(); + let mine = held.get(&id(&entry)).filter(|h| h.session == entry.session); if entry.rev.is_empty() { - return Ok(entry); - } - if let Some(h) = v.held().get(&id(&entry)).filter(|h| h.rev == entry.rev) { - return Ok(h.apply(&entry)); - } - // Without a keychain, tokens kept elsewhere are another process's. - let Some(store) = v.store() else { return Ok(entry) }; - let saved = store - .load(&secret_name(&entry.client_id, &entry.subject)) - .map_err(|e| keychain_error("the ChatGPT sign-in can't be read", &e))?; - let Some(saved) = saved.and_then(|s| serde_json::from_str::(s.expose()).ok()) else { return Ok(entry) }; - let field = |k: &str| saved.get(k).and_then(Value::as_str).unwrap_or("").to_string(); - if field("rev") != entry.rev { - return Ok(entry); + return Ok(match mine.filter(|h| h.kept == Kept::Session) { + Some(h) => h.apply(&entry), + None => entry, + }); } - Ok(Credentials { - id_token: field("id_token"), - access_token: field("access_token"), - refresh_token: field("refresh_token"), - kept: Kept::Keychain, - ..entry + // Another process's renewal rotates the refresh token, not this process's access token. + Ok(match mine.filter(|h| h.kept == Kept::Keychain) { + Some(h) => h.apply(&entry), + None => Credentials { expires_at: 0, kept: Kept::Keychain, ..entry }, }) } - /// Puts a registration and its tokens in `accounts` and selects it: the tokens go to the - /// keychain; to the file only for an entry whose tokens are still there (from before the - /// keychain) when the keychain refuses them; else to this session. Without tokens, any are - /// deleted. Gives the credentials as kept; the caller writes the file, under the store lock. - fn put_tokens(v: &Vault, accounts: &mut Accounts, c: &Credentials) -> Result { + /// The refresh token of `c`: the keychain's for a keychain sign-in (none when it is out of + /// date or missing). Under the store lock. + fn load_refresh(v: &Vault, c: &Credentials) -> Result { + if c.kept != Kept::Keychain { + return Ok(c.refresh_token.clone()); + } + let store = v.store().ok_or_else(|| Error::new(Recovery::Auth, KEYCHAIN_ELSEWHERE))?; + let saved = + store.load(&secret_name(&c.client_id, &c.subject)).map_err(|e| keychain_error("the ChatGPT sign-in can't be read", &e))?; + let saved = saved.and_then(|s| serde_json::from_str::(s.expose()).ok()); + let field = |k: &str| saved.as_ref().and_then(|s| s.get(k)).and_then(Value::as_str).unwrap_or("").to_string(); + Ok(if field("rev") == c.rev { field("refresh_token") } else { String::new() }) + } + + /// Keeps a registration and its tokens, selects it and writes the file: the refresh token + /// goes to the keychain (the item noted in the file first). If the keychain doesn't take it, + /// tokens renewed from the file's own go back there, others last for this session; a new + /// sign-in (`new`) over an account the keychain still holds is refused instead. Without + /// tokens, any in the keychain are deleted. Gives the credentials as kept. Under the store lock. + fn keep_tokens(v: &Vault, path: &Path, accounts: &mut Accounts, c: &Credentials, new: bool) -> Result { let name = secret_name(&c.client_id, &c.subject); let store = v.store(); + let existing = accounts.entries.iter().find(|e| id(e) == id(c)).cloned(); + let mut in_keychain = c.in_keychain || existing.as_ref().is_some_and(|e| e.in_keychain); if !c.has_tokens() { - let entry = Credentials { rev: String::new(), ..c.without_tokens() }; v.held().remove(&id(c)); - // The file's empty `rev` already makes any left in the keychain unusable. - if let Some(Err(e)) = store.map(|s| s.delete(&name)) { - log::warn!("the ChatGPT sign-in's tokens couldn't be deleted from the system keychain: {e}"); + // The empty `rev` already makes an item left behind unusable; `keychain` remembers it. + match store.map(|s| s.delete(&name)) { + Some(Ok(())) => in_keychain = false, + Some(Err(e)) if in_keychain => log::warn!("the ChatGPT sign-in couldn't be deleted from the system keychain: {e}"), + _ => {} } + let entry = Credentials { rev: String::new(), in_keychain, ..c.without_tokens() }; accounts.put(&entry); + write_accounts(path, accounts)?; return Ok(entry); } let mut kept = Credentials { rev: random_token()?, kept: Kept::Keychain, ..c.clone() }; - // Only tokens renewed from the file's own may go back there; a new sign-in never does. - let in_file = c.kept == Kept::SettingsFile && accounts.entries.iter().any(|e| id(e) == id(c) && e.kept == Kept::SettingsFile); - match store.map(|s| s.save(&name, &secret(&kept))) { - Some(Ok(())) => {} - failed => { - if let Some(Err(e)) = failed { - log::warn!("the system keychain didn't keep the ChatGPT sign-in: {e}"); + let saved = match &store { + Some(store) => { + if !in_keychain { + // Noted before the item exists, so it is never left behind unreferenced. + in_keychain = true; + let mut marked = Accounts { entries: accounts.entries.clone(), active: accounts.active.clone() }; + match marked.entries.iter_mut().find(|e| id(e) == id(c)) { + Some(e) => e.in_keychain = true, + None => marked.entries.push(Credentials { rev: String::new(), in_keychain: true, ..c.without_tokens() }), + } + write_accounts(path, &marked)?; + *accounts = marked; } - kept.kept = if in_file { Kept::SettingsFile } else { Kept::Session }; + store.save(&name, &secret(&kept)).map_err(|e| e.to_string()) } + None => Err("no system keychain here".to_string()), + }; + if let Err(why) = saved { + log::warn!("the system keychain didn't keep the ChatGPT sign-in: {why}"); + // Replacing it would sign the account out where the keychain holds it, unrevoked. + if new && existing.as_ref().is_some_and(|e| !e.rev.is_empty()) { + return Err(Error::new( + Recovery::Auth, + "the system keychain didn't keep the new sign-in, and it holds this account's earlier one; sign in again where the keychain is available", + )); + } + // Only tokens renewed from the file's own may go back there; a new sign-in never does. + let from_file = c.kept == Kept::SettingsFile && existing.as_ref().is_some_and(|e| e.kept == Kept::SettingsFile); + kept.kept = if from_file { Kept::SettingsFile } else { Kept::Session }; + kept.rev = String::new(); } - if kept.kept == Kept::Session { - v.held().insert(id(c), Held::of(&kept)); - } else { + kept.in_keychain = in_keychain; + if kept.kept == Kept::SettingsFile { v.held().remove(&id(c)); + accounts.put(&kept); + } else { + v.held().insert(id(c), Held::of(&kept)); + accounts.put(&kept.without_tokens()); } - accounts.put(&if kept.kept == Kept::SettingsFile { kept.clone() } else { kept.without_tokens() }); + write_accounts(path, accounts)?; Ok(kept) } @@ -936,14 +1006,10 @@ mod native { /// Saves `c` and selects it. Under the store lock. fn write_credentials(path: &Path, v: &Vault, c: &Credentials) -> Result { - let mut accounts = read_accounts(path)?; - let kept = put_tokens(v, &mut accounts, c)?; - write_accounts(path, &accounts)?; - Ok(kept) + keep_tokens(v, path, &mut read_accounts(path)?, c, false) } - /// The selected registration, including signed-out accounts (without its tokens when the - /// keychain can't give them). + /// The selected registration, including signed-out accounts. Never reads the keychain. pub fn load_any() -> Option { let path = auth_path()?; let _guard = store(&path).ok()?; @@ -1070,26 +1136,40 @@ mod native { check_id_claims(&claims, client_id, nonce, now()).map_err(Error::auth) } - /// Renews the tokens with `send` and keeps the result. An unusable refresh token is cleared - /// (the registration stays for the next sign-in). + /// Renews the tokens with `send` and keeps the result; the caller holds the renewal lock, so + /// no other process uses the refresh token until the rotated one is kept. An unusable or + /// missing refresh token signs the account out (the registration stays for the next sign-in). fn refresh(path: &Path, v: &Vault, c: &Credentials, send: Send) -> Result { - if c.refresh_token.is_empty() { - return Err(Error::new(Recovery::SignIn, "the sign-in can't be renewed; sign in again")); - } - let resp = send(&refresh_request(c))?; + let current = { + let _guard = store(path)?; + let current = selected_at(path, v)? + .filter(|now| now.binding() == c.binding()) + .ok_or_else(|| Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; try again"))?; + // Another request of this process renewed it meanwhile. + if current.access_token != c.access_token && !current.access_token.is_empty() && !current.expires_soon(now()) { + return Ok(current); + } + let refresh_token = load_refresh(v, ¤t)?; + if refresh_token.is_empty() { + // A keychain sign-in whose token is gone or out of date can't be used again here. + if current.kept == Kept::Keychain { + write_credentials(path, v, ¤t.registration())?; + } + return Err(Error::new(Recovery::SignIn, "the sign-in can't be renewed; sign in again")); + } + Credentials { refresh_token, ..current } + }; + let resp = send(&refresh_request(¤t))?; let grant = parse_grant(&resp); let _guard = store(path)?; // A user can sign out or sign into another account while the grant is in flight. - if selected_at(path, v).ok().flatten().as_ref() != Some(c) { + if selected_at(path, v).ok().flatten().map(|now| (now.binding(), now.rev)) != Some((current.binding(), current.rev.clone())) { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; try again")); } match grant { - Ok(g) => write_credentials(path, v, &renewed(c, &g, now())), + Ok(g) => write_credentials(path, v, &renewed(¤t, &g, now())), Err(GrantError::Unusable) => { - // Not a sign-out: the id token stays to skip choosing the account next time. - let mut registration = c.registration(); - registration.id_token = c.id_token.clone(); - write_credentials(path, v, ®istration)?; + write_credentials(path, v, ¤t.registration())?; Err(Error::token(&resp, GrantError::Unusable)) } Err(e) => Err(Error::token(&resp, e)), @@ -1216,58 +1296,58 @@ mod native { } } + /// What a sign-out leaves to do. + #[derive(Debug, Default)] + pub struct SignedOut { + /// The session to end at OpenAI ([`revoke`], off the UI thread). + pub revocation: Option, + /// Why it can't be ended from here: the keychain couldn't give its refresh token. + pub unrevoked: Option, + } + /// Signs out on this computer at once: the tokens are deleted (the registration and host id - /// stay for the next sign-in). Revoke the session with what it gives ([`revoke`]), off the - /// UI thread. - pub fn sign_out() -> Result, Error> { + /// stay for the next sign-in). Revoke the session with what it gives. + pub fn sign_out() -> Result { match auth_path() { Some(path) => sign_out_at(&path, vault()), - None => Ok(None), + None => Ok(SignedOut::default()), } } /// Signs out only if the displayed registration is still selected. - pub fn sign_out_selected(account: &Binding) -> Result, Error> { + pub fn sign_out_selected(account: &Binding) -> Result { let path = auth_path().ok_or("no settings folder")?; sign_out_checked(&path, vault(), Some(account)) } - fn sign_out_at(path: &Path, v: &Vault) -> Result, Error> { + fn sign_out_at(path: &Path, v: &Vault) -> Result { sign_out_checked(path, v, None) } - fn sign_out_checked(path: &Path, v: &Vault, expected: Option<&Binding>) -> Result, Error> { + fn sign_out_checked(path: &Path, v: &Vault, expected: Option<&Binding>) -> Result { let _guard = store(path)?; let mut accounts = read_accounts(path)?; - let Some(entry) = accounts.selected().cloned() else { return Ok(None) }; + let Some(entry) = accounts.selected().cloned() else { return Ok(SignedOut::default()) }; if expected.is_some_and(|e| *e != entry.binding()) { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; select the account again")); } - // The refresh token to revoke. A keychain that can't give it stops the sign-out: its - // tokens would stay there, unrevoked. let c = with_tokens(v, path, &mut accounts, entry)?; - out_of_reach(v, &c)?; + let (refresh_token, unrevoked) = match load_refresh(v, &c) { + Ok(r) => (r, None), + // Without a keychain here (the command line), the app can still revoke it. + Err(e) if v.store().is_none() => return Err(e), + // A keychain that can't give it doesn't keep anyone signed in: the session can't be + // ended at OpenAI from here, which the caller says. + Err(e) => (String::new(), Some(e)), + }; let mut registration = c.registration(); registration.session = random_token()?; - put_tokens(v, &mut accounts, ®istration)?; - write_accounts(path, &accounts)?; - Ok(revocation(&c)) + keep_tokens(v, path, &mut accounts, ®istration, false)?; + Ok(SignedOut { revocation: revocation(&c.client_id, refresh_token), unrevoked }) } - fn revocation(c: &Credentials) -> Option { - (!c.refresh_token.is_empty()).then(|| Revocation { client_id: c.client_id.clone(), refresh_token: c.refresh_token.clone() }) - } - - /// Refuses to forget tokens kept in a keychain this process doesn't have (the command line's): - /// they would stay there, unrevoked. - fn out_of_reach(v: &Vault, c: &Credentials) -> Result<(), Error> { - if v.store().is_none() && !c.rev.is_empty() && !c.has_tokens() { - return Err(Error::new( - Recovery::Auth, - "this ChatGPT sign-in is kept in the system keychain, which isn't available here; sign out in the RustGIS app", - )); - } - Ok(()) + fn revocation(client_id: &str, refresh_token: String) -> Option { + (!refresh_token.is_empty()).then(|| Revocation { client_id: client_id.to_string(), refresh_token }) } /// Removes a saved registration, with its tokens (signing it out first when signed in). @@ -1282,18 +1362,28 @@ mod native { let who = (client_id.to_string(), subject.to_string()); let Some(entry) = accounts.entries.iter().find(|e| id(e) == who).cloned() else { return Ok(None) }; let c = with_tokens(v, path, &mut accounts, entry)?; - out_of_reach(v, &c)?; - // The tokens go first: a registration whose tokens the keychain kept is never forgotten. - v.held().remove(&who); - if let Some(store) = v.store() { - store.delete(&secret_name(client_id, subject)).map_err(|e| keychain_error("the ChatGPT account can't be removed", &e))?; + // The refresh token to revoke, and the keychain item go first: a registration whose item + // may still exist is never forgotten, or the item would be left behind for good. + let refresh_token = load_refresh(v, &c)?; + let item = c.in_keychain || !c.rev.is_empty(); + match v.store() { + Some(store) => { + if let Err(e) = store.delete(&secret_name(client_id, subject)) + && item + { + return Err(keychain_error("the ChatGPT account can't be removed", &e)); + } + } + None if item => return Err(Error::new(Recovery::Auth, KEYCHAIN_ELSEWHERE)), + None => {} } + v.held().remove(&who); accounts.entries.retain(|e| id(e) != who); if accounts.active.as_ref() == Some(&who) { accounts.active = None; } write_accounts(path, &accounts)?; - Ok(revocation(&c)) + Ok(revocation(client_id, refresh_token)) } /// Ends a signed-out session at OpenAI (blocking; retried a few times on network and server @@ -1597,9 +1687,7 @@ mod native { return Err(Error::new(Recovery::AccountChanged, "the ChatGPT sign-in changed; select the account again")); } // Keep valid identity even when plan permission was declined. - let kept = put_tokens(v, &mut accounts, c)?; - write_accounts(path, &accounts)?; - Ok(kept) + keep_tokens(v, path, &mut accounts, c, true) } fn save_if_active( @@ -1700,6 +1788,7 @@ mod native { saved_at: String::new(), session: "session-1".into(), rev: String::new(), + in_keychain: false, kept: Kept::Nowhere, } } @@ -1722,9 +1811,10 @@ mod native { selected_at(path, v)?.ok_or_else(|| "nothing selected".into()) } - /// The credentials without where, and as which revision, they are kept. + /// The credentials without where and as which revision they are kept, nor the refresh + /// token (a keychain sign-in's is read only when needed). fn plain(c: Credentials) -> Credentials { - Credentials { rev: String::new(), kept: Kept::Nowhere, ..c } + Credentials { rev: String::new(), in_keychain: false, kept: Kept::Nowhere, refresh_token: String::new(), ..c } } /// An entry as versions before the keychain saved it, tokens and all. @@ -1741,6 +1831,11 @@ mod native { ["synthetic-id", "synthetic-refresh", "rotated", "e30."].iter().any(|t| text.contains(t)) } + fn entry_json(path: &Path) -> Value { + let json: Value = serde_json::from_slice(&std::fs::read(path).unwrap()).unwrap(); + json["accounts"][0].clone() + } + const NAME: &str = "chatgpt/oaiapp_synthetic/user-1"; fn grant(expires_at: i64, refresh: &str) -> HttpResponse { @@ -1758,7 +1853,7 @@ mod native { } #[test] - fn tokens_go_to_the_keychain_and_never_to_the_file() { + fn the_refresh_token_goes_to_the_keychain_and_no_token_to_the_file() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); let store = Arc::new(MemorySecrets::new()); @@ -1766,20 +1861,30 @@ mod native { let c = synthetic(now() + 100_000); assert_eq!(write_credentials(&path, &v, &c).unwrap().kept, Kept::Keychain); assert!(!file_has_tokens(&path)); - let json: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); - let entry = &json["accounts"][0]; + let entry = entry_json(&path); for k in ["client_id", "subject", "email", "scopes", "expires_at"] { assert!(!entry[k].is_null(), "{k} stays in the file"); } - assert_eq!(json["active"]["subject"], "user-1"); - assert!(store.load(NAME).unwrap().unwrap().expose().contains("synthetic-refresh")); - // Another process, on the same keychain, reads the same sign-in. - let other = read_credentials(&path, &sharing(&store)).unwrap(); - assert_eq!(other.kept, Kept::Keychain); - assert_eq!(plain(other), c); - // Without the keychain's tokens, it is signed out; no error. + assert_eq!(entry["keychain"], true); + // The keychain keeps the refresh token alone: small enough for Windows (2560 bytes of + // UTF-16). + let saved = store.load(NAME).unwrap().unwrap(); + assert!( + saved.expose().contains("synthetic-refresh") + && !saved.expose().contains("e30.") + && !saved.expose().contains("synthetic-id") + ); + // This process keeps using its access token. + assert_eq!(plain(read_credentials(&path, &v).unwrap()), plain(c.clone())); + // Another process, on the same keychain, is signed in and renews first. + let (other, process) = (read_credentials(&path, &sharing(&store)).unwrap(), sharing(&store)); + assert!(other.signed_in() && other.kept == Kept::Keychain && other.access_token.is_empty() && other.expires_soon(now())); + assert_eq!(load_refresh(&process, &other).unwrap(), "synthetic-refresh"); + // Without the keychain's refresh token, renewing signs out, without any request. store.delete(NAME).unwrap(); - assert!(!read_credentials(&path, &v).unwrap().signed_in()); + let e = send_at(&test_request(), &c.binding(), &path, &process, &mut |_| panic!("nothing may be sent")).unwrap_err(); + assert_eq!(e.recovery, Recovery::SignIn); + assert!(!read_credentials(&path, &process).unwrap().signed_in()); } #[test] @@ -1794,7 +1899,8 @@ mod native { // Without a keychain, or with one that refuses, the file stays as it is and works. for v in [Vault::new(None), failing()] { let c = read_credentials(&path, &v).unwrap(); - assert_eq!((plain(c.clone()), c.kept), (old.clone(), Kept::SettingsFile)); + assert_eq!((c.refresh_token.as_str(), c.kept), ("synthetic-refresh", Kept::SettingsFile)); + assert_eq!(plain(c), plain(old.clone())); assert_eq!(std::fs::read_to_string(&path).unwrap(), text); } // Renewing such a sign-in without the keychain keeps the rotated token in the file: @@ -1813,10 +1919,11 @@ mod native { // The keychain takes them: out of the file. let store = Arc::new(MemorySecrets::new()); let moved = read_credentials(&path, &sharing(&store)).unwrap(); - assert_eq!((moved.refresh_token.as_str(), moved.kept), ("rotated", Kept::Keychain)); + assert_eq!(moved.kept, Kept::Keychain); assert!(!file_has_tokens(&path)); assert!(store.load(NAME).unwrap().unwrap().expose().contains("rotated")); - assert_eq!(read_credentials(&path, &sharing(&store)).unwrap().refresh_token, "rotated"); + let other = sharing(&store); + assert_eq!(load_refresh(&other, &read_credentials(&path, &other).unwrap()).unwrap(), "rotated"); } #[test] @@ -1835,10 +1942,33 @@ mod native { new.refresh_token = "rotated".into(); assert_eq!(save_at(&path, &v, &new, expected.as_ref()).unwrap().kept, Kept::Session); assert!(!file_has_tokens(&path), "never written to the file instead"); + assert_eq!(entry_json(&path)["rev"], "", "no keychain token to look for"); let c = read_credentials(&path, &v).unwrap(); assert!(c.signed_in() && c.kept == Kept::Session && c.refresh_token == "rotated"); - // Another process (or the next run) isn't signed in. - assert!(!read_credentials(&path, &Vault::new(None)).unwrap().signed_in()); + // Another process (or the next run) isn't signed in, and can sign out and remove + // the account without a keychain: there is nothing in it. + let next = Vault::new(None); + assert!(!read_credentials(&path, &next).unwrap().signed_in()); + assert!(sign_out_at(&path, &next).is_ok()); + if v.store().is_none() { + assert!(remove_at(&path, &next, "oaiapp_synthetic", "user-1").is_ok()); + } + } + } + + #[test] + fn a_sign_in_without_the_keychain_never_replaces_one_it_holds() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + write_credentials(&path, &sharing(&store), &synthetic(now() + 100_000)).unwrap(); + let before = (entry_json(&path)["rev"].clone(), store.load(NAME).unwrap()); + for v in [Vault::new(None), failing()] { + let expected = load_any_at(&path, &v); + let mut new = synthetic(now() + 100_000); + new.session = "session-2".into(); + assert!(save_at(&path, &v, &new, expected.as_ref()).is_err()); + assert_eq!((entry_json(&path)["rev"].clone(), store.load(NAME).unwrap()), before); } } @@ -1849,25 +1979,27 @@ mod native { let store = Arc::new(MemorySecrets::new()); let v = sharing(&store); write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); - // Without the keychain (the command line), its tokens are out of reach: neither - // signing out nor removing forgets them unrevoked. + // Without the keychain (the command line), its token is out of reach: neither signing + // out nor removing forgets it unrevoked. let none = Vault::new(None); assert!(sign_out_at(&path, &none).is_err()); assert!(remove_at(&path, &none, "oaiapp_synthetic", "user-1").is_err()); assert!(read_credentials(&path, &v).unwrap().signed_in()); - let r = sign_out_at(&path, &v).unwrap().unwrap(); - assert_eq!(r.refresh_token, "synthetic-refresh"); + let out = sign_out_at(&path, &v).unwrap(); + assert_eq!(out.revocation.unwrap().refresh_token, "synthetic-refresh"); + assert!(out.unrevoked.is_none()); assert_eq!(store.load(NAME).unwrap(), None); + assert_eq!(entry_json(&path)["keychain"], false); // Removing: the tokens and the registration. let mut other = synthetic(now() + 100_000); other.client_id = "oaiapp_other".into(); write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); write_credentials(&path, &v, &other).unwrap(); - // A keychain that refuses keeps the account: its tokens would be left behind. + // A keychain that refuses keeps the account: its item would be left behind. assert!(remove_at(&path, &failing(), "oaiapp_synthetic", "user-1").is_err()); assert_eq!(read_accounts(&path).unwrap().entries.len(), 2); let r = remove_at(&path, &v, "oaiapp_synthetic", "user-1").unwrap().unwrap(); - assert_eq!(r.client_id, "oaiapp_synthetic"); + assert_eq!((r.client_id.as_str(), r.refresh_token.as_str()), ("oaiapp_synthetic", "synthetic-refresh")); assert_eq!(store.load(NAME).unwrap(), None); let accounts = read_accounts(&path).unwrap(); assert_eq!(accounts.entries.iter().map(|e| e.client_id.as_str()).collect::>(), ["oaiapp_other"]); @@ -1877,24 +2009,97 @@ mod native { assert!(remove_at(&path, &v, "oaiapp_other", "user-1").unwrap().is_none()); } + #[test] + fn an_unreadable_keychain_signs_out_here_and_keeps_its_item_noted() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + write_credentials(&path, &sharing(&store), &synthetic(now() + 100_000)).unwrap(); + let out = sign_out_at(&path, &failing()).unwrap(); + assert!(out.revocation.is_none() && out.unrevoked.is_some(), "the caller says it wasn't revoked"); + assert!(!read_credentials(&path, &sharing(&store)).unwrap().signed_in()); + // The item couldn't be deleted: it stays noted, so removal still deletes it first. + assert_eq!(entry_json(&path)["keychain"], true); + assert!(remove_at(&path, &failing(), "oaiapp_synthetic", "user-1").is_err()); + assert!(remove_at(&path, &Vault::new(None), "oaiapp_synthetic", "user-1").is_err()); + remove_at(&path, &sharing(&store), "oaiapp_synthetic", "user-1").unwrap(); + assert_eq!(store.load(NAME).unwrap(), None); + assert!(read_accounts(&path).unwrap().entries.is_empty()); + } + + #[test] + fn an_account_without_a_keychain_item_is_removed_whatever_the_keychain() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + std::fs::write(&path, synthetic(0).registration().to_json().to_string()).unwrap(); + remove_at(&path, &failing(), "oaiapp_synthetic", "user-1").unwrap(); + assert!(read_accounts(&path).unwrap().entries.is_empty()); + } + + /// A keychain that checks the file notes its item before it is written. + struct Noting { + inner: MemorySecrets, + path: std::path::PathBuf, + } + + impl SecretStore for Noting { + fn load(&self, account: &str) -> Result, SecretError> { + self.inner.load(account) + } + fn save(&self, account: &str, value: &Secret) -> Result<(), SecretError> { + assert_eq!(entry_json(&self.path)["keychain"], true, "noted before the keychain write"); + self.inner.save(account, value) + } + fn delete(&self, account: &str) -> Result<(), SecretError> { + self.inner.delete(account) + } + } + + #[test] + fn a_keychain_item_is_noted_in_the_file_before_it_is_written() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let v = Vault::new(Some(Arc::new(Noting { inner: MemorySecrets::new(), path: path.clone() }))); + save_at(&path, &v, &synthetic(now() + 100_000), None).unwrap(); + assert_eq!(read_credentials(&path, &v).unwrap().kept, Kept::Keychain); + } + #[test] fn keychain_tokens_of_another_revision_are_never_used() { - // A renewal whose tokens the keychain took but whose file write failed: the rotated + // A renewal whose token the keychain took but whose file write failed: the rotated // refresh token is the keychain's, the file still names the previous revision. let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); let store = Arc::new(MemorySecrets::new()); - let (first, second) = (sharing(&store), sharing(&store)); - write_credentials(&path, &first, &synthetic(0)).unwrap(); - assert!(read_credentials(&path, &second).unwrap().signed_in()); - let mut rotated = synthetic(now() + 100_000); - rotated.refresh_token = "rotated".into(); - put_tokens(&first, &mut read_accounts(&path).unwrap(), &rotated).unwrap(); - for v in [&first, &second] { - assert!(!read_credentials(&path, v).unwrap().signed_in()); - let e = send_at(&test_request(), &synthetic(0).binding(), &path, v, &mut |_| panic!("no token may be used")).unwrap_err(); - assert_eq!(e.recovery, Recovery::SignIn); + write_credentials(&path, &sharing(&store), &synthetic(0)).unwrap(); + store.save(NAME, &Secret::new(serde_json::json!({"rev": "another", "refresh_token": "rotated"}).to_string())).unwrap(); + let v = sharing(&store); + let e = send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |_| panic!("no token may be used")).unwrap_err(); + assert_eq!(e.recovery, Recovery::SignIn); + assert!(!read_credentials(&path, &v).unwrap().signed_in()); + } + + #[test] + fn processes_renew_in_turn_with_the_latest_refresh_token() { + // Each process renews once for its own access token; the renewal lock and the + // keychain read under it mean each uses the refresh token the last one kept. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let store = Arc::new(MemorySecrets::new()); + write_credentials(&path, &sharing(&store), &synthetic(now() + 100_000)).unwrap(); + let mut used = Vec::new(); + for (i, process) in [sharing(&store), sharing(&store)].iter().enumerate() { + send_at(&test_request(), &synthetic(0).binding(), &path, process, &mut |req| { + if req.url == super::super::TOKEN_URL { + let body = req.body.clone().unwrap_or_default(); + used.push(body.split('&').find_map(|p| p.strip_prefix("refresh_token=")).unwrap_or("").to_string()); + return Ok(grant(now() + 100_000, &format!("rotated-{i}"))); + } + Ok(HttpResponse { status: 200, body: vec![], headers: vec![] }) + }) + .unwrap(); } + assert_eq!(used, ["synthetic-refresh", "rotated-0"]); } #[test] @@ -2023,7 +2228,7 @@ mod native { assert!(result.is_err()); assert!(save_at(&path, &v, &old, Some(&old)).is_err(), "late login must not undo the change"); let current = read_credentials(&path, &v).unwrap(); - assert_ne!(current.refresh_token, "late-refresh"); + assert!(!store.load(NAME).unwrap().is_some_and(|s| s.expose().contains("late-refresh"))); assert_eq!(current.signed_in(), !signout); } } @@ -2036,20 +2241,20 @@ mod native { let a = synthetic(now() + 100_000); std::fs::write(&path, legacy_json(&a).to_string()).unwrap(); let read = read_credentials(&path, &v).unwrap(); - assert_eq!(plain(read.clone()), a); + assert_eq!(plain(read.clone()), plain(a.clone())); let mut b = a.clone(); b.client_id = "oaiapp_workspace".into(); assert_ne!(a.label(), b.label()); save_at(&path, &v, &b, Some(&read)).unwrap(); let selected = select_at(&path, &v, &a.client_id, &a.subject).unwrap(); - assert_eq!(plain(selected), a, "selecting changes nothing but the selection"); + assert_eq!(plain(selected), plain(a.clone()), "selecting changes nothing but the selection"); sign_out_at(&path, &v).unwrap(); let accounts = read_accounts(&path).unwrap(); assert_eq!(accounts.entries.len(), 2); assert!(!read_credentials(&path, &v).unwrap().signed_in()); let selected = select_at(&path, &v, &b.client_id, &b.subject).unwrap(); assert!(selected.signed_in()); - assert_eq!(selected.refresh_token, b.refresh_token); + assert_eq!(load_refresh(&v, &selected).unwrap(), b.refresh_token); let json: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); assert_eq!(json["version"], 2); } @@ -2099,19 +2304,18 @@ mod native { #[test] fn simultaneous_401_responses_rotate_only_once() { - // Two processes: their own vaults, one keychain. + // Two requests of one process: the second uses the first's renewal. let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("test-auth.json"); let far = now() + 100_000; - let store = Arc::new(MemorySecrets::new()); - write_credentials(&path, &sharing(&store), &synthetic(far)).unwrap(); - let barrier = Arc::new(std::sync::Barrier::new(2)); - let rotations = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let v = keychain(); + write_credentials(&path, &v, &synthetic(far)).unwrap(); + let barrier = std::sync::Barrier::new(2); + let rotations = std::sync::atomic::AtomicUsize::new(0); std::thread::scope(|scope| { let workers: Vec<_> = (0..2) .map(|_| { - let (barrier, rotations, path, v) = (barrier.clone(), rotations.clone(), path.clone(), sharing(&store)); - scope.spawn(move || { + scope.spawn(|| { let mut inference = 0; send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |req| { if req.url == super::super::TOKEN_URL { @@ -2137,7 +2341,7 @@ mod native { } }); assert_eq!(rotations.load(std::sync::atomic::Ordering::SeqCst), 1); - assert_eq!(read_credentials(&path, &sharing(&store)).unwrap().refresh_token, "synthetic-rotated"); + assert_eq!(load_refresh(&v, &read_credentials(&path, &v).unwrap()).unwrap(), "synthetic-rotated"); } #[test] @@ -2196,8 +2400,6 @@ mod native { let left = read_credentials(&path, &v).unwrap(); assert!(!left.signed_in() && left.refresh_token.is_empty()); assert_eq!(left.client_id, "oaiapp_synthetic"); - // The id token stays, to skip choosing the account next time. - assert_eq!(left.id_token, "synthetic-id"); // A passing failure keeps the tokens. write_credentials(&path, &v, &synthetic(0)).unwrap(); assert!( @@ -2229,15 +2431,16 @@ mod native { let path = dir.path().join("test-auth.json"); let v = keychain(); write_credentials(&path, &v, &synthetic(now() + 100_000)).unwrap(); - let r = sign_out_at(&path, &v).unwrap().unwrap(); + let out = sign_out_at(&path, &v).unwrap(); + let r = out.revocation.unwrap(); assert_eq!((r.client_id.as_str(), r.refresh_token.as_str()), ("oaiapp_synthetic", "synthetic-refresh")); assert!(!format!("{r:?}").contains("synthetic-refresh")); let left = read_credentials(&path, &v).unwrap(); assert!(!left.signed_in() && left.id_token.is_empty() && left.refresh_token.is_empty()); assert_eq!((left.client_id.as_str(), left.subject.as_str()), ("oaiapp_synthetic", "user-1")); // Again: nothing more to revoke. A missing file is no error. - assert!(sign_out_at(&path, &v).unwrap().is_none()); - assert!(sign_out_at(&dir.path().join("missing.json"), &v).unwrap().is_none()); + assert!(sign_out_at(&path, &v).unwrap().revocation.is_none()); + assert!(sign_out_at(&dir.path().join("missing.json"), &v).unwrap().revocation.is_none()); } #[test] @@ -2309,9 +2512,9 @@ mod native { let path = dir.path().join("test-auth.json"); let v = keychain(); let first = synthetic(1); - let next = synthetic(2); + let next = plain(synthetic(2)); write_credentials(&path, &v, &first).unwrap(); - write_credentials(&path, &v, &next).unwrap(); + write_credentials(&path, &v, &synthetic(2)).unwrap(); assert_eq!(plain(read_credentials(&path, &v).unwrap()), next); #[cfg(unix)] { diff --git a/crates/assistant/src/provider.rs b/crates/assistant/src/provider.rs index a91c10e76..7de89b7bf 100644 --- a/crates/assistant/src/provider.rs +++ b/crates/assistant/src/provider.rs @@ -193,9 +193,10 @@ impl Config { } } - /// Uses a signed-in account's access token. + /// Uses a signed-in account. The key only says that it is signed in, never a token: + /// `chatgpt::send` puts the current access token on each request. pub fn sign_in(&mut self, c: &crate::chatgpt::Credentials) { - self.key = if c.plan_allowed() { c.access_token.clone() } else { String::new() }; + self.key = if c.plan_allowed() && c.signed_in() { crate::chatgpt::SIGNED_IN.to_string() } else { String::new() }; self.client_id = c.client_id.clone(); self.account = c.subject.clone(); self.session = c.session.clone(); diff --git a/crates/assistant/src/tests.rs b/crates/assistant/src/tests.rs index efe107a8e..31a41b55f 100644 --- a/crates/assistant/src/tests.rs +++ b/crates/assistant/src/tests.rs @@ -1273,6 +1273,7 @@ fn chatgpt_credentials(expires_at: i64) -> chatgpt::Credentials { saved_at: "2026-10-14T00:00:00Z".into(), session: "session-1".into(), rev: String::new(), + in_keychain: false, kept: chatgpt::Kept::Nowhere, } } @@ -1305,7 +1306,8 @@ fn responses_request_shape() { let Step::Send(req) = a.ask("Zoom to Paris", &s) else { panic!("expected a request") }; assert_eq!(req.url, "https://api.openai.com/v1/responses"); let h = |k: &str| req.headers.iter().find(|(n, _)| n == k).map(|(_, v)| v.as_str()); - assert!(h("Authorization").is_some_and(|v| v.starts_with("Bearer ey"))); + // No token: `chatgpt::send` authorizes each request with the current one. + assert_eq!(h("Authorization").map(str::to_string), Some(format!("Bearer {}", chatgpt::SIGNED_IN))); assert_eq!(req.headers.iter().filter(|(k, _)| !["Content-Type", "Accept", "Authorization"].contains(&k.as_str())).count(), 0); let b = body(&req); assert_eq!(b["model"], "gpt-6.1-sol"); diff --git a/crates/ui-egui/src/assistantpane.rs b/crates/ui-egui/src/assistantpane.rs index 72d52633e..61f426c15 100644 --- a/crates/ui-egui/src/assistantpane.rs +++ b/crates/ui-egui/src/assistantpane.rs @@ -75,7 +75,7 @@ pub struct AssistantState { /// The ChatGPT sign-in, being read on a worker: its tokens are in the system keychain, which /// may block or ask (desktop only). #[cfg(not(target_arch = "wasm32"))] - sign_in_read: Option>>, + sign_in_read: Option, /// The first sign-in's confirmation that the ChatGPT plan is in use, until dismissed. welcome: bool, /// A failed request to send again: its generation and when. @@ -180,7 +180,7 @@ fn ensure_loaded(app: &mut RustGisApp, ctx: &egui::Context) { } #[cfg(not(target_arch = "wasm32"))] if cfg.login() { - read_sign_in(app, ctx); + read_sign_in(app, None, ctx); } #[cfg(not(target_arch = "wasm32"))] let loading = reading(app, &provider); @@ -260,9 +260,15 @@ fn sync_skills(app: &mut RustGisApp, force: bool) { } } -/// Starts reading the selected ChatGPT sign-in on a worker; [`poll_sign_in`] applies it. +/// A sign-in read under way: the account it checks (`None`: take whichever is selected) and +/// where the worker leaves what it read. #[cfg(not(target_arch = "wasm32"))] -fn read_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { +type SignInRead = (Option, Slot>); + +/// Starts reading the selected ChatGPT sign-in on a worker; [`poll_sign_in`] applies it. With +/// `checking`, it only says whether that sign-in is still there (after a failed request). +#[cfg(not(target_arch = "wasm32"))] +fn read_sign_in(app: &mut RustGisApp, checking: Option, ctx: &egui::Context) { let slot: Slot> = Slot::default(); let (out, ctx) = (slot.clone(), ctx.clone()); std::thread::spawn(move || { @@ -272,35 +278,54 @@ fn read_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { } ctx.request_repaint(); }); - app.assistant.sign_in_read = Some(slot); + app.assistant.sign_in_read = Some((checking, slot)); } -/// Applies a finished sign-in read to the sign-in provider, if it's still the one chosen: a new -/// sign-in loads its catalog; one that's gone (expired, revoked, signed out elsewhere) clears the -/// key and opens the settings to sign in again. +/// Applies a finished sign-in read, if the sign-in provider is still the one chosen (account +/// actions drop a read begun before them). A check after a failed request never takes another +/// account; when its sign-in is gone (expired, revoked, signed out elsewhere) the key is cleared +/// and the settings open to sign in again. A read taking the selected account goes through +/// [`apply_config`], which stops work bound to another account, and loads the new catalog. #[cfg(not(target_arch = "wasm32"))] fn poll_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { - let Some(read) = app.assistant.sign_in_read.as_ref().and_then(|s| s.lock().ok().and_then(|mut s| s.take())) else { return }; + let Some((checking, slot)) = app.assistant.sign_in_read.as_ref() else { return }; + let Some(read) = slot.lock().ok().and_then(|mut s| s.take()) else { return }; + let checking = checking.clone(); app.assistant.sign_in_read = None; - let Some(agent) = app.assistant.agent.as_mut().filter(|a| a.config.login()) else { return }; - let had = !agent.config.key.is_empty(); - match &read { - Some(c) => agent.config.sign_in(c), - None => agent.config.key.clear(), - } - let cfg = agent.config.clone(); - if !cfg.key.is_empty() { - app.assistant.account = read.map(|c| c.account()); - if !had { - list_models(app, &cfg, ctx); + let Some(cfg) = app.assistant.agent.as_ref().map(|a| a.config.clone()).filter(Config::login) else { return }; + let usable = read.filter(|c| c.signed_in() && c.plan_allowed()); + if let Some(binding) = checking { + // Still signed in, or another account selected and signed in (requests for this one + // then fail with "the account changed", and the user chooses): nothing to clear. + let signed_in_somewhere = usable.is_some(); + if signed_in_somewhere || cfg.binding() != binding || cfg.key.is_empty() { + return; } - } else { + let mut next = cfg; + next.key.clear(); + apply_config(app, next); app.assistant.account = None; - if had { - app.assistant.welcome = false; - app.assistant.settings = true; - app.assistant.accounts = None; - } + app.assistant.welcome = false; + app.assistant.settings = true; + app.assistant.accounts = None; + return; + } + let mut next = cfg.clone(); + match &usable { + Some(c) => next.sign_in(c), + None => next.key.clear(), + } + if next == cfg { + return; + } + apply_config(app, next.clone()); + app.assistant.account = usable.map(|c| c.account()); + app.assistant.models.clear(); + app.assistant.catalog.clear(); + app.assistant.models_error = None; + app.assistant.models_slot = None; + if !next.key.is_empty() { + list_models(app, &next, ctx); } } @@ -460,6 +485,7 @@ fn apply_config(app: &mut RustGisApp, cfg: Config) { /// A ChatGPT account signed in: the assistant uses its plan from now on. #[cfg(not(target_arch = "wasm32"))] fn signed_in(app: &mut RustGisApp, c: &chatgpt::Credentials, ctx: &egui::Context) { + app.assistant.sign_in_read = None; app.assistant.account = Some(c.account()); app.assistant.login_error = None; let saved = chatgpt::accounts().unwrap_or_default(); @@ -491,8 +517,9 @@ fn signed_in(app: &mut RustGisApp, c: &chatgpt::Credentials, ctx: &egui::Context /// or were revoked), the settings say so and offer to sign in again (read off the frame). fn sync_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { #[cfg(not(target_arch = "wasm32"))] - if app.assistant.agent.as_ref().is_some_and(|a| a.config.login() && !a.config.key.is_empty()) { - read_sign_in(app, ctx); + if let Some(binding) = app.assistant.agent.as_ref().filter(|a| a.config.login() && !a.config.key.is_empty()).map(|a| a.config.binding()) + { + read_sign_in(app, Some(binding), ctx); } #[cfg(target_arch = "wasm32")] let _ = (app, ctx); @@ -890,7 +917,7 @@ fn provider_settings(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Co app.assistant.login_error = None; #[cfg(not(target_arch = "wasm32"))] if cfg.login() { - read_sign_in(app, ui.ctx()); + read_sign_in(app, None, ui.ctx()); } } } @@ -1091,6 +1118,8 @@ fn account_row(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Config) if ui.selectable_label(selected, account.label()).clicked() { app.assistant.accounts = None; app.assistant.duplicate = false; + // A read begun before this choice must not undo it. + app.assistant.sign_in_read = None; match chatgpt::select(&account.client_id, &account.subject) { Ok(c) => { cfg.sign_in(&c); @@ -1145,13 +1174,21 @@ fn account_row(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens, cfg: &mut Config) ui.label(RichText::new(who.unwrap_or_else(|| tl!("signed in").to_string())).small()); if ui.small_button(tl!("Sign out")).clicked() { match chatgpt::sign_out_selected(&cfg.binding()) { - Ok(revocation) => revoke_later(app, revocation, ui.ctx()), + Ok(out) => { + revoke_later(app, out.revocation, ui.ctx()); + forget_sign_in(app, cfg); + if out.unrevoked.is_some() { + app.assistant.login_error = Some( + tl!("Signed out on this computer, but the system keychain couldn't be read, so OpenAI wasn't asked to end the session. You can disconnect RustGIS in ChatGPT's settings.") + .into(), + ); + } + } Err(e) => { app.assistant.login_error = Some(e.auth_context()); return; } } - forget_sign_in(app, cfg); } if app.assistant.duplicate { ui.label( @@ -1186,6 +1223,7 @@ fn revoke_later(app: &mut RustGisApp, revocation: Option, c /// After signing out or removing the account: no tokens, no catalog. #[cfg(not(target_arch = "wasm32"))] fn forget_sign_in(app: &mut RustGisApp, cfg: &mut Config) { + app.assistant.sign_in_read = None; app.assistant.models_slot = None; cfg.key.clear(); app.assistant.account = None; @@ -1208,6 +1246,7 @@ fn waiting_for_browser(ui: &mut Ui, t: &Tokens, url: &str) -> (bool, bool) { #[cfg(not(target_arch = "wasm32"))] fn start_login(app: &mut RustGisApp, cfg: &mut Config, another: bool) { + app.assistant.sign_in_read = None; if !another && !cfg.client_id.is_empty() { match chatgpt::select(&cfg.client_id, &cfg.account) { Ok(c) => cfg.sign_in(&c), @@ -2421,15 +2460,65 @@ mod tests { h.run_steps(1); } h.state_mut().assistant.models_slot = None; + // A read begun before choosing an account must not undo the choice when it finishes. + let stale: Slot> = Slot::default(); + h.state_mut().assistant.sign_in_read = Some((None, stale.clone())); h.get_by_value(&a.label()).click(); h.run_steps(4); h.get_by_label(&b.label()).click(); h.run_steps(4); + *stale.lock().unwrap() = Some(Some(a.clone())); + h.run_steps(4); assert!(h.state().assistant.models_slot.is_some()); assert_eq!(h.state().assistant.agent.as_ref().unwrap().config.client_id, b.client_id); assert_eq!(chatgpt::load_any().unwrap().client_id, b.client_id); } + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn a_sign_in_check_never_switches_accounts_and_a_switch_stops_work() { + if !in_isolated_settings("assistantpane::tests::a_sign_in_check_never_switches_accounts_and_a_switch_stops_work") { + return; + } + let ctx = egui::Context::default(); + let creds = |client: &str| chatgpt::Credentials { + client_id: client.into(), + subject: "user".into(), + session: "session".into(), + access_token: "synthetic-token".into(), + scopes: vec![chatgpt::PLAN_SCOPE.into()], + ..Default::default() + }; + let finish = |app: &mut RustGisApp, checking: Option, read: Option| { + let slot: Slot> = Slot::default(); + *slot.lock().unwrap() = Some(read); + app.assistant.sign_in_read = Some((checking, slot)); + poll_sign_in(app, &ctx); + }; + let mut app = plan_app(); + let mut first = app.assistant.agent.as_ref().unwrap().config.clone(); + first.sign_in(&creds("first")); + apply_config(&mut app, first.clone()); + // A check after a failed request that finds another account selected changes nothing: + // the user chooses. + app.assistant.retry = Some((1, web_time::Instant::now())); + finish(&mut app, Some(first.binding()), Some(creds("second"))); + assert_eq!(app.assistant.agent.as_ref().unwrap().config, first); + assert!(app.assistant.retry.is_some()); + // One that finds the sign-in gone clears it and stops the work bound to it. + finish(&mut app, Some(first.binding()), None); + assert!(app.assistant.agent.as_ref().unwrap().config.key.is_empty()); + assert!(app.assistant.retry.is_none() && app.assistant.settings); + // Taking the selected account: another one stops work for the first and drops its catalog. + apply_config(&mut app, first.clone()); + app.assistant.retry = Some((2, web_time::Instant::now())); + app.assistant.models = vec!["first-model".into()]; + finish(&mut app, None, Some(creds("second"))); + assert_eq!(app.assistant.agent.as_ref().unwrap().config.client_id, "second"); + assert!(app.assistant.retry.is_none()); + assert!(!app.assistant.models.contains(&"first-model".to_string())); + } + #[cfg(not(target_arch = "wasm32"))] #[test] fn sign_in_loads_catalog_and_applies_first_offered_model() { diff --git a/docs/architecture.md b/docs/architecture.md index b82bec098..985270576 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -74,15 +74,21 @@ browser and the CLI, where callers keep a secret for the session only, as before the requested bucket's explicit-grant status. Copy only that login and grant boolean, not the grant list; parse and validate addresses and prepare signatures outside the lock. Keychain calls and network I/O never run under the registry lock. -- **ChatGPT sign-in.** The desktop app gives its store to `chatgpt::set_secret_store` at start; - each registration's tokens are one secret, `chatgpt//`, carrying the `rev` - its entry in `chatgpt-auth.json` names (any other is out of date and never used). The file's - `store-lock` covers every keychain read and write too, so a rotating refresh token is never used - twice. Keychain tokens are read again for each use; only those it couldn't keep stay in memory, - for the session. Tokens from an earlier version's file leave it only after the keychain has - taken them. `Config::from_env` never reads the sign-in: the Assistant reads it on a worker - (startup, switching to the provider, after a failed request) and requests wait for it; only - selecting, signing out and removing an account, user actions, read the keychain on the frame. +- **ChatGPT sign-in.** The desktop app gives its store to `chatgpt::set_secret_store` at start. + The keychain keeps only each registration's refresh token, `chatgpt//`, + with the `rev` its entry in `chatgpt-auth.json` names (any other is out of date and never + used), small enough for Windows Credential Manager's 2560 bytes of UTF-16. Access and id + tokens stay with the process that got them; a process without one renews first. The keychain + is read only to renew, sign out or remove, under the file's `store-lock` (and, to renew, the + `renewal-lock`), so a rotating refresh token is never used twice. The entry's `keychain` flag is + set before the first keychain write and cleared after a delete, so removal never strands an + item. Tokens the keychain couldn't keep stay in memory for the session, with an empty `rev`; a + new sign-in never replaces one the keychain holds. Tokens from an earlier version's file leave it + only after the keychain has taken them. `Config::from_env` never reads the sign-in, and a + configuration's key is a marker (`chatgpt::SIGNED_IN`), never a token: `chatgpt::send` + authorizes each request. The Assistant reads the sign-in on a worker (startup, switching to the + provider, after a failed request) bound to the selection that started it, and requests wait for + it. - **macOS.** The system asks again for access when the app's code signature changes, which happens with unsigned or re-signed builds. - **Tests.** Use `MemorySecrets`, and `MemorySecrets::failing(..)` for error paths. The desktop diff --git a/docs/cli.md b/docs/cli.md index 0f918c3c7..975630c40 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -112,9 +112,10 @@ plan. `--another-account` starts a new registration, `--accounts` lists saved re `--account ` selects one, `--remove-account ` signs it out and forgets it, and `--logout` signs out. The command line has no system keychain: it uses a sign-in an earlier version saved in the settings folder, but not one the desktop app keeps in the keychain, and its -own sign-ins last for the command only. `--logout` and `--remove-account` refuse an account whose -tokens are in the keychain (sign out or remove it in the app), so they are never left there -unrevoked. See [Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). +own sign-ins last for the command only (it can still sign them out and remove them later). +`--logout` and `--remove-account` refuse an account whose token is in the keychain (sign out or +remove it in the app), so it is never left there unrevoked, and `--login` never replaces such a +sign-in. See [Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). ## Serving AI agents diff --git a/docs/user-guide/assistant.md b/docs/user-guide/assistant.md index 1cf3cb5bf..662375fdd 100644 --- a/docs/user-guide/assistant.md +++ b/docs/user-guide/assistant.md @@ -287,20 +287,25 @@ is cleared only after the keychain key is read or the old key is saved successfu operation fails, the old preference is retained and the unencrypted-storage warning stays visible. In the web app, **Remember** still saves the key unencrypted in the browser's storage, with a note in settings. -A ChatGPT sign-in's tokens are kept in the system keychain, never in the preferences or a project -file, and never logged; RustGIS renews them as they expire. `chatgpt-auth.json` in the app's -settings folder keeps only the saved accounts (their client id, account, email, permissions, when -the tokens expire, and which one is selected), no token. A sign-in from an earlier version, whose -tokens are in that file, keeps working: they move to the keychain the first time they are used, -and leave the file only once the keychain has taken them. If the keychain is unavailable, they -stay in the file until it is, and the settings say so. A new sign-in the keychain can't take lasts -until RustGIS closes and is never written to the file; the settings say it won't be remembered. -**Sign out** revokes the session at OpenAI and deletes its tokens; **Remove account** also forgets -the account. The command line has no keychain (it uses a sign-in from an earlier version still in -the file), so its sign-ins last for the command only and it can't sign out an account whose tokens -are in the keychain. The ChatGPT provider's address is fixed, so only the OpenAI API receives the -tokens. Renewals are serialized across RustGIS processes (a refresh token can be used only once), -and so are the keychain's reads and writes. +A ChatGPT sign-in's refresh token is kept in the system keychain, never in the preferences or a +project file, and no token is ever logged. The short-lived access token stays in the memory of the +RustGIS window that got it, so each time RustGIS starts it renews the sign-in once before its first +request; after a restart, the browser may ask you to choose the account again when you sign in. +`chatgpt-auth.json` in the app's settings folder keeps only the saved accounts (their client id, +account, email, permissions, when the tokens expire, and which one is selected), no token. A +sign-in from an earlier version, whose tokens are in that file, keeps working: they move to the +keychain the first time they are used, and leave the file only once the keychain has taken them. +If the keychain is unavailable, they stay in the file until it is, and the settings say so. A new +sign-in the keychain can't take lasts until RustGIS closes and is never written to the file; the +settings say it won't be remembered, and it never replaces a sign-in the keychain still holds for +that account. **Sign out** revokes the session at OpenAI and deletes its token; if the keychain +can't be read, you are still signed out on this computer, and the settings say the session +couldn't be ended at OpenAI. **Remove account** also forgets the account, once any keychain item it +had is deleted. The command line has no keychain (it uses a sign-in from an earlier version still +in the file), so its sign-ins last for the command only, and it can't use, sign out or remove an +account whose token is in the keychain. The ChatGPT provider's address is fixed, so only the +OpenAI API receives the tokens. Renewals are serialized across RustGIS processes (a refresh token +can be used only once), and the keychain is read under the same lock. Plain HTTP is only used for servers on your computer or local network. In the web app, the browser calls the provider directly. Anthropic, OpenAI, Gemini and From 346fc784eb04bffcea8ac117ee8735db44900568 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 11:30:30 +0100 Subject: [PATCH 5/7] Assistant: name a keychain item and its revision before writing it; keep access-only file sign-ins in the file --- crates/assistant/src/chatgpt.rs | 119 +++++++++++++++++++++----------- docs/architecture.md | 3 +- 2 files changed, 82 insertions(+), 40 deletions(-) diff --git a/crates/assistant/src/chatgpt.rs b/crates/assistant/src/chatgpt.rs index 5f76bb32c..070d9b2c7 100644 --- a/crates/assistant/src/chatgpt.rs +++ b/crates/assistant/src/chatgpt.rs @@ -807,8 +807,9 @@ mod native { (c.client_id.clone(), c.subject.clone()) } - /// The keychain's value: the refresh token and the `rev` it is for. Kept small: Windows - /// Credential Manager takes at most 2560 bytes of UTF-16. + /// The keychain's value: the refresh token and the `rev` it is for, without the much longer + /// access and id tokens, for Windows Credential Manager's 2560 bytes of UTF-16 (a real refresh + /// token's length is not verified here). fn secret(c: &Credentials) -> Secret { Secret::new(serde_json::json!({"rev": c.rev, "refresh_token": c.refresh_token}).to_string()) } @@ -881,7 +882,9 @@ mod native { /// them; the file changes only then. Under the store lock. fn with_tokens(v: &Vault, path: &Path, accounts: &mut Accounts, entry: Credentials) -> Result { if entry.kept == Kept::SettingsFile { - let Some(store) = v.store() else { return Ok(entry) }; + // Without a refresh token there is nothing the keychain could renew it with: the + // file's access token keeps working until it expires. + let Some(store) = v.store().filter(|_| !entry.refresh_token.is_empty()) else { return Ok(entry) }; let moved = Credentials { rev: random_token()?, in_keychain: true, kept: Kept::Keychain, ..entry.clone() }; if let Err(e) = store.save(&secret_name(&entry.client_id, &entry.subject), &secret(&moved)) { log::warn!("the ChatGPT sign-in stays in the settings folder: {e}"); @@ -931,10 +934,11 @@ mod native { } /// Keeps a registration and its tokens, selects it and writes the file: the refresh token - /// goes to the keychain (the item noted in the file first). If the keychain doesn't take it, - /// tokens renewed from the file's own go back there, others last for this session; a new - /// sign-in (`new`) over an account the keychain still holds is refused instead. Without - /// tokens, any in the keychain are deleted. Gives the credentials as kept. Under the store lock. + /// goes to the keychain, and the file names it (and the item) before the keychain is written. + /// If the keychain doesn't take it (or there is none to take), tokens renewed from the file's + /// own go back there, others last for this session; a new sign-in (`new`) over an account the + /// keychain still holds is refused instead. Without tokens, any in the keychain are deleted. + /// Gives the credentials as kept. Under the store lock. fn keep_tokens(v: &Vault, path: &Path, accounts: &mut Accounts, c: &Credentials, new: bool) -> Result { let name = secret_name(&c.client_id, &c.subject); let store = v.store(); @@ -953,40 +957,49 @@ mod native { write_accounts(path, accounts)?; return Ok(entry); } - let mut kept = Credentials { rev: random_token()?, kept: Kept::Keychain, ..c.clone() }; + // Only tokens renewed from the file's own may go back there; a new sign-in never does. + let from_file = c.kept == Kept::SettingsFile && existing.as_ref().is_some_and(|e| e.kept == Kept::SettingsFile); + let before = Accounts { entries: accounts.entries.clone(), active: accounts.active.clone() }; + let mut kept = Credentials { rev: random_token()?, in_keychain: true, kept: Kept::Keychain, ..c.clone() }; + let attempted = store.is_some() && !c.refresh_token.is_empty(); let saved = match &store { + None => Err("no system keychain here".to_string()), + // Nothing outlasts the access token: nothing to keep. + Some(_) if !attempted => Err("the sign-in has no refresh token".to_string()), Some(store) => { - if !in_keychain { - // Noted before the item exists, so it is never left behind unreferenced. - in_keychain = true; - let mut marked = Accounts { entries: accounts.entries.clone(), active: accounts.active.clone() }; - match marked.entries.iter_mut().find(|e| id(e) == id(c)) { - Some(e) => e.in_keychain = true, - None => marked.entries.push(Credentials { rev: String::new(), in_keychain: true, ..c.without_tokens() }), - } - write_accounts(path, &marked)?; - *accounts = marked; - } + // Written first, so a failure after the keychain's write never leaves a token the + // file can't name. Tokens from the file stay in it until the keychain has them. + accounts.put(&if from_file { Credentials { kept: Kept::SettingsFile, ..kept.clone() } } else { kept.without_tokens() }); + write_accounts(path, accounts)?; store.save(&name, &secret(&kept)).map_err(|e| e.to_string()) } - None => Err("no system keychain here".to_string()), }; + if saved.is_ok() { + v.held().insert(id(c), Held::of(&kept)); + if from_file { + accounts.put(&kept.without_tokens()); + write_accounts(path, accounts)?; + } + return Ok(kept); + } if let Err(why) = saved { log::warn!("the system keychain didn't keep the ChatGPT sign-in: {why}"); - // Replacing it would sign the account out where the keychain holds it, unrevoked. - if new && existing.as_ref().is_some_and(|e| !e.rev.is_empty()) { - return Err(Error::new( - Recovery::Auth, - "the system keychain didn't keep the new sign-in, and it holds this account's earlier one; sign in again where the keychain is available", - )); + } + // Replacing it would sign the account out where the keychain holds it, unrevoked. + if new && existing.as_ref().is_some_and(|e| !e.rev.is_empty()) { + if attempted { + write_accounts(path, &before)?; } - // Only tokens renewed from the file's own may go back there; a new sign-in never does. - let from_file = c.kept == Kept::SettingsFile && existing.as_ref().is_some_and(|e| e.kept == Kept::SettingsFile); - kept.kept = if from_file { Kept::SettingsFile } else { Kept::Session }; - kept.rev = String::new(); + *accounts = before; + return Err(Error::new( + Recovery::Auth, + "the system keychain didn't keep the new sign-in, and it holds this account's earlier one; sign in again where the keychain is available", + )); } - kept.in_keychain = in_keychain; - if kept.kept == Kept::SettingsFile { + kept.kept = if from_file { Kept::SettingsFile } else { Kept::Session }; + kept.rev = String::new(); + kept.in_keychain = in_keychain || attempted; + if from_file { v.held().remove(&id(c)); accounts.put(&kept); } else { @@ -1866,8 +1879,8 @@ mod native { assert!(!entry[k].is_null(), "{k} stays in the file"); } assert_eq!(entry["keychain"], true); - // The keychain keeps the refresh token alone: small enough for Windows (2560 bytes of - // UTF-16). + // The keychain keeps the refresh token alone, not the access and id tokens, to stay + // within Windows' limit (a real refresh token's length is not verified by this test). let saved = store.load(NAME).unwrap().unwrap(); assert!( saved.expose().contains("synthetic-refresh") @@ -2036,7 +2049,8 @@ mod native { assert!(read_accounts(&path).unwrap().entries.is_empty()); } - /// A keychain that checks the file notes its item before it is written. + /// A keychain that checks the file already names each item, and its revision, before the + /// item is written: a failed file write after it can't leave a token the file can't name. struct Noting { inner: MemorySecrets, path: std::path::PathBuf, @@ -2047,7 +2061,9 @@ mod native { self.inner.load(account) } fn save(&self, account: &str, value: &Secret) -> Result<(), SecretError> { - assert_eq!(entry_json(&self.path)["keychain"], true, "noted before the keychain write"); + let saved: Value = serde_json::from_str(value.expose()).unwrap(); + let entry = entry_json(&self.path); + assert_eq!((entry["keychain"].clone(), entry["rev"].clone()), (Value::Bool(true), saved["rev"].clone())); self.inner.save(account, value) } fn delete(&self, account: &str) -> Result<(), SecretError> { @@ -2056,12 +2072,37 @@ mod native { } #[test] - fn a_keychain_item_is_noted_in_the_file_before_it_is_written() { + fn the_file_names_a_keychain_item_before_it_is_written() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("auth.json"); let v = Vault::new(Some(Arc::new(Noting { inner: MemorySecrets::new(), path: path.clone() }))); - save_at(&path, &v, &synthetic(now() + 100_000), None).unwrap(); - assert_eq!(read_credentials(&path, &v).unwrap().kept, Kept::Keychain); + // A sign-in, then a renewal of it. + save_at(&path, &v, &synthetic(0), None).unwrap(); + send_at(&test_request(), &synthetic(0).binding(), &path, &v, &mut |req| { + Ok(if req.url == super::super::TOKEN_URL { + grant(now() + 100_000, "rotated") + } else { + HttpResponse { status: 200, body: vec![], headers: vec![] } + }) + }) + .unwrap(); + let c = read_credentials(&path, &v).unwrap(); + assert_eq!((c.kept, load_refresh(&v, &c).unwrap().as_str()), (Kept::Keychain, "rotated")); + } + + #[test] + fn a_file_sign_in_without_a_refresh_token_stays_in_the_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let mut old = synthetic(now() + 100_000); + old.refresh_token.clear(); + let text = legacy_json(&old).to_string(); + std::fs::write(&path, &text).unwrap(); + let store = Arc::new(MemorySecrets::new()); + let c = read_credentials(&path, &sharing(&store)).unwrap(); + assert!(c.signed_in() && c.kept == Kept::SettingsFile); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text, "its access token keeps working"); + assert_eq!(store.load(NAME).unwrap(), None); } #[test] diff --git a/docs/architecture.md b/docs/architecture.md index 985270576..3e1efbb2e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -77,7 +77,8 @@ browser and the CLI, where callers keep a secret for the session only, as before - **ChatGPT sign-in.** The desktop app gives its store to `chatgpt::set_secret_store` at start. The keychain keeps only each registration's refresh token, `chatgpt//`, with the `rev` its entry in `chatgpt-auth.json` names (any other is out of date and never - used), small enough for Windows Credential Manager's 2560 bytes of UTF-16. Access and id + used), without the access and id tokens, to stay within Windows Credential Manager's 2560 bytes + of UTF-16 (a real refresh token's length is not verified yet). Access and id tokens stay with the process that got them; a process without one renews first. The keychain is read only to renew, sign out or remove, under the file's `store-lock` (and, to renew, the `renewal-lock`), so a rotating refresh token is never used twice. The entry's `keychain` flag is From c66177bac3173a6839530484d81a43f4055812ab Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 13:37:28 +0100 Subject: [PATCH 6/7] Assistant: a failed-request check that finds the sign-in gone clears like a sign-out; doc fixes --- crates/assistant/src/chatgpt.rs | 3 ++- crates/ui-egui/src/assistantpane.rs | 21 ++++++++++++++++----- docs/cli.md | 4 +++- docs/user-guide/assistant.md | 3 ++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/crates/assistant/src/chatgpt.rs b/crates/assistant/src/chatgpt.rs index 070d9b2c7..7c10d7d45 100644 --- a/crates/assistant/src/chatgpt.rs +++ b/crates/assistant/src/chatgpt.rs @@ -1022,7 +1022,8 @@ mod native { keep_tokens(v, path, &mut read_accounts(path)?, c, false) } - /// The selected registration, including signed-out accounts. Never reads the keychain. + /// The selected registration, including signed-out accounts. Reads the keychain only to move + /// an older version's tokens into it (once); otherwise only the file. pub fn load_any() -> Option { let path = auth_path()?; let _guard = store(&path).ok()?; diff --git a/crates/ui-egui/src/assistantpane.rs b/crates/ui-egui/src/assistantpane.rs index 61f426c15..757ff728c 100644 --- a/crates/ui-egui/src/assistantpane.rs +++ b/crates/ui-egui/src/assistantpane.rs @@ -301,13 +301,12 @@ fn poll_sign_in(app: &mut RustGisApp, ctx: &egui::Context) { if signed_in_somewhere || cfg.binding() != binding || cfg.key.is_empty() { return; } + // The same as signing out here: no key, account or catalog, and a model list still on + // its way is dropped. let mut next = cfg; - next.key.clear(); + forget_sign_in(app, &mut next); apply_config(app, next); - app.assistant.account = None; - app.assistant.welcome = false; app.assistant.settings = true; - app.assistant.accounts = None; return; } let mut next = cfg.clone(); @@ -2505,10 +2504,22 @@ mod tests { finish(&mut app, Some(first.binding()), Some(creds("second"))); assert_eq!(app.assistant.agent.as_ref().unwrap().config, first); assert!(app.assistant.retry.is_some()); - // One that finds the sign-in gone clears it and stops the work bound to it. + // One that finds the sign-in gone clears it and stops the work bound to it, and a model + // list still on its way can't bring its catalog back. + let late: Reply = Slot::default(); + app.assistant.models_slot = Some(late.clone()); + app.assistant.models = vec!["first-model".into()]; finish(&mut app, Some(first.binding()), None); assert!(app.assistant.agent.as_ref().unwrap().config.key.is_empty()); assert!(app.assistant.retry.is_none() && app.assistant.settings); + assert!(app.assistant.models.is_empty() && app.assistant.models_slot.is_none()); + *late.lock().unwrap() = Some(Ok(HttpResponse { + status: 200, + headers: vec![], + body: serde_json::json!({"models": [{"slug": "late", "visibility": "list"}]}).to_string().into_bytes(), + })); + poll(&mut app, &ctx); + assert!(app.assistant.models.is_empty() && app.assistant.catalog.is_empty()); // Taking the selected account: another one stops work for the first and drops its catalog. apply_config(&mut app, first.clone()); app.assistant.retry = Some((2, web_time::Instant::now())); diff --git a/docs/cli.md b/docs/cli.md index 975630c40..e1a422f5d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -112,7 +112,9 @@ plan. `--another-account` starts a new registration, `--accounts` lists saved re `--account ` selects one, `--remove-account ` signs it out and forgets it, and `--logout` signs out. The command line has no system keychain: it uses a sign-in an earlier version saved in the settings folder, but not one the desktop app keeps in the keychain, and its -own sign-ins last for the command only (it can still sign them out and remove them later). +own sign-ins last for the command only. A later `--logout` or `--remove-account` of such a sign-in +clears it on this computer but can't end its session at OpenAI (its token is gone); disconnect +RustGIS in ChatGPT's settings for that. `--logout` and `--remove-account` refuse an account whose token is in the keychain (sign out or remove it in the app), so it is never left there unrevoked, and `--login` never replaces such a sign-in. See [Signing in with ChatGPT](user-guide/assistant.md#signing-in-with-chatgpt). diff --git a/docs/user-guide/assistant.md b/docs/user-guide/assistant.md index 662375fdd..9dd1a6083 100644 --- a/docs/user-guide/assistant.md +++ b/docs/user-guide/assistant.md @@ -301,7 +301,8 @@ settings say it won't be remembered, and it never replaces a sign-in the keychai that account. **Sign out** revokes the session at OpenAI and deletes its token; if the keychain can't be read, you are still signed out on this computer, and the settings say the session couldn't be ended at OpenAI. **Remove account** also forgets the account, once any keychain item it -had is deleted. The command line has no keychain (it uses a sign-in from an earlier version still +had is deleted: while the keychain can't be read or can't delete it, removal refuses, so no token +is left behind in it. The command line has no keychain (it uses a sign-in from an earlier version still in the file), so its sign-ins last for the command only, and it can't use, sign out or remove an account whose token is in the keychain. The ChatGPT provider's address is fixed, so only the OpenAI API receives the tokens. Renewals are serialized across RustGIS processes (a refresh token From acbfb560e056e910b653e4f8ee36c481db544ea3 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Sun, 25 Oct 2026 14:39:48 +0100 Subject: [PATCH 7/7] Assistant: nothing queued goes out after a sign-in check signs out; CLI --logout says when it only signed out locally --- apps/rustgis-cli/src/main.rs | 9 ++++++--- crates/ui-egui/src/assistantpane.rs | 14 +++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/apps/rustgis-cli/src/main.rs b/apps/rustgis-cli/src/main.rs index 6910dd779..710f51dcf 100644 --- a/apps/rustgis-cli/src/main.rs +++ b/apps/rustgis-cli/src/main.rs @@ -276,9 +276,12 @@ fn ask(s: &mut Session, args: &[String]) -> Result<(), String> { ); return Ok(()); } - if let Some(r) = out.revocation - && let Err(e) = chatgpt::revoke(&r) - { + let Some(r) = out.revocation else { + // No token here to end a session with (none, or a command-line sign-in that has ended). + eprintln!("signed out on this computer; no session could be ended at OpenAI from here"); + return Ok(()); + }; + if let Err(e) = chatgpt::revoke(&r) { eprintln!( "signed out on this computer, but OpenAI didn't confirm it ({e}); disconnect RustGIS in ChatGPT's settings if needed" ); diff --git a/crates/ui-egui/src/assistantpane.rs b/crates/ui-egui/src/assistantpane.rs index 757ff728c..38e43adf9 100644 --- a/crates/ui-egui/src/assistantpane.rs +++ b/crates/ui-egui/src/assistantpane.rs @@ -1219,11 +1219,16 @@ fn revoke_later(app: &mut RustGisApp, revocation: Option, c app.assistant.signout = Some(slot); } -/// After signing out or removing the account: no tokens, no catalog. +/// After signing out or removing the account: no tokens, no catalog, and nothing waiting to be +/// sent with them (a queued message goes back to the input box, not to the provider). #[cfg(not(target_arch = "wasm32"))] fn forget_sign_in(app: &mut RustGisApp, cfg: &mut Config) { app.assistant.sign_in_read = None; app.assistant.models_slot = None; + app.assistant.catalog_pending = false; + if let Some(text) = app.assistant.queued.take() { + app.assistant.input = text; + } cfg.key.clear(); app.assistant.account = None; app.assistant.models.clear(); @@ -2509,6 +2514,9 @@ mod tests { let late: Reply = Slot::default(); app.assistant.models_slot = Some(late.clone()); app.assistant.models = vec!["first-model".into()]; + // A message typed while the check was under way, and a catalog request it deferred. + app.assistant.queued = Some("synthetic question".into()); + app.assistant.catalog_pending = true; finish(&mut app, Some(first.binding()), None); assert!(app.assistant.agent.as_ref().unwrap().config.key.is_empty()); assert!(app.assistant.retry.is_none() && app.assistant.settings); @@ -2520,6 +2528,10 @@ mod tests { })); poll(&mut app, &ctx); assert!(app.assistant.models.is_empty() && app.assistant.catalog.is_empty()); + // Neither goes out after the sign-out; the message is back in the input box. + assert!(app.assistant.queued.is_none() && !app.assistant.catalog_pending); + assert!(app.assistant.inflight.is_none() && app.assistant.models_slot.is_none()); + assert_eq!(app.assistant.input, "synthetic question"); // Taking the selected account: another one stops work for the first and drops its catalog. apply_config(&mut app, first.clone()); app.assistant.retry = Some((2, web_time::Instant::now()));