Disallow unowned group annotation edits, but allow deletions

Update `DataObject::isEditable()` to take an optional `op` argument to
test individual operations as opposed to general library editing.
Erasing objects now tests `erase`, and `Item::isEditable()` allows
`erase` for unowned group annotations while disallowing the default
`edit`.

It's still up to the reader to handle this appropriately in the UI and
not allow operations it shouldn't, but this enforces it in the data
layer.

chrome/content/zotero/xpcom/data/dataObject.js

@@ -850,27 +850,17 @@ Zotero.DataObject.prototype._markForReload = function (dataType) {
 }
 
 
-Zotero.DataObject.prototype.isEditable = function () {
-	return Zotero.Libraries.get(this.libraryID).editable;
-}
-
-
-Zotero.DataObject.prototype.editCheck = function () {
+/**
+ * @param {String} [op='edit'] - Operation to check; if not provided, check edit privileges for
+ *     library
+ */
+Zotero.DataObject.prototype.isEditable = function (_op = 'edit') {
 	let library = Zotero.Libraries.get(this.libraryID);
-	if ((this._objectType == 'collection' || this._objectType == 'search')
-			&& library.libraryType == 'publications') {
-		throw new Error(this._ObjectTypePlural + " cannot be added to My Publications");
-	}
-	
 	if (library.libraryType == 'feed') {
-		return;
+		return true;
 	}
-	
-	if (!this.isEditable()) {
-		throw new Error("Cannot edit " + this._objectType + " in read-only library "
-			+ Zotero.Libraries.get(this.libraryID).name);
-	}
-}
+	return library.editable;
+};
 
 /**
  * Save changes to database
@@ -998,7 +988,10 @@ Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env)
 	env.isNew = !this.id;
 	
 	if (!env.options.skipEditCheck) {
-		this.editCheck();
+		if (!this.isEditable()) {
+			throw new Error("Cannot edit " + this._objectType + " in library "
+				+ Zotero.Libraries.get(this.libraryID).name);
+		}
 	}
 	
 	let targetLib = Zotero.Libraries.get(this.libraryID);
@@ -1293,7 +1286,12 @@ Zotero.DataObject.prototype._initErase = Zotero.Promise.method(function (env) {
 		key: this.key
 	};
 	
-	if (!env.options.skipEditCheck) this.editCheck();
+	if (!env.options.skipEditCheck) {
+		if (!this.isEditable('erase')) {
+			throw new Error(`Cannot erase ${this._objectType} in library `
+				+ Zotero.Libraries.get(this.libraryID).name);
+		}
+	}
 	
 	return true;
 });

chrome/content/zotero/xpcom/data/item.js

@@ -1228,21 +1228,46 @@ Zotero.Item.prototype.removeRelatedItem = Zotero.Promise.coroutine(function* (it
 });
 
 
-Zotero.Item.prototype.isEditable = function() {
-	var editable = Zotero.Item._super.prototype.isEditable.apply(this);
+/**
+ * @param {String} [op='edit'] - Operation to check; if not provided, check edit privileges for
+ *     library
+ */
+Zotero.Item.prototype.isEditable = function (op = 'edit') {
+	// DataObject::isEditable() checks if library is editable
+	var editable = Zotero.Item._super.prototype.isEditable.call(this, op);
 	if (!editable) return false;
 	
-	// Check if we're allowed to save attachments
+	// Check if we're allowed to edit file attachments
 	if (this.isAttachment()
-		&& (this.attachmentLinkMode == Zotero.Attachments.LINK_MODE_IMPORTED_URL ||
-			this.attachmentLinkMode == Zotero.Attachments.LINK_MODE_IMPORTED_FILE)
-		&& !Zotero.Libraries.get(this.libraryID).filesEditable
-	) {
+			&& (this.attachmentLinkMode == Zotero.Attachments.LINK_MODE_IMPORTED_URL
+				|| this.attachmentLinkMode == Zotero.Attachments.LINK_MODE_IMPORTED_FILE)
+			&& !Zotero.Libraries.get(this.libraryID).filesEditable) {
 		return false;
 	}
 	
+	switch (op) {
+		case 'edit':
+			// Group library annotations created by other users aren't editable
+			if (this.isAnnotation()) {
+				let library = this.library;
+				if (library.isGroup
+						&& this.createdByUserID
+						&& this.createdByUserID != Zotero.Users.getCurrentUserID()) {
+					return false;
+				}
+			}
+			break;
+		
+		case 'erase':
+			break;
+		
+		default:
+			throw new Error(`Unknown operation ${op}`);
+	}
+	
+	
 	return true;
-}
+};
 
 Zotero.Item.prototype._initSave = Zotero.Promise.coroutine(function* (env) {
 	if (!this.itemTypeID) {

test/tests/dataObjectTest.js

@@ -343,7 +343,7 @@ describe("Zotero.DataObject", function() {
 				let item = createUnsavedDataObject('item', { libraryID: group.libraryID });
 				var e = yield getPromiseError(item.saveTx());
 				assert.ok(e);
-				assert.include(e.message, "read-only");
+				assert.include(e.message, "Cannot edit item");
 			});
 			
 			it("should allow saving if skipEditCheck is passed", function* () {
@@ -362,14 +362,6 @@ describe("Zotero.DataObject", function() {
 				assert.isFalse(e);
 			});
 		});
-		
-		describe("Options", function () {
-			describe("#skipAll", function () {
-				it("should include edit check", function* () {
-					
-				});
-			});
-		});
 	})
 	
 	describe("#erase()", function () {

test/tests/itemTest.js

@@ -1466,6 +1466,66 @@ describe("Zotero.Item", function () {
 				assert.sameMembers(items, [annotation1, annotation2]);
 			});
 		});
+		
+		describe("#isEditable()", function () {
+			var group;
+			var groupAttachment;
+			var groupAnnotation1;
+			var groupAnnotation2;
+			var groupAnnotation3;
+			
+			before(async function () {
+				await Zotero.Users.setCurrentUserID(1);
+				await Zotero.Users.setName(1, 'Abc');
+				await Zotero.Users.setName(12345, 'Def');
+				group = await createGroup();
+				groupAttachment = await importFileAttachment('test.pdf', { libraryID: group.libraryID });
+				groupAnnotation1 = await createAnnotation('highlight', groupAttachment);
+				groupAnnotation2 = await createAnnotation('highlight', groupAttachment, { createdByUserID: Zotero.Users.getCurrentUserID() });
+				groupAnnotation3 = await createAnnotation('highlight', groupAttachment, { createdByUserID: 12345 });
+			});
+			
+			describe("'edit'", function () {
+				it("should return true for personal library annotation", async function () {
+					var item = await createDataObject('item');
+					var attachment = await importFileAttachment('test.pdf', { parentID: item.id });
+					var annotation = await createAnnotation('highlight', attachment);
+					assert.isTrue(annotation.isEditable());
+				});
+				
+				it("should return true for group annotation created locally", async function () {
+					assert.isTrue(groupAnnotation1.isEditable());
+				});
+				
+				it("should return true for group annotation created by current user elsewhere", async function () {
+					assert.isTrue(groupAnnotation2.isEditable());
+				});
+				
+				it("should return false for annotations created by another user", async function () {
+					assert.isFalse(groupAnnotation3.isEditable());
+				});
+				
+				it("shouldn't allow editing of group annotation owned by another user", async function () {
+					var annotation = await createAnnotation('image', groupAttachment, { createdByUserID: 12345 });
+					
+					annotation.annotationComment = 'foobar';
+					var e = await getPromiseError(annotation.saveTx());
+					assert.ok(e);
+					assert.include(e.message, "Cannot edit item");
+				});
+			});
+			
+			describe("'erase'", function () {
+				it("should return true for annotations created by another user", async function () {
+					assert.isTrue(groupAnnotation3.isEditable('erase'));
+				});
+				
+				it("should allow deletion of group annotation owned by another user", async function () {
+					var annotation = await createAnnotation('image', groupAttachment, { createdByUserID: 12345 });
+					await annotation.eraseTx();
+				});
+			});
+		});
 	});
 	
 	
@@ -1711,7 +1771,7 @@ describe("Zotero.Item", function () {
 			);
 		});
 		
-		it("should remove an item in a collection in a read-only library", async function () {
+		it("should remove an item in a collection in a read-only library with 'skipEditCheck'", async function () {
 			var group = await createGroup();
 			var libraryID = group.libraryID;
 			var collection = await createDataObject('collection', { libraryID });