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

Unshare feature is incomplete for groups when a user tries to unshare/delete a file #26346

Closed
GustavoSLinden opened this Issue Oct 11, 2016 · 19 comments

Comments

Projects
None yet
4 participants
@GustavoSLinden

GustavoSLinden commented Oct 11, 2016

Steps to reproduce

  1. Create a group and assign 2 members (user1 and user2).
  2. Share a file with user1 to the group.
  3. Unshare the file with user2.

Expected behaviour

Everything should work perfectly.

Actual behaviour

The file is correctly unshared but after these 3 errors some functions did not work correctly:

{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: shareType at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#172","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}
{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: shareWith at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#177","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}
{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: fileTarget at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#177","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}

Server configuration

Operating system: Debian 7 x86_64

Web server: Apache 2.2

Database: Postgresql 9.4

PHP version: 5.4.45-0+deb7u2

ownCloud version: 9.0.1

Updated from an older ownCloud or fresh install: Updated

Logs

ownCloud log (data/owncloud.log)

{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: shareType at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#172","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}
{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: shareWith at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#177","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}
{"reqId":"iaLbkobz6NFFnbKTN5ZT","remoteAddr":"10.35.82.142","app":"PHP","message":"Undefined index: fileTarget at \/var\/www\/nextcloud\/apps\/files_sharing\/lib\/updater.php#177","level":3,"time":"2016-10-11T19:25:29+00:00","method":"DELETE","url":"\/remote.php\/webdav\/test.txt","user":"user2"}

Workaround

This patch could be applied to correct the errors:

--- /var/www/owncloud/apps/files_sharing/lib/updater.php    2016-10-11 17:58:35.121227338 -0300
+++ /var/www/owncloud/apps/files_sharing/lib/updater.php    2016-10-11 18:00:05.449227352 -0300
@@ -168,10 +168,15 @@
    static public function postUnshareFromSelfHook($params) {
        if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') {
            foreach ($params['unsharedItems'] as $item) {
-               if ($item['shareType'] === \OCP\Share::SHARE_TYPE_GROUP) {
+               if ( isset($item['shareType']) && $item['shareType'] === \OCP\Share::SHARE_TYPE_GROUP) {
                    foreach (\OC_Group::usersInGroup($item['shareWith']) as $user) {
                        self::correctUsersFolder($user, $item['fileTarget']);
                    }
+               }
+               elseif ( isset($item['share_type']) && $item['share_type'] === \OCP\Share::SHARE_TYPE_GROUP) {
+                   foreach (\OC_Group::usersInGroup($item['share_with']) as $user) {
+                                                self::correctUsersFolder($user, $item['file_target']);
+                                        }
                } else {
                    self::correctUsersFolder($item['shareWith'], $item['fileTarget']);
                }
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 12, 2016

Member

"var/www/nextcloud" ?

The code you pointed at is under "/var/www/owncloud". Are you getting the same error messages with owncloud ?

Please update to OC 9.0.5 as 9.0.1 is old.
Let us know if the problem persists there.

Member

PVince81 commented Oct 12, 2016

"var/www/nextcloud" ?

The code you pointed at is under "/var/www/owncloud". Are you getting the same error messages with owncloud ?

Please update to OC 9.0.5 as 9.0.1 is old.
Let us know if the problem persists there.

@PVince81 PVince81 added the needs info label Oct 12, 2016

@rogerfv1

This comment has been minimized.

Show comment
Hide comment
@rogerfv1

rogerfv1 Oct 12, 2016

I have tested and the issue occurs both in OC 9.0.5 and in Nextcloud.

rogerfv1 commented Oct 12, 2016

I have tested and the issue occurs both in OC 9.0.5 and in Nextcloud.

@PVince81 PVince81 added this to the 9.1.2 milestone Oct 13, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Okay thanks, for confirming.

Member

PVince81 commented Oct 13, 2016

Okay thanks, for confirming.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Confirmed happening on 9.0.5:

{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: shareType at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#171","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: shareWith at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#176","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: fileTarget at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#176","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
  • stable9 also affected (upcoming 9.0.6)
  • v9.1.1 doesn't have the warnings, so it's likely fixed there.
Member

PVince81 commented Oct 13, 2016

Confirmed happening on 9.0.5:

{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: shareType at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#171","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: shareWith at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#176","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
{"reqId":"\/ovqILw9+yQVy1EU2iBq","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined index: fileTarget at \/srv\/www\/htdocs\/owncloud\/apps\/files_sharing\/lib\/updater.php#176","level":3,"time":"2016-10-13T07:13:17+00:00","method":"DELETE","url":"\/owncloud\/remote.php\/webdav\/bacon.txt","user":"user2"}
  • stable9 also affected (upcoming 9.0.6)
  • v9.1.1 doesn't have the warnings, so it's likely fixed there.

@PVince81 PVince81 modified the milestones: 9.0.6, 9.1.2 Oct 13, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Hmmmm, there is something even more suspicious here. @GustavoSLinden's patch will not work.
The problem is not that the attributes do not exist, but they have different names:

$item                            = (array[17]);
  $item['id']                    = (string[1]) '1';
  $item['share_type']            = (string[1]) '1';
  $item['share_with']            = (string[6]) 'group1';
  $item['uid_owner']             = (string[5]) 'user1';
  $item['uid_initiator']         = (string[5]) 'user1';
  $item['parent']                = (null);
  $item['item_type']             = (string[4]) 'file';
  $item['item_source']           = (string[2]) '20';
  $item['item_target']           = (null);
  $item['file_source']           = (string[2]) '20';
  $item['file_target']           = (string[10]) '/bacon.txt';
  $item['permissions']           = (string[2]) '19';
  $item['stime']                 = (string[10]) '1476343633';
  $item['accepted']              = (string[1]) '0';
  $item['expiration']            = (null);
  $item['token']                 = (null);
  $item['mail_send']             = (string[1]) '0';

they seem to have the old underscore-style name.
I also see that the other method postUnshareHook also use the wrong attribute styles.

Needs further research to find out when and why the attribute names were renamed in the first place.

Member

PVince81 commented Oct 13, 2016

Hmmmm, there is something even more suspicious here. @GustavoSLinden's patch will not work.
The problem is not that the attributes do not exist, but they have different names:

$item                            = (array[17]);
  $item['id']                    = (string[1]) '1';
  $item['share_type']            = (string[1]) '1';
  $item['share_with']            = (string[6]) 'group1';
  $item['uid_owner']             = (string[5]) 'user1';
  $item['uid_initiator']         = (string[5]) 'user1';
  $item['parent']                = (null);
  $item['item_type']             = (string[4]) 'file';
  $item['item_source']           = (string[2]) '20';
  $item['item_target']           = (null);
  $item['file_source']           = (string[2]) '20';
  $item['file_target']           = (string[10]) '/bacon.txt';
  $item['permissions']           = (string[2]) '19';
  $item['stime']                 = (string[10]) '1476343633';
  $item['accepted']              = (string[1]) '0';
  $item['expiration']            = (null);
  $item['token']                 = (null);
  $item['mail_send']             = (string[1]) '0';

they seem to have the old underscore-style name.
I also see that the other method postUnshareHook also use the wrong attribute styles.

Needs further research to find out when and why the attribute names were renamed in the first place.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Oh, the code already exists in this form in 8.1 apparently and the error appears there as well.

Member

PVince81 commented Oct 13, 2016

Oh, the code already exists in this form in 8.1 apparently and the error appears there as well.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Hmmmm seems the hook code converts the share and changes the key names in v8.1.10: https://github.com/owncloud/core/blob/v8.1.10/lib/private/share/helper.php#L114

I put a breakpoint there and it seems the code path doesn't even use that method... so this explains why the format doesn't match.

Member

PVince81 commented Oct 13, 2016

Hmmmm seems the hook code converts the share and changes the key names in v8.1.10: https://github.com/owncloud/core/blob/v8.1.10/lib/private/share/helper.php#L114

I put a breakpoint there and it seems the code path doesn't even use that method... so this explains why the format doesn't match.

@PVince81 PVince81 added the sev2-high label Oct 13, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

This code was originally introduced through a0b85fc but I believe that some refactoring/cleanup made parts of it obsolete and used a shortcut. I'll bisect this to find out when it happened to confirm that it is ok and expected to also change the hook methods back to the old attribute...

Member

PVince81 commented Oct 13, 2016

This code was originally introduced through a0b85fc but I believe that some refactoring/cleanup made parts of it obsolete and used a shortcut. I'll bisect this to find out when it happened to confirm that it is ok and expected to also change the hook methods back to the old attribute...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Note: from what I remember, etag propagation/changing was implemented using a different approach, so while this code piece wouldn't run the desired effect would still exist. If that's the case, then this code could be removed.

Plan:

  • find out what commit/PR introduced the issue in 8.1 OC 8.0 not worth it
  • check whether the etag propagation still works without a fix. if yes: remove this code. if not: fix the code
Member

PVince81 commented Oct 13, 2016

Note: from what I remember, etag propagation/changing was implemented using a different approach, so while this code piece wouldn't run the desired effect would still exist. If that's the case, then this code could be removed.

Plan:

  • find out what commit/PR introduced the issue in 8.1 OC 8.0 not worth it
  • check whether the etag propagation still works without a fix. if yes: remove this code. if not: fix the code
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Tests:

  • stable8: 🚫 warning appears, 🚫 parent etag not updated
  • stable8.1: 🚫 warning appears, 🚫 parent etag not updated
  • stable8.2: 🚫 warning appears, 🚫 parent etag not updated
  • stable9: 🚫 warning appears: parent etag IS updated due to new computation logic
  • stable9.1: no warning, parent etag updated properly
Member

PVince81 commented Oct 13, 2016

Tests:

  • stable8: 🚫 warning appears, 🚫 parent etag not updated
  • stable8.1: 🚫 warning appears, 🚫 parent etag not updated
  • stable8.2: 🚫 warning appears, 🚫 parent etag not updated
  • stable9: 🚫 warning appears: parent etag IS updated due to new computation logic
  • stable9.1: no warning, parent etag updated properly
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Fix for OC 9: #26362

Member

PVince81 commented Oct 13, 2016

Fix for OC 9: #26362

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

More about OC <= 8.2:

  • sharing/unsharing etag change works fine, because postShareHook and postUnshareHook both receive the shares in the expected format, using camel case attributes like "shareType"
  • unshare from self: postUnshareFromSelfHook receives the share in non-camelcase "share_type" but expects camel case "shareType". For consistency in hook processing, it should also be camel case.
Member

PVince81 commented Oct 13, 2016

More about OC <= 8.2:

  • sharing/unsharing etag change works fine, because postShareHook and postUnshareHook both receive the shares in the expected format, using camel case attributes like "shareType"
  • unshare from self: postUnshareFromSelfHook receives the share in non-camelcase "share_type" but expects camel case "shareType". For consistency in hook processing, it should also be camel case.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Additional details:

  • unshare from self for a user share works correctly
  • unshare from self from a group share is affected

From what I see the location where the array is built to be passed to the hook, it puts the wrong unconverted item into the array. Fix incoming.

Member

PVince81 commented Oct 13, 2016

Additional details:

  • unshare from self for a user share works correctly
  • unshare from self from a group share is affected

From what I see the location where the array is built to be passed to the hook, it puts the wrong unconverted item into the array. Fix incoming.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

OC 8.2 fix: #26364

Member

PVince81 commented Oct 13, 2016

OC 8.2 fix: #26364

@GustavoSLinden

This comment has been minimized.

Show comment
Hide comment
@GustavoSLinden

GustavoSLinden Oct 13, 2016

Thanks Vincent, your fix works perfectly!!

Sorry for the string Nextcloud, I was testing several versions of Owncloud and Nextcloud.

GustavoSLinden commented Oct 13, 2016

Thanks Vincent, your fix works perfectly!!

Sorry for the string Nextcloud, I was testing several versions of Owncloud and Nextcloud.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Reopening the ticket. Need to keep it open until the fixes are merged. You probably misclicked 😄

Member

PVince81 commented Oct 13, 2016

Reopening the ticket. Need to keep it open until the fixes are merged. You probably misclicked 😄

@PVince81 PVince81 reopened this Oct 13, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

btw @GustavoSLinden thanks for testing the fix

Member

PVince81 commented Oct 13, 2016

btw @GustavoSLinden thanks for testing the fix

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 17, 2016

Member

The actual fix now will be: #26364 which has been forward ported up to master: #26364 (comment)

Even though stable9.1 and master would not have the error as the etag propagation code changed, it is still important to fix the hook format in case third party apps would be using them.

Member

PVince81 commented Oct 17, 2016

The actual fix now will be: #26364 which has been forward ported up to master: #26364 (comment)

Even though stable9.1 and master would not have the error as the etag propagation code changed, it is still important to fix the hook format in case third party apps would be using them.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 20, 2016

Member

All PRs merged, fix will be in 8.2.9 and 9.0.6

Member

PVince81 commented Oct 20, 2016

All PRs merged, fix will be in 8.2.9 and 9.0.6

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