BIT-1720: Logout if security stamp changes (#515)

This commit is contained in:
Matt Czech 2024-03-14 10:07:29 -05:00 committed by GitHub
parent 99fa1edb8a
commit fc655de381
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 174 additions and 7 deletions

View File

@ -118,7 +118,7 @@ extension Account {
let orgIdentifier: String?
/// The account's security stamp.
let stamp: String?
var stamp: String?
/// User decryption options for the account.
let userDecryptionOptions: UserDecryptionOptions?

View File

@ -50,7 +50,7 @@ extension Account.AccountProfile {
kdfType: KdfType? = .pbkdf2sha256,
name: String? = nil,
orgIdentifier: String? = nil,
stamp: String? = "STAMP",
stamp: String? = "stamp",
userDecryptionOptions: UserDecryptionOptions? = nil,
userId: String = "1"
) -> Account.AccountProfile {

View File

@ -1306,6 +1306,7 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
profile.email = response.email ?? profile.email
profile.emailVerified = response.emailVerified
profile.name = response.name
profile.stamp = response.securityStamp
state.accounts[userId]?.profile = profile
}

View File

@ -1224,4 +1224,49 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
XCTAssertEqual(publishedValues, [true, false])
}
/// `updateProfile(from:userId:)` updates the user's profile from the profile response.
func test_updateProfile() async throws {
await subject.addAccount(
.fixture(
profile: .fixture(
avatarColor: nil,
email: "user@bitwarden.com",
emailVerified: false,
hasPremiumPersonally: false,
name: "User",
stamp: "stamp",
userId: "1"
)
)
)
await subject.updateProfile(
from: .fixture(
avatarColor: "175DDC",
email: "other@bitwarden.com",
emailVerified: true,
name: "Other",
premium: true,
securityStamp: "new stamp"
),
userId: "1"
)
let updatedAccount = try await subject.getActiveAccount()
XCTAssertEqual(
updatedAccount,
.fixture(
profile: .fixture(
avatarColor: "175DDC",
email: "other@bitwarden.com",
emailVerified: true,
hasPremiumPersonally: true,
name: "Other",
stamp: "new stamp",
userId: "1"
)
)
)
}
} // swiftlint:disable:this file_length

View File

@ -11,7 +11,7 @@
"twoFactorEnabled": false,
"key": "key",
"privateKey": "private key",
"securityStamp": "security stamp",
"securityStamp": "stamp",
"forcePasswordReset": false,
"usesKeyConnector": false,
"avatarColor": null,

View File

@ -11,7 +11,7 @@
"twoFactorEnabled": false,
"key": "key",
"privateKey": "private key",
"securityStamp": "security stamp",
"securityStamp": "stamp",
"forcePasswordReset": false,
"usesKeyConnector": false,
"avatarColor": null,

View File

@ -136,7 +136,7 @@ class SyncAPIServiceTests: BitwardenTestCase {
key: "key",
organizations: [],
privateKey: "private key",
securityStamp: "security stamp"
securityStamp: "stamp"
),
sends: []
)

View File

