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-12051: Fix sync error after logout and switch accounts #1100

Merged
merged 1 commit into from
Nov 1, 2024
Merged

PM-12051: Fix sync error after logout and switch accounts #1100

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
28 changes: 28 additions & 0 deletions BitwardenShared/UI/Auth/AuthRouterTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,33 @@ final class AuthRouterTests: BitwardenTestCase { // swiftlint:disable:this type_
)
)
XCTAssertEqual(route, .complete)
XCTAssertEqual(authRepository.setActiveAccountId, active.profile.userId)
}

/// `handleAndRoute(_ :)` redirects `.switchAccount()` to `.vaultUnlock` when switching to a
/// locked inactive account.
func test_handleAndRoute_switchAccount_toInactive() async {
let active = Account.fixture()
let inactive = Account.fixture(profile: .fixture(userId: "2"))
authRepository.activeAccount = active
authRepository.altAccounts = [inactive]
authRepository.isLockedResult = .success(true)
stateService.isAuthenticated["2"] = true
let route = await subject.handleAndRoute(
.action(
.switchAccount(isAutomatic: true, userId: inactive.profile.userId)
)
)
XCTAssertEqual(
route,
.vaultUnlock(
inactive,
animated: false,
attemptAutomaticBiometricUnlock: true,
didSwitchAccountAutomatically: true
)
)
XCTAssertEqual(authRepository.setActiveAccountId, inactive.profile.userId)
}

/// `handleAndRoute(_ :)` redirects `.switchAccount()` to `.landingSoftLoggedOut` when that
Expand All @@ -1099,5 +1126,6 @@ final class AuthRouterTests: BitwardenTestCase { // swiftlint:disable:this type_
)
)
XCTAssertEqual(route, .landingSoftLoggedOut(email: account.profile.email))
XCTAssertEqual(authRepository.setActiveAccountId, account.profile.userId)
}
}
11 changes: 0 additions & 11 deletions BitwardenShared/UI/Auth/Extensions/AuthRouter+Redirects.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,17 +306,6 @@ extension AuthRouter {
/// - Returns: A suggested route for the active account with state pre-configured.
///
func switchAccountRedirect(isAutomatic: Bool, userId: String) async -> AuthRoute {
if let account = try? await services.authRepository.getAccount(),
userId == account.profile.userId {
return await handleAndRoute(
.accountBecameActive(
account,
animated: false,
attemptAutomaticBiometricUnlock: true,
didSwitchAccountAutomatically: false
)
)
}
Comment on lines -309 to -319
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 logic here is the same as what's below, without the call to authRepository.setActiveAccount().

As far as I can tell, this was put in originally as a shortcut if you attempted to switch accounts to the already active account, then it would skip the setActiveAccount() call. However, this causes issues because we also have logic in StateService that makes the next account active when logging out. So what would happen in this scenario, is the first account would logout, StateService would make the next account active and eventually this switchAccountRedirect would be called. Since StateService has already updated the active account, this would skip the authRepository.setActiveAccount call, which does a few other things like reloads the URLs in the environment service. So I think it's better if we always call setActiveAccount() here. We have checks in place in the profile switcher if you try to switch to the already active account, so I couldn't find a scenario where this would be called and you wouldn't want to call setActiveAccount().

ios/BitwardenShared/Core/Platform/Services/StateService.swift

Lines 1531 to 1533 in aa70a30

if state.activeUserId == knownUserId, userInitiated {
// Find the next account to make the active account.
state.activeUserId = state.accounts.first?.key

do {
let activeAccount = try await services.authRepository.setActiveAccount(userId: userId)
// Setup the unlock route for the active account.
Expand Down
Loading