Skip to content

Commit 04e496a

Browse files
committed
fix: address review issues in iCloud Keychain password sync
1 parent f98b745 commit 04e496a

3 files changed

Lines changed: 19 additions & 5 deletions

File tree

TablePro/AppDelegate.swift

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,13 @@ class AppDelegate: NSObject, NSApplicationDelegate {
5555
KeychainHelper.shared.migrateFromLegacyKeychainIfNeeded()
5656
let syncSettings = AppSettingsStorage.shared.loadSync()
5757
let passwordSyncExpected = syncSettings.enabled && syncSettings.syncConnections && syncSettings.syncPasswords
58-
UserDefaults.standard.set(passwordSyncExpected, forKey: "com.TablePro.keychainPasswordSyncEnabled")
58+
let previousSyncState = UserDefaults.standard.bool(forKey: KeychainHelper.passwordSyncEnabledKey)
59+
UserDefaults.standard.set(passwordSyncExpected, forKey: KeychainHelper.passwordSyncEnabledKey)
60+
if passwordSyncExpected != previousSyncState {
61+
Task.detached(priority: .background) {
62+
KeychainHelper.shared.migratePasswordSyncState(synchronizable: passwordSyncExpected)
63+
}
64+
}
5965
PluginManager.shared.loadPlugins()
6066

6167
Task { @MainActor in

TablePro/Core/Storage/KeychainHelper.swift

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ final class KeychainHelper {
1313
private let service = "com.TablePro"
1414
private static let logger = Logger(subsystem: "com.TablePro", category: "KeychainHelper")
1515
private static let migrationKey = "com.TablePro.keychainMigratedToDataProtection"
16-
private static let passwordSyncEnabledKey = "com.TablePro.keychainPasswordSyncEnabled"
16+
static let passwordSyncEnabledKey = "com.TablePro.keychainPasswordSyncEnabled"
17+
18+
private let migrationLock = NSLock()
1719

1820
private var isPasswordSyncEnabled: Bool {
1921
UserDefaults.standard.bool(forKey: Self.passwordSyncEnabledKey)
@@ -40,6 +42,7 @@ final class KeychainHelper {
4042
var status = SecItemAdd(addQuery as CFDictionary, nil)
4143

4244
if status == errSecDuplicateItem {
45+
let synchronizable = isPasswordSyncEnabled
4346
let searchQuery: [String: Any] = [
4447
kSecClass as String: kSecClassGenericPassword,
4548
kSecAttrService as String: service,
@@ -48,7 +51,8 @@ final class KeychainHelper {
4851
kSecAttrSynchronizable as String: kSecAttrSynchronizableAny
4952
]
5053
let updateAttributes: [String: Any] = [
51-
kSecValueData as String: data
54+
kSecValueData as String: data,
55+
kSecAttrSynchronizable as String: synchronizable
5256
]
5357
status = SecItemUpdate(searchQuery as CFDictionary, updateAttributes as CFDictionary)
5458
}
@@ -195,7 +199,11 @@ final class KeychainHelper {
195199
// MARK: - Password Sync Migration
196200

197201
/// Migrates all TablePro keychain items between local-only and iCloud-synchronizable.
202+
/// Serialized via `migrationLock` to prevent concurrent migrations from rapid toggling.
198203
func migratePasswordSyncState(synchronizable: Bool) {
204+
migrationLock.lock()
205+
defer { migrationLock.unlock() }
206+
199207
Self.logger.info("Starting keychain sync migration: synchronizable=\(synchronizable)")
200208

201209
let searchQuery: [String: Any] = [
@@ -260,7 +268,7 @@ final class KeychainHelper {
260268
kSecAttrService as String: service,
261269
kSecAttrAccount as String: account,
262270
kSecUseDataProtectionKeychain as String: true,
263-
kSecAttrSynchronizable as String: !synchronizable as CFBoolean
271+
kSecAttrSynchronizable as String: !synchronizable
264272
]
265273
let deleteStatus = SecItemDelete(deleteQuery as CFDictionary)
266274
if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound {

TablePro/Views/Settings/SyncSettingsView.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,9 @@ struct SyncSettingsView: View {
178178
}
179179

180180
private func onPasswordSyncChanged(_ enabled: Bool) {
181-
UserDefaults.standard.set(enabled, forKey: "com.TablePro.keychainPasswordSyncEnabled")
182181
Task.detached {
183182
KeychainHelper.shared.migratePasswordSyncState(synchronizable: enabled)
183+
UserDefaults.standard.set(enabled, forKey: KeychainHelper.passwordSyncEnabledKey)
184184
}
185185
}
186186

0 commit comments

Comments
 (0)