[stable9] Group shares with same source and target

[PVince81]

Backport of #25113 to stable9

⚠️ this required a lot of code changes due to incompatibility of the original PR and should be reviewed as if it was a brand new PR

• backport server side grouping fix: aa42b7b and e5af146 (I have a local squashed version to make it easier) => alternative fix
• backport JS side fix
• backport unit tests
• backport integration tests
• backport repair step + its unit test

@owncloud/sharing

[mention-bot]

@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @SergioBertolinSG, @rullzer and @icewind1991 to be potential reviewers

[PVince81]

Fixes #24575

[PVince81]

How strange. I see that OC 9.0 had a $share['grouped'] value, so there is already some old grouping logic there in the SharedStorage/SharedMount, but somehow it stopped working. Maybe there's a way to reuse it ?

[PVince81]

This 577651f is my tentative backport. It isn't finished yet as there are many unclear things. I just solved the conflicts and pushed. This is broken.

[PVince81]

Just debugged into the getItemsSharedWithUser and it DOES group the items initially.
It returns a single $share item that has a $share['grouped'] key. Now to find out why it doesn't work.

[PVince81]

Oh, it does work! I had to manually fix it in the DB then it continues working.

This means that there is another part of the code that will create the bogus extra share with target "/test (2)" before this mounting code was reached in the first place.

[PVince81]

Okay confirmed. As soon as a I share with "group2" after sharing with "group1", some code is automatically creating the bogus entries:

+----+------------+------------+-----------+---------------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| id | share_type | share_with | uid_owner | uid_initiator | parent | item_type | item_source | item_target | file_source | file_target | permissions | stime      | accepted | expiration | token | mail_send |
+----+------------+------------+-----------+---------------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
|  4 |          1 | group1     | admin     | admin         |   NULL | folder    | 19          | NULL        |          19 | /test       |          31 | 1469109293 |        0 | NULL       | NULL  |         0 |
|  5 |          1 | group2     | admin     | admin         |   NULL | folder    | 19          | NULL        |          19 | /test       |          31 | 1469109325 |        0 | NULL       | NULL  |         0 |
|  6 |          2 | user1      | admin     | NULL          |      5 | folder    | 19          | NULL        |          19 | /test (2)   |          31 | 1469109325 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+---------------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

So if I could prevent that to happen, then the original grouping code could be made to work again and we won't need to full backport of the MountProvider logic.

[PVince81]

Shared_Updater::postShareHook is called, it calls correctUsersFolder. The plot thickens.

[PVince81]

