Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify container notifications for websockets-pubsub #233

Closed
michielbdejong opened this issue Feb 12, 2021 · 7 comments
Closed

Clarify container notifications for websockets-pubsub #233

michielbdejong opened this issue Feb 12, 2021 · 7 comments

Comments

@michielbdejong
Copy link
Contributor

I can think of three ways to interpret the text "all CRUD operations (POST, PUT, PATCH, DELETE) performed on resources of that container will trigger a notification for the container URI" in https://github.com/solid/solid-spec/blob/master/api-websockets.md#subscription:

  • literally for Create, Read, Update, and Delete (Probably not! That makes little sense and also contradicts the earlier phrase "If a change occurs").
  • For Create, Update, and Delete, but not for Read. That would justify the statement if the containment triples of the container change, i.e. if a resource is created or removed.
  • only for Create and Delete, and not for Read or Update. That has always been how I interpreted, and the fact that the example uses DELETE and not PATCH is at least consistent with it. So this is basically: only publish on the container if that container's containment triples changed. Which in itself is quite neat and simple, I think.

PS: I think we agree that "resources of that container" means only resources that are directly contained in it (so equivalent of ls on unix), so not resources that are contained in subcontainers (what would be the equivalent of a recursive ls -R on unix). But let's add a test in solid-crud-tests to be sure that servers don't notify for all ancestor containers up to the root.

@csarven
Copy link
Member

csarven commented Feb 12, 2021

Thanks for raising this! Will incorporate this in the Protocol specification.

I offer an explanation that is closer to your third interpretation but with some differences. It is based on what's currently in the Protocol specification and also aligned with security considerations eg. #228 :

As per reading resources, a WebSocket subscription to a container requires read access privilege on the container in order to observe changes to container's state. Observable changes generally include changes to the containment statements ie. when adding or removing resources. Updates to the container (as with any resource) are also state changes eg. when adding a label, and so triggers a notification to subscribers.

Aside: at this time, the Protocol specification doesn't require that container representation includes anything beyond containment statements. See also: #227

As per PR 228:
When a server includes information beyond containment statements about the contained resources in the response with proper authorization, eg. last modification, the server triggers a notification about the container to its subscribers.

When a server creates a resource on HTTP PATCH, the container representation of the created resource will be effected as per containment rules. Hence, a notification is triggered.

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Feb 12, 2021

Thanks! I agree that is the interpretation that makes the most sense. It is also what the test suite expects (hence the failing tests for CSS).

@michielbdejong
Copy link
Contributor Author

@timbl can you take a decision on this?

@michielbdejong
Copy link
Contributor Author

@RubenVerborgh Just to clarify, are you saying that current CSS behavior leaks less information?

@michielbdejong
Copy link
Contributor Author

Can you give an example where resource r is added to or removed from container c (a containment triple is added or removed for c) and emitting only on r instead of on both r and c leaks less information?

@michielbdejong
Copy link
Contributor Author

On a PATCH, we do not want to leak whether it has succeeded or not, given that an agent might have Write but not Read

Sure but subscribing to resource changes via websockets-pubsub is already a read operation by definition. That's already true even if you emit only on r (as CSS already does). Emitting also on containers whose triples change doesn't change anything about what clients without read access can find out.

If CSS doesn't emit for containers, then how can you know if a new post came into your inbox?

Or do you make an exception for POST? If so, then how can you find out if a new resource was added to a container using a verb other than POST?

Another downside of your interpretation is scalability, I think. If a container has 1,000 resources that rarely change then the client would need to send 1,000 sub requests to know if any of them are deleted. That's An edge case, of course.

The main downside of your proposal is, I think, that it treats containment triple changes as somehow less real than other changes in non-container RDF sources. It raises all sorts of questions: do you also not emit if a container is changed with POST? Do you also not emit if other triples the container may have (e.g. folder title) are changed? What makes containment triples so special that we shouldn't emit a pub when they change?

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Feb 14, 2021

The CSS behavior can be seen here.

  1. On POST (as well as on DELETE), updates will be signaled to the parent as well.

Ah great, so then we all agree that adding or removing a containment triple does count as a change for which pub should be emitted. That was the question I wanted to get clarified in this issue. So then we all agree! :) Closing this issue.

  1. On PUT currently not; we do not take PATCH-to-create into account yet.

Great! "not yet" is fine, should be easy to fix. See https://github.com/solid/community-server/issues/612 and @joachimvh's comment there.

  1. On PATCH, we say unconditionally "modified" for security reasons.

Good point about unconditionally emitting on PATCH, I'll add a test for that in solid-crud-tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

2 participants