From e77e0a84a9af0746512b5d7021d1ab18e2dabb55 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Mon, 21 Oct 2024 15:42:26 +0200 Subject: [PATCH 1/8] Key migration (v4) to shared keychain access group --- .../AutofillKeyStoreProvider.swift | 128 +++++++++++++++--- 1 file changed, 111 insertions(+), 17 deletions(-) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift index 0a22e1f0a..d94e184f1 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift @@ -21,12 +21,55 @@ import Foundation import SecureStorage import os.log +protocol KeyStorePlatformProviding { + var keychainServiceName: String { get } + func keychainIdentifier(for rawValue: String) -> String + var keychainSecurityGroup: String { get } +} + +struct iOSKeyStorePlatformProvider: KeyStorePlatformProviding { + private let appGroupName: String + + // Using appGroupName in the initializer, allowing injection for tests + init(appGroupName: String = Bundle.main.appGroupName) { + self.appGroupName = appGroupName + } + + var keychainServiceName: String { + return AutofillKeyStoreProvider.Constants.v4ServiceName + } + + func keychainIdentifier(for rawValue: String) -> String { + return appGroupName + rawValue + } + + var keychainSecurityGroup: String { + return appGroupName + } +} + +struct macOSKeyStorePlatformProvider: KeyStorePlatformProviding { + var keychainServiceName: String { + return AutofillKeyStoreProvider.Constants.v3ServiceName + } + + func keychainIdentifier(for rawValue: String) -> String { + return (Bundle.main.bundleIdentifier ?? "com.duckduckgo") + rawValue + } + + var keychainSecurityGroup: String { + return "" + } + +} + final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { struct Constants { static let v1ServiceName = "DuckDuckGo Secure Vault" static let v2ServiceName = "DuckDuckGo Secure Vault v2" static let v3ServiceName = "DuckDuckGo Secure Vault v3" + static let v4ServiceName = "DuckDuckGo Secure Vault v4" } // DO NOT CHANGE except if you want to deliberately invalidate all users's vaults. @@ -38,7 +81,12 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { case l2Key = "A5711F4D-7AA5-4F0C-9E4F-BE553F1EA299" // `keychainIdentifier` should be used as Keychain Account names, as app variants (e.g App Store, DMG) should have separate entries - var keychainIdentifier: String { + func keychainIdentifier(using platformProvider: KeyStorePlatformProviding) -> String { + return platformProvider.keychainIdentifier(for: self.rawValue) + } + + // `legacyKeychainIdentifier` is the Keychain Account name pre migration to shared app groups (currently only on iOS) + var legacyKeychainIdentifier: String { (Bundle.main.bundleIdentifier ?? "com.duckduckgo") + rawValue } @@ -53,13 +101,13 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { } } - init?(_ keyValue: String) { + init?(_ keyValue: String, using platformProvider: KeyStorePlatformProviding) { switch keyValue { - case EntryName.generatedPassword.keychainIdentifier: + case platformProvider.keychainIdentifier(for: EntryName.generatedPassword.rawValue), EntryName.generatedPassword.legacyKeychainIdentifier: self = .generatedPassword - case EntryName.l1Key.keychainIdentifier: + case platformProvider.keychainIdentifier(for: EntryName.l1Key.rawValue), EntryName.l1Key.legacyKeychainIdentifier: self = .l1Key - case EntryName.l2Key.keychainIdentifier: + case platformProvider.keychainIdentifier(for: EntryName.l2Key.rawValue), EntryName.l2Key.legacyKeychainIdentifier: self = .l2Key default: return nil @@ -69,27 +117,40 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { let keychainService: KeychainService private var reporter: SecureVaultReporting? + private let platformProvider: KeyStorePlatformProviding init(keychainService: KeychainService = DefaultKeychainService(), - reporter: SecureVaultReporting? = nil) { + reporter: SecureVaultReporting? = nil, + platformProvider: KeyStorePlatformProviding? = nil) { self.keychainService = keychainService self.reporter = reporter + + // Use default platform provider based on the platform. + if let platformProvider = platformProvider { + self.platformProvider = platformProvider + } else { +#if os(iOS) + self.platformProvider = iOSKeyStorePlatformProvider() +#else + self.platformProvider = macOSKeyStorePlatformProvider() +#endif + } } var keychainServiceName: String { - return Constants.v3ServiceName + return platformProvider.keychainServiceName } var generatedPasswordEntryName: String { - return EntryName.generatedPassword.keychainIdentifier + return EntryName.generatedPassword.keychainIdentifier(using: platformProvider) } var l1KeyEntryName: String { - return EntryName.l1Key.keychainIdentifier + return EntryName.l1Key.keychainIdentifier(using: platformProvider) } var l2KeyEntryName: String { - return EntryName.l2Key.keychainIdentifier + return EntryName.l2Key.keychainIdentifier(using: platformProvider) } func readData(named name: String, serviceName: String) throws -> Data? { @@ -103,15 +164,19 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { /// - Returns: Optional data private func readOrMigrate(named name: String, serviceName: String) throws -> Data? { if let data = try read(named: name, serviceName: serviceName) { - Logger.autofill.debug("Autofill Keystore data retrieved") + Logger.autofill.debug("Autofill Keystore \(serviceName) data retrieved") return data } else { - guard let entryName = EntryName(name) else { return nil } + guard let entryName = EntryName(name, using: platformProvider) else { return nil } reporter?.secureVaultKeyStoreEvent(entryName.keyStoreMigrationEvent) + // If V4 migration, look for items in V3 vault (i.e pre-shared Keychain storage) + if isPostV3(serviceName), let data = try migrateEntry(entryName: entryName, serviceName: Constants.v3ServiceName) { + Logger.autofill.debug("Migrated V3 Autofill Keystore data") + return data // Look for items in V2 vault (i.e pre-bundle-specifc Keychain storage) - if let data = try migrateEntry(entryName: entryName, serviceName: Constants.v2ServiceName) { + } else if let data = try migrateEntry(entryName: entryName, serviceName: Constants.v2ServiceName) { Logger.autofill.debug("Migrated V2 Autofill Keystore data") return data // Look for items in V1 vault @@ -120,6 +185,7 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { return data } + Logger.autofill.debug("Keychain migration failed for \(name)") return nil } } @@ -139,7 +205,7 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { let status = keychainService.itemMatching(query, &item) switch status { case errSecSuccess: - if isPostV1(serviceName) { + if isPostV1(serviceName) || isPostV3(serviceName) { guard let itemData = item as? Data, let itemString = String(data: itemData, encoding: .utf8), let decodedData = Data(base64Encoded: itemString) else { @@ -162,14 +228,15 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { /// Migrates an entry to new bundle-specific Keychain storage /// - Parameters: - /// - entryName: Entry to migrate. It's `rawValue` is used when reading from old storage, and it's `keyValue` is used when writing to storage + /// - entryName: Entry to migrate. It's `rawValue` is used when reading from old storage pre-V2, while its `legacyKeychainIdentifier` is used post V2, and it's `keyValue` is used when writing to storage /// - serviceName: Service name to use when querying Keychain for the entry /// - Returns: Optional data private func migrateEntry(entryName: EntryName, serviceName: String) throws -> Data? { - guard let data = try read(named: entryName.rawValue, serviceName: serviceName) else { + let name = serviceName == Constants.v3ServiceName ? entryName.legacyKeychainIdentifier : entryName.rawValue + guard let data = try read(named: name, serviceName: serviceName) else { return nil } - try writeData(data, named: entryName.keychainIdentifier, serviceName: keychainServiceName) + try writeData(data, named: entryName.keychainIdentifier(using: platformProvider), serviceName: keychainServiceName) return data } @@ -177,11 +244,17 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { [Constants.v2ServiceName, Constants.v3ServiceName].contains(serviceName) } + private func isPostV3(_ serviceName: String) -> Bool { + [Constants.v4ServiceName].contains(serviceName) + } + // MARK: - Autofill Attributes func attributesForEntry(named name: String, serviceName: String) -> [String: Any] { if isPostV1(serviceName) { return defaultAttributesForEntry(named: name) + } else if isPostV3(serviceName) { + return defaultAttributesForSharedEntry(named: name) } else { return legacyAttributesForEntry(named: name) } @@ -205,4 +278,25 @@ final class AutofillKeyStoreProvider: SecureStorageKeyStoreProvider { ] as [String: Any] } + private func defaultAttributesForSharedEntry(named name: String) -> [String: Any] { + return [ + kSecClass: kSecClassGenericPassword, + kSecUseDataProtectionKeychain: false, + kSecAttrSynchronizable: false, + kSecAttrAccount: name, + kSecAttrAccessGroup: platformProvider.keychainSecurityGroup + ] as [String: Any] + } +} + +fileprivate extension Bundle { + + static let vaultAppGroupName = "VAULT_APP_GROUP" + + var appGroupName: String { + guard let appGroup = object(forInfoDictionaryKey: Bundle.vaultAppGroupName) as? String else { + fatalError("Info.plist is missing \(Bundle.vaultAppGroupName)") + } + return appGroup + } } From 2f0867044c4f1ba11dfda217dd9c432fc39c6719 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Mon, 21 Oct 2024 15:42:42 +0200 Subject: [PATCH 2/8] Key migration test updates --- .../MockKeystoreProvider.swift | 13 +++ .../AutofillKeyStoreProviderTests.swift | 95 +++++++++++++++++-- 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/Sources/SecureStorageTestsUtils/MockKeystoreProvider.swift b/Sources/SecureStorageTestsUtils/MockKeystoreProvider.swift index 689641386..dc47eec1f 100644 --- a/Sources/SecureStorageTestsUtils/MockKeystoreProvider.swift +++ b/Sources/SecureStorageTestsUtils/MockKeystoreProvider.swift @@ -23,6 +23,7 @@ public final class MockKeychainService: KeychainService { public enum Mode { case nothingFound + case v4Found case v3Found case v2Found case v1Found @@ -55,9 +56,21 @@ public final class MockKeychainService: KeychainService { switch mode { case .nothingFound: return errSecItemNotFound + case .v4Found: + setResult() + return errSecSuccess case .v3Found: +#if os(iOS) + if itemMatchingCallCount == 2 { + setResult() + return errSecSuccess + } else { + return errSecItemNotFound + } +#else setResult() return errSecSuccess +#endif case .v2Found: if itemMatchingCallCount == 2 { setResult() diff --git a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift index 89bce30c3..0c53a0b59 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift @@ -23,6 +23,82 @@ import SecureStorageTestsUtils final class AutofillKeyStoreProviderTests: XCTestCase { +#if os(iOS) + let iOSPlatformProvider = iOSKeyStorePlatformProvider(appGroupName: "MockAppGroup") + + func testV1ToV4Migration() throws { + try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in + // Given + let keychainService = MockKeychainService() + keychainService.mode = .v1Found // Simulate a v1 keychain entry found + let sut = AutofillKeyStoreProvider(keychainService: keychainService, platformProvider: iOSPlatformProvider) + + // When + _ = try sut.readData(named: entry.keychainIdentifier(using: iOSPlatformProvider), serviceName: sut.keychainServiceName) + + // Then + XCTAssertEqual(keychainService.addCallCount, 1) // Migration should trigger a write + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v4ServiceName) // Ensure it's migrated to v4 + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessGroup as String] as! String, iOSPlatformProvider.keychainSecurityGroup as String) // Ensure correct accessibility + } + } + + func testV2ToV4Migration() throws { + try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in + // Given + let keychainService = MockKeychainService() + keychainService.mode = .v2Found // Simulate a v2 keychain entry found + let sut = AutofillKeyStoreProvider(keychainService: keychainService, platformProvider: iOSPlatformProvider) + + // When + _ = try sut.readData(named: entry.keychainIdentifier(using: iOSPlatformProvider), serviceName: sut.keychainServiceName) + + // Then + XCTAssertEqual(keychainService.addCallCount, 1) // Migration should trigger a write + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v4ServiceName) // Ensure it's migrated to v4 + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessGroup as String] as! String, iOSPlatformProvider.keychainSecurityGroup as String) // Ensure correct accessibility + } + } + + func testV3ToV4Migration() throws { + try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in + // Given + let keychainService = MockKeychainService() + keychainService.mode = .v3Found // Simulate a v3 keychain entry found + let sut = AutofillKeyStoreProvider(keychainService: keychainService, platformProvider: iOSPlatformProvider) + + // When + _ = try sut.readData(named: entry.keychainIdentifier(using: iOSPlatformProvider), serviceName: sut.keychainServiceName) + + // Then + XCTAssertEqual(keychainService.addCallCount, 1) // Migration should trigger a write + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v4ServiceName) // Ensure it's migrated to v4 + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessGroup as String] as! String, iOSPlatformProvider.keychainSecurityGroup as String) // Ensure correct accessibility + } + } + + func testWhenWriteData_v4KeychainUsed() throws { + try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in + // Given + let originalString = "Mock Keychain data for v4!" + let data = originalString.data(using: .utf8)! + let encodedString = data.base64EncodedString() + let mockData = encodedString.data(using: .utf8)! + let keychainService = MockKeychainService() + let sut = AutofillKeyStoreProvider(keychainService: keychainService, platformProvider: iOSPlatformProvider) + + // When + _ = try sut.writeData(mockData, named: entry.keychainIdentifier(using: iOSPlatformProvider), serviceName: AutofillKeyStoreProvider.Constants.v4ServiceName) + + // Then + XCTAssertEqual(keychainService.addCallCount, 1) + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v4ServiceName) // Ensure v4 is used for write + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleWhenUnlocked as String) // Ensure correct accessibility + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessGroup as String] as! String, iOSPlatformProvider.keychainSecurityGroup as String) // Ensure correct accessibility + } + } + + #else func testWhenReadData_AndValueIsFound_NoFallbackSearchIsPerformed() throws { try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in @@ -32,7 +108,7 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - let result = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + let result = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.itemMatchingCallCount, 1) @@ -48,7 +124,7 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - _ = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + _ = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.itemMatchingCallCount, 3) @@ -64,7 +140,7 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - let result = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + let result = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.itemMatchingCallCount, 2) @@ -83,7 +159,7 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - _ = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + _ = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.itemMatchingCallCount, 3) @@ -101,11 +177,11 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - let result = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + let result = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.addCallCount, 1) - XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccount as String] as! String, entry.keychainIdentifier) + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccount as String] as! String, entry.keychainIdentifier(using: macOSKeyStorePlatformProvider())) XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v3ServiceName) XCTAssertEqual(String(decoding: result!, as: UTF8.self), "Mock Keychain data!") } @@ -120,11 +196,11 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - _ = try sut.readData(named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + _ = try sut.readData(named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.addCallCount, 1) - XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccount as String] as! String, entry.keychainIdentifier) + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccount as String] as! String, entry.keychainIdentifier(using: macOSKeyStorePlatformProvider())) XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v3ServiceName) } } @@ -140,11 +216,12 @@ final class AutofillKeyStoreProviderTests: XCTestCase { let sut = AutofillKeyStoreProvider(keychainService: keychainService) // When - _ = try sut.writeData(mockData, named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + _ = try sut.writeData(mockData, named: entry.keychainIdentifier(using: macOSKeyStorePlatformProvider()), serviceName: sut.keychainServiceName) // Then XCTAssertEqual(keychainService.addCallCount, 1) XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleWhenUnlocked as String) } } +#endif } From d9ec4662583562279ad95f39854a329729d47e36 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Mon, 21 Oct 2024 15:43:21 +0200 Subject: [PATCH 3/8] Vault.db file migration to shared storage + tests --- .../AppGroupFileStorageManager.swift | 96 +++++++++++++++ .../AutofillDatabaseProvider.swift | 47 ++++++- .../AppGroupFileStorageManagerTests.swift | 116 ++++++++++++++++++ 3 files changed, 257 insertions(+), 2 deletions(-) create mode 100644 Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift create mode 100644 Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift diff --git a/Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift b/Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift new file mode 100644 index 000000000..127d61e79 --- /dev/null +++ b/Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift @@ -0,0 +1,96 @@ +// +// AppGroupFileStorageManager.swift +// DuckDuckGo +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import os.log + +public protocol FileStorageManaging { + func migrateDatabaseToSharedStorageIfNeeded(from databaseURL: URL, to sharedDatabaseURL: URL) throws -> URL +} + + +final public class AppGroupFileStorageManager: FileStorageManaging { + + private let fileManager: FileManager + + public init(fileManager: FileManager = FileManager.default) { + self.fileManager = fileManager + } + + private func fileExists(at url: URL) -> Bool { + return fileManager.fileExists(atPath: url.path) + } + + private func createDirectoryIfNeeded(at url: URL) throws { + let directoryPath = url.path + if !fileManager.fileExists(atPath: directoryPath) { + do { + try fileManager.createDirectory(at: url, withIntermediateDirectories: true, attributes: nil) + Logger.secureStorage.info("Created directory at \(directoryPath)") + } catch { + Logger.secureStorage.error("Failed to create directory: \(error.localizedDescription)") + throw error + } + } + } + + private func copyFile(from sourceURL: URL, to destinationURL: URL) throws { + do { + try fileManager.copyItem(at: sourceURL, to: destinationURL) + } catch { + Logger.secureStorage.error("Error moving file: \(error.localizedDescription)") + throw error + } + } + + private func removeFile(at url: URL) throws { + do { + try fileManager.removeItem(at: url) + Logger.secureStorage.info("Removed file at \(url.path)") + } catch { + Logger.secureStorage.error("Error removing file: \(error.localizedDescription)") + throw error + } + } + + public func migrateDatabaseToSharedStorageIfNeeded(from databaseURL: URL, to sharedDatabaseURL: URL) throws -> URL { + if fileExists(at: sharedDatabaseURL) { + return sharedDatabaseURL + } + + do { + // Ensure the shared group directory exists + try createDirectoryIfNeeded(at: sharedDatabaseURL.deletingLastPathComponent()) + + if fileExists(at: databaseURL) { + try copyFile(from: databaseURL, to: sharedDatabaseURL) + + // If the copy was successful, delete the original file + try removeFile(at: databaseURL) + + return sharedDatabaseURL + } + } catch { + Logger.secureStorage.error("Failed to migrate Vault.db: \(error.localizedDescription)") + throw error + } + + return sharedDatabaseURL + } +} diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift index b145523ea..461ef2dc9 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift @@ -20,6 +20,7 @@ import Common import Foundation import GRDB import SecureStorage +import os.log public protocol AutofillDatabaseProvider: SecureStorageDatabaseProvider { @@ -82,14 +83,35 @@ public protocol AutofillDatabaseProvider: SecureStorageDatabaseProvider { public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabaseProvider, AutofillDatabaseProvider { + struct Constants { + static let dbDirectoryName = "Vault" + static let dbFileName = "Vault.db" + } + public static func defaultDatabaseURL() -> URL { - return DefaultAutofillDatabaseProvider.databaseFilePath(directoryName: "Vault", fileName: "Vault.db") + return DefaultAutofillDatabaseProvider.databaseFilePath(directoryName: Constants.dbDirectoryName, fileName: Constants.dbFileName, appGroupIdentifier: nil) + } + + public static func defaultSharedDatabaseURL() -> URL { + return DefaultAutofillDatabaseProvider.databaseFilePath(directoryName: Constants.dbDirectoryName, fileName: Constants.dbFileName, appGroupIdentifier: Bundle.main.appGroupPrefix + ".vault") } public init(file: URL = DefaultAutofillDatabaseProvider.defaultDatabaseURL(), key: Data, + fileStorageManager: FileStorageManaging = AppGroupFileStorageManager(), customMigrations: ((inout DatabaseMigrator) -> Void)? = nil) throws { - try super.init(file: file, key: key, writerType: .queue) { migrator in + let databaseURL: URL + +#if os(iOS) + databaseURL = Self.migrateDatabaseToSharedGroupIfNeeded(using: fileStorageManager, + from: Self.defaultDatabaseURL(), + to: Self.defaultSharedDatabaseURL()) +#else + // macOS stays in its sandbox location + databaseURL = file +#endif + + try super.init(file: databaseURL, key: key, writerType: .queue) { migrator in if let customMigrations { customMigrations(&migrator) } else { @@ -110,6 +132,17 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro } } + private static func migrateDatabaseToSharedGroupIfNeeded(using fileStorageManager: FileStorageManaging = AppGroupFileStorageManager(), + from databaseURL: URL, + to sharedDatabaseURL: URL) -> URL { + do { + return try fileStorageManager.migrateDatabaseToSharedStorageIfNeeded(from: databaseURL, to: sharedDatabaseURL) + } catch { + Logger.secureStorage.error("Failed to migrate database to shared storage: \(error.localizedDescription)") + return databaseURL + } + } + public func inTransaction(_ block: @escaping (GRDB.Database) throws -> Void) throws { try db.write { database in try block(database) @@ -1412,3 +1445,13 @@ extension SecureVaultModels.Identity: PersistableRecord, FetchableRecord { public static var databaseTableName: String = "identities" } + +private extension Bundle { + var appGroupPrefix: String { + let groupIdPrefixKey = "DuckDuckGoGroupIdentifierPrefix" + guard let groupIdPrefix = Bundle.main.object(forInfoDictionaryKey: groupIdPrefixKey) as? String else { + fatalError("Info.plist must contain a \"\(groupIdPrefixKey)\" entry with a string value") + } + return groupIdPrefix + } +} diff --git a/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift new file mode 100644 index 000000000..3edec0218 --- /dev/null +++ b/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift @@ -0,0 +1,116 @@ +// +// AppGroupFileStorageManagerTests.swift +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import XCTest +@testable import BrowserServicesKit + +final class AppGroupFileStorageManagerTests: XCTestCase { + + var mockFileManager: MockFileManager! + var fileStorageManager: AppGroupFileStorageManager! + + override func setUp() { + super.setUp() + mockFileManager = MockFileManager() + fileStorageManager = AppGroupFileStorageManager(fileManager: mockFileManager) + } + + func testWhenMigrateDatabaseToSharedStorageIfNeeded_ThenMigratesDatabaseSuccessfully() throws { + let originalURL = URL(fileURLWithPath: "/path/to/Vault.db") + let sharedDatabaseURL = URL(fileURLWithPath: "/shared/path/Vault.db") + + mockFileManager.files.insert(originalURL) + + let resultURL = try fileStorageManager.migrateDatabaseToSharedStorageIfNeeded(from: originalURL, to: sharedDatabaseURL) + + XCTAssertEqual(resultURL, sharedDatabaseURL, "The shared database URL should be returned after migration.") + XCTAssertTrue(mockFileManager.files.contains(sharedDatabaseURL), "Shared database should exist after migration.") + XCTAssertTrue(mockFileManager.files.contains(sharedDatabaseURL), "The file should have been copied to the shared location.") + XCTAssertFalse(mockFileManager.files.contains(originalURL), "The original file should have been deleted after a successful migration.") + } + + func testWhenMigrateDatabaseToSharedStorageIfNeeded_ThenDoesNotMigrateIfSharedDatabaseExists() throws { + let originalURL = URL(fileURLWithPath: "/path/to/Vault.db") + let sharedDatabaseURL = URL(fileURLWithPath: "/shared/path/Vault.db") + + mockFileManager.files.insert(originalURL) + mockFileManager.files.insert(sharedDatabaseURL) + + let resultURL = try fileStorageManager.migrateDatabaseToSharedStorageIfNeeded(from: originalURL, to: sharedDatabaseURL) + + XCTAssertEqual(resultURL, sharedDatabaseURL, "The shared database URL should be returned since it already exists.") + XCTAssertTrue(mockFileManager.files.contains(sharedDatabaseURL), "The shared database should still exist.") + XCTAssertTrue(mockFileManager.files.contains(originalURL), "The original file should not be deleted if the shared database already exists.") + } + + func testWhenMigrateDatabaseToSharedStorageIfNeeded_ThenRestoresOriginalIfCopyFails() throws { + let originalURL = URL(fileURLWithPath: "/path/to/Vault.db") + let sharedDatabaseURL = URL(fileURLWithPath: "/shared/path/Vault.db") + + mockFileManager.files.insert(originalURL) + // Simulate copy failure + mockFileManager.shouldFailOnCopy = true + + var returnedURL: URL? + do { + returnedURL = try fileStorageManager.migrateDatabaseToSharedStorageIfNeeded(from: originalURL, to: sharedDatabaseURL) + XCTFail("Expected failure when copying the file.") + } catch { + // Expected failure + } + + XCTAssertNil(returnedURL, "The migration should fail and no URL should be returned.") + XCTAssertTrue(mockFileManager.files.contains(originalURL), "The original file should still exist after a failed migration.") + } +} + +final class MockFileManager: FileManager { + var files: Set = [] + var createdDirectories: Set = [] + var copiedFiles: [(from: URL, to: URL)] = [] + var movedFiles: [(from: URL, to: URL)] = [] + var removedFiles: [URL] = [] + var shouldFailOnCopy = false + + override func fileExists(atPath path: String) -> Bool { + return files.contains(URL(fileURLWithPath: path)) + } + + override func copyItem(at srcURL: URL, to dstURL: URL) throws { + if shouldFailOnCopy { + throw NSError(domain: "MockFileManager", code: 1, userInfo: [NSLocalizedDescriptionKey: "Simulated copy failure"]) + } + if !files.contains(srcURL) { + throw NSError(domain: "MockFileManager", code: 1, userInfo: [NSLocalizedDescriptionKey: "File not found"]) + } + files.insert(dstURL) + copiedFiles.append((from: srcURL, to: dstURL)) + } + + override func createDirectory(at url: URL, withIntermediateDirectories createIntermediates: Bool, attributes: [FileAttributeKey : Any]? = nil) throws { + createdDirectories.insert(url) + } + + override func removeItem(at URL: URL) throws { + if !files.contains(URL) { + throw NSError(domain: "MockFileManager", code: 1, userInfo: [NSLocalizedDescriptionKey: "File not found"]) + } + files.remove(URL) + removedFiles.append(URL) + } +} From 0565fcd75fffe71796bed2b4661787d94fb67576 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Mon, 21 Oct 2024 16:02:37 +0200 Subject: [PATCH 4/8] Linter violation fixes --- ...roupFileStorageManager.swift => FileStorageManaging.swift} | 4 +--- .../SecureVault/AppGroupFileStorageManagerTests.swift | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) rename Sources/BrowserServicesKit/SecureVault/{AppGroupFileStorageManager.swift => FileStorageManaging.swift} (98%) diff --git a/Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift b/Sources/BrowserServicesKit/SecureVault/FileStorageManaging.swift similarity index 98% rename from Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift rename to Sources/BrowserServicesKit/SecureVault/FileStorageManaging.swift index 127d61e79..fdcf6e210 100644 --- a/Sources/BrowserServicesKit/SecureVault/AppGroupFileStorageManager.swift +++ b/Sources/BrowserServicesKit/SecureVault/FileStorageManaging.swift @@ -1,6 +1,5 @@ // -// AppGroupFileStorageManager.swift -// DuckDuckGo +// FileStorageManaging.swift // // Copyright © 2024 DuckDuckGo. All rights reserved. // @@ -24,7 +23,6 @@ public protocol FileStorageManaging { func migrateDatabaseToSharedStorageIfNeeded(from databaseURL: URL, to sharedDatabaseURL: URL) throws -> URL } - final public class AppGroupFileStorageManager: FileStorageManaging { private let fileManager: FileManager diff --git a/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift index 3edec0218..ff9cda5e6 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/AppGroupFileStorageManagerTests.swift @@ -102,7 +102,7 @@ final class MockFileManager: FileManager { copiedFiles.append((from: srcURL, to: dstURL)) } - override func createDirectory(at url: URL, withIntermediateDirectories createIntermediates: Bool, attributes: [FileAttributeKey : Any]? = nil) throws { + override func createDirectory(at url: URL, withIntermediateDirectories createIntermediates: Bool, attributes: [FileAttributeKey: Any]? = nil) throws { createdDirectories.insert(url) } From 2736df6c39d1cd576f76c739036844ebf247ba2b Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 3 Dec 2024 13:21:03 +0100 Subject: [PATCH 5/8] Update for unit testing --- .../SecureVault/AutofillDatabaseProvider.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift index 461ef2dc9..dae41804d 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift @@ -1450,7 +1450,11 @@ private extension Bundle { var appGroupPrefix: String { let groupIdPrefixKey = "DuckDuckGoGroupIdentifierPrefix" guard let groupIdPrefix = Bundle.main.object(forInfoDictionaryKey: groupIdPrefixKey) as? String else { + #if DEBUG && os(iOS) + return "group.com.duckduckgo.test" + #else fatalError("Info.plist must contain a \"\(groupIdPrefixKey)\" entry with a string value") + #endif } return groupIdPrefix } From eb7e4af1a0264f3b9499e28249ef9d06c897f067 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 3 Dec 2024 13:47:50 +0100 Subject: [PATCH 6/8] Update for unit testing --- .../SecureVault/AutofillKeyStoreProvider.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift index d94e184f1..b3eed9b99 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillKeyStoreProvider.swift @@ -295,7 +295,11 @@ fileprivate extension Bundle { var appGroupName: String { guard let appGroup = object(forInfoDictionaryKey: Bundle.vaultAppGroupName) as? String else { + #if DEBUG && os(iOS) + return "com.duckduckgo.vault.test" + #else fatalError("Info.plist is missing \(Bundle.vaultAppGroupName)") + #endif } return appGroup } From 3b030bdb9b8a2d294949be582cc3d6a8a3c9fb28 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 3 Dec 2024 14:56:51 +0100 Subject: [PATCH 7/8] Update to unit test to support migrated vault.db file --- .../SecureVault/SecureVaultSyncableCredentialsTests.swift | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultSyncableCredentialsTests.swift b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultSyncableCredentialsTests.swift index 26c0a649a..2270750e1 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultSyncableCredentialsTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultSyncableCredentialsTests.swift @@ -286,6 +286,13 @@ class SecureVaultSyncableCredentialsTests: XCTestCase { try FileManager.default.removeItem(atPath: dbFileContainer.appendingPathComponent(file).path) } +#if os(iOS) + let sharedDbFileContainer = DefaultAutofillDatabaseProvider.defaultSharedDatabaseURL().deletingLastPathComponent() + for file in try FileManager.default.contentsOfDirectory(atPath: sharedDbFileContainer.path) { + guard ["db", "bak"].contains((file as NSString).pathExtension) else { continue } + try FileManager.default.removeItem(atPath: sharedDbFileContainer.appendingPathComponent(file).path) + } +#endif } catch let error as NSError { // File not found if error.domain != NSCocoaErrorDomain || error.code != 4 { From ae0d97c90b7ff310bf37a4302182e39d3ebf0c92 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 3 Dec 2024 16:14:01 +0100 Subject: [PATCH 8/8] Update to unit test to support migrated vault.db file --- .../SecureVault/DatabaseProviderTests.swift | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Tests/BrowserServicesKitTests/SecureVault/DatabaseProviderTests.swift b/Tests/BrowserServicesKitTests/SecureVault/DatabaseProviderTests.swift index f791954ae..8ad2e578d 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/DatabaseProviderTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/DatabaseProviderTests.swift @@ -34,6 +34,13 @@ class DatabaseProviderTests: XCTestCase { try FileManager.default.removeItem(atPath: dbFileContainer.appendingPathComponent(file).path) } + #if os(iOS) + let sharedDbFileContainer = DefaultAutofillDatabaseProvider.defaultSharedDatabaseURL().deletingLastPathComponent() + for file in try FileManager.default.contentsOfDirectory(atPath: sharedDbFileContainer.path) { + guard ["db", "bak"].contains((file as NSString).pathExtension) else { continue } + try FileManager.default.removeItem(atPath: sharedDbFileContainer.appendingPathComponent(file).path) + } + #endif } catch let error as NSError { // File not found if error.domain != NSCocoaErrorDomain || error.code != 4 {