Skip to content

Commit

Permalink
Enable field validation for Sync payloads (#804)
Browse files Browse the repository at this point in the history 
Task/Issue URL: https://app.asana.com/0/0/1207196051122978/f

Description:
This change enables local field validation mechanism for Sync payloads, implemented a while ago (#735)
and kept behind a feature flag. The validation mechanism 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.
  • Loading branch information
@ayoy
ayoy authored May 5, 2024
1 parent dedd5ef commit 9906b94
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 111 deletions.
7 changes: 0 additions & 7 deletions Sources/DDGSync/DDGSync.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,6 @@ import DDGSyncCrypto
import Foundation

public class DDGSync: DDGSyncing {
/**
* Temporary feature flag that controls field validation feature.
*
* The flag must be set to false at all times before field validation is released.
* Before field validations release, it should be removed.
*/
public static let isFieldValidationEnabled = false

public static let bundle = Bundle.module

Expand Down
7 changes: 1 addition & 6 deletions Sources/DDGSync/SyncMetadataStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,6 @@ public final class LocalSyncMetadataStore: SyncMetadataStore {
}

public func updateLocalTimestamp(_ localTimestamp: Date?, forFeatureNamed name: String) {
guard DDGSync.isFieldValidationEnabled else {
return
}
context.performAndWait {
let feature = SyncFeatureUtils.fetchFeature(with: name, in: context)
feature?.lastSyncLocalTimestamp = localTimestamp
Expand All @@ -138,9 +135,7 @@ public final class LocalSyncMetadataStore: SyncMetadataStore {
context.performAndWait {
let feature = SyncFeatureUtils.fetchFeature(with: name, in: context)
feature?.lastModified = serverTimestamp
if DDGSync.isFieldValidationEnabled {
feature?.lastSyncLocalTimestamp = localTimestamp
}
feature?.lastSyncLocalTimestamp = localTimestamp
feature?.featureState = state

try? context.save()
Expand Down
12 changes: 4 additions & 8 deletions Sources/SyncDataProviders/Bookmarks/internal/SyncableBookmarkAdapter.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,19 +125,15 @@ extension Syncable {

if let title = bookmark.title {
let encryptedTitle = try encrypt(title)
if DDGSync.isFieldValidationEnabled {
guard encryptedTitle.count <= BookmarkValidationConstraints.maxEncryptedBookmarkTitleLength else {
throw SyncableBookmarkError.validationFailed
}
guard encryptedTitle.count <= BookmarkValidationConstraints.maxEncryptedBookmarkTitleLength else {
throw SyncableBookmarkError.validationFailed
}
payload["title"] = encryptedTitle
}
if let url = bookmark.url {
let encryptedURL = try encrypt(url)
if DDGSync.isFieldValidationEnabled {
guard encryptedURL.count <= BookmarkValidationConstraints.maxEncryptedBookmarkURLLength else {
throw SyncableBookmarkError.validationFailed
}
guard encryptedURL.count <= BookmarkValidationConstraints.maxEncryptedBookmarkURLLength else {
throw SyncableBookmarkError.validationFailed
}
payload["page"] = ["url": encryptedURL]
}
Expand Down
32 changes: 11 additions & 21 deletions Sources/SyncDataProviders/Credentials/internal/SyncableCredentialsAdapter.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ extension Syncable {
static let maxEncryptedNotesLength = 1000
}

// swiftlint:disable:next cyclomatic_complexity function_body_length
// swiftlint:disable:next cyclomatic_complexity
init(syncableCredentials: SecureVaultModels.SyncableCredentials, encryptedUsing encrypt: (String) throws -> String) throws {
var payload: [String: Any] = [:]

Expand All @@ -85,47 +85,37 @@ extension Syncable {

if let title = credential.account.title {
let encryptedTitle = try encrypt(title)
if DDGSync.isFieldValidationEnabled {
guard encryptedTitle.count <= CredentialValidationConstraints.maxEncryptedTitleLength else {
throw SyncableCredentialError.validationFailed
}
guard encryptedTitle.count <= CredentialValidationConstraints.maxEncryptedTitleLength else {
throw SyncableCredentialError.validationFailed
}
payload["title"] = encryptedTitle
}
if let domain = credential.account.domain {
let encryptedDomain = try encrypt(domain)
if DDGSync.isFieldValidationEnabled {
guard encryptedDomain.count <= CredentialValidationConstraints.maxEncryptedDomainLength else {
throw SyncableCredentialError.validationFailed
}
guard encryptedDomain.count <= CredentialValidationConstraints.maxEncryptedDomainLength else {
throw SyncableCredentialError.validationFailed
}
payload["domain"] = encryptedDomain
}
if let username = credential.account.username {
let encryptedUsername = try encrypt(username)
if DDGSync.isFieldValidationEnabled {
guard encryptedUsername.count <= CredentialValidationConstraints.maxEncryptedUsernameLength else {
throw SyncableCredentialError.validationFailed
}
guard encryptedUsername.count <= CredentialValidationConstraints.maxEncryptedUsernameLength else {
throw SyncableCredentialError.validationFailed
}
payload["username"] = encryptedUsername
}
if let notes = credential.account.notes {
let encryptedNotes = try encrypt(notes)
if DDGSync.isFieldValidationEnabled {
guard encryptedNotes.count <= CredentialValidationConstraints.maxEncryptedNotesLength else {
throw SyncableCredentialError.validationFailed
}
guard encryptedNotes.count <= CredentialValidationConstraints.maxEncryptedNotesLength else {
throw SyncableCredentialError.validationFailed
}
payload["notes"] = encryptedNotes
}

if let passwordData = credential.password, let password = String(data: passwordData, encoding: .utf8) {
let encryptedPassword = try encrypt(password)
if DDGSync.isFieldValidationEnabled {
guard encryptedPassword.count <= CredentialValidationConstraints.maxEncryptedPasswordLength else {
throw SyncableCredentialError.validationFailed
}
guard encryptedPassword.count <= CredentialValidationConstraints.maxEncryptedPasswordLength else {
throw SyncableCredentialError.validationFailed
}
payload["password"] = encryptedPassword
}
Expand Down
15 changes: 2 additions & 13 deletions Tests/SyncDataProvidersTests/Bookmarks/BookmarksProviderTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,15 @@ internal class BookmarksProviderTests: BookmarksProviderTestsBase {

func testThatLastSyncTimestampIsNilByDefault() {
XCTAssertNil(provider.lastSyncTimestamp)
if DDGSync.isFieldValidationEnabled {
XCTAssertNil(provider.lastSyncLocalTimestamp)
}
XCTAssertNil(provider.lastSyncLocalTimestamp)
}

func testThatLastSyncTimestampIsPersisted() throws {
try provider.registerFeature(withState: .readyToSync)
let date = Date()
provider.updateSyncTimestamps(server: "12345", local: date)
XCTAssertEqual(provider.lastSyncTimestamp, "12345")
if DDGSync.isFieldValidationEnabled {
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}

func testThatPrepareForFirstSyncClearsLastSyncTimestampAndSetsModifiedAtForAllBookmarks() async throws {
Expand Down Expand Up @@ -146,9 +142,6 @@ internal class BookmarksProviderTests: BookmarksProviderTestsBase {
}

func testThatFetchChangedObjectsFiltersOutInvalidBookmarksAndTruncatesFolderTitles() async throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}
let context = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType)

let longTitle = String(repeating: "x", count: 10000)
Expand Down Expand Up @@ -262,10 +255,6 @@ internal class BookmarksProviderTests: BookmarksProviderTestsBase {
}

func testThatItemsThatFailedValidationRetainTheirTimestamps() async throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

let context = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType)
let longValue = String(repeating: "x", count: 10000)
let timestamp = Date()
Expand Down
12 changes: 0 additions & 12 deletions Tests/SyncDataProvidersTests/Bookmarks/SyncableBookmarkValidationTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,27 +60,15 @@ final class SyncableBookmarkValidationTests: XCTestCase {
}

func testWhenBookmarkFieldsPassLengthValidationThenSyncableIsInitializedWithoutThrowingErrors() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

XCTAssertNoThrow(try Syncable(bookmark: bookmark, encryptedUsing: { $0 }))
}

func testWhenBookmarkTitleIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

bookmark.title = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}

func testWhenBookmarkURLIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

bookmark.url = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}
Expand Down
16 changes: 2 additions & 14 deletions Tests/SyncDataProvidersTests/Credentials/CredentialsProviderTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,15 @@ final class CredentialsProviderTests: CredentialsProviderTestsBase {

func testThatLastSyncTimestampIsNilByDefault() {
XCTAssertNil(provider.lastSyncTimestamp)
if DDGSync.isFieldValidationEnabled {
XCTAssertNil(provider.lastSyncLocalTimestamp)
}
XCTAssertNil(provider.lastSyncLocalTimestamp)
}

func testThatLastSyncTimestampIsPersisted() throws {
try provider.registerFeature(withState: .readyToSync)
let date = Date()
provider.updateSyncTimestamps(server: "12345", local: date)
XCTAssertEqual(provider.lastSyncTimestamp, "12345")
if DDGSync.isFieldValidationEnabled {
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}

func testThatPrepareForFirstSyncClearsLastSyncTimestampAndSetsModifiedAtForAllCredentials() throws {
Expand Down Expand Up @@ -82,10 +78,6 @@ final class CredentialsProviderTests: CredentialsProviderTestsBase {
}

func testThatFetchChangedObjectsFiltersOutInvalidCredentials() async throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

let longValue = String(repeating: "x", count: 10000)

try secureVault.inDatabaseTransaction { database in
Expand Down Expand Up @@ -148,10 +140,6 @@ final class CredentialsProviderTests: CredentialsProviderTestsBase {
}

func testThatItemsThatFailedValidationRetainTheirTimestamps() async throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

let longValue = String(repeating: "x", count: 10000)
let timestamp = Date().withMillisecondPrecision

Expand Down
24 changes: 0 additions & 24 deletions Tests/SyncDataProvidersTests/Credentials/SyncableCredentialsValidationTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,54 +34,30 @@ final class SyncableCredentialsValidationTests: XCTestCase {
}

func testWhenCredentialsFieldsPassLengthValidationThenSyncableIsInitializedWithoutThrowingErrors() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

XCTAssertNoThrow(try Syncable(syncableCredentials: syncableCredentials, encryptedUsing: { $0 }))
}

func testWhenAccountTitleIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

syncableCredentials.account?.title = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}

func testWhenAccountUsernameIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

syncableCredentials.account?.username = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}

func testWhenAccountDomainIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

syncableCredentials.account?.domain = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}

func testWhenAccountNotesIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

syncableCredentials.account?.notes = String(repeating: "x", count: 10000)
assertSyncableInitializerThrowsValidationError()
}

func testWhenPasswordIsTooLongThenSyncableInitializerThrowsError() throws {
guard DDGSync.isFieldValidationEnabled else {
throw XCTSkip("Field validation is disabled")
}

syncableCredentials.credentials?.password = String(repeating: "x", count: 10000).data(using: .utf8)
assertSyncableInitializerThrowsValidationError()
}
Expand Down
8 changes: 2 additions & 6 deletions Tests/SyncDataProvidersTests/Settings/SettingsProviderTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,15 @@ final class SettingsProviderTests: SettingsProviderTestsBase {

func testThatLastSyncTimestampIsNilByDefault() {
XCTAssertNil(provider.lastSyncTimestamp)
if DDGSync.isFieldValidationEnabled {
XCTAssertNil(provider.lastSyncLocalTimestamp)
}
XCTAssertNil(provider.lastSyncLocalTimestamp)
}

func testThatLastSyncTimestampIsPersisted() throws {
try provider.registerFeature(withState: .readyToSync)
let date = Date()
provider.updateSyncTimestamps(server: "12345", local: date)
XCTAssertEqual(provider.lastSyncTimestamp, "12345")
if DDGSync.isFieldValidationEnabled {
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}
XCTAssertEqual(provider.lastSyncLocalTimestamp, date)
}

func testThatPrepareForFirstSyncClearsLastSyncTimestampAndSetsModifiedAtForAllSettings() throws {
Expand Down

0 comments on commit 9906b94

Please sign in to comment.