From 9a52dcf9ac97d1b4266970575c3aded957aca65e Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Fri, 24 Nov 2023 16:03:38 +0000 Subject: [PATCH] Add a state machine to AppLockFlowCoordinator. (#2166) Fixes a bug when cancelling Face ID would loop back round to presenting Face ID. --- .../AppLockFlowCoordinator.swift | 187 +++++++++++++----- .../Mocks/Generated/GeneratedMocks.swift | 11 +- ...ppLockSetupBiometricsScreenViewModel.swift | 2 +- .../AppLockSetupSettingsScreenViewModel.swift | 2 +- .../Services/AppLock/AppLockService.swift | 27 ++- .../AppLock/AppLockServiceProtocol.swift | 17 +- .../UITests/UITestsAppCoordinator.swift | 3 +- .../Sources/AppLock/AppLockServiceTests.swift | 2 +- changelog.d/2134.bugfix | 1 + 9 files changed, 188 insertions(+), 64 deletions(-) create mode 100644 changelog.d/2134.bugfix diff --git a/ElementX/Sources/FlowCoordinators/AppLockFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/AppLockFlowCoordinator.swift index d75dd7c05..ee45e7b4e 100644 --- a/ElementX/Sources/FlowCoordinators/AppLockFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/AppLockFlowCoordinator.swift @@ -15,6 +15,7 @@ // import Combine +import SwiftState import SwiftUI enum AppLockFlowCoordinatorAction: Equatable { @@ -31,100 +32,193 @@ class AppLockFlowCoordinator: CoordinatorProtocol { let appLockService: AppLockServiceProtocol let navigationCoordinator: NavigationRootCoordinator + /// States the flow can find itself in + enum State: StateType { + /// The initial state before the app has launched. + case initial + /// The app is in the foreground and visible to the user. + case unlocked + /// The app has resigned active but is not yet in the background. This state + /// shows the placeholder, but doesn't require an unlock on becoming active. + case appObscured + /// The app is in the background. + case backgrounded + /// The app is presenting biometric unlock to the user. + case attemptingBiometricUnlock + /// Biometric unlock has completed but the system UI is still the active input. + /// Once the app becomes active again, it will trigger the next state. + case dismissingBiometricUnlock(AppLockServiceBiometricResult) + /// The app is presenting the unlock screen for PIN code entry. + case attemptingPINUnlock + /// The user failed to unlock the app (or forgot their PIN) and is being logged out. + case loggingOut + } + + /// Events that can be triggered on the flow state machine + enum Event: EventType { + /// The app is resigning active (going into the app switcher, showing system UI like Face ID, permissions prompt etc). + case willResignActive + /// The app is now backgrounded and not visible to the user. + case didEnterBackground + /// The app is in the foreground and has been given focus. + case didBecomeActive + /// Biometric unlock has completed with the following result. + case didFinishBiometricUnlock(AppLockServiceBiometricResult) + /// The entered PIN code was accepted. + case didUnlockWithPIN + /// The user failed to unlock the app (or forgot their PIN). + case forceLogout + /// The service has been enabled. + case serviceEnabled + /// The service has been disabled. + case serviceDisabled + } + + private let stateMachine: StateMachine + /// A task used to await biometric unlock before showing the PIN screen. @CancellableTask private var unlockTask: Task? private var cancellables: Set = [] + /// Whether or not biometric unlock should be attempted instead of asking for a PIN. + private var isBiometricUnlockAvailable: Bool { + appLockService.biometricUnlockEnabled && appLockService.biometricUnlockTrusted + } + private let actionsSubject: PassthroughSubject = .init() var actions: AnyPublisher { actionsSubject.eraseToAnyPublisher() } - init(appLockService: AppLockServiceProtocol, + init(initialState: State = .initial, + appLockService: AppLockServiceProtocol, navigationCoordinator: NavigationRootCoordinator, notificationCenter: NotificationCenter = .default) { self.appLockService = appLockService self.navigationCoordinator = navigationCoordinator - // Set the initial background state. + // Set the initial state and start with the placeholder screen as the root view. + stateMachine = .init(state: initialState) showPlaceholder() notificationCenter.publisher(for: UIApplication.willResignActiveNotification) .sink { [weak self] _ in - self?.applicationWillResignActive() + self?.stateMachine.tryEvent(.willResignActive) } .store(in: &cancellables) notificationCenter.publisher(for: UIApplication.didEnterBackgroundNotification) .sink { [weak self] _ in - self?.applicationDidEnterBackground() + self?.stateMachine.tryEvent(.didEnterBackground) } .store(in: &cancellables) notificationCenter.publisher(for: UIApplication.didBecomeActiveNotification) .sink { [weak self] _ in - self?.applicationDidBecomeActive() + self?.stateMachine.tryEvent(.didBecomeActive) } .store(in: &cancellables) + + appLockService.isEnabledPublisher + .sink { [weak self] isEnabled in + self?.stateMachine.tryEvent(isEnabled ? .serviceEnabled : .serviceDisabled) + } + .store(in: &cancellables) + + configureStateMachine() } func toPresentable() -> AnyView { AnyView(navigationCoordinator.toPresentable()) } - // MARK: - App unlock + // MARK: - State machine - private func applicationWillResignActive() { - unlockTask = nil - - guard appLockService.isEnabled else { return } - showPlaceholder() - } - - private func applicationDidEnterBackground() { - guard appLockService.isEnabled else { return } - appLockService.applicationDidEnterBackground() - showPlaceholder() // Double call but just to be safe - } - - private func applicationDidBecomeActive() { - guard appLockService.isEnabled else { return } - - guard appLockService.computeNeedsUnlock(didBecomeActiveAt: .now) else { - // Reveal the app again if within the grace period. - actionsSubject.send(.unlockApp) - return - } - - // Show the relevant unlock mechanism. - unlockTask = Task { [weak self] in - guard let self else { return } - await startUnlockFlow() - } - } - - /// Runs the unlock flow, showing Touch ID/Face ID if available, transitioning to PIN unlock if it fails or isn't available. - private func startUnlockFlow() async { - if appLockService.biometricUnlockEnabled, appLockService.biometricUnlockTrusted { - showPlaceholder() // For the unlock background. + private func configureStateMachine() { + stateMachine.addRouteMapping { [weak self] event, fromState, _ in + guard let self, appLockService.isEnabled else { return fromState } - if await appLockService.unlockWithBiometrics(), UIApplication.shared.applicationState == .active { - actionsSubject.send(.unlockApp) - return + switch (fromState, event) { + case (.unlocked, .willResignActive): + return .appObscured + case (.appObscured, .didBecomeActive): + return .unlocked + case (_, .didEnterBackground): + return .backgrounded + case (.backgrounded, .didBecomeActive), (.initial, .didBecomeActive): + guard appLockService.computeNeedsUnlock(didBecomeActiveAt: .now) else { return .unlocked } + return isBiometricUnlockAvailable ? .attemptingBiometricUnlock : .attemptingPINUnlock + case (.attemptingBiometricUnlock, .didFinishBiometricUnlock(let result)): + return .dismissingBiometricUnlock(result) // Transitional state until the app becomes active again. + case (.dismissingBiometricUnlock(let result), .didBecomeActive): + return switch result { + case .unlocked: .unlocked + case .failed: .attemptingPINUnlock + case .interrupted: .attemptingBiometricUnlock + } + case (.attemptingPINUnlock, .didUnlockWithPIN): + return .unlocked + case (.attemptingPINUnlock, .forceLogout): + return .loggingOut + + // Transition to a valid state when enabling the service for the first time. + case (.initial, .serviceEnabled): + return .unlocked + // Transition to a valid state once the service is disabled following a forced logout. + case (.loggingOut, .serviceDisabled): + return .unlocked + + default: + return fromState } } - guard !Task.isCancelled else { return } + stateMachine.addAnyHandler(.any => .any) { [weak self] context in + guard let self, context.fromState != context.toState else { return } + + MXLog.info("Transitioning from `\(context.fromState)` to `\(context.toState)` with event `\(String(describing: context.event))`.") + + switch (context.fromState, context.toState) { + case (_, .appObscured): + showPlaceholder() + case (_, .backgrounded): + appLockService.applicationDidEnterBackground() + showPlaceholder() // Double call but just to be safe. + case (_, .attemptingBiometricUnlock): + showPlaceholder() // For the unlock background. Triple call but just to be safe. + Task { await self.attemptBiometricUnlock() } + case (.attemptingBiometricUnlock, .dismissingBiometricUnlock): + break // Transitional state, no need to do anything. + case (_, .attemptingPINUnlock): + showUnlockScreen() + case (_, .unlocked): + actionsSubject.send(.unlockApp) + case (_, .loggingOut): + actionsSubject.send(.forceLogout) + default: + fatalError("Unhandled transition.") + } + } - showUnlockScreen() + stateMachine.addErrorHandler { context in + fatalError("Unexpected transition from `\(context.fromState)` to `\(context.toState)` with event `\(String(describing: context.event))`.") + } } + // MARK: - App unlock + /// Displays the unlock flow with the app's placeholder view to hide obscure the view hierarchy in the app switcher. private func showPlaceholder() { navigationCoordinator.setRootCoordinator(PlaceholderScreenCoordinator(showsBackgroundGradient: true), animated: false) actionsSubject.send(.lockApp) } + /// Attempts to authenticate the user using Face ID, Touch ID or (possibly) Optic ID. + private func attemptBiometricUnlock() async { + let result = await appLockService.unlockWithBiometrics() + stateMachine.tryEvent(.didFinishBiometricUnlock(result)) + } + /// Displays the unlock flow with the main unlock screen. private func showUnlockScreen() { let coordinator = AppLockScreenCoordinator(parameters: .init(appLockService: appLockService)) @@ -132,10 +226,9 @@ class AppLockFlowCoordinator: CoordinatorProtocol { guard let self else { return } switch action { case .appUnlocked: - guard UIApplication.shared.applicationState == .active else { return } - actionsSubject.send(.unlockApp) + stateMachine.tryEvent(.didUnlockWithPIN) case .forceLogout: - actionsSubject.send(.forceLogout) + stateMachine.tryEvent(.forceLogout) } } .store(in: &cancellables) diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index 02f48caa5..cb4dcd478 100644 --- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift +++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift @@ -128,6 +128,11 @@ class AppLockServiceMock: AppLockServiceProtocol { set(value) { underlyingIsEnabled = value } } var underlyingIsEnabled: Bool! + var isEnabledPublisher: AnyPublisher { + get { return underlyingIsEnabledPublisher } + set(value) { underlyingIsEnabledPublisher = value } + } + var underlyingIsEnabledPublisher: AnyPublisher! var biometryType: LABiometryType { get { return underlyingBiometryType } set(value) { underlyingBiometryType = value } @@ -292,10 +297,10 @@ class AppLockServiceMock: AppLockServiceProtocol { var unlockWithBiometricsCalled: Bool { return unlockWithBiometricsCallsCount > 0 } - var unlockWithBiometricsReturnValue: Bool! - var unlockWithBiometricsClosure: (() async -> Bool)? + var unlockWithBiometricsReturnValue: AppLockServiceBiometricResult! + var unlockWithBiometricsClosure: (() async -> AppLockServiceBiometricResult)? - func unlockWithBiometrics() async -> Bool { + func unlockWithBiometrics() async -> AppLockServiceBiometricResult { unlockWithBiometricsCallsCount += 1 if let unlockWithBiometricsClosure = unlockWithBiometricsClosure { return await unlockWithBiometricsClosure() diff --git a/ElementX/Sources/Screens/AppLock/AppLockSetupBiometricsScreen/AppLockSetupBiometricsScreenViewModel.swift b/ElementX/Sources/Screens/AppLock/AppLockSetupBiometricsScreen/AppLockSetupBiometricsScreenViewModel.swift index 65481406c..7ec3cdfb7 100644 --- a/ElementX/Sources/Screens/AppLock/AppLockSetupBiometricsScreen/AppLockSetupBiometricsScreenViewModel.swift +++ b/ElementX/Sources/Screens/AppLock/AppLockSetupBiometricsScreen/AppLockSetupBiometricsScreenViewModel.swift @@ -56,7 +56,7 @@ class AppLockSetupBiometricsScreenViewModel: AppLockSetupBiometricsScreenViewMod // Attempt unlock to trigger Face ID permissions alert. if appLockService.biometryType == .faceID, - await !appLockService.unlockWithBiometrics() { + await appLockService.unlockWithBiometrics() != .unlocked { MXLog.info("Confirmation failed. Disabling biometric unlock.") appLockService.disableBiometricUnlock() } diff --git a/ElementX/Sources/Screens/AppLock/AppLockSetupSettingsScreen/AppLockSetupSettingsScreenViewModel.swift b/ElementX/Sources/Screens/AppLock/AppLockSetupSettingsScreen/AppLockSetupSettingsScreenViewModel.swift index 14113ee0a..561a04ebd 100644 --- a/ElementX/Sources/Screens/AppLock/AppLockSetupSettingsScreen/AppLockSetupSettingsScreenViewModel.swift +++ b/ElementX/Sources/Screens/AppLock/AppLockSetupSettingsScreen/AppLockSetupSettingsScreenViewModel.swift @@ -62,7 +62,7 @@ class AppLockSetupSettingsScreenViewModel: AppLockSetupSettingsScreenViewModelTy // Attempt unlock to trigger Face ID permissions alert. if appLockService.biometryType == .faceID, - await !appLockService.unlockWithBiometrics() { + await appLockService.unlockWithBiometrics() != .unlocked { MXLog.info("Confirmation failed. Disabling biometric unlock.") state.bindings.enableBiometrics = false appLockService.disableBiometricUnlock() diff --git a/ElementX/Sources/Services/AppLock/AppLockService.swift b/ElementX/Sources/Services/AppLock/AppLockService.swift index 429b19998..c22276538 100644 --- a/ElementX/Sources/Services/AppLock/AppLockService.swift +++ b/ElementX/Sources/Services/AppLock/AppLockService.swift @@ -38,6 +38,9 @@ class AppLockService: AppLockServiceProtocol { } } + private var isEnabledSubject: PassthroughSubject = .init() + var isEnabledPublisher: AnyPublisher { isEnabledSubject.eraseToAnyPublisher() } + var biometryType: LABiometryType { updateBiometrics() guard context.evaluatedPolicyDomainState != nil else { return .none } @@ -71,6 +74,7 @@ class AppLockService: AppLockServiceProtocol { do { try keychainController.setPINCode(pinCode) + isEnabledSubject.send(true) return .success(()) } catch { MXLog.error("Keychain access error: \(error)") @@ -105,6 +109,7 @@ class AppLockService: AppLockServiceProtocol { keychainController.removePINCode() keychainController.removePINCodeBiometricState() appSettings.appLockNumberOfPINAttempts = 0 + isEnabledSubject.send(false) } func applicationDidEnterBackground() { @@ -128,30 +133,35 @@ class AppLockService: AppLockServiceProtocol { _ = enableBiometricUnlock() } - return completeUnlock() + completeUnlock() + return true } - func unlockWithBiometrics() async -> Bool { + func unlockWithBiometrics() async -> AppLockServiceBiometricResult { guard biometryType != .none, biometricUnlockEnabled else { MXLog.error("Biometric unlock not setup.") - return false + return .failed } guard biometricUnlockTrusted else { MXLog.error("Biometrics have changed. PIN should be shown.") - return false + return .failed } do { let context = unlockContext() guard try await context.evaluatePolicy(unlockPolicy, localizedReason: L10n.screenAppLockBiometricUnlockReasonIos) else { MXLog.warning("\(context.biometryType) failed without error.") - return false + return .failed } - return completeUnlock() + completeUnlock() + return .unlocked + } catch LAError.systemCancel { + MXLog.error("\(context.biometryType) failed: The system cancelled.") + return .interrupted } catch { MXLog.error("\(context.biometryType) failed: \(error)") - return false + return .failed } } @@ -180,9 +190,8 @@ class AppLockService: AppLockServiceProtocol { } /// Shared logic for completing an unlock via a PIN or biometrics. - private func completeUnlock() -> Bool { + private func completeUnlock() { timer.registerUnlock() appSettings.appLockNumberOfPINAttempts = 0 - return true } } diff --git a/ElementX/Sources/Services/AppLock/AppLockServiceProtocol.swift b/ElementX/Sources/Services/AppLock/AppLockServiceProtocol.swift index ca4194e74..1d48e2d94 100644 --- a/ElementX/Sources/Services/AppLock/AppLockServiceProtocol.swift +++ b/ElementX/Sources/Services/AppLock/AppLockServiceProtocol.swift @@ -30,6 +30,18 @@ enum AppLockServiceError: Error { case biometricUnlockNotSupported } +/// The result of an attempt to unlock the app using Touch ID or Face ID. +enum AppLockServiceBiometricResult { + /// Biometric lock was successful. + case unlocked + /// Biometric lock failed to authenticate the user. This represents any failure + /// other than the app being backgrounded during the authentication. + case failed + /// Biometric lock was interrupted by the system and did not complete. The expected + /// cause for this that the app was backgrounded whilst the request was in progress. + case interrupted +} + @MainActor protocol AppLockServiceProtocol: AnyObject { /// The use of a PIN code is mandatory for this device. @@ -37,6 +49,9 @@ protocol AppLockServiceProtocol: AnyObject { /// The app has been configured to automatically lock with a PIN code. var isEnabled: Bool { get } + /// A publisher that advertises when the service has been enabled or disabled. + var isEnabledPublisher: AnyPublisher { get } + /// The type of biometric authentication supported by the device. var biometryType: LABiometryType { get } /// Whether or not the user has enabled unlock via TouchID, FaceID or (possibly) OpticID. @@ -64,7 +79,7 @@ protocol AppLockServiceProtocol: AnyObject { /// Attempt to unlock the app with the supplied PIN code. func unlock(with pinCode: String) -> Bool /// Attempt to unlock the app using FaceID or TouchID. - func unlockWithBiometrics() async -> Bool + func unlockWithBiometrics() async -> AppLockServiceBiometricResult /// The number of attempts the user had made to unlock with a PIN code. /// diff --git a/ElementX/Sources/UITests/UITestsAppCoordinator.swift b/ElementX/Sources/UITests/UITestsAppCoordinator.swift index 93aae5a0a..449b3b52d 100644 --- a/ElementX/Sources/UITests/UITestsAppCoordinator.swift +++ b/ElementX/Sources/UITests/UITestsAppCoordinator.swift @@ -201,7 +201,8 @@ class MockScreen: Identifiable { fatalError("Failed to start listening for notifications.") } - let coordinator = AppLockFlowCoordinator(appLockService: appLockService, + let coordinator = AppLockFlowCoordinator(initialState: .unlocked, + appLockService: appLockService, navigationCoordinator: navigationCoordinator, notificationCenter: notificationCenter) diff --git a/UnitTests/Sources/AppLock/AppLockServiceTests.swift b/UnitTests/Sources/AppLock/AppLockServiceTests.swift index b34ba19d8..bfc953ed4 100644 --- a/UnitTests/Sources/AppLock/AppLockServiceTests.swift +++ b/UnitTests/Sources/AppLock/AppLockServiceTests.swift @@ -207,7 +207,7 @@ class AppLockServiceTests: XCTestCase { XCTAssertEqual(service.biometryType, .touchID, "The biometry type should not change.") XCTAssertTrue(service.biometricUnlockEnabled, "Biometric unlock should now be enabled.") XCTAssertTrue(service.biometricUnlockTrusted, "Biometric unlock should now be trusted.") - guard await service.unlockWithBiometrics() else { + guard await service.unlockWithBiometrics() == .unlocked else { XCTFail("The biometric unlock should work.") return } diff --git a/changelog.d/2134.bugfix b/changelog.d/2134.bugfix new file mode 100644 index 000000000..27f73cb1e --- /dev/null +++ b/changelog.d/2134.bugfix @@ -0,0 +1 @@ +Fix a bug where cancelling Face ID unlock would loop back round and present Face ID again. \ No newline at end of file