Skip to content

Commit

Permalink
Add Local Authentication for Sync (#2191)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ayoy
ayoy authored Feb 12, 2024
1 parent c28ab75 commit c295cc6
Show file tree
Hide file tree
Showing 9 changed files with 122 additions and 31 deletions.
1 change: 1 addition & 0 deletions DuckDuckGo/Common/Localizables/UserText.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
46 changes: 43 additions & 3 deletions DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -38,20 +39,23 @@ final class DeviceAuthenticator: UserAuthenticating {
case changeLoginsSettings
case unlockLogins
case exportLogins
case syncSettings

var localizedDescription: String {
switch self {
case .autofill, .autofillCreditCards: return UserText.pmAutoLockPromptAutofill
case .changeLoginsSettings: return UserText.pmAutoLockPromptChangeLoginsSettings
case .unlockLogins: return UserText.pmAutoLockPromptUnlockLogins
case .exportLogins: return UserText.pmAutoLockPromptExportLogins
case .syncSettings: return UserText.syncAutoLockPrompt
}
}
}

internal enum Constants {
static var intervalBetweenIdleChecks: TimeInterval = 1
static var intervalBetweenCreditCardAutofillChecks: TimeInterval = 10
static var intervalBetweenSyncSettingsChecks: TimeInterval = 15
}

static var deviceSupportsBiometrics: Bool {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
}

}
12 changes: 12 additions & 0 deletions DuckDuckGo/Localizable.xcstrings
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
30 changes: 21 additions & 9 deletions DuckDuckGo/Preferences/Model/SyncPreferences.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -352,6 +354,7 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {
private let appearancePreferences: AppearancePreferences
private var cancellables = Set<AnyCancellable>()
private var connector: RemoteConnecting?
private let userAuthenticator: UserAuthenticating
}

extension SyncPreferences: ManagementDialogModelDelegate {
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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 {
Expand All @@ -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?
Expand Down
6 changes: 3 additions & 3 deletions LocalPackages/SyncUI/Sources/SyncUI/ViewModels/ManagementViewModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down
27 changes: 20 additions & 7 deletions LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncSetupView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,19 @@ struct SyncSetupView<ViewModel>: 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)
}
}
}
Expand All @@ -53,9 +62,13 @@ struct SyncSetupView<ViewModel>: 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)
Expand Down
8 changes: 4 additions & 4 deletions LocalPackages/SyncUI/Sources/SyncUI/Views/ManagementView/SyncedDevicesView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ struct SyncedDevicesView<ViewModel>: 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)
Expand Down
3 changes: 3 additions & 0 deletions UnitTests/Preferences/AutofillPreferencesModelTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ final class UserAuthenticatorMock: UserAuthenticating {
let authenticationResult = _authenticateUser(reason)
result(authenticationResult)
}
func authenticateUser(reason: DeviceAuthenticator.AuthenticationReason) async -> DeviceAuthenticationResult {
_authenticateUser(reason)
}
}

@MainActor
Expand Down
20 changes: 15 additions & 5 deletions UnitTests/Sync/SyncPreferencesTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -51,7 +60,8 @@ final class SyncPreferencesTests: XCTestCase {
syncService: ddgSyncing,
syncBookmarksAdapter: syncBookmarksAdapter,
appearancePreferences: appearancePreferences,
managementDialogModel: managementDialogModel
managementDialogModel: managementDialogModel,
userAuthenticator: MockUserAuthenticator()
)
}

Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit c295cc6

Please sign in to comment.