From 6df425016fe45ddcdafbd5a2a89a59ee1d536545 Mon Sep 17 00:00:00 2001 From: Pete Date: Tue, 21 May 2024 11:08:25 +0100 Subject: [PATCH] Correctly use keychainAccessibilityValue, fix incorrect test assertion, slightly improve implementation --- ...DataBrokerProtectionKeyStoreProvider.swift | 43 +++++++++++-------- ...rokerProtectionKeyStoreProviderTests.swift | 2 +- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionKeyStoreProvider.swift b/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionKeyStoreProvider.swift index 7ce84b4140..24971a4cba 100644 --- a/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionKeyStoreProvider.swift +++ b/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionKeyStoreProvider.swift @@ -51,6 +51,10 @@ final class DataBrokerProtectionKeyStoreProvider: SecureStorageKeyStoreProvider return Constants.defaultServiceName } + var keychainAccessibilityValue: String { + kSecAttrAccessibleAfterFirstUnlock as String + } + let keychainService: KeychainService private let groupNameProvider: GroupNameProviding private let getLog: () -> OSLog @@ -71,16 +75,20 @@ final class DataBrokerProtectionKeyStoreProvider: SecureStorageKeyStoreProvider } func attributesForEntry(named: String, serviceName: String) -> [String: Any] { - var attributes = legacyAttributesForEntry(named: named, serviceName: serviceName) - attributes[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock - return attributes + [ + kSecClass: kSecClassGenericPassword, + kSecUseDataProtectionKeychain: true, + kSecAttrSynchronizable: false, + kSecAttrAccessGroup: groupNameProvider.appGroupName, + kSecAttrAccount: named, + ] as [String: Any] } } private extension DataBrokerProtectionKeyStoreProvider { - var attributeUpdate: [String: Any] { - [kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock] + var afterFirstUnlockAttributeUpdate: [String: Any] { + [kSecAttrAccessible as String: keychainAccessibilityValue] } /// Reads data from the Keychain using the latest query attributes, or if not found, reads using old query attributes and if found, migrates @@ -91,21 +99,21 @@ private extension DataBrokerProtectionKeyStoreProvider { func readOrMigrate(named name: String, serviceName: String) throws -> Data? { // Try to read keychain data using attributes which include `kSecAttrAccessible` value of `kSecAttrAccessibleAfterFirstUnlock` - let attributes = attributesForEntry(named: name, serviceName: serviceName) + let attributes = afterFirstUnlockQueryAttributes(named: name, serviceName: serviceName) if let data = try read(serviceName: serviceName, queryAttributes: attributes) { return data } else { // If we didn't find Keychain data, try using attributes WITHOUT `kSecAttrAccessible` value of `kSecAttrAccessibleAfterFirstUnlock` - let legacyAttributes = legacyAttributesForEntry(named: name, serviceName: serviceName) + let legacyAttributes = whenUnlockedQueryAttributes(named: name, serviceName: serviceName) let accessibilityValueString = legacyAttributes[kSecAttrAccessible as String] as? String ?? "[value unavailable]" os_log("Attempting read and migrate of DBP Keychain data with kSecAttrAccessible value of \(accessibilityValueString)", log: .dataBrokerProtection, type: .debug) if let data = try read(serviceName: serviceName, queryAttributes: legacyAttributes) { // We found Keychain data, so update it's `kSecAttrAccessible` value to `kSecAttrAccessibleAfterFirstUnlock` - try update(serviceName: serviceName, queryAttributes: legacyAttributes, attributeUpdate: attributeUpdate) + try update(serviceName: serviceName, queryAttributes: legacyAttributes, attributeUpdate: afterFirstUnlockAttributeUpdate) return data } } @@ -164,14 +172,15 @@ private extension DataBrokerProtectionKeyStoreProvider { os_log("Updated DBP Keychain data kSecAttrAccessible value to \(accessibilityValueString)", log: .dataBrokerProtection, type: .debug) } - func legacyAttributesForEntry(named entryName: String, serviceName: String) -> [String: Any] { - return [ - kSecClass: kSecClassGenericPassword, - kSecUseDataProtectionKeychain: true, - kSecAttrSynchronizable: false, - kSecAttrAccessGroup: groupNameProvider.appGroupName, - kSecAttrAccount: entryName, - kSecAttrAccessible: kSecAttrAccessibleWhenUnlocked - ] as [String: Any] + func afterFirstUnlockQueryAttributes(named name: String, serviceName: String) -> [String: Any] { + var attributes = attributesForEntry(named: name, serviceName: serviceName) + attributes[kSecAttrAccessible as String] = keychainAccessibilityValue + return attributes + } + + func whenUnlockedQueryAttributes(named name: String, serviceName: String) -> [String: Any] { + var attributes = attributesForEntry(named: name, serviceName: serviceName) + attributes[kSecAttrAccessible as String] = kSecAttrAccessibleWhenUnlocked + return attributes } } diff --git a/LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionKeyStoreProviderTests.swift b/LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionKeyStoreProviderTests.swift index bfb34379ba..5af72c98dd 100644 --- a/LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionKeyStoreProviderTests.swift +++ b/LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionKeyStoreProviderTests.swift @@ -102,7 +102,7 @@ final class DataBrokerProtectionKeyStoreProviderTests: XCTestCase { // Then XCTAssertEqual(mockKeychainService.addCallCount, 1) - XCTAssertEqual(mockKeychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleWhenUnlocked as String) + XCTAssertEqual(mockKeychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleAfterFirstUnlock as String) } }