Add support for local field validation for synced bookmarks and crede…

…ntials (#2614) Task/Issue URL: https://app.asana.com/0/0/1206637913741251/f Description: This change adds a mechanism that filters out syncable objects that would fail validation on the backend before sending Sync patch request. Objects rejected from patch payload are retried on every subsequent Sync request, until they're updated to pass validation or deleted.
duckduckgo · Mar 26, 2024 · cddb637 · cddb637
1 parent 5158917
commit cddb637
Show file tree

Hide file tree

Showing 14 changed files with 201 additions and 32 deletions.
diff --git a/Core/StringExtension.swift b/Core/StringExtension.swift
@@ -22,6 +22,10 @@ import BrowserServicesKit
 
 extension String {
 
+    public func truncated(length: Int, trailing: String = "…") -> String {
+      return (self.count > length) ? self.prefix(length) + trailing : self
+    }
+
     /// Useful if loaded from UserText, for example
     public func format(arguments: CVarArg...) -> String {
         return String(format: self, arguments: arguments)

diff --git a/Core/SyncBookmarksAdapter.swift b/Core/SyncBookmarksAdapter.swift
@@ -153,7 +153,7 @@ public final class SyncBookmarksAdapter {
         )
         if !didMigrateToImprovedListsHandling {
             didMigrateToImprovedListsHandling = true
-            provider.lastSyncTimestamp = nil
+            provider.updateSyncTimestamps(server: nil, local: nil)
         }
 
         bindSyncErrorPublisher(provider)

diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj
@@ -10033,7 +10033,7 @@
 			repositoryURL = "https://github.com/DuckDuckGo/BrowserServicesKit";
 			requirement = {
 				kind = exactVersion;
-				version = 129.2.0;
+				version = 130.0.0;
 			};
 		};
 		B6F997C22B8F374300476735 /* XCRemoteSwiftPackageReference "apple-toolbox" */ = {

diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -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"
       }
     },
     {

diff --git a/DuckDuckGo/SettingsLegacyViewProvider.swift b/DuckDuckGo/SettingsLegacyViewProvider.swift
@@ -91,6 +91,7 @@ class SettingsLegacyViewProvider: ObservableObject {
     var syncSettings: UIViewController {
         return SyncSettingsViewController(syncService: self.syncService,
                                           syncBookmarksAdapter: self.syncDataProviders.bookmarksAdapter,
+                                          syncCredentialsAdapter: self.syncDataProviders.credentialsAdapter,
                                           appSettings: self.appSettings)
     }
 

diff --git a/DuckDuckGo/SyncSettingsViewController.swift b/DuckDuckGo/SyncSettingsViewController.swift
@@ -31,6 +31,7 @@ class SyncSettingsViewController: UIHostingController<SyncSettingsView> {
 
     let syncService: DDGSyncing
     let syncBookmarksAdapter: SyncBookmarksAdapter
+    let syncCredentialsAdapter: SyncCredentialsAdapter
     var connector: RemoteConnecting?
 
     let userAuthenticator = UserAuthenticator(reason: UserText.syncUserUserAuthenticationReason)
@@ -55,9 +56,15 @@ class SyncSettingsViewController: UIHostingController<SyncSettingsView> {
     var cancellables = Set<AnyCancellable>()
 
     // For some reason, on iOS 14, the viewDidLoad wasn't getting called so do some setup here
-    init(syncService: DDGSyncing, syncBookmarksAdapter: SyncBookmarksAdapter, appSettings: AppSettings = AppDependencyProvider.shared.appSettings) {
+    init(
+        syncService: DDGSyncing,
+        syncBookmarksAdapter: SyncBookmarksAdapter,
+        syncCredentialsAdapter: SyncCredentialsAdapter,
+        appSettings: AppSettings = AppDependencyProvider.shared.appSettings
+    ) {
         self.syncService = syncService
         self.syncBookmarksAdapter = syncBookmarksAdapter
+        self.syncCredentialsAdapter = syncCredentialsAdapter
 
         let viewModel = SyncSettingsViewModel(
             isOnDevEnvironment: { syncService.serverEnvironment == .development },
@@ -72,6 +79,9 @@ class SyncSettingsViewController: UIHostingController<SyncSettingsView> {
         setUpFaviconsFetcherSwitch(viewModel)
         setUpFavoritesDisplayModeSwitch(viewModel, appSettings)
         setUpSyncPaused(viewModel, appSettings)
+        if DDGSync.isFieldValidationEnabled {
+            setUpSyncInvalidObjectsInfo(viewModel)
+        }
         setUpSyncFeatureFlags(viewModel)
         refreshForState(syncService.authState)
 
@@ -186,6 +196,27 @@ class SyncSettingsViewController: UIHostingController<SyncSettingsView> {
             .store(in: &cancellables)
     }
 
+    private func setUpSyncInvalidObjectsInfo(_ viewModel: SyncSettingsViewModel) {
+        syncService.isSyncInProgressPublisher
+            .removeDuplicates()
+            .filter { !$0 }
+            .receive(on: DispatchQueue.main)
+            .sink { [weak self] _ in
+                self?.updateInvalidObjects(viewModel)
+            }
+            .store(in: &cancellables)
+    }
+
+    private func updateInvalidObjects(_ viewModel: SyncSettingsViewModel) {
+        viewModel.invalidBookmarksTitles = syncBookmarksAdapter.provider?
+            .fetchDescriptionsForObjectsThatFailedValidation()
+            .map { $0.truncated(length: 15) } ?? []
+
+        let invalidCredentialsObjects: [String] = (try? syncCredentialsAdapter.provider?.fetchDescriptionsForObjectsThatFailedValidation()) ?? []
+        viewModel.invalidCredentialsTitles = invalidCredentialsObjects.map({ $0.truncated(length: 15) })
+    }
+
+
     override func viewDidLoad() {
         super.viewDidLoad()
         applyTheme(ThemeManager.shared.currentTheme)

diff --git a/DuckDuckGo/en.lproj/Localizable.stringsdict b/DuckDuckGo/en.lproj/Localizable.stringsdict
@@ -226,33 +226,33 @@ I blocked them!
 			<string>Are you sure you want to delete this password?</string>
 		</dict>
 	</dict>
-    <key>autofill.delete.all.passwords.confirmation.body</key>
-    <dict>
-        <key>NSStringLocalizedFormatKey</key>
-        <string>%1$#@passwords@</string>
-        <key>passwords</key>
-        <dict>
-            <key>NSStringFormatSpecTypeKey</key>
-            <string>NSStringPluralRuleType</string>
-            <key>NSStringFormatValueTypeKey</key>
-            <string>d</string>
-            <key>other</key>
-            <string>Your passwords will be deleted from this device. Make sure you still have a way to access your %2$#@accounts@.</string>
-            <key>one</key>
-            <string>Your password will be deleted from this device. Make sure you still have a way to access your %2$#@accounts@.</string>
-        </dict>
-        <key>accounts</key>
-        <dict>
-            <key>NSStringFormatSpecTypeKey</key>
-            <string>NSStringPluralRuleType</string>
-            <key>NSStringFormatValueTypeKey</key>
-            <string>d</string>
-            <key>other</key>
-            <string>accounts</string>
-            <key>one</key>
-            <string>account</string>
-        </dict>
-    </dict>
+	<key>autofill.delete.all.passwords.confirmation.body</key>
+	<dict>
+		<key>NSStringLocalizedFormatKey</key>
+		<string>%1$#@passwords@</string>
+		<key>passwords</key>
+		<dict>
+			<key>NSStringFormatSpecTypeKey</key>
+			<string>NSStringPluralRuleType</string>
+			<key>NSStringFormatValueTypeKey</key>
+			<string>d</string>
+			<key>other</key>
+			<string>Your passwords will be deleted from this device. Make sure you still have a way to access your %2$#@accounts@.</string>
+			<key>one</key>
+			<string>Your password will be deleted from this device. Make sure you still have a way to access your %2$#@accounts@.</string>
+		</dict>
+		<key>accounts</key>
+		<dict>
+			<key>NSStringFormatSpecTypeKey</key>
+			<string>NSStringPluralRuleType</string>
+			<key>NSStringFormatValueTypeKey</key>
+			<string>d</string>
+			<key>other</key>
+			<string>accounts</string>
+			<key>one</key>
+			<string>account</string>
+		</dict>
+	</dict>
 	<key>autofill.delete.all.passwords.sync.confirmation.body</key>
 	<dict>
 		<key>NSStringLocalizedFormatKey</key>

diff --git a/DuckDuckGoTests/MockSecureVault.swift b/DuckDuckGoTests/MockSecureVault.swift
@@ -210,6 +210,10 @@ final class MockSecureVault<T: AutofillDatabaseProvider>: AutofillSecureVault {
         []
     }
 
+    func accountTitlesForSyncableCredentials(modifiedBefore date: Date) throws -> [String] {
+        []
+    }
+
     func deleteSyncableCredentials(_ syncableCredentials: SecureVaultModels.SyncableCredentials, in database: Database) throws {
     }
 
@@ -403,6 +407,10 @@ class MockDatabaseProvider: AutofillDatabaseProvider {
         []
     }
 
+    func modifiedSyncableCredentials(before date: Date) throws -> [SecureVaultModels.SyncableCredentials] {
+        []
+    }
+
     func syncableCredentialsForSyncIds(_ syncIds: any Sequence<String>, in database: Database) throws -> [SecureVaultModels.SyncableCredentials] {
         []
     }

diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Resources/en.lproj/Localizable.strings b/LocalPackages/SyncUI/Sources/SyncUI/Resources/en.lproj/Localizable.strings
@@ -1,6 +1,12 @@
 /* Standard Buttons - Back Button */
 "back.button" = "Back";
 
+/* Do not translate - stringsdict entry */
+"bookmarks.invalid.objects.present.description" = "bookmarks.invalid.objects.present.description";
+
+/* Alert title for invalid bookmarks being filtered out of synced data */
+"bookmarks.invalid.objects.present.title" = "Some bookmarks are not syncing due to excessively long content in certain fields.";
+
 /* Sync Paused Errors - Bookmarks Limit Exceeded Action */
 "bookmarks.limit.exceeded.action" = "Manage Bookmarks";
 
@@ -40,6 +46,12 @@
 /* Connect With Server Sheet - Title */
 "connect.with.server.sheet.title" = "Sync and Back Up This Device";
 
+/* Do not translate - stringsdict entry */
+"credentials.invalid.objects.present.description" = "credentials.invalid.objects.present.description";
+
+/* Alert title for invalid logins being filtered out of synced data */
+"credentials.invalid.objects.present.title" = "Some logins are not syncing due to excessively long content in certain fields.";
+
 /* Sync Paused Errors - Credentials Limit Exceeded Action */
 "credentials.limit.exceeded.action" = "Manage Logins";
 

diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Resources/en.lproj/Localizable.stringsdict b/LocalPackages/SyncUI/Sources/SyncUI/Resources/en.lproj/Localizable.stringsdict
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>bookmarks.invalid.objects.present.description</key>
+	<dict>
+		<key>NSStringLocalizedFormatKey</key>
+		<string>%#@sites@</string>
+		<key>sites</key>
+		<dict>
+			<key>NSStringFormatSpecTypeKey</key>
+			<string>NSStringPluralRuleType</string>
+			<key>NSStringFormatValueTypeKey</key>
+			<string>d</string>
+			<key>zero</key>
+			<string>Your bookmark for %2$@ can&apos;t sync because one of its fields exceeds the character limit.</string>
+			<key>one</key>
+			<string>Your bookmarks for %2$@ and 1 other site can&apos;t sync because some of their fields exceed the character limit.</string>
+			<key>other</key>
+			<string>Your bookmarks for %2$@ and %1$d other sites can&apos;t sync because some of their fields exceed the character limit.</string>
+		</dict>
+	</dict>
+	<key>credentials.invalid.objects.present.description</key>
+	<dict>
+		<key>NSStringLocalizedFormatKey</key>
+		<string>%#@sites@</string>
+		<key>sites</key>
+		<dict>
+			<key>NSStringFormatSpecTypeKey</key>
+			<string>NSStringPluralRuleType</string>
+			<key>NSStringFormatValueTypeKey</key>
+			<string>d</string>
+			<key>zero</key>
+			<string>Your password for %2$@ can&apos;t sync because one of its fields exceeds the character limit.</string>
+			<key>one</key>
+			<string>Your passwords for %2$@ and 1 other site can&apos;t sync because some of their fields exceed the character limit.</string>
+			<key>other</key>
+			<string>Your passwords for %2$@ and %1$d other sites can&apos;t sync because some of their fields exceed the character limit.</string>
+		</dict>
+	</dict>
+</dict>
+</plist>
diff --git a/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/SyncSettingsViewModel.swift b/LocalPackages/SyncUI/Sources/SyncUI/ViewModels/SyncSettingsViewModel.swift
@@ -81,6 +81,8 @@ public class SyncSettingsViewModel: ObservableObject {
     @Published public var isSyncingDevices = false
     @Published public var isSyncBookmarksPaused = false
     @Published public var isSyncCredentialsPaused = false
+    @Published public var invalidBookmarksTitles: [String] = []
+    @Published public var invalidCredentialsTitles: [String] = []
 
     @Published var isBusy = false
     @Published var recoveryCode = ""

diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Views/Internal/UserText.swift b/LocalPackages/SyncUI/Sources/SyncUI/Views/Internal/UserText.swift
@@ -48,6 +48,20 @@ public struct UserText {
     static let credentialsLimitExceededDescription = NSLocalizedString("credentials.limit.exceeded.description", bundle: Bundle.module, value: "Logins limit exceeded. Delete some to resume syncing.", comment: "Sync Paused Errors - Credentials Limit Exceeded Description")
     static let bookmarksLimitExceededAction = NSLocalizedString("bookmarks.limit.exceeded.action", bundle: Bundle.module, value: "Manage Bookmarks", comment: "Sync Paused Errors - Bookmarks Limit Exceeded Action")
     static let credentialsLimitExceededAction = NSLocalizedString("credentials.limit.exceeded.action", bundle: Bundle.module, value: "Manage Logins", comment: "Sync Paused Errors - Credentials Limit Exceeded Action")
+    // Sync Filtered Items Errors
+    static let invalidBookmarksPresentTitle = NSLocalizedString("bookmarks.invalid.objects.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("credentials.invalid.objects.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, numberOfOtherInvalidItems: Int) -> String {
+        let message = NSLocalizedString("bookmarks.invalid.objects.present.description", bundle: Bundle.module, comment: "Do not translate - stringsdict entry")
+        return String(format: message, numberOfOtherInvalidItems, invalidItemTitle)
+    }
+
+    static func invalidCredentialsPresentDescription(_ invalidItemTitle: String, numberOfOtherInvalidItems: Int) -> String {
+        let message = NSLocalizedString("credentials.invalid.objects.present.description", bundle: Bundle.module, comment: "Do not translate - stringsdict entry")
+        return String(format: message, numberOfOtherInvalidItems, invalidItemTitle)
+    }
+
     // Synced Devices
     static let syncedDevicesSectionHeader = NSLocalizedString("synced.devices.section.header", bundle: Bundle.module, value: "Synced Devices", comment: "Synced Devices - Section Header")
     static let syncedDevicesThisDeviceLabel = NSLocalizedString("synced.devices.this.device.label", bundle: Bundle.module, value: "This Device", comment: "Synced Devices - This Device Label")

diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Views/SyncSettingsView.swift b/LocalPackages/SyncUI/Sources/SyncUI/Views/SyncSettingsView.swift
@@ -59,6 +59,14 @@ public struct SyncSettingsView: View {
                         syncPaused(for: .credentials)
                     }
 
+                    if !model.invalidBookmarksTitles.isEmpty {
+                        syncHasInvalidItems(for: .bookmarks)
+                    }
+
+                    if !model.invalidCredentialsTitles.isEmpty {
+                        syncHasInvalidItems(for: .credentials)
+                    }
+
                     devices()
 
                     options()

diff --git a/LocalPackages/SyncUI/Sources/SyncUI/Views/SyncSettingsViewExtension.swift b/LocalPackages/SyncUI/Sources/SyncUI/Views/SyncSettingsViewExtension.swift
@@ -286,6 +286,53 @@ extension SyncSettingsView {
         }
     }
 
+    @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,
+                    numberOfOtherInvalidItems: model.invalidBookmarksTitles.count - 1
+                )
+
+            case .credentials:
+                assert(!model.invalidCredentialsTitles.isEmpty)
+                let firstInvalidCredentialTitle = model.invalidCredentialsTitles.first ?? ""
+                return UserText.invalidCredentialsPresentDescription(
+                    firstInvalidCredentialTitle,
+                    numberOfOtherInvalidItems: model.invalidCredentialsTitles.count - 1
+                )
+            }
+        }
+        var actionTitle: String {
+            switch itemType {
+            case .bookmarks:
+                return UserText.bookmarksLimitExceededAction
+            case .credentials:
+                return UserText.credentialsLimitExceededAction
+            }
+        }
+        SyncWarningMessageView(title: title, message: description, buttonTitle: actionTitle) {
+            switch itemType {
+            case .bookmarks:
+                model.manageBookmarks()
+            case .credentials:
+                model.manageLogins()
+            }
+        }
+    }
+
     @ViewBuilder
     func devEnvironmentIndicator() -> some View {
         if model.isOnDevEnvironment {
-Original file line number
+Diff line change
@@ Expand Up @@
             []
         }
+        func accountTitlesForSyncableCredentials(modifiedBefore date: Date) throws -> [String] {
+            []
+        }
         func deleteSyncableCredentials(_ syncableCredentials: SecureVaultModels.SyncableCredentials, in database: Database) throws {
         }
@@ Expand Down Expand Up / @@ -403,6 +407,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 @@