-
Notifications
You must be signed in to change notification settings - Fork 25
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
notes about container write permissions #48
Conversation
@RubenVerborgh or @kjetilk, maybe one of you can review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the decision leading to this PR?
README.md
Outdated
@@ -461,6 +461,11 @@ API context, this would include `PUT`, `POST`, `DELETE` and `PATCH`. This also | |||
includes the ability to perform SPARQL queries that perform updates, if those | |||
are supported. | |||
|
|||
Write permissions on a container means the right to delete that container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diverges from an earlier decision where DELETE
requires write permissions on the container above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this about containers rather than resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The story you're referring to is a good example of a case where the spec process failed, it's described in 'NB2' below.
Right, I only described the container case because that's where I was doubting myself, but it's a good idea to also describe non-container resources. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The story you're referring to is a good example of a case where the spec process failed
That is precisely why we should not spec it until there is agreement. The NBs show that this is too early to spec. Let's have a resolution before we write anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll mark this one as "still ambiguous what the current situation is", let's discuss it in the weekly meeting now!
@RubenVerborgh I changed my opinion on this topic, so now I agree with you :) Can you re-review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me now, now awaiting @RubenVerborgh 's review.
@RubenVerborgh please re-review |
Received a comment by notification that I can't find, but still want to react:
NSS is buggy, very buggy, so it's behavior is not a good predictor for what we want. We should really wonder here what is the desired situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this reflects what we want, but would like @timbl to confirm.
@@ -459,12 +459,12 @@ QUERY or SEARCH verbs, if supported. | |||
gives access to a class of operations that can modify an existing resource. | |||
There are two levels: write permissions on an existing resource allows modifying | |||
(but not deleting) that resource. | |||
In a REST API context, this would include `POST` and `PATCH`. This also | |||
In a HTTP context, this would include `POST` and `PATCH`. This also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a HTTP context
-> In an HTTP context
includes the ability to perform SPARQL queries that perform updates, if those | ||
are supported. | ||
For creating or deleting a resource, write permissions on the container of which | ||
the resources is a member, are also needed. | ||
In a REST API context, this would include `PUT` and `DELETE`. | ||
In a HTTP context, this would include `PUT` and `DELETE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a HTTP context
-> In an HTTP context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absoltely disagree with "not on the resource
itself" -- to delete a file you MUST have Write on the file as well as Write on the container.
includes the ability to perform SPARQL queries that perform updates, if those | ||
are supported. | ||
For creating or deleting a resource, write permissions on the container of which | ||
the resources is a member, are also needed. | ||
In a REST API context, this would include `PUT` and `DELETE`. | ||
In a HTTP context, this would include `PUT` and `DELETE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absoltely disagree with "not on the resource
itself" -- to delete a file you MUST have Write on the file as well as Write on the container.
We introduced this change to be compatible with *nix, which requires only
folder write (and execute) permissions. I don’t see a benefit of going
half-*nix; rather just require document Write then.
|
ok, so there are three options here: for create/delete, we can require write permissions:
Shall we just close this PR and leave the spec as it is? That seems to be the only option of the three that nobody objects to. |
Let's wait; we need @timbl's feedback on what I wrote. |
What you wrote applies only to deleting a file, right, as creating a file there is no file permissions -- or did you mean the ACL which would be created for the new file from the defaults in the directory acl? |
I think we need to think about the guarantees that a program or a user can expect -- things like - I protected this so it can't have changed ... |
@timbl so is option 1 ok then? |
Unless anyone objects, I'm going to close this PR and leave the spec as it is. |
Objection; we need an issue to figure out what we want. |
If you can propose a change that @timbl agrees with then I'm happy with that too! It feels a bit like a deadlock now, where none of the proposed changes are agreeable, so then I guess the default option is not to change the spec at all. |
No, the default option when there is no agreement is to create an issue to discuss. |
Right, so there should be an issue (which should probably have been created prior to this PR), which @RubenVerborgh could create now as easily as @michielbdejong. |
I guess we include this discussion in #60, which is an attempt to make what the current spec says more precise. |
Follow-up in https://github.com/solid/solid-spec/issues/195. |
No description provided.