From 707ccccc8197b610c953138187f58774dcbbc2b5 Mon Sep 17 00:00:00 2001 From: Kaede Akino Date: Tue, 29 Sep 2026 15:50:43 +0800 Subject: [PATCH] fix(keychain): unify mobile hardware key editors --- FrameworkTests/SecurityKeyEditorTests.swift | 40 +++++++ .../SecurityKeyEditorModel.swift | 33 +++--- .../SecurityKeyEditorForm.swift | 20 +--- .../SecurityKeyEditorSections.swift | 39 +++++++ TFI/Features/Keychain/MobileKeyEditor.swift | 1 + TFI/Features/Keychain/SecurityKeyEditor.swift | 25 +++-- ...obileHardwareKeyEditorRenderingTests.swift | 101 ++++++++++++++++++ 7 files changed, 217 insertions(+), 42 deletions(-) create mode 100644 Frameworks/TermindPresentation/SecurityKeyEditorSections.swift create mode 100644 TFITests/MobileHardwareKeyEditorRenderingTests.swift diff --git a/FrameworkTests/SecurityKeyEditorTests.swift b/FrameworkTests/SecurityKeyEditorTests.swift index 8c9ed417..6c3c766d 100644 --- a/FrameworkTests/SecurityKeyEditorTests.swift +++ b/FrameworkTests/SecurityKeyEditorTests.swift @@ -74,6 +74,46 @@ struct SecurityKeyEditorTests { #expect(!persisted) } + @Test func dirtyStateTracksEachEditableFieldAndRevertingChanges() { + let existing = SSHKeyRef(label: "Key", securityKey: credential) + let model = SecurityKeyEditorModel( + existing: existing, exportReference: { _ in "reference" }, validate: { _ in }, persist: { _ in }) + model.load() + #expect(!model.isDirty) + model.label = "Renamed" + #expect(model.isDirty) + model.label = existing.label + #expect(!model.isDirty) + model.colorTag = .purple + #expect(model.isDirty) + model.colorTag = existing.colorTag + #expect(!model.isDirty) + model.verification = .required + #expect(model.isDirty) + model.verification = credential.userVerification + #expect(!model.isDirty) + } + + @Test func nativeEditorReceivesSaveFailuresAndCanRetryTheSameDraft() async throws { + var shouldFail = true + var persisted: SSHKeyRef? + let model = SecurityKeyEditorModel( + existing: SSHKeyRef(label: "Key", securityKey: credential), + exportReference: { _ in "reference" }, validate: { _ in }, + persist: { + if shouldFail { throw EditFailure() } + persisted = $0 + }) + model.load() + model.label = "Renamed" + await #expect(throws: EditFailure.self) { try await model.saveChanges() } + #expect(persisted == nil && model.isDirty && model.canSave) + #expect(model.label == "Renamed") + shouldFail = false + try await model.saveChanges() + #expect(persisted?.label == "Renamed") + } + private struct EditFailure: LocalizedError { var errorDescription: String? { "Fixture edit failure" } } diff --git a/Frameworks/TermindApplication/FeatureModels/SecurityKeyEditorModel.swift b/Frameworks/TermindApplication/FeatureModels/SecurityKeyEditorModel.swift index 20a1bb84..f205445f 100644 --- a/Frameworks/TermindApplication/FeatureModels/SecurityKeyEditorModel.swift +++ b/Frameworks/TermindApplication/FeatureModels/SecurityKeyEditorModel.swift @@ -14,6 +14,10 @@ public final class SecurityKeyEditorModel { public private(set) var isSaving = false public var canSave: Bool { originalCredential != nil && !isSaving } public let existing: SSHKeyRef + public var isDirty: Bool { + label != existing.label || colorTag != existing.colorTag + || verification != (originalCredential?.userVerification ?? .preferred) + } private var originalCredential: SecurityKeyCredential? private let exportReference: (SecurityKeyCredential) throws -> String @@ -43,21 +47,26 @@ public final class SecurityKeyEditorModel { } catch { self.error = error.localizedDescription } } - public func save() async -> Bool { - guard !isSaving else { return false } + /// Throws so native editor containers can retain the draft and present save failures. + public func saveChanges() async throws { + guard !isSaving else { throw CancellationError() } isSaving = true - error = nil defer { isSaving = false } + guard var credential = originalCredential else { throw TermindSSHError.missingCredential } + credential.userVerification = verification + try validate(credential) + try Task.checkCancellation() + var key = existing + key.label = label.trimmingCharacters(in: .whitespacesAndNewlines) + key.colorTag = colorTag + key.securityKey = credential + try persist(key) + } + + public func save() async -> Bool { + error = nil do { - guard var credential = originalCredential else { throw TermindSSHError.missingCredential } - credential.userVerification = verification - try validate(credential) - try Task.checkCancellation() - var key = existing - key.label = label.trimmingCharacters(in: .whitespacesAndNewlines) - key.colorTag = colorTag - key.securityKey = credential - try persist(key) + try await saveChanges() return true } catch is CancellationError { } catch { diff --git a/Frameworks/TermindPresentation/SecurityKeyEditorForm.swift b/Frameworks/TermindPresentation/SecurityKeyEditorForm.swift index 15ed524c..e1e4a522 100644 --- a/Frameworks/TermindPresentation/SecurityKeyEditorForm.swift +++ b/Frameworks/TermindPresentation/SecurityKeyEditorForm.swift @@ -23,25 +23,7 @@ public struct SecurityKeyEditorForm: View { public var body: some View { VStack(spacing: 0) { Form { - Section { - Label(AppStrings.SecurityKey.title.swiftUIKey, systemImage: "key.radiowaves.forward.fill").font( - .headline) - VaultNameField(text: $model.label, prompt: Text(AppStrings.SecurityKey.title)) - VaultColorField(selection: $model.colorTag) - } - Section(AppStrings.SecurityKey.reference.swiftUIKey) { - LabeledContent(AppStrings.Keychain.storeIn.swiftUIKey) { - Label(AppStrings.SecurityKey.title.swiftUIKey, systemImage: "key.radiowaves.forward.fill") - } - WebAuthnVerificationPicker(selection: $model.verification) - if !model.reference.isEmpty { - Text(model.reference).font(.caption.monospaced()).textSelection(.enabled) - } - } - Section(AppStrings.Keychain.publicKey.swiftUIKey) { - Text(model.existing.publicKey).font(.caption.monospaced()).textSelection(.enabled) - } - if let error = model.error { Text(error).foregroundStyle(.red) } + SecurityKeyEditorSections(model: model, showsTitle: true) } .formStyle(.grouped) HStack { diff --git a/Frameworks/TermindPresentation/SecurityKeyEditorSections.swift b/Frameworks/TermindPresentation/SecurityKeyEditorSections.swift new file mode 100644 index 00000000..d206e4d2 --- /dev/null +++ b/Frameworks/TermindPresentation/SecurityKeyEditorSections.swift @@ -0,0 +1,39 @@ +import SwiftUI +import TermindApplication +import TermindDomain + +/// Shared fields; each platform owns the surrounding editor and actions. +public struct SecurityKeyEditorSections: View { + @Bindable private var model: SecurityKeyEditorModel + private let showsTitle: Bool + + public init(model: SecurityKeyEditorModel, showsTitle: Bool = false) { + self.model = model + self.showsTitle = showsTitle + } + + public var body: some View { + Section { + if showsTitle { + Label(AppStrings.SecurityKey.title.swiftUIKey, systemImage: "key.radiowaves.forward.fill") + .font(.headline) + } + VaultNameField(text: $model.label, prompt: Text(AppStrings.SecurityKey.title)) + VaultColorField(selection: $model.colorTag) + } + Section(AppStrings.SecurityKey.reference.swiftUIKey) { + LabeledContent(AppStrings.Keychain.storeIn.swiftUIKey) { + Label(AppStrings.SecurityKey.title.swiftUIKey, systemImage: "key.radiowaves.forward.fill") + .labelStyle(.titleAndIcon) + } + WebAuthnVerificationPicker(selection: $model.verification) + if !model.reference.isEmpty { + Text(model.reference).font(.caption.monospaced()).textSelection(.enabled) + } + } + Section(AppStrings.Keychain.publicKey.swiftUIKey) { + Text(model.existing.publicKey).font(.caption.monospaced()).textSelection(.enabled) + } + if let error = model.error { Text(error).foregroundStyle(.red) } + } +} diff --git a/TFI/Features/Keychain/MobileKeyEditor.swift b/TFI/Features/Keychain/MobileKeyEditor.swift index ceea6d5e..b8dcc98c 100644 --- a/TFI/Features/Keychain/MobileKeyEditor.swift +++ b/TFI/Features/Keychain/MobileKeyEditor.swift @@ -152,6 +152,7 @@ struct MobileKeyEditor: View { Section(AppStrings.Keychain.secureEnclave.swiftUIKey) { LabeledContent(AppStrings.Keychain.storeIn.swiftUIKey) { Label(AppStrings.Keychain.secureEnclave.swiftUIKey, systemImage: "checkmark.shield.fill") + .labelStyle(.titleAndIcon) } } } diff --git a/TFI/Features/Keychain/SecurityKeyEditor.swift b/TFI/Features/Keychain/SecurityKeyEditor.swift index 2a839bc5..7abcac5d 100644 --- a/TFI/Features/Keychain/SecurityKeyEditor.swift +++ b/TFI/Features/Keychain/SecurityKeyEditor.swift @@ -5,18 +5,21 @@ import TermindPresentation import TermindRemoteInfrastructure struct SecurityKeyEditor: View { - let credentials: CredentialLibraryModel - let existing: SSHKeyRef - var close: (() -> Void)? = nil - @Environment(\.dismiss) private var dismiss + @State private var model: SecurityKeyEditorModel + + init(credentials: CredentialLibraryModel, existing: SSHKeyRef) { + let model = SecurityKeyEditorModel( + existing: existing, exportReference: WebAuthnSSH.exportReference, validate: WebAuthnSSH.validate, + persist: { try credentials.upsert($0) }) + model.load() + _model = State(initialValue: model) + } var body: some View { - SecurityKeyEditorForm( - existing: existing, exportReference: WebAuthnSSH.exportReference, validate: WebAuthnSSH.validate, - persist: { try credentials.upsert($0) }, close: finish) - } - - private func finish() { - if let close { close() } else { dismiss() } + MobileEditorSheet( + title: String(localized: AppStrings.Keychain.editKey), + isDirty: model.isDirty, canSave: model.canSave, + save: { try await model.saveChanges() }, + content: { SecurityKeyEditorSections(model: model) }) } } diff --git a/TFITests/MobileHardwareKeyEditorRenderingTests.swift b/TFITests/MobileHardwareKeyEditorRenderingTests.swift new file mode 100644 index 00000000..d6e07610 --- /dev/null +++ b/TFITests/MobileHardwareKeyEditorRenderingTests.swift @@ -0,0 +1,101 @@ +import CryptoKit +import SwiftUI +import TermindApplication +import TermindDomain +import TermindPresentation +import TermindRemoteInfrastructure +import Testing +import UIKit + +@testable import TermindIOS + +@Suite( + .enabled( + if: ProcessInfo.processInfo.environment["TERMIND_MOBILE_UI_TESTS"] == "1", + "Set TERMIND_MOBILE_UI_TESTS=1 and disable parallel testing for key-window integration tests"), + .serialized) +@MainActor +struct MobileHardwareKeyEditorRenderingTests { + @Test(arguments: ["en", "zh-Hans"], [false, true]) + func hardwareKeyEditorsUseNativeActionsAndCompactStorageRows(language: String, enclave: Bool) async throws { + let key: SSHKeyRef + if enclave { + key = SSHKeyRef( + label: "Device Key", algorithm: .ecdsa256, + publicKey: "ecdsa-sha2-nistp256 " + String(repeating: "AAAA", count: 40), + fingerprint: "SHA256:fixture", storage: .secureEnclave) + } else { + key = try WebAuthnSSH.keyReference( + SecurityKeyCredential( + credentialID: Data(repeating: 1, count: 64), + publicKeyX963: P256.Signing.PrivateKey().publicKey.x963Representation), label: "Hardware Key") + } + let credentials = CredentialLibraryModel(library: HostLibraryModel(keys: [key])) + let scene = try #require(UIApplication.shared.connectedScenes.compactMap { $0 as? UIWindowScene }.first) + let previous = scene.keyWindow + let window = UIWindow(windowScene: scene) + let controller = UIHostingController( + rootView: HardwareKeyEditorHost(credentials: credentials, key: key, enclave: enclave) + .environment(\.locale, Locale(identifier: language))) + window.rootViewController = controller + window.windowLevel = .alert + 1 + window.makeKeyAndVisible() + defer { + window.isHidden = true + window.rootViewController = nil + previous?.makeKey() + } + try await Task.sleep(for: .milliseconds(800)) + let sheet = try #require(controller.presentedViewController) + sheet.view.layoutIfNeeded() + let navigation = try #require(descendant(UINavigationController.self, in: sheet)) + let item = try #require(navigation.topViewController?.navigationItem) + let buttons = + item.leadingItemGroups.flatMap(\.barButtonItems) + + item.trailingItemGroups.flatMap(\.barButtonItems) + + (item.leftBarButtonItems ?? []) + (item.rightBarButtonItems ?? []) + let names = Set(buttons.compactMap { $0.accessibilityLabel ?? $0.title }) + #expect(!names.isDisjoint(with: ["Save", "保存"])) + #expect(!names.isDisjoint(with: ["Cancel", "取消"])) + let collection = try #require(descendant(UICollectionView.self, in: sheet.view)) + let storage = try #require(collection.cellForItem(at: IndexPath(item: 0, section: 1))) + #expect(storage.bounds.height < 100, "Storage row height: \(storage.bounds.height)") + let image = UIGraphicsImageRenderer(bounds: window.bounds).image { _ in + window.drawHierarchy(in: window.bounds, afterScreenUpdates: true) + } + let data = try #require(image.pngData()) + let directory = FileManager.default.temporaryDirectory.appending(path: "hardware-key-editors") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let device = UIDevice.current.userInterfaceIdiom == .pad ? "ipad" : "iphone" + let output = directory.appending(path: "\(device)-\(enclave ? "enclave" : "security-key")-\(language).png") + try data.write(to: output) + print("Hardware key editor snapshot: \(output.path)") + } + + private func descendant(_ type: T.Type, in controller: UIViewController) -> T? { + if let value = controller as? T { return value } + return controller.children.lazy.compactMap { descendant(type, in: $0) }.first + } + + private func descendant(_ type: T.Type, in view: UIView) -> T? { + if let value = view as? T { return value } + return view.subviews.lazy.compactMap { descendant(type, in: $0) }.first + } +} + +private struct HardwareKeyEditorHost: View { + let credentials: CredentialLibraryModel + let key: SSHKeyRef + let enclave: Bool + @State private var presented = true + + var body: some View { + Color.clear.sheet(isPresented: $presented) { + if enclave { + MobileKeyEditor(model: MobileKeyEditorModel(mode: .edit, credentials: credentials, key: key)) + } else { + SecurityKeyEditor(credentials: credentials, existing: key) + } + } + } +}