Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ability to Delete All Saved Passwords #2265

Merged
merged 51 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
cfc7281
Implement AutofillActionExecutor, concrete Delete All Passwords type,…
aataraxiaa Feb 21, 2024
0be6ca3
Add initial Strings (copy to be approved)
aataraxiaa Feb 21, 2024
2569353
Implement AutofillActionPresenter, create custom NSAlert, update Auto…
aataraxiaa Feb 21, 2024
5847268
Add Autofill More menu item & validation, add new image for alert
aataraxiaa Feb 21, 2024
b146c7d
Update AutofillActionExecutor test, add some documentation
aataraxiaa Feb 21, 2024
b121f4a
Iterate on Action Executor and Presenter, implement Builder to aid in…
aataraxiaa Feb 22, 2024
288cfb8
Implement initial Settings UI (Design in Review), implement logic in …
aataraxiaa Feb 22, 2024
47db351
Use Builder in PasswordManagementVC and update notification reference
aataraxiaa Feb 22, 2024
3079176
Add mocks and testing
aataraxiaa Feb 22, 2024
d79e256
Fix file -> target issues
aataraxiaa Feb 22, 2024
675b426
Revert Settings screen related changes and associated code based on D…
aataraxiaa Feb 23, 2024
a259d4e
Hide/show import/export buttons in Settings based on active password …
aataraxiaa Feb 23, 2024
6427f97
Remove checkbox, remove new icon
aataraxiaa Feb 23, 2024
b5be13b
Iterate on copy based on in-progress copy-review
aataraxiaa Feb 26, 2024
26da4cc
Update copy strings
aataraxiaa Feb 26, 2024
5f259ef
Update copy
aataraxiaa Feb 26, 2024
09a96ed
Fix extra period in copy
aataraxiaa Feb 26, 2024
418020e
Fix rebase package resolved
aataraxiaa Feb 27, 2024
41d45c7
Improve naming
aataraxiaa Feb 27, 2024
eaed1af
PR feedback: Check authState instead of nil for syncEnabled
aataraxiaa Feb 27, 2024
ab3d54b
PR feedback and fix copy
aataraxiaa Feb 27, 2024
40461cc
Merge branch 'main' into pete/delete-all-passwords
aataraxiaa Feb 27, 2024
9ee16cc
Fix Swiftlint warning
aataraxiaa Feb 27, 2024
935ec43
Ship Review feedback: Don't hide Autofill dialog on delete, refresh l…
aataraxiaa Feb 28, 2024
3ece6ea
Update to latest string copy, move NSAlert extensions to better locat…
aataraxiaa Feb 28, 2024
609e3bd
Merge latest main
aataraxiaa Feb 28, 2024
8611884
Update BSK reference to PR branch
aataraxiaa Feb 28, 2024
64808f8
Package errors
aataraxiaa Feb 28, 2024
48611d5
Merge branch 'main' into pete/delete-all-passwords
aataraxiaa Feb 28, 2024
e03edcd
Merge latest main to pick up BSK changes
aataraxiaa Feb 28, 2024
aac7d7c
Update copy based on copy review
aataraxiaa Feb 29, 2024
c08178d
Merge latest main
aataraxiaa Feb 29, 2024
51ff740
Show import button on empty passwords
aataraxiaa Feb 29, 2024
7d91ccc
Display approapriate empty state when switching autofill type
aataraxiaa Mar 1, 2024
04cd79e
PR feedback, build feedback: Fix old copy, implement autofill type se…
aataraxiaa Mar 1, 2024
16cf922
Merge branch 'main' into pete/delete-all-passwords
aataraxiaa Mar 1, 2024
df094e0
Update BSK reference after BSK main merge
aataraxiaa Mar 1, 2024
b871ee1
Undo previous change, implement not ideal fix, rename existing proper…
aataraxiaa Mar 3, 2024
8a57841
Improve comment
aataraxiaa Mar 3, 2024
adc5d6e
Update renamed property reference in tests
aataraxiaa Mar 4, 2024
e6d65a8
Ship Review Feedback: Refresh Autofill UI while before displaying suc…
aataraxiaa Mar 4, 2024
73331ad
Merge branch 'main' into pete/delete-all-passwords
aataraxiaa Mar 5, 2024
91e4507
Import translations
aataraxiaa Mar 5, 2024
635f550
Fix string key
aataraxiaa Mar 5, 2024
c9cbd5b
Merge latest main
aataraxiaa Mar 5, 2024
9ff31f4
Fix merge project package issues
aataraxiaa Mar 5, 2024
670101f
Remove bad references introduced by other work into main
aataraxiaa Mar 5, 2024
34b885b
Translations for Delete All feature (#2335)
aataraxiaa Mar 7, 2024
01452cb
Merge main
aataraxiaa Mar 7, 2024
76b7372
Fix merge conflict issues
aataraxiaa Mar 7, 2024
4187dee
Undo submodule commit\
aataraxiaa Mar 8, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions DuckDuckGo.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/duckduckgo/BrowserServicesKit",
"state" : {
"revision" : "04c35220aa94bd005171086acccadd677400e7d5",
"version" : "111.0.2"
"revision" : "f087cf8389926b58692870bd5a459bad492ce66d"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll update this to point to the latest version once the related BSK PR is merged.

}
},
{
Expand Down Expand Up @@ -147,7 +146,7 @@
{
"identity" : "trackerradarkit",
"kind" : "remoteSourceControl",
"location" : "https://github.com/duckduckgo/TrackerRadarKit.git",
"location" : "https://github.com/duckduckgo/TrackerRadarKit",
"state" : {
"revision" : "a6b7ba151d9dc6684484f3785293875ec01cc1ff",
"version" : "1.2.2"
Expand Down
45 changes: 45 additions & 0 deletions DuckDuckGo/Autofill/AutofillActionBuilder.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//
// AutofillActionBuilder.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import BrowserServicesKit
import Foundation

/// Conforming types provide methods to build an `AutofillActionExecutor` and an `AutofillActionPresenter`
protocol AutofillActionBuilder {
func buildExecutor() -> AutofillActionExecutor?
func buildPresenter() -> AutofillActionPresenter
}

extension AutofillActionBuilder {
func buildPresenter() -> AutofillActionPresenter {
DefaultAutofillActionPresenter()
}
}

/// Builds an `AutofillActionExecutor`
struct AutofillDeleteAllPasswordsBuilder: AutofillActionBuilder {
@MainActor
func buildExecutor() -> AutofillActionExecutor? {
guard let secureVault = try? AutofillSecureVaultFactory.makeVault(errorReporter: SecureVaultErrorReporter.shared),
let syncService = NSApp.delegateTyped.syncService else { return nil }

return AutofillDeleteAllPasswordsExecutor(userAuthenticator: DeviceAuthenticator.shared,
secureVault: secureVault,
syncService: syncService)
}
}
76 changes: 76 additions & 0 deletions DuckDuckGo/Autofill/AutofillActionExecutor.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//
// AutofillActionExecutor.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import Foundation
import BrowserServicesKit
import DDGSync
import AppKit

/// Conforming types provide an `execute` method which performs some action on autofill types (e.g delete all passwords)
protocol AutofillActionExecutor {
init(userAuthenticator: UserAuthenticating, secureVault: any AutofillSecureVault, syncService: DDGSyncing)
/// NSAlert to display asking a user to confirm the action
var confirmationAlert: NSAlert { get }
/// NSAlert to display when the action is complete
var completionAlert: NSAlert { get }
/// Executes the action
func execute(_ onSuccess: (() -> Void)?)
}

/// Concrete `AutofillActionExecutor` for deletion of all autofill passwords
struct AutofillDeleteAllPasswordsExecutor: AutofillActionExecutor {

var confirmationAlert: NSAlert {
let accounts = (try? secureVault.accounts()) ?? []
let syncEnabled = syncService.account != nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I think on iOS we went with checking if account.authenticated is true. I’m unsure which is the better check, but we check the accounts object at another point in the macOS codebase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, in the iOS codebase we're usually using syncService.authState == .inactive which is what I went with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've checked with Dominik and he says syncService.authState == .inactive is best to use as:

we have more states between inactive and active and every other than inactive means that sync is enabled or being enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @amddg44, I’ll update PR and Ship Review build

return NSAlert.deleteAllPasswordsConfirmationAlert(count: accounts.count, syncEnabled: syncEnabled)
}

var completionAlert: NSAlert {
let accounts = (try? secureVault.accounts()) ?? []
let syncEnabled = syncService.account != nil
return NSAlert.deleteAllPasswordsCompletionAlert(count: accounts.count, syncEnabled: syncEnabled)
}

private var userAuthenticator: UserAuthenticating
private var secureVault: any AutofillSecureVault
private var syncService: DDGSyncing

init(userAuthenticator: UserAuthenticating, secureVault: any AutofillSecureVault, syncService: DDGSyncing) {
self.userAuthenticator = userAuthenticator
self.secureVault = secureVault
self.syncService = syncService
}

func execute(_ onSuccess: (() -> Void)? = nil) {
userAuthenticator.authenticateUser(reason: .deleteAllPasswords) { authenticationResult in
guard authenticationResult.authenticated else { return }

do {
try secureVault.deleteAllWebsiteCredentials()
syncService.scheduler.notifyDataChanged()
NotificationCenter.default.post(name: .PasswordManagerChanged, object: nil)
onSuccess?()
} catch {
Pixel.fire(.debug(event: .secureVaultError, error: error))
}

return
}
}
}
62 changes: 62 additions & 0 deletions DuckDuckGo/Autofill/AutofillActionPresenter.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
//
// AutofillActionPresenter.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import Foundation
import AppKit

/// Conforming types handles presentation of `NSAlert`s associated with an `AutofillActionExecutor`
protocol AutofillActionPresenter {
func show(actionExecutor: AutofillActionExecutor)
}

/// Handles presentation of an alert associated with an `AutofillActionExecutor`
struct DefaultAutofillActionPresenter: AutofillActionPresenter {

@MainActor
func show(actionExecutor: AutofillActionExecutor) {
guard let window else { return }

let confirmationAlert = actionExecutor.confirmationAlert
let completionAlert = actionExecutor.completionAlert

confirmationAlert.beginSheetModal(for: window) { response in
switch response {
case .alertFirstButtonReturn:
actionExecutor.execute {
show(completionAlert)
}
default:
break
}
}
}
}

private extension DefaultAutofillActionPresenter {

@MainActor
func show(_ alert: NSAlert) {
guard let window else { return }
alert.beginSheetModal(for: window)
}

@MainActor
var window: NSWindow? {
WindowControllersManager.shared.lastKeyMainWindowController?.window
}
}
35 changes: 35 additions & 0 deletions DuckDuckGo/Common/Extensions/NSAlertExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,41 @@ extension NSAlert {
return alert
}

static func deleteAllPasswordsConfirmationAlert(count: Int, syncEnabled: Bool) -> NSAlert {
let messageText = UserText.deleteAllPasswordsConfirmationMessageText(count: count)
let informationText = UserText.deleteAllPasswordsConfirmationInformationText(count: count, syncEnabled: syncEnabled)
return autofillActionConfirmationAlert(messageText: messageText,
informationText: informationText,
confirmButtonText: UserText.passwordManagerAlerDeleteButton)
}

private static func autofillActionConfirmationAlert(messageText: String,
informationText: String,
confirmButtonText: String) -> NSAlert {
let alert = NSAlert()
alert.messageText = messageText
alert.informativeText = informationText
alert.alertStyle = .warning
alert.addButton(withTitle: confirmButtonText)
alert.addButton(withTitle: UserText.cancel)
return alert
}

static func deleteAllPasswordsCompletionAlert(count: Int, syncEnabled: Bool) -> NSAlert {
let messageText = UserText.deleteAllPasswordsCompletionMessageText(count: count)
let informationText = UserText.deleteAllPasswordsCompletionInformationText(count: count, syncEnabled: syncEnabled)
return autofillActionCompletionAlert(messageText: messageText,
informationText: informationText)
}

private static func autofillActionCompletionAlert(messageText: String, informationText: String) -> NSAlert {
let alert = NSAlert()
alert.messageText = messageText
alert.informativeText = informationText
alert.addButton(withTitle: UserText.ok)
return alert
}

@discardableResult
func runModal() async -> NSApplication.ModalResponse {
await withCheckedContinuation { continuation in
Expand Down
57 changes: 57 additions & 0 deletions DuckDuckGo/Common/Localizables/UserText.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,63 @@ struct UserText {
}
}

// MARK: Autofill Item Deletion (Autofill -> More Menu, Settings -> Autofill)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It’s possible copy with change after review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is actually a dedicated UserText+PasswordManager.swift that contains all the autofill related copy; perhaps these strings can be moved there too? 🙂

static let deleteAllPasswords = NSLocalizedString("autofill.items.delete-all-passwords", value: "Delete All Passwords…", comment: "Opens Delete All Passwords dialog")

// Confirmation Message Text
static func deleteAllPasswordsConfirmationMessageText(count: Int) -> String {
let localized = NSLocalizedString("autofill.items.delete-all-passwords-confirmation-message-text", value: "Are you sure you want to delete all passwords (%d)?", comment: "Message displayed on dialog asking user to confirm deletion of all passwords")
return String(format: localized, count)
}

// Confirmation Information Text
static func deleteAllPasswordsConfirmationInformationText(count: Int, syncEnabled: Bool) -> String {
switch(count, syncEnabled) {
case (1, true):
return NSLocalizedString("autofill.items.delete-one-password-synced-confirmation-information-text", value: "Your password will be deleted from all synced devices. Make sure you still have a way to access your accounts.", comment: "Information message displayed when deleting one password on a synced device")
case (1, false):
return NSLocalizedString("autofill.items.delete-one-password-device-confirmation-information-text", value: "Your password will be deleted from this device.", comment: "Information message displayed when deleting one password on a device")
case (_, true):
return NSLocalizedString("autofill.items.delete-all-passwords-synced-confirmation-information-text", value: "Your passwords will be deleted from all synced devices. Make sure you still have a way to access your accounts.", comment: "Information message displayed when deleting all passwords on a synced device")
default:
return NSLocalizedString("autofill.items.delete-all-passwords-device-confirmation-information-text", value: "Your passwords will be deleted from this device.", comment: "Information message displayed when deleting all passwords on a device")
}
}

// Completion Message Text
static func deleteAllPasswordsCompletionMessageText(count: Int) -> String {
if count == 1 {
return NSLocalizedString("autofill.items.delete-one-password-confirmation-message-text", value: "Password deleted", comment: "Message displayed on completion of single password deletion")
} else {
let localized = NSLocalizedString("autofill.items.delete-all-passwords-confirmation-message-text", value: "Passwords deleted (%d)", comment: "Message displayed on completion of multiple password deletion")
return String(format: localized, count)
}
}

// Completion Information Text
static func deleteAllPasswordsCompletionInformationText(count: Int, syncEnabled: Bool) -> String {
switch(count, syncEnabled) {
case (1, true):
return NSLocalizedString("autofill.items.delete-one-password-synced-completion-information-text",
value: "Your password have been deleted from all synced devices.",
comment: "Information message displayed on completion of single password deletion when devices are synced")
case (1, false):
return NSLocalizedString("autofill.items.delete-one-password-device-completion-information-text",
value: "Your password have been deleted from this device.",
comment: "Information message displayed on completion of single password deletion when devices are not synced")
case (_, true):
return NSLocalizedString("autofill.items.delete-all-passwords-synced-completion-information-text",
value: "Your passwords have been deleted from all synced devices.",
comment: "Information message displayed on completion of multiple password deletion when devices are synced")
default:
return NSLocalizedString("autofill.items.delete-all-passwords-device-completion-information-text",
value: "Your passwords have been deleted from this device.",
comment: "Information message displayed on completion of multiple password deletion when no devices are not synced")
}
}

static let deleteAllPasswordsPermissionText = NSLocalizedString("autofill.items.delete-all-passwords-permisson-text", value: "Authenticate to confirm you want to delete all passwords", comment: "Message displayed in system authentication dialog")

#if SUBSCRIPTION
// Key: "subscription.menu.item"
// Comment: "Title for Subscription item in the options menu"
Expand Down
5 changes: 4 additions & 1 deletion DuckDuckGo/DeviceAuthentication/DeviceAuthenticator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ final class DeviceAuthenticator: UserAuthenticating {
case unlockLogins
case exportLogins
case syncSettings
case deleteAllPasswords

var localizedDescription: String {
switch self {
Expand All @@ -48,6 +49,7 @@ final class DeviceAuthenticator: UserAuthenticating {
case .unlockLogins: return UserText.pmAutoLockPromptUnlockLogins
case .exportLogins: return UserText.pmAutoLockPromptExportLogins
case .syncSettings: return UserText.syncAutoLockPrompt
case .deleteAllPasswords: return UserText.deleteAllPasswordsPermissionText
}
}
}
Expand Down Expand Up @@ -154,7 +156,8 @@ final class DeviceAuthenticator: UserAuthenticating {
func authenticateUser(reason: AuthenticationReason, result: @escaping (DeviceAuthenticationResult) -> Void) {
let needsAuthenticationForCreditCardsAutofill = reason == .autofillCreditCards && isCreditCardTimeIntervalExpired()
let needsAuthenticationForSyncSettings = reason == .syncSettings && isSyncSettingsTimeIntervalExpired()
guard needsAuthenticationForCreditCardsAutofill || needsAuthenticationForSyncSettings || requiresAuthentication else {
let needsAuthenticationForDeleteAllPasswords = reason == .deleteAllPasswords
guard needsAuthenticationForCreditCardsAutofill || needsAuthenticationForSyncSettings || needsAuthenticationForDeleteAllPasswords || requiresAuthentication else {
result(.success)
return
}
Expand Down
Loading
Loading