Delete .acl file for resource when resource is deleted #1180
Conversation
|
Fixes #1179 |
|
I'll rebase in & re-push once the other failing tests are corrected. |
|
@fabiancook There seems to be a problem with quotas causing the test to fail. Would you mind looking at that? |
|
The quota tests look like they've never worked, I don't have the bandwidth to debug those errors at this time. |
|
I think they did work. But I'm unsure why this should need to touch those? |
|
Yeah. I checked yesterday. The current master has 100% passing tests. |
|
Perfect, I'll rebase to current master |
fabiancook
force-pushed
the
delete-resource-acl
branch
from
f9a7114 to
ade5686
Compare
|
Can someone help me with the quota issue? A pointer would be helpful. The original quota tests never would have run, either, which is why I added a new one. |
fabiancook
force-pushed
the
delete-resource-acl
branch
2 times, most recently
from
0c0a0c5 to
994b827
Compare
…ny acl or meta files in directory when deleting
fabiancook
force-pushed
the
delete-resource-acl
branch
from
994b827 to
4e268e3
Compare
|
The only thing I'm wondering now is if I should change this line here so only 4e268e3#diff-4b9b8e50857ade6dd5db969387fe5f20R415 This would depend on this issue: solid/solid-spec#172 |
|
See solid/solid-spec#157 as well |
|
So I think we're good to go to merge this then. |
I'm not sure, do we have consensus that this is the desired behavior? Also, given that we're considering https://github.com/solid/solid-spec/issues/190, there are other means of discovering and deleting the ACL file. |
There was a problem hiding this comment.
Two major concerns:
a) the requesting agent is not checked for Control permissions on the resource
b) if, for whatever reason, delete for the ACL succeeds but the delete for the file does not, we have effectively erased the file's permissions. We very likely want to proceed in sequence and first delete the file, then its ACL.
|
Technically, it looks good to me, but I think we don't yet have consensus on whether this is what we want. |
|
It sounds like @RubenVerborgh 's two suggestions should be fixed first. |
|
Also need to consider security, in https://github.com/solid/solid-spec/issues/196 |
There was a problem hiding this comment.
In isolation it seems this patch is OK, as there is rough consensus that the permissions on the resource is what is relevant when deleting both the ACL and the resource. solid/specification#145
Apart from @RubenVerborgh 's concern 2) above, it would be nice to make the operation atomic.
Then, my other concern is the method name, because deleting just the ACL, when it happens not as a side-effect of the resource deletion, then it requires Control, and that should be clear from the code too, so that others do not by accident call deleteResourceAcl independently.
|
Continued in #1415. |
Delete .acl file for resource when resource is deleted, also ignore any acl or meta files in directory when deleting.
I had troubles with some TLS not passing, locally I skipped them. I also had issues with the quota tests, its like the signature of ldp changed at some point, so I redid the test so it tests correctly.