From 752300181883f3b142655468c7c7a8d976d19151 Mon Sep 17 00:00:00 2001 From: Yaro Luchko Date: Wed, 21 Aug 2024 19:46:18 -0700 Subject: [PATCH] Addressing review comments: documentation, no more credentials valid check, only delete items if absolutely necessary --- .../Categories/Auth/Models/AccessGroup.swift | 23 ++++++++- .../AWSCognitoAuthCredentialStore.swift | 48 +++++-------------- .../AWSCognitoSecureStoragePreferences.swift | 4 ++ .../MockCredentialStoreBehavior.swift | 5 -- .../Support/DefaultConfig.swift | 4 -- .../Keychain/KeychainStore.swift | 46 ------------------ .../Keychain/KeychainStoreMigrator.swift | 20 ++++---- .../Mocks/MockKeychainStore.swift | 14 ------ 8 files changed, 49 insertions(+), 115 deletions(-) diff --git a/Amplify/Categories/Auth/Models/AccessGroup.swift b/Amplify/Categories/Auth/Models/AccessGroup.swift index 27fa47d561..5842598910 100644 --- a/Amplify/Categories/Auth/Models/AccessGroup.swift +++ b/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 diff --git a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift index 55b054bcd0..c578060aaa 100644 --- a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift +++ b/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 { } diff --git a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Models/AWSCognitoSecureStoragePreferences.swift b/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Models/AWSCognitoSecureStoragePreferences.swift index c7dcefe42b..3ee17a804c 100644 --- a/AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Models/AWSCognitoSecureStoragePreferences.swift +++ b/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 } diff --git a/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/ActionTests/CredentialStore/MockCredentialStoreBehavior.swift b/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/ActionTests/CredentialStore/MockCredentialStoreBehavior.swift index 1f00f81df3..d0ca03f00b 100644 --- a/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/ActionTests/CredentialStore/MockCredentialStoreBehavior.swift +++ b/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/ActionTests/CredentialStore/MockCredentialStoreBehavior.swift @@ -15,7 +15,6 @@ class MockKeychainStoreBehavior: KeychainStoreBehavior { typealias VoidHandler = () -> Void let data: String - let allData: [(key: String, value: Data)] let removeAllHandler: VoidHandler? let mockKey: String = "mockKey" @@ -23,7 +22,6 @@ class MockKeychainStoreBehavior: KeychainStoreBehavior { 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 - } } diff --git a/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/Support/DefaultConfig.swift b/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/Support/DefaultConfig.swift index 650d08bf38..9beaf029c3 100644 --- a/AmplifyPlugins/Auth/Tests/AWSCognitoAuthPluginUnitTests/Support/DefaultConfig.swift +++ b/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 [] - } } diff --git a/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift b/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift index a015a09e3c..4d1063c44c 100644 --- a/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift +++ b/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) - } - } } diff --git a/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift b/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift index 8eb78b77d6..580366c519 100644 --- a/AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift +++ b/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 { } diff --git a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockKeychainStore.swift b/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockKeychainStore.swift index 3596ce8241..e2c127588e 100644 --- a/AmplifyPlugins/Internal/Tests/InternalAWSPinpointUnitTests/Mocks/MockKeychainStore.swift +++ b/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