Improved validation of creators during sync (#747)

zotero · Aug 14, 2023 · a589e52 · a589e52
1 parent fae3a84
commit a589e52
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 29 deletions.
diff --git a/ZShare/DbRequests/CreateBackendItemDbRequest.swift b/ZShare/DbRequests/CreateBackendItemDbRequest.swift
@@ -24,7 +24,14 @@ struct CreateBackendItemDbRequest: DbResponseRequest {
             throw DbError.objectNotFound
         }
 
-        _ = try StoreItemsDbResponseRequest(responses: [self.item], schemaController: self.schemaController, dateParser: self.dateParser, preferResponseData: true).process(in: database)
+        _ = try StoreItemsDbResponseRequest(
+            responses: [self.item],
+            schemaController: self.schemaController,
+            dateParser: self.dateParser,
+            preferResponseData: true,
+            denyIncorrectCreator: false
+        )
+        .process(in: database)
 
         guard let item = database.objects(RItem.self).filter(.key(self.item.key, in: libraryId)).first else {
             throw DbError.objectNotFound

diff --git a/ZShare/DbRequests/CreateItemWithAttachmentDbRequest.swift b/ZShare/DbRequests/CreateItemWithAttachmentDbRequest.swift
@@ -21,10 +21,14 @@ struct CreateItemWithAttachmentDbRequest: DbResponseRequest {
     var needsWrite: Bool { return true }
 
     func process(in database: Realm) throws -> (RItem, RItem) {
-        _ = try StoreItemsDbResponseRequest(responses: [self.item],
-                                            schemaController: self.schemaController,
-                                            dateParser: self.dateParser,
-                                            preferResponseData: true).process(in: database)
+        _ = try StoreItemsDbResponseRequest(
+            responses: [self.item],
+            schemaController: self.schemaController,
+            dateParser: self.dateParser,
+            preferResponseData: true,
+            denyIncorrectCreator: false
+        )
+        .process(in: database)
 
         guard let item = database.objects(RItem.self).filter(.key(self.item.key, in: self.attachment.libraryId)).first else {
             throw DbError.objectNotFound
@@ -35,12 +39,15 @@ struct CreateItemWithAttachmentDbRequest: DbResponseRequest {
         item.fields.forEach({ $0.changed = true })
 
         let localizedType = self.schemaController.localized(itemType: ItemTypes.attachment) ?? ""
-        let attachment = try CreateAttachmentDbRequest(attachment: self.attachment,
-                                                       parentKey: nil,
-                                                       localizedType: localizedType,
-                                                       includeAccessDate: self.attachment.hasUrl,
-                                                       collections: [],
-                                                       tags: []).process(in: database)
+        let attachment = try CreateAttachmentDbRequest(
+            attachment: self.attachment,
+            parentKey: nil,
+            localizedType: localizedType,
+            includeAccessDate: self.attachment.hasUrl,
+            collections: [],
+            tags: []
+        )
+        .process(in: database)
 
         attachment.parent = item
         attachment.changes.append(RObjectChange.create(changes: RItemChanges.parent))

diff --git a/Zotero/Controllers/Database/Requests/CreateTranslatedItemsDbRequest.swift b/Zotero/Controllers/Database/Requests/CreateTranslatedItemsDbRequest.swift
@@ -20,7 +20,14 @@ struct CreateTranslatedItemsDbRequest: DbRequest {
 
     func process(in database: Realm) throws {
         for response in self.responses {
-            let (item, _) = try StoreItemDbRequest(response: response, schemaController: self.schemaController, dateParser: self.dateParser, preferRemoteData: true).process(in: database)
+            let (item, _) = try StoreItemDbRequest(
+                response: response,
+                schemaController: self.schemaController,
+                dateParser: self.dateParser,
+                preferRemoteData: true,
+                denyIncorrectCreator: false
+            )
+            .process(in: database)
 
             item.changeType = .user
             for field in item.fields {

diff --git a/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift b/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift
@@ -119,19 +119,27 @@ struct MarkItemAsSyncedAndUpdateDbRequest: DbRequest {
         guard let item = database.objects(RItem.self).filter(.key(self.response.key, in: self.libraryId)).first else { return }
 
         item.deleteChanges(uuids: self.changeUuids, database: database)
-        self.updateUnchangedData(of: item, with: self.response, database: database)
+        try self.updateUnchangedData(of: item, with: self.response, database: database)
 
         if let parent = item.parent {
             // This is to mitigate the issue in item detail screen (ItemDetailActionHandler.shouldReloadData) where observing of `children` doesn't report changes between `oldValue` and `newValue`.
             parent.version = parent.version
         }
     }
 
-    private func updateUnchangedData(of item: RItem, with response: ItemResponse, database: Realm) {
+    private func updateUnchangedData(of item: RItem, with response: ItemResponse, database: Realm) throws {
         let localChanges = item.changedFields
 
         if localChanges.isEmpty {
-            _ = StoreItemDbRequest.update(item: item, libraryId: self.libraryId, with: response, schemaController: self.schemaController, dateParser: self.dateParser, database: database)
+            _ = try StoreItemDbRequest.update(
+                item: item,
+                libraryId: self.libraryId,
+                with: response,
+                denyIncorrectCreator: false,
+                schemaController: self.schemaController,
+                dateParser: self.dateParser,
+                database: database
+            )
             item.changeType = .syncResponse
             return
         }
@@ -168,7 +176,7 @@ struct MarkItemAsSyncedAndUpdateDbRequest: DbRequest {
         }
 
         if !localChanges.contains(.creators) {
-            StoreItemDbRequest.sync(creators: response.creators, item: item, schemaController: self.schemaController, database: database)
+            try StoreItemDbRequest.sync(creators: response.creators, item: item, denyIncorrectCreator: false, schemaController: self.schemaController, database: database)
         }
 
         if !localChanges.contains(.relations) {

diff --git a/Zotero/Controllers/Database/Requests/StoreItemsDbResponseRequest.swift b/Zotero/Controllers/Database/Requests/StoreItemsDbResponseRequest.swift
@@ -22,6 +22,8 @@ struct StoreItemsResponse {
     enum Error: Swift.Error {
         case itemDeleted(ItemResponse)
         case itemChanged(ItemResponse)
+        case noValidCreators(key: String, itemType: String)
+        case invalidCreator(key: String, creatorType: String)
     }
 
     let changedFilenames: [FilenameChange]
@@ -35,6 +37,7 @@ struct StoreItemsDbResponseRequest: DbResponseRequest {
     unowned let schemaController: SchemaController
     unowned let dateParser: DateParser
     let preferResponseData: Bool
+    let denyIncorrectCreator: Bool
 
     var needsWrite: Bool { return true }
 
@@ -44,7 +47,14 @@ struct StoreItemsDbResponseRequest: DbResponseRequest {
 
         for response in self.responses {
             do {
-                let (_, change) = try StoreItemDbRequest(response: response, schemaController: self.schemaController, dateParser: self.dateParser, preferRemoteData: self.preferResponseData).process(in: database)
+                let (_, change) = try StoreItemDbRequest(
+                    response: response,
+                    schemaController: self.schemaController,
+                    dateParser: self.dateParser,
+                    preferRemoteData: self.preferResponseData,
+                    denyIncorrectCreator: self.denyIncorrectCreator
+                )
+                .process(in: database)
                 if let change = change {
                     filenameChanges.append(change)
                 }
@@ -66,6 +76,7 @@ struct StoreItemDbRequest: DbResponseRequest {
     unowned let schemaController: SchemaController
     unowned let dateParser: DateParser
     let preferRemoteData: Bool
+    let denyIncorrectCreator: Bool
 
     var needsWrite: Bool { return true }
 
@@ -96,10 +107,26 @@ struct StoreItemDbRequest: DbResponseRequest {
             item.attachmentNeedsSync = false
         }
 
-        return StoreItemDbRequest.update(item: item, libraryId: libraryId, with: self.response, schemaController: self.schemaController, dateParser: self.dateParser, database: database)
+        return try StoreItemDbRequest.update(
+            item: item,
+            libraryId: libraryId,
+            with: self.response,
+            denyIncorrectCreator: self.denyIncorrectCreator,
+            schemaController: self.schemaController,
+            dateParser: self.dateParser,
+            database: database
+        )
     }
 
-    static func update(item: RItem, libraryId: LibraryIdentifier, with response: ItemResponse, schemaController: SchemaController, dateParser: DateParser, database: Realm) -> (RItem, StoreItemsResponse.FilenameChange?) {
+    static func update(
+        item: RItem,
+        libraryId: LibraryIdentifier,
+        with response: ItemResponse,
+        denyIncorrectCreator: Bool,
+        schemaController: SchemaController,
+        dateParser: DateParser,
+        database: Realm
+    ) throws -> (RItem, StoreItemsResponse.FilenameChange?) {
         item.key = response.key
         item.rawType = response.rawType
         item.localizedType = schemaController.localized(itemType: response.rawType) ?? ""
@@ -118,7 +145,7 @@ struct StoreItemDbRequest: DbResponseRequest {
         self.syncParent(key: response.parentKey, libraryId: libraryId, item: item, database: database)
         self.syncCollections(keys: response.collectionKeys, libraryId: libraryId, item: item, database: database)
         self.sync(tags: response.tags, libraryId: libraryId, item: item, database: database)
-        self.sync(creators: response.creators, item: item, schemaController: schemaController, database: database)
+        try self.sync(creators: response.creators, item: item, denyIncorrectCreator: denyIncorrectCreator, schemaController: schemaController, database: database)
         self.sync(relations: response.relations, item: item, database: database)
         self.syncLinks(data: response, item: item, database: database)
         self.syncUsers(createdBy: response.createdBy, lastModifiedBy: response.lastModifiedBy, item: item, database: database)
@@ -406,12 +433,21 @@ struct StoreItemDbRequest: DbResponseRequest {
         }
     }
 
-    static func sync(creators: [CreatorResponse], item: RItem, schemaController: SchemaController, database: Realm) {
+    static func sync(creators: [CreatorResponse], item: RItem, denyIncorrectCreator: Bool, schemaController: SchemaController, database: Realm) throws {
+        switch item.rawType {
+        case ItemTypes.annotation, ItemTypes.attachment, ItemTypes.note:
+            // These item types don't support creators, so `validCreators` would always be empty.
+            return
+
+        default:
+            break
+        }
+
         database.delete(item.creators)
 
         guard let validCreators = schemaController.creators(for: item.rawType), !validCreators.isEmpty else {
             DDLogError("StoreItemsDbResponseRequest: can't find valid creators for item type \(item.rawType). Skipping creators.")
-            return
+            throw StoreItemsResponse.Error.noValidCreators(key: item.key, itemType: item.rawType)
         }
 
         for (idx, object) in creators.enumerated() {
@@ -423,6 +459,8 @@ struct StoreItemDbRequest: DbResponseRequest {
 
             if validCreators.contains(where: { $0.creatorType == object.creatorType }) {
                 creator.rawType = object.creatorType
+            } else if denyIncorrectCreator {
+                throw StoreItemsResponse.Error.invalidCreator(key: item.key, creatorType: object.creatorType)
             } else if let primaryCreator = validCreators.first(where: { $0.primary }) {
                 DDLogError("StoreItemsDbResponseRquest: creator type '\(object.creatorType)' isn't valid for \(item.rawType) - changing to primary creator")
                 creator.rawType = primaryCreator.creatorType

diff --git a/Zotero/Controllers/Sync/SyncActions/RevertLibraryFilesSyncAction.swift b/Zotero/Controllers/Sync/SyncActions/RevertLibraryFilesSyncAction.swift
@@ -64,7 +64,13 @@ struct RevertLibraryFilesSyncAction: SyncAction {
                     try coordinator.perform(request: DeleteObjectsDbRequest<RItem>(keys: failedKeys, libraryId: self.libraryId))
                     // Store cached objects from backend to local database to get rid of local changes.
                     DDLogError("RevertLibraryFilesSyncAction: restore cached objects")
-                    let request = StoreItemsDbResponseRequest(responses: cachedResponses, schemaController: self.schemaController, dateParser: self.dateParser, preferResponseData: true)
+                    let request = StoreItemsDbResponseRequest(
+                        responses: cachedResponses,
+                        schemaController: self.schemaController,
+                        dateParser: self.dateParser,
+                        preferResponseData: true,
+                        denyIncorrectCreator: true
+                    )
                     changedFilenames = try coordinator.perform(request: request).changedFilenames
                     coordinator.invalidate()
                 }

diff --git a/Zotero/Controllers/Sync/SyncActions/RevertLibraryUpdatesSyncAction.swift b/Zotero/Controllers/Sync/SyncActions/RevertLibraryUpdatesSyncAction.swift
@@ -61,7 +61,13 @@ struct RevertLibraryUpdatesSyncAction: SyncAction {
                     try coordinator.perform(writeRequests: [storeCollectionsRequest, storeSearchesRequest])
 
                     // Force response data here, since we're reverting
-                    let storeItemsRequest = StoreItemsDbResponseRequest(responses: items.responses, schemaController: self.schemaController, dateParser: self.dateParser, preferResponseData: true)
+                    let storeItemsRequest = StoreItemsDbResponseRequest(
+                        responses: items.responses,
+                        schemaController: self.schemaController,
+                        dateParser: self.dateParser,
+                        preferResponseData: true,
+                        denyIncorrectCreator: true
+                    )
                     changes = try coordinator.perform(request: storeItemsRequest).changedFilenames
 
                     failedCollections = collections.failed

diff --git a/Zotero/Controllers/Sync/SyncBatchProcessor.swift b/Zotero/Controllers/Sync/SyncBatchProcessor.swift
@@ -162,7 +162,7 @@ final class SyncBatchProcessor {
             self.storeIndividualObjects(from: objects, type: .item, libraryId: libraryId)
 
             // BETA: - forcing preferResponseData to true for beta, it should be false here so that we report conflicts
-            let request = StoreItemsDbResponseRequest(responses: items, schemaController: self.schemaController, dateParser: self.dateParser, preferResponseData: true)
+            let request = StoreItemsDbResponseRequest(responses: items, schemaController: self.schemaController, dateParser: self.dateParser, preferResponseData: true, denyIncorrectCreator: true)
             let response = try self.dbStorage.perform(request: request, on: self.storageQueue, invalidateRealm: true)
             let failedKeys = self.failedKeys(from: expectedKeys, parsedKeys: items.map({ $0.key }), errors: errors)
 

diff --git a/ZoteroTests/SyncActionsSpec.swift b/ZoteroTests/SyncActionsSpec.swift
@@ -84,7 +84,8 @@ final class SyncActionsSpec: QuickSpec {
                             responses: [itemResponse],
                             schemaController: TestControllers.schemaController,
                             dateParser: TestControllers.dateParser,
-                            preferResponseData: true
+                            preferResponseData: true,
+                            denyIncorrectCreator: true
                         ),
                         on: .main
                     )
@@ -229,7 +230,8 @@ final class SyncActionsSpec: QuickSpec {
                             responses: [itemResponse],
                             schemaController: TestControllers.schemaController,
                             dateParser: TestControllers.dateParser,
-                            preferResponseData: true
+                            preferResponseData: true,
+                            denyIncorrectCreator: true
                         ),
                         on: .main
                     )
@@ -346,7 +348,8 @@ final class SyncActionsSpec: QuickSpec {
                             responses: [itemResponse],
                             schemaController: TestControllers.schemaController,
                             dateParser: TestControllers.dateParser,
-                            preferResponseData: true
+                            preferResponseData: true,
+                            denyIncorrectCreator: false
                         ),
                         on: .main
                     )

diff --git a/ZoteroTests/WebDavControllerSpec.swift b/ZoteroTests/WebDavControllerSpec.swift
@@ -130,7 +130,7 @@ final class WebDavControllerSpec: QuickSpec {
                                 switch error {
                                 case .sessionTaskFailed(let error):
                                     let nsError = error as NSError
-                                    if nsError.code == NSURLErrorCannotConnectToHost {
+                                    if nsError.code == NSURLErrorCannotConnectToHost || nsError.code == NSURLErrorAppTransportSecurityRequiresSecureConnection {
                                         finished()
                                         return
                                     }