Okay, so that re-setup the storages for the receiver and instead of grouping the shares it returned both separately, for this specific call (we're still in the call that creates the share):

  $shares[14]                    = (array[21]);
    $shares[14]['id']            = (int) '14';
    $shares[14]['item_type']     = (string[6]) 'folder';
    $shares[14]['item_source']   = (string[2]) '19';
    $shares[14]['item_target']   = (null);
    $shares[14]['parent']        = (null);
    $shares[14]['share_type']    = (int) '1';
    $shares[14]['share_with']    = (string[6]) 'group1';
    $shares[14]['uid_owner']     = (string[5]) 'admin';
    $shares[14]['file_source']   = (int) '19';
    $shares[14]['path']          = (string[10]) 'files/test';
    $shares[14]['file_target']   = (string[5]) '/test';
    $shares[14]['permissions']   = (int) '31';
    $shares[14]['stime']         = (int) '1469110131';
    $shares[14]['expiration']    = (null);
    $shares[14]['token']         = (null);
    $shares[14]['storage']       = (int) '1';
    $shares[14]['mail_send']     = (string[1]) '0';
    $shares[14]['storage_id']    = (string[11]) 'home::admin';
    $shares[14]['file_parent']   = (int) '2';
    $shares[14]['share_with_displayname'] = (string[6]) 'group1';
    $shares[14]['displayname_owner'] = (string[5]) 'admin';
  $shares[15]                    = (array[21]);
    $shares[15]['id']            = (int) '15';
    $shares[15]['item_type']     = (string[6]) 'folder';
    $shares[15]['item_source']   = (string[2]) '19';
    $shares[15]['item_target']   = (null);
    $shares[15]['parent']        = (null);
    $shares[15]['share_type']    = (int) '1';
    $shares[15]['share_with']    = (string[6]) 'group2';
    $shares[15]['uid_owner']     = (string[5]) 'admin';
    $shares[15]['file_source']   = (int) '19';
    $shares[15]['path']          = (string[10]) 'files/test';
    $shares[15]['file_target']   = (string[5]) '/test';
    $shares[15]['permissions']   = (int) '31';
    $shares[15]['stime']         = (int) '1469110138';
    $shares[15]['expiration']    = (null);
    $shares[15]['token']         = (null);
    $shares[15]['storage']       = (int) '1';
    $shares[15]['mail_send']     = (string[1]) '0';
    $shares[15]['storage_id']    = (string[11]) 'home::admin';
    $shares[15]['file_parent']   = (int) '2';
    $shares[15]['share_with_displayname'] = (string[6]) 'group2';
    $shares[15]['displayname_owner'] = (string[5]) 'admin';

[PVince81]

Noooooooooooo...

The grouping logic is only called if the logged in user matches: https://github.com/owncloud/core/blob/v9.0.4/lib/private/share/share.php#L1932

But in this specific call, we're logged in as "admin" and creating a share that affects the user "user1". Since the logged in user is not "user1", the grouping doesn't happen. WTF!

Now I think one difference between this and OC 8.2 is that we probably did not call correctUserFolders or somehow didn't setup the FS of the target user. So do we really need to do it here now ?

[PVince81]

And here we go. In the light of #25543 (comment), here is a fix that adds a flag to force-group the received shares in the MountProvider: 2ac24cb

@rullzer what do you think ?

[PVince81]

• BUG: rename received group share then add extra user share doesn't group properly

1. Admin shares "test" with "group1" and "group2" (user1 is member of both)
2. "user1" renames "test" to "test_renamed"
3. admin now shares "test" directly with "user1"

Expected: received share is still called "test_renamed" (8.2 behavior)

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target   | permissions | stime      | accepted | expiration | token | mail_send | uid_initiator |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+
|  6 |          1 | group1     | admin     |   NULL | folder    | 9           | NULL        |           9 | /test         |          31 | 1469167795 |        0 | NULL       | NULL  |         0 | admin         |
|  7 |          1 | group2     | admin     |   NULL | folder    | 9           | NULL        |           9 | /test         |          31 | 1469167798 |        0 | NULL       | NULL  |         0 | admin         |
|  8 |          2 | user1      | admin     |      6 | folder    | 9           | NULL        |           9 | /test_renamed |          31 | 1469167795 |        0 | NULL       | NULL  |         0 | NULL          |
|  9 |          2 | user1      | admin     |      7 | folder    | 9           | NULL        |           9 | /test_renamed |          31 | 1469167798 |        0 | NULL       | NULL  |         0 | NULL          |
| 10 |          0 | user1      | admin     |   NULL | folder    | 9           | NULL        |           9 | /test_renamed |          31 | 1469167819 |        0 | NULL       | NULL  |         0 | admin         |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+

Actual: two folders, "test" and "test_renamed"

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target   | permissions | stime      | accepted | expiration | token | mail_send | uid_initiator |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+
|  6 |          1 | group1     | admin     |   NULL | folder    | 9           | NULL        |           9 | /test         |          31 | 1469167795 |        0 | NULL       | NULL  |         0 | admin         |
|  7 |          1 | group2     | admin     |   NULL | folder    | 9           | NULL        |           9 | /test         |          31 | 1469167798 |        0 | NULL       | NULL  |         0 | admin         |
|  8 |          2 | user1      | admin     |      6 | folder    | 9           | NULL        |           9 | /test_renamed |          31 | 1469167795 |        0 | NULL       | NULL  |         0 | NULL          |
|  9 |          2 | user1      | admin     |      7 | folder    | 9           | NULL        |           9 | /test_renamed |          31 | 1469167798 |        0 | NULL       | NULL  |         0 | NULL          |
| 10 |          0 | user1      | admin     |   NULL | folder    | 9           | NULL        |           9 | /test         |          31 | 1469167819 |        0 | NULL       | NULL  |         0 | admin         |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+---------------+-------------+------------+----------+------------+-------+-----------+---------------+

[PVince81]

• Backported Fix RepairUnmergedShares to not skip valid repair cases #25567 here for the additional repair step fix

[PVince81]

Raised #25568 for the case from #25543 (comment)

[PVince81]

Backported the tests from #25568 and part of its logic to fix #25543 (comment)

Now the tests will tell you that it works, and a manual test also shows so.

However, if you look at d5f2d2c I didn't bother to sort by stime like I did in the original commit. For some weirdly twisted reason, it seems the logic that comes before that already properly generates the correct order for the files_target.

• TODO: check again whether sorting by stime would still be better => no
  => I see that the SQL query is already sorting by share id which should enforce the correct order already

[PVince81]

Okay, I decided to improve the repair logic to pick the best target name based on all subshares and exclude the ones with numbers like "(2)". In case the user renamed all of the duplicates, the most recent will be used.

7e98a39

• forward port 7e98a39 and deb1f08 to stable9.1 and master

This is ready for review now.

[PVince81]

annnd another quick fix for yet another case with direct share: deb1f08

[PVince81]

@owncloud/qa can you help testing this ? Would be good to have it in 9.0.5

[PVince81]

@rullzer are you still interested in reviewing this ?

[rullzer]

👍 from my side.

The code makes sense to me.
Tests look pretty complete which gives even more confidence :)

[PVince81]

@rullzer thanks a bunch !

Now waiting for @owncloud/qa to do a final test check

[PVince81]

Forward ports of the two last commits about file_target decision when repairing:

• master/9.2: Fix unmerged shares repair targetdecision #25810
• stable9.1: [stable9.1] Fix unmerged shares repair targetdecision #25812

[SergioBertolinSG]

Works fine 👍

[lock]

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.