Addressing review comments: documentation, no more credentials valid check, only delete items if absolutely necessary

Author: yaroluchko

Amplify/Categories/Auth/Models/AccessGroup.swift

@@ -7,22 +7,43 @@
 
 import Foundation
 
+/// A structure representing an access group for managing keychain items.
 public struct AccessGroup {
+    /// The name of the access group.
     public let name: String?
+    
+    /// A flag indicating whether to migrate keychain items.
     public let migrateKeychainItems: Bool
 
+    /**
+     Initializes an `AccessGroup` with the specified name and migration option.
+     
+     - Parameter name: The name of the access group.
+     - Parameter migrateKeychainItemsOfUserSession: A flag indicating whether to migrate keychain items. Defaults to `false`.
+     */
     public init(name: String, migrateKeychainItemsOfUserSession: Bool = false) {
         self.init(name: name, migrateKeychainItems: migrateKeychainItemsOfUserSession)
     }
 
+    /**
+     Creates an `AccessGroup` instance with no specified name.
+     
+     - Parameter migrateKeychainItemsOfUserSession: A flag indicating whether to migrate keychain items.
+     - Returns: An `AccessGroup` instance with the migration option set.
+     */
     public static func none(migrateKeychainItemsOfUserSession: Bool) -> AccessGroup {
         return .init(migrateKeychainItems: migrateKeychainItemsOfUserSession)
     }
 
+    /**
+     A static property representing an `AccessGroup` with no name and no migration.
+     
+     - Returns: An `AccessGroup` instance with no name and the migration option set to `false`.
+     */
     public static var none: AccessGroup {
         return .none(migrateKeychainItemsOfUserSession: false)
     }
-
+    
     private init(name: String? = nil, migrateKeychainItems: Bool) {
         self.name = name
         self.migrateKeychainItems = migrateKeychainItems

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

@@ -26,6 +26,10 @@ struct AWSCognitoAuthCredentialStore {
     private var isKeychainConfiguredKey: String {
         "\(userDefaultsNameSpace).isKeychainConfigured"
     }
+    /// This UserDefaults Key is use to retrieve the stored access group to determine
+    /// which access group the migration should happen from
+    /// If none is found, the unshared service is used for migration and all items
+    /// under that service are queried
     private var accessGroupKey: String {
         "\(userDefaultsNameSpace).accessGroup"
     }
@@ -48,11 +52,14 @@ struct AWSCognitoAuthCredentialStore {
             self.keychain = KeychainStore(service: service)
         }
 
+        let oldAccessGroup = retrieveStoredAccessGroup()
         if migrateKeychainItemsOfUserSession {
             try? migrateKeychainItemsToAccessGroup()
+        } else if oldAccessGroup == nil && oldAccessGroup != accessGroup{
+            try? KeychainStore(service: service)._removeAll()
         }
 
-        try? saveStoredAccessGroup()
+        saveStoredAccessGroup()
 
         if !userDefaults.bool(forKey: isKeychainConfiguredKey) {
             try? clearAllCredentials()
@@ -202,11 +209,11 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior {
         try keychain._removeAll()
     }
 
-    private func retrieveStoredAccessGroup() throws -> String? {
+    private func retrieveStoredAccessGroup() -> String? {
         return userDefaults.string(forKey: accessGroupKey)
     }
 
-    private func saveStoredAccessGroup() throws {
+    private func saveStoredAccessGroup() {
         if let accessGroup {
             userDefaults.set(accessGroup, forKey: accessGroupKey)
         } else {
@@ -215,33 +222,10 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior {
     }
 
     private func migrateKeychainItemsToAccessGroup() throws {
-        let oldAccessGroup = try? retrieveStoredAccessGroup()
-        let oldKeychain: KeychainStoreBehavior
+        let oldAccessGroup = retrieveStoredAccessGroup()
 
         if oldAccessGroup == accessGroup {
-            log.verbose("[AWSCognitoAuthCredentialStore] Stored access group is the same as current access group, aborting migration")
-            return
-        }
-        
-        if let oldAccessGroup {
-            oldKeychain = KeychainStore(service: sharedService, accessGroup: oldAccessGroup)
-        } else {
-            oldKeychain = KeychainStore(service: service)
-        }
-        
-        let authCredentialStoreKey = generateSessionKey(for: authConfiguration)
-        let authCredentialData: Data
-        let awsCredential: AmplifyCredentials
-        do {
-            authCredentialData = try oldKeychain._getData(authCredentialStoreKey)
-            awsCredential = try decode(data: authCredentialData)
-        } catch {
-            log.verbose("[AWSCognitoAuthCredentialStore] Could not retrieve previous credentials in keychain under old access group, nothing to migrate")
-            return
-        }
-        
-        guard awsCredential.areValid() else {
-            log.verbose("[AWSCognitoAuthCredentialStore] Credentials found are not valid (expired) in old access group keychain, aborting migration")
+            log.info("[AWSCognitoAuthCredentialStore] Stored access group is the same as current access group, aborting migration")
             return
         }
 
@@ -281,10 +265,4 @@ private extension AWSCognitoAuthCredentialStore {
 
 }
 
-extension AWSCognitoAuthCredentialStore: DefaultLogger {
-    public static var log: Logger {
-        Amplify.Logging.logger(forNamespace: String(describing: self))
-    }
-
-    public nonisolated var log: Logger { Self.log }
-}
+extension AWSCognitoAuthCredentialStore: DefaultLogger { }

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Models/AWSCognitoSecureStoragePreferences.swift

@@ -8,11 +8,15 @@
 import Foundation
 import Amplify
 
+/// A struct to store preferences for how the plugin uses storage
 public struct AWSCognitoSecureStoragePreferences {
 
     /// The access group that the keychain will use for auth items
     public let accessGroup: AccessGroup?
 
+    /// Creates an intstance of AWSCognitoSecureStoragePreferences
+    /// - Parameters:
+    ///   - accessGroup: access group to be used
     public init(accessGroup: AccessGroup? = nil) {
         self.accessGroup = accessGroup
     }

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

@@ -15,15 +15,13 @@ class MockKeychainStoreBehavior: KeychainStoreBehavior {
     typealias VoidHandler = () -> Void
 
     let data: String
-    let allData: [(key: String, value: Data)]
     let removeAllHandler: VoidHandler?
     let mockKey: String = "mockKey"
 
     init(data: String,
          removeAllHandler: VoidHandler? = nil) {
         self.data = data
         self.removeAllHandler = removeAllHandler
-        self.allData = [(key: mockKey, value: Data(data.utf8))]
     }
 
     func _getString(_ key: String) throws -> String {
@@ -45,7 +43,4 @@ class MockKeychainStoreBehavior: KeychainStoreBehavior {
         removeAllHandler?()
     }
 
-    func _getAll() throws -> [(key: String, value: Data)] {
-        return allData
-    }
 }

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

@@ -366,10 +366,6 @@ struct MockLegacyStore: KeychainStoreBehavior {
     func _removeAll() throws {
 
     }
-    
-    func _getAll() throws -> [(key: String, value: Data)] {
-        return []
-    }
 
 }
 

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift

@@ -53,10 +53,6 @@ public protocol KeychainStoreBehavior {
     /// This System Programming Interface (SPI) may have breaking changes in future updates.
     func _removeAll() throws
 
-    @_spi(KeychainStore)
-    /// Retrieves all key-value pairs in the keychain
-    /// This System Programming Interface (SPI) may have breaking changes in future updates.
-    func _getAll() throws -> [(key: String, value: Data)]
 }
 
 public struct KeychainStore: KeychainStoreBehavior {
@@ -236,48 +232,6 @@ public struct KeychainStore: KeychainStoreBehavior {
         }
         log.verbose("[KeychainStore] Successfully removed all items from keychain")
     }
-    
-    @_spi(KeychainStore)
-    /// Retrieves all key-value pairs in the keychain
-    /// This System Programming Interface (SPI) may have breaking changes in future updates.
-    public func _getAll() throws -> [(key: String, value: Data)] {
-        log.verbose("[KeychainStore] Starting to retrieve all items from keychain")
-        var query = attributes.defaultGetQuery()
-        query[Constants.MatchLimit] = Constants.MatchLimitAll
-        query[Constants.ReturnData] = kCFBooleanTrue
-        query[Constants.ReturnAttributes] = kCFBooleanTrue
-        query[Constants.ReturnRef] = kCFBooleanTrue
-
-        var result: AnyObject?
-        let status = SecItemCopyMatching(query as CFDictionary, &result)
-
-        switch status {
-        case errSecSuccess:
-            guard let items = result as? [[String: Any]] else {
-                log.error("[KeychainStore] The keychain items retrieved are not the correct type")
-                throw KeychainStoreError.unknown("The keychain items retrieved are not the correct type")
-            }
-
-            var keyValuePairs = [(key: String, value: Data)]()
-            for item in items {
-                guard let key = item[Constants.AttributeAccount] as? String,
-                      let value = item[Constants.ValueData] as? Data else {
-                    log.error("[KeychainStore] Unable to retrieve key or value from keychain item")
-                    continue
-                }
-                keyValuePairs.append((key: key, value: value))
-            }
-
-            log.verbose("[KeychainStore] Successfully retrieved \(keyValuePairs.count) items from keychain")
-            return keyValuePairs
-        case errSecItemNotFound:
-            log.verbose("[KeychainStore] No items found in keychain")
-            return []
-        default:
-            log.error("[KeychainStore] Error of status=\(status) occurred when attempting to retrieve all items from keychain")
-            throw KeychainStoreError.securityError(status)
-        }
-    }
 
 }
 

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift

@@ -20,15 +20,21 @@ public struct KeychainStoreMigrator {
     public func migrate() throws {
         log.verbose("[KeychainStoreMigrator] Starting to migrate items")
 
+        // Check if there are any existing items under the new service and access group
+        let existingItemsQuery = newAttributes.defaultGetQuery()
+        let existingItemsStatus = SecItemCopyMatching(existingItemsQuery as CFDictionary, nil)
+
+        if existingItemsStatus == errSecSuccess {
+            // Remove existing items to avoid duplicate item error
+            try? KeychainStore(service: newAttributes.service, accessGroup: newAttributes.accessGroup)._removeAll()
+        }
+        
         var updateQuery = oldAttributes.defaultGetQuery()
 
         var updateAttributes = [String: Any]()
         updateAttributes[KeychainStore.Constants.AttributeService] = newAttributes.service
         updateAttributes[KeychainStore.Constants.AttributeAccessGroup] = newAttributes.accessGroup
 
-        // Remove any current items to avoid duplicate item error
-        try? KeychainStore(service: newAttributes.service, accessGroup: newAttributes.accessGroup)._removeAll()
-        
         let updateStatus = SecItemUpdate(updateQuery as CFDictionary, updateAttributes as CFDictionary)
         switch updateStatus {
         case errSecSuccess:
@@ -47,10 +53,4 @@ public struct KeychainStoreMigrator {
     }
 }
 
-extension KeychainStoreMigrator: DefaultLogger {
-    public static var log: Logger {
-        Amplify.Logging.logger(forNamespace: String(describing: self))
-    }
-
-    public nonisolated var log: Logger { Self.log }
-}
+extension KeychainStoreMigrator: DefaultLogger { }

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

@@ -61,20 +61,6 @@ class MockKeychainStore: KeychainStoreBehavior {
         stringValues.removeAll()
         dataValues.removeAll()
     }
-    
-    func _getAll() throws -> [(key: String, value: Data)] {
-        var allValues: [(key: String, value: Data)] = []
-        
-        for (key, value) in dataValues {
-            allValues.append((key: key, value: value))
-        }
-        
-        for (key, value) in stringValues {
-            allValues.append((key: key, value: value.data(using: .utf8)!))
-        }
-        
-        return allValues
-    }
 
     func resetCounters() {
         dataForKeyCount = 0