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 support for local field validation for synced bookmarks and credentials #2454

Merged
merged 13 commits into from
Mar 26, 2024
2 changes: 1 addition & 1 deletion DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -14167,7 +14167,7 @@
repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
requirement = {
kind = exactVersion;
version = 129.2.0;
version = 130.0.0;
};
};
AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/duckduckgo/BrowserServicesKit",
"state" : {
"revision" : "284c328a097132a12e8abcf94d8f4d369063dcb4",
"version" : "129.2.0"
"revision" : "24da852f8726af668d9fdc6c5ea1c2d3b72e8888",
"version" : "130.0.0"
}
},
{
Expand Down
28 changes: 28 additions & 0 deletions DuckDuckGo/Preferences/Model/SyncPreferences.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {

@Published var isSyncCredentialsPaused: Bool

@Published var invalidBookmarksTitles: [String] = []
@Published var invalidCredentialsTitles: [String] = []

private var shouldRequestSyncOnFavoritesOptionChange: Bool = true
private var isScreenLocked: Bool = false
private var recoveryKey: SyncCode.RecoveryKey?
Expand Down Expand Up @@ -117,12 +120,14 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {
init(
syncService: DDGSyncing,
syncBookmarksAdapter: SyncBookmarksAdapter,
syncCredentialsAdapter: SyncCredentialsAdapter,
appearancePreferences: AppearancePreferences = .shared,
managementDialogModel: ManagementDialogModel = ManagementDialogModel(),
userAuthenticator: UserAuthenticating = DeviceAuthenticator.shared
) {
self.syncService = syncService
self.syncBookmarksAdapter = syncBookmarksAdapter
self.syncCredentialsAdapter = syncCredentialsAdapter
self.appearancePreferences = appearancePreferences
self.syncFeatureFlags = syncService.featureFlags
self.userAuthenticator = userAuthenticator
Expand All @@ -140,6 +145,16 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {
setUpSyncOptionsObservables(apperancePreferences: appearancePreferences)
}

private func updateInvalidObjects() {
invalidBookmarksTitles = syncBookmarksAdapter.provider?
.fetchDescriptionsForObjectsThatFailedValidation()
.map { $0.truncated(length: 15) } ?? []

let invalidCredentialsObjects: [String] = (try? syncCredentialsAdapter.provider?.fetchDescriptionsForObjectsThatFailedValidation()) ?? []
invalidCredentialsTitles = invalidCredentialsObjects.map({ $0.truncated(length: 15) })
}

// swiftlint:disable:next function_body_length
private func setUpObservables() {
syncService.featureFlagsPublisher
.dropFirst()
Expand All @@ -157,6 +172,18 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {
}
.store(in: &cancellables)

if DDGSync.isFieldValidationEnabled {
syncService.isSyncInProgressPublisher
.removeDuplicates()
.filter { !$0 }
.asVoid()
.receive(on: DispatchQueue.main)
.sink { [weak self] in
self?.updateInvalidObjects()
}
.store(in: &cancellables)
}

$syncErrorMessage
.map { $0 != nil }
.receive(on: DispatchQueue.main)
Expand Down Expand Up @@ -351,6 +378,7 @@ final class SyncPreferences: ObservableObject, SyncUI.ManagementViewModel {

private let syncService: DDGSyncing
private let syncBookmarksAdapter: SyncBookmarksAdapter
private let syncCredentialsAdapter: SyncCredentialsAdapter
private let appearancePreferences: AppearancePreferences
private var cancellables = Set<AnyCancellable>()
private var connector: RemoteConnecting?
Expand Down
7 changes: 6 additions & 1 deletion DuckDuckGo/Preferences/View/PreferencesSyncView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ struct SyncView: View {

var body: some View {
if let syncService = NSApp.delegateTyped.syncService, let syncDataProviders = NSApp.delegateTyped.syncDataProviders {
SyncUI.ManagementView(model: SyncPreferences(syncService: syncService, syncBookmarksAdapter: syncDataProviders.bookmarksAdapter))
let syncPreferences = SyncPreferences(
syncService: syncService,
syncBookmarksAdapter: syncDataProviders.bookmarksAdapter,
syncCredentialsAdapter: syncDataProviders.credentialsAdapter
)
SyncUI.ManagementView(model: syncPreferences)
.onAppear {
requestSync()
}
Expand Down
3 changes: 2 additions & 1 deletion DuckDuckGo/Sync/SyncBookmarksAdapter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ final class SyncBookmarksAdapter {
database: database,
metadataStore: metadataStore,
metricsEvents: metricsEventsHandler,
log: OSLog.sync,
syncDidUpdateData: { [weak self] in
LocalBookmarkManager.shared.loadBookmarks()
self?.isSyncBookmarksPaused = false
Expand All @@ -132,7 +133,7 @@ final class SyncBookmarksAdapter {

if !didMigrateToImprovedListsHandling {
didMigrateToImprovedListsHandling = true
provider.lastSyncTimestamp = nil
provider.updateSyncTimestamps(server: nil, local: nil)
}

bindSyncErrorPublisher(provider)
Expand Down
1 change: 1 addition & 0 deletions DuckDuckGo/Sync/SyncCredentialsAdapter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ final class SyncCredentialsAdapter {
secureVaultErrorReporter: SecureVaultErrorReporter.shared,
metadataStore: metadataStore,
metricsEvents: metricsEventsHandler,
log: OSLog.sync,
syncDidUpdateData: { [weak self] in
self?.syncDidCompleteSubject.send()
self?.isSyncCredentialsPaused = false
Expand Down
1 change: 1 addition & 0 deletions DuckDuckGo/Sync/SyncSettingsAdapter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ final class SyncSettingsAdapter {
metadataStore: metadataStore,
settingsHandlers: [FavoritesDisplayModeSyncHandler(), EmailProtectionSyncHandler(emailManager: emailManager)],
metricsEvents: metricsEventsHandler,
log: OSLog.sync,
syncDidUpdateData: { [weak self] in
self?.syncDidCompleteSubject.send()
}
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/DataBrokerProtection/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ let package = Package(
targets: ["DataBrokerProtection"])
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "129.2.0"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "130.0.0"),
.package(path: "../PixelKit"),
.package(path: "../SwiftUIExtensions"),
.package(path: "../XPCHelper"),
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/NetworkProtectionMac/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ let package = Package(
.library(name: "NetworkProtectionUI", targets: ["NetworkProtectionUI"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "129.2.0"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "130.0.0"),
.package(path: "../XPCHelper"),
.package(path: "../SwiftUIExtensions"),
.package(path: "../LoginItems"),
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/SubscriptionUI/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let package = Package(
targets: ["SubscriptionUI"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "129.2.0"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "130.0.0"),
.package(path: "../SwiftUIExtensions")
],
targets: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4739,6 +4739,78 @@
}
}
},
"prefrences.sync.invalid-bookmarks-present-description-many" : {
"comment" : "Alert message for multiple invalid bookmark being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Some bookmarks (%d) can't sync because some of their fields exceed the character limit."
}
}
}
},
"prefrences.sync.invalid-bookmarks-present-description-one" : {
"comment" : "Alert message for 1 invalid bookmark being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Your bookmark for %@ can't sync because one of its fields exceeds the character limit."
}
}
}
},
"prefrences.sync.invalid-bookmarks-present-title" : {
"comment" : "Alert title for invalid bookmarks being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Some bookmarks are not syncing due to excessively long content in certain fields."
}
}
}
},
"prefrences.sync.invalid-credentials-present-description-many" : {
"comment" : "Alert message for multiple invalid logins being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Some passwords (n) can't sync because some of their fields exceed the character limit."
}
}
}
},
"prefrences.sync.invalid-credentials-present-description-one" : {
"comment" : "Alert message for 1 invalid login being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Your password for %@ can't sync because one of its fields exceeds the character limit."
}
}
}
},
"prefrences.sync.invalid-credentials-present-title" : {
"comment" : "Alert title for invalid logins being filtered out of synced data",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Some logins are not syncing due to excessively long content in certain fields."
}
}
}
},
"prefrences.sync.keep-favicons-updated" : {
"comment" : "Title of the confirmation button for favicons fetching",
"extractionState" : "extracted_with_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public protocol ManagementViewModel: ObservableObject {
var isSyncBookmarksPaused: Bool { get }
var isSyncCredentialsPaused: Bool { get }

var invalidBookmarksTitles: [String] { get }
var invalidCredentialsTitles: [String] { get }

var recoveryCode: String? { get }
var codeToDisplay: String? { get }
var devices: [SyncDevice] { get }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ struct SyncEnabledView<ViewModel>: View where ViewModel: ManagementViewModel {
if model.isSyncCredentialsPaused {
syncPaused(for: .credentials)
}
if !model.invalidBookmarksTitles.isEmpty {
syncHasInvalidItems(for: .bookmarks)
}
if !model.invalidCredentialsTitles.isEmpty {
syncHasInvalidItems(for: .credentials)
}
}

// Sync Enabled
Expand Down Expand Up @@ -107,6 +113,47 @@ struct SyncEnabledView<ViewModel>: View where ViewModel: ManagementViewModel {
}
}

