BIT-2316: Fix vault timeout never lock migration

Bitwarden/Application/Support/Info.plist

@@ -4,6 +4,8 @@
 <dict>
 	<key>BitwardenAppIdentifier</key>
 	<string>$(BASE_BUNDLE_ID)</string>
+	<key>BitwardenKeychainAccessGroup</key>
+	<string>$(AppIdentifierPrefix)$(BASE_BUNDLE_ID)</string>
 	<key>CADisableMinimumFrameDurationOnPhone</key>
 	<true/>
 	<key>CFBundleDevelopmentRegion</key>

BitwardenActionExtension/Application/Support/Info.plist

@@ -4,6 +4,8 @@
 <dict>
 	<key>BitwardenAppIdentifier</key>
 	<string>$(BASE_BUNDLE_ID)</string>
+    <key>BitwardenKeychainAccessGroup</key>
+    <string>$(AppIdentifierPrefix)$(BASE_BUNDLE_ID)</string>
 	<key>CFBundleDisplayName</key>
 	<string>Autofill with Bitwarden</string>
 	<key>CFBundleName</key>

BitwardenAutoFillExtension/Application/Support/Info.plist

@@ -4,6 +4,8 @@
 <dict>
 	<key>BitwardenAppIdentifier</key>
 	<string>$(BASE_BUNDLE_ID)</string>
+    <key>BitwardenKeychainAccessGroup</key>
+    <string>$(AppIdentifierPrefix)$(BASE_BUNDLE_ID)</string>
 	<key>CFBundleDisplayName</key>
 	<string>Bitwarden</string>
 	<key>CFBundleName</key>

BitwardenShareExtension/Application/Support/Info.plist

@@ -4,6 +4,8 @@
 <dict>
 	<key>BitwardenAppIdentifier</key>
 	<string>$(BASE_BUNDLE_ID)</string>
+    <key>BitwardenKeychainAccessGroup</key>
+    <string>$(AppIdentifierPrefix)$(BASE_BUNDLE_ID)</string>
 	<key>CFBundleDisplayName</key>
 	<string>Bitwarden Share</string>
 	<key>CFBundleName</key>

BitwardenShared/Core/Auth/Services/KeychainRepository.swift

