Merge branch 'main' into sam/additional-privacy-pro-rmf-changes

* main: Enable DBP Keychain Item Accessibility Migration (#827)
duckduckgo · Jun 3, 2024 · d40eed0 · d40eed0
2 parents f0c629e + cd7850d
commit d40eed0
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 1 deletion.
diff --git a/Sources/SecureStorage/SecureStorageError.swift b/Sources/SecureStorage/SecureStorageError.swift
@@ -44,6 +44,8 @@ public enum SecureStorageError: Error {
     case secError(status: Int32)
     case generalCryptoError
     case encodingFailed
+    case keystoreReadError(status: Int32)
+    case keystoreUpdateError(status: Int32)
 }
 
 extension SecureStorageError: CustomNSError {
@@ -66,6 +68,8 @@ extension SecureStorageError: CustomNSError {
         case .secError: return 11
         case .generalCryptoError: return 12
         case .encodingFailed: return 13
+        case .keystoreReadError: return 14
+        case .keystoreUpdateError: return 15
         }
     }
 
@@ -87,6 +91,10 @@ extension SecureStorageError: CustomNSError {
             }
         case .keystoreError(status: let code):
             errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreError", code: Int(code), userInfo: nil)
+        case .keystoreReadError(status: let code):
+            errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreReadError", code: Int(code), userInfo: nil)
+        case .keystoreUpdateError(status: let code):
+            errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreUpdateError", code: Int(code), userInfo: nil)
         case .secError(status: let code):
             errorUserInfo["NSUnderlyingError"] = NSError(domain: "secError", code: Int(code), userInfo: nil)
         case .authRequired, .invalidPassword, .noL1Key, .noL2Key, .duplicateRecord, .generalCryptoError, .encodingFailed:

diff --git a/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift b/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift
@@ -23,6 +23,7 @@ public protocol KeychainService {
     func itemMatching(_ query: [String: Any], _ result: UnsafeMutablePointer<CFTypeRef?>?) -> OSStatus
     func add(_ attributes: [String: Any], _ result: UnsafeMutablePointer<CFTypeRef?>?) -> OSStatus
     func delete(_ query: [String: Any]) -> OSStatus
+    func update(_ query: [String: Any], _ attributesToUpdate: [String: Any]) -> OSStatus
 }
 
 public extension KeychainService {
@@ -37,6 +38,10 @@ public extension KeychainService {
     func delete(_ query: [String: Any]) -> OSStatus {
         SecItemDelete(query as CFDictionary)
     }
+
+    func update(_ query: [String: Any], _ attributesToUpdate: [String: Any]) -> OSStatus {
+        SecItemUpdate(query as CFDictionary, attributesToUpdate as CFDictionary)
+    }
 }
 
 public struct DefaultKeychainService: KeychainService {
@@ -56,6 +61,7 @@ public protocol SecureStorageKeyStoreProvider {
     var l1KeyEntryName: String { get }
     var l2KeyEntryName: String { get }
     var keychainServiceName: String { get }
+    var keychainAccessibilityValue: String { get }
 
     func storeGeneratedPassword(_ password: Data) throws
     func generatedPassword() throws -> Data?
@@ -79,6 +85,10 @@ public extension SecureStorageKeyStoreProvider {
         DefaultKeychainService()
     }
 
+    var keychainAccessibilityValue: String {
+        kSecAttrAccessibleWhenUnlocked as String
+    }
+
     func generatedPassword() throws -> Data? {
         return try readData(named: generatedPasswordEntryName, serviceName: keychainServiceName)
     }
@@ -144,7 +154,7 @@ public extension SecureStorageKeyStoreProvider {
 
         var query = attributesForEntry(named: name, serviceName: serviceName)
         query[kSecAttrService as String] = serviceName
-        query[kSecAttrAccessible as String] = kSecAttrAccessibleWhenUnlocked
+        query[kSecAttrAccessible as String] = keychainAccessibilityValue
         query[kSecValueData as String] = base64Data
 
         let status = keychainService.add(query, nil)

diff --git a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift
@@ -128,4 +128,23 @@ final class AutofillKeyStoreProviderTests: XCTestCase {
             XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v3ServiceName)
         }
     }
+
+    func testWhenWriteData_correctKeychainAccessibilityValueIsUsed() throws {
+        try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in
+            // Given
+            let originalString = "Mock Keychain data!"
+            let data = originalString.data(using: .utf8)!
+            let encodedString = data.base64EncodedString()
+            let mockData = encodedString.data(using: .utf8)!
+            let keychainService = MockKeychainService()
+            let sut = AutofillKeyStoreProvider(keychainService: keychainService)
+
+            // When
+            _ = try sut.writeData(mockData, named: entry.keychainIdentifier, serviceName: sut.keychainServiceName)
+
+            // Then
+            XCTAssertEqual(keychainService.addCallCount, 1)
+            XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleWhenUnlocked as String)
+        }
+    }
 }