@ViewBuilder
func syncHasInvalidItems(for itemType: LimitedItemType) -> some View {
var title: String {
switch itemType {
case .bookmarks:
return UserText.invalidBookmarksPresentTitle
case .credentials:
return UserText.invalidCredentialsPresentTitle
}
}
var description: String {
switch itemType {
case .bookmarks:
assert(!model.invalidBookmarksTitles.isEmpty)
let firstInvalidBookmarkTitle = model.invalidBookmarksTitles.first ?? ""
return UserText.invalidBookmarksPresentDescription(firstInvalidBookmarkTitle, numberOfInvalidItems: model.invalidBookmarksTitles.count)

case .credentials:
assert(!model.invalidCredentialsTitles.isEmpty)
let firstInvalidCredentialTitle = model.invalidCredentialsTitles.first ?? ""
return UserText.invalidCredentialsPresentDescription(firstInvalidCredentialTitle, numberOfInvalidItems: model.invalidCredentialsTitles.count)
}
}
var actionTitle: String {
switch itemType {
case .bookmarks:
return UserText.bookmarksLimitExceededAction
case .credentials:
return UserText.credentialsLimitExceededAction
}
}
SyncWarningMessage(title: title, message: description, buttonTitle: actionTitle) {
switch itemType {
case .bookmarks:
model.manageBookmarks()
case .credentials:
model.manageLogins()
}
}
}

