Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PM-11974: Fix login with device notification for inactive account not switching to that account #1157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+84 −62
Open

PM-11974: Fix login with device notification for inactive account not switching to that account #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions BitwardenShared/Core/Platform/Models/Domain/PushNotificationData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
34 changes: 14 additions & 20 deletions BitwardenShared/Core/Platform/Services/NotificationService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -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
Expand Down Expand Up @@ -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
Comment on lines -289 to +288
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This previously stored the email of the account that the request was for. Which would be problematic if there were multiple accounts with the same email. So I switched to the use the user ID.

))

// Create an in-app banner notification to tell the user about the login request.
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API request was being made if the login request was for an active or inactive user. And it fails for an inactive user since it has the wrong access token. This instead needs to happen after switching accounts if the request is for an inactive user.

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)
}
}

Expand Down Expand Up @@ -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)
Expand Down
39 changes: 19 additions & 20 deletions BitwardenShared/Core/Platform/Services/NotificationServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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) ?? "",
Expand All @@ -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()
Expand All @@ -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) ?? "",
Expand All @@ -423,14 +424,15 @@ 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()
let activeAccount: Account = .fixture()
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)\"}",
Expand All @@ -445,14 +447,15 @@ 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()
let activeAccount: Account = .fixture()
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)\"}",
Expand All @@ -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: "[email protected]"
userId: "2"
)
let testData = try JSONEncoder().encode(loginRequest)
let message: [AnyHashable: Any] = [
nonisolated(unsafe) let message: [AnyHashable: Any] = [
"notificationData": String(data: testData, encoding: .utf8) ?? "",
]

Expand All @@ -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()]
Expand All @@ -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) ?? "",
]

Expand All @@ -506,31 +511,27 @@ 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) ?? "",
]

// Test.
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)
}
}

Expand All @@ -542,7 +543,6 @@ class MockNotificationServiceDelegate: NotificationServiceDelegate {
var showLoginRequestRequest: LoginRequest?

var switchAccountsAccount: Account?
var switchAccountsLoginRequest: LoginRequest?
var switchAccountsShowAlert: Bool?

func routeToLanding() async {
Expand All @@ -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
36 changes: 16 additions & 20 deletions BitwardenShared/UI/Platform/Application/AppProcessor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
33 changes: 33 additions & 0 deletions BitwardenShared/UI/Platform/Application/AppProcessorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading