deleteFile(url) not removing the file from the server #145
Comments
bmsantos
changed the title
|
The problem is, that async deleteFile (url) { return super._deleteItemWithLinks(url) }
async _deleteItemWithLinks (itemUrl) {
const links = await this.getItemLinks(itemUrl, { links: LINKS.INCLUDE })
if (links.meta) {
await this._deleteItemWithLinks(links.meta)
}
if (links.acl) {
await this.delete(links.acl)
}
// Note: Deleting item after deleting links to make it work for folders
// Change this if a new spec allows to delete them together (to avoid deleting the permissions before the folder)
return this.delete(itemUrl)
}This means, that it also tries to delete any existing acl and meta files associated to the file. As it would need CONTROL permissions to check if the acl exists (and apparently you don't have it) it fails with 403. The solution for you would be to use delete, which just sends a DELETE request without any additional checks (equivalent to the solidAuth.fetch variant you described): this.solidFileClient.delete(`${this.baseUrl}/private/photo1.png`); |
|
@jeff-zucker @bourgeoa |
|
So, you are telling me that the deleteFile() method is guaranteed to fail if the user does not have Control over what they are deleting even if no .acl file exists? Why on earth would we want it to act like that? |
Yes, this is the current behavior.
I agree that this is not what it should be. I'll try to explain why it is like this currently and what options I see to change it: Current behaviorIn SolidFileClient In my opinion the behavior of The behavior of Possible changesBut I am not sure what the behavior of
The problem with (2) is, that it can result in unexpected 403 errors while only trying to delete the file as mentioned by the OP. The problem with (1) and (3) is, that it could lead to orphaned acl and meta files. About (3) I don't like that the behavior would depend on the users permissions which make it harder for the developer to know what really happened (ie to predict the changes in the pod). My suggestions are (but feel free to disagree):
|
|
So itemExists(foo.acl) will error with 403 if the user doesn't have Control? |
|
I just tested and apparently it does. How absurd. |
That's the general behavior in NSS: If I wouldn't have the permissions to view* a file, I have no possibility to find out if it exists or not. For instance you can try to access I'm not sure if it makes sense, but that's up to NSS or the solid/ldp/http spec. *edit: it's likely about having any permissions or none. Not only read permissions. Haven't tested that though |
|
Yes, it's up to NSS and the spec writers. So, which other methods will also fail unless the user has Control and which accept a "withAcl=false"? -- copyFile? copyFolder? copy? and same for delete and move? |
|
What if we did this: find out from the wac-allow header on the resource what perms the user has. If they have Control deleteFile(foo) would delete both foo and foo.acl. If they have Write but not Control, deleteFile(foo) would delete(foo) and ignore foo.acl. Yes, there is a remote possibility of an orphan, but not one we introduced. In order for there to be an orphan, someone would have to have created a foo.acl that gave Write but not Contol over foo. Why would anyone create such an acl on a file? And if they did, they would be opening themselves to orphans with or without solid-file-client. Am I making sense? |
|
My 2 cents here. As a user I expect the behavior to be consistent with other calls. So, in this case, I created a file using postFile() without any issues and using the default permissions I’m granted when I login into the POD. The same behavior should be expected when I try to delete that same file originally created with postFile(). This means that I’m with Jeff and Otto-A option 3. For sure, for internal maintenance you may want to try to delete .acl and .meta files along with the file that the user really really wants to be gone from the server and should be so by the end of the call. Still, if.acl and .meta files are not removed and are left hanging around because the login does not provide such level of privileges then so be it... nothing the user can do anyway. Some other system strategy will have to be put in place to deal with those dead files from the POD side, but a user should never have to be concerned about it. Cheers, |
This is actually my least favourite option, because the method then depends on some user settings and therefore is hardly predictable by the app developer.
In SolidFileClient, the default option is to include links for api calls. For example Here's an example to demonstrate that /*
- src/bar.ttl
- src/bar.ttl.acl
- dest/
*/
await copy('./src/bar.ttl', './dest/bar.ttl') // also copies bar.ttl.acl
await deleteFile('./dest/bar.ttl') // would leave an orphaned acl file if it does not delete bar.ttl.aclRegarding
and
Pod owners per default have Read/Write/Control permissions on NSS. So in general they would be able to remove acl files. Trouble can occur, when (1) the app (->trustedApps) isn't granted Control permissions, or (2) it's shared with a different user, but also without Control permissions. For instance, with the Data Browser (which seems to be completely trusted by default) I could create a Therefore I don't think that the no-Control scenario is so unlikely. So now we are still left, with how we want to handle this. As always I am a fan of "The developer should know what it's doing and when it can fail", whereas you seem to focus more on "Simple defaults, but many capabilities". And with my previous suggestion to implement two methods ( |
@Otto-AA,I'm just a beginner here trying to understand SOLID and its complexities. As a recent user, all I did was try SOLID as a way to store and provide a private and secure way for my app users to reliably store their data in their own SOLID POD. When I first created my Inrupt SOLID POD I was not told that there were a "Control" permissions. I just opened the POD and relied that the default permission will work for everything of basic needs. As such, and as a user I expect all APIs to run smoothly under the default permissions. Until this discussion I did not even knew about Control permissions and have never seen such info in documentation so far. So, it is fair to say that most SOLID users will probably not even turn Control permissions on to begin with. At this stage, I think I have no need for acl and meta files and I will avoid turning the complicator on as much as I can. Also, you mentioned:
I've haven't used copyFile() yet, but you state that it will TRY to copy the acl and meta files, meaning that if no Control permissions are granted, it will fail to copy the acl and meta files but still copy the file of importance to the user and that matches the permissions granted by default to the user. In short, for these two sample commands I would expect the following:
You also mention:
This is not true as you can in the following picture. I've recently started my Inrupt POD and by default these are my permissions (never changed them): In addition, I'm such a newbie to SOLID that I don't even know how to add Trusted Apps at this stage. Right now all my app needs is to store data in a private user owned storage area and SOLID is perfect for it. I do not expect at this stage for this data to be shared with third party apps and never occurred to me that I would have to add extra permissions. From looking at the code: async copyFile (from, to, options) {
// ...
// Copy File
const getResponse = await this.get(from)
const content = await getResponse.blob()
const contentType = getResponse.headers.get('content-type')
const putResponse = await this.putFile(to, content, contentType, options)
// Optionally copy ACL and Meta Files
// TODO: What do we want to do when the source has no acl, but the target has one?
// Currently it keeps the old acl.
await this.copyLinksForItem(from, to, options, getResponse, putResponse)
return putResponse
}When compared, the copyFile() strategy is different than the deleteFile() strategy. async deleteFile (url) { return super._deleteItemWithLinks(url) }
async _deleteItemWithLinks (itemUrl) {
const links = await this.getItemLinks(itemUrl, { links: LINKS.INCLUDE })
if (links.meta) {
await this._deleteItemWithLinks(links.meta)
}
if (links.acl) {
await this.delete(links.acl)
}
// Note: Deleting item after deleting links to make it work for folders
// Change this if a new spec allows to delete them together (to avoid deleting the permissions before the folder)
return this.delete(itemUrl)
}The deleteFile() method does the opposite and starts by deleting the acl and meta files and only then deletes the user file. Since there's no guard against the failure of deleting or a way to bypass the deletion of acl and meta files it will never delete the user files after the guard. One should be able to guard using a simple try/catch around the acl and meta deletions. Sorry for the long reply but I myself I'm learning and digging to get a better understating and grasp on the issue and its possible side-effects. Cheers |
|
@bmsantos, the documentation for Solid needs improvement but users need to understand the access permissions, they are central to Solid. Apps that you expect to use to delete things should have Control permissions. So, in my opinion your trustedApps are misconfigured. (see https://github.com/solid/web-access-control-spec). @Otto-AA, your examples are not to the point. The app or container's permissions are not under question. Given my approach above (check wac-allow and don't try to delete foo.acl if the user has no Control), delete for write will not fail or cause problems if the app or container is only trusted for write. It will only fail if someone purposely creates a .acl for a file. While app or container level permissions are quite likely to be Write but not Control, I do not think that applies to file level permissions. Again - under what circumstance would someone create foo.acl for a file foo and give Write but not Control specifically over the file foo? That is bad programming and leaves them open to orphan .acls regardless of what apps do. |
|
@Otto-AA I would phrase the differences between our styles more as you care more about the logical consistency of the code and I care more about what users expect. |
|
@jeff-zucker, thanks for sharing the spec. |
|
@bmsantos there is a reason copy has to copy the resource first and the acl second - the copy location may have a different location it keeps .acl files in (the spec does NOT mandate foo.acl as the only location for an acl file on foo) so we need to read the resource to find out where its acl needs to be written. When it comes to recursively managing a tree of resources, I am with @Otto-AA - these should fail if the user does not have Control permission and tries to copy or move with withAcl=true. Otherwise, all of the file protections would be lost. It would be like taking encrypted data and un-encrypting it by default. So we make the user do this on purpose with the withAcl flag if they want different behavior. At the moment, I favor my wac-allow proposal first and my second choice is make delete exactly like the others - fail without Control unless withAcl=false. |
|
Also, whatever is decided about deleteFile(), I will write a description of how Control/Write permissions are handled to add to the README. |
|
@bmsantos wrote "needs a way to validate if user has Control permissions." That way is the wac-allow header. It can be read by reading the resource or getting its head and does not require viewing the actual .acl file so will not error if the user does not have permission to read the .acl. If you want to see this in action, try this let response = await fc.get(file)
console.log( response.headers.get('wac-allow') ) |
This is definitely a good idea. And as you said earlier, we should also look at what other methods could be affected by it (I guess all that have an |
|
On a related note : what would you think of building the wac-allow test into itemExists() - it could return true, false, or "you don't have Control rights needed to see if this item exists". |
I think I didn't express myself clearly with that example. I think the main misunderstanding was, that I meant missing Control permissions of the app, not the user. But I'll try to explain everything a bit more in detail: There are two apps:
In pseudo code, this is what my scenario looks like: /** App A **/
await createFile('bar.ttl', someContent)
await givePublicAccess('bar.ttl') // or share with friend, etc
/** results in
- bar.ttl
- bar.ttl.acl
*/
/** App B **/
await fc.deleteFile('bar.ttl')
// tries to delete bar.ttl.acl -> 403 (up to our implementation if we throw or ignore)
// deletes bar.ttl (up to our implementation)So the point is, that App A has Control permissions, while App B has no Control permissions. |
|
Regarding your wac-allow solution, my initial hesitation with it was, because I'm not clear about the status of it. I've created an issue about it one year ago, apparently it's in the api-rest spec, but it still doesn't seem like the discussion has been finished. Here's the related issue: solid/specification#170 Apart from that, I'm not sure if it would be beneficial for the implementation. One with wac-allow would look approximately like this: async _deleteItemWithLinks (itemUrl) {
const links = await this.getItemLinks(itemUrl, { links: LINKS.INCLUDE })
const allowedMethods = await this.getWacAllow()
if (links.meta) {
await this._deleteItemWithLinks(links.meta)
}
if (links.acl && allowedMethods.CONTROL) {
await this.delete(links.acl)
}
// Note: Deleting item after deleting links to make it work for folders
// Change this if a new spec allows to delete them together (to avoid deleting the permissions before the folder)
return this.delete(itemUrl)
}This would delete the file if one has CONTROL permissions, but just skip it if one doesn't. I think the similar behaviour would be achieved by catching 403: async _deleteItemWithLinks (itemUrl) {
const links = await this.getItemLinks(itemUrl, { links: LINKS.INCLUDE })
if (links.meta) {
await this._deleteItemWithLinks(links.meta)
}
if (links.acl) {
await this.delete(links.acl).catch(assertResponseStatus(403)))
}
// Note: Deleting item after deleting links to make it work for folders
// Change this if a new spec allows to delete them together (to avoid deleting the permissions before the folder)
return this.delete(itemUrl)
}So if we want that behaviour, I would also consider just catching 403 for the acl file. But I'm not sure about it |
I think of Solid as having two levels of specifying permissions:
With the default I meant, that per default the owner of a pod has Read, Write and Control permissions for all files and folders in it. Nonetheless, you could still use an app you don't fully trust, for instance you give it write access to modify files, but don't allow it to change permissions for a file. If that's the case, you won't be able to change permissions via that app, because you don't allow it too. In your case, this means that no app running on A special kind of app is the Data Browser which is shown when you open any folder in your pod directly. It has all permissions if I'm not mistaken. I don't think it's part of the spec, just an implementation of node-solid-server (NSS)
And a side note on that: Imo solid is still in development and not ready to securely store user data. And here's some general info on Solid, maybe it makes some things clearer to you: https://solidproject.org/ |
|
I wasn't aware that wac-allow was so tenuous, thanks for raising the issue and pointing me to it. I guess, at the moment, I am leaning towards something like this:
In most cases deleteFile() will do what's wanted, other cases are accomodated. |
|
I should also point out that eventually, as I understand it, the server will handle .acl files automatically and this sort of stuff will not need to be handled on the client end. |
yes, looking forward to an atomic solution :)) |
Sounds reasonable. I would prefer And just to be sure: |
|
@bmsantos Btw, did changing it to |
|
@Otto-AA, |
Okay.
Agreed.
Yes. How about copy() and move() - do they also ignore ACLs or is delete an outlier? |
Both, solid-file-client/src/SolidApi.js Lines 557 to 569 in a14dc87 So if Something which is probably noteworthy and discussable: If in a recursive method one request fails it doesn't mean that all requests fail. For instance: /** App has no Control permissions. Folder looks like this:
- foo/
- foo/bar.ttl
- foo/bar.ttl.acl
- foo/other.ttl
*/
await copy('./foo/', './dest/', { withAcl: true }) // -> will throw with an aggregated error, showing successes and failures
/** result (not tested, only judging from src code)
- foo/... (foo and contents stay the same)
- dest/
- dest/bar.ttl
- dest/other.ttl
*/The problem with this outcome is, that "dest/bar.ttl" was copied, but the permissions for it not. So maybe some people now have access that shouldn't be able to read/... the file. While I don't think that we will get a perfect solution without atomic creation of the file AND its permissions anyway¹, I think we should still try to prevent such situations. For /** result (not tested, only judging from src code)
- foo/... (foo and contents stay the same)
*/Regarding "or is delete an outlier?": ¹without atomic create, we always have to first create the file/folder and then it's permissions. This means that there is always a time frame where it has no specific permissions and considering an internet outage this could be hours or days long. And even if the time frame is short, listening for changes via websockets would sound possible to get a timely fetch. One could modify the default permissions of the parent for that duration, but that seems overkill, and likely breaks other things. So I honestly don't have any idea how we should do this without atomicity. |
|
(I hope that was understandable. If not I can rewrite it later...) |
|
Okay, here's a different idea. Let's say we have a new method "del" which operates like copy and move - accepts a withAcl flag, defaults to true, and operates on a file or folder depending on the trailing slash. I could then document like this: Access Control Modes of Solid-File-Client High Level Methods
|
Agreed.
If we do it as I outlined above (only a suggestion) then if withAcl=true, all would fail immediately without Control and there would be no partials.
I meant something like what I have "del" doing above - acting like copy and move. I understand that delete means DELETE so I guess it needs to remain as is. Regarding atomicity - I don't think we can or should attempt to completely solve that ahead of Solid solving it. |
|
My "fail immediately" thinking was faulty, ignore that. |
Yes, I'm fine using it as an option. I'm not sure if merging the different delete methods is a good idea, but I don't have strong opinions about that. I'm hesitating a bit, because it acts like a And I think we should rewrite the methods which use |
|
Yes, I see what you mean about rm -rf. Let me think about that. How would the "fail immediately" work? Suppose user has Control over the top level folder but lacks it for some sub folders or files? |
|
The "fail immediately" approach wouldn't be perfect, just better than the current situation. What it would do, is checking if one has access to the acl file, before copying something from src. But citing my previous statement about the possible implementation:
In that case, we'd copy the files and then fail. And in the case one has control over the top level folder, but not sub folders, it would only copy the top level folder and immediately fail for the sub folders. So it wouldn't leave anything with bad permissions in this case, only the part which works would be copied. |
|
I am thinking now that I will simply add a withAcl option to deleteFile and deleteFolder so that by default they will fail without Control but user can specify withAcl=false to force delete of just the resource (basically just a pass-through to delete()). As long as I am adding flags, I will add a method flag to createFile. It will default to method=put but user may set it to method=post. These changes in flags will allow me document like this:
@bourgeoa - could you comment on this comment, no need to read the whole issue if you haven't been. |
The following line is failing to remove the file from the server:
And I get the error:
When removing using the REST API all works just fine:
The .acl and the .meta files don't exist. Never created by:
The text was updated successfully, but these errors were encountered: