Fix file permissions for SMB (read-only folders will be writeable) #25301

jvillafanez · 2016-06-29T16:24:21Z

Backport for OC9 should follow.

mention-bot · 2016-06-29T16:24:23Z

By analyzing the blame information on this pull request, we identified @icewind1991, @blizzz, @jmaciasportela and @Xenopathic to be potential reviewers

icewind1991 · 2016-06-29T20:01:21Z

Shouldn't this also include a check if the path is the share root?

jvillafanez · 2016-06-30T07:25:35Z

It doesn't seems to be needed

icewind1991 · 2016-06-30T12:09:53Z

ok, 👍

PVince81 · 2016-06-30T16:12:49Z

apps/files_external/lib/Lib/Storage/SMB.php

 		try {
 			$info = $this->getFileInfo($path);
-			return !$info->isHidden() && !$info->isReadOnly();
+			return !$info->isHidden() && (!$info->isReadOnly() || $this->is_dir($path));


does readonly still work if the share itself is readonly ?

Uploads will fail anywhere in the share if it's set as read-only. Anyway, I don't think we can't get the share configuration from the client.

Seems I'm able to stat the share directly, getting this from $this->share->getStat('') =>

$result[0] = (int) '2094732529'; $result[1] = (int) '-676439979'; $result[2] = (int) '16877'; $result[3] = (int) '2'; $result[4] = (int) '30'; $result[5] = (int) '8'; $result[6] = (int) '0'; $result[7] = (int) '0'; $result[8] = (int) '1467378490'; $result[9] = (int) '1466774556'; $result[10] = (int) '1466774556'; $result[11] = (int) '512'; $result[12] = (int) '0'; $result['dev'] = (int) '2094732529'; $result['ino'] = (int) '-676439979'; $result['mode'] = (int) '16877'; $result['nlink'] = (int) '2'; $result['uid'] = (int) '30'; $result['gid'] = (int) '8'; $result['rdev'] = (int) '0'; $result['size'] = (int) '0'; $result['atime'] = (int) '1467378490'; $result['mtime'] = (int) '1466774556'; $result['ctime'] = (int) '1466774556'; $result['blksize'] = (int) '512'; $result['blocks'] = (int) '0';

Not sure which is the permission.
But as I see on master we also don't handle share permissions, so might be something to do separately.

PVince81 · 2016-07-01T13:19:29Z

Tested, works 👍

However I tested only against a Linux Samba server. And to make permissions read-only I did chmod u-w rofolder. The folder still appeared read-write in ownCloud, as discussed.
But when uploading there, the Linux Samba didn't accept the file, which is expected due to the permissions.

I hope someone can test against a Windows Server.

davitol · 2016-07-04T08:48:38Z

👍 Using a WND backend located in a windows 2003 server

Now applying the patch, it is possible to add files in the R/O Subfolders

@jvillafanez will comment ASAP about other scenarios in this PR in order to clarify if it is needed more changes

jvillafanez · 2016-07-04T08:59:31Z

I'm checking if read-only folder can be deleted or not. As it is right now with this PR, all folders can be deleted from ownCloud, but there are errors deleting read-only folders.

We might need to adjust the delete permission.

jvillafanez · 2016-07-04T09:20:59Z

https://support.microsoft.com/en-us/kb/326549

Note Unlike the Read-only attribute for a file, the Read-only attribute for a folder is typically ignored by Windows, Windows components and accessories, and other programs. For example, you can delete, rename, and change a folder with the Read-only attribute by using Windows Explorer.

I wonder if this still applies if we're accessing through SMB...

jvillafanez · 2016-07-04T10:39:45Z

It seems that it's possible to delete the folder from the windows explorer, but not from the windows console nor from smbclient.

Note that with the current ownCloud implementation, even though the folder isn't being deleted, its contents are deleted (or at least tried to)

PVince81 · 2016-07-07T15:18:36Z

@jvillafanez how does Mac OS behave in regards to deleting the mount ?

PVince81 · 2016-07-12T08:50:20Z

@jvillafanez any update ? Any risks ?

jvillafanez · 2016-07-13T07:14:18Z

If the SMB server is in windows, the server rejects the deletion of the read-only folder.

Regarding the mounts, they shouldn't be deleted anyway.

I'll have to have a look at the delete permissions.

PVince81 · 2016-07-19T09:38:38Z

apps/files_external/lib/Lib/Storage/SMB.php

 	}

 	public function isUpdatable($path) {
+		try {


a quick comment here would be nice to avoid future devs to scratch their head trying to understand why we allow writing for read-only folders

PVince81 · 2016-07-19T09:39:45Z

@davitol mind doing another test against a Windows server in regards to the deletable part ?

PVince81 · 2016-07-19T09:40:36Z

SMB tests against a Windows Server still pass:

± % ./autotest-external.sh sqlite smb-windows                                                                                              !10112
Using database oc_autotest
Setup environment for sqlite testing ...
Installing ....
ownCloud is not installed - only a limited number of commands are available
{"reqId":"3P008aRwh4IqcM89HenL","remoteAddr":"","app":"PHP","message":"file_put_contents(\/srv\/www\/htdocs\/owncloud\/data\/.htaccess): failed to open stream: Permission denied at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Setup.php#500","level":3,"time":"2016-07-19T09:36:37+00:00","method":"--","url":"--","user":"--"}
{"reqId":"3P008aRwh4IqcM89HenL","remoteAddr":"","app":"PHP","message":"file_put_contents(\/srv\/www\/htdocs\/owncloud\/data\/index.html): failed to open stream: Permission denied at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Setup.php#501","level":3,"time":"2016-07-19T09:36:37+00:00","method":"--","url":"--","user":"--"}
creating sqlite db
ownCloud was successfully installed
{"reqId":"3P008aRwh4IqcM89HenL","remoteAddr":"","app":"PHP","message":"chmod(): Operation not permitted at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Log\/Owncloud.php#114","level":3,"time":"2016-07-19T09:36:38+00:00","method":"--","url":"--","user":"admin"}
Testing with sqlite ...
Run only smb-windows ...
files_external enabled
No coverage
PHPUnit 5.2.6 by Sebastian Bergmann and contributors.

Runtime:       PHP 5.6.23 with Xdebug 2.4.0
Configuration: /srv/www/htdocs/owncloud/tests/phpunit-autotest-external.xml

...............................................................  63 / 189 ( 33%)
............................................................... 126 / 189 ( 66%)
............................................................... 189 / 189 (100%)


Time: 1.68 seconds, Memory: 41.00Mb

OK (189 tests, 689 assertions)
skip: start-amazons3-ceph.sh
skip: start-ftp-morrisjobke.sh
skip: start-sftp-atmoz.sh
skip: start-smb-silvershell.sh
start: start-smb-windows.sh
name: smb-windows

No coverage
PHPUnit 5.2.6 by Sebastian Bergmann and contributors.

Runtime:       PHP 5.6.23 with Xdebug 2.4.0
Configuration: /srv/www/htdocs/owncloud/tests/phpunit-autotest-external.xml

................................................................. 65 / 77 ( 84%)
............                                                      77 / 77 (100%)

Time: 2.71 minutes, Memory: 31.75Mb

OK (77 tests, 346 assertions)
stop: stop-smb-windows.sh
skip: start-swift-ceph.sh
skip: start-webdav-apache.sh
skip: start-webdav-ownCloud.sh

Ideally we should add unit tests here, would require to mock the file info bit.

DeepDiver1975 · 2016-07-20T12:21:41Z

👍

jvillafanez · 2016-07-29T11:11:40Z

Will there be backports for 9.1 and 9.0?

PVince81 · 2016-08-08T18:16:01Z

@jvillafanez yes, would be good to backport this to 9.1 and 9.0 if possible.

CC @cdamken @felixboehm

cdamken · 2016-08-09T15:09:41Z

@jvillafanez yes, would be good to backport this to 9.1 and 9.0 if possible.

Yes, would be great to have them in those versions too! Thanks!

lock · 2019-08-05T02:02:03Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Fix file permissions for SMB (read-only folders will be writeable)

51f6948

jvillafanez added 3 - To Review app:files_external backport-request labels Jun 29, 2016

jvillafanez added this to the 9.1-current milestone Jun 29, 2016

PVince81 reviewed Jun 30, 2016
View reviewed changes

PVince81 modified the milestones: 9.2-next, 9.1-current Jun 30, 2016

Read-only folders won't be deletable

04ca3f6

PVince81 reviewed Jul 19, 2016
View reviewed changes

Added comment for the read-only behaviour for folders

5cf5edc

DeepDiver1975 merged commit 88585d3 into master Jul 20, 2016

DeepDiver1975 deleted the fix_smb_attributes branch July 20, 2016 12:22

LukasReschke mentioned this pull request Jul 20, 2016

Fix file permissions for SMB (read-only folders will be writeable) (#… nextcloud/server#465

Merged

icewind1991 mentioned this pull request Aug 3, 2016

Fix file permissions for SMB (read-only folders will be writeable) nextcloud/server#720

Closed

PVince81 added the blue-ticket label Aug 8, 2016

This was referenced Aug 9, 2016

[stable9.1] Fix smb attributes stable9 1 #25733

Merged

[stable9] Fix file permissions for SMB (read-only folders will be writeable) #25734

Merged

DeepDiver1975 removed the backport-request label Oct 11, 2016

lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019

Fix file permissions for SMB (read-only folders will be writeable) #25301

Fix file permissions for SMB (read-only folders will be writeable) #25301

Uh oh!

Conversation

jvillafanez commented Jun 29, 2016

Uh oh!

mention-bot commented Jun 29, 2016

Uh oh!

icewind1991 commented Jun 29, 2016

Uh oh!

jvillafanez commented Jun 30, 2016

Uh oh!

icewind1991 commented Jun 30, 2016

Uh oh!

PVince81 Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jul 1, 2016

Uh oh!

davitol commented Jul 4, 2016

Uh oh!

jvillafanez commented Jul 4, 2016

Uh oh!

jvillafanez commented Jul 4, 2016

Uh oh!

jvillafanez commented Jul 4, 2016

Uh oh!

PVince81 commented Jul 7, 2016

Uh oh!

PVince81 commented Jul 12, 2016

Uh oh!

jvillafanez commented Jul 13, 2016

Uh oh!

PVince81 Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jul 19, 2016

Uh oh!

PVince81 commented Jul 19, 2016

Uh oh!

DeepDiver1975 commented Jul 20, 2016

Uh oh!

jvillafanez commented Jul 29, 2016

Uh oh!

PVince81 commented Aug 8, 2016

Uh oh!

cdamken commented Aug 9, 2016

Uh oh!

lock bot commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants