From c295cc63855ac048f5b54b627668b2af27a6fcfd Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Mon, 12 Feb 2024 11:24:31 +0100 Subject: [PATCH] Add Local Authentication for Sync (#2191) Task/Issue URL: https://app.asana.com/0/0/1206519747938381/f Description: This change prevents unauthorized access to sync settings. Local authentication is valid for 15 seconds. --- DuckDuckGo/Common/Localizables/UserText.swift | 1 + .../DeviceAuthenticator.swift | 46 +++++++++++++++++-- DuckDuckGo/Localizable.xcstrings | 12 +++++ .../Preferences/Model/SyncPreferences.swift | 30 ++++++++---- .../ViewModels/ManagementViewModel.swift | 6 +-- .../Views/ManagementView/SyncSetupView.swift | 27 ++++++++--- .../ManagementView/SyncedDevicesView.swift | 8 ++-- .../AutofillPreferencesModelTests.swift | 3 ++ UnitTests/Sync/SyncPreferencesTests.swift | 20 ++++++-- 9 files changed, 122 insertions(+), 31 deletions(-) diff --git a/DuckDuckGo/Common/Localizables/UserText.swift b/DuckDuckGo/Common/Localizables/UserText.swift index 4e676a7aac..0eeb4640af 100644 --- a/DuckDuckGo/Common/Localizables/UserText.swift +++ b/DuckDuckGo/Common/Localizables/UserText.swift @@ -575,6 +575,7 @@ struct UserText { static let general = NSLocalizedString("preferences.general", value: "General", comment: "Show general preferences") static let sync = NSLocalizedString("preferences.sync", value: "Sync & Backup", comment: "Show sync preferences") + static let syncAutoLockPrompt = NSLocalizedString("preferences.sync.auto-lock-prompt", value:"Unlock device to setup Sync & Backup", comment: "Reason for auth when setting up Sync") static let syncBookmarkPausedAlertTitle = NSLocalizedString("alert.sync-bookmarks-paused-title", value: "Bookmarks Sync is Paused", comment: "Title for alert shown when sync bookmarks paused for too many items") static let syncBookmarkPausedAlertDescription = NSLocalizedString("alert.sync-bookmarks-paused-description", value: "You have exceeded the bookmarks sync limit. Try deleting some bookmarks. Until this is resolved your bookmarks will not be backed up.", comment: "Description for alert shown when sync bookmarks paused for too many items") static let syncCredentialsPausedAlertTitle = NSLocalizedString("alert.sync-credentials-paused-title", value: "Passwords Sync is Paused", comment: "Title for alert shown when sync credentials paused for too many items") diff --git a/DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift b/DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift index e2bc62376d..8ce6626df5 100644 --- a/DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift +++ b/DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift @@ -28,6 +28,7 @@ extension NSNotification.Name { protocol UserAuthenticating { func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason, result: @escaping (DeviceAuthenticationResult) -> Void) + func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason) async -> DeviceAuthenticationResult } final class DeviceAuthenticator: UserAuthenticating { @@ -38,6 +39,7 @@ final class DeviceAuthenticator: UserAuthenticating { case changeLoginsSettings case unlockLogins case exportLogins + case syncSettings var localizedDescription: String { switch self { @@ -45,6 +47,7 @@ final class DeviceAuthenticator: UserAuthenticating { case .changeLoginsSettings: return UserText.pmAutoLockPromptChangeLoginsSettings case .unlockLogins: return UserText.pmAutoLockPromptUnlockLogins case .exportLogins: return UserText.pmAutoLockPromptExportLogins + case .syncSettings: return UserText.syncAutoLockPrompt } } } @@ -52,6 +55,7 @@ final class DeviceAuthenticator: UserAuthenticating { internal enum Constants { static var intervalBetweenIdleChecks: TimeInterval = 1 static var intervalBetweenCreditCardAutofillChecks: TimeInterval = 10 + static var intervalBetweenSyncSettingsChecks: TimeInterval = 15 } static var deviceSupportsBiometrics: Bool { @@ -94,6 +98,7 @@ final class DeviceAuthenticator: UserAuthenticating { private var timer: Timer? private var timerCreditCard: Timer? + private var timerSyncSettings: Timer? private var _isAuthenticating: Bool = false private var _deviceIsLocked: Bool = false @@ -147,7 +152,9 @@ final class DeviceAuthenticator: UserAuthenticating { } func authenticateUser(reason: AuthenticationReason, result: @escaping (DeviceAuthenticationResult) -> Void) { - guard (reason == .autofillCreditCards && creditCardTimeIntervalExpired()) || requiresAuthentication else { + let needsAuthenticationForCreditCardsAutofill = reason == .autofillCreditCards && isCreditCardTimeIntervalExpired() + let needsAuthenticationForSyncSettings = reason == .syncSettings && isSyncSettingsTimeIntervalExpired() + guard needsAuthenticationForCreditCardsAutofill || needsAuthenticationForSyncSettings || requiresAuthentication else { result(.success) return } @@ -166,13 +173,14 @@ final class DeviceAuthenticator: UserAuthenticating { // Now that the user has unlocked the device, begin the idle timer again. self.beginIdleCheckTimer() self.beginCreditCardAutofillTimer() + self.beginSyncSettingsTimer() } result(authenticationResult) } } - func authenticateUser(reason: AuthenticationReason) async -> DeviceAuthenticationResult { + func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason) async -> DeviceAuthenticationResult { await withCheckedContinuation { continuation in authenticateUser(reason: reason) { result in continuation.resume(returning: result) @@ -262,11 +270,43 @@ final class DeviceAuthenticator: UserAuthenticating { self.timerCreditCard = nil } - private func creditCardTimeIntervalExpired() -> Bool { + private func isCreditCardTimeIntervalExpired() -> Bool { guard let timer = timerCreditCard else { return true } return timer.timeInterval >= Constants.intervalBetweenCreditCardAutofillChecks } + // MARK: - Sync Timer + + private func beginSyncSettingsTimer() { + os_log("Beginning Sync Settings timer", log: .autoLock) + + self.timerSyncSettings?.invalidate() + self.timerSyncSettings = nil + + let timer = Timer(timeInterval: Constants.intervalBetweenSyncSettingsChecks, repeats: false) { [weak self] _ in + guard let self = self else { + return + } + self.cancelSyncSettingsTimer() + } + + self.timerSyncSettings = timer + RunLoop.current.add(timer, forMode: .common) + } + + private func cancelSyncSettingsTimer() { + os_log("Cancelling Sync Settings timer", log: .autoLock) + self.timerSyncSettings?.invalidate() + self.timerSyncSettings = nil + } + + private func isSyncSettingsTimeIntervalExpired() -> Bool { + guard let timer = timerSyncSettings else { + return true + } + return timer.timeInterval >= Constants.intervalBetweenSyncSettingsChecks + } + } diff --git a/DuckDuckGo/Localizable.xcstrings b/DuckDuckGo/Localizable.xcstrings index 27dd6da276..63a3274895 100644 --- a/DuckDuckGo/Localizable.xcstrings +++ b/DuckDuckGo/Localizable.xcstrings @@ -8741,6 +8741,18 @@ } } }, + "preferences.sync.auto-lock-prompt" : { + "comment" : "Reason for auth when setting up Sync", + "extractionState" : "extracted_with_value", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "new", + "value" : "Unlock device to setup Sync & Backup" + } + } + } + }, "preferences.vpn" : { "comment" : "Show VPN preferences", "extractionState" : "extracted_with_value", diff --git a/DuckDuckGo/Preferences/Model/SyncPreferences.swift b/DuckDuckGo/Preferences/Model/SyncPreferences.swift index 8d891c1dca..8fb0e3a523 100644 --- a/DuckDuckGo/Preferences/Model/SyncPreferences.swift +++ b/DuckDuckGo/Preferences/Model/SyncPreferences.swift @@ -118,12 +118,14 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel { syncService: DDGSyncing, syncBookmarksAdapter: SyncBookmarksAdapter, appearancePreferences: AppearancePreferences = .shared, - managementDialogModel: ManagementDialogModel = ManagementDialogModel() + managementDialogModel: ManagementDialogModel = ManagementDialogModel(), + userAuthenticator: UserAuthenticating = DeviceAuthenticator.shared ) { self.syncService = syncService self.syncBookmarksAdapter = syncBookmarksAdapter self.appearancePreferences = appearancePreferences self.syncFeatureFlags = syncService.featureFlags + self.userAuthenticator = userAuthenticator self.isFaviconsFetchingEnabled = syncBookmarksAdapter.isFaviconsFetchingEnabled self.isUnifiedFavoritesEnabled = appearancePreferences.favoritesDisplayMode.isDisplayUnified @@ -352,6 +354,7 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel { private let appearancePreferences: AppearancePreferences private var cancellables = Set() private var connector: RemoteConnecting? + private let userAuthenticator: UserAuthenticating } extension SyncPreferences: ManagementDialogModelDelegate { @@ -532,6 +535,11 @@ extension SyncPreferences: ManagementDialogModelDelegate { .generate(recoveryCode) Task { @MainActor in + let authenticationResult = await userAuthenticator.authenticateUser(reason: .syncSettings) + guard authenticationResult.authenticated else { + return + } + let panel = NSSavePanel.savePanelWithFileTypeChooser(fileTypes: [.pdf], suggestedFilename: "Sync Data Recovery - DuckDuckGo.pdf") let response = await panel.begin() @@ -570,7 +578,10 @@ extension SyncPreferences: ManagementDialogModelDelegate { } @MainActor - func syncWithAnotherDevicePressed() { + func syncWithAnotherDevicePressed() async { + guard await userAuthenticator.authenticateUser(reason: .syncSettings).authenticated else { + return + } if isSyncEnabled { presentDialog(for: .syncWithAnotherDevice(code: recoveryCode ?? "")) } else { @@ -579,20 +590,21 @@ extension SyncPreferences: ManagementDialogModelDelegate { } @MainActor - func syncWithServerPressed() { + func syncWithServerPressed() async { + guard await userAuthenticator.authenticateUser(reason: .syncSettings).authenticated else { + return + } presentDialog(for: .syncWithServer) } @MainActor - func recoverDataPressed() { + func recoverDataPressed() async { + guard await userAuthenticator.authenticateUser(reason: .syncSettings).authenticated else { + return + } presentDialog(for: .recoverSyncedData) } - @MainActor - func downloadDDGPressed() { - - } - @MainActor func copyCode() { var code: String? diff --git a/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/ManagementViewModel.swift b/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/ManagementViewModel.swift index c537af56ef..26580aee97 100644 --- a/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/ManagementViewModel.swift +++ b/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/ManagementViewModel.swift @@ -49,9 +49,9 @@ public protocol ManagementViewModel: ObservableObject { func manageBookmarks() func manageLogins() - func syncWithAnotherDevicePressed() - func syncWithServerPressed() - func recoverDataPressed() + func syncWithAnotherDevicePressed() async + func syncWithServerPressed() async + func recoverDataPressed() async func turnOffSyncPressed() } diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncSetupView.swift b/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncSetupView.swift index e1c49e5a21..e1d77dcb9f 100644 --- a/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncSetupView.swift +++ b/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncSetupView.swift @@ -36,10 +36,19 @@ struct SyncSetupView: View where ViewModel: ManagementViewModel { VStack(alignment: .leading, spacing: 12) { SyncUIViews.TextHeader2(text: UserText.otherOptionsSectionTitle) VStack(alignment: .leading, spacing: 8) { - TextButton(UserText.syncThisDeviceLink, weight: .semibold, action: model.syncWithServerPressed) - .disabled(!model.isAccountCreationAvailable) - TextButton(UserText.recoverDataLink, weight: .semibold, action: model.recoverDataPressed) - .disabled(!model.isAccountRecoveryAvailable) + TextButton(UserText.syncThisDeviceLink, weight: .semibold) { + Task { + await model.syncWithServerPressed() + } + } + .disabled(!model.isAccountCreationAvailable) + + TextButton(UserText.recoverDataLink, weight: .semibold) { + Task { + await model.recoverDataPressed() + } + } + .disabled(!model.isAccountRecoveryAvailable) } } } @@ -53,9 +62,13 @@ struct SyncSetupView: View where ViewModel: ManagementViewModel { SyncUIViews.TextDetailSecondary(text: UserText.beginSyncDescription) } .padding(.bottom, 16) - Button(UserText.beginSyncButton, action: model.syncWithAnotherDevicePressed) - .buttonStyle(SyncWithAnotherDeviceButtonStyle(enabled: model.isConnectingDevicesAvailable)) - .disabled(!model.isConnectingDevicesAvailable) + Button(UserText.beginSyncButton) { + Task { + await model.syncWithAnotherDevicePressed() + } + } + .buttonStyle(SyncWithAnotherDeviceButtonStyle(enabled: model.isConnectingDevicesAvailable)) + .disabled(!model.isConnectingDevicesAvailable) } .frame(maxWidth: .infinity) .frame(height: 254) diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncedDevicesView.swift b/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncedDevicesView.swift index 766f8596ca..415c523b79 100644 --- a/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncedDevicesView.swift +++ b/LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncedDevicesView.swift @@ -41,10 +41,10 @@ struct SyncedDevicesView: View where ViewModel: ManagementViewModel { .onDisappear { isVisible = false } - Button { - model.syncWithAnotherDevicePressed() - } label: { - Text(UserText.beginSyncButton) + Button(UserText.beginSyncButton) { + Task { + await model.syncWithAnotherDevicePressed() + } } .disabled(!model.isConnectingDevicesAvailable) .padding(.horizontal, 10) diff --git a/UnitTests/Preferences/AutofillPreferencesModelTests.swift b/UnitTests/Preferences/AutofillPreferencesModelTests.swift index c1384846e4..184758d8c9 100644 --- a/UnitTests/Preferences/AutofillPreferencesModelTests.swift +++ b/UnitTests/Preferences/AutofillPreferencesModelTests.swift @@ -37,6 +37,9 @@ final class UserAuthenticatorMock: UserAuthenticating { let authenticationResult = _authenticateUser(reason) result(authenticationResult) } + func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason) async -> DeviceAuthenticationResult { + _authenticateUser(reason) + } } @MainActor diff --git a/UnitTests/Sync/SyncPreferencesTests.swift b/UnitTests/Sync/SyncPreferencesTests.swift index 5317d03c02..76cfc93428 100644 --- a/UnitTests/Sync/SyncPreferencesTests.swift +++ b/UnitTests/Sync/SyncPreferencesTests.swift @@ -26,6 +26,15 @@ import TestUtils @testable import DDGSync @testable import DuckDuckGo_Privacy_Browser +private final class MockUserAuthenticator: UserAuthenticating { + func authenticateUser(reason: DuckDuckGo_Privacy_Browser.DeviceAuthenticator.AuthenticationReason) async -> DeviceAuthenticationResult { + .success + } + func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason, result: @escaping (DeviceAuthenticationResult) -> Void) { + result(.success) + } +} + final class SyncPreferencesTests: XCTestCase { let scheduler = CapturingScheduler() @@ -51,7 +60,8 @@ final class SyncPreferencesTests: XCTestCase { syncService: ddgSyncing, syncBookmarksAdapter: syncBookmarksAdapter, appearancePreferences: appearancePreferences, - managementDialogModel: managementDialogModel + managementDialogModel: managementDialogModel, + userAuthenticator: MockUserAuthenticator() ) } @@ -98,14 +108,14 @@ final class SyncPreferencesTests: XCTestCase { XCTAssertEqual(syncPreferences.recoveryCode, account.recoveryCode) } - @MainActor func testOnPresentRecoverSyncAccountDialogThenRecoverAccountDialogShown() { - syncPreferences.recoverDataPressed() + @MainActor func testOnPresentRecoverSyncAccountDialogThenRecoverAccountDialogShown() async { + await syncPreferences.recoverDataPressed() XCTAssertEqual(managementDialogModel.currentDialog, .recoverSyncedData) } - @MainActor func testOnSyncWithServerPressedThenSyncWithServerDialogShown() { - syncPreferences.syncWithServerPressed() + @MainActor func testOnSyncWithServerPressedThenSyncWithServerDialogShown() async { + await syncPreferences.syncWithServerPressed() XCTAssertEqual(managementDialogModel.currentDialog, .syncWithServer) }