search

assemblee-virtuelle / semapps

A toolbox to create semantic web applications
https://semapps.org
Apache License 2.0
88 stars 9 forks source link

Update activity shouldn't remove existing permissions #1336

Open srosset81 opened 5 days ago

srosset81 commented 5 days ago

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).

https://github.com/assemblee-virtuelle/semapps/blob/c0fec29803e959effcb9718abeb036078aef88b4/src/middleware/tests/activitypub/object.test.js#L105-L108

This is the result of the new code introduced in https://github.com/assemblee-virtuelle/semapps/pull/1332

https://github.com/assemblee-virtuelle/semapps/blob/c0fec29803e959effcb9718abeb036078aef88b4/src/middleware/packages/activitypub/services/activitypub/subservices/activity-handlers/setRightsHandler.js#L108-L119

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 commented 4 days ago

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 commented 4 days ago

@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...