From 232c39da1619bd05583d4a13b94ec693892f1d0e Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 9 May 2026 22:56:52 +0700 Subject: [PATCH] fix(storage): distinguish keychain user cancellation, auth failure, and error status from not-found --- CHANGELOG.md | 1 + TablePro/Core/Storage/AIKeyStorage.swift | 16 +++++++++--- TablePro/Core/Storage/ConnectionStorage.swift | 17 ++++++++++--- TablePro/Core/Storage/KeychainHelper.swift | 25 ++++++++++++++----- TablePro/Core/Storage/LicenseStorage.swift | 13 ++++++++-- TablePro/Core/Storage/SSHProfileStorage.swift | 16 +++++++++--- 6 files changed, 68 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cc4d4f35..b3c3bd02b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - When saving the connections file fails (disk full, sandbox denied, encoding error), TablePro now aborts the dependent steps instead of continuing as if the save had succeeded. Previously a failed delete could remove the keychain password and queue a CloudKit tombstone for a record that was still on disk; on next sync the connection would be nuked from iCloud as well. Now: delete, add, update, duplicate, batch-delete, sync-pull, group-cleanup, and the plugin secure field migration all check the persistence result and skip the downstream side effects on failure. The connection form surfaces a localized error and keeps the form open instead of dismissing. - iCloud sync no longer silently substitutes empty defaults when an SSH config, SSL config, jump-host list, or driver-specific field stored in a synced record fails to decode. A device on an older app build that pulled a record written by a newer build would previously end up with a connection missing its SSH/SSL config, then push that empty config back to iCloud and overwrite the authoritative copy. Decode failures now skip the record entirely and log which field failed; the cloud copy stays intact until the device is updated. - iCloud sync of app settings (general, appearance, editor, data grid, history, tabs, keyboard, AI) no longer silently does nothing when a category's payload fails to decode. Each of the eight category branches previously wrapped the decode in `try?`, so a record written by a newer schema version would fall through with no log, no error, and no UI signal: the user would think their settings synced when they hadn't. Decode failures now skip the category and log which one failed and why. +- Keychain reads no longer collapse a cancelled Touch ID prompt, a failed biometric auth, or any unknown OSStatus into "not found". The `KeychainResult` enum now distinguishes `.userCancelled`, `.authFailed`, and `.error(OSStatus)` from `.notFound`, and the read paths in connection passwords, SSH profile secrets, AI provider keys, and the license key log each case with its own message. Previously a cancelled prompt looked identical to a missing entry, so the caller would treat the password as gone and silently re-save with an empty string on the next write, producing duplicate keychain entries or a connection saved with a blank password. - Result-grid cells on rows marked for deletion keep their dropdown / date / JSON / blob chevron visible at reduced opacity instead of hiding it, so the cell type is still legible while clearly inactive. Click on the dimmed chevron is a no-op; FK arrow navigation is unchanged. Matches the macOS HIG "disabled appearance" guideline. - Foreign key navigation from a table with unsaved edits opens the referenced table in a new window tab to preserve the edit buffer. Closing that new tab no longer wipes the original tab's data grid. Previously the new tab's teardown broadcast a connection-scoped event that other coordinators on the same connection received, causing them to release their cell data. - Tables sidebar refreshes automatically after a successful SQL import; the refresh notification now fires after the success sheet's dismissal animation, so the main window is key when the observer runs (#1114) diff --git a/TablePro/Core/Storage/AIKeyStorage.swift b/TablePro/Core/Storage/AIKeyStorage.swift index d8db36f9f..2c58a9c34 100644 --- a/TablePro/Core/Storage/AIKeyStorage.swift +++ b/TablePro/Core/Storage/AIKeyStorage.swift @@ -23,15 +23,23 @@ final class AIKeyStorage { func loadAPIKey(for providerID: UUID) -> String? { let key = "com.TablePro.aikey.\(providerID.uuidString)" + let pid = providerID.uuidString switch KeychainHelper.shared.readStringResult(forKey: key) { case .found(let value): return value + case .notFound: + return nil case .locked: - Self.logger.warning( - "AI API key unavailable — Keychain locked (providerID=\(providerID.uuidString, privacy: .public))" - ) + Self.logger.warning("AI API key unavailable: Keychain locked (providerID=\(pid, privacy: .public))") return nil - case .notFound: + case .userCancelled: + Self.logger.notice("AI API key prompt cancelled (providerID=\(pid, privacy: .public))") + return nil + case .authFailed: + Self.logger.warning("AI API key auth failed (providerID=\(pid, privacy: .public))") + return nil + case .error(let status): + Self.logger.error("AI API key read error \(status) (providerID=\(pid, privacy: .public))") return nil } } diff --git a/TablePro/Core/Storage/ConnectionStorage.swift b/TablePro/Core/Storage/ConnectionStorage.swift index 0668185c4..ec64ca447 100644 --- a/TablePro/Core/Storage/ConnectionStorage.swift +++ b/TablePro/Core/Storage/ConnectionStorage.swift @@ -382,15 +382,24 @@ final class ConnectionStorage { } private func resolveString(_ context: SecretContext, forKey key: String) -> String? { + let label = context.label + let connId = context.connectionId.uuidString switch KeychainHelper.shared.readStringResult(forKey: key) { case .found(let value): return value + case .notFound: + return nil case .locked: - Self.logger.warning( - "\(context.label, privacy: .public) unavailable — Keychain locked (connId=\(context.connectionId.uuidString, privacy: .public))" - ) + Self.logger.warning("\(label, privacy: .public) unavailable: Keychain locked (connId=\(connId, privacy: .public))") return nil - case .notFound: + case .userCancelled: + Self.logger.notice("\(label, privacy: .public) prompt cancelled (connId=\(connId, privacy: .public))") + return nil + case .authFailed: + Self.logger.warning("\(label, privacy: .public) auth failed (connId=\(connId, privacy: .public))") + return nil + case .error(let status): + Self.logger.error("\(label, privacy: .public) read error \(status) (connId=\(connId, privacy: .public))") return nil } } diff --git a/TablePro/Core/Storage/KeychainHelper.swift b/TablePro/Core/Storage/KeychainHelper.swift index b6649897d..3131048b6 100644 --- a/TablePro/Core/Storage/KeychainHelper.swift +++ b/TablePro/Core/Storage/KeychainHelper.swift @@ -11,12 +11,18 @@ enum KeychainResult: Sendable, Equatable { case found(Data) case notFound case locked + case userCancelled + case authFailed + case error(OSStatus) } enum KeychainStringResult: Sendable, Equatable { case found(String) case notFound case locked + case userCancelled + case authFailed + case error(OSStatus) } final class KeychainHelper: Sendable { @@ -101,9 +107,15 @@ final class KeychainHelper: Sendable { case errSecInteractionNotAllowed: Self.logger.warning("Keychain locked (before first unlock) for '\(key, privacy: .public)'") return .locked + case errSecUserCanceled: + Self.logger.notice("Keychain prompt cancelled for '\(key, privacy: .public)'") + return .userCancelled + case errSecAuthFailed: + Self.logger.warning("Keychain auth failed for '\(key, privacy: .public)'") + return .authFailed default: log(status: status, operation: "read", key: key) - return .notFound + return .error(status) } } @@ -140,13 +152,14 @@ final class KeychainHelper: Sendable { case .found(let data): guard let value = String(data: data, encoding: .utf8) else { Self.logger.error("UTF-8 decode failed for '\(key, privacy: .public)'") - return .notFound + return .error(errSecDecode) } return .found(value) - case .notFound: - return .notFound - case .locked: - return .locked + case .notFound: return .notFound + case .locked: return .locked + case .userCancelled: return .userCancelled + case .authFailed: return .authFailed + case .error(let status): return .error(status) } } diff --git a/TablePro/Core/Storage/LicenseStorage.swift b/TablePro/Core/Storage/LicenseStorage.swift index 4b4f8339f..7653d27bc 100644 --- a/TablePro/Core/Storage/LicenseStorage.swift +++ b/TablePro/Core/Storage/LicenseStorage.swift @@ -34,10 +34,19 @@ final class LicenseStorage { switch KeychainHelper.shared.readStringResult(forKey: Keys.keychainLicenseKey) { case .found(let value): return value + case .notFound: + return nil case .locked: - Self.logger.warning("License key unavailable — Keychain locked") + Self.logger.warning("License key unavailable: Keychain locked") return nil - case .notFound: + case .userCancelled: + Self.logger.notice("License key prompt cancelled") + return nil + case .authFailed: + Self.logger.warning("License key auth failed") + return nil + case .error(let status): + Self.logger.error("License key read error \(status)") return nil } } diff --git a/TablePro/Core/Storage/SSHProfileStorage.swift b/TablePro/Core/Storage/SSHProfileStorage.swift index f438ebcdd..ae500d9b9 100644 --- a/TablePro/Core/Storage/SSHProfileStorage.swift +++ b/TablePro/Core/Storage/SSHProfileStorage.swift @@ -145,15 +145,23 @@ final class SSHProfileStorage { } private func resolveString(label: String, profileId: UUID, forKey key: String) -> String? { + let pid = profileId.uuidString switch KeychainHelper.shared.readStringResult(forKey: key) { case .found(let value): return value + case .notFound: + return nil case .locked: - Self.logger.warning( - "\(label, privacy: .public) unavailable — Keychain locked (profileId=\(profileId.uuidString, privacy: .public))" - ) + Self.logger.warning("\(label, privacy: .public) unavailable: Keychain locked (profileId=\(pid, privacy: .public))") return nil - case .notFound: + case .userCancelled: + Self.logger.notice("\(label, privacy: .public) prompt cancelled (profileId=\(pid, privacy: .public))") + return nil + case .authFailed: + Self.logger.warning("\(label, privacy: .public) auth failed (profileId=\(pid, privacy: .public))") + return nil + case .error(let status): + Self.logger.error("\(label, privacy: .public) read error \(status) (profileId=\(pid, privacy: .public))") return nil } }