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

Table connecting verbs and modes #60

Closed
wants to merge 5 commits into from

Conversation

Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in favor as currently phrased, and also not a big fan of the idea in general as it ties things closely to HTTP (but might clarify things for people). Could be considered as a note.

What I don't like:

  • Makes assumptions about URI form and child–parent relationship that are not in the spec.
  • "Write on resulting resource" seems incorrect
  • "Patch instructions contain deletions" is not exact or complete
  • Read access is also needed for some patches
  • DELETE has not been clarified yet, so issue should be on hold in any case
  • The "empty" exception for DELETE is unknown to me.

@TallTed
Copy link
Contributor

TallTed commented Jun 21, 2019

ties things closely to HTTP

@RubenVerborgh - Perhaps you might be less against the idea if surrounding phrasing described the HTTP verbs as clarifying examples, rather than implying (if only by not mentioning any other verbs/systems) that they're the only possible methods?

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jun 21, 2019

@RubenVerborgh and @timbl, let's discuss this in person on Monday!

@michielbdejong
Copy link
Contributor Author

Sure, we can move this to a note, or even a wiki page, or part of the readme of the testsuite, as long as it's written down somewhere exactly what modes are required for exactly which actions.

@michielbdejong
Copy link
Contributor Author

@RubenVerborgh @acoburn changes applied, please re-review?

Copy link
Member

@acoburn acoburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is really useful, but it is also a particular interpretation of the text in the specification. I would suggest that some explicit note be added that this table is for general background (non-normative) by implementations but not a (normative) statement of requirement.

README.md Outdated
| PATCH | /foo/bar | patch instructions contain only insertions | Append or Write on /foo/bar |
| DELETE | /foo/bar | | Write on /foo/bar |
| DELETE | /foo/ | /foo/ is empty | Write on /foo/ |
| DELETE | /foo/ | No resources other than /foo/ will get deleted | Write on /foo/ |
Copy link
Member

@acoburn acoburn Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than the DELETE note in the line above? Perhaps just adding the note about "No resource other than..." to the delete method above would be appropriate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DELETE /foo/bar seems obviously different than DELETE /foo/...

The first (tries to) DELETE a resource (/foo/bar).
The second (tries to) DELETE a container (/foo/).

That said, in several (but not all) other rows, the comment in the 3rd column is read with the titular preceding "if...", so "No resources other than /foo/ will get deleted" is definitely incorrect, and /foo/ is empty was closer to correct. Possibly, [if] /foo/ contains no other resources would be better. Possibly, the latter columns of this table should be reconsidered and rewritten overall, perhaps adding a column for "results" such that multiple IF conditions (e.g., if /foo/ contains another resource) could also be addressed in this table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All true! The root problem here is that the 'container is empty' thing implicitly ties WAC to Solid which we don't want. We should say 'For instance like when you're using WAC as part of Solid', and then link to https://github.com/solid/solid-spec/blob/master/api-rest.md#deleting-containers

| PUT | /foo/bar | /foo/ gets created | Append or Write on / + Append or Write on /foo/ + Write on /foo/bar |
| PATCH | /foo/bar | patch instructions contain deletions | Write on /foo/bar |
| PUT | /foo/bar | /foo/ exists, will contain /foo/bar | Append or Write on /foo/ + Write on resulting resource |
| PUT | /foo/bar | /foo/ gets created, will contain /foo/bar | Append or Write on / + Append or Write on /foo/ + Write on /foo/bar |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interaction (recursive create of intermediate directories) is not defined in the specification. If we are describing this as an example, it should be clear whether this is an expected interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! This implicitly ties WAC to Solid which we don't want. We should say 'For instance like when you're using WAC as part of Solid which has mkdir -p behaviour', and then link to https://github.com/solid/solid-spec/blob/master/api-rest.md#http-put-to-create

| PATCH | /foo/bar | patch instructions contain deletions | Write on /foo/bar |
| PUT | /foo/bar | /foo/ exists, will contain /foo/bar | Append or Write on /foo/ + Write on resulting resource |
| PUT | /foo/bar | /foo/ gets created, will contain /foo/bar | Append or Write on / + Append or Write on /foo/ + Write on /foo/bar |
| PATCH | /foo/bar | patch instructions contain deletions | Read and Write on /foo/bar |
| PATCH | /foo/bar | patch instructions contain only insertions | Append or Write on /foo/bar |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PATCH instructions that only contain insertions, I can imagine that Read would be useful or necessary in an implementation. For example, given the document:

<> a <https://example.com/MyType> .

the SPARQL Query:

INSERT {
    <> a <https://example.com/MyType> .
} WHERE {}

would result in no change to the original Graph, but that cannot be determined unless the document is first read into a Graph.

@michielbdejong
Copy link
Contributor Author

The data browser Sharing pane offers 5 useful defaults:

  • 'owners' is read+write+control, "can read, write, and control sharing."
  • 'editors' is read+write, "can read and change information"
  • 'posters' is read+append, "can add new information, and read but not change existing information"
  • 'submitters' is append, "can add new information but not read any"
  • 'viewers' is read, "can read but not change information"

This is then to be interpreted differently for a container than for a non-container. For a container, read permissions means you can read its ldp:contains triples. For a non-container resource, it means you can read its contents as it was last written or updated through POST, PUT or PATCH (and optionally make the server do some RDF operations on it).

For creating a new resource, I confirmed empirically against NSS that if you choose the URL with PUT, then write on the container is not enough, you also need write on the resulting resource. Append on the resulting resource is also not enough then.

But what's funny is that if you append to a container with POST, then append on the container is enough and no rights at all at the resulting resource are needed.

For DELETE, write on the container seems to be enough. You don't need to have any write permissions on the resource. This is weird because it means that there can be resources which you can DELETE but not PUT or PATCH.

* No permissions on resulting resource required for POST
* Require write on container and on resource for DELETE
@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jan 19, 2020

As mentioned in solid/solid-spec#193 (comment) the success/failure of a DELETE can expose existence, and therefore it should require that read access. It's a diversion from POSIX but it makes sense and it's how NSS currently implemented it for PATCH. That discusssion is only in the context of deleting triples from an RDF source, but we could equally apply it to containers; to delete a resource you should probably have access to read the container it's in.
However, that's not what NSS does (I just tried:

curl -X DELETE https://michielbdejong.inrupt.net/public/public-write/test.ttl

and that succeeded without read access to https://michielbdejong.inrupt.net/public/public-write/)
so I guess the reasoning for deleting triples inside a file is slightly different from the one applied for deleting files from a folder. So this table is correct in stating that DELETE does not require container read, but PATCH containing deletions does require Read in addition to Write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants