owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.38k stars 2.05k forks source link

Browser upload to a mount where the account is read only fails late #29999

Open mmattel opened 6 years ago

mmattel commented 6 years ago

Steps to reproduce

Create a mount, eg SMB, and use an account which has read only access. When you upload via browser a file, it uploads it to the temp-space and post process uploading from temp to the mount. But because the mount has by the account read only access it fails - which is totally ok.

Expected behaviour

Browser upload should be denied before uploading. Please see also https://github.com/owncloud/core/issues/29873 ([Feature Request] Mark mount as read only in the mount settings)

Actual behaviour

It uploads to temp first and fails writing to the mount afterwards - a error message is presented. This can be a nasty thing. On slower networks and big files, eg +GB you get the error AFTER uploading and after waiting a long time.

Server configuration

Operating system: Ubuntu 16.04

Web server: nginx

Database: mysql

PHP version: 7.0

ownCloud version: 10.0.4

Updated from an older ownCloud or fresh install: updated

Where did you install ownCloud from: tar

Signing status (ownCloud 9.0 and above):

no issues

@PVince81

PVince81 commented 6 years ago

@mmattel the bug here is likely that the SMB storage implementation doesn't return the correct read-only permissions. The web UI is already able to deny uploads if a read-only permission was detected.

@jvillafanez do you know more about this issue ?

mmattel commented 6 years ago

Does any storage implementation return a read only permission ? SMB, FTP, DB, ect I doubt. Thats where #29873 would help on a generic base...

PVince81 commented 6 years ago

@mmattel they should. If they don't, they're broken 😱

I did hear that read-only perm check was missing in SFTP storage.

Storages have the methods isCreatable, isUpdatable, isDeletable and getPermissions. Now it is also possible that some storages have no ways to know the permissions but I doubt it...

mmattel commented 6 years ago

I know that these methods are present, but are these methods used when setting up a mount ? Is this info (esp. isCreatable) handed over ? (it is if you create a ro-share via oC) Is this re-checked (what happens if the account used gets different grants at the target side?)

It seems that this needs some testing over all possible mount types to gather info about the current state. I can´t do that because lack of time...

PVince81 commented 6 years ago

Yes. Without this info read-only shared mounts would be broken as well as they use the same mechanism. In any case, this needs debugging

jvillafanez commented 6 years ago

The permissions set by the SMB connector are based on the attributes of the files, not really the user permissions, so this is kind of expected.

The last time I checked if it was possible to retrieve user permissions on a file I think it wasn't possible or there were several issues. Note that we have to fetch those permissions with the libsmbclient-php library.

PVince81 commented 6 years ago

@jvillafanez is it possible maybe to fetch the permissions of the share itself ? So if the SMB share is read-only just use that and don't bother with file permissions. ACLs is another story... I assume @mmattel was talking mostly about a read-only SMB share.

mmattel commented 6 years ago

I assume @mmattel was talking mostly about a read-only SMB share.

That is correct

mmattel commented 6 years ago

Just tested on an ftp mount where you have read only access: Commandline access via Ubuntu server is fine and works as expected.

FTP Server: Filezilla Server on W10x64 image

It first uploads via browser to owncloud and then fails image

IMHO this means, that there is no check at mount creation/use if you have permissions to write or update in that mount.

As more this evolves I see more the need for #29873 ([Feature Request] Mark mount as read only in the mount settings) to cover that easily. But I maybe wrong...

jvillafanez commented 6 years ago

From the tools provided by https://github.com/eduardok/libsmbclient-php we can't get any information related to the share itself. We might use smbclient_statvfs but it returns FS information, not share information, so the read-only value we might be able to extract will refer to the FS where the share resides (which will usually be read + write), but not for the share itself.

For ACLs, I remember the problem we have is with the inherited permissions and permissions over groups: while per-user permissions might work, if there are permissions over a group we can't really know if the account we're using is affected by those permissions over the group, or if the account is affected by permissions over the parent folders.

ownclouders commented 6 years ago

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

mmattel commented 6 years ago

Seems that share level permissons for a given user is really hard to get. I have seen that there are possibilities when using eg Powershell in a pure Windows environment, so methods may exist but currently as far I see it not in libsmbclient.

Another method may be and would work on a generic level for all storage types:

It is about to simulate a check if writing is possible or not based on the success of touch and remember the result for some time.

mmattel commented 5 years ago

Not a complete fix but an improvement if merged: https://github.com/owncloud/core/pull/33035 (29873 read only mount)