PM-14620: Remove unused accessSecretsManager property to fix JSON dec…

…oding errors (#1116) Co-authored-by: Brant DeBow <[email protected]> Co-authored-by: Federico Maccaroni <[email protected]>
bitwarden · Nov 8, 2024 · 91a1853 · 91a1853
1 parent 627ecd0
commit 91a1853
Show file tree

Hide file tree

Showing 15 changed files with 299 additions and 13 deletions.
diff --git a/BitwardenShared/Core/Platform/Services/AuthenticatorSyncService.swift b/BitwardenShared/Core/Platform/Services/AuthenticatorSyncService.swift
@@ -47,6 +47,13 @@ actor DefaultAuthenticatorSyncService: NSObject, AuthenticatorSyncService {
     /// The service to get server-specified configuration.
     private let configService: ConfigService
 
+    /// A task that sets up syncing for a user.
+    ///
+    /// Using an actor-protected `Task` ensures that multiple threads don't attempt
+    /// to setup sync at the same time. `enableSyncForUserId(_)` `await`s
+    /// the result of this task before adding the next sync setup to the end of it.
+    private var enableSyncTask: Task<Void, Never>?
+
     /// The service used by the application to report non-fatal errors.
     private let errorReporter: ErrorReporter
 
@@ -246,19 +253,34 @@ actor DefaultAuthenticatorSyncService: NSObject, AuthenticatorSyncService {
     /// - Parameter userId: The userId of the user whose sync status is being determined.
     ///
     private func determineSyncForUserId(_ userId: String) async throws {
-        guard try await stateService.getSyncToAuthenticator(userId: userId) else {
+        if try await stateService.getSyncToAuthenticator(userId: userId) {
+            enableSyncForUserId(userId)
+        } else {
             cipherPublisherTasks[userId]?.cancel()
             cipherPublisherTasks.removeValue(forKey: userId)
             try await keychainRepository.deleteAuthenticatorVaultKey(userId: userId)
             try await authBridgeItemService.deleteAllForUserId(userId)
             try await deleteKeyIfSyncingIsOff()
-            return
         }
+    }
+
+    /// Enable sync for the provided userId.
+    ///
+    /// - Parameter userId: The userId of the user whose sync is being enabled.
+    ///
+    private func enableSyncForUserId(_ userId: String) {
+        guard !vaultTimeoutService.isLocked(userId: userId) else { return }
+
+        enableSyncTask = Task { [enableSyncTask] in
+            _ = await enableSyncTask?.result
 
-        if !vaultTimeoutService.isLocked(userId: userId) {
-            try await createAuthenticatorKeyIfNeeded()
-            try await createAuthenticatorVaultKeyIfNeeded(userId: userId)
-            subscribeToCipherUpdates(userId: userId)
+            do {
+                try await createAuthenticatorKeyIfNeeded()
+                try await createAuthenticatorVaultKeyIfNeeded(userId: userId)
+                subscribeToCipherUpdates(userId: userId)
+            } catch {
+                errorReporter.log(error: error)
+            }
         }
     }
 

diff --git a/BitwardenShared/Core/Platform/Services/AuthenticatorSyncServiceTests.swift b/BitwardenShared/Core/Platform/Services/AuthenticatorSyncServiceTests.swift
@@ -51,6 +51,7 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
     override func tearDown() {
         super.tearDown()
 
+        subject = nil
         authBridgeItemService = nil
         authRepository = nil
         cipherDataStore = nil
@@ -60,7 +61,6 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         keychainRepository = nil
         sharedKeychainRepository = nil
         stateService = nil
-        subject = nil
         vaultTimeoutService = nil
     }
 
@@ -464,7 +464,9 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         // Unsubscribe from sync, wait for items to be deleted
         stateService.syncToAuthenticatorByUserId["1"] = false
         stateService.syncToAuthenticatorSubject.send(("1", false))
-        waitFor((authBridgeItemService.storedItems["1"]?.isEmpty) ?? false)
+        try await waitForAsync {
+            (self.authBridgeItemService.storedItems["1"]?.isEmpty) ?? false
+        }
 
         // Sending additional updates should not appear in Store
         cipherDataStore.cipherSubjectByUserId["1"]?.send([
@@ -481,6 +483,37 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         XCTAssertTrue(authBridgeItemService.storedItems["1"]?.isEmpty ?? false)
     }
 
+    /// The sync service should be properly handling multiple publishes which could happen on multiple threads.
+    /// By generating a `send` on both the sync status and the vault unlock, the service will receive two
+    /// simultaneous attempts to determine syncing.
+    ///
+    @MainActor
+    func test_determineSyncForUserId_threadSafetyCheck() async throws {
+        setupInitialState()
+        await subject.start()
+
+        for _ in 0 ..< 4 {
+            async let result1: Void = stateService.syncToAuthenticatorSubject.send(("1", true))
+            async let result2: Void = vaultTimeoutService.vaultLockStatusSubject.send(
+                VaultLockStatus(isVaultLocked: false, userId: "1")
+            )
+            await _ = (result1, result2)
+        }
+
+        cipherDataStore.cipherSubjectByUserId["1"]?.send([
+            .fixture(
+                id: "1234",
+                login: .fixture(
+                    username: "[email protected]",
+                    totp: "totp"
+                )
+            ),
+        ])
+        try await waitForAsync {
+            self.authBridgeItemService.storedItems["1"]?.first != nil
+        }
+    }
+
     /// When user "1" has sync turned on and user "2" unlocks their vault, the service should not take
     /// any action because "1" has a locked vault and "2" doesn't have sync turned on.
     ///

diff --git a/BitwardenShared/Core/Platform/Services/StateService.swift b/BitwardenShared/Core/Platform/Services/StateService.swift
@@ -29,6 +29,13 @@ protocol StateService: AnyObject {
     ///
     func deleteAccount() async throws
 
+    /// Returns whether the active account was switched in the extension. This compares the current
+    /// active account in memory with what's stored on disk to determine if the account was switched.
+    ///
+    /// - Returns: Whether the active was switched in the extension.
+    ///
+    func didAccountSwitchInExtension() async throws -> Bool
+
     /// Returns whether the active user account has access to premium features.
     ///
     /// - Returns: Whether the active account has access to premium features.
@@ -1309,6 +1316,18 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
         try await logoutAccount(userInitiated: true)
     }
 
+    func didAccountSwitchInExtension() async throws -> Bool {
+        do {
+            return try getActiveAccountUserId() != appSettingsStore.cachedActiveUserId
+        } catch StateServiceError.noActiveAccount {
+            let cachedActiveUserId = appSettingsStore.cachedActiveUserId
+            // If the user was logged out in the extension, but there's a cached active user,
+            // reset the state to update the cached active user.
+            appSettingsStore.state = appSettingsStore.state
+            return cachedActiveUserId != nil
+        }
+    }
+
     func doesActiveAccountHavePremium() async throws -> Bool {
         let account = try await getActiveAccount()
         let hasPremiumPersonally = account.profile.hasPremiumPersonally ?? false

diff --git a/BitwardenShared/Core/Platform/Services/StateServiceTests.swift b/BitwardenShared/Core/Platform/Services/StateServiceTests.swift
@@ -129,6 +129,46 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
         XCTAssertNil(state.activeUserId)
     }
 
+    /// `didAccountSwitchInExtension` returns `false` if there's no active user.
+    func test_didAccountSwitchInExtension_noActiveUser() async throws {
+        let didSwitch = try await subject.didAccountSwitchInExtension()
+        XCTAssertFalse(didSwitch)
+    }
+
+    /// `didAccountSwitchInExtension` returns `true` if there's a cached active user but no active
+    /// user in the state.
+    func test_didAccountSwitchInExtension_noActiveUser_cachedActiveUserId() async throws {
+        appSettingsStore.cachedActiveUserId = "1"
+        appSettingsStore.activeIdSubject.send("1")
+
+        var publishedValues = [String?]()
+        let publisher = appSettingsStore.activeIdSubject
+            .sink(receiveValue: { publishedValues.append($0) })
+        defer { publisher.cancel() }
+
+        let didSwitch = try await subject.didAccountSwitchInExtension()
+        XCTAssertTrue(didSwitch)
+        XCTAssertEqual(publishedValues, ["1", nil])
+    }
+
+    /// `didAccountSwitchInExtension` returns whether the active account was switched in the
+    /// extension.
+    func test_didAccountSwitchInExtension() async throws {
+        await subject.addAccount(.fixture(profile: .fixture(userId: "1")))
+        appSettingsStore.cachedActiveUserId = nil
+
+        var didSwitch = try await subject.didAccountSwitchInExtension()
+        XCTAssertTrue(didSwitch)
+
+        appSettingsStore.cachedActiveUserId = "1"
+        didSwitch = try await subject.didAccountSwitchInExtension()
+        XCTAssertFalse(didSwitch)
+
+        await subject.addAccount(.fixture(profile: .fixture(userId: "2")))
+        didSwitch = try await subject.didAccountSwitchInExtension()
+        XCTAssertTrue(didSwitch)
+    }
+
     /// `doesActiveAccountHavePremium()` with premium personally and no organizations returns true.
     func test_doesActiveAccountHavePremium_personalTrue_noOrganization() async throws {
         await subject.addAccount(.fixture(profile: .fixture(hasPremiumPersonally: true)))

diff --git a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift
@@ -21,6 +21,11 @@ protocol AppSettingsStore: AnyObject {
     /// The app's theme.
     var appTheme: String? { get set }
 
+    /// The last published active user ID by `activeAccountIdPublisher` in the current process.
+    /// If this is different than the active user ID in the `State`, the active user was likely
+    /// switched in an extension and the main app should update accordingly.
+    var cachedActiveUserId: String? { get }
+
     /// Whether to disable the website icons.
     var disableWebIcons: Bool { get set }
 
@@ -832,6 +837,10 @@ extension DefaultAppSettingsStore: AppSettingsStore {
         set { store(newValue, for: .appTheme) }
     }
 
+    var cachedActiveUserId: String? {
+        activeAccountIdSubject.value
+    }
+
     var disableWebIcons: Bool {
         get { fetch(for: .disableWebIcons) }
         set { store(newValue, for: .disableWebIcons) }

diff --git a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift
@@ -213,6 +213,26 @@ class AppSettingsStoreTests: BitwardenTestCase { // swiftlint:disable:this type_
         XCTAssertNil(userDefaults.string(forKey: "bwPreferencesStorage:theme"))
     }
 
+    /// `cachedActiveUserId` returns `nil` if there isn't a cached active user.
+    func test_cachedActiveUserId_isInitiallyNil() {
+        XCTAssertNil(subject.cachedActiveUserId)
+    }
+
+    /// `cachedActiveUserId` returns the user ID of the last active user ID in the current process.
+    func test_cachedActiveUserId_withValue() {
+        subject.state = State(accounts: ["1": .fixture()], activeUserId: "1")
+        XCTAssertEqual(subject.cachedActiveUserId, "1")
+
+        subject.state = State(
+            accounts: [
+                "1": .fixture(profile: .fixture(userId: "1")),
+                "2": .fixture(profile: .fixture(userId: "2")),
+            ],
+            activeUserId: "2"
+        )
+        XCTAssertEqual(subject.cachedActiveUserId, "2")
+    }
+
     /// `clearClipboardValue(userId:)` returns `.never` if there isn't a previously stored value.
     func test_clearClipboardValue_isInitiallyNever() {
         XCTAssertEqual(subject.clearClipboardValue(userId: "0"), .never)

diff --git a/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift b/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift
@@ -13,6 +13,7 @@ class MockAppSettingsStore: AppSettingsStore { // swiftlint:disable:this type_bo
     var appLocale: String?
     var appRehydrationState = [String: AppRehydrationState]()
     var appTheme: String?
+    var cachedActiveUserId: String?
     var disableWebIcons = false
     var introCarouselShown = false
     var lastUserShouldConnectToWatch = false

diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift
@@ -29,6 +29,7 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
     var connectToWatchSubject = CurrentValueSubject<(String?, Bool), Never>((nil, false))
     var timeProvider = MockTimeProvider(.currentTime)
     var defaultUriMatchTypeByUserId = [String: UriMatchType]()
+    var didAccountSwitchInExtensionResult: Result<Bool, Error> = .success(false)
     var disableAutoTotpCopyByUserId = [String: Bool]()
     var doesActiveAccountHavePremiumCalled = false
     var doesActiveAccountHavePremiumResult: Result<Bool, Error> = .success(true)
@@ -113,6 +114,10 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
         })
     }
 
+    func didAccountSwitchInExtension() async throws -> Bool {
+        try didAccountSwitchInExtensionResult.get()
+    }
+
     func doesActiveAccountHavePremium() async throws -> Bool {
         doesActiveAccountHavePremiumCalled = true
         return try doesActiveAccountHavePremiumResult.get()

diff --git a/...hared/Core/Vault/Models/Response/Fixtures/ProfileOrganizationResponseModel+Fixtures.swift b/...hared/Core/Vault/Models/Response/Fixtures/ProfileOrganizationResponseModel+Fixtures.swift
@@ -4,7 +4,6 @@ import Foundation
 
 extension ProfileOrganizationResponseModel {
     static func fixture(
-        accessSecretsManager: Bool = false,
         enabled: Bool = true,
         familySponsorshipAvailable: Bool = false,
         familySponsorshipFriendlyName: String? = nil,
@@ -49,7 +48,6 @@ extension ProfileOrganizationResponseModel {
         usersGetPremium: Bool = false
     ) -> ProfileOrganizationResponseModel {
         self.init(
-            accessSecretsManager: accessSecretsManager,
             enabled: enabled,
             familySponsorshipAvailable: familySponsorshipAvailable,
             familySponsorshipFriendlyName: familySponsorshipFriendlyName,

diff --git a/BitwardenShared/Core/Vault/Models/Response/ProfileOrganizationResponseModel.swift b/BitwardenShared/Core/Vault/Models/Response/ProfileOrganizationResponseModel.swift
@@ -5,9 +5,6 @@ import Foundation
 struct ProfileOrganizationResponseModel: Codable, Equatable {
     // MARK: Properties
 
-    /// Whether the user can access secrets manager.
-    let accessSecretsManager: Bool
-
     /// Whether the profile organization is enabled.
     let enabled: Bool
 

diff --git a/BitwardenShared/UI/Platform/Application/AppCoordinator.swift b/BitwardenShared/UI/Platform/Application/AppCoordinator.swift
@@ -81,6 +81,16 @@ class AppCoordinator: Coordinator, HasRootNavigator {
             await handleAuthEvent(.didTimeout(userId: userId))
         case let .setAuthCompletionRoute(route):
             authCompletionRoute = route
+        case let .switchAccounts(userId, isAutomatic):
+            await handleAuthEvent(
+                .action(
+                    .switchAccount(
+                        isAutomatic: isAutomatic,
+                        userId: userId,
+                        authCompletionRoute: nil
+                    )
+                )
+            )
         }
     }
 

diff --git a/BitwardenShared/UI/Platform/Application/AppCoordinatorTests.swift b/BitwardenShared/UI/Platform/Application/AppCoordinatorTests.swift
@@ -246,6 +246,23 @@ class AppCoordinatorTests: BitwardenTestCase { // swiftlint:disable:this type_bo
         XCTAssertEqual(module.authCoordinator.routes, [.landing])
     }
 
+    /// `handleEvent(_:)` with `.switchAccounts` has the router handle switching accounts.
+    @MainActor
+    func test_handleEvent_switchAccounts() async {
+        await subject.handleEvent(.switchAccounts(userId: "1", isAutomatic: false))
+        XCTAssertEqual(
+            router.events,
+            [.action(.switchAccount(isAutomatic: false, userId: "1", authCompletionRoute: nil))]
+        )
+        router.events.removeAll()
+
+        await subject.handleEvent(.switchAccounts(userId: "2", isAutomatic: true))
+        XCTAssertEqual(
+            router.events,
+            [.action(.switchAccount(isAutomatic: true, userId: "2", authCompletionRoute: nil))]
+        )
+    }
+
     /// `navigate(to:)` with `.onboarding` starts the auth coordinator and navigates to the proper auth route.
     @MainActor
     func test_navigateTo_auth() throws {

diff --git a/BitwardenShared/UI/Platform/Application/AppProcessor.swift b/BitwardenShared/UI/Platform/Application/AppProcessor.swift
@@ -82,6 +82,7 @@ public class AppProcessor {
         Task {
             for await _ in services.notificationCenterService.willEnterForegroundPublisher() {
                 startEventTimer()
+                await checkIfExtensionSwitchedAccounts()
                 await checkAccountsForTimeout()
                 await completeAutofillAccountSetupIfEnabled()
                 #if DEBUG
@@ -353,6 +354,22 @@ extension AppProcessor {
         }
     }
 
+    /// Checks if the active account was switched while in the extension. If this occurs, the app
+    /// needs to also switch to the updated active account.
+    ///
+    private func checkIfExtensionSwitchedAccounts() async {
+        guard appExtensionDelegate?.isInAppExtension != true else { return }
+        do {
+            guard try await services.stateService.didAccountSwitchInExtension() == true else { return }
+            let userId = try await services.stateService.getActiveAccountId()
+            await coordinator?.handleEvent(.switchAccounts(userId: userId, isAutomatic: false))
+        } catch StateServiceError.noActiveAccount {
+            await coordinator?.handleEvent(.didStart)
+        } catch {
+            services.errorReporter.log(error: error)
+        }
+    }
+
     /// If the native create account feature flag and the autofill extension are enabled, this marks
     /// any user's autofill account setup completed. This should be called on app startup.
     ///