From 4eacad55f4a2db6bba4cbaf3d005d46e3493fb4b Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Thu, 21 Nov 2024 22:08:07 -0300 Subject: [PATCH 01/13] PM-14800 Implemented CXP import logic PoC --- Bitwarden/Application/SceneDelegate.swift | 4 +- .../Extensions/JSONEncoder+Bitwarden.swift | 10 ++ .../Platform/Services/ServiceContainer.swift | 16 ++ .../Core/Platform/Services/Services.swift | 8 + .../Utilities/OSLogErrorReporter.swift | 2 +- .../Request/ImportCiphersRequestModel.swift | 9 +- .../ImportCiphersRepository.swift | 164 ++++++++++++++++++ .../API/ImportCiphersAPIService.swift | 4 +- .../API/Requests/ImportCiphersRequest.swift | 4 +- .../Tools/Services/ImportCiphersService.swift | 56 ++++++ .../Utilities/ImportedCredentialsResult.swift | 14 ++ .../Platform/Application/AppCoordinator.swift | 23 +++ .../Platform/Application/AppProcessor.swift | 31 ++-- .../UI/Platform/Application/AppRoute.swift | 2 + .../en.lproj/Localizable.strings | 9 + .../ImportCXP/ImportCXP/ImportCXPEffect.swift | 19 ++ .../ImportCXP/ImportCXPProcessor.swift | 104 +++++++++++ .../ImportCXP/ImportCXP/ImportCXPState.swift | 60 +++++++ .../ImportCXP/ImportCXP/ImportCXPView.swift | 124 +++++++++++++ .../ImportCXP/ImportCXPCoordinator.swift | 68 ++++++++ .../UI/Tools/ImportCXP/ImportCXPModule.swift | 27 +++ .../UI/Tools/ImportCXP/ImportCXPRoute.swift | 13 ++ .../UI/Vault/Extensions/Alert+Vault.swift | 14 ++ .../UI/Vault/Vault/VaultCoordinator.swift | 18 ++ .../UI/Vault/Vault/VaultRoute.swift | 3 + 25 files changed, 787 insertions(+), 19 deletions(-) create mode 100644 BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift create mode 100644 BitwardenShared/Core/Tools/Services/ImportCiphersService.swift create mode 100644 BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXPCoordinator.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXPModule.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXPRoute.swift diff --git a/Bitwarden/Application/SceneDelegate.swift b/Bitwarden/Application/SceneDelegate.swift index c8597368c..86e98298b 100644 --- a/Bitwarden/Application/SceneDelegate.swift +++ b/Bitwarden/Application/SceneDelegate.swift @@ -96,7 +96,9 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { return } - appProcessor.handleImportCredentials(credentialImportToken: token) + Task { + await appProcessor.handleImportCredentials(credentialImportToken: token) + } } #endif diff --git a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift index 4ece78d43..9e7f48301 100644 --- a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift +++ b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift @@ -15,4 +15,14 @@ extension JSONEncoder { } return jsonEncoder }() + + /// The default `JSONEncoder` used to encode JSON payloads when in Credential Exchange flow. + static let cxpEncoder: JSONEncoder = { + let jsonEncoder = JSONEncoder() + jsonEncoder.dateEncodingStrategy = .custom { date, encoder in + var container = encoder.singleValueContainer() + try container.encode(Int(date.timeIntervalSince1970)) + } + return jsonEncoder + }() } diff --git a/BitwardenShared/Core/Platform/Services/ServiceContainer.swift b/BitwardenShared/Core/Platform/Services/ServiceContainer.swift index 8bdac9670..4a783bf58 100644 --- a/BitwardenShared/Core/Platform/Services/ServiceContainer.swift +++ b/BitwardenShared/Core/Platform/Services/ServiceContainer.swift @@ -82,6 +82,9 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le /// The repository used by the application to manage generator data for the UI layer. let generatorRepository: GeneratorRepository + /// The repository used by the application to manage importing credential in Credential Exhange flow. + let importCiphersRepository: ImportCiphersRepository + /// The service used to access & store data on the device keychain. let keychainService: KeychainService @@ -183,6 +186,8 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le /// and extends the capabilities of the `Fido2UserInterface` from the SDK. /// - fido2CredentialStore: A store to be used on Fido2 flows to get/save credentials. /// - generatorRepository: The repository used by the application to manage generator data for the UI layer. + /// - importCiphersRepository: The repository used by the application to manage importing credential + /// in Credential Exhange flow. /// - keychainRepository: The repository used to manages keychain items. /// - keychainService: The service used to access & store data on the device keychain. /// - localAuthService: The service used by the application to evaluate local auth policies. @@ -230,6 +235,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le fido2CredentialStore: Fido2CredentialStore, fido2UserInterfaceHelper: Fido2UserInterfaceHelper, generatorRepository: GeneratorRepository, + importCiphersRepository: ImportCiphersRepository, keychainRepository: KeychainRepository, keychainService: KeychainService, localAuthService: LocalAuthService, @@ -276,6 +282,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le self.fido2CredentialStore = fido2CredentialStore self.fido2UserInterfaceHelper = fido2UserInterfaceHelper self.generatorRepository = generatorRepository + self.importCiphersRepository = importCiphersRepository self.keychainService = keychainService self.keychainRepository = keychainRepository self.localAuthService = localAuthService @@ -635,6 +642,14 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le vaultTimeoutService: vaultTimeoutService ) + let importCiphersRepository = DefaultImportCiphersRepository( + clientService: clientService, + importCiphersService: DefaultImportCiphersService( + importCiphersAPIService: apiService + ), + syncService: syncService + ) + let authenticatorDataStore = AuthenticatorBridgeDataStore( errorReporter: errorReporter, groupIdentifier: Bundle.main.sharedAppGroupIdentifier, @@ -692,6 +707,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le fido2CredentialStore: fido2CredentialStore, fido2UserInterfaceHelper: fido2UserInterfaceHelper, generatorRepository: generatorRepository, + importCiphersRepository: importCiphersRepository, keychainRepository: keychainRepository, keychainService: keychainService, localAuthService: localAuthService, diff --git a/BitwardenShared/Core/Platform/Services/Services.swift b/BitwardenShared/Core/Platform/Services/Services.swift index bf59fea30..f2d0d2e7f 100644 --- a/BitwardenShared/Core/Platform/Services/Services.swift +++ b/BitwardenShared/Core/Platform/Services/Services.swift @@ -24,6 +24,7 @@ typealias Services = HasAPIService & HasFido2UserInterfaceHelper & HasFileAPIService & HasGeneratorRepository + & HasImportCiphersRepository & HasLocalAuthService & HasNFCReaderService & HasNotificationCenterService @@ -207,6 +208,13 @@ protocol HasGeneratorRepository { var generatorRepository: GeneratorRepository { get } } +/// Protocol for an object that provides a `ImportCiphersRepository`. +/// +protocol HasImportCiphersRepository { + /// The repository used by the application to manage importing credential in Credential Exhange flow. + var importCiphersRepository: ImportCiphersRepository { get } +} + /// Protocol for an object that provides a `LocalAuthService`. /// protocol HasLocalAuthService { diff --git a/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift b/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift index 6d1843bb2..2442a4743 100644 --- a/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift +++ b/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift @@ -27,7 +27,7 @@ public final class OSLogErrorReporter: ErrorReporter { guard !error.isNetworkingError else { return } // Crash in debug builds to make the error more visible during development. - assertionFailure("Unexpected error: \(error)") +// assertionFailure("Unexpected error: \(error)") } public func setRegion(_ region: String, isPreAuth: Bool) { diff --git a/BitwardenShared/Core/Tools/Models/Request/ImportCiphersRequestModel.swift b/BitwardenShared/Core/Tools/Models/Request/ImportCiphersRequestModel.swift index 55d0f18b4..e38cdb318 100644 --- a/BitwardenShared/Core/Tools/Models/Request/ImportCiphersRequestModel.swift +++ b/BitwardenShared/Core/Tools/Models/Request/ImportCiphersRequestModel.swift @@ -16,5 +16,12 @@ struct ImportCiphersRequestModel: JSONRequestBody { /// The cipher<->folder relationships map. The key is the cipher index and the value is the folder index /// in their respective arrays. - var folderRelationships: [Int: Int] + var folderRelationships: [FolderRelationship] +} + +/// The cipher<->folder relationships map. The key is the cipher index and the value is the folder index +/// in their respective arrays. +struct FolderRelationship: Codable { + let key: Int + let value: Int } diff --git a/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift new file mode 100644 index 000000000..c808b8c72 --- /dev/null +++ b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift @@ -0,0 +1,164 @@ +import AuthenticationServices +import BitwardenSdk +import Foundation + +/// A protocol for a `ImportCiphersRepository` which manages importing credentials needed by the UI layer. +/// +protocol ImportCiphersRepository: AnyObject { + /// Performs an API request to import ciphers in the vault. + /// - Parameters: + /// - credentialImportToken: The token used in `ASCredentialImportManager` to get the credentials to import. + /// - Returns: A dictionary containing the localized cipher type (key) and count (value) of that type + /// that was imported, e.g. ["Passwords": 3, "Cards": 2]. + @available(iOS 18.2, *) + func importCiphers(credentialImportToken: UUID) async throws -> [ImportedCredentialsResult] +} + +// MARK: - DefaultImportCiphersRepository + +/// A default implementation of a `ImportCiphersRepository`. +/// +class DefaultImportCiphersRepository { + // MARK: Properties + + /// The service that handles common client functionality such as encryption and decryption. + let clientService: ClientService + + /// The service that manages importing credentials. + let importCiphersService: ImportCiphersService + + /// The service used to handle syncing vault data with the API. + let syncService: SyncService + + // MARK: Initialization + + /// Initialize a `DefaultImportCiphersRepository` + /// + /// - Parameters: + /// - clientService: The service that handles common client functionality such as encryption and decryption. + /// - importCiphersService: A service that manages importing credentials. + /// - syncService: The service used to handle syncing vault data with the API. + /// + init( + clientService: ClientService, + importCiphersService: ImportCiphersService, + syncService: SyncService + ) { + self.clientService = clientService + self.importCiphersService = importCiphersService + self.syncService = syncService + } +} + +// MARK: ImportCiphersRepository + +extension DefaultImportCiphersRepository: ImportCiphersRepository { + @available(iOS 18.2, *) + func importCiphers( // swiftlint:disable:this function_body_length + credentialImportToken: UUID + ) async throws -> [ImportedCredentialsResult] { + #if compiler(>=6.0.3) + + let credentialData = try await ASCredentialImportManager().importCredentials(token: credentialImportToken) + guard let accountData = credentialData.accounts.first else { + // this should never happen. + throw ImportCiphersRepositoryError.noDataFound + } + + let accountJsonData = try JSONEncoder.cxpEncoder.encode(accountData) + guard let accountJsonString = String(data: accountJsonData, encoding: .utf8) else { + // this should never happen. + throw ImportCiphersRepositoryError.dataEncodingFailed + } + + let ciphers = try await clientService.exporters().importCxf(payload: accountJsonString) + + _ = try await importCiphersService + .importCiphers( + ciphers: ciphers, + folders: [], + folderRelationships: [] + ) + + try await syncService.fetchSync(forceSync: true) + + var importedCredentialsCount: [ImportedCredentialsResult] = [] + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.passwords, + when: { cipher in + cipher.type == .login && cipher.login?.fido2Credentials?.isEmpty != false + } + ) + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.passkeys, + when: { cipher in + cipher.type == .login && cipher.login?.fido2Credentials?.isEmpty == false + } + ) + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.cards, + when: { $0.type == .card } + ) + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.identities, + when: { $0.type == .identity } + ) + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.secureNotes, + when: { $0.type == .secureNote } + ) + appendImportedCredentialCountIfAny( + importedCredentialsCount: &importedCredentialsCount, + ciphers: ciphers, + localizedType: Localizations.sshKeys, + when: { $0.type == .sshKey } + ) + return importedCredentialsCount + #else + return [] + #endif + } + + // MARK: Private + + /// Appends imported credential count when the condition is true for the count. + /// - Parameters: + /// - importedCredentialsCount: The array to update. + /// - ciphers: The ciphers to count. + /// - localizedType: The localized type to add if the condition is met. + /// - when: The filter to apply to count the ciphers. + private func appendImportedCredentialCountIfAny( + importedCredentialsCount: inout [ImportedCredentialsResult], + ciphers: [Cipher], + localizedType: String, + when: (Cipher) -> Bool + ) { + let count = ciphers.count { when($0) } + if count > 0 { // swiftlint:disable:this empty_count + importedCredentialsCount + .append( + ImportedCredentialsResult( + localizedType: localizedType, + count: count + ) + ) + } + } +} + +// MARK: - ImportCiphersRepositoryError + +enum ImportCiphersRepositoryError: Error { + case noDataFound + case dataEncodingFailed +} diff --git a/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIService.swift b/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIService.swift index db8165d2b..452045970 100644 --- a/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIService.swift +++ b/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIService.swift @@ -15,7 +15,7 @@ protocol ImportCiphersAPIService { func importCiphers( ciphers: [Cipher], folders: [Folder], - folderRelationships: [Int: Int] + folderRelationships: [(key: Int, value: Int)] ) async throws -> EmptyResponse } @@ -23,7 +23,7 @@ extension APIService: ImportCiphersAPIService { func importCiphers( ciphers: [Cipher], folders: [Folder], - folderRelationships: [Int: Int] + folderRelationships: [(key: Int, value: Int)] ) async throws -> EmptyResponse { try await apiService .send( diff --git a/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequest.swift b/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequest.swift index 5e422effa..b284177f2 100644 --- a/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequest.swift +++ b/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequest.swift @@ -33,7 +33,7 @@ struct ImportCiphersRequest: Request { init( ciphers: [Cipher], folders: [Folder] = [], - folderRelationships: [Int: Int] = [:] + folderRelationships: [(key: Int, value: Int)] = [] ) throws { guard !ciphers.isEmpty else { throw BitwardenError.dataError("There are no ciphers to import.") @@ -42,7 +42,7 @@ struct ImportCiphersRequest: Request { requestModel = ImportCiphersRequestModel( ciphers: ciphers.map { CipherRequestModel(cipher: $0) }, folders: folders.map { FolderWithIdRequestModel(folder: $0) }, - folderRelationships: folderRelationships + folderRelationships: folderRelationships.map { FolderRelationship(key: $0.key, value: $0.value) } ) } } diff --git a/BitwardenShared/Core/Tools/Services/ImportCiphersService.swift b/BitwardenShared/Core/Tools/Services/ImportCiphersService.swift new file mode 100644 index 000000000..09971e581 --- /dev/null +++ b/BitwardenShared/Core/Tools/Services/ImportCiphersService.swift @@ -0,0 +1,56 @@ +import BitwardenSdk +import Combine +import Foundation + +// MARK: - ImportCiphersService + +/// A protocol for a `ImportCiphersService` which manages importing credentials. +/// +protocol ImportCiphersService { + /// Performs an API request to import ciphers in the vault. + /// - Parameters: + /// - ciphers: The ciphers to import. + /// - folders: The folders to import. + /// - folderRelationships: The cipher<->folder relationships map. The key is the cipher index + /// and the value is the folder index in their respective arrays. + func importCiphers( + ciphers: [Cipher], + folders: [Folder], + folderRelationships: [(key: Int, value: Int)] + ) async throws +} + +// MARK: - DefaultImportCiphersService + +class DefaultImportCiphersService: ImportCiphersService { + // MARK: Properties + + /// The service used to make import ciphers related API requests. + private let importCiphersAPIService: ImportCiphersAPIService + + // MARK: Initialization + + /// Initialize a `DefaultCipherService`. + /// + /// - Parameters: + /// - importCiphersAPIService: The service used to make import ciphers related API requests. + /// + init(importCiphersAPIService: ImportCiphersAPIService) { + self.importCiphersAPIService = importCiphersAPIService + } +} + +extension DefaultImportCiphersService { + func importCiphers( + ciphers: [Cipher], + folders: [Folder], + folderRelationships: [(key: Int, value: Int)] + ) async throws { + _ = try await importCiphersAPIService + .importCiphers( + ciphers: ciphers, + folders: folders, + folderRelationships: folderRelationships + ) + } +} diff --git a/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift new file mode 100644 index 000000000..3d4f2e588 --- /dev/null +++ b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift @@ -0,0 +1,14 @@ +/// Represents the result of imported credentials of one type. +struct ImportedCredentialsResult: Equatable, Sendable { + /// The credential type imported. + let localizedType: String + + /// The number of credentials imported for the type + let count: Int +} + +extension ImportedCredentialsResult: Identifiable { + public var id: String { + localizedType + } +} diff --git a/BitwardenShared/UI/Platform/Application/AppCoordinator.swift b/BitwardenShared/UI/Platform/Application/AppCoordinator.swift index 6ac66c5d6..2ee307f6a 100644 --- a/BitwardenShared/UI/Platform/Application/AppCoordinator.swift +++ b/BitwardenShared/UI/Platform/Application/AppCoordinator.swift @@ -14,6 +14,7 @@ class AppCoordinator: Coordinator, HasRootNavigator { & DebugMenuModule & ExtensionSetupModule & FileSelectionModule + & ImportCXPModule & LoginRequestModule & SendItemModule & TabModule @@ -102,6 +103,8 @@ class AppCoordinator: Coordinator, HasRootNavigator { showDebugMenu() case let .extensionSetup(extensionSetupRoute): showExtensionSetup(route: extensionSetupRoute) +// case let .importCXP(route): +// showImportCXP(route: route) case let .loginRequest(loginRequest): showLoginRequest(loginRequest) case let .sendItem(sendItemRoute): @@ -188,6 +191,26 @@ class AppCoordinator: Coordinator, HasRootNavigator { } } +// /// Shows the Credential Exchange import route (not in a tab). This is used when another app +// /// exporting credentials with Credential Exchange protocol chooses our app as a provider to import credentials. +// /// +// /// - Parameter route: The `ImportCXPRoute` to show. +// /// +// private func showImportCXP(route: ImportCXPRoute) { +// if let coordinator = childCoordinator as? AnyCoordinator { +// coordinator.navigate(to: route) +// } else { +// let stackNavigator = UINavigationController() +// let coordinator = module.makeImportCXPCoordinator( +// stackNavigator: stackNavigator +// ) +// coordinator.start() +// coordinator.navigate(to: route) +// childCoordinator = coordinator +// rootNavigator?.show(child: stackNavigator) +// } +// } + /// Shows the send item route (not in a tab). This is used within the app extensions. /// /// - Parameter route: The `SendItemRoute` to show. diff --git a/BitwardenShared/UI/Platform/Application/AppProcessor.swift b/BitwardenShared/UI/Platform/Application/AppProcessor.swift index 895210174..7e2013d77 100644 --- a/BitwardenShared/UI/Platform/Application/AppProcessor.swift +++ b/BitwardenShared/UI/Platform/Application/AppProcessor.swift @@ -121,14 +121,7 @@ public class AppProcessor { route = await getOtpAuthUrlRoute(url: url) } guard let route else { return } - - if let userId = try? await services.stateService.getActiveAccountId(), - !services.vaultTimeoutService.isLocked(userId: userId), - await (try? services.vaultTimeoutService.hasPassedSessionTimeout(userId: userId)) == false { - coordinator?.navigate(to: route) - } else { - await coordinator?.handleEvent(.setAuthCompletionRoute(route)) - } + await checkIfLockedAndPerformNavigation(route: route) } /// Starts the application flow by navigating the user to the first flow. @@ -214,10 +207,11 @@ public class AppProcessor { /// Handles importing credentials using Credential Exchange Protocol. /// - Parameter credentialImportToken: The credentials import token to user with the `ASCredentialImportManager`. @available(iOSApplicationExtension 18.2, *) - public func handleImportCredentials(credentialImportToken: UUID) { - // TODO: PM-14800 Move this to a specific view to handle importing process - // and handle credential data. - // let credentialData = try await ASCredentialImportManager().importCredentials(token: credentialImportToken) + public func handleImportCredentials(credentialImportToken: UUID) async { + let route = AppRoute.tab(.vault(.importCXP( + .importCredentials(credentialImportToken: credentialImportToken) + ))) + await checkIfLockedAndPerformNavigation(route: route) } // MARK: Autofill Methods @@ -398,6 +392,19 @@ extension AppProcessor { } } + /// Checks if the vault is locked and performs the navigation to the `AppRoute` + /// or sets it as the auth completion route. + /// - Parameter route: The `AppRoute` to go to. + private func checkIfLockedAndPerformNavigation(route: AppRoute) async { + if let userId = try? await services.stateService.getActiveAccountId(), + !services.vaultTimeoutService.isLocked(userId: userId), + await (try? services.vaultTimeoutService.hasPassedSessionTimeout(userId: userId)) == false { + coordinator?.navigate(to: route) + } else { + await coordinator?.handleEvent(.setAuthCompletionRoute(route)) + } + } + /// If the native create account feature flag and the autofill extension are enabled, this marks /// any user's autofill account setup completed. This should be called on app startup. /// diff --git a/BitwardenShared/UI/Platform/Application/AppRoute.swift b/BitwardenShared/UI/Platform/Application/AppRoute.swift index 1c9acdf43..4a6c98575 100644 --- a/BitwardenShared/UI/Platform/Application/AppRoute.swift +++ b/BitwardenShared/UI/Platform/Application/AppRoute.swift @@ -1,3 +1,5 @@ +import Foundation + // MARK: - AppRoute /// A top level route from the initial screen of the app to anywhere in the app. diff --git a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings index e1c161a12..2d547a7bd 100644 --- a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings +++ b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings @@ -1061,3 +1061,12 @@ "SSHKeys" = "SSH keys"; "ExportingFailed" = "Exporting failed"; "YouMayNeedToEnableDevicePasscodeOrBiometrics" = "You may need to enable device passcode or biometrics."; +"StartImportCXPDescriptionLong" = "Import passwords, passkeys, credit cards, and any personal identity information from another password manager.\n\nYour data will not be deleted from your previous provider."; +"ImportPasswords" = "Import passwords"; +"ImportingEllipsis" = "Importing..."; +"AreYouSureYouWantToCancelTheImportProcessQuestionMark" = "Are you sure you want to cancel the import process?"; +"ImportFailed" = "Import failed"; +"ItemsSuccessfullyImported" = "%1$@ items successfully imported"; +"ThereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted" = "There was an issue importing all of your passwords.\n\nNo data was deleted."; +"RetryImport" = "Retry import"; +"ShowVault" = "Show vault"; diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift new file mode 100644 index 000000000..f314542c6 --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift @@ -0,0 +1,19 @@ +import Foundation + +// MARK: - ImportCXPEffect + +/// Effects that can be processed by a `ImportCXPProcessor`. +/// +enum ImportCXPEffect: Equatable { + /// The view appeared. + case appeared + + /// User wants to cancel the import process. + case cancel + + /// Shows the vault after finishing the import process. + case showVault + + /// User pressed button to start import process. + case startImport +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift new file mode 100644 index 000000000..46327d391 --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -0,0 +1,104 @@ +import AuthenticationServices + +// MARK: - ImportCXPProcessor + +/// The processor used to manage state and handle actions/effects for the Credential Exchange import screen. +/// +class ImportCXPProcessor: StateProcessor { + // MARK: Types + + typealias Services = HasConfigService + & HasErrorReporter + & HasImportCiphersRepository + & HasStateService + + // MARK: Private Properties + + /// The coordinator that handles navigation. + private let coordinator: AnyCoordinator + + /// The services used by this processor. + private let services: Services + + // MARK: Initialization + + /// Creates a new `ImportCXPProcessor`. + /// + /// - Parameters: + /// - coordinator: The coordinator that handles navigation. + /// - services: The services used by the processor. + /// - state: The initial state of the processor. + /// + init( + coordinator: AnyCoordinator, + services: Services, + state: ImportCXPState + ) { + self.coordinator = coordinator + self.services = services + super.init(state: state) + } + + // MARK: Methods + + override func perform(_ effect: ImportCXPEffect) async { + switch effect { + case .appeared: + break + case .cancel: + cancelWithConfirmation() + case .showVault: + coordinator.navigate(to: .dismiss) + case .startImport: + await startImport() + } + } + + // MARK: Private + + /// Starts the import process. + private func startImport() async { + #if compiler(>=6.0.3) + + guard #available(iOS 18.2, *), let credentialImportToken = state.credentialImportToken else { + coordinator.showAlert( + .defaultAlert( + title: Localizations.importError, + message: Localizations.featureUnavailable + ) + ) + return + } + + state.status = .importing + + do { + let result = try await services.importCiphersRepository.importCiphers( + credentialImportToken: credentialImportToken + ) + + state.status = .success( + totalImportedCredentials: result.map(\.count).reduce(0, +), + credentialsByTypeCount: result + ) + } catch ImportCiphersRepositoryError.noDataFound { + state.status = .failure(message: "No data found to import.") + } catch ImportCiphersRepositoryError.dataEncodingFailed { + state.status = .failure(message: "Import data encoding failed.") + } catch { + state.status = .failure(message: Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) + services.errorReporter.log(error: error) + } + + #endif + } + + /// Shows the alert confirming the user wants to import logins later. + /// + private func cancelWithConfirmation() { + coordinator.showAlert(.confirmCancelCXPImport { [weak self] in + guard let self else { return } + coordinator.navigate(to: .dismiss) + }) + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift new file mode 100644 index 000000000..b960d2194 --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -0,0 +1,60 @@ +import Foundation + +// MARK: - ImportCXPState + +/// The state used to present the `ImportCXPView`. +/// +struct ImportCXPState: Equatable, Sendable { + // MARK: Types + + /// The status of the import process. + enum ImportCXPStatus: Equatable, Sendable { + /// The import flow is at the start point. + case start + + /// The import flow is in progress. + case importing + + /// The import flow succeded. + case success(totalImportedCredentials: Int, credentialsByTypeCount: [ImportedCredentialsResult]) + + /// The import flow failed. + case failure(message: String) + } + + // MARK: Properties + + /// The token used in `ASCredentialImportManager` to get the credentials to import. + var credentialImportToken: UUID? + + /// The message to display on the page header. + var message: String { + return switch status { + case .start: + Localizations.startImportCXPDescriptionLong + case .importing: + "" + case let .success(total, _): + Localizations.itemsSuccessfullyImported(total) + case let .failure(message): + message + } + } + + /// The title to display on the page header. + var title: String { + return switch status { + case .start: + Localizations.importPasswords + case .importing: + Localizations.importingEllipsis + case .success: + Localizations.importSuccessful + case .failure: + Localizations.importFailed + } + } + + /// The current status of the import process. + var status: ImportCXPStatus = .start +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift new file mode 100644 index 000000000..a5e380fbb --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -0,0 +1,124 @@ +import SwiftUI + +// MARK: - ImportCXPView + +/// A view to import credentials in the Credential Exchange protocol flow. +/// +struct ImportCXPView: View { + // MARK: Properties + + /// The `Store` for this view. + @ObservedObject var store: Store + + // MARK: View + + var body: some View { + VStack(spacing: 16) { + PageHeaderView( + image: Asset.Images.Illustrations.import, + title: store.state.title, + message: store.state.message + ) + switch store.state.status { + case .start: + Spacer() + AsyncButton(Localizations.continue) { + await store.perform(.startImport) + } + .buttonStyle(.primary()) + AsyncButton(Localizations.cancel) { + await store.perform(.cancel) + } + .buttonStyle(.secondary()) + case .importing: + ProgressView() + .frame(maxWidth: .infinity) + case let .success(_, countByType): + VStack(spacing: 8) { + ForEach(countByType) { type in + HStack { + Text(type.localizedType) + Spacer() + Text("\(type.count)") + } + } + } + Spacer() + AsyncButton(Localizations.showVault) { + await store.perform(.showVault) + } + case .failure: + Spacer() + AsyncButton(Localizations.retryImport) { + await store.perform(.startImport) + } + .buttonStyle(.primary()) + AsyncButton(Localizations.cancel) { + await store.perform(.cancel) + } + .buttonStyle(.secondary()) + } + } + .padding(.top, 8) + .transition(.opacity) +// .animation(.easeInOut, value: store.state.page) + .navigationBar(title: Localizations.importPasswords, titleDisplayMode: .inline) +// .toolbar { +// cancelToolbarItem { +// store.send(.dismiss) +// } +// } + .task { + await store.perform(.appeared) + } + } + + // MARK: Private +} + +// MARK: - Previews + +#if DEBUG +#Preview("Start") { + ImportCXPView(store: Store(processor: StateProcessor(state: ImportCXPState()))) + .navStackWrapped +} + +#Preview("Importing") { + ImportCXPView(store: Store(processor: StateProcessor(state: ImportCXPState(status: .importing)))) + .navStackWrapped +} + +#Preview("Success") { + ImportCXPView( + store: Store( + processor: StateProcessor( + state: ImportCXPState( + status: .success( + totalImportedCredentials: 30, + credentialsByTypeCount: [ + ImportedCredentialsResult(localizedType: "Passwords", count: 13), + ImportedCredentialsResult(localizedType: "Passkeys", count: 7), + ImportedCredentialsResult(localizedType: "Cards", count: 10), + ] + ) + ) + ) + ) + ).navStackWrapped +} + +#Preview("Failure") { + ImportCXPView( + store: Store( + processor: StateProcessor( + state: ImportCXPState( + status: .failure( + message: "Something went wrong" + ) + ) + ) + ) + ).navStackWrapped +} +#endif diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXPCoordinator.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPCoordinator.swift new file mode 100644 index 000000000..2b7443461 --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPCoordinator.swift @@ -0,0 +1,68 @@ +import Foundation + +/// A coordinator that manages navigation for the Credential Exchange import flow. +/// +class ImportCXPCoordinator: Coordinator, HasStackNavigator { + // MARK: Types + + typealias Services = HasConfigService + & HasErrorReporter + & HasImportCiphersRepository + & HasStateService + + // MARK: Private Properties + + /// The services used by this coordinator. + private let services: Services + + // MARK: Properties + + /// The stack navigator that is managed by this coordinator. + private(set) weak var stackNavigator: StackNavigator? + + // MARK: Initialization + + /// Creates a new `ImportCoordinator`. + /// + /// - Parameters: + /// - services: The services used by this coordinator. + /// - stackNavigator: The stack navigator that is managed by this coordinator. + /// + init( + services: Services, + stackNavigator: StackNavigator + ) { + self.services = services + self.stackNavigator = stackNavigator + } + + // MARK: Methods + + func navigate( + to route: ImportCXPRoute, + context: AnyObject? + ) { + switch route { + case .dismiss: + stackNavigator?.dismiss() + case let .importCredentials(credentialImportToken): + showImportCXP(credentialImportToken: credentialImportToken) + } + } + + func start() {} + + // MARK: Private Methods + + /// Configures and displays the Credential Exchange import view. + private func showImportCXP(credentialImportToken: UUID) { + let processor = ImportCXPProcessor( + coordinator: asAnyCoordinator(), + services: services, + state: ImportCXPState(credentialImportToken: credentialImportToken) + ) + + let view = ImportCXPView(store: Store(processor: processor)) + stackNavigator?.replace(view) + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXPModule.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPModule.swift new file mode 100644 index 000000000..3257199dd --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPModule.swift @@ -0,0 +1,27 @@ +// MARK: - ImportCXPModule + +/// An object that builds coordinators for the Credential Exchange import flow. +/// +@MainActor +protocol ImportCXPModule { + /// Initializes a coordinator for navigating between `ImportCXPRoute`s. + /// + /// - Parameters: + /// - stackNavigator: The stack navigator that will be used to navigate between routes. + /// - Returns: A coordinator that can navigate to `ImportCXPRoute`s. + /// + func makeImportCXPCoordinator( + stackNavigator: StackNavigator + ) -> AnyCoordinator +} + +extension DefaultAppModule: ImportCXPModule { + func makeImportCXPCoordinator( + stackNavigator: StackNavigator + ) -> AnyCoordinator { + ImportCXPCoordinator( + services: services, + stackNavigator: stackNavigator + ).asAnyCoordinator() + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXPRoute.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPRoute.swift new file mode 100644 index 000000000..a129e962d --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXPRoute.swift @@ -0,0 +1,13 @@ +import Foundation + +// MARK: - ImportCXPRoute + +/// A route to specific screens in the Credential Exhange import flow. +public enum ImportCXPRoute: Equatable, Hashable { + /// A route to dismiss the screen currently presented modally. + case dismiss + + /// A route to begin importing using Credential Exchange protocol. + /// - Parameter: The `credentialImportToken` to use in the import manager. + case importCredentials(credentialImportToken: UUID) +} diff --git a/BitwardenShared/UI/Vault/Extensions/Alert+Vault.swift b/BitwardenShared/UI/Vault/Extensions/Alert+Vault.swift index bffba53ab..ef8070dc8 100644 --- a/BitwardenShared/UI/Vault/Extensions/Alert+Vault.swift +++ b/BitwardenShared/UI/Vault/Extensions/Alert+Vault.swift @@ -4,6 +4,20 @@ import UIKit // MARK: - Alert+Vault extension Alert { + /// Returns an alert confirming cancelling the CXP import process. + /// - Parameter action: The action to perform if the user confirms. + /// - Returns: An alert confirming cancelling the CXP import process. + static func confirmCancelCXPImport(action: @escaping () async -> Void) -> Alert { + Alert( + title: Localizations.cancel, + message: Localizations.areYouSureYouWantToCancelTheImportProcessQuestionMark, + alertActions: [ + AlertAction(title: Localizations.yes, style: .default) { _, _ in await action() }, + AlertAction(title: Localizations.no, style: .cancel), + ] + ) + } + /// Returns an alert confirming whether to clone an item without the FIDO2 credential. /// /// - Parameter action: The action to perform if the user confirms. diff --git a/BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift b/BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift index 5a1b8ed4c..df5a0ff9d 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift @@ -58,6 +58,7 @@ final class VaultCoordinator: Coordinator, HasStackNavigator { // MARK: Types typealias Module = GeneratorModule + & ImportCXPModule & ImportLoginsModule & VaultItemModule @@ -186,6 +187,8 @@ final class VaultCoordinator: Coordinator, HasStackNavigator { stackNavigator?.dismiss() case let .group(group, filter): showGroup(group, filter: filter) + case let .importCXP(cxpRoute): + showImportCXP(route: cxpRoute) case .importLogins: showImportLogins() case .list: @@ -259,6 +262,21 @@ final class VaultCoordinator: Coordinator, HasStackNavigator { ) } + /// Shows the Credential Exchange import route (not in a tab). This is used when another app + /// exporting credentials with Credential Exchange protocol chooses our app as a provider to import credentials. + /// + /// - Parameter route: The `ImportCXPRoute` to show. + /// + private func showImportCXP(route: ImportCXPRoute) { + let navigationController = UINavigationController() + let coordinator = module.makeImportCXPCoordinator( + stackNavigator: navigationController + ) + coordinator.start() + coordinator.navigate(to: route) + stackNavigator?.present(navigationController) + } + /// Shows the import login items screen. /// private func showImportLogins() { diff --git a/BitwardenShared/UI/Vault/Vault/VaultRoute.swift b/BitwardenShared/UI/Vault/Vault/VaultRoute.swift index f0daa7003..7102dec30 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultRoute.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultRoute.swift @@ -42,6 +42,9 @@ public enum VaultRoute: Equatable, Hashable { /// A route to the vault item list screen for the specified group. case group(_ group: VaultListGroup, filter: VaultFilterType) + /// A route to the Credential Exchange import flow with the CXP specific route as a parameter. + case importCXP(ImportCXPRoute) + /// A route to the import logins screen. case importLogins From f63dd34b733c44a2ff61b2090af51cfef2e570f8 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 22 Nov 2024 11:20:11 +0100 Subject: [PATCH 02/13] Dirty fix for converting ID to id. --- .../Extensions/JSONEncoder+Bitwarden.swift | 50 +++++++++++++++++++ .../ImportCXP/ImportCXPProcessor.swift | 4 ++ 2 files changed, 54 insertions(+) diff --git a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift index 9e7f48301..b7cefa052 100644 --- a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift +++ b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift @@ -3,6 +3,23 @@ import Foundation extension JSONEncoder { // MARK: Static Properties + /// `AnyKey` is a `CodingKey` type that can be used for encoding and decoding keys for custom + /// key decoding strategies. + struct AnyKey: CodingKey { + let stringValue: String + let intValue: Int? + + init(stringValue: String) { + self.stringValue = stringValue + intValue = nil + } + + init(intValue: Int) { + stringValue = String(intValue) + self.intValue = intValue + } + } + /// The default `JSONEncoder` used to encode JSON payloads throughout the app. static let defaultEncoder: JSONEncoder = { let dateFormatterWithFractionalSeconds = ISO8601DateFormatter() @@ -23,6 +40,39 @@ extension JSONEncoder { var container = encoder.singleValueContainer() try container.encode(Int(date.timeIntervalSince1970)) } + jsonEncoder.keyEncodingStrategy = .custom { keys in + let key = keys.last!.stringValue + return AnyKey(stringValue: customTransformCodingKeyForCXP(key: key)) + } return jsonEncoder }() + + // MARK: Static Functions + + /// Transforms the keys from CXP format handled by the Bitwarden SDK into the keys that Apple expects. + static func customTransformCodingKeyForCXP(key: String) -> String { + return switch key { + case "credentialID": + "credentialId" + case "rpID": + "rpId" + default: + key + } + } + + /// Transforms a snake_case, PascalCase or camelCase key into camelCase. + static func keyToCamelCase(key: String) -> String { + if key.contains("_") { + // Handle snake_case. + return key.lowercased() + .split(separator: "_") + .enumerated() + .map { $0.offset > 0 ? $0.element.capitalized : $0.element.lowercased() } + .joined() + } + + // Handle PascalCase or camelCase. + return key.prefix(1).lowercased() + key.dropFirst() + } } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index 46327d391..104f27e1d 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -1,4 +1,5 @@ import AuthenticationServices +import BitwardenSdk // MARK: - ImportCXPProcessor @@ -85,6 +86,9 @@ class ImportCXPProcessor: StateProcessor state.status = .failure(message: "No data found to import.") } catch ImportCiphersRepositoryError.dataEncodingFailed { state.status = .failure(message: "Import data encoding failed.") + } catch BitwardenSdk.BitwardenError.E(let message) { + print(message) + } catch { state.status = .failure(message: Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) services.errorReporter.log(error: error) From 8dd43fe11c91f1ee134903640702addd2ee2e16e Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 22 Nov 2024 10:06:17 -0300 Subject: [PATCH 03/13] PM-14800 Fix Import CXP View padding by adding ScrollView. --- .../ImportCXP/ImportCXP/ImportCXPView.swift | 89 ++++++++++--------- 1 file changed, 47 insertions(+), 42 deletions(-) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index a5e380fbb..a1a2dbebd 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -13,53 +13,58 @@ struct ImportCXPView: View { // MARK: View var body: some View { - VStack(spacing: 16) { - PageHeaderView( - image: Asset.Images.Illustrations.import, - title: store.state.title, - message: store.state.message - ) - switch store.state.status { - case .start: - Spacer() - AsyncButton(Localizations.continue) { - await store.perform(.startImport) - } - .buttonStyle(.primary()) - AsyncButton(Localizations.cancel) { - await store.perform(.cancel) - } - .buttonStyle(.secondary()) - case .importing: - ProgressView() - .frame(maxWidth: .infinity) - case let .success(_, countByType): - VStack(spacing: 8) { - ForEach(countByType) { type in - HStack { - Text(type.localizedType) - Spacer() - Text("\(type.count)") + Group { + VStack(spacing: 16) { + PageHeaderView( + image: Asset.Images.Illustrations.import, + title: store.state.title, + message: store.state.message + ) + switch store.state.status { + case .start: + Spacer() + AsyncButton(Localizations.continue) { + await store.perform(.startImport) + } + .buttonStyle(.primary()) + AsyncButton(Localizations.cancel) { + await store.perform(.cancel) + } + .buttonStyle(.secondary()) + case .importing: + ProgressView() + .frame(maxWidth: .infinity) + case let .success(_, countByType): + VStack(spacing: 8) { + ForEach(countByType) { type in + HStack { + Text(type.localizedType) + Spacer() + Text("\(type.count)") + } } } + Spacer() + AsyncButton(Localizations.showVault) { + await store.perform(.showVault) + } + .buttonStyle(.primary()) + case .failure: + Spacer() + AsyncButton(Localizations.retryImport) { + await store.perform(.startImport) + } + .buttonStyle(.primary()) + AsyncButton(Localizations.cancel) { + await store.perform(.cancel) + } + .buttonStyle(.secondary()) } - Spacer() - AsyncButton(Localizations.showVault) { - await store.perform(.showVault) - } - case .failure: - Spacer() - AsyncButton(Localizations.retryImport) { - await store.perform(.startImport) - } - .buttonStyle(.primary()) - AsyncButton(Localizations.cancel) { - await store.perform(.cancel) - } - .buttonStyle(.secondary()) } + .padding(.top, 8) + .frame(maxWidth: .infinity) + .scrollView() } - .padding(.top, 8) .transition(.opacity) // .animation(.easeInOut, value: store.state.page) .navigationBar(title: Localizations.importPasswords, titleDisplayMode: .inline) From 8911766587fbd356974ad76527af83896debf6dc Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 22 Nov 2024 10:46:40 -0300 Subject: [PATCH 04/13] PM-14800 Fix CXP import to be initiated both with app in background as with app closed. --- Bitwarden/Application/SceneDelegate.swift | 43 +++++++++++++++++++---- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/Bitwarden/Application/SceneDelegate.swift b/Bitwarden/Application/SceneDelegate.swift index 86e98298b..5c190a487 100644 --- a/Bitwarden/Application/SceneDelegate.swift +++ b/Bitwarden/Application/SceneDelegate.swift @@ -72,6 +72,15 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { let incomingURL = userActivity.webpageURL { appProcessor.handleAppLinks(incomingURL: incomingURL) } + + #if compiler(>=6.0.3) + + if #available(iOS 18.2, *), + let userActivity = connectionOptions.userActivities.first { + await checkAndHandleCredentialExchangeActivity(appProcessor: appProcessor, userActivity: userActivity) + } + + #endif } } @@ -90,14 +99,9 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { #if compiler(>=6.0.3) - if #available(iOS 18.2, *), - userActivity.activityType == ASCredentialExchangeActivity { - guard let token = userActivity.userInfo?[ASCredentialImportToken] as? UUID else { - return - } - + if #available(iOS 18.2, *) { Task { - await appProcessor.handleImportCredentials(credentialImportToken: token) + await checkAndHandleCredentialExchangeActivity(appProcessor: appProcessor, userActivity: userActivity) } } @@ -175,3 +179,28 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { } #endif } + +// MARK: - SceneDelegate 18.2 + +#if compiler(>=6.0.3) + +@available(iOS 18.2, *) +extension SceneDelegate { + /// Checks whether there is an `ASCredentialExchangeActivity` in the `userActivity` and handles it. + /// - Parameters: + /// - appProcessor: The `AppProcessor` to handle the logic. + /// - userActivity: The activity to handle. + private func checkAndHandleCredentialExchangeActivity( + appProcessor: AppProcessor, + userActivity: NSUserActivity + ) async { + guard userActivity.activityType == ASCredentialExchangeActivity, + let token = userActivity.userInfo?[ASCredentialImportToken] as? UUID else { + return + } + + await appProcessor.handleImportCredentials(credentialImportToken: token) + } +} + +#endif From e8afb1c1545e6037cf2f4b4c12552d08252a1331 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 22 Nov 2024 16:25:48 -0300 Subject: [PATCH 05/13] PM-14800 Improved Import CXP UI/UX and added check for the feature flag. --- .../ImportCXP/ImportCXP/ImportCXPEffect.swift | 7 +--- .../ImportCXP/ImportCXPProcessor.swift | 27 ++++++++---- .../ImportCXP/ImportCXP/ImportCXPState.swift | 27 ++++++++++++ .../ImportCXP/ImportCXP/ImportCXPView.swift | 42 +++++++++---------- 4 files changed, 69 insertions(+), 34 deletions(-) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift index f314542c6..21e5c6699 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPEffect.swift @@ -11,9 +11,6 @@ enum ImportCXPEffect: Equatable { /// User wants to cancel the import process. case cancel - /// Shows the vault after finishing the import process. - case showVault - - /// User pressed button to start import process. - case startImport + /// The main button was tapped. + case mainButtonTapped } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index 104f27e1d..7896a334d 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -45,17 +45,27 @@ class ImportCXPProcessor: StateProcessor override func perform(_ effect: ImportCXPEffect) async { switch effect { case .appeared: - break + await checkEnabled() case .cancel: cancelWithConfirmation() - case .showVault: + case .mainButtonTapped: + guard case .success = state.status else { + await startImport() + return + } coordinator.navigate(to: .dismiss) - case .startImport: - await startImport() } } // MARK: Private + + /// Checks whether the CXP import feature is enabled. + private func checkEnabled() async { + guard #available(iOS 18.2, *), await services.configService.getFeatureFlag(.cxpImportMobile) else { + state.status = .failure(message: Localizations.featureUnavailable) + return + } + } /// Starts the import process. private func startImport() async { @@ -86,9 +96,8 @@ class ImportCXPProcessor: StateProcessor state.status = .failure(message: "No data found to import.") } catch ImportCiphersRepositoryError.dataEncodingFailed { state.status = .failure(message: "Import data encoding failed.") - } catch BitwardenSdk.BitwardenError.E(let message) { + } catch BitwardenSdk.BitwardenError.E(let message) { print(message) - } catch { state.status = .failure(message: Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) services.errorReporter.log(error: error) @@ -98,8 +107,12 @@ class ImportCXPProcessor: StateProcessor } /// Shows the alert confirming the user wants to import logins later. - /// private func cancelWithConfirmation() { + guard !state.isFeatureUnvailable else { + coordinator.navigate(to: .dismiss) + return + } + coordinator.showAlert(.confirmCancelCXPImport { [weak self] in guard let self else { return } coordinator.navigate(to: .dismiss) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index b960d2194..ed3f081f7 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -27,6 +27,23 @@ struct ImportCXPState: Equatable, Sendable { /// The token used in `ASCredentialImportManager` to get the credentials to import. var credentialImportToken: UUID? + /// Whether the CXP import feature is available. + var isFeatureUnvailable: Bool = false + + /// The title of the main button. + var mainButtonTitle: String { + return switch status { + case .start: + Localizations.continue + case .importing: + "" + case .success: + Localizations.showVault + case .failure: + Localizations.retryImport + } + } + /// The message to display on the page header. var message: String { return switch status { @@ -55,6 +72,16 @@ struct ImportCXPState: Equatable, Sendable { } } + /// Whether to show the cancel button. + var showCancelButton: Bool { + return switch status { + case .importing, .success: + false + case .start, .failure: + true + } + } + /// The current status of the import process. var status: ImportCXPStatus = .start } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index a1a2dbebd..8f26c8e5a 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -22,20 +22,12 @@ struct ImportCXPView: View { ) switch store.state.status { case .start: - Spacer() - AsyncButton(Localizations.continue) { - await store.perform(.startImport) - } - .buttonStyle(.primary()) - AsyncButton(Localizations.cancel) { - await store.perform(.cancel) - } - .buttonStyle(.secondary()) + EmptyView() case .importing: ProgressView() .frame(maxWidth: .infinity) case let .success(_, countByType): - VStack(spacing: 8) { + VStack(spacing: 16) { ForEach(countByType) { type in HStack { Text(type.localizedType) @@ -44,29 +36,35 @@ struct ImportCXPView: View { } } } - Spacer() - AsyncButton(Localizations.showVault) { - await store.perform(.showVault) - } - .buttonStyle(.primary()) + .padding(.horizontal, 20) + .padding(.top, 12) case .failure: - Spacer() - AsyncButton(Localizations.retryImport) { - await store.perform(.startImport) + EmptyView() + } + } + .padding(.top, 8) + .frame(maxWidth: .infinity) + .scrollView() + .safeAreaInset(edge: .bottom) { + VStack { + AsyncButton(store.state.mainButtonTitle) { + await store.perform(.mainButtonTapped) } .buttonStyle(.primary()) + .hidden(store.state.status == .importing && !store.state.isFeatureUnvailable) + AsyncButton(Localizations.cancel) { await store.perform(.cancel) } .buttonStyle(.secondary()) + .hidden(!store.state.showCancelButton) } + .padding(.horizontal, 16) + .background(Asset.Colors.backgroundPrimary.swiftUIColor) } - .padding(.top, 8) - .frame(maxWidth: .infinity) - .scrollView() } .transition(.opacity) -// .animation(.easeInOut, value: store.state.page) + .animation(.easeInOut, value: store.state.status) .navigationBar(title: Localizations.importPasswords, titleDisplayMode: .inline) // .toolbar { // cancelToolbarItem { From ae402d6225163f6975c2f9b317a8d3e927f84acf Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 22 Nov 2024 16:35:04 -0300 Subject: [PATCH 06/13] PM-14800 Removed toolbar from Import CXP view. --- .../Tools/ImportCXP/ImportCXP/ImportCXPView.swift | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index 8f26c8e5a..240f3956a 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -65,15 +65,16 @@ struct ImportCXPView: View { } .transition(.opacity) .animation(.easeInOut, value: store.state.status) - .navigationBar(title: Localizations.importPasswords, titleDisplayMode: .inline) -// .toolbar { -// cancelToolbarItem { -// store.send(.dismiss) -// } -// } .task { await store.perform(.appeared) } + .apply { view in + if #available(iOSApplicationExtension 16.0, *) { + view.toolbar(.hidden) + } else { + view.navigationBarHidden(true) + } + } } // MARK: Private From 34b9cbfafd21909838b4112736ee751550cc909f Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Mon, 25 Nov 2024 14:36:48 -0300 Subject: [PATCH 07/13] PM-14800 Cleaned code for encoding/decoding CXP --- .../Services/API/Extensions/AnyKey.swift | 16 ++++++++++ .../Extensions/JSONDecoder+Bitwarden.swift | 19 ----------- .../Extensions/JSONEncoder+Bitwarden.swift | 32 ------------------- .../ImportCXP/ImportCXPProcessor.swift | 4 +-- .../ImportCXP/ImportCXP/ImportCXPState.swift | 2 +- 5 files changed, 19 insertions(+), 54 deletions(-) create mode 100644 BitwardenShared/Core/Platform/Services/API/Extensions/AnyKey.swift diff --git a/BitwardenShared/Core/Platform/Services/API/Extensions/AnyKey.swift b/BitwardenShared/Core/Platform/Services/API/Extensions/AnyKey.swift new file mode 100644 index 000000000..e119df5bd --- /dev/null +++ b/BitwardenShared/Core/Platform/Services/API/Extensions/AnyKey.swift @@ -0,0 +1,16 @@ +/// `AnyKey` is a `CodingKey` type that can be used for encoding and decoding keys for custom +/// key decoding strategies. +struct AnyKey: CodingKey { + let stringValue: String + let intValue: Int? + + init(stringValue: String) { + self.stringValue = stringValue + intValue = nil + } + + init(intValue: Int) { + stringValue = String(intValue) + self.intValue = intValue + } +} diff --git a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONDecoder+Bitwarden.swift b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONDecoder+Bitwarden.swift index a4d9a9588..f382aa1dc 100644 --- a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONDecoder+Bitwarden.swift +++ b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONDecoder+Bitwarden.swift @@ -1,25 +1,6 @@ import Foundation extension JSONDecoder { - // MARK: Types - - /// `AnyKey` is a `CodingKey` type that can be used for encoding and decoding keys for custom - /// key decoding strategies. - struct AnyKey: CodingKey { - let stringValue: String - let intValue: Int? - - init(stringValue: String) { - self.stringValue = stringValue - intValue = nil - } - - init(intValue: Int) { - stringValue = String(intValue) - self.intValue = intValue - } - } - // MARK: Static Properties /// The default `JSONDecoder` used to decode JSON payloads throughout the app. diff --git a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift index b7cefa052..68c68c229 100644 --- a/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift +++ b/BitwardenShared/Core/Platform/Services/API/Extensions/JSONEncoder+Bitwarden.swift @@ -3,23 +3,6 @@ import Foundation extension JSONEncoder { // MARK: Static Properties - /// `AnyKey` is a `CodingKey` type that can be used for encoding and decoding keys for custom - /// key decoding strategies. - struct AnyKey: CodingKey { - let stringValue: String - let intValue: Int? - - init(stringValue: String) { - self.stringValue = stringValue - intValue = nil - } - - init(intValue: Int) { - stringValue = String(intValue) - self.intValue = intValue - } - } - /// The default `JSONEncoder` used to encode JSON payloads throughout the app. static let defaultEncoder: JSONEncoder = { let dateFormatterWithFractionalSeconds = ISO8601DateFormatter() @@ -60,19 +43,4 @@ extension JSONEncoder { key } } - - /// Transforms a snake_case, PascalCase or camelCase key into camelCase. - static func keyToCamelCase(key: String) -> String { - if key.contains("_") { - // Handle snake_case. - return key.lowercased() - .split(separator: "_") - .enumerated() - .map { $0.offset > 0 ? $0.element.capitalized : $0.element.lowercased() } - .joined() - } - - // Handle PascalCase or camelCase. - return key.prefix(1).lowercased() + key.dropFirst() - } } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index 7896a334d..b0b63fc20 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -58,7 +58,7 @@ class ImportCXPProcessor: StateProcessor } // MARK: Private - + /// Checks whether the CXP import feature is enabled. private func checkEnabled() async { guard #available(iOS 18.2, *), await services.configService.getFeatureFlag(.cxpImportMobile) else { @@ -96,7 +96,7 @@ class ImportCXPProcessor: StateProcessor state.status = .failure(message: "No data found to import.") } catch ImportCiphersRepositoryError.dataEncodingFailed { state.status = .failure(message: "Import data encoding failed.") - } catch BitwardenSdk.BitwardenError.E(let message) { + } catch let BitwardenSdk.BitwardenError.E(message) { print(message) } catch { state.status = .failure(message: Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index ed3f081f7..089de6a77 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -77,7 +77,7 @@ struct ImportCXPState: Equatable, Sendable { return switch status { case .importing, .success: false - case .start, .failure: + case .failure, .start: true } } From 212ce48643305404aacd490f572aabb02012d5fa Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Mon, 25 Nov 2024 17:12:34 -0300 Subject: [PATCH 08/13] PM-14800 Improved import CXP UI/UX --- .../en.lproj/Localizable.strings | 2 +- .../Application/Views/PageHeaderView.swift | 56 ++++++++++++++++--- .../ImportCXP/ImportCXP/ImportCXPState.swift | 5 ++ .../ImportCXP/ImportCXP/ImportCXPView.swift | 23 +++++--- 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings index 2d547a7bd..50b521251 100644 --- a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings +++ b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings @@ -1061,7 +1061,7 @@ "SSHKeys" = "SSH keys"; "ExportingFailed" = "Exporting failed"; "YouMayNeedToEnableDevicePasscodeOrBiometrics" = "You may need to enable device passcode or biometrics."; -"StartImportCXPDescriptionLong" = "Import passwords, passkeys, credit cards, and any personal identity information from another password manager.\n\nYour data will not be deleted from your previous provider."; +"StartImportCXPDescriptionLong" = "Import passwords, passkeys, credit cards, and any personal identity information from another password manager.\n\nYour data will **not** be deleted from your previous provider."; "ImportPasswords" = "Import passwords"; "ImportingEllipsis" = "Importing..."; "AreYouSureYouWantToCancelTheImportProcessQuestionMark" = "Are you sure you want to cancel the import process?"; diff --git a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift index 05873ecdd..a9694d285 100644 --- a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift +++ b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift @@ -5,6 +5,17 @@ import SwiftUI /// A view that renders a header for page. This support displaying an image, title, and message. /// struct PageHeaderView: View { + // MARK: Types + + /// The style to apply to the `PageHeaderView`. + enum StyleMode { + /// Normal font style. + case normal + + /// Large font style. + case large + } + // MARK: Properties /// The image to display in the page header. @@ -16,6 +27,9 @@ struct PageHeaderView: View { /// The title to display in the page header. let title: String + /// The style to apply to this view. + let style: StyleMode + /// An environment variable for getting the vertical size class of the view. @Environment(\.verticalSizeClass) var verticalSizeClass @@ -29,10 +43,24 @@ struct PageHeaderView: View { VStack(spacing: 16) { Text(title) - .styleGuide(.title2, weight: .bold) - - Text(message) - .styleGuide(.body) + .apply { text in + switch style { + case .normal: + text.styleGuide(.title2, weight: .bold) + case .large: + text.styleGuide(.largeTitle, weight: .bold) + } + } + + Text(LocalizedStringKey(message)) + .apply { text in + switch style { + case .normal: + text.styleGuide(.body) + case .large: + text.styleGuide(.title2) + } + } } } .foregroundStyle(Asset.Colors.textPrimary.swiftUIColor) @@ -47,11 +75,13 @@ struct PageHeaderView: View { /// - image: The image to display. /// - title: The title to display. /// - message: The message to display. + /// - style: The style to use for this view. /// - init(image: Image, title: String, message: String) { + init(image: Image, title: String, message: String, style: StyleMode = .normal) { self.image = image self.message = message self.title = title + self.style = style } /// Initialize a `PageHeaderView`. @@ -60,11 +90,13 @@ struct PageHeaderView: View { /// - image: The image asset to display. /// - title: The title to display. /// - message: The message to display. + /// - style: The style to use for this view. /// - init(image: ImageAsset, title: String, message: String) { + init(image: ImageAsset, title: String, message: String, style: StyleMode = .normal) { self.image = image.swiftUIImage self.message = message self.title = title + self.style = style } // MARK: Private @@ -85,11 +117,21 @@ struct PageHeaderView: View { // MARK: - Previews #if DEBUG -#Preview("PageHeader") { +#Preview("PageHeader Normal") { PageHeaderView( image: Asset.Images.Illustrations.biometricsPhone, title: Localizations.setUpUnlock, message: Localizations.setUpBiometricsOrChooseAPinCodeToQuicklyAccessYourVaultAndAutofillYourLogins ) } + +#Preview("PageHeader Large") { + PageHeaderView( + image: Asset.Images.Illustrations.biometricsPhone, + title: Localizations.setUpUnlock, + message: Localizations.setUpBiometricsOrChooseAPinCodeToQuicklyAccessYourVaultAndAutofillYourLogins, + style: .large + ) +} + #endif diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index 089de6a77..61f499634 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -82,6 +82,11 @@ struct ImportCXPState: Equatable, Sendable { } } + /// Whether to show the main button. + var showMainButton: Bool { + status != .importing || isFeatureUnvailable + } + /// The current status of the import process. var status: ImportCXPStatus = .start } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index 240f3956a..af29a60cf 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -18,7 +18,8 @@ struct ImportCXPView: View { PageHeaderView( image: Asset.Images.Illustrations.import, title: store.state.title, - message: store.state.message + message: store.state.message, + style: .large ) switch store.state.status { case .start: @@ -31,8 +32,10 @@ struct ImportCXPView: View { ForEach(countByType) { type in HStack { Text(type.localizedType) + .styleGuide(.body) Spacer() Text("\(type.count)") + .styleGuide(.body) } } } @@ -47,17 +50,19 @@ struct ImportCXPView: View { .scrollView() .safeAreaInset(edge: .bottom) { VStack { - AsyncButton(store.state.mainButtonTitle) { - await store.perform(.mainButtonTapped) + if store.state.showMainButton { + AsyncButton(store.state.mainButtonTitle) { + await store.perform(.mainButtonTapped) + } + .buttonStyle(.primary()) } - .buttonStyle(.primary()) - .hidden(store.state.status == .importing && !store.state.isFeatureUnvailable) - AsyncButton(Localizations.cancel) { - await store.perform(.cancel) + if store.state.showCancelButton { + AsyncButton(Localizations.cancel) { + await store.perform(.cancel) + } + .buttonStyle(.secondary()) } - .buttonStyle(.secondary()) - .hidden(!store.state.showCancelButton) } .padding(.horizontal, 16) .background(Asset.Colors.backgroundPrimary.swiftUIColor) From 35748efaa9f6e5e4ff55c36afef54e1058636afd Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Tue, 26 Nov 2024 14:05:25 -0300 Subject: [PATCH 09/13] PM-14800 Improved CXP import UI to adapt to style guides and better fit to Figma design. --- .../TestHelpers/ServiceContainer+Mocks.swift | 2 + .../Utilities/OSLogErrorReporter.swift | 2 +- .../MockImportCiphersRepository.swift | 13 ++ .../API/ImportCiphersAPIServiceTests.swift | 4 +- .../Requests/ImportCiphersRequestTests.swift | 5 +- .../TestHelpers/MockClientExporters.swift | 11 ++ .../Appearance/StyleGuideFont.swift | 15 +++ .../en.lproj/Localizable.strings | 2 + .../Application/Views/PageHeaderView.swift | 40 +++--- .../ImportCXP/ImportCXPProcessor.swift | 4 +- .../ImportCXP/ImportCXP/ImportCXPState.swift | 14 +- .../ImportCXP/ImportCXPStateTests.swift | 120 ++++++++++++++++++ .../ImportCXP/ImportCXP/ImportCXPView.swift | 10 +- GlobalTestHelpers/MockAppModule.swift | 6 + 14 files changed, 220 insertions(+), 28 deletions(-) create mode 100644 BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift index 952e05626..670e7f49e 100644 --- a/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift +++ b/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift @@ -24,6 +24,7 @@ extension ServiceContainer { fido2CredentialStore: Fido2CredentialStore = MockFido2CredentialStore(), fido2UserInterfaceHelper: Fido2UserInterfaceHelper = MockFido2UserInterfaceHelper(), generatorRepository: GeneratorRepository = MockGeneratorRepository(), + importCiphersRepository: ImportCiphersRepository = MockImportCiphersRepository(), httpClient: HTTPClient = MockHTTPClient(), keychainRepository: KeychainRepository = MockKeychainRepository(), keychainService: KeychainService = MockKeychainService(), @@ -75,6 +76,7 @@ extension ServiceContainer { fido2CredentialStore: fido2CredentialStore, fido2UserInterfaceHelper: fido2UserInterfaceHelper, generatorRepository: generatorRepository, + importCiphersRepository: importCiphersRepository, keychainRepository: keychainRepository, keychainService: keychainService, localAuthService: localAuthService, diff --git a/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift b/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift index 2442a4743..6d1843bb2 100644 --- a/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift +++ b/BitwardenShared/Core/Platform/Utilities/OSLogErrorReporter.swift @@ -27,7 +27,7 @@ public final class OSLogErrorReporter: ErrorReporter { guard !error.isNetworkingError else { return } // Crash in debug builds to make the error more visible during development. -// assertionFailure("Unexpected error: \(error)") + assertionFailure("Unexpected error: \(error)") } public func setRegion(_ region: String, isPreAuth: Bool) { diff --git a/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift new file mode 100644 index 000000000..6be4e7765 --- /dev/null +++ b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift @@ -0,0 +1,13 @@ +import BitwardenSdk +import Foundation + +@testable import BitwardenShared + +class MockImportCiphersRepository: ImportCiphersRepository { + var importCiphersResult = InvocationMockerWithThrowingResult() + .withResult([]) + + func importCiphers(credentialImportToken: UUID) async throws -> [ImportedCredentialsResult] { + try importCiphersResult.invoke(param: credentialImportToken) + } +} diff --git a/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIServiceTests.swift b/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIServiceTests.swift index 02d69e58b..6b7b8a0ad 100644 --- a/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIServiceTests.swift +++ b/BitwardenShared/Core/Tools/Services/API/ImportCiphersAPIServiceTests.swift @@ -31,7 +31,7 @@ class ImportCiphersAPIServiceTests: BitwardenTestCase { client.results = [ .httpSuccess(testData: .emptyResponse), ] - _ = try await subject.importCiphers(ciphers: [.fixture()], folders: [], folderRelationships: [:]) + _ = try await subject.importCiphers(ciphers: [.fixture()], folders: [], folderRelationships: []) XCTAssertEqual(client.requests.count, 1) XCTAssertNotNil(client.requests[0].body) @@ -46,7 +46,7 @@ class ImportCiphersAPIServiceTests: BitwardenTestCase { ] await assertAsyncThrows(error: BitwardenTestError.example) { - _ = try await subject.importCiphers(ciphers: [.fixture()], folders: [], folderRelationships: [:]) + _ = try await subject.importCiphers(ciphers: [.fixture()], folders: [], folderRelationships: []) } } } diff --git a/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequestTests.swift b/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequestTests.swift index 718ffb2a1..4536ecdd2 100644 --- a/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequestTests.swift +++ b/BitwardenShared/Core/Tools/Services/API/Requests/ImportCiphersRequestTests.swift @@ -12,11 +12,12 @@ class ImportCiphersRequestTests: BitwardenTestCase { let subject = try ImportCiphersRequest( ciphers: [.fixture(name: "cipherTest")], folders: [.fixture(name: "folderTest")], - folderRelationships: [1: 1] + folderRelationships: [(1, 1)] ) XCTAssertEqual(subject.body?.ciphers[0].name, "cipherTest") XCTAssertEqual(subject.body?.folders[0].name, "folderTest") - XCTAssertEqual(subject.body?.folderRelationships[1], 1) + XCTAssertEqual(subject.body?.folderRelationships[0].key, 1) + XCTAssertEqual(subject.body?.folderRelationships[0].value, 1) } /// `init(ciphers:folders:folderRelationships:)` initializes the request successfully. diff --git a/BitwardenShared/Core/Vault/Services/TestHelpers/MockClientExporters.swift b/BitwardenShared/Core/Vault/Services/TestHelpers/MockClientExporters.swift index 53a491426..e0f7e8316 100644 --- a/BitwardenShared/Core/Vault/Services/TestHelpers/MockClientExporters.swift +++ b/BitwardenShared/Core/Vault/Services/TestHelpers/MockClientExporters.swift @@ -26,6 +26,12 @@ class MockClientExporters { /// The result of a call to `exportVault(_:)` var exportVaultResult: Result = .failure(BitwardenTestError.example) + /// The payload passed to `importCxf(payload:)` + var importCxfPayload: String? + + /// The result of a call to `importCxf(payload:)` + var importCxfResult: Result<[BitwardenSdk.Cipher], Error> = .failure(BitwardenTestError.example) + /// The folders exported in a call to `exportVault(_:)`. var folders = [BitwardenSdk.Folder]() @@ -63,4 +69,9 @@ extension MockClientExporters: ClientExportersProtocol { self.format = format return try exportVaultResult.get() } + + func importCxf(payload: String) throws -> [BitwardenSdk.Cipher] { + importCxfPayload = payload + return try importCxfResult.get() + } } diff --git a/BitwardenShared/UI/Platform/Application/Appearance/StyleGuideFont.swift b/BitwardenShared/UI/Platform/Application/Appearance/StyleGuideFont.swift index f6bd12d84..db01ec274 100644 --- a/BitwardenShared/UI/Platform/Application/Appearance/StyleGuideFont.swift +++ b/BitwardenShared/UI/Platform/Application/Appearance/StyleGuideFont.swift @@ -49,6 +49,9 @@ extension StyleGuideFont { // MARK: - StyleGuideFont Constants extension StyleGuideFont { + /// The font for the huge title style. + static let hugeTitle = StyleGuideFont.dmSans(lineHeight: 41, size: 34, textStyle: .largeTitle) + /// The font for the large title style. static let largeTitle = StyleGuideFont.dmSans(lineHeight: 32, size: 26, textStyle: .largeTitle) @@ -208,6 +211,8 @@ struct StyleGuideFont_Previews: PreviewProvider { static var previews: some View { HStack { VStack(alignment: .trailing, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle) Text("Large Title") .styleGuide(.largeTitle) Text("Title") @@ -236,6 +241,8 @@ struct StyleGuideFont_Previews: PreviewProvider { .styleGuide(.caption2Monospaced) } VStack(alignment: .leading, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle, weight: .semibold) Text("Large Title") .styleGuide(.largeTitle, weight: .semibold) Text("Title") @@ -269,6 +276,8 @@ struct StyleGuideFont_Previews: PreviewProvider { HStack { VStack(alignment: .trailing, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle) Text("Large Title") .styleGuide(.largeTitle) Text("Title") @@ -297,6 +306,8 @@ struct StyleGuideFont_Previews: PreviewProvider { .styleGuide(.caption2Monospaced) } VStack(alignment: .leading, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle, isItalic: true) Text("Large Title") .styleGuide(.largeTitle, isItalic: true) Text("Title") @@ -330,6 +341,8 @@ struct StyleGuideFont_Previews: PreviewProvider { HStack { VStack(alignment: .trailing, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle) Text("Large Title") .styleGuide(.largeTitle) Text("Title") @@ -358,6 +371,8 @@ struct StyleGuideFont_Previews: PreviewProvider { .styleGuide(.caption2Monospaced) } VStack(alignment: .leading, spacing: 8) { + Text("Huge Title") + .styleGuide(.hugeTitle, weight: .semibold, isItalic: true) Text("Large Title") .styleGuide(.largeTitle, weight: .semibold, isItalic: true) Text("Title") diff --git a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings index 50b521251..b568bff06 100644 --- a/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings +++ b/BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings @@ -1070,3 +1070,5 @@ "ThereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted" = "There was an issue importing all of your passwords.\n\nNo data was deleted."; "RetryImport" = "Retry import"; "ShowVault" = "Show vault"; +"ImportNotAvailable" = "Import not available"; +"ImportingFromAnotherProviderIsNotAvailableForThisDevice" = "Importing from another provider is not available for this device."; diff --git a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift index a9694d285..79e9a02e2 100644 --- a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift +++ b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift @@ -9,11 +9,11 @@ struct PageHeaderView: View { /// The style to apply to the `PageHeaderView`. enum StyleMode { - /// Normal font style. - case normal + /// Normal font style with illustration as image. + case normalWithIllustration - /// Large font style. - case large + /// Large font style with tinted icon as image. + case largeWithTintedIcon } // MARK: Properties @@ -37,27 +37,35 @@ struct PageHeaderView: View { var body: some View { dynamicStackView { - image - .resizable() - .frame(width: 100, height: 100) + switch style { + case .normalWithIllustration: + image + .resizable() + .frame(width: 100, height: 100) + case .largeWithTintedIcon: + image + .resizable() + .frame(width: 70, height: 70) + .foregroundStyle(Asset.Colors.iconSecondary.swiftUIColor) + } VStack(spacing: 16) { Text(title) .apply { text in switch style { - case .normal: + case .normalWithIllustration: text.styleGuide(.title2, weight: .bold) - case .large: - text.styleGuide(.largeTitle, weight: .bold) + case .largeWithTintedIcon: + text.styleGuide(.hugeTitle, weight: .bold) } } Text(LocalizedStringKey(message)) .apply { text in switch style { - case .normal: + case .normalWithIllustration: text.styleGuide(.body) - case .large: + case .largeWithTintedIcon: text.styleGuide(.title2) } } @@ -77,7 +85,7 @@ struct PageHeaderView: View { /// - message: The message to display. /// - style: The style to use for this view. /// - init(image: Image, title: String, message: String, style: StyleMode = .normal) { + init(image: Image, title: String, message: String, style: StyleMode = .normalWithIllustration) { self.image = image self.message = message self.title = title @@ -92,7 +100,7 @@ struct PageHeaderView: View { /// - message: The message to display. /// - style: The style to use for this view. /// - init(image: ImageAsset, title: String, message: String, style: StyleMode = .normal) { + init(image: ImageAsset, title: String, message: String, style: StyleMode = .normalWithIllustration) { self.image = image.swiftUIImage self.message = message self.title = title @@ -127,10 +135,10 @@ struct PageHeaderView: View { #Preview("PageHeader Large") { PageHeaderView( - image: Asset.Images.Illustrations.biometricsPhone, + image: Asset.Images.plus24, title: Localizations.setUpUnlock, message: Localizations.setUpBiometricsOrChooseAPinCodeToQuicklyAccessYourVaultAndAutofillYourLogins, - style: .large + style: .largeWithTintedIcon ) } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index b0b63fc20..771e87fb4 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -62,7 +62,7 @@ class ImportCXPProcessor: StateProcessor /// Checks whether the CXP import feature is enabled. private func checkEnabled() async { guard #available(iOS 18.2, *), await services.configService.getFeatureFlag(.cxpImportMobile) else { - state.status = .failure(message: Localizations.featureUnavailable) + state.status = .failure(message: Localizations.importingFromAnotherProviderIsNotAvailableForThisDevice) return } } @@ -75,7 +75,7 @@ class ImportCXPProcessor: StateProcessor coordinator.showAlert( .defaultAlert( title: Localizations.importError, - message: Localizations.featureUnavailable + message: Localizations.importingFromAnotherProviderIsNotAvailableForThisDevice ) ) return diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index 61f499634..ff75b83e2 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -44,6 +44,18 @@ struct ImportCXPState: Equatable, Sendable { } } + /// The main icon to be displayed. + var mainIcon: ImageAsset { + return switch status { + case .importing, .start: + Asset.Images.Illustrations.import + case .success: + Asset.Images.checkCircle24 + case .failure: + Asset.Images.circleX16 + } + } + /// The message to display on the page header. var message: String { return switch status { @@ -68,7 +80,7 @@ struct ImportCXPState: Equatable, Sendable { case .success: Localizations.importSuccessful case .failure: - Localizations.importFailed + isFeatureUnvailable ? Localizations.importNotAvailable : Localizations.importFailed } } diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift new file mode 100644 index 000000000..b15521c0b --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift @@ -0,0 +1,120 @@ +import XCTest + +@testable import BitwardenShared + +// MARK: - ImportCXPStateTests + +class ImportCXPStateTests: BitwardenTestCase { + // MARK: Properties + + var subject: ImportCXPState! + + // MARK: Setup & Teardown + + override func setUp() { + super.setUp() + + subject = ImportCXPState() + } + + override func tearDown() { + super.tearDown() + + subject = nil + } + + // MARK: Tests + + /// `getter:mainButtonTitle` returns the appropriate value depending on the `status`. + func test_mainButtonTitle() { + subject.status = .start + XCTAssertEqual(subject.mainButtonTitle, Localizations.continue) + + subject.status = .importing + XCTAssertEqual(subject.mainButtonTitle, "") + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertEqual(subject.mainButtonTitle, Localizations.showVault) + + subject.status = .failure(message: "") + XCTAssertEqual(subject.mainButtonTitle, Localizations.retryImport) + } + + /// `getter:mainIcon` returns the appropriate value depending on the `status`. + func test_mainIcon() { + subject.status = .start + XCTAssertEqual(subject.mainIcon.name, Asset.Images.Illustrations.import.name) + + subject.status = .importing + XCTAssertEqual(subject.mainIcon.name, Asset.Images.Illustrations.import.name) + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertEqual(subject.mainIcon.name, Asset.Images.checkCircle24.name) + + subject.status = .failure(message: "") + XCTAssertEqual(subject.mainIcon.name, Asset.Images.circleX16.name) + } + + /// `getter:message` returns the appropriate value depending on the `status`. + func test_message() { + subject.status = .start + XCTAssertEqual(subject.message, Localizations.startImportCXPDescriptionLong) + + subject.status = .importing + XCTAssertEqual(subject.message, "") + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertEqual(subject.message, Localizations.itemsSuccessfullyImported(1)) + + subject.status = .failure(message: "Something went wrong") + XCTAssertEqual(subject.message, "Something went wrong") + } + + /// `getter:title` returns the appropriate value depending on the `status`. + func test_title() { + subject.status = .start + XCTAssertEqual(subject.title, Localizations.importPasswords) + + subject.status = .importing + XCTAssertEqual(subject.title, Localizations.importingEllipsis) + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertEqual(subject.title, Localizations.importSuccessful) + + subject.status = .failure(message: "Something went wrong") + XCTAssertEqual(subject.title, Localizations.importFailed) + + subject.isFeatureUnvailable = true + XCTAssertEqual(subject.title, Localizations.importNotAvailable) + } + + /// `getter:showCancelButton` returns the appropriate value depending on the `status`. + func test_showCancelButton() { + subject.status = .start + XCTAssertTrue(subject.showCancelButton) + + subject.status = .importing + XCTAssertFalse(subject.showCancelButton) + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertFalse(subject.showCancelButton) + + subject.status = .failure(message: "Something went wrong") + XCTAssertTrue(subject.showCancelButton) + } + + /// `getter:showMainButton` returns the appropriate value depending on the `status`. + func test_showMainButton() { + subject.status = .start + XCTAssertTrue(subject.showMainButton) + + subject.status = .importing + XCTAssertFalse(subject.showMainButton) + + subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + XCTAssertTrue(subject.showMainButton) + + subject.status = .failure(message: "Something went wrong") + XCTAssertTrue(subject.showMainButton) + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index af29a60cf..3a46b0b3a 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -16,10 +16,10 @@ struct ImportCXPView: View { Group { VStack(spacing: 16) { PageHeaderView( - image: Asset.Images.Illustrations.import, + image: store.state.mainIcon.swiftUIImage, title: store.state.title, message: store.state.message, - style: .large + style: .largeWithTintedIcon ) switch store.state.status { case .start: @@ -47,7 +47,7 @@ struct ImportCXPView: View { } .padding(.top, 8) .frame(maxWidth: .infinity) - .scrollView() + .scrollView(backgroundColor: Asset.Colors.backgroundSecondary.swiftUIColor) .safeAreaInset(edge: .bottom) { VStack { if store.state.showMainButton { @@ -55,6 +55,7 @@ struct ImportCXPView: View { await store.perform(.mainButtonTapped) } .buttonStyle(.primary()) + .accessibilityIdentifier("MainButton") } if store.state.showCancelButton { @@ -62,10 +63,11 @@ struct ImportCXPView: View { await store.perform(.cancel) } .buttonStyle(.secondary()) + .accessibilityIdentifier("CancelButton") } } .padding(.horizontal, 16) - .background(Asset.Colors.backgroundPrimary.swiftUIColor) + .background(Asset.Colors.backgroundSecondary.swiftUIColor) } } .transition(.opacity) diff --git a/GlobalTestHelpers/MockAppModule.swift b/GlobalTestHelpers/MockAppModule.swift index f6f75dd86..e46a7d925 100644 --- a/GlobalTestHelpers/MockAppModule.swift +++ b/GlobalTestHelpers/MockAppModule.swift @@ -9,6 +9,7 @@ class MockAppModule: ExtensionSetupModule, FileSelectionModule, GeneratorModule, + ImportCXPModule, ImportLoginsModule, LoginRequestModule, PasswordAutoFillModule, @@ -27,6 +28,7 @@ class MockAppModule: var fileSelectionDelegate: FileSelectionDelegate? var fileSelectionCoordinator = MockCoordinator() var generatorCoordinator = MockCoordinator() + var importCXPCoordinator = MockCoordinator() var importLoginsCoordinator = MockCoordinator() var loginRequestCoordinator = MockCoordinator() var passwordAutoFillCoordinator = MockCoordinator() @@ -88,6 +90,10 @@ class MockAppModule: generatorCoordinator.asAnyCoordinator() } + func makeImportCXPCoordinator(stackNavigator: any BitwardenShared.StackNavigator) -> BitwardenShared.AnyCoordinator { + importCXPCoordinator.asAnyCoordinator() + } + func makeImportLoginsCoordinator( delegate: any ImportLoginsCoordinatorDelegate, stackNavigator: any StackNavigator From 4389ca3c085004cf4d9b17fddf93157c97788e77 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Tue, 26 Nov 2024 16:35:22 -0300 Subject: [PATCH 10/13] PM-14800 Change CXP Import to use a progress bar with three discrete progress updates, also improved accessibility IDs and improved naming. --- .../ImportCiphersRepository.swift | 36 ++++++++----- .../MockImportCiphersRepository.swift | 5 +- .../Utilities/ImportedCredentialsResult.swift | 42 ++++++++++++++-- .../ImportedCredentialsResultTests.swift | 42 ++++++++++++++++ .../Application/Views/PageHeaderView.swift | 2 + .../ImportCXP/ImportCXPProcessor.swift | 10 ++-- .../ImportCXP/ImportCXP/ImportCXPState.swift | 5 +- .../ImportCXP/ImportCXPStateTests.swift | 12 ++--- .../ImportCXP/ImportCXP/ImportCXPView.swift | 50 +++++++++++++------ 9 files changed, 160 insertions(+), 44 deletions(-) create mode 100644 BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResultTests.swift diff --git a/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift index c808b8c72..a5928a8f2 100644 --- a/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift +++ b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift @@ -8,10 +8,14 @@ protocol ImportCiphersRepository: AnyObject { /// Performs an API request to import ciphers in the vault. /// - Parameters: /// - credentialImportToken: The token used in `ASCredentialImportManager` to get the credentials to import. + /// - onProgress: Closure to update progress in the process. /// - Returns: A dictionary containing the localized cipher type (key) and count (value) of that type /// that was imported, e.g. ["Passwords": 3, "Cards": 2]. @available(iOS 18.2, *) - func importCiphers(credentialImportToken: UUID) async throws -> [ImportedCredentialsResult] + func importCiphers( + credentialImportToken: UUID, + onProgress: @MainActor (_ progress: Double) -> Void + ) async throws -> [ImportedCredentialsResult] } // MARK: - DefaultImportCiphersRepository @@ -55,7 +59,8 @@ class DefaultImportCiphersRepository { extension DefaultImportCiphersRepository: ImportCiphersRepository { @available(iOS 18.2, *) func importCiphers( // swiftlint:disable:this function_body_length - credentialImportToken: UUID + credentialImportToken: UUID, + onProgress: @MainActor (_ progress: Double) -> Void ) async throws -> [ImportedCredentialsResult] { #if compiler(>=6.0.3) @@ -73,6 +78,8 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { let ciphers = try await clientService.exporters().importCxf(payload: accountJsonString) + await onProgress(0.3) + _ = try await importCiphersService .importCiphers( ciphers: ciphers, @@ -80,13 +87,15 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { folderRelationships: [] ) + await onProgress(0.8) + try await syncService.fetchSync(forceSync: true) var importedCredentialsCount: [ImportedCredentialsResult] = [] appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.passwords, + type: .password, when: { cipher in cipher.type == .login && cipher.login?.fido2Credentials?.isEmpty != false } @@ -94,7 +103,7 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.passkeys, + type: .passkey, when: { cipher in cipher.type == .login && cipher.login?.fido2Credentials?.isEmpty == false } @@ -102,27 +111,30 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.cards, + type: .card, when: { $0.type == .card } ) appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.identities, + type: .identity, when: { $0.type == .identity } ) appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.secureNotes, + type: .secureNote, when: { $0.type == .secureNote } ) appendImportedCredentialCountIfAny( importedCredentialsCount: &importedCredentialsCount, ciphers: ciphers, - localizedType: Localizations.sshKeys, + type: .sshKey, when: { $0.type == .sshKey } ) + + await onProgress(1.0) + return importedCredentialsCount #else return [] @@ -135,12 +147,12 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { /// - Parameters: /// - importedCredentialsCount: The array to update. /// - ciphers: The ciphers to count. - /// - localizedType: The localized type to add if the condition is met. + /// - type: The type to add if the condition is met. /// - when: The filter to apply to count the ciphers. private func appendImportedCredentialCountIfAny( importedCredentialsCount: inout [ImportedCredentialsResult], ciphers: [Cipher], - localizedType: String, + type: ImportedCredentialsResult.ImportedCredentialType, when: (Cipher) -> Bool ) { let count = ciphers.count { when($0) } @@ -148,8 +160,8 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { importedCredentialsCount .append( ImportedCredentialsResult( - localizedType: localizedType, - count: count + count: count, + type: type ) ) } diff --git a/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift index 6be4e7765..4913735ca 100644 --- a/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift +++ b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift @@ -7,7 +7,10 @@ class MockImportCiphersRepository: ImportCiphersRepository { var importCiphersResult = InvocationMockerWithThrowingResult() .withResult([]) - func importCiphers(credentialImportToken: UUID) async throws -> [ImportedCredentialsResult] { + func importCiphers( + credentialImportToken: UUID, + onProgress: @MainActor (_ progress: Double) -> Void + ) async throws -> [ImportedCredentialsResult] { try importCiphersResult.invoke(param: credentialImportToken) } } diff --git a/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift index 3d4f2e588..02dc311ce 100644 --- a/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift +++ b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResult.swift @@ -1,14 +1,48 @@ /// Represents the result of imported credentials of one type. struct ImportedCredentialsResult: Equatable, Sendable { - /// The credential type imported. - let localizedType: String + // MARK: Types + + /// The available imported credential type. + enum ImportedCredentialType: String, Equatable, Sendable { + case card = "Card" + case identity = "Identity" + case passkey = "Passkey" + case password = "Password" + case secureNote = "SecureNote" + case sshKey = "SSHKey" + } + + // MARK: Properties /// The number of credentials imported for the type let count: Int + + /// The localized type in plural. + var localizedTypePlural: String { + return switch type { + case .card: + Localizations.cards + case .identity: + Localizations.identities + case .passkey: + Localizations.passkeys + case .password: + Localizations.passwords + case .secureNote: + Localizations.secureNotes + case .sshKey: + Localizations.sshKeys + } + } + + /// The credential type imported. + let type: ImportedCredentialType } +// MARK: - Identifiable + extension ImportedCredentialsResult: Identifiable { - public var id: String { - localizedType + public var id: ImportedCredentialType { + type } } diff --git a/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResultTests.swift b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResultTests.swift new file mode 100644 index 000000000..b04ebec7a --- /dev/null +++ b/BitwardenShared/Core/Tools/Utilities/ImportedCredentialsResultTests.swift @@ -0,0 +1,42 @@ +import XCTest + +@testable import BitwardenShared + +// MARK: - ImportedCredentialsResultTests + +class ImportedCredentialsResultTests: BitwardenTestCase { + // MARK: Properties + + var subject: ImportedCredentialsResult! + + // MARK: Setup & Teardown + + override func tearDown() { + super.tearDown() + + subject = nil + } + + // MARK: Tests + + /// `localizedTypePlural` returns the localized string for the type in plural. + func test_localizedTypePlural() { + subject = ImportedCredentialsResult(count: 1, type: .card) + XCTAssertEqual(subject.localizedTypePlural, Localizations.cards) + + subject = ImportedCredentialsResult(count: 1, type: .identity) + XCTAssertEqual(subject.localizedTypePlural, Localizations.identities) + + subject = ImportedCredentialsResult(count: 1, type: .passkey) + XCTAssertEqual(subject.localizedTypePlural, Localizations.passkeys) + + subject = ImportedCredentialsResult(count: 1, type: .password) + XCTAssertEqual(subject.localizedTypePlural, Localizations.passwords) + + subject = ImportedCredentialsResult(count: 1, type: .secureNote) + XCTAssertEqual(subject.localizedTypePlural, Localizations.secureNotes) + + subject = ImportedCredentialsResult(count: 1, type: .sshKey) + XCTAssertEqual(subject.localizedTypePlural, Localizations.sshKeys) + } +} diff --git a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift index 79e9a02e2..3f4f8ebb0 100644 --- a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift +++ b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift @@ -59,6 +59,7 @@ struct PageHeaderView: View { text.styleGuide(.hugeTitle, weight: .bold) } } + .accessibilityLabel("HeaderTitle") Text(LocalizedStringKey(message)) .apply { text in @@ -69,6 +70,7 @@ struct PageHeaderView: View { text.styleGuide(.title2) } } + .accessibilityLabel("HeaderMessage") } } .foregroundStyle(Asset.Colors.textPrimary.swiftUIColor) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index 771e87fb4..ee62b0370 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -84,13 +84,15 @@ class ImportCXPProcessor: StateProcessor state.status = .importing do { - let result = try await services.importCiphersRepository.importCiphers( + let results = try await services.importCiphersRepository.importCiphers( credentialImportToken: credentialImportToken - ) + ) { progress in + state.progress = progress + } state.status = .success( - totalImportedCredentials: result.map(\.count).reduce(0, +), - credentialsByTypeCount: result + totalImportedCredentials: results.map(\.count).reduce(0, +), + importedResults: results ) } catch ImportCiphersRepositoryError.noDataFound { state.status = .failure(message: "No data found to import.") diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index ff75b83e2..5a7709f70 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -16,7 +16,7 @@ struct ImportCXPState: Equatable, Sendable { case importing /// The import flow succeded. - case success(totalImportedCredentials: Int, credentialsByTypeCount: [ImportedCredentialsResult]) + case success(totalImportedCredentials: Int, importedResults: [ImportedCredentialsResult]) /// The import flow failed. case failure(message: String) @@ -70,6 +70,9 @@ struct ImportCXPState: Equatable, Sendable { } } + /// The progress of importing credentials. + var progress = 0.0 + /// The title to display on the page header. var title: String { return switch status { diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift index b15521c0b..5a0ff0c57 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPStateTests.swift @@ -33,7 +33,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertEqual(subject.mainButtonTitle, "") - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertEqual(subject.mainButtonTitle, Localizations.showVault) subject.status = .failure(message: "") @@ -48,7 +48,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertEqual(subject.mainIcon.name, Asset.Images.Illustrations.import.name) - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertEqual(subject.mainIcon.name, Asset.Images.checkCircle24.name) subject.status = .failure(message: "") @@ -63,7 +63,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertEqual(subject.message, "") - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertEqual(subject.message, Localizations.itemsSuccessfullyImported(1)) subject.status = .failure(message: "Something went wrong") @@ -78,7 +78,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertEqual(subject.title, Localizations.importingEllipsis) - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertEqual(subject.title, Localizations.importSuccessful) subject.status = .failure(message: "Something went wrong") @@ -96,7 +96,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertFalse(subject.showCancelButton) - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertFalse(subject.showCancelButton) subject.status = .failure(message: "Something went wrong") @@ -111,7 +111,7 @@ class ImportCXPStateTests: BitwardenTestCase { subject.status = .importing XCTAssertFalse(subject.showMainButton) - subject.status = .success(totalImportedCredentials: 1, credentialsByTypeCount: []) + subject.status = .success(totalImportedCredentials: 1, importedResults: []) XCTAssertTrue(subject.showMainButton) subject.status = .failure(message: "Something went wrong") diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index 3a46b0b3a..f1b3d8260 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -25,18 +25,15 @@ struct ImportCXPView: View { case .start: EmptyView() case .importing: - ProgressView() + ProgressView(value: store.state.progress) + .tint(Asset.Colors.tintPrimary.swiftUIColor) .frame(maxWidth: .infinity) - case let .success(_, countByType): + .scaleEffect(x: 1, y: 3, anchor: .center) + .accessibilityIdentifier("ImportProgress") + case let .success(_, results): VStack(spacing: 16) { - ForEach(countByType) { type in - HStack { - Text(type.localizedType) - .styleGuide(.body) - Spacer() - Text("\(type.count)") - .styleGuide(.body) - } + ForEach(results) { result in + importedTypeRow(result: result) } } .padding(.horizontal, 20) @@ -85,6 +82,19 @@ struct ImportCXPView: View { } // MARK: Private + + /// The row for an imported type result. + @ViewBuilder + private func importedTypeRow(result: ImportedCredentialsResult) -> some View { + HStack { + Text(result.localizedTypePlural) + .styleGuide(.body) + Spacer() + Text("\(result.count)") + .styleGuide(.body) + .accessibilityIdentifier("\(result.type)ImportTotal") + } + } } // MARK: - Previews @@ -96,8 +106,16 @@ struct ImportCXPView: View { } #Preview("Importing") { - ImportCXPView(store: Store(processor: StateProcessor(state: ImportCXPState(status: .importing)))) - .navStackWrapped + ImportCXPView( + store: Store( + processor: StateProcessor( + state: ImportCXPState( + progress: 0.3, + status: .importing + ) + ) + ) + ).navStackWrapped } #Preview("Success") { @@ -107,10 +125,10 @@ struct ImportCXPView: View { state: ImportCXPState( status: .success( totalImportedCredentials: 30, - credentialsByTypeCount: [ - ImportedCredentialsResult(localizedType: "Passwords", count: 13), - ImportedCredentialsResult(localizedType: "Passkeys", count: 7), - ImportedCredentialsResult(localizedType: "Cards", count: 10), + importedResults: [ + ImportedCredentialsResult(count: 13, type: .password), + ImportedCredentialsResult(count: 7, type: .passkey), + ImportedCredentialsResult(count: 10, type: .card), ] ) ) From dc4e0e60cd5589aa655fa6d92969e3ccf9f74ea3 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Wed, 27 Nov 2024 17:39:33 -0300 Subject: [PATCH 11/13] PM-14800 Improved and cleaned import logic on the processor. Also added a `ProgressDelegate` to report progress to improve maintenance and unit testing and added a bunch of unit tests. --- .../TestHelpers/ServiceContainer+Mocks.swift | 2 +- .../ImportCiphersRepository.swift | 12 +- .../MockImportCiphersRepository.swift | 2 +- .../TestHelpers/MockProgressDelegate.swift | 9 + .../Utilities/ProgressDelegate.swift | 9 + .../ImportCXP/ImportCXPProcessor.swift | 24 +- .../ImportCXP/ImportCXPProcessorTests.swift | 325 ++++++++++++++++++ .../ImportCXP/ImportCXP/ImportCXPView.swift | 1 + GlobalTestHelpers/MockAppModule.swift | 4 +- 9 files changed, 370 insertions(+), 18 deletions(-) create mode 100644 BitwardenShared/UI/Platform/Application/TestHelpers/MockProgressDelegate.swift create mode 100644 BitwardenShared/UI/Platform/Application/Utilities/ProgressDelegate.swift create mode 100644 BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessorTests.swift diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift index 670e7f49e..3a37be721 100644 --- a/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift +++ b/BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift @@ -3,7 +3,7 @@ import Networking @testable import BitwardenShared -extension ServiceContainer { +extension ServiceContainer { // swiftlint:disable:this function_body_length static func withMocks( application: Application? = nil, appSettingsStore: AppSettingsStore = MockAppSettingsStore(), diff --git a/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift index a5928a8f2..bff4676bf 100644 --- a/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift +++ b/BitwardenShared/Core/Tools/Repositories/ImportCiphersRepository.swift @@ -8,13 +8,13 @@ protocol ImportCiphersRepository: AnyObject { /// Performs an API request to import ciphers in the vault. /// - Parameters: /// - credentialImportToken: The token used in `ASCredentialImportManager` to get the credentials to import. - /// - onProgress: Closure to update progress in the process. + /// - progressDelegate: Delegate to update progress. /// - Returns: A dictionary containing the localized cipher type (key) and count (value) of that type /// that was imported, e.g. ["Passwords": 3, "Cards": 2]. @available(iOS 18.2, *) func importCiphers( credentialImportToken: UUID, - onProgress: @MainActor (_ progress: Double) -> Void + progressDelegate: ProgressDelegate ) async throws -> [ImportedCredentialsResult] } @@ -60,7 +60,7 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { @available(iOS 18.2, *) func importCiphers( // swiftlint:disable:this function_body_length credentialImportToken: UUID, - onProgress: @MainActor (_ progress: Double) -> Void + progressDelegate: ProgressDelegate ) async throws -> [ImportedCredentialsResult] { #if compiler(>=6.0.3) @@ -78,7 +78,7 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { let ciphers = try await clientService.exporters().importCxf(payload: accountJsonString) - await onProgress(0.3) + await progressDelegate.report(progress: 0.3) _ = try await importCiphersService .importCiphers( @@ -87,7 +87,7 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { folderRelationships: [] ) - await onProgress(0.8) + await progressDelegate.report(progress: 0.8) try await syncService.fetchSync(forceSync: true) @@ -133,7 +133,7 @@ extension DefaultImportCiphersRepository: ImportCiphersRepository { when: { $0.type == .sshKey } ) - await onProgress(1.0) + await progressDelegate.report(progress: 1.0) return importedCredentialsCount #else diff --git a/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift index 4913735ca..f7962a540 100644 --- a/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift +++ b/BitwardenShared/Core/Tools/Repositories/TestHelpers/MockImportCiphersRepository.swift @@ -9,7 +9,7 @@ class MockImportCiphersRepository: ImportCiphersRepository { func importCiphers( credentialImportToken: UUID, - onProgress: @MainActor (_ progress: Double) -> Void + progressDelegate: ProgressDelegate ) async throws -> [ImportedCredentialsResult] { try importCiphersResult.invoke(param: credentialImportToken) } diff --git a/BitwardenShared/UI/Platform/Application/TestHelpers/MockProgressDelegate.swift b/BitwardenShared/UI/Platform/Application/TestHelpers/MockProgressDelegate.swift new file mode 100644 index 000000000..fd723fd8b --- /dev/null +++ b/BitwardenShared/UI/Platform/Application/TestHelpers/MockProgressDelegate.swift @@ -0,0 +1,9 @@ +@testable import BitwardenShared + +class MockProgressDelegate: ProgressDelegate { + var progress = 0.0 + + func report(progress: Double) { + self.progress = progress + } +} diff --git a/BitwardenShared/UI/Platform/Application/Utilities/ProgressDelegate.swift b/BitwardenShared/UI/Platform/Application/Utilities/ProgressDelegate.swift new file mode 100644 index 000000000..a55965bb1 --- /dev/null +++ b/BitwardenShared/UI/Platform/Application/Utilities/ProgressDelegate.swift @@ -0,0 +1,9 @@ +import Foundation + +/// A protocol to report progress. +@MainActor +protocol ProgressDelegate: AnyObject { + /// Reports progress in an operation. + /// - Parameter progress: The progress being made in an operation. + func report(progress: Double) +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift index ee62b0370..0e54a6d1e 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessor.swift @@ -49,11 +49,14 @@ class ImportCXPProcessor: StateProcessor case .cancel: cancelWithConfirmation() case .mainButtonTapped: - guard case .success = state.status else { + switch state.status { + case .failure, .start: await startImport() - return + case .importing: + break + case .success: + coordinator.navigate(to: .dismiss) } - coordinator.navigate(to: .dismiss) } } @@ -85,10 +88,9 @@ class ImportCXPProcessor: StateProcessor do { let results = try await services.importCiphersRepository.importCiphers( - credentialImportToken: credentialImportToken - ) { progress in - state.progress = progress - } + credentialImportToken: credentialImportToken, + progressDelegate: self + ) state.status = .success( totalImportedCredentials: results.map(\.count).reduce(0, +), @@ -98,8 +100,6 @@ class ImportCXPProcessor: StateProcessor state.status = .failure(message: "No data found to import.") } catch ImportCiphersRepositoryError.dataEncodingFailed { state.status = .failure(message: "Import data encoding failed.") - } catch let BitwardenSdk.BitwardenError.E(message) { - print(message) } catch { state.status = .failure(message: Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) services.errorReporter.log(error: error) @@ -121,3 +121,9 @@ class ImportCXPProcessor: StateProcessor }) } } + +extension ImportCXPProcessor: ProgressDelegate { + func report(progress: Double) { + state.progress = progress + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessorTests.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessorTests.swift new file mode 100644 index 000000000..2bb70bc31 --- /dev/null +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPProcessorTests.swift @@ -0,0 +1,325 @@ +import XCTest + +@testable import BitwardenShared + +// MARK: - ImportCXPProcessorTests + +class ImportCXPProcessorTests: BitwardenTestCase { + // MARK: Properties + + var configService: MockConfigService! + var coordinator: MockCoordinator! + var errorReporter: MockErrorReporter! + var importCiphersRepository: MockImportCiphersRepository! + var state: ImportCXPState! + var stateService: MockStateService! + var subject: ImportCXPProcessor! + + // MARK: Setup & Teardown + + override func setUp() { + super.setUp() + + configService = MockConfigService() + coordinator = MockCoordinator() + errorReporter = MockErrorReporter() + importCiphersRepository = MockImportCiphersRepository() + state = ImportCXPState() + stateService = MockStateService() + subject = ImportCXPProcessor( + coordinator: coordinator.asAnyCoordinator(), + services: ServiceContainer.withMocks( + configService: configService, + errorReporter: errorReporter, + importCiphersRepository: importCiphersRepository, + stateService: stateService + ), + state: state + ) + } + + override func tearDown() { + super.tearDown() + + configService = nil + coordinator = nil + errorReporter = nil + importCiphersRepository = nil + state = nil + stateService = nil + subject = nil + } + + // MARK: Tests + + /// `perform(_:)` with `.appeared` sets the status as `.failure` with a message + /// when the feature flag `.cxpImportMobile` is not enabled. + @MainActor + func test_perform_appearedNoFeatureFlag() async { + await subject.perform(.appeared) + guard case let .failure(message) = subject.state.status else { + XCTFail("Status should be failure") + return + } + XCTAssertEqual(message, Localizations.importingFromAnotherProviderIsNotAvailableForThisDevice) + } + + /// `perform(_:)` with `.appeared` sets the status as `.failure` with a message + /// when the feature flag `.cxpImportMobile` is not enabled. + @MainActor + func test_perform_appearedFeatureFlagEnabled() async throws { + guard #available(iOS 18.2, *) else { + throw XCTSkip("CXP Import feature is not available on this device") + } + + configService.featureFlagsBool[.cxpImportMobile] = true + await subject.perform(.appeared) + if case .failure = subject.state.status { + XCTFail("Status shouldn't be failure when CXP import is enabled") + } + } + + /// `perform(_:)` with `.cancel` with feature available shows confirmation and navigates to dismiss. + @MainActor + func test_perform_cancel() async throws { + subject.state.isFeatureUnvailable = false + let task = Task { + await subject.perform(.cancel) + } + defer { task.cancel() } + + try await waitForAsync { [weak self] in + guard let self else { return true } + return !coordinator.alertShown.isEmpty + } + + let confirmCancelAlert = try XCTUnwrap(coordinator.alertShown.first) + try await confirmCancelAlert.tapAction(title: Localizations.yes) + + try await waitForAsync { [weak self] in + guard let self else { return true } + return !coordinator.routes.isEmpty + } + + XCTAssertEqual(.dismiss, coordinator.routes.last) + } + + /// `perform(_:)` with `.cancel` with feature available shows confirmation and + /// doesn't navigate to dismiss if the user cancels the confirmation dialog. + @MainActor + func test_perform_cancelNoConfirmation() async throws { + subject.state.isFeatureUnvailable = false + let task = Task { + await subject.perform(.cancel) + } + defer { task.cancel() } + + try await waitForAsync { [weak self] in + guard let self else { return true } + return !coordinator.alertShown.isEmpty + } + + let confirmCancelAlert = try XCTUnwrap(coordinator.alertShown.first) + try await confirmCancelAlert.tapAction(title: Localizations.no) + + XCTAssertTrue(coordinator.routes.isEmpty) + } + + /// `perform(_:)` with `.cancel` with feature unavailable navigates to dismiss. + @MainActor + func test_perform_cancelFeatureUnavailable() async throws { + subject.state.isFeatureUnvailable = true + let task = Task { + await subject.perform(.cancel) + } + defer { task.cancel() } + + try await waitForAsync { [weak self] in + guard let self else { return true } + return !coordinator.routes.isEmpty + } + + XCTAssertEqual(.dismiss, coordinator.routes.last) + } + + /// `perform(_:)` with `.mainButtonTapped` with `.start` status. + @MainActor + func test_perform_mainButtonTappedStart() async throws { + subject.state.status = .start + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + try await perform_mainButtonTapped_startImport() + } + + /// `perform(_:)` with `.mainButtonTapped` with `.failure` status. + @MainActor + func test_perform_mainButtonTappedFailure() async throws { + subject.state.status = .failure(message: "Error") + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + try await perform_mainButtonTapped_startImport() + } + + /// `perform(_:)` with `.mainButtonTapped` with `.success` status which dismisses the view. + @MainActor + func test_perform_mainButtonTappedSuccess() async throws { + subject.state.status = .success(totalImportedCredentials: 10, importedResults: []) + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + + await subject.perform(.mainButtonTapped) + + XCTAssertEqual(coordinator.routes.last, .dismiss) + } + + /// `perform(_:)` with `.mainButtonTapped` with `.start` status but no data found. + @MainActor + func test_perform_mainButtonTappedStartNoDataFound() async throws { + guard try checkCompiler() else { + return + } + + subject.state.status = .start + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + + importCiphersRepository.importCiphersResult.withVerification { _ in + self.subject.state.status == .importing + }.throwing(ImportCiphersRepositoryError.noDataFound) + + await subject.perform(.mainButtonTapped) + + guard checkAlertShownWhenNotInCorrectIOSVersion() else { + return + } + + guard case let .failure(message) = subject.state.status else { + XCTFail("Importing status is not failure.") + return + } + + XCTAssertEqual(message, "No data found to import.") + } + + /// `perform(_:)` with `.mainButtonTapped` with `.start` status but data encoding failed. + @MainActor + func test_perform_mainButtonTappedStartDataEncodingFailed() async throws { + guard try checkCompiler() else { + return + } + + subject.state.status = .start + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + + importCiphersRepository.importCiphersResult.withVerification { _ in + self.subject.state.status == .importing + }.throwing(ImportCiphersRepositoryError.dataEncodingFailed) + + await subject.perform(.mainButtonTapped) + + guard checkAlertShownWhenNotInCorrectIOSVersion() else { + return + } + + guard case let .failure(message) = subject.state.status else { + XCTFail("Importing status is not failure.") + return + } + + XCTAssertEqual(message, "Import data encoding failed.") + } + + /// `perform(_:)` with `.mainButtonTapped` with `.start` status but throws error. + @MainActor + func test_perform_mainButtonTappedStartThrowing() async throws { + guard try checkCompiler() else { + return + } + + subject.state.status = .start + subject.state.credentialImportToken = UUID(uuidString: "e8f3b381-aac2-4379-87fe-14fac61079ec") + + importCiphersRepository.importCiphersResult.withVerification { _ in + self.subject.state.status == .importing + }.throwing(BitwardenTestError.example) + + await subject.perform(.mainButtonTapped) + + guard checkAlertShownWhenNotInCorrectIOSVersion() else { + return + } + + guard case let .failure(message) = subject.state.status else { + XCTFail("Importing status is not failure.") + return + } + + XCTAssertEqual(message, Localizations.thereWasAnIssueImportingAllOfYourPasswordsNoDataWasDeleted) + XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example]) + } + + /// `report(progress:)` updates the progress in the state. + @MainActor + func test_report() async throws { + subject.report(progress: 0.6) + XCTAssertEqual(subject.state.progress, 0.6) + } + + // MARK: Private + + /// Performs `.perform(.mainButtonTapped)` to start import and checks everything went good. + @MainActor + private func perform_mainButtonTapped_startImport() async throws { + guard try checkCompiler() else { + return + } + + let expectedResults = [ + ImportedCredentialsResult(count: 12, type: .password), + ImportedCredentialsResult(count: 7, type: .passkey), + ImportedCredentialsResult(count: 11, type: .card), + ] + importCiphersRepository.importCiphersResult.withVerification { _ in + self.subject.state.status == .importing + }.withResult(expectedResults) + + await subject.perform(.mainButtonTapped) + + guard checkAlertShownWhenNotInCorrectIOSVersion() else { + return + } + + guard case let .success(total, results) = subject.state.status else { + XCTFail("Importing status is not success.") + return + } + + XCTAssertEqual(total, 30) + XCTAssertEqual(results, expectedResults) + } + + /// Checks whether the appropriate compiler is being used to have the code available. + /// - Returns: `true` if the compiler is correct, `false`otherwise. + private func checkCompiler() throws -> Bool { + #if compiler(>=6.0.3) + return true + #else + throw XCTSkip("CXP Import works only from 6.0.3 compiler.") + #endif + } + + /// Checks whether the alert is shown when not in the correct iOS version for CXP Import to work. + @MainActor + private func checkAlertShownWhenNotInCorrectIOSVersion() -> Bool { + guard #available(iOS 18.2, *) else { + XCTAssertEqual( + coordinator.alertShown, + [ + .defaultAlert( + title: Localizations.importError, + message: Localizations.importingFromAnotherProviderIsNotAvailableForThisDevice + ), + ] + ) + return false + } + + return true + } +} diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index f1b3d8260..e64c9b95f 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -150,4 +150,5 @@ struct ImportCXPView: View { ) ).navStackWrapped } + #endif diff --git a/GlobalTestHelpers/MockAppModule.swift b/GlobalTestHelpers/MockAppModule.swift index e46a7d925..4b1eda038 100644 --- a/GlobalTestHelpers/MockAppModule.swift +++ b/GlobalTestHelpers/MockAppModule.swift @@ -90,7 +90,9 @@ class MockAppModule: generatorCoordinator.asAnyCoordinator() } - func makeImportCXPCoordinator(stackNavigator: any BitwardenShared.StackNavigator) -> BitwardenShared.AnyCoordinator { + func makeImportCXPCoordinator( + stackNavigator: any StackNavigator + ) -> AnyCoordinator { importCXPCoordinator.asAnyCoordinator() } From b64c5dc8b7c21225c95f4bdd00169d5f37e58994 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 6 Dec 2024 16:12:07 -0300 Subject: [PATCH 12/13] PM-14800 Updated import CXP icon. --- .../Icons/file-upload24.imageset/Contents.json | 16 ++++++++++++++++ .../file-upload24.imageset/file_upload.pdf | Bin 0 -> 4173 bytes .../ImportCXP/ImportCXP/ImportCXPState.swift | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/Contents.json create mode 100644 BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/file_upload.pdf diff --git a/BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/Contents.json b/BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/Contents.json new file mode 100644 index 000000000..2f8cc19e3 --- /dev/null +++ b/BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/Contents.json @@ -0,0 +1,16 @@ +{ + "images" : [ + { + "filename" : "file_upload.pdf", + "idiom" : "universal" + } + ], + "info" : { + "author" : "xcode", + "version" : 1 + }, + "properties" : { + "preserves-vector-representation" : true, + "template-rendering-intent" : "template" + } +} diff --git a/BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/file_upload.pdf b/BitwardenShared/UI/Platform/Application/Support/Images.xcassets/Icons/file-upload24.imageset/file_upload.pdf new file mode 100644 index 0000000000000000000000000000000000000000..f9799ec884a5f7314b8227a43fc725ff878835a6 GIT binary patch literal 4173 zcmY!laBBLODsuM(055K%S=uUa(4ns0fD|- zCP)rQ=K!T$Qj_yjQlT4d%BhXX{8 z`mYYUVAa7VD`V8@eB}~XSz(5@h^s*e>+0ZHb)301Dw;us!r_`fVut&kJ6a zwrRy89j}wfV-q z`B%fIAC-9(cqUO;O`8H}}2hjNF2A`nPU5F9UpJbj7vP@-DKd)Cl_g1QPexvqZc5%(DJzS}IDbR2R zvOrPEWdx5?P@KR-jp3rv3i_VT&Q6KNsVNGe_zzNug{v}ws{*A5+>stqSpZBV5&lV8 zsmUd%3BeR(Bvi<~D8IA-oU|YsziUp+>=a&{Grxt^)aL&)kFA6S5OiqPx4HZCs4g#h`P-29b;^dTHp%ATL zpl1LE2!@dfObEe>1!_V{#*DpFy!&Jod0d~bS#)fo+#!jcy*+CdF41VduDf+py{7mI zqbWun*B403{@MBO!#<{?Ci|4{wojk`NAIyi*v|_yv>mL@^*>1aFZXZG{KhDqWv#ay zE%}z0)UaIgSamjIal%?3@2)VR>%UUU&%LX?EWOWsM$%ffQkOlOb}UgnF8HW$$r=8R z3d?QFZhmTVcsFNuX4?VYlNT;M&b<(|$n4F!486+KQkL-R&jo_S?!2^7`c@jXA-eBy zf~l9{?cnmXTOJD^r3r2J*j{v2`r{?DveE;}m9uT;3tut41+x!Lmbo)sG_Pt?Z! z?aD9SwDfa}{J+Q5B6aJvSABUqGmz{1JAUgOKKv$U1&^%#XI0yN|Kaui50Z6l`YYHE zwRimJes90NdDYjxn#ZL}Jf^G^oONwQmR$egg-29Qu8|X)x>1|ykZP%MH*2oEnYd-M ztJCAHI?Do|N}lXgw`txS?pR$E?EF*LC{%1p*Hz!EJNRZuJv}BXwvbQNYxUL_%%aPT z*6Ll~XSt*|_lD%Go)g@LEaARfCA*8(WF6xP@110^T1LM^Oeo}s+s2bC6uxj#r8x-w*u;=^XKTW{EIV07-%} z9kd8=ttfF1E&(;kK?R4aA(#sGO)Sm^vmhl2hzn7VXjNfl>ANd{N+M|af>jAZP#@SX zbId8oNF;1D%()27NGxb^2lojs-Bqj2J@7Uqs_g z-`P;XIhYGXB^PrQa)ClJy_icM7~n4XIC3T8UTwAG$9joA!8$R zV8TXGWo&GQDTLt&ps#@?2&zUi6GV8G6eVWnq!w|3(t@W8FePXd=jWzsDrjV;XhM>M meo%fsFfG8#3H{*As#H+4fpb%0Q3*IAjm-@Vxl~nM{oMfK5mz4o literal 0 HcmV?d00001 diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift index 5a7709f70..b0a538e99 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPState.swift @@ -48,7 +48,7 @@ struct ImportCXPState: Equatable, Sendable { var mainIcon: ImageAsset { return switch status { case .importing, .start: - Asset.Images.Illustrations.import + Asset.Images.fileUpload24 case .success: Asset.Images.checkCircle24 case .failure: From db237c20bbe4d09ba2177335d9320ea22f947674 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 6 Dec 2024 18:25:41 -0300 Subject: [PATCH 13/13] PM-14800 Improved accessibility in import CXP flow. --- .../UI/Platform/Application/Views/PageHeaderView.swift | 4 ++-- .../UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift index 3f4f8ebb0..f93693ef1 100644 --- a/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift +++ b/BitwardenShared/UI/Platform/Application/Views/PageHeaderView.swift @@ -59,7 +59,7 @@ struct PageHeaderView: View { text.styleGuide(.hugeTitle, weight: .bold) } } - .accessibilityLabel("HeaderTitle") + .accessibilityIdentifier("HeaderTitle") Text(LocalizedStringKey(message)) .apply { text in @@ -70,7 +70,7 @@ struct PageHeaderView: View { text.styleGuide(.title2) } } - .accessibilityLabel("HeaderMessage") + .accessibilityIdentifier("HeaderMessage") } } .foregroundStyle(Asset.Colors.textPrimary.swiftUIColor) diff --git a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift index e64c9b95f..711332c31 100644 --- a/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift +++ b/BitwardenShared/UI/Tools/ImportCXP/ImportCXP/ImportCXPView.swift @@ -16,7 +16,7 @@ struct ImportCXPView: View { Group { VStack(spacing: 16) { PageHeaderView( - image: store.state.mainIcon.swiftUIImage, + image: Image(decorative: store.state.mainIcon), title: store.state.title, message: store.state.message, style: .largeWithTintedIcon