Fix copying or moving from shared groupfolders #47847
Conversation
When copying or moving between two local storages the source path (on disk) to copy or move from is got from the unjailed path of the source storage. However, if the source storage has more than one jail getting the unjailed path resolves the most external jail, but the source path needs to be got from the most internal jail instead (the one closer to the local storage). This can happen, for example, with a shared groupfolder: in that case there is an external jail for the shared storage, and one internal jail for the groupfolder storage wrapped by the shared storage. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
|
/backport to stable30 |
|
/backport to stable29 |
|
/backport to stable28 |
|
This seems like it must have some open Issues associated with it, but I didn't find any with a quick look. Are you aware of any open bug reports this likely fixes? |
| // resolve any jailed paths | ||
| while ($sourceStorage->instanceOfStorage(Jail::class)) { | ||
| /** | ||
| * @var \OC\Files\Storage\Wrapper\Jail $sourceStorage | ||
| */ | ||
| $sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath); | ||
| $sourceStorage = $sourceStorage->getUnjailedStorage(); |
There was a problem hiding this comment.
We probably have that same issue in other places in Nextcloud. How about changing getUnjailedStorage to include that while loop?
There was a problem hiding this comment.
Note that the important part is getting the unjailed path; getting the unjailed storage is just a shortcut to avoid looping through all the wrapped storages (which in fact is how I initially implemented the fix, until I noticed the other code in Common::moveFromStorage) and just "jump" directly to the storage wrapped by each Jail. But getting the unjailed storage is not relevant for the fix; it would work with the original source storage as long as the most internal unjailed path is given (unless I am missing something, of course, but at least in the case of shared groupfolders it would work :-) ).
The problem is that if getUnjailedStorage was changed to include the loop and therefore return the storage wrapped by the most internal Jail then it would not be possible to get its unjailed path, as the reference to that most internal Jail would not be available.
There was a problem hiding this comment.
Indeed, then maybe a getUnjailedPath method?
@icewind1991 do you have an opinion?
There was a problem hiding this comment.
I would prefer a new method than overloading getUnjailedPath but it would be good to have a single place to do this logic yes.
No, sorry 🤷 |
Signed-off-by: Robin Appelman <[email protected]>
|
Added a test |
When copying a shared groupfolder into another local folder the whole data directory ends being (recursively) copied to the other folder. There are other scenarios that fail, but that one is probably the most severe one.
The problem is that shared groupfolders have two jails, one for the shared storage and another one for the groupfolder storage wrapped by the shared storage, and when a file or folder is copied between two local storages the source path (on disk) to copy or move from is got from the unjailed path of the source storage. However, if the source storage has more than one jail getting the unjailed path resolves the most external jail, but the source path needs to be got from the most internal jail instead (the one closer to the local storage).
In the case of a shared groupfolder the external unjailed path of the shared groupfolder itself is
"", and when getting the source path of""in the local storage the full path ends being the data directory, so the whole data directory ends being copied to the target.The fix is based on the code from
Common::moveFromStorage, which already uses the most internal unjailed path.E2E tests (as integration tests are not setup yet in the groupfolders app) were added in nextcloud/groupfolders#3185
Note that the issue can not be reproduced when trying to move the shared groupfolder into another folder (not shared and in the same parent mount) because, in that case, the shared mount is moved. On the other hand, it can be reproduced when trying to move a file from the shared groupfolder into another folder, as in that case
moveFromStorageis used. Thus the slightly different scenarios below, one copying the whole shared groupfolder, and one moving a file from the shared groupfolder.Beware when testing the scenario 1 below, as it will mess with the Nextcloud data directory and can potentially take a lot of disk space.
How to test (scenario 1)
Result with this pull request
The copy succeeds; opening the target directory shows the copied groupfolder
Result without this pull request
The copy eventually fails; although the WebUI might not show any file inside the target directory, if the filesystem is checked it can be seen that it tried to recursively copy the Nextcloud data directory into the target
How to test (scenario 2)
Result with this pull request
The move succeeds; opening the target directory shows the moved file
Result without this pull request
The move fails; the Nextcloud logs contain an error about not being able to move
{NEXTCLOUD_DATA_DIRECTORY}/{NAME_OF_THE_FILE_TO_MOVE}