From 99649bdd539a401e00d14c253f125690a193d071 Mon Sep 17 00:00:00 2001 From: Matt Czech Date: Thu, 21 Nov 2024 10:23:57 -0600 Subject: [PATCH] PM-11974: Fix login with device notification for inactive account not switching to that account --- .../Models/Domain/PushNotificationData.swift | 4 +- .../Services/NotificationService.swift | 34 +++++++--------- .../Services/NotificationServiceTests.swift | 39 +++++++++---------- .../Platform/Application/AppProcessor.swift | 36 ++++++++--------- .../Application/AppProcessorTests.swift | 33 ++++++++++++++++ 5 files changed, 84 insertions(+), 62 deletions(-) diff --git a/BitwardenShared/Core/Platform/Models/Domain/PushNotificationData.swift b/BitwardenShared/Core/Platform/Models/Domain/PushNotificationData.swift index a17548ced..2b4ac853d 100644 --- a/BitwardenShared/Core/Platform/Models/Domain/PushNotificationData.swift +++ b/BitwardenShared/Core/Platform/Models/Domain/PushNotificationData.swift @@ -103,6 +103,6 @@ struct LoginRequestPushNotification: Codable, Equatable { /// How long until the request times out. let timeoutInMinutes: Int - /// The email of the account sending the login request. - let userEmail: String + /// The user id that sent the login request. + let userId: String } diff --git a/BitwardenShared/Core/Platform/Services/NotificationService.swift b/BitwardenShared/Core/Platform/Services/NotificationService.swift index bc533e7fa..84cb5e167 100644 --- a/BitwardenShared/Core/Platform/Services/NotificationService.swift +++ b/BitwardenShared/Core/Platform/Services/NotificationService.swift @@ -51,6 +51,7 @@ protocol NotificationService { /// The delegate to handle login request actions originating from notifications. /// +@MainActor protocol NotificationServiceDelegate: AnyObject { /// Users are logged out, route to landing page. /// @@ -66,10 +67,9 @@ protocol NotificationServiceDelegate: AnyObject { /// /// - Parameters: /// - account: The account associated with the login request. - /// - loginRequest: The login request to show. /// - showAlert: Whether to show the alert or simply switch the account. /// - func switchAccounts(to account: Account, for loginRequest: LoginRequest, showAlert: Bool) + func switchAccountsForLoginRequest(to account: Account, showAlert: Bool) async } // MARK: - DefaultNotificationService @@ -279,14 +279,13 @@ class DefaultNotificationService: NotificationService { await stateService.setLoginRequest(data) // Get the email of the account that the login request is coming from. - let loginSourceAccount = try await stateService.getAccounts() - .first(where: { $0.profile.userId == data.userId }) - let loginSourceEmail = loginSourceAccount?.profile.email ?? "" + let loginSourceAccount = try await stateService.getAccount(userId: data.userId) + let loginSourceEmail = loginSourceAccount.profile.email // Assemble the data to add to the in-app banner notification. let loginRequestData = try? JSONEncoder().encode(LoginRequestPushNotification( timeoutInMinutes: Constants.loginRequestTimeoutMinutes, - userEmail: loginSourceEmail + userId: loginSourceAccount.profile.userId )) // Create an in-app banner notification to tell the user about the login request. @@ -308,14 +307,14 @@ class DefaultNotificationService: NotificationService { let request = UNNotificationRequest(identifier: data.id, content: content, trigger: nil) try await UNUserNotificationCenter.current().add(request) - // If the request is for the existing account, show the login request view automatically. - guard let loginRequest = try await authService.getPendingLoginRequest(withId: data.id).first - else { return } if data.userId == userId { - delegate?.showLoginRequest(loginRequest) - } else if let loginSourceAccount { + // If the request is for the existing account, show the login request view automatically. + guard let loginRequest = try await authService.getPendingLoginRequest(withId: data.id).first + else { return } + await delegate?.showLoginRequest(loginRequest) + } else { // Otherwise, show an alert asking the user if they want to switch accounts. - delegate?.switchAccounts(to: loginSourceAccount, for: loginRequest, showAlert: true) + await delegate?.switchAccountsForLoginRequest(to: loginSourceAccount, showAlert: true) } } @@ -361,20 +360,15 @@ class DefaultNotificationService: NotificationService { private func handleNotificationTapped(_ loginRequestData: LoginRequestPushNotification) async { do { // Get the user id of the source of the login request. - guard let loginSourceAccount = try await stateService.getAccounts() - .first(where: { $0.profile.email == loginRequestData.userEmail }) - else { return } + let loginSourceAccount = try await stateService.getAccount(userId: loginRequestData.userId) // Get the active account for comparison. let activeAccount = try await stateService.getActiveAccount() // If the notification banner was tapped but it's for a different account, switch // to that account automatically. - if activeAccount.profile.userId != loginSourceAccount.profile.userId, - let loginRequestData = await stateService.getLoginRequest(), - let loginRequest = try await authService.getPendingLoginRequest(withId: loginRequestData.id).first { - try await stateService.setActiveAccount(userId: loginSourceAccount.profile.userId) - delegate?.switchAccounts(to: loginSourceAccount, for: loginRequest, showAlert: false) + if activeAccount.profile.userId != loginSourceAccount.profile.userId { + await delegate?.switchAccountsForLoginRequest(to: loginSourceAccount, showAlert: false) } } catch { errorReporter.log(error: error) diff --git a/BitwardenShared/Core/Platform/Services/NotificationServiceTests.swift b/BitwardenShared/Core/Platform/Services/NotificationServiceTests.swift index 1a010cb00..48f828777 100644 --- a/BitwardenShared/Core/Platform/Services/NotificationServiceTests.swift +++ b/BitwardenShared/Core/Platform/Services/NotificationServiceTests.swift @@ -371,6 +371,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty /// `messageReceived(_:notificationDismissed:notificationTapped:)` tells /// the delegate to show the switch account alert if it's a login request for a non-active account. + @MainActor func test_messageReceived_loginRequest_differentAccount() async throws { // Set up the mock data. stateService.setIsAuthenticated() @@ -379,7 +380,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty authService.getPendingLoginRequestResult = .success([.fixture()]) let loginRequestNotification = LoginRequestNotification(id: "requestId", userId: "differentUser") let notificationData = try JSONEncoder().encode(loginRequestNotification) - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "data": [ "type": NotificationType.authRequest.rawValue, "payload": String(data: notificationData, encoding: .utf8) ?? "", @@ -392,12 +393,12 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty // Confirm the results. XCTAssertEqual(stateService.loginRequest, loginRequestNotification) XCTAssertEqual(delegate.switchAccountsAccount, .fixture(profile: .fixture(userId: "differentUser"))) - XCTAssertEqual(delegate.switchAccountsLoginRequest, .fixture()) XCTAssertEqual(delegate.switchAccountsShowAlert, true) } /// `messageReceived(_:notificationDismissed:notificationTapped:)` tells /// the delegate to show the login request if it's a login request for the active account. + @MainActor func test_messageReceived_loginRequest_sameAccount() async throws { // Set up the mock data. stateService.setIsAuthenticated() @@ -406,7 +407,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty authService.getPendingLoginRequestResult = .success([.fixture()]) let loginRequestNotification = LoginRequestNotification(id: "requestId", userId: "1") let notificationData = try JSONEncoder().encode(loginRequestNotification) - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "data": [ "type": NotificationType.authRequest.rawValue, "payload": String(data: notificationData, encoding: .utf8) ?? "", @@ -423,6 +424,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty /// `messageReceived(_:notificationDismissed:notificationTapped:)` handles logout requests and will not route /// to the landing screen if the logged-out account was not the currently active account. + @MainActor func test_messageReceived_logout_nonActiveUser() async throws { // Set up the mock data. stateService.setIsAuthenticated() @@ -430,7 +432,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty let nonActiveAccount: Account = .fixture(profile: .fixture(userId: "b245a33f")) stateService.accounts = [activeAccount, nonActiveAccount] - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "data": [ "type": NotificationType.logOut.rawValue, "payload": "{\"UserId\":\"\(nonActiveAccount.profile.userId)\"}", @@ -445,6 +447,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty /// `messageReceived(_:notificationDismissed:notificationTapped:)` handles logout requests and will route /// to the landing screen if the logged-out account was the currently active account. + @MainActor func test_messageReceived_logout_activeUser() async throws { // Set up the mock data. stateService.setIsAuthenticated() @@ -452,7 +455,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty let nonActiveAccount: Account = .fixture(profile: .fixture(userId: "b245a33f")) stateService.accounts = [activeAccount, nonActiveAccount] - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "data": [ "type": NotificationType.logOut.rawValue, "payload": "{\"UserId\":\"\(activeAccount.profile.userId)\"}", @@ -466,15 +469,16 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty } /// `messageReceived(_:notificationDismissed:notificationTapped:)` handles notifications being dismissed. + @MainActor func test_messageReceived_notificationDismissed() async throws { // Set up the mock data. stateService.loginRequest = LoginRequestNotification(id: "1", userId: "2") let loginRequest = LoginRequestPushNotification( timeoutInMinutes: 15, - userEmail: "test@email.com" + userId: "2" ) let testData = try JSONEncoder().encode(loginRequest) - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "notificationData": String(data: testData, encoding: .utf8) ?? "", ] @@ -486,6 +490,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty } /// `messageReceived(_:notificationDismissed:notificationTapped:)` handles notifications being tapped. + @MainActor func test_messageReceived_notificationTapped() async throws { // Set up the mock data. stateService.accounts = [.fixture()] @@ -494,10 +499,10 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty authService.getPendingLoginRequestResult = .success([.fixture(id: "requestId")]) let loginRequest = LoginRequestPushNotification( timeoutInMinutes: 15, - userEmail: Account.fixture().profile.email + userId: Account.fixture().profile.userId ) let testData = try JSONEncoder().encode(loginRequest) - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "notificationData": String(data: testData, encoding: .utf8) ?? "", ] @@ -506,23 +511,19 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty // Confirm the results. XCTAssertEqual(delegate.switchAccountsAccount, .fixture()) - XCTAssertEqual(delegate.switchAccountsLoginRequest, .fixture(id: "requestId")) XCTAssertEqual(delegate.switchAccountsShowAlert, false) } /// `messageReceived(_:notificationDismissed:notificationTapped:)` handles errors. + @MainActor func test_messageReceived_notificationTapped_error() async throws { // Set up the mock data. - stateService.accounts = [.fixture()] - stateService.activeAccount = .fixtureAccountLogin() - stateService.loginRequest = LoginRequestNotification(id: "requestId", userId: "1") - authService.getPendingLoginRequestResult = .failure(BitwardenTestError.example) let loginRequest = LoginRequestPushNotification( timeoutInMinutes: 15, - userEmail: Account.fixture().profile.email + userId: Account.fixture().profile.userId ) let testData = try JSONEncoder().encode(loginRequest) - let message: [AnyHashable: Any] = [ + nonisolated(unsafe) let message: [AnyHashable: Any] = [ "notificationData": String(data: testData, encoding: .utf8) ?? "", ] @@ -530,7 +531,7 @@ class NotificationServiceTests: BitwardenTestCase { // swiftlint:disable:this ty await subject.messageReceived(message, notificationDismissed: nil, notificationTapped: true) // Confirm the results. - XCTAssertEqual(errorReporter.errors.last as? BitwardenTestError, .example) + XCTAssertEqual(errorReporter.errors.last as? StateServiceError, .noAccounts) } } @@ -542,7 +543,6 @@ class MockNotificationServiceDelegate: NotificationServiceDelegate { var showLoginRequestRequest: LoginRequest? var switchAccountsAccount: Account? - var switchAccountsLoginRequest: LoginRequest? var switchAccountsShowAlert: Bool? func routeToLanding() async { @@ -553,9 +553,8 @@ class MockNotificationServiceDelegate: NotificationServiceDelegate { showLoginRequestRequest = loginRequest } - func switchAccounts(to account: Account, for loginRequest: LoginRequest, showAlert: Bool) { + func switchAccountsForLoginRequest(to account: Account, showAlert: Bool) { switchAccountsAccount = account - switchAccountsLoginRequest = loginRequest switchAccountsShowAlert = showAlert } } // swiftlint:disable:this file_length diff --git a/BitwardenShared/UI/Platform/Application/AppProcessor.swift b/BitwardenShared/UI/Platform/Application/AppProcessor.swift index 1729d9ef9..aecaed7ed 100644 --- a/BitwardenShared/UI/Platform/Application/AppProcessor.swift +++ b/BitwardenShared/UI/Platform/Application/AppProcessor.swift @@ -520,33 +520,29 @@ extension AppProcessor: NotificationServiceDelegate { /// /// - Parameters: /// - account: The account associated with the login request. - /// - loginRequest: The login request to show. /// - showAlert: Whether to show the alert or simply switch the account. /// - func switchAccounts(to account: Account, for loginRequest: LoginRequest, showAlert: Bool) { - DispatchQueue.main.async { - if showAlert { - self.coordinator?.showAlert(.confirmation( - title: Localizations.logInRequested, - message: Localizations.loginAttemptFromXDoYouWantToSwitchToThisAccount(account.profile.email) - ) { - self.switchAccounts(to: account.profile.userId, for: loginRequest) - }) - } else { - self.switchAccounts(to: account.profile.userId, for: loginRequest) - } + func switchAccountsForLoginRequest(to account: Account, showAlert: Bool) async { + if showAlert { + coordinator?.showAlert(.confirmation( + title: Localizations.logInRequested, + message: Localizations.loginAttemptFromXDoYouWantToSwitchToThisAccount(account.profile.email) + ) { + await self.switchAccountsForLoginRequest(to: account.profile.userId) + }) + } else { + await switchAccountsForLoginRequest(to: account.profile.userId) } } - /// Switch to the specified account and show the login request. + /// Switch to the specified account so they can see the login request. /// - /// - Parameters: - /// - userId: The userId of the account to switch to. - /// - loginRequest: The login request to show. + /// - Parameter userId: The user ID of the account to switch to. /// - private func switchAccounts(to userId: String, for loginRequest: LoginRequest) { - (coordinator as? VaultCoordinatorDelegate)?.didTapAccount(userId: userId) - coordinator?.navigate(to: .loginRequest(loginRequest)) + private func switchAccountsForLoginRequest(to userId: String) async { + // Switch to the account, the login request will be shown when their vault loads (either + // immediately or after vault unlock). + await coordinator?.handleEvent(.switchAccounts(userId: userId, isAutomatic: false)) } } diff --git a/BitwardenShared/UI/Platform/Application/AppProcessorTests.swift b/BitwardenShared/UI/Platform/Application/AppProcessorTests.swift index 60df2c1f8..59bb32c81 100644 --- a/BitwardenShared/UI/Platform/Application/AppProcessorTests.swift +++ b/BitwardenShared/UI/Platform/Application/AppProcessorTests.swift @@ -1055,6 +1055,39 @@ class AppProcessorTests: BitwardenTestCase { // swiftlint:disable:this type_body XCTAssertEqual(stateService.accountSetupAutofill, ["1": .complete]) } + /// `switchAccountsForLoginRequest(to:showAlert:)` has the coordinator switch to the specified + /// account without showing a confirmation alert. + @MainActor + func test_switchAccountsForLoginRequest() async { + await subject.switchAccountsForLoginRequest(to: .fixture(), showAlert: false) + + XCTAssertEqual(coordinator.events, [.switchAccounts(userId: "1", isAutomatic: false)]) + } + + /// `switchAccountsForLoginRequest(to:showAlert:)` shows an alert to confirm the user wants to + /// switch to the specified account and then has the coordinator switch accounts. + @MainActor + func test_switchAccountsForLoginRequest_showAlert() async throws { + let account = Account.fixture() + await subject.switchAccountsForLoginRequest(to: account, showAlert: true) + + let alert = try XCTUnwrap(coordinator.alertShown.last) + XCTAssertEqual( + alert, + .confirmation( + title: Localizations.logInRequested, + message: Localizations.loginAttemptFromXDoYouWantToSwitchToThisAccount(account.profile.email), + confirmationHandler: {} + ) + ) + + try await alert.tapAction(title: Localizations.cancel) + XCTAssertTrue(coordinator.events.isEmpty) + + try await alert.tapAction(title: Localizations.yes) + XCTAssertEqual(coordinator.events, [.switchAccounts(userId: account.profile.userId, isAutomatic: false)]) + } + /// `unlockVaultWithNeverlockKey()` unlocks it calling the auth repository. func test_unlockVaultWithNeverlockKey() async throws { try await subject.unlockVaultWithNeverlockKey()