[sharing] fix addToGroup hook #17381
Conversation
|
cc @blizzz maybe you can also have a look as we discussed the issue earlier today. |
| '`file_source`' => $insert->expr()->literal($item['file_source']), | ||
| ] | ||
| ); | ||
| $insert->execute(); |
There was a problem hiding this comment.
Does this work correctly when you run execute multiple times? Since you keep the original $insert object.
There was a problem hiding this comment.
should work because we replace the values on the object for each execute()... but I will double-check.
|
Looks good. But I would really like unit tests here. |
For files we need to check anyway on every mount for potential name collisions and resolve it if it happens. Because on a external storage someone could create a file with the same name, the next time you login to you ownCloud the share needs to be moved to a unique name. Also I think it is better for performance reasons on a per user basis if needed instead of checking it for all users during the share operation. Most likely I would remove this logic completely from a share API 2.0 and let the back-end handle it, like it is already done by sharing. |
|
Hm, tests fail for the same reason it didn't worked for files. We are in a post-hook. This means that the user is already a member of the group. If we now ask for all shares we will already get the new group share, detect a collision and generate a new target "test1.txt". I don't see a way to fix this for the tests. Don't know why this works for address books... will look deeper into it. Update: it seems to work for contact because contact only compares the target with the local address books of the recipient but not with other shared address books. So it is basically a bug in contacts which makes it work. |
schiessle
force-pushed
the
fix_sharing_add_to_group
branch
from
c731b78
to
3c01c0a
Compare
|
@rullzer please have another look, I changed the approach based on #17381 (comment) We now have a pre-hook to calculate the unique target name and a post-hook to add it to the table |
|
Ignore that last one... I'll have another look :) |
schiessle
force-pushed
the
fix_sharing_add_to_group
branch
from
3c01c0a
to
82d319c
Compare
| $item['stime'], $item['file_source'], $fileTarget)); | ||
| \OC_DB::insertid('*PREFIX*share'); | ||
| if (($fileTarget === null && $itemTarget != $item['item_target']) || | ||
| ($fileTarget !== null && $fileTarget !== $item['file_target'])) { |
There was a problem hiding this comment.
coding style: please put || at the beginning of the new line
|
The code itself looks good, and also I really appreciate the tests. Is there anything specific to test manually (steps)? |
|
Following steps can be used to test the correct behavior:
Now check the oc_share table in the database. With this patch you should see a extra row for the group share for user3 with a target "test.txt (2)" or similar because we created a unique name for him.
Now check the oc_share table in the database. No additional row for the group share for user4 should have been created. Note: It is important that you check the db directly after adding user3 (user4) to the group. If user3 (user4) log-in we will detect the name collision and create a unique target for him. So after login everything will look the same with or without the patch. |
targets before the user was added to the group otherwise we will always detect a name collision
schiessle
force-pushed
the
fix_sharing_add_to_group
branch
from
82d319c
to
058d910
Compare
|
A new inspection was created. |
|
🚀 Test PASSed.🚀 |
|
I assume this has to go into 8.2? |
|
Code looks good Probably hard to add that tests to the testsuite? Otherwise 👍 |
|
needs a second reviewer... @nickvergessen ? Thanks! |
|
👍 |
[sharing] fix addToGroup hook
|
@schiesbn What about the backport to stable8.1? This was only merged against master. |
|
@schiesbn Ping |
|
@schiesbn You filed this ticket for 8.1.1 and this is against master, so you need to create a backport. |
|
Fix the add to group hook in sharing. This was discovered while working on #17327
We skip this step completely for file shares because for file shares we generate a unique target during the mount operation if needed. Further we make sure that the code gets executed at all. In the old version "$sourcrExists" was always true because we checked it in a post hook. So we always found a share for the user after he was added to the group.
cc @rullzer maybe you have some time to review it? Thanks!