@ -7,6 +7,11 @@ import Foundation
/// A protocol for a service that manages syncing vault data with the API.
///
protocol SyncService: AnyObject {
// MARK: Properties
/// A delegate of the `SyncService` that is notified if a user's security stamp changes.
var delegate: SyncServiceDelegate? { get set }
// MARK: Methods
/// Performs an API request to sync the user's vault data.
@ -59,6 +64,19 @@ protocol SyncService: AnyObject {
func needsSync(for userId: String) async throws -> Bool
}
// MARK: - SyncServiceDelegate
/// A protocol for a delegate of a `SyncService` which is notified to handle actions that need to
/// be taken outside of the service layer.
///
protocol SyncServiceDelegate: AnyObject {
/// The user's security stamp changed.
///
/// - Parameter userId: The user ID of the user who's security stamp changed.
///
func securityStampChanged(userId: String) async
}
// MARK: - DefaultSyncService
/// A default implementation of a `SyncService` which manages syncing vault data with the API.
@ -99,6 +117,9 @@ class DefaultSyncService: SyncService {
/// The API service used to perform sync API requests.
private let syncAPIService: SyncAPIService
/// A delegate of the `SyncService` that is notified if a user's security stamp changes.
weak var delegate: SyncServiceDelegate?
/// The time provider for this service.
private let timeProvider: TimeProvider
@ -196,12 +217,20 @@ class DefaultSyncService: SyncService {
extension DefaultSyncService {
func fetchSync(forceSync: Bool) async throws {
let userId = try await stateService.getActiveAccountId()
let account = try await stateService.getActiveAccount()
let userId = account.profile.userId
guard try await needsSync(forceSync: forceSync, userId: userId) else { return }
let response = try await syncAPIService.getSync()
if let savedStamp = account.profile.stamp,
let currentStamp = response.profile?.securityStamp,
savedStamp != currentStamp {
await delegate?.securityStampChanged(userId: userId)
return
}
if let organizations = response.profile?.organizations {
await organizationService.initializeOrganizationCrypto(
organizations: organizations.compactMap(Organization.init)

View File

@ -19,6 +19,7 @@ class SyncServiceTests: BitwardenTestCase {
var settingsService: MockSettingsService!
var stateService: MockStateService!
var subject: SyncService!
var syncServiceDelegate: MockSyncServiceDelegate!
var timeProvider: MockTimeProvider!
// MARK: Setup & Teardown
@ -36,6 +37,7 @@ class SyncServiceTests: BitwardenTestCase {
sendService = MockSendService()
settingsService = MockSettingsService()
stateService = MockStateService()
syncServiceDelegate = MockSyncServiceDelegate()
timeProvider = MockTimeProvider(
.mockTime(
Date(
@ -60,6 +62,7 @@ class SyncServiceTests: BitwardenTestCase {
syncAPIService: APIService(client: client),
timeProvider: timeProvider
)
subject.delegate = syncServiceDelegate
}
override func tearDown() {
@ -76,6 +79,7 @@ class SyncServiceTests: BitwardenTestCase {
settingsService = nil
stateService = nil
subject = nil
syncServiceDelegate = nil
timeProvider = nil
}
@ -351,12 +355,37 @@ class SyncServiceTests: BitwardenTestCase {
key: "key",
organizations: [],
privateKey: "private key",
securityStamp: "security stamp"
securityStamp: "stamp"
)
)
XCTAssertEqual(stateService.updateProfileUserId, "1")
}
/// `fetchSync()` notifies the sync service delegate if the security stamp changes and doesn't
/// replace any of the user's data.
func test_fetchSync_securityStampChanged() async throws {
client.result = .httpSuccess(testData: .syncWithProfile)
stateService.activeAccount = .fixture(profile: .fixture(stamp: "old stamp"))
try await subject.fetchSync(forceSync: false)
XCTAssertTrue(syncServiceDelegate.securityStampChangedCalled)
XCTAssertEqual(syncServiceDelegate.securityStampChangedUserId, "1")
XCTAssertNil(stateService.updateProfileResponse)
}
/// `fetchSync()` does not notify the sync service delegate if the security stamp is the same
/// and syncs the user's data.
func test_fetchSync_securityStampSame() async throws {
client.result = .httpSuccess(testData: .syncWithProfile)
stateService.activeAccount = .fixture(profile: .fixture(stamp: "stamp"))
try await subject.fetchSync(forceSync: false)
XCTAssertFalse(syncServiceDelegate.securityStampChangedCalled)
XCTAssertNotNil(stateService.updateProfileResponse)
}
/// `fetchSync()` replaces the list of the user's sends.
func test_fetchSync_sends() async throws {
client.result = .httpSuccess(testData: .syncWithSends)
@ -593,4 +622,14 @@ class SyncServiceTests: BitwardenTestCase {
try await subject.fetchUpsertSyncSend(data: notification)
XCTAssertEqual(sendService.syncSendWithServerId, "id")
}
}
class MockSyncServiceDelegate: SyncServiceDelegate {
var securityStampChangedCalled = false
var securityStampChangedUserId: String?
func securityStampChanged(userId: String) async {
securityStampChangedCalled = true
securityStampChangedUserId = userId
}
} // swiftlint:disable:this file_length

View File

@ -3,6 +3,7 @@ import Combine
@testable import BitwardenShared
class MockSyncService: SyncService {
var delegate: SyncServiceDelegate?
var didFetchSync = false
var fetchSyncForceSync: Bool?
var fetchSyncResult: Result<Void, Error> = .success(())

View File

@ -69,6 +69,8 @@ class AppCoordinator: Coordinator, HasRootNavigator {
func handleEvent(_ event: AppEvent, context: AnyObject?) async {
switch event {
case let .didLogout(userId, userInitiated):
await handleAuthEvent(.didLogout(userId: userId, userInitiated: userInitiated))
case .didStart:
await handleAuthEvent(.didStart)
case let .didTimeout(userId):

View File

@ -154,6 +154,12 @@ class AppCoordinatorTests: BitwardenTestCase {
XCTAssertEqual(module.authCoordinator.routes, [.complete])
}
/// `handleEvent(_:)` navigates the user to the auth landing view.
func test_handleEvent_didLogout() async {
await subject.handleEvent(.didLogout(userId: "1", userInitiated: false))
XCTAssertEqual(module.authCoordinator.routes, [.landing])
}
/// `navigate(to:)` with `.onboarding` starts the auth coordinator and navigates to the proper auth route.
func test_navigateTo_auth() throws {
subject.navigate(to: .auth(.landing))

View File

@ -34,6 +34,7 @@ public class AppProcessor {
self.services = services
self.services.notificationService.setDelegate(self)
self.services.syncService.delegate = self
UI.initialLanguageCode = services.appSettingsStore.appLocale ?? Locale.current.languageCode
UI.applyDefaultAppearances()
@ -137,6 +138,8 @@ public class AppProcessor {
}
}
// MARK: - NotificationServiceDelegate
extension AppProcessor: NotificationServiceDelegate {
/// Users are logged out, route to landing page.
///
@ -185,3 +188,14 @@ extension AppProcessor: NotificationServiceDelegate {
coordinator?.navigate(to: .loginRequest(loginRequest))
}
}
// MARK: - SyncServiceDelegate
extension AppProcessor: SyncServiceDelegate {
func securityStampChanged(userId: String) async {
// Log the user out if their security stamp changes.
coordinator?.hideLoadingOverlay()
try? await services.authRepository.logout(userId: userId)
await coordinator?.handleEvent(.didLogout(userId: userId, userInitiated: false))
}
}

View File

@ -7,6 +7,7 @@ class AppProcessorTests: BitwardenTestCase {
// MARK: Properties
var appModule: MockAppModule!
var authRepository: MockAuthRepository!
var appSettingStore: MockAppSettingsStore!
var coordinator: MockCoordinator<AppRoute, AppEvent>!
var errorReporter: MockErrorReporter!
@ -27,6 +28,7 @@ class AppProcessorTests: BitwardenTestCase {
router = MockRouter(routeForEvent: { _ in .landing })
appModule = MockAppModule()
authRepository = MockAuthRepository()
coordinator = MockCoordinator()
appModule.authRouter = router
appModule.appCoordinator = coordinator
@ -44,6 +46,7 @@ class AppProcessorTests: BitwardenTestCase {
appModule: appModule,
services: ServiceContainer.withMocks(
appSettingsStore: appSettingStore,
authRepository: authRepository,
errorReporter: errorReporter,
migrationService: migrationService,
notificationService: notificationService,
@ -59,6 +62,7 @@ class AppProcessorTests: BitwardenTestCase {
super.tearDown()
appModule = nil
authRepository = nil
appSettingStore = nil
coordinator = nil
errorReporter = nil
@ -107,6 +111,12 @@ class AppProcessorTests: BitwardenTestCase {
XCTAssertEqual(errorReporter.errors.last as? BitwardenTestError, .example)
}
/// `init()` sets the `AppProcessor` as the delegate of any necessary services.
func test_init_setDelegates() {
XCTAssertIdentical(notificationService.delegate, subject)
XCTAssertIdentical(syncService.delegate, subject)
}
/// `messageReceived(_:notificationDismissed:notificationTapped)` passes the data to the notification service.
func test_messageReceived() async {
let message: [AnyHashable: Any] = ["knock knock": "who's there?"]
@ -122,6 +132,18 @@ class AppProcessorTests: BitwardenTestCase {
XCTAssertEqual(coordinator.routes.last, .auth(.landing))
}
/// `securityStampChanged(userId:)` logs the user out and notifies the coordinator.
func test_securityStampChanged() async {
coordinator.isLoadingOverlayShowing = true
await subject.securityStampChanged(userId: "1")
XCTAssertTrue(authRepository.logoutCalled)
XCTAssertEqual(authRepository.logoutUserId, "1")
XCTAssertFalse(coordinator.isLoadingOverlayShowing)
XCTAssertEqual(coordinator.events, [.didLogout(userId: "1", userInitiated: false)])
}
/// Upon a session timeout on app foreground, send the user to the `.didTimeout` route.
func test_shouldSessionTimeout_navigateTo_didTimeout() throws {
let rootNavigator = MockRootNavigator()

View File

@ -23,6 +23,14 @@ public enum AppRoute: Equatable {
}
public enum AppEvent: Equatable {
/// When the user logs out from an account.
///
/// - Parameters:
/// - userId: The userId of the account that was logged out.
/// - userInitiated: Did a user action trigger the account switch?
///
case didLogout(userId: String, userInitiated: Bool)
/// When the app has started.
case didStart