@@ -183,18 +183,20 @@ class DefaultKeychainRepository: KeychainRepository {
     ///
     let appIdService: AppIdService
 
-    /// An identifier for this application and extensions.
-    ///   ie: "LTZ2PFU5D6.com.8bit.bitwarden"
+    /// An identifier for the keychain service used by the application and extensions.
+    ///
+    /// Example: "com.8bit.bitwarden".
     ///
     var appSecAttrService: String {
         Bundle.main.appIdentifier
     }
 
-    /// An identifier for this application group and extensions
-    ///   ie: "group.LTZ2PFU5D6.com.8bit.bitwarden"
+    /// An identifier for the keychain access group used by the application group and extensions.
+    ///
+    /// Example: "LTZ2PFU5D6.com.8bit.bitwarden"
     ///
     var appSecAttrAccessGroup: String {
-        Bundle.main.groupIdentifier
+        Bundle.main.keychainAccessGroup
     }
 
     /// The keychain service used by the repository

BitwardenShared/Core/Auth/Services/KeychainRepositoryTests.swift

@@ -51,7 +51,7 @@ final class KeychainRepositoryTests: BitwardenTestCase { // swiftlint:disable:th
     ///
     func test_appSecAttrAccessGroup() {
         XCTAssertEqual(
-            Bundle.main.groupIdentifier,
+            Bundle.main.keychainAccessGroup,
             subject.appSecAttrAccessGroup
         )
     }

BitwardenShared/Core/Platform/Extensions/Bundle+Extensions.swift

@@ -32,4 +32,9 @@ extension Bundle {
     var groupIdentifier: String {
         "group." + appIdentifier
     }
+
+    /// Return's the app's access group identifier for storing keychain items.
+    var keychainAccessGroup: String {
+        infoDictionary?["BitwardenKeychainAccessGroup"] as? String ?? appIdentifier
+    }
 }

BitwardenShared/Core/Platform/Services/MigrationService.swift

@@ -26,6 +26,9 @@ class DefaultMigrationService {
     /// The repository used to manage keychain items.
     let keychainRepository: KeychainRepository
 
+    /// The service name associated with the app's keychain items.
+    let keychainServiceName: String
+
     /// The shared UserDefaults instance (NOTE: this should be the standard one just for the app,
     /// not one in the app group).
     let standardUserDefaults: UserDefaults
@@ -38,17 +41,20 @@ class DefaultMigrationService {
     ///   - appSettingsStore: The service used by the application to persist app setting values.
     ///   - errorReporter: The service used by the application to report non-fatal errors.
     ///   - keychainRepository: The repository used to manage keychain items.
+    ///   - keychainServiceName: The service name associated with the app's keychain items.
     ///   - standardUserDefaults: The shared UserDefaults instance.
     ///
     init(
         appSettingsStore: AppSettingsStore,
         errorReporter: ErrorReporter,
         keychainRepository: KeychainRepository,
+        keychainServiceName: String = Bundle.main.appIdentifier,
         standardUserDefaults: UserDefaults = .standard
     ) {
         self.appSettingsStore = appSettingsStore
         self.errorReporter = errorReporter
         self.keychainRepository = keychainRepository
+        self.keychainServiceName = keychainServiceName
         self.standardUserDefaults = standardUserDefaults
     }
 
@@ -90,23 +96,95 @@ class DefaultMigrationService {
             try await keychainRepository.setRefreshToken(tokens.refreshToken, userId: accountId)
         }
     }
+
+    /// Performs migration 2.
+    ///
+    /// Notes:
+    /// - Migrate Keychain items, migrating data in kSecAttrGeneric to kSecValueData.
+    ///
+    private func performMigration2() async throws {
+        let query = [
+            kSecClass: kSecClassGenericPassword,
+            kSecMatchLimit: kSecMatchLimitAll,
+            kSecAttrService: keychainServiceName,
+            kSecReturnData: true,
+            kSecReturnAttributes: true,
+        ] as CFDictionary
+        var keychainItems: AnyObject?
+        let status = SecItemCopyMatching(query, &keychainItems)
+        guard status == errSecSuccess else {
+            Logger.application.error("Error searching for keychain items: \(status)")
+            return
+        }
+
+        if let keychainItems = keychainItems as? NSArray {
+            for keychainItem in keychainItems {
+                guard let itemDictionary = keychainItem as? NSDictionary else { continue }
+
+                let query = [
+                    kSecClass: kSecClassGenericPassword,
+                    kSecAttrAccount: itemDictionary[kSecAttrAccount],
+                    kSecAttrService: keychainServiceName,
+                ] as CFDictionary
+
+                var attributesToUpdate: [CFString: Any] = [
+                    kSecAttrAccessGroup: Bundle.main.keychainAccessGroup,
+                ]
+                if let genericData = itemDictionary[kSecAttrGeneric] as? Data,
+                   !genericData.isEmpty,
+                   itemDictionary[kSecValueData] == nil {
+                    // Migrate data from kSecAttrGeneric to kSecValueData.
+                    attributesToUpdate[kSecValueData] = genericData
+                    attributesToUpdate[kSecAttrGeneric] = Data()
+                }
+
+                let status = SecItemUpdate(query, attributesToUpdate as CFDictionary)
+                guard status == errSecSuccess else {
+                    Logger.application.error("Error updating keychain item: \(status)")
+                    continue
+                }
+            }
+        }
+    }
+}
+
+extension DefaultMigrationService {
+    /// The list of migrations that can be performed.
+    var migrations: [() async throws -> Void] {
+        [
+            performMigration1,
+            performMigration2,
+        ]
+    }
+
+    /// Performs a single migration for a migration version number.
+    ///
+    /// - Note: `performMigrations()` should be used in almost all cases to perform the full set of
+    ///     migrations. This exists to allow tests to perform a single migration.
+    ///
+    /// - Parameter version: The migration version to perform.
+    ///
+    func performMigration(version: Int) async throws {
+        let migrationIndex = version - 1
+        guard migrationIndex >= 0, migrationIndex < migrations.count else { return }
+        try await migrations[migrationIndex]()
+        appSettingsStore.migrationVersion = version
+    }
 }
 
 extension DefaultMigrationService: MigrationService {
     func performMigrations() async {
         var migrationVersion = appSettingsStore.migrationVersion
         defer { appSettingsStore.migrationVersion = migrationVersion }
 
-        // The list of migrations that can be performed.
-        let migrations: [(version: Int, method: () async throws -> Void)] = [
-            (1, performMigration1),
-        ]
-
         do {
-            for migration in migrations where migrationVersion < migration.version {
-                try await migration.method()
-                migrationVersion = migration.version
-                Logger.application.info("Completed data migration \(migration.version)")
+            for (migrationIndex, migration) in migrations.enumerated() {
+                let version = migrationIndex + 1
+                guard migrationVersion < version else { continue }
+
+                try await migration()
+                migrationVersion = version
+                Logger.application.info("Completed data migration \(version)")
             }
         } catch {
             errorReporter.log(error: error)

BitwardenShared/Core/Platform/Services/MigrationServiceTests.swift

@@ -22,11 +22,13 @@ class MigrationServiceTests: BitwardenTestCase {
         standardUserDefaults = UserDefaults(suiteName: "test")
 
         standardUserDefaults.removeObject(forKey: "MSAppCenterCrashesIsEnabled")
+        SecItemDelete([kSecClass: kSecClassGenericPassword] as CFDictionary)
 
         subject = DefaultMigrationService(
             appSettingsStore: appSettingsStore,
             errorReporter: errorReporter,
             keychainRepository: keychainRepository,
+            keychainServiceName: "com.bitwarden.test",
             standardUserDefaults: standardUserDefaults
         )
     }
@@ -43,6 +45,15 @@ class MigrationServiceTests: BitwardenTestCase {
 
     // MARK: Tests
 
+    /// `performMigrations()` performs all migrations and updates the migration version.
+    func test_performMigrations() async throws {
+        appSettingsStore.migrationVersion = 0
+
+        await subject.performMigrations()
+
+        XCTAssertEqual(appSettingsStore.migrationVersion, subject.migrations.count)
+    }
+
     /// `performMigrations()` logs an error to the error reporter if one occurs.
     func test_performMigrations_error() async throws {
         appSettingsStore.migrationVersion = 0
@@ -91,7 +102,7 @@ class MigrationServiceTests: BitwardenTestCase {
             appSettingsStore.notificationsLastRegistrationDates[userId] = Date()
         }
 
-        await subject.performMigrations()
+        try await subject.performMigration(version: 1)
 
         XCTAssertEqual(appSettingsStore.migrationVersion, 1)
 
@@ -121,7 +132,7 @@ class MigrationServiceTests: BitwardenTestCase {
     func test_performMigrations_1_withAppCenterCrashesKey_false() async throws {
         appSettingsStore.migrationVersion = 0
         standardUserDefaults.setValue(false, forKey: "MSAppCenterCrashesIsEnabled")
-        await subject.performMigrations()
+        try await subject.performMigration(version: 1)
         XCTAssertFalse(errorReporter.isEnabled)
     }
 
@@ -130,7 +141,7 @@ class MigrationServiceTests: BitwardenTestCase {
     func test_performMigrations_1_withAppCenterCrashesKey_true() async throws {
         appSettingsStore.migrationVersion = 0
         standardUserDefaults.setValue(true, forKey: "MSAppCenterCrashesIsEnabled")
-        await subject.performMigrations()
+        try await subject.performMigration(version: 1)
         XCTAssertTrue(errorReporter.isEnabled)
     }
 
@@ -139,11 +150,61 @@ class MigrationServiceTests: BitwardenTestCase {
         appSettingsStore.migrationVersion = 0
         appSettingsStore.state = nil
 
-        await subject.performMigrations()
+        try await subject.performMigration(version: 1)
 
         XCTAssertEqual(appSettingsStore.migrationVersion, 1)
         XCTAssertNil(appSettingsStore.state)
         XCTAssertTrue(keychainRepository.deleteAllItemsCalled)
         XCTAssertTrue(errorReporter.isEnabled)
     }
+
+    /// `performMigrations()` for migration 2 migrates keychain data in kSecAttrGeneric to kSecValueData.
+    func test_performMigrations_2() async throws {
+        let itemsToAdd: [(account: String, value: String)] = [
+            ("TEST_ACCOUNT_1", "secret"),
+            ("TEST_ACCOUNT_2", "password"),
+        ]
+        for item in itemsToAdd {
+            SecItemAdd(
+                [
+                    kSecClass: kSecClassGenericPassword,
+                    kSecAttrAccount: item.account,
+                    kSecAttrService: "com.bitwarden.test",
+                    kSecAttrGeneric: Data(item.value.utf8),
+                ] as CFDictionary,
+                nil
+            )
+        }
+
+        try await subject.performMigration(version: 2)
+
+        var copyResult: AnyObject?
+        SecItemCopyMatching(
+            [
+                kSecClass: kSecClassGenericPassword,
+                kSecAttrService: "com.bitwarden.test",
+                kSecMatchLimit: kSecMatchLimitAll,
+                kSecReturnData: true,
+                kSecReturnAttributes: true,
+            ] as CFDictionary,
+            &copyResult
+        )
+
+        let keychainItems = try XCTUnwrap(copyResult as? [[CFString: Any]])
+        XCTAssertEqual(keychainItems.count, 2)
+
+        let item1 = try XCTUnwrap(keychainItems[0])
+        XCTAssertEqual(item1[kSecAttrAccessGroup] as? String, Bundle.main.keychainAccessGroup)
+        XCTAssertEqual(item1[kSecAttrAccount] as? String, "TEST_ACCOUNT_1")
+        XCTAssertEqual(item1[kSecAttrGeneric] as? Data, Data())
+        XCTAssertEqual(item1[kSecValueData] as? Data, Data("secret".utf8))
+
+        let item2 = try XCTUnwrap(keychainItems[1])
+        XCTAssertEqual(item2[kSecAttrAccessGroup] as? String, Bundle.main.keychainAccessGroup)
+        XCTAssertEqual(item2[kSecAttrAccount] as? String, "TEST_ACCOUNT_2")
+        XCTAssertEqual(item2[kSecAttrGeneric] as? Data, Data())
+        XCTAssertEqual(item2[kSecValueData] as? Data, Data("password".utf8))
+
+        XCTAssertEqual(appSettingsStore.migrationVersion, 2)
+    }
 }

BitwardenShared/Core/Platform/Services/ServiceContainer.swift

@@ -276,7 +276,11 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
         )
         let timeProvider = CurrentTime()
 
-        let stateService = DefaultStateService(appSettingsStore: appSettingsStore, dataStore: dataStore)
+        let stateService = DefaultStateService(
+            appSettingsStore: appSettingsStore,
+            dataStore: dataStore,
+            keychainRepository: keychainRepository
+        )
 
         let clientBuilder = DefaultClientBuilder(errorReporter: errorReporter)
         let clientService = DefaultClientService(

BitwardenShared/Core/Platform/Services/StateService.swift

@@ -945,6 +945,9 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
     /// A subject containing the last sync time mapped to user ID.
     private var lastSyncTimeByUserIdSubject = CurrentValueSubject<[String: Date], Never>([:])
 
+    /// A service used to access data in the keychain.
+    private let keychainRepository: KeychainRepository
+
     /// A subject containing whether to show the website icons.
     private var showWebIconsSubject: CurrentValueSubject<Bool, Never>
 
@@ -955,13 +958,16 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
     /// - Parameters:
     ///  - appSettingsStore: The service that persists app settings.
     ///  - dataStore: The data store that handles performing data requests.
+    ///  - keychainRepository: A service used to access data in the keychain.
     ///
     init(
         appSettingsStore: AppSettingsStore,
-        dataStore: DataStore
+        dataStore: DataStore,
+        keychainRepository: KeychainRepository
     ) {
         self.appSettingsStore = appSettingsStore
         self.dataStore = dataStore
+        self.keychainRepository = keychainRepository
 
         appThemeSubject = CurrentValueSubject(AppTheme(appSettingsStore.appTheme))
         showWebIconsSubject = CurrentValueSubject(!appSettingsStore.disableWebIcons)
@@ -1148,7 +1154,8 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
     func getVaultTimeout(userId: String?) async throws -> SessionTimeoutValue {
         let userId = try getAccount(userId: userId).profile.userId
         guard let rawValue = appSettingsStore.vaultTimeout(userId: userId) else {
-            return .fifteenMinutes
+            let userAuthKey = try? await keychainRepository.getUserAuthKeyValue(for: .neverLock(userId: userId))
+            return userAuthKey == nil ? .fifteenMinutes : .never
         }
         return SessionTimeoutValue(rawValue: rawValue)
     }