Skip to content

Commit 5859f19

Browse files
authored
fix(auth): don't remove items from keychain if it already has items (#4105)
1 parent 16539be commit 5859f19

5 files changed

Lines changed: 68 additions & 1 deletion

File tree

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,13 @@ struct AWSCognitoAuthCredentialStore {
5656
if migrateKeychainItemsOfUserSession {
5757
try? migrateKeychainItemsToAccessGroup()
5858
} else if oldAccessGroup == nil && oldAccessGroup != accessGroup {
59-
try? KeychainStore(service: service)._removeAll()
59+
// Only clear the old keychain if the shared keychain doesn't already have items.
60+
// This prevents data loss when an app extension (e.g., widget) initializes before
61+
// the main app has a chance to record the migration in UserDefaults, since
62+
// UserDefaults is not shared between app and extensions.
63+
if !sharedKeychainHasItems(accessGroup: accessGroup) {
64+
try? KeychainStore(service: service)._removeAll()
65+
}
6066
}
6167

6268
saveStoredAccessGroup()
@@ -252,6 +258,15 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior {
252258
return
253259
}
254260

261+
// If the shared keychain already has items, migration has already occurred
262+
// (likely by the main app). Skip migration to prevent data loss.
263+
// This check is necessary because UserDefaults is not shared between app and extensions,
264+
// so the extension may not know that migration already happened.
265+
if sharedKeychainHasItems(accessGroup: accessGroup) {
266+
log.info("[AWSCognitoAuthCredentialStore] Shared keychain already has items, migration already completed, aborting")
267+
return
268+
}
269+
255270
let oldService = oldAccessGroup != nil ? sharedService : service
256271
let newService = accessGroup != nil ? sharedService : service
257272

@@ -265,6 +280,17 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior {
265280
log.verbose("[AWSCognitoAuthCredentialStore] Migration of keychain items from old access group to new access group successful")
266281
}
267282

283+
/// Checks if the shared keychain (with the given access group) already contains items.
284+
/// This is used to determine if migration has already occurred, which helps prevent
285+
/// data loss when app extensions initialize with their own UserDefaults that don't
286+
/// reflect the migration state recorded by the main app.
287+
private func sharedKeychainHasItems(accessGroup: String?) -> Bool {
288+
guard let accessGroup else { return false }
289+
290+
let sharedKeychain = KeychainStore(service: sharedService, accessGroup: accessGroup)
291+
return (try? sharedKeychain._hasItems()) ?? false
292+
}
293+
268294
}
269295

270296
/// Helpers for encode and decoding

AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/ActionTests/CredentialStore/MockCredentialStoreBehavior.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,8 @@ class MockKeychainStoreBehavior: KeychainStoreBehavior {
4343
func _removeAll() throws {
4444
removeAllHandler?()
4545
}
46+
47+
func _hasItems() throws -> Bool {
48+
return !data.isEmpty
49+
}
4650
}

AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/Support/DefaultConfig.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,10 @@ struct MockLegacyStore: KeychainStoreBehavior {
396396

397397
}
398398

399+
func _hasItems() throws -> Bool {
400+
return false
401+
}
402+
399403
}
400404

401405
struct MockASF: AdvancedSecurityBehavior {

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,12 @@ public protocol KeychainStoreBehavior {
5353
@_spi(KeychainStore)
5454
func _removeAll() throws
5555

56+
/// Checks if the Keychain contains any items for this service and access group.
57+
/// This System Programming Interface (SPI) may have breaking changes in future updates.
58+
/// - Returns: `true` if at least one item exists, `false` otherwise
59+
@_spi(KeychainStore)
60+
func _hasItems() throws -> Bool
61+
5662
}
5763

5864
public struct KeychainStore: KeychainStoreBehavior {
@@ -233,6 +239,29 @@ public struct KeychainStore: KeychainStoreBehavior {
233239
log.verbose("[KeychainStore] Successfully removed all items from keychain")
234240
}
235241

242+
/// Checks if the Keychain contains any items for this service and access group.
243+
/// This System Programming Interface (SPI) may have breaking changes in future updates.
244+
/// - Returns: `true` if at least one item exists, `false` otherwise
245+
@_spi(KeychainStore)
246+
public func _hasItems() throws -> Bool {
247+
log.verbose("[KeychainStore] Checking if keychain has any items")
248+
var query = attributes.defaultGetQuery()
249+
query[Constants.MatchLimit] = Constants.MatchLimitOne
250+
251+
let status = SecItemCopyMatching(query as CFDictionary, nil)
252+
switch status {
253+
case errSecSuccess:
254+
log.verbose("[KeychainStore] Keychain has items")
255+
return true
256+
case errSecItemNotFound:
257+
log.verbose("[KeychainStore] Keychain has no items")
258+
return false
259+
default:
260+
log.error("[KeychainStore] Error checking keychain items with status=\(status)")
261+
throw KeychainStoreError.securityError(status)
262+
}
263+
}
264+
236265
}
237266

238267
extension KeychainStore {

AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockKeychainStore.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ class MockKeychainStore: KeychainStoreBehavior {
6262
dataValues.removeAll()
6363
}
6464

65+
func _hasItems() throws -> Bool {
66+
return !stringValues.isEmpty || !dataValues.isEmpty
67+
}
68+
6569
func resetCounters() {
6670
dataForKeyCount = 0
6771
stringForKeyCount = 0

0 commit comments

Comments
 (0)