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

notes about container write permissions #48

Closed
wants to merge 5 commits into from

Conversation

michielbdejong
Copy link
Contributor

No description provided.

@michielbdejong
Copy link
Contributor Author

r? @dmitrizagidulin

@michielbdejong
Copy link
Contributor Author

@RubenVerborgh or @kjetilk, maybe one of you can review this?

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.

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.
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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!

@michielbdejong
Copy link
Contributor Author

@RubenVerborgh I changed my opinion on this topic, so now I agree with you :) Can you re-review?

kjetilk
kjetilk previously approved these changes Jun 17, 2019
Copy link
Member

@kjetilk kjetilk left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@michielbdejong
Copy link
Contributor Author

@RubenVerborgh please re-review

@RubenVerborgh
Copy link
Contributor

Received a comment by notification that I can't find, but still want to react:

ok, will double-check what you implemented in NSS, and describe that :)

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.

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.

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
Copy link
Contributor

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`.
Copy link
Contributor

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

Copy link

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`.
Copy link

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.

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 18, 2019 via email

@michielbdejong
Copy link
Contributor Author

ok, so there are three options here: for create/delete, we can require write permissions:

  1. only on the resource (current spec text)
  2. only on the container (current NSS behaviour, @timbl is strongly against this)
  3. on both (@RubenVerborgh is against this)

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.

@RubenVerborgh
Copy link
Contributor

Let's wait; we need @timbl's feedback on what I wrote.

@timbl
Copy link

timbl commented Jun 19, 2019

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?

@timbl
Copy link

timbl commented Jun 19, 2019

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

@michielbdejong
Copy link
Contributor Author

@timbl so is option 1 ok then?

@michielbdejong
Copy link
Contributor Author

Unless anyone objects, I'm going to close this PR and leave the spec as it is.

@RubenVerborgh
Copy link
Contributor

Objection; we need an issue to figure out what we want.

@michielbdejong
Copy link
Contributor Author

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.

@RubenVerborgh
Copy link
Contributor

No, the default option when there is no agreement is to create an issue to discuss.

@TallTed
Copy link
Contributor

TallTed commented Jun 21, 2019

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.

@michielbdejong
Copy link
Contributor Author

I guess we include this discussion in #60, which is an attempt to make what the current spec says more precise.

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 23, 2019

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

Successfully merging this pull request may close these issues.

5 participants