From eb275cb716166d2b622b1742e0a0cd9f9153ff15 Mon Sep 17 00:00:00 2001 From: Matt Czech Date: Tue, 14 May 2024 09:12:29 -0500 Subject: [PATCH] BIT-2316: Fix vault timeout migration (#635) --- .../Core/Platform/Services/StateService.swift | 2 +- .../Services/Stores/AppSettingsStore.swift | 18 ++++----- .../Stores/AppSettingsStoreTests.swift | 2 +- .../TestHelpers/MockAppSettingsStore.swift | 4 +- .../Core/Vault/Services/PolicyService.swift | 2 +- .../Vault/Services/PolicyServiceTests.swift | 4 +- .../Vault/Services/SyncServiceTests.swift | 2 +- .../Views/CountdownDatePicker.swift | 2 +- .../AccountSecurityAction.swift | 6 +-- .../AccountSecurityProcessor.swift | 4 +- .../AccountSecurityProcessorTests.swift | 34 ++++++++-------- .../AccountSecurityState.swift | 40 +++++++++---------- .../AccountSecurity/AccountSecurityView.swift | 4 +- .../AccountSecurityViewTests.swift | 10 ++--- 14 files changed, 67 insertions(+), 67 deletions(-) diff --git a/BitwardenShared/Core/Platform/Services/StateService.swift b/BitwardenShared/Core/Platform/Services/StateService.swift index 1d66a8027b..44fdce4185 100644 --- a/BitwardenShared/Core/Platform/Services/StateService.swift +++ b/BitwardenShared/Core/Platform/Services/StateService.swift @@ -1346,7 +1346,7 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le func setVaultTimeout(value: SessionTimeoutValue, userId: String?) async throws { let userId = try userId ?? getActiveAccountUserId() - appSettingsStore.setVaultTimeout(key: value.rawValue, userId: userId) + appSettingsStore.setVaultTimeout(minutes: value.rawValue, userId: userId) } func updateProfile(from response: ProfileResponseModel, userId: String) async { diff --git a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift index 4df435cc25..b1bc66f722 100644 --- a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift +++ b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift @@ -338,13 +338,13 @@ protocol AppSettingsStore: AnyObject { /// func setUnsuccessfulUnlockAttempts(_ attempts: Int, userId: String) - /// Sets the user's session timeout date. + /// Sets the user's session timeout, in minutes. /// /// - Parameters: - /// - key: The session timeout date. - /// - userId: The user ID associated with the session timeout date. + /// - key: The session timeout, in minutes. + /// - userId: The user ID associated with the session timeout. /// - func setVaultTimeout(key: Int, userId: String) + func setVaultTimeout(minutes: Int, userId: String) /// Sets the username generation options for a user ID. /// @@ -388,10 +388,10 @@ protocol AppSettingsStore: AnyObject { /// func unsuccessfulUnlockAttempts(userId: String) -> Int - /// Returns the session timeout date. + /// Returns the session timeout in minutes. /// - /// - Parameter userId: The user ID associated with the session timeout date. - /// - Returns: The user's session timeout date. + /// - Parameter userId: The user ID associated with the session timeout. + /// - Returns: The user's session timeout in minutes. /// func vaultTimeout(userId: String) -> Int? @@ -864,8 +864,8 @@ extension DefaultAppSettingsStore: AppSettingsStore { store(options, for: .usernameGenerationOptions(userId: userId)) } - func setVaultTimeout(key: Int, userId: String) { - store(key, for: .vaultTimeout(userId: userId)) + func setVaultTimeout(minutes: Int, userId: String) { + store(minutes, for: .vaultTimeout(userId: userId)) } func timeoutAction(userId: String) -> Int? { diff --git a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift index 0992647a52..fc80ef65e5 100644 --- a/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift +++ b/BitwardenShared/Core/Platform/Services/Stores/AppSettingsStoreTests.swift @@ -757,7 +757,7 @@ class AppSettingsStoreTests: BitwardenTestCase { // swiftlint:disable:this type_ /// `.vaultTimeout(userId:)` returns the correct vault timeout value. func test_vaultTimeout() throws { - subject.setVaultTimeout(key: 60, userId: "1") + subject.setVaultTimeout(minutes: 60, userId: "1") XCTAssertEqual(subject.vaultTimeout(userId: "1"), 60) XCTAssertEqual(userDefaults.double(forKey: "bwPreferencesStorage:vaultTimeout_1"), 60) diff --git a/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift b/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift index 9b944f89d7..b506b5c1f1 100644 --- a/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift +++ b/BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift @@ -208,8 +208,8 @@ class MockAppSettingsStore: AppSettingsStore { usernameGenerationOptions[userId] = options } - func setVaultTimeout(key: Int, userId: String) { - vaultTimeout[userId] = key + func setVaultTimeout(minutes: Int, userId: String) { + vaultTimeout[userId] = minutes } func shouldTrustDevice(userId: String) -> Bool? { diff --git a/BitwardenShared/Core/Vault/Services/PolicyService.swift b/BitwardenShared/Core/Vault/Services/PolicyService.swift index f186105458..eecad81a7b 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyService.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyService.swift @@ -241,7 +241,7 @@ extension DefaultPolicyService { for policy in policies { guard let policyTimeoutValue = policy[.minutes]?.intValue else { continue } - timeoutValue = policyTimeoutValue * 60 + timeoutValue = policyTimeoutValue // If the policy's timeout action is not lock or logOut, there is no policy timeout action. // In that case, we would present both timeout action options to the user. diff --git a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift index ac9c48116b..f9b582f3ec 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift @@ -321,7 +321,7 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod let policyValues = try await subject.fetchTimeoutPolicyValues() - XCTAssertEqual(policyValues?.value, 60 * 60) + XCTAssertEqual(policyValues?.value, 60) XCTAssertEqual(policyValues?.action, .lock) } @@ -334,7 +334,7 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod let policyValues = try await subject.fetchTimeoutPolicyValues() - XCTAssertEqual(policyValues?.value, 60 * 60) + XCTAssertEqual(policyValues?.value, 60) XCTAssertNil(policyValues?.action) } diff --git a/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift b/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift index a945672b74..dd5f0a8863 100644 --- a/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift @@ -144,7 +144,7 @@ class SyncServiceTests: BitwardenTestCase { stateService.activeAccount = .fixture() stateService.vaultTimeout["1"] = .never - policyService.fetchTimeoutPolicyValuesResult = .success((.lock, 15 * 60)) + policyService.fetchTimeoutPolicyValuesResult = .success((.lock, 15)) try await subject.fetchSync(forceSync: false) diff --git a/BitwardenShared/UI/Platform/Application/Views/CountdownDatePicker.swift b/BitwardenShared/UI/Platform/Application/Views/CountdownDatePicker.swift index 3cdfff2cab..d238e14f89 100644 --- a/BitwardenShared/UI/Platform/Application/Views/CountdownDatePicker.swift +++ b/BitwardenShared/UI/Platform/Application/Views/CountdownDatePicker.swift @@ -37,7 +37,7 @@ struct CountdownDatePicker: UIViewRepresentable { // MARK: Properties - /// The count down durration. + /// The count down duration, in seconds. @Binding var duration: Int // MARK: Methods diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift index 81f81c30a0..be1af81b46 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift @@ -11,6 +11,9 @@ enum AccountSecurityAction: Equatable { /// Clears the two step login URL after the web app has been opened. case clearTwoStepLoginUrl + /// Sets the custom session timeout value in seconds. + case customTimeoutValueSecondsChanged(Int) + /// The delete account button was pressed. case deleteAccountPressed @@ -26,9 +29,6 @@ enum AccountSecurityAction: Equatable { /// The session timeout value has changed. case sessionTimeoutValueChanged(SessionTimeoutValue) - /// Sets the custom session timeout value. - case customTimeoutValueChanged(Int) - /// Unlock with pin code was toggled. case toggleUnlockWithPINCode(Bool) diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift index c871c6b94d..c9f28b78f2 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift @@ -77,8 +77,8 @@ final class AccountSecurityProcessor: StateProcessor< state.fingerprintPhraseUrl = nil case .clearTwoStepLoginUrl: state.twoStepLoginUrl = nil - case let .customTimeoutValueChanged(newValue): - setVaultTimeout(value: .custom(newValue)) + case let .customTimeoutValueSecondsChanged(seconds): + setVaultTimeout(value: .custom(seconds / 60)) case .deleteAccountPressed: coordinator.navigate(to: .deleteAccount) case .logout: diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessorTests.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessorTests.swift index 3a1bbb4a72..2c94a284d0 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessorTests.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessorTests.swift @@ -79,13 +79,13 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th /// `perform(_:)` with `.appeared` sets the policy related state properties when the policy is enabled. func test_perform_appeared_timeoutPolicyEnabled() async throws { - policyService.fetchTimeoutPolicyValuesResult = .success((.logout, 3600)) + policyService.fetchTimeoutPolicyValuesResult = .success((.logout, 60)) await subject.perform(.appeared) XCTAssertEqual(subject.state.isTimeoutPolicyEnabled, true) XCTAssertEqual(subject.state.policyTimeoutAction, .logout) - XCTAssertEqual(subject.state.policyTimeoutValue, 3600) + XCTAssertEqual(subject.state.policyTimeoutValue, 60) XCTAssertEqual(subject.state.availableTimeoutActions, [.logout]) XCTAssertEqual(subject.state.policyTimeoutHours, 1) XCTAssertEqual(subject.state.policyTimeoutMinutes, 0) @@ -106,12 +106,12 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th /// `perform(_:)` with `.appeared` sets the policy related state properties when the policy is enabled, /// but the policy doesn't return an action. func test_perform_appeared_timeoutPolicyEnabled_noPolicyAction() async throws { - policyService.fetchTimeoutPolicyValuesResult = .success((nil, 3660)) + policyService.fetchTimeoutPolicyValuesResult = .success((nil, 61)) await subject.perform(.appeared) XCTAssertEqual(subject.state.isTimeoutPolicyEnabled, true) - XCTAssertEqual(subject.state.policyTimeoutValue, 3660) + XCTAssertEqual(subject.state.policyTimeoutValue, 61) XCTAssertEqual(subject.state.availableTimeoutActions, [.lock, .logout]) XCTAssertEqual(subject.state.policyTimeoutHours, 1) XCTAssertEqual(subject.state.policyTimeoutMinutes, 1) @@ -131,7 +131,7 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th /// `perform(_:)` with `.appeared` sets the policy related state properties when the policy is enabled. func test_perform_appeared_timeoutPolicyEnabled_oddTime() async throws { - policyService.fetchTimeoutPolicyValuesResult = .success((.logout, 3660)) + policyService.fetchTimeoutPolicyValuesResult = .success((.logout, 61)) await subject.perform(.appeared) @@ -234,6 +234,17 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th XCTAssertNil(subject.state.twoStepLoginUrl) } + /// `receive(_:)` with `customTimeoutValueSecondsChanged(_:)` updates the custom session timeout value in the state. + func test_receive_customTimeoutValueSecondsChanged() { + XCTAssertEqual(subject.state.customTimeoutValueSeconds, 60) + + let account = Account.fixture() + authRepository.activeAccount = account + + subject.receive(.customTimeoutValueSecondsChanged(120)) + waitFor(subject.state.customTimeoutValueSeconds == 120) + } + /// `receive(_:)` with `.deleteAccountPressed` shows the `DeleteAccountView`. func test_receive_deleteAccountPressed() throws { subject.receive(.deleteAccountPressed) @@ -406,7 +417,7 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th let account = Account.fixture() authRepository.activeAccount = account subject.state.isTimeoutPolicyEnabled = true - subject.state.policyTimeoutValue = 60 + subject.state.policyTimeoutValue = 1 subject.receive(.sessionTimeoutValueChanged(.fourHours)) @@ -430,17 +441,6 @@ class AccountSecurityProcessorTests: BitwardenTestCase { // swiftlint:disable:th waitFor(errorReporter.errors.last as? BitwardenTestError == BitwardenTestError.example) } - /// `receive(_:)` with `setCustomSessionTimeoutValue(_:)` updates the custom session timeout value in the state. - func test_receive_setCustomSessionTimeoutValue() { - XCTAssertEqual(subject.state.customTimeoutValue, 60) - - let account = Account.fixture() - authRepository.activeAccount = account - - subject.receive(.customTimeoutValueChanged(120)) - waitFor(subject.state.customTimeoutValue == 120) - } - /// `receive(_:)` with `.toggleUnlockWithPINCode` updates the state when submit has been pressed. func test_receive_toggleUnlockWithPINCode_toggleOff() { subject.state.isUnlockWithPINCodeOn = true diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift index e841c26780..1a93e74325 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift @@ -93,12 +93,12 @@ public enum SessionTimeoutValue: RawRepresentable, CaseIterable, Equatable, Menu public var rawValue: Int { switch self { case .immediately: 0 - case .oneMinute: 60 - case .fiveMinutes: 300 - case .fifteenMinutes: 900 - case .thirtyMinutes: 1800 - case .oneHour: 3600 - case .fourHours: 14400 + case .oneMinute: 1 + case .fiveMinutes: 5 + case .fifteenMinutes: 15 + case .thirtyMinutes: 30 + case .oneHour: 60 + case .fourHours: 240 case .onAppRestart: -1 case .never: -2 case let .custom(customValue): customValue @@ -109,17 +109,17 @@ public enum SessionTimeoutValue: RawRepresentable, CaseIterable, Equatable, Menu switch rawValue { case 0: self = .immediately - case 60: + case 1: self = .oneMinute - case 300: + case 5: self = .fiveMinutes - case 900: + case 15: self = .fifteenMinutes - case 1800: + case 30: self = .thirtyMinutes - case 3600: + case 60: self = .oneHour - case 14400: + case 240: self = .fourHours case -1: self = .onAppRestart @@ -218,20 +218,20 @@ struct AccountSecurityState: Equatable { /// The accessibility label used for the custom timeout value. var customTimeoutAccessibilityLabel: String { - customTimeoutValue.timeInHoursMinutes(shouldSpellOut: true) + customTimeoutValueSeconds.timeInHoursMinutes(shouldSpellOut: true) } - /// The custom session timeout value, initially set to 60 seconds. - var customTimeoutValue: Int { - guard case let .custom(customValue) = sessionTimeoutValue else { + /// The custom session timeout value, in seconds, initially set to 60 seconds. + var customTimeoutValueSeconds: Int { + guard case let .custom(customValueInMinutes) = sessionTimeoutValue, customValueInMinutes > 0 else { return 60 } - return customValue + return customValueInMinutes * 60 } /// The string representation of the custom session timeout value. var customTimeoutString: String { - customTimeoutValue.timeInHoursMinutes() + customTimeoutValueSeconds.timeInHoursMinutes() } /// Whether the user has a method to unlock the vault (master password, pin set, or biometrics @@ -258,11 +258,11 @@ struct AccountSecurityState: Equatable { /// The policy's timeout value in hours. var policyTimeoutHours: Int { - policyTimeoutValue / (60 * 60) + policyTimeoutValue / 60 } /// The policy's timeout value in minutes. var policyTimeoutMinutes: Int { - policyTimeoutValue / 60 % 60 + policyTimeoutValue % 60 } } diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift index 0a40bc600f..fd4297d0ec 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift @@ -150,8 +150,8 @@ struct AccountSecurityView: View { title: Localizations.custom, customTimeoutValue: store.state.customTimeoutString, pickerValue: store.binding( - get: \.customTimeoutValue, - send: AccountSecurityAction.customTimeoutValueChanged + get: \.customTimeoutValueSeconds, + send: AccountSecurityAction.customTimeoutValueSecondsChanged ), customTimeoutAccessibilityLabel: store.state.customTimeoutAccessibilityLabel ) diff --git a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityViewTests.swift b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityViewTests.swift index 572e4a60ee..77b6354442 100644 --- a/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityViewTests.swift +++ b/BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityViewTests.swift @@ -123,7 +123,7 @@ class AccountSecurityViewTests: BitwardenTestCase { enabled: false, hasValidIntegrity: true ), - sessionTimeoutValue: .custom(60) + sessionTimeoutValue: .custom(1) ) ) ) @@ -142,7 +142,7 @@ class AccountSecurityViewTests: BitwardenTestCase { enabled: true, hasValidIntegrity: true ), - sessionTimeoutValue: .custom(60) + sessionTimeoutValue: .custom(1) ) ) ) @@ -161,7 +161,7 @@ class AccountSecurityViewTests: BitwardenTestCase { enabled: true, hasValidIntegrity: false ), - sessionTimeoutValue: .custom(60) + sessionTimeoutValue: .custom(1) ) ) ) @@ -174,7 +174,7 @@ class AccountSecurityViewTests: BitwardenTestCase { let subject = AccountSecurityView( store: Store( processor: StateProcessor( - state: AccountSecurityState(sessionTimeoutValue: .custom(60)) + state: AccountSecurityState(sessionTimeoutValue: .custom(1)) ) ) ) @@ -203,7 +203,7 @@ class AccountSecurityViewTests: BitwardenTestCase { processor: StateProcessor( state: AccountSecurityState( isTimeoutPolicyEnabled: true, - sessionTimeoutValue: .custom(60) + sessionTimeoutValue: .custom(1) ) ) )