From 495ee8ab380625c3b3379bc4b5e3936159aabe24 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Thu, 7 Dec 2023 13:19:58 +0200 Subject: [PATCH] More secure backup tweaks (#2212) * Make sure the indicator for disabling backups is dismissed together with the screen * Get rid of the .disabled key backup state as it has no correspondant on the rust side and is confusing * Remove superflous BackupState extensions, add more logs --- ...SecureBackupKeyBackupScreenViewModel.swift | 21 ++++-- .../SecureBackupScreenViewModel.swift | 2 +- .../View/SecureBackupScreen.swift | 8 +-- .../SecureBackup/SecureBackupController.swift | 66 ++++++++++++------- .../SecureBackupControllerProtocol.swift | 1 - 5 files changed, 64 insertions(+), 34 deletions(-) diff --git a/ElementX/Sources/Screens/SecureBackup/SecureBackupKeyBackupScreen/SecureBackupKeyBackupScreenViewModel.swift b/ElementX/Sources/Screens/SecureBackup/SecureBackupKeyBackupScreen/SecureBackupKeyBackupScreenViewModel.swift index ba6a8b2da0..6572c7a58c 100644 --- a/ElementX/Sources/Screens/SecureBackup/SecureBackupKeyBackupScreen/SecureBackupKeyBackupScreenViewModel.swift +++ b/ElementX/Sources/Screens/SecureBackup/SecureBackupKeyBackupScreen/SecureBackupKeyBackupScreenViewModel.swift @@ -21,6 +21,7 @@ typealias SecureBackupKeyBackupScreenViewModelType = StateStoreViewModel = .init() var actions: AnyPublisher { @@ -29,18 +30,18 @@ class SecureBackupKeyBackupScreenViewModel: SecureBackupKeyBackupScreenViewModel init(secureBackupController: SecureBackupControllerProtocol, userIndicatorController: UserIndicatorControllerProtocol?) { self.secureBackupController = secureBackupController + self.userIndicatorController = userIndicatorController super.init(initialViewState: .init(mode: secureBackupController.keyBackupState.value.viewMode)) secureBackupController.keyBackupState .receive(on: DispatchQueue.main) - .sink { [weak userIndicatorController] state in - let loadingIndicatorIdentifier = "SecureBackupKeyBackupScreenLoading" + .sink { [weak self] state in switch state { case .disabling, .enabling, .unknown: - userIndicatorController?.submitIndicator(.init(id: loadingIndicatorIdentifier, type: .modal, title: L10n.commonLoading, persistent: true)) + self?.showLoadingIndicator() default: - userIndicatorController?.retractIndicatorWithId(loadingIndicatorIdentifier) + self?.dismissLoadingIndicator() } } .store(in: &cancellables) @@ -80,8 +81,20 @@ class SecureBackupKeyBackupScreenViewModel: SecureBackupKeyBackupScreenViewModel MXLog.error("Failed disabling key backup with error: \(error)") state.bindings.alertInfo = .init(id: .init()) } + + dismissLoadingIndicator() } } + + private static let loadingIndicatorIdentifier = "SecureBackupKeyBackupScreenLoading" + + private func showLoadingIndicator() { + userIndicatorController?.submitIndicator(.init(id: Self.loadingIndicatorIdentifier, type: .modal, title: L10n.commonLoading, persistent: true)) + } + + private func dismissLoadingIndicator() { + userIndicatorController?.retractIndicatorWithId(Self.loadingIndicatorIdentifier) + } } extension SecureBackupKeyBackupState { diff --git a/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/SecureBackupScreenViewModel.swift b/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/SecureBackupScreenViewModel.swift index 1f6e425dc1..d4524113a9 100644 --- a/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/SecureBackupScreenViewModel.swift +++ b/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/SecureBackupScreenViewModel.swift @@ -55,7 +55,7 @@ class SecureBackupScreenViewModel: SecureBackupScreenViewModelType, SecureBackup actionsSubject.send(.recoveryKey) case .keyBackup: switch secureBackupController.keyBackupState.value { - case .disabled, .unknown: + case .unknown: enableBackup() case .enabled: actionsSubject.send(.keyBackup) diff --git a/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/View/SecureBackupScreen.swift b/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/View/SecureBackupScreen.swift index eb35a42fda..c473dd0696 100644 --- a/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/View/SecureBackupScreen.swift +++ b/ElementX/Sources/Screens/SecureBackup/SecureBackupScreen/View/SecureBackupScreen.swift @@ -23,7 +23,7 @@ struct SecureBackupScreen: View { var body: some View { Form { - if context.viewState.keyBackupState == .disabled { + if context.viewState.keyBackupState == .unknown { keyBackupSection } else { keyBackupSection @@ -77,12 +77,10 @@ struct SecureBackupScreen: View { ListRow(label: .plain(title: L10n.screenChatBackupKeyBackupActionDisable, role: .destructive), kind: .navigationLink { context.send(viewAction: .keyBackup) }) - case .disabled, .enabling: + case .unknown, .enabling: ListRow(label: .plain(title: L10n.screenChatBackupKeyBackupActionEnable), kind: .navigationLink { context.send(viewAction: .keyBackup) }) - case .unknown: - EmptyView() } } @@ -128,7 +126,7 @@ struct SecureBackupScreen: View { struct SecureBackupScreen_Previews: PreviewProvider, TestablePreview { static let bothSetupViewModel = viewModel(keyBackupState: .enabled, recoveryKeyState: .enabled) static let onlyKeyBackupSetUpViewModel = viewModel(keyBackupState: .enabled, recoveryKeyState: .disabled) - static let keyBackupDisabledViewModel = viewModel(keyBackupState: .disabled, recoveryKeyState: .disabled) + static let keyBackupDisabledViewModel = viewModel(keyBackupState: .unknown, recoveryKeyState: .disabled) static let recoveryIncompleteViewModel = viewModel(keyBackupState: .enabled, recoveryKeyState: .incomplete) static var previews: some View { diff --git a/ElementX/Sources/Services/SecureBackup/SecureBackupController.swift b/ElementX/Sources/Services/SecureBackup/SecureBackupController.swift index ef3c671194..ffce00e3c4 100644 --- a/ElementX/Sources/Services/SecureBackup/SecureBackupController.swift +++ b/ElementX/Sources/Services/SecureBackup/SecureBackupController.swift @@ -44,7 +44,24 @@ class SecureBackupController: SecureBackupControllerProtocol { backupStateListenerTaskHandle = encryption.backupStateListener(listener: SecureBackupControllerBackupStateListener { [weak self] state in guard let self else { return } - keyBackupStateSubject.send(state.keyBackupState) + switch state { + case .unknown: + keyBackupStateSubject.send(.unknown) + case .creating: + keyBackupStateSubject.send(.enabling) + case .enabling: + keyBackupStateSubject.send(.enabling) + case .resuming: + keyBackupStateSubject.send(.enabled) + case .enabled: + keyBackupStateSubject.send(.enabled) + case .downloading: + keyBackupStateSubject.send(.enabled) + case .disabling: + keyBackupStateSubject.send(.disabling) + } + + MXLog.info("Key backup state changed to: \(state), setting local state to \(keyBackupStateSubject.value)") if case .unknown = state { updateBackupStateFromRemote() @@ -64,15 +81,21 @@ class SecureBackupController: SecureBackupControllerProtocol { case .incomplete: recoveryKeyStateSubject.send(.incomplete) } + + MXLog.info("Recovery state changed to: \(state), setting local state to \(recoveryKeyStateSubject.value)") }) updateBackupStateFromRemote() } func enable() async -> Result { + MXLog.info("Enabling secure backup") + do { try await encryption.enableBackups() } catch { + MXLog.error("Failed enabling secure backup with error: \(error)") + return .failure(.failedEnablingBackup) } @@ -80,9 +103,12 @@ class SecureBackupController: SecureBackupControllerProtocol { } func disable() async -> Result { + MXLog.info("Disabling secure backup") + do { try await encryption.disableRecovery() } catch { + MXLog.error("Failed disabling secure backup with error: \(error)") return .failure(.failedDisablingBackup) } @@ -92,10 +118,14 @@ class SecureBackupController: SecureBackupControllerProtocol { func generateRecoveryKey() async -> Result { do { guard recoveryKeyState.value == .disabled else { + MXLog.info("Resetting recovery key") + let key = try await encryption.resetRecoveryKey() return .success(key) } + MXLog.info("Enabling recovery") + var keyUploadErrored = false let recoveryKey = try await encryption.enableRecovery(waitForBackupsToUpload: false, progressListener: SecureBackupEnableRecoveryProgressListener { [weak self] state in guard let self else { return } @@ -106,38 +136,48 @@ class SecureBackupController: SecureBackupControllerProtocol { case .done: recoveryKeyStateSubject.send(.enabled) case .roomKeyUploadError: + MXLog.error("Failed enabling recovery: room key upload error") keyUploadErrored = true } }) return keyUploadErrored ? .failure(.failedGeneratingRecoveryKey) : .success(recoveryKey) } catch { + MXLog.error("Failed generating recovery key with error: \(error)") + return .failure(.failedGeneratingRecoveryKey) } } func confirmRecoveryKey(_ key: String) async -> Result { do { + MXLog.info("Confirming recovery key") try await encryption.recover(recoveryKey: key) return .success(()) } catch { + MXLog.info("Failed confirming recovery key with error: \(error)") return .failure(.failedConfirmingRecoveryKey) } } func isLastSession() async -> Result { do { + MXLog.info("Checking if last session") return try await .success(encryption.isLastDevice()) } catch { + MXLog.error("Failed checking if last session with error: \(error)") return .failure(.failedFetchingSessionState) } } func waitForKeyBackupUpload() async -> Result { do { + MXLog.info("Waiting for backup upload steady state") try await encryption.waitForBackupUploadSteadyState(progressListener: nil) return .success(()) } catch let error as SteadyStateError { + MXLog.error("Failed waiting for backup upload steady state with error: \(error)") + switch error { case .BackupDisabled: MXLog.error("Key backup disabled, continuing logout.") @@ -157,6 +197,7 @@ class SecureBackupController: SecureBackupControllerProtocol { private func updateBackupStateFromRemote(retry: Bool = true) { remoteBackupStateTask = Task { do { + MXLog.info("Checking if backup exists on the server") let backupExists = try await self.encryption.backupExistsOnServer() if Task.isCancelled { @@ -164,7 +205,7 @@ class SecureBackupController: SecureBackupControllerProtocol { } if !backupExists { - keyBackupStateSubject.send(.disabled) + keyBackupStateSubject.send(.unknown) } } catch { MXLog.error("Failed retrieving remote backup state with error: \(error)") @@ -212,24 +253,3 @@ private final class SecureBackupEnableRecoveryProgressListener: EnableRecoveryPr onUpdateClosure(status) } } - -extension BackupState { - var keyBackupState: SecureBackupKeyBackupState { - switch self { - case .unknown: - return .unknown - case .creating: - return .enabling - case .enabling: - return .enabling - case .resuming: - return .enabled - case .enabled: - return .enabled - case .downloading: - return .enabled - case .disabling: - return .disabling - } - } -} diff --git a/ElementX/Sources/Services/SecureBackup/SecureBackupControllerProtocol.swift b/ElementX/Sources/Services/SecureBackup/SecureBackupControllerProtocol.swift index 0311b52540..49a9fb6e10 100644 --- a/ElementX/Sources/Services/SecureBackup/SecureBackupControllerProtocol.swift +++ b/ElementX/Sources/Services/SecureBackup/SecureBackupControllerProtocol.swift @@ -34,7 +34,6 @@ enum SecureBackupKeyBackupState { case enabling case enabled case disabling - case disabled } enum SecureBackupControllerError: Error {