Update activity shouldn't remove existing permissions

[srosset81]

If I have an object that has anonymous read right, and I send a as:Update activity to my followers, then the anonymous read right will be removed.

This is why the test below now fails. We can get the resource as an anymous user when it was created (because the newResourcePermissions adds anonymous read by default), but after the as:Update activity is sent, it is removed, and we now need to fetch it with the webId of the user, or system (this would actually be the way to fix the test).

semapps/src/middleware/tests/activitypub/object.test.js

Lines 105 to 108 in c0fec29

	const object = await broker.call('ldp.resource.get', {
	resourceUri: objectUri,
	accept: MIME_TYPES.JSON
	});

This is the result of the new code introduced in #1332

semapps/src/middleware/packages/activitypub/services/activitypub/subservices/activity-handlers/setRightsHandler.js

Lines 108 to 119 in c0fec29

	// We remove rights from all actors that aren't recipients anymore.
	// *And* we do not skip the object-watcher middleware, to send `Delete` activities
	// to actors that cannot see the object anymore.
	if (removedRecipients.length > 0 || !activityIsPublic)
	await removeReadRights({
	ctx,
	resourceUri: objectUri,
	recipientUris: removedRecipients,
	skipObjectsWatcher: false,
	anon: !activityIsPublic
	});
	}

However I don't think it's a good practice, because permissions may have been added in other places. We may also send an as:Update activity to user X, and then another as:Update activity to user Y, but we don't want only user Y to have read access on the related object.

IMO for activities the recipients should be the single source of truth (they are immutable anyway, so no need to worry about updating rights), but not for the objects being created/updated with ActivityPub.

What do you think @Laurin-W ?

[srosset81]

I've also noticed that almost all interop tests fail, but if I remove the lines to remove read rights (the ones highlighted above), only two tests still fail.

[srosset81]

@Laurin-W I've commented out the code which removes permissions, as it generates too many errors in interop tests, and I'm afraid it also has unexpected consequences in other places. I've also fixed another test.

We can talk about this later, and see what to do, but I don't like to have failing SemApps tests (I know it's difficult to keep up with these tests because they are not automated. I'm also responsible for not being careful enough with that).

For me there is a real question about why an Update activity should remove permissions of an object it is referring to, and especially do that based on the recipients of the activity. If there is a need somewhere for this, the code should probably go somewhere else. But we can talk about it later...