@ViewBuilder
fileprivate func syncUnavailableView() -> some View {
if model.isDataSyncingAvailable {
Expand Down
41 changes: 41 additions & 0 deletions LocalPackages/SyncUI/Sources/SyncUI/internal/UserText.swift
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,47 @@ enum UserText {
static let credentialsLimitExceededDescription = NSLocalizedString("prefrences.sync.credentials-limit-exceeded-description", bundle: Bundle.module, value: "Logins limit exceeded. Delete some to resume syncing.", comment: "Description for sync credentials limits exceeded warning")
static let bookmarksLimitExceededAction = NSLocalizedString("prefrences.sync.bookmarks-limit-exceeded-action", bundle: Bundle.module, value: "Manage Bookmarks", comment: "Button title for sync bookmarks limits exceeded warning to go to manage bookmarks")
static let credentialsLimitExceededAction = NSLocalizedString("prefrences.sync.credentials-limit-exceeded-action", bundle: Bundle.module, value: "Manage passwords…", comment: "Button title for sync credentials limits exceeded warning to go to manage passwords")
static let invalidBookmarksPresentTitle = NSLocalizedString("prefrences.sync.invalid-bookmarks-present-title", bundle: Bundle.module, value: "Some bookmarks are not syncing due to excessively long content in certain fields.", comment: "Alert title for invalid bookmarks being filtered out of synced data")
static let invalidCredentialsPresentTitle = NSLocalizedString("prefrences.sync.invalid-credentials-present-title", bundle: Bundle.module, value: "Some logins are not syncing due to excessively long content in certain fields.", comment: "Alert title for invalid logins being filtered out of synced data")

static func invalidBookmarksPresentDescription(_ invalidItemTitle: String, numberOfInvalidItems: Int) -> String {
guard numberOfInvalidItems > 1 else {
let message = NSLocalizedString(
"prefrences.sync.invalid-bookmarks-present-description-one",
bundle: Bundle.module,
value: "Your bookmark for %@ can't sync because one of its fields exceeds the character limit.",
comment: "Alert message for 1 invalid bookmark being filtered out of synced data"
)
return String(format: message, invalidItemTitle)
}
let message = NSLocalizedString(
"prefrences.sync.invalid-bookmarks-present-description-many",
bundle: Bundle.module,
value: "Some bookmarks (%d) can't sync because some of their fields exceed the character limit.",
comment: "Alert message for multiple invalid bookmark being filtered out of synced data"
)
return String(format: message, numberOfInvalidItems)
}

static func invalidCredentialsPresentDescription(_ invalidItemTitle: String, numberOfInvalidItems: Int) -> String {
guard numberOfInvalidItems > 1 else {
let message = NSLocalizedString(
"prefrences.sync.invalid-credentials-present-description-one",
bundle: Bundle.module,
value: "Your password for %@ can't sync because one of its fields exceeds the character limit.",
comment: "Alert message for 1 invalid login being filtered out of synced data"
)
return String(format: message, invalidItemTitle)
}
let message = NSLocalizedString(
"prefrences.sync.invalid-credentials-present-description-many",
bundle: Bundle.module,
value: "Some passwords (n) can't sync because some of their fields exceed the character limit.",
comment: "Alert message for multiple invalid logins being filtered out of synced data"
)
return String(format: message, numberOfInvalidItems)
}

static let syncErrorAlertTitle = NSLocalizedString("alert.sync-error", bundle: Bundle.module, value: "Sync & Backup Error", comment: "Title for sync error alert")
static let unableToSyncToServerDescription = NSLocalizedString("alert.unable-to-sync-to-server-description", bundle: Bundle.module, value: "Unable to connect to the server.", comment: "Description for unable to sync to server error")
static let unableToSyncWithAnotherDeviceDescription = NSLocalizedString("alert.unable-to-sync-with-another-device-description", bundle: Bundle.module, value: "Unable to Sync with another device.", comment: "Description for unable to sync with another device error")
Expand Down
8 changes: 8 additions & 0 deletions UnitTests/DataExport/MockSecureVault.swift
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ final class MockSecureVault<T: AutofillDatabaseProvider>: AutofillSecureVault {
[]
}

func accountTitlesForSyncableCredentials(modifiedBefore date: Date) throws -> [String] {
[]
}

func deleteSyncableCredentials(_ syncableCredentials: SecureVaultModels.SyncableCredentials, in database: Database) throws {
if let accountId = syncableCredentials.metadata.objectId {
try deleteWebsiteCredentialsFor(accountId: accountId)
Expand Down Expand Up @@ -425,6 +429,10 @@ class MockDatabaseProvider: AutofillDatabaseProvider {
[]
}

func modifiedSyncableCredentials(before date: Date) throws -> [SecureVaultModels.SyncableCredentials] {
[]
}

func syncableCredentialsForSyncIds(_ syncIds: any Sequence<String>, in database: Database) throws -> [SecureVaultModels.SyncableCredentials] {
[]
}
Expand Down
Loading
Loading