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

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

Open
mmattel opened this issue Dec 31, 2017 · 13 comments
Open

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

mmattel opened this issue Dec 31, 2017 · 13 comments

Comments

@mmattel
Copy link
Contributor

mmattel commented Dec 31, 2017

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

PVince81 commented Jan 8, 2018

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

mmattel commented Jan 9, 2018

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

PVince81 commented Jan 9, 2018

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

mmattel commented Jan 9, 2018

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

PVince81 commented Jan 9, 2018

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

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

PVince81 commented Jan 9, 2018

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

mmattel commented Jan 9, 2018

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

That is correct

@mmattel
Copy link
Contributor Author

mmattel commented Jan 9, 2018

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

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

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

mmattel commented Feb 17, 2018

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:

  • on write attempt to target with a target file given or directory to be created
  • create a unique dummy filename and remember it
  • write the dummy file 0Bytes to the target
  • check if writing is possible or not
    if yes: delete the dummy and write to target
    if no: tell the caller you have read only permissions
  • remember the state for x-minutes
    permissions change seldom, no need to do that on every write attempt

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

mmattel commented Jan 10, 2019

Not a complete fix but an improvement if merged: #33035 (29873 read only mount)

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

No branches or pull requests

5 participants