From 34e2369220962c9c6dff811aa400c45a2c0eb7aa Mon Sep 17 00:00:00 2001 From: Milan Sawicki Date: Wed, 24 May 2023 13:27:03 +0200 Subject: [PATCH] [CBW-1031] fix app flow when new t&c available password is created (#318) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Purpose After upgrading an existing wallet though ​[CBW-910](unreleased) and accepting terms and condition, the user is prompted to create a new password. This would allow an attacker to reset the password on a stolen phone, and is undesired from perspective of user experience. ## Changes I reasured that after current T&C version response is fetched, we do check if user has a password already set. I covered the missing scenario with adequate tests. [CBW-910]: https://concordium.atlassian.net/browse/CBW-910?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --- ConcordiumWallet/AppCoordinator.swift | 47 ++++---- .../Views/Login/LoginCoordinator.swift | 43 +++++--- .../Coordinators/LoginCoordinatorTests.swift | 100 ++++++++++++++++-- 3 files changed, 146 insertions(+), 44 deletions(-) diff --git a/ConcordiumWallet/AppCoordinator.swift b/ConcordiumWallet/AppCoordinator.swift index 3405861b..15822d2f 100644 --- a/ConcordiumWallet/AppCoordinator.swift +++ b/ConcordiumWallet/AppCoordinator.swift @@ -15,7 +15,7 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate var navigationController: UINavigationController let defaultProvider = ServicesProvider.defaultProvider() - + private var accountsCoordinator: AccountsCoordinator? private var needsAppCheck = true private var cancellables: [AnyCancellable] = [] @@ -49,25 +49,31 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate } private func showLogin() { - let loginCoordinator = LoginCoordinator(navigationController: navigationController, - parentCoordinator: self, - dependencyProvider: defaultProvider) + let loginCoordinator = LoginCoordinator( + navigationController: navigationController, + parentCoordinator: self, + dependencyProvider: defaultProvider, + termsAndCondtionsFactory: { [weak self] termsAndConditions in + guard let self = self else { return nil } + return TermsAndConditionsViewModel(storageManager: self.defaultProvider.storageManager(), termsAndConditions: termsAndConditions) + } + ) childCoordinators.append(loginCoordinator) loginCoordinator.start() } func showMainTabbar() { navigationController.setupBaseNavigationControllerStyle() - + accountsCoordinator = AccountsCoordinator( - navigationController: self.navigationController, + navigationController: navigationController, dependencyProvider: defaultProvider, appSettingsDelegate: self, accountsPresenterDelegate: self ) // accountsCoordinator?.delegate = self accountsCoordinator?.start() - + sanityChecker.showValidateIdentitiesAlert(report: SanityChecker.lastSanityReport, mode: .automatic, completion: { self.showDelegationWarningIfNeeded() }) @@ -154,9 +160,9 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate delegate: self ) recoveryPhraseCoordinator.start() - self.navigationController.viewControllers = Array(self.navigationController.viewControllers.lastElements(1)) + navigationController.viewControllers = Array(navigationController.viewControllers.lastElements(1)) childCoordinators.append(recoveryPhraseCoordinator) - self.navigationController.setupBaseNavigationControllerStyle() + navigationController.setupBaseNavigationControllerStyle() } } else { let initialAccountCreateCoordinator = InitialAccountsCoordinator(navigationController: navigationController, @@ -169,7 +175,7 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate navigationController.setupBaseNavigationControllerStyle() } } - + func showSeedIdentityCreation() { let coordinator = SeedIdentitiesCoordinator( navigationController: navigationController, @@ -177,24 +183,24 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate dependencyProvider: defaultProvider, delegate: self ) - + coordinator.start() - + childCoordinators.append(coordinator) - self.navigationController.setupBaseNavigationControllerStyle() + navigationController.setupBaseNavigationControllerStyle() } - + func logout() { let keyWindow = UIApplication.shared.windows.filter { $0.isKeyWindow }.first if var topController = keyWindow?.rootViewController { while let presentedViewController = topController.presentedViewController { topController = presentedViewController } - + if (topController as? TransparentNavigationController)?.viewControllers.last is EnterPasswordViewController { return } - + topController.dismiss(animated: false) { Logger.trace("logout due to application timeout") self.childCoordinators.removeAll() @@ -267,9 +273,9 @@ extension AppCoordinator: AccountsPresenterDelegate { func didSelectPendingIdentity(identity: IdentityDataType) { } - + func showSettings() { - let moreCoordinator = MoreCoordinator(navigationController: self.navigationController, + let moreCoordinator = MoreCoordinator(navigationController: navigationController, dependencyProvider: defaultProvider, parentCoordinator: self) moreCoordinator.start() @@ -364,7 +370,6 @@ extension AppCoordinator: IdentitiesCoordinatorDelegate, MoreCoordinatorDelegate } extension AppCoordinator: AppSettingsDelegate { - func checkForAppSettings() { guard needsAppCheck else { return } needsAppCheck = false @@ -379,7 +384,7 @@ extension AppCoordinator: AppSettingsDelegate { ) .store(in: &cancellables) } - + private func handleAppSettings(response: AppSettingsResponse) { showUpdateDialogIfNeeded( appSettingsResponse: response @@ -402,7 +407,7 @@ extension AppCoordinator: RecoveryPhraseCoordinatorDelegate { showSeedIdentityCreation() childCoordinators.removeAll { $0 is RecoveryPhraseCoordinator } } - + func recoveryPhraseCoordinatorFinishedRecovery() { showMainTabbar() childCoordinators.removeAll { $0 is RecoveryPhraseCoordinator } diff --git a/ConcordiumWallet/Views/Login/LoginCoordinator.swift b/ConcordiumWallet/Views/Login/LoginCoordinator.swift index 49f4314a..a234c093 100644 --- a/ConcordiumWallet/Views/Login/LoginCoordinator.swift +++ b/ConcordiumWallet/Views/Login/LoginCoordinator.swift @@ -16,6 +16,8 @@ protocol LoginCoordinatorDelegate: AppSettingsDelegate { func passwordSelectionDone() } +typealias TermsAndConditionsViewFactory = (TermsAndConditionsResponse) -> (TermsAndConditionsViewModel?) + class LoginCoordinator: Coordinator, ShowAlert { var childCoordinators = [Coordinator]() @@ -23,8 +25,14 @@ class LoginCoordinator: Coordinator, ShowAlert { let dependencyProvider: LoginDependencyProvider private var cancellables: Set = [] private weak var parentCoordinator: LoginCoordinatorDelegate? - - init(navigationController: UINavigationController, parentCoordinator: LoginCoordinatorDelegate, dependencyProvider: LoginDependencyProvider) { + private var termsAndCondtionsFactory: TermsAndConditionsViewFactory + init( + navigationController: UINavigationController, + parentCoordinator: LoginCoordinatorDelegate, + dependencyProvider: LoginDependencyProvider, + termsAndCondtionsFactory: @escaping TermsAndConditionsViewFactory + ) { + self.termsAndCondtionsFactory = termsAndCondtionsFactory self.navigationController = navigationController self.parentCoordinator = parentCoordinator self.dependencyProvider = dependencyProvider @@ -57,21 +65,22 @@ class LoginCoordinator: Coordinator, ShowAlert { navigationController.pushViewController(vc, animated: true) } - func show(termsAndConditions: TermsAndConditionsResponse) { - let viewModel = TermsAndConditionsViewModel( - storageManager: dependencyProvider.storageManager(), - termsAndConditions: termsAndConditions - ) + func show(termsAndConditions: TermsAndConditionsResponse, isPasswordCreated: Bool) { + guard let viewModel = termsAndCondtionsFactory(termsAndConditions) else { return } viewModel.didAcceptTermsAndConditions = { [weak self] in - self?.showInitialScreen() + if isPasswordCreated { + self?.showLogin() + } else { + self?.showInitialScreen() + } } - let vc = UIHostingController(rootView: TermsAndConditionsView(viewModel: viewModel)) - navigationController.pushViewController(vc, animated: true) + let viewController = UIHostingController(rootView: TermsAndConditionsView(viewModel: viewModel)) + navigationController.pushViewController(viewController, animated: true) } func start() { let passwordCreated = dependencyProvider.keychainWrapper().passwordCreated() - let version = dependencyProvider.storageManager().getLastAcceptedTermsAndConditionsVersion() + let acceptedVersion = dependencyProvider.storageManager().getLastAcceptedTermsAndConditionsVersion() dependencyProvider.appSettingsService() .getTermsAndConditionsVersion() .sink(receiveCompletion: { [weak self] completion in @@ -79,13 +88,17 @@ class LoginCoordinator: Coordinator, ShowAlert { case .finished: break case let .failure(serverError): Logger.error(serverError) - self?.showErrorAlert(ErrorMapper.toViewError(error: serverError)) + self?.showErrorAlert(ErrorMapper.toViewError(error: serverError)) } }, receiveValue: { [weak self] termsAndConditions in - if passwordCreated && termsAndConditions.version == version { - self?.showLogin() + if termsAndConditions.version == acceptedVersion { + if passwordCreated { + self?.showLogin() + } else { + self?.showInitialScreen() + } } else { - self?.show(termsAndConditions: termsAndConditions) + self?.show(termsAndConditions: termsAndConditions, isPasswordCreated: passwordCreated) } }) .store(in: &cancellables) diff --git a/ConcordiumWalletTests/Coordinators/LoginCoordinatorTests.swift b/ConcordiumWalletTests/Coordinators/LoginCoordinatorTests.swift index 3f776cc1..5df836ad 100644 --- a/ConcordiumWalletTests/Coordinators/LoginCoordinatorTests.swift +++ b/ConcordiumWalletTests/Coordinators/LoginCoordinatorTests.swift @@ -18,7 +18,7 @@ class LoginCoordinatorTests: XCTestCase { var keychainMock: InMemoryKeychain! var dependencyProvider: LoginDependencyProviderMock! private var cancellables: Set! - + private var tacFactory: TermsAndConditionsViewFactory! var termsAndConditionsLink = "http://wallet-proxy.mainnet.concordium.software/v0/termsAndConditionsVersion" override func setUp() async throws { @@ -30,6 +30,9 @@ class LoginCoordinatorTests: XCTestCase { cancellables = [] dependencyProvider.keychainWrapperReturnValue = keychainMock dependencyProvider.storageManagerReturnValue = storageManagerMock + tacFactory = { response in + TermsAndConditionsViewModel(storageManager: self.storageManagerMock, termsAndConditions: response) + } dependencyProvider.mobileWalletReturnValue = MobileWalletProtocolMock() } @@ -52,7 +55,8 @@ class LoginCoordinatorTests: XCTestCase { sut = .init( navigationController: .init(), parentCoordinator: LoginCoordinatorDelegateMock(), - dependencyProvider: dependencyProvider + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: tacFactory ) sut.start() @@ -66,7 +70,7 @@ class LoginCoordinatorTests: XCTestCase { func test_start__password_set_and_tac_not_changed_should_display_login_view() { // given - keychainMock.storePassword(password: "anypass") + _ = keychainMock.storePassword(password: "anypass") storageManagerMock.latestTermsAndConditionsVersion = "1.0.0" let returnedResponse = TermsAndConditionsResponse(url: URL(string: termsAndConditionsLink)!, version: "1.0.0") appSettingsMock.getTermsAndConditionsVersionReturnValue = .just(returnedResponse) @@ -75,7 +79,8 @@ class LoginCoordinatorTests: XCTestCase { sut = .init( navigationController: .init(), parentCoordinator: LoginCoordinatorDelegateMock(), - dependencyProvider: dependencyProvider + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: tacFactory ) sut.start() @@ -86,7 +91,7 @@ class LoginCoordinatorTests: XCTestCase { } @MainActor - func test_start__password_not_set_terms_up_to_date_should_display_tac_view() { + func test_start__password_not_set_terms_up_to_date_should_display_enter_password_screen() { // given storageManagerMock.latestTermsAndConditionsVersion = "1.0.0" let returnedResponse = TermsAndConditionsResponse(url: URL(string: termsAndConditionsLink)!, version: "1.0.0") @@ -96,14 +101,15 @@ class LoginCoordinatorTests: XCTestCase { sut = .init( navigationController: .init(), parentCoordinator: LoginCoordinatorDelegateMock(), - dependencyProvider: dependencyProvider + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: tacFactory ) sut.start() // then XCTAssertTrue(appSettingsMock.getTermsAndConditionsVersionCalled) XCTAssertEqual(appSettingsMock.getTermsAndConditionsVersionCallsCount, 1) - XCTAssertTrue(sut.navigationController.topViewController is UIHostingController) + XCTAssertTrue(sut.navigationController.topViewController is InitialAccountInfoViewController) } @MainActor @@ -115,7 +121,8 @@ class LoginCoordinatorTests: XCTestCase { sut = .init( navigationController: .init(), parentCoordinator: LoginCoordinatorDelegateMock(), - dependencyProvider: dependencyProvider + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: tacFactory ) // when @@ -132,4 +139,81 @@ class LoginCoordinatorTests: XCTestCase { XCTAssertNotNil(encounteredError) XCTAssertEqual(encounteredError.localizedDescription, NetworkError.invalidResponse.localizedDescription) } + + @MainActor + func test_start__password_created_new_terms_and_conditions_accepted_should_display_login() { + let returnedResponse = TermsAndConditionsResponse(url: URL(string: termsAndConditionsLink)!, version: "1.0.1") + _ = keychainMock.storePassword(password: "anypass") + storageManagerMock.latestTermsAndConditionsVersion = "1.0.0" + appSettingsMock.getTermsAndConditionsVersionReturnValue = .just(returnedResponse) + let viewModel = TermsAndConditionsViewModel(storageManager: storageManagerMock, termsAndConditions: returnedResponse) + let factory: TermsAndConditionsViewFactory = { _ in viewModel } + + sut = .init( + navigationController: .init(), + parentCoordinator: LoginCoordinatorDelegateMock(), + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: factory + ) + + // when + sut.start() + viewModel.termsAndConditionsAccepted = true + viewModel.continueButtonTapped() + + // then + XCTAssertTrue(sut.navigationController.topViewController is EnterPasswordViewController) + } + + @MainActor + func test_start__password_created_no_new_terms_and_conditions_available_should_display_login() { + let returnedResponse = TermsAndConditionsResponse(url: URL(string: termsAndConditionsLink)!, version: "1.0.0") + _ = keychainMock.storePassword(password: "anypass") + storageManagerMock.latestTermsAndConditionsVersion = "1.0.0" + appSettingsMock.getTermsAndConditionsVersionReturnValue = .just(returnedResponse) + let viewModel = TermsAndConditionsViewModel(storageManager: storageManagerMock, termsAndConditions: returnedResponse) + let factory: TermsAndConditionsViewFactory = { _ in viewModel } + + sut = .init( + navigationController: .init(), + parentCoordinator: LoginCoordinatorDelegateMock(), + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: factory + ) + + // when + sut.start() + viewModel.termsAndConditionsAccepted = true + viewModel.continueButtonTapped() + + // then + XCTAssertTrue(sut.navigationController.topViewController is EnterPasswordViewController) + } + + @MainActor + func test_start__password_not_created_new_terms_and_conditions_accepted_should_display_initial_screen() { + // given + let returnedResponse = TermsAndConditionsResponse(url: URL(string: termsAndConditionsLink)!, version: "1.0.1") + storageManagerMock.latestTermsAndConditionsVersion = "1.0.0" + appSettingsMock.getTermsAndConditionsVersionReturnValue = .just(returnedResponse) + let viewModel = TermsAndConditionsViewModel(storageManager: storageManagerMock, termsAndConditions: returnedResponse) + let factory: TermsAndConditionsViewFactory = { _ in viewModel } + sut = .init( + navigationController: .init(), + parentCoordinator: LoginCoordinatorDelegateMock(), + dependencyProvider: dependencyProvider, + termsAndCondtionsFactory: factory + ) + + // when + sut.start() + viewModel.termsAndConditionsAccepted = true + viewModel.continueButtonTapped() + + // then + // Not sure why but delaying the assertion is necessary for the test to pass. + DispatchQueue.main.asyncAfter(deadline: .now() + 1) { + XCTAssertTrue(self.sut.navigationController.topViewController is InitialAccountInfoViewController) + } + } }