Better handling of 403 for attachment metadata upload

Check file-editing access for the group from the API before offering to reset, update the filesEditable setting properly, and restart the sync automatically after resetting.
Juris-M · Feb 23, 2018 · c0b63e5 · c0b63e5
1 parent 5ed10c6
commit c0b63e5
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 12 deletions.
diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js
@@ -46,6 +46,7 @@ Zotero.Sync.Data.Engine = function (options) {
 	}
 
 	this.apiClient = options.apiClient;
+	this.userID = options.userID;
 	this.libraryID = options.libraryID;
 	this.library = Zotero.Libraries.get(options.libraryID);
 	this.libraryTypeID = this.library.libraryTypeID;
@@ -1924,6 +1925,12 @@ Zotero.Sync.Data.Engine.prototype._handleUploadError = Zotero.Promise.coroutine(
 			return this.UPLOAD_RESULT_OBJECT_CONFLICT;
 		}
 	}
+	else if (e.name == "ZoteroUploadRestartError") {
+		return this.UPLOAD_RESULT_RESTART;
+	}
+	else if (e.name == "ZoteroUploadCancelError") {
+		return this.UPLOAD_RESULT_CANCEL;
+	}
 	throw e;
 });
 
@@ -1984,17 +1991,44 @@ Zotero.Sync.Data.Engine.prototype._checkObjectUploadError = Zotero.Promise.corou
 		}
 	}
 	else if (code == 403) {
-		// Prompt to reset local group files on 403 for file attachment upload
+		// If we get a 403 for a local group attachment, check the group permissions to confirm
+		// that we no longer have file-editing access and prompt to reset local group files
 		if (objectType == 'item') {
 			let item = Zotero.Items.getByLibraryAndKey(this.libraryID, key);
 			if (this.library.libraryType == 'group' && item.isFileAttachment()) {
-				let index = Zotero.Sync.Storage.Utilities.showFileWriteAccessLostPrompt(
-					null, this.library
-				);
-				if (index === 0) {
-					yield Zotero.Sync.Data.Local.resetUnsyncedLibraryFiles(this.libraryID);
+				let reset = false;
+				let groupID = Zotero.Groups.getGroupIDFromLibraryID(this.libraryID);
+				let info = yield this.apiClient.getGroup(groupID);
+				if (info) {
+					Zotero.debug(info);
+					let { editable, filesEditable } = Zotero.Groups.getPermissionsFromJSON(
+						info.data, this.userID
+					);
+					// If we do still have file-editing access, something else went wrong,
+					// and we should just fail without resetting
+					if (!filesEditable) {
+						let index = Zotero.Sync.Storage.Utilities.showFileWriteAccessLostPrompt(
+							null, this.library
+						);
+
+						let e = new Error(message);
+						if (index === 0) {
+							let group = Zotero.Groups.get(groupID);
+							group.filesEditable = false;
+							yield group.saveTx();
+
+							yield Zotero.Sync.Data.Local.resetUnsyncedLibraryFiles(this.libraryID);
+							e.name = "ZoteroUploadRestartError";
+						}
+						else {
+							e.name = "ZoteroUploadCancelError";
+						}
+						throw e;
+					}
+				}
+				else {
+					Zotero.logError("Couldn't get metadata for group " + groupID);
 				}
-				return false;
 			}
 		}
 	}

diff --git a/chrome/content/zotero/xpcom/sync/syncRunner.js b/chrome/content/zotero/xpcom/sync/syncRunner.js
@@ -175,6 +175,7 @@ Zotero.Sync.Runner_Module = function (options = {}) {
 			}
 
 			let engineOptions = {
+				userID: keyInfo.userID,
 				apiClient: client,
 				caller: this.caller,
 				setStatus: this.setSyncStatus.bind(this),

diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
@@ -6,6 +6,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 	var apiKey = Zotero.Utilities.randomString(24);
 	var baseURL = "http://local.zotero/";
 	var engine, server, client, caller, stub, spy;
+	var userID = 1;
 
 	var responses = {};
 
@@ -29,6 +30,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 		});
 
 		var engine = new Zotero.Sync.Data.Engine({
+			userID,
 			apiClient: client,
 			libraryID: options.libraryID || Zotero.Libraries.userLibraryID,
 			stopOnError
@@ -171,7 +173,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 
 		Zotero.HTTP.mock = sinon.FakeXMLHttpRequest;
 
-		yield Zotero.Users.setCurrentUserID(1);
+		yield Zotero.Users.setCurrentUserID(userID);
 		yield Zotero.Users.setCurrentUsername("testuser");
 	})
 
@@ -3027,7 +3029,9 @@ describe("Zotero.Sync.Data.Engine", function () {
 
 
 		it("should show file-write-access-lost dialog on 403 for attachment upload in group", async function () {
-			var group = await createGroup();
+			var group = await createGroup({
+				filesEditable: true
+			});
 			var libraryID = group.libraryID;
 			var libraryVersion = 5;
 			group.libraryVersion = libraryVersion;
@@ -3071,16 +3075,42 @@ describe("Zotero.Sync.Data.Engine", function () {
 						})
 					);
 				}
+				else if (called == 1 && req.url == baseURL + `groups/${group.id}`) {
+					req.respond(
+						200,
+						{
+							"Last-Modified-Version": group.libraryVersion
+						},
+						JSON.stringify({
+							id: group.id,
+							version: group.libraryVersion,
+							data: {
+								id: group.id,
+								version: group.libraryVersion,
+								name: group.name,
+								owner: 10,
+								type: "Private",
+								description: "",
+								url: "",
+								libraryEditing: "members",
+								libraryReading: "all",
+								fileEditing: "admins"
+							}
+						})
+					);
+				}
 				called++;
 			});
 
 			var promise = waitForDialog();
 			var spy = sinon.spy(engine, "onError");
 			var result = await engine._startUpload();
 			assert.isTrue(promise.isResolved());
-			assert.equal(result, engine.UPLOAD_RESULT_SUCCESS);
-			assert.equal(called, 1);
-			assert.equal(spy.callCount, 1);
+			assert.equal(result, engine.UPLOAD_RESULT_RESTART);
+			assert.equal(called, 2);
+			assert.equal(spy.callCount, 0);
+
+			assert.isFalse(group.filesEditable);
 
 			assert.ok(Zotero.Items.get(item1.id));
 			assert.isFalse(Zotero.Items.get(item2.id));