From 730ed43af92b79f21116b0562b56ffc63097170c Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Tue, 26 May 2026 20:05:25 +0100 Subject: [PATCH] Fix login loop (#6402) --- .../editor/src-tauri/src/commands/auth.rs | 119 +++++++++++------- frontend/editor/src-tauri/src/lib.rs | 2 +- .../src-tauri/tests/refresh_token_fallback.rs | 94 ++++++++++++++ 3 files changed, 172 insertions(+), 43 deletions(-) create mode 100644 frontend/src-tauri/tests/refresh_token_fallback.rs diff --git a/frontend/editor/src-tauri/src/commands/auth.rs b/frontend/editor/src-tauri/src/commands/auth.rs index 528f6d8de..28d2ddd37 100644 --- a/frontend/editor/src-tauri/src/commands/auth.rs +++ b/frontend/editor/src-tauri/src/commands/auth.rs @@ -1,7 +1,7 @@ use keyring::{Entry}; use serde::{Deserialize, Serialize}; use std::sync::{Arc, Mutex}; -use tauri::AppHandle; +use tauri::{AppHandle, Runtime}; use tauri_plugin_store::StoreExt; use tiny_http::{Response, Server}; use sha2::{Sha256, Digest}; @@ -18,6 +18,9 @@ const KEYRING_SERVICE: &str = "stirling-pdf"; const KEYRING_TOKEN_KEY: &str = "auth-token"; const KEYRING_REFRESH_TOKEN_KEY: &str = "refresh-token"; +pub const TOKENS_STORE_FILE_FOR_TESTS: &str = TOKENS_STORE_FILE; +pub const REFRESH_TOKEN_STORE_KEY_FOR_TESTS: &str = REFRESH_TOKEN_STORE_KEY; + #[derive(Debug, Serialize, Deserialize, Clone)] pub struct UserInfo { pub username: String, @@ -25,6 +28,9 @@ pub struct UserInfo { } fn get_keyring_entry() -> Result { + if std::env::var("STIRLING_PDF_TEST_FORCE_AUTH_KEYRING_FAIL").is_ok() { + return Err("Forced keyring failure for tests".to_string()); + } log::debug!("Creating keyring entry with service='{}' username='{}'", KEYRING_SERVICE, KEYRING_TOKEN_KEY); let entry = Entry::new(KEYRING_SERVICE, KEYRING_TOKEN_KEY) .map_err(|e| { @@ -35,11 +41,28 @@ fn get_keyring_entry() -> Result { Ok(entry) } -fn get_refresh_token_keyring_entry() -> Result { +pub fn get_refresh_token_keyring_entry() -> Result { + if std::env::var("STIRLING_PDF_TEST_FORCE_REFRESH_KEYRING_FAIL").is_ok() { + return Err("Forced keyring failure for tests".to_string()); + } Entry::new(KEYRING_SERVICE, KEYRING_REFRESH_TOKEN_KEY) .map_err(|e| format!("Failed to access keyring: {}", e)) } +/// Returns Ok(true) when keyring round-tripped the token; Ok(false) otherwise (caller must fall through to disk). +pub fn try_save_refresh_token_to_keyring(token: &str) -> Result { + match get_refresh_token_keyring_entry() { + Ok(entry) => match entry.set_password(token) { + Ok(_) => match entry.get_password() { + Ok(saved) if saved == token => Ok(true), + _ => Ok(false), + }, + Err(_) => Ok(false), + }, + Err(_) => Ok(false), + } +} + #[tauri::command] pub async fn save_auth_token(app_handle: AppHandle, token: String) -> Result<(), String> { let trimmed = token.trim(); @@ -172,31 +195,30 @@ pub async fn clear_auth_token(app_handle: AppHandle) -> Result<(), String> { } #[tauri::command] -pub async fn save_refresh_token(app_handle: AppHandle, token: String) -> Result<(), String> { +pub async fn save_refresh_token(app_handle: AppHandle, token: String) -> Result<(), String> { log::info!("Saving refresh token - trying keyring first"); - let entry = get_refresh_token_keyring_entry()?; - - // Try keyring (works in production with code signing) - match entry.set_password(&token) { - Ok(_) => { - // Verify it persists (fails in unsigned dev builds) - match entry.get_password() { - Ok(saved) if saved == token => { - log::info!("✅ Refresh token saved to keyring (production mode)"); - return Ok(()); - } - _ => { - log::info!("Keyring doesn't persist - using Tauri Store fallback (dev mode)"); + match try_save_refresh_token_to_keyring(&token) { + Ok(true) => { + // Clear any stale fallback copy so the keyring stays authoritative. + if let Ok(store) = app_handle.store(TOKENS_STORE_FILE) { + if store.get(REFRESH_TOKEN_STORE_KEY).is_some() { + store.delete(REFRESH_TOKEN_STORE_KEY); + let _ = store.save(); } } + log::info!("Refresh token saved to keyring"); + return Ok(()); + } + Ok(false) => { + log::info!("Keyring did not persist refresh token - using Tauri Store fallback"); } Err(e) => { - log::info!("Keyring failed: {} - using Tauri Store fallback", e); + log::info!("Keyring error for refresh token: {} - using Tauri Store fallback", e); } } - // Fallback to Tauri Store (dev mode without code signing) + // Fallback to Tauri Store (dev mode without code signing, or restricted environments) let store = app_handle .store(TOKENS_STORE_FILE) .map_err(|e| format!("Failed to access tokens store: {}", e))?; @@ -211,28 +233,33 @@ pub async fn save_refresh_token(app_handle: AppHandle, token: String) -> Result< .save() .map_err(|e| format!("Failed to save tokens store: {}", e))?; - log::info!("✅ Refresh token saved to Tauri Store (fallback)"); + log::info!("Refresh token saved to Tauri Store (fallback)"); Ok(()) } #[tauri::command] -pub async fn get_refresh_token(app_handle: AppHandle) -> Result, String> { - // Try keyring first (production) - let entry = get_refresh_token_keyring_entry()?; - match entry.get_password() { - Ok(token) => { - log::info!("✅ Refresh token retrieved from keyring"); - return Ok(Some(token)); - } - Err(keyring::Error::NoEntry) => { - log::debug!("No token in keyring, trying Tauri Store"); - } +pub async fn get_refresh_token(app_handle: AppHandle) -> Result, String> { + // Try keyring first (production / unrestricted environments). Any failure - + // including entry creation - falls through to the Tauri Store fallback below. + match get_refresh_token_keyring_entry() { + Ok(entry) => match entry.get_password() { + Ok(token) => { + log::info!("Refresh token retrieved from keyring"); + return Ok(Some(token)); + } + Err(keyring::Error::NoEntry) => { + log::debug!("No refresh token in keyring, trying Tauri Store"); + } + Err(e) => { + log::warn!("Keyring error reading refresh token: {} - trying Tauri Store", e); + } + }, Err(e) => { - log::warn!("Keyring error: {} - trying Tauri Store", e); + log::warn!("Keyring entry unavailable for refresh token: {} - trying Tauri Store", e); } } - // Fallback to Tauri Store (dev) + // Fallback to Tauri Store (dev or restricted environments) let store = app_handle .store(TOKENS_STORE_FILE) .map_err(|e| format!("Failed to access tokens store: {}", e))?; @@ -242,7 +269,7 @@ pub async fn get_refresh_token(app_handle: AppHandle) -> Result, .and_then(|v| serde_json::from_value(v.clone()).ok()); if token.is_some() { - log::info!("✅ Refresh token retrieved from Tauri Store"); + log::info!("Refresh token retrieved from Tauri Store"); } else { log::info!("No refresh token found"); } @@ -251,18 +278,26 @@ pub async fn get_refresh_token(app_handle: AppHandle) -> Result, } #[tauri::command] -pub async fn clear_refresh_token(app_handle: AppHandle) -> Result<(), String> { +pub async fn clear_refresh_token(app_handle: AppHandle) -> Result<(), String> { log::info!("Clearing refresh token from all storage"); - // Clear from keyring - let entry = get_refresh_token_keyring_entry()?; - match entry.delete_credential() { - Ok(_) => log::info!("Cleared from keyring"), - Err(keyring::Error::NoEntry) => log::debug!("Not in keyring"), - Err(e) => log::warn!("Keyring clear error: {}", e), + // Best-effort keyring clear; never blocks disk clear. + match get_refresh_token_keyring_entry() { + Ok(entry) => match entry.delete_credential() { + Ok(_) | Err(keyring::Error::NoEntry) => {} + Err(e) => { + log::warn!("Failed to delete keyring refresh token: {}. Attempting overwrite with empty token.", e); + if let Err(e2) = entry.set_password("") { + log::warn!("Failed to overwrite keyring refresh token: {}", e2); + } + } + }, + Err(e) => { + log::warn!("Keyring entry unavailable while clearing refresh token: {} - clearing Tauri Store fallback only", e); + } } - // Clear from Tauri Store + // Clear from Tauri Store fallback let store = app_handle .store(TOKENS_STORE_FILE) .map_err(|e| format!("Failed to access tokens store: {}", e))?; @@ -273,7 +308,7 @@ pub async fn clear_refresh_token(app_handle: AppHandle) -> Result<(), String> { .save() .map_err(|e| format!("Failed to save tokens store: {}", e))?; - log::info!("✅ Refresh token cleared"); + log::info!("Refresh token cleared"); Ok(()) } diff --git a/frontend/editor/src-tauri/src/lib.rs b/frontend/editor/src-tauri/src/lib.rs index 08986b2d3..f06633c22 100644 --- a/frontend/editor/src-tauri/src/lib.rs +++ b/frontend/editor/src-tauri/src/lib.rs @@ -1,7 +1,7 @@ use tauri::{AppHandle, Emitter, Manager, RunEvent, WindowEvent}; mod utils; -mod commands; +pub mod commands; mod state; use commands::{ diff --git a/frontend/src-tauri/tests/refresh_token_fallback.rs b/frontend/src-tauri/tests/refresh_token_fallback.rs new file mode 100644 index 000000000..8172e792d --- /dev/null +++ b/frontend/src-tauri/tests/refresh_token_fallback.rs @@ -0,0 +1,94 @@ +//! Asserts the refresh-token keyring helper falls through (Ok(false)) +//! instead of bailing (Err) when keyring access fails. This was the +//! original login-loop bug. + +use app_lib::commands::auth::{ + get_refresh_token_keyring_entry, try_save_refresh_token_to_keyring, + REFRESH_TOKEN_STORE_KEY_FOR_TESTS, TOKENS_STORE_FILE_FOR_TESTS, +}; +use serde_json::json; + +const ENV_FORCE_FAIL: &str = "STIRLING_PDF_TEST_FORCE_REFRESH_KEYRING_FAIL"; + +// Single test fn — env var is process-global; parallel tests would race. +#[test] +fn refresh_token_fallback_proof() { + // ===== Step 1: env-var injection forces keyring entry-creation to Err ===== + std::env::set_var(ENV_FORCE_FAIL, "1"); + + let entry_result = get_refresh_token_keyring_entry(); + assert!( + entry_result.is_err(), + "with {} set, get_refresh_token_keyring_entry must return Err", + ENV_FORCE_FAIL + ); + let err_msg = entry_result.err().unwrap(); + assert!( + err_msg.contains("Forced keyring failure for tests"), + "expected forced-failure marker; got: {}", + err_msg + ); + + // ===== Step 2: the bug-fix proof ===== + // Original bug: `let entry = get_refresh_token_keyring_entry()?;` would + // propagate Err and short-circuit save_refresh_token, never reaching the + // disk-store fallback. Fixed behaviour: the same keyring failure must be + // converted into a "fall through" signal (Ok(false)), NOT propagated as Err. + let outcome = try_save_refresh_token_to_keyring("test-token-xyz-456"); + assert!( + matches!(outcome, Ok(false)), + "REGRESSION: keyring entry-creation failure must convert to Ok(false) \ + so save_refresh_token falls through to the disk store. Got {:?}. \ + This means the function would short-circuit and the user's refresh \ + token would never be persisted - exactly the original login-loop bug.", + outcome + ); + + std::env::remove_var(ENV_FORCE_FAIL); + + // ===== Step 3: env-var unset does NOT carry the forced-failure marker ===== + let result_unset = get_refresh_token_keyring_entry(); + if let Err(e) = &result_unset { + assert!( + !e.contains("Forced keyring failure for tests"), + "with {} unset, error must NOT contain forced-failure marker; got: {}", + ENV_FORCE_FAIL, + e + ); + } + + // ===== Step 4: disk-fallback JSON round-trips ===== + // tauri_plugin_store writes via serde_json::to_vec_pretty(&HashMap). Reproduce that format and verify it round-trips, proving + // the file save_refresh_token's disk fallback writes is readable by + // get_refresh_token's disk fallback. + let tmp = std::env::temp_dir().join(format!( + "stirling_pdf_refresh_token_test_{}.json", + std::process::id() + )); + let token = "test-refresh-token-disk-roundtrip-abc-123"; + let mut map = std::collections::HashMap::::new(); + map.insert(REFRESH_TOKEN_STORE_KEY_FOR_TESTS.to_string(), json!(token)); + let bytes = serde_json::to_vec_pretty(&map).expect("serialize tokens map"); + std::fs::write(&tmp, &bytes).expect("write tokens file"); + + let read_bytes = std::fs::read(&tmp).expect("read tokens file"); + let read_map: std::collections::HashMap = + serde_json::from_slice(&read_bytes).expect("parse tokens json"); + let retrieved: Option = read_map + .get(REFRESH_TOKEN_STORE_KEY_FOR_TESTS) + .and_then(|v| serde_json::from_value(v.clone()).ok()); + + let _ = std::fs::remove_file(&tmp); + + assert_eq!( + retrieved, + Some(token.to_string()), + "disk-fallback JSON format must round-trip the refresh token (file: {})", + tmp.display() + ); + + // ===== Step 5: constants match production strings ===== + assert_eq!(TOKENS_STORE_FILE_FOR_TESTS, "tokens.json"); + assert_eq!(REFRESH_TOKEN_STORE_KEY_FOR_TESTS, "refresh_token"); +}