Skip to content

Commit

Permalink
[CBW-1031] fix app flow when new t&c available password is created (#318
Browse files Browse the repository at this point in the history 
)

## 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
  • Loading branch information
@milsawicki
milsawicki authored May 24, 2023
1 parent 19203b6 commit 34e2369
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 44 deletions.
47 changes: 26 additions & 21 deletions ConcordiumWallet/AppCoordinator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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] = []
Expand Down Expand Up @@ -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()
})
Expand Down Expand Up @@ -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,
Expand All @@ -169,32 +175,32 @@ class AppCoordinator: NSObject, Coordinator, ShowAlert, RequestPasswordDelegate
navigationController.setupBaseNavigationControllerStyle()
}
}

func showSeedIdentityCreation() {
let coordinator = SeedIdentitiesCoordinator(
navigationController: navigationController,
action: .createInitialIdentity,
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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -364,7 +370,6 @@ extension AppCoordinator: IdentitiesCoordinatorDelegate, MoreCoordinatorDelegate
}

extension AppCoordinator: AppSettingsDelegate {

func checkForAppSettings() {
guard needsAppCheck else { return }
needsAppCheck = false
Expand All @@ -379,7 +384,7 @@ extension AppCoordinator: AppSettingsDelegate {
)
.store(in: &cancellables)
}

private func handleAppSettings(response: AppSettingsResponse) {
showUpdateDialogIfNeeded(
appSettingsResponse: response
Expand All @@ -402,7 +407,7 @@ extension AppCoordinator: RecoveryPhraseCoordinatorDelegate {
showSeedIdentityCreation()
childCoordinators.removeAll { $0 is RecoveryPhraseCoordinator }
}

func recoveryPhraseCoordinatorFinishedRecovery() {
showMainTabbar()
childCoordinators.removeAll { $0 is RecoveryPhraseCoordinator }
Expand Down
43 changes: 28 additions & 15 deletions ConcordiumWallet/Views/Login/LoginCoordinator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@ protocol LoginCoordinatorDelegate: AppSettingsDelegate {
func passwordSelectionDone()
}

typealias TermsAndConditionsViewFactory = (TermsAndConditionsResponse) -> (TermsAndConditionsViewModel?)

class LoginCoordinator: Coordinator, ShowAlert {
var childCoordinators = [Coordinator]()

var navigationController: UINavigationController
let dependencyProvider: LoginDependencyProvider
private var cancellables: Set<AnyCancellable> = []
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
Expand Down Expand Up @@ -57,35 +65,40 @@ 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
switch completion {
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)
Expand Down
100 changes: 92 additions & 8 deletions ConcordiumWalletTests/Coordinators/LoginCoordinatorTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class LoginCoordinatorTests: XCTestCase {
var keychainMock: InMemoryKeychain!
var dependencyProvider: LoginDependencyProviderMock!
private var cancellables: Set<AnyCancellable>!

private var tacFactory: TermsAndConditionsViewFactory!
var termsAndConditionsLink = "http://wallet-proxy.mainnet.concordium.software/v0/termsAndConditionsVersion"

override func setUp() async throws {
Expand All @@ -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()
}

Expand All @@ -52,7 +55,8 @@ class LoginCoordinatorTests: XCTestCase {
sut = .init(
navigationController: .init(),
parentCoordinator: LoginCoordinatorDelegateMock(),
dependencyProvider: dependencyProvider
dependencyProvider: dependencyProvider,
termsAndCondtionsFactory: tacFactory
)
sut.start()

Expand All @@ -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)
Expand All @@ -75,7 +79,8 @@ class LoginCoordinatorTests: XCTestCase {
sut = .init(
navigationController: .init(),
parentCoordinator: LoginCoordinatorDelegateMock(),
dependencyProvider: dependencyProvider
dependencyProvider: dependencyProvider,
termsAndCondtionsFactory: tacFactory
)
sut.start()

Expand All @@ -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")
Expand All @@ -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<TermsAndConditionsView>)
XCTAssertTrue(sut.navigationController.topViewController is InitialAccountInfoViewController)
}

@MainActor
Expand All @@ -115,7 +121,8 @@ class LoginCoordinatorTests: XCTestCase {
sut = .init(
navigationController: .init(),
parentCoordinator: LoginCoordinatorDelegateMock(),
dependencyProvider: dependencyProvider
dependencyProvider: dependencyProvider,
termsAndCondtionsFactory: tacFactory
)

// when
Expand All @@ -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)
}
}
}

0 comments on commit 34e2369

Please sign in to comment.