[Bug]: File ID changes when moving folder in or out of a share with object store as primary storage

artonge commented 2 years ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

When moving a file or a folder in or out of a share, the file ID is changes. This means that favorites and shares are lost.

Tested on v22, v23 and v24.

Steps to reproduce

Alice share /Folder1 with Bob
Bob share /Folder2 with Charlie
Bob favorite /Folder2
Bob moves /Folder2 in /Folder1
/Folder1/Folder2 is now neither share nor favorite.

Expected behavior

Either:

The file ID should stay the same.
The file ID should be updated everywhere it is referenced.

Installation method

Manual installation

Operating system

RHEL/CentOS

PHP engine version

PHP 7.4

Web server

Nginx

Database engine version

No response

Is this bug present after an update or on a fresh install?

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

No response

What user-backends are you using?

[ ] Default user-backend (database)
[ ] LDAP/ Active Directory
[ ] SSO - SAML
[ ] Other

Configuration report

No response

List of activated Apps

Enabled:
  - accessibility: 1.10.0
  - activity: 2.16.0
  - cloud_federation_api: 1.7.0
  - comments: 1.14.0
  - contactsinteraction: 1.5.0
  - dav: 1.22.0
  - federatedfilesharing: 1.14.0
  - federation: 1.14.0
  - files: 1.19.0
  - files_pdfviewer: 2.5.0
  - files_rightclick: 1.3.0
  - files_sharing: 1.16.1
  - files_trashbin: 1.14.0
  - files_versions: 1.17.0
  - files_videoplayer: 1.13.0
  - firstrunwizard: 2.13.0
  - logreader: 2.9.0
  - lookup_server_connector: 1.12.0
  - nextcloud_announcements: 1.13.0
  - notifications: 2.12.0
  - oauth2: 1.12.0
  - photos: 1.6.0
  - privacy: 1.8.0
  - provisioning_api: 1.14.0
  - recommendations: 1.3.0
  - serverinfo: 1.14.0
  - settings: 1.6.0
  - sharebymail: 1.14.0
  - support: 1.7.0
  - survey_client: 1.12.0
  - systemtags: 1.14.0
  - text: 3.5.1
  - theming: 1.15.0
  - twofactor_backupcodes: 1.13.0
  - updatenotification: 1.14.0
  - user_status: 1.4.0
  - viewer: 1.8.0
  - weather_status: 1.4.0
  - workflowengine: 2.6.0
Disabled:
  - admin_audit
  - bruteforcesettings
  - dashboard: 7.4.0
  - encryption
  - files_external
  - hmr_enabler
  - testing
  - user_ldap

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

PVince81 commented 2 years ago

@icewind1991 FYI

It looks like ObjectStoreStorage doesn't override moveFromStorage so perhaps it doesn't detect properly that the underlying storage is the same, so it might use copy and delete ?

But even with copy and delete, isn't there already a code path that will fix the file id of the target so that it stays the same ?

This seems to work fine for Local storage, probably because of the shortcut that uses rename.

PVince81 commented 2 years ago

shouldn't the View use ->isSameStorage() ? https://github.com/nextcloud/server/blob/master/lib/private/Files/View.php#L824 (I know it's legacy)
can we use getId() to compare if storages are the same ?

I've found that isSameStorage in this scenario returns false, because $this is a HomeObjectStoreStorage but the wrapped one from the SharedStorage is actually a ObjectStorageStorage, so their id is the same but somehow the instance isn't ???!

PVince81 commented 2 years ago

never mind, I looked wrong. needs further debugging of isSameStorage

PVince81 commented 2 years ago

it seems they're both HomeObjectStoreStorage but they are not equal

so either we need to fix equality check in isSameStorage, for example using the id, or find out why they have different instances

artonge commented 2 years ago

[ ] Re-enable the test that discovered this issue https://github.com/nextcloud/server/pull/31728/files#diff-acda801c52c37d51d5c26bbd6a59fb34dc3d139d574ca4f94d22248b16a47dcdR243

unteem commented 2 years ago

I have been facing this issue

If I make the isSameStorage condition pass (did not debug it but just removed the condition), I'm still having issues the rename function in lib/private/Files/ObjectStore/ObjectStoreStorage.php is not working as it supposed in this context

if bob transfer files to jane what is happening is: everything files in jane's space are deleted then bob files are transferred then jane files root folder is recreated with wrong permissions

While files are transferred with shares but it seems that its not doing things in the right order

I'll try to debug this further

szaimen commented 1 year ago

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

PVince81 commented 1 year ago

reproduced on stable25 (e50d0d2440f0f0d70bcf586afd3c7adcbf593a8c) post 25.0.3

moving a file into a received share or out of it will change the file id

PVince81 commented 1 year ago

[0] OC\Files\Storage\Common->moveFromStorage @ /srv/www/htdocs/server/lib/private/Files/Storage/Common.php:695
[1] OC\Files\Storage\Wrapper\Wrapper->moveFromStorage @ /srv/www/htdocs/server/lib/private/Files/Storage/Wrapper/Wrapper.php:595
[2] OC\Files\Storage\Wrapper\Availability->moveFromStorage @ /srv/www/htdocs/server/lib/private/Files/Storage/Wrapper/Availability.php:437
[3] OC\Files\Storage\Wrapper\Quota->moveFromStorage @ /srv/www/htdocs/server/lib/private/Files/Storage/Wrapper/Quota.php:239
[4] OC\Files\Storage\Wrapper\Wrapper->moveFromStorage @ /srv/www/htdocs/server/lib/private/Files/Storage/Wrapper/Wrapper.php:595
[5] OCA\Files_Trashbin\Storage->moveFromStorage @ /srv/www/htdocs/server/apps/files_trashbin/lib/Storage.php:239
[6] OC\Files\View->rename @ /srv/www/htdocs/server/lib/private/Files/View.php:831
[7] OCA\DAV\Connector\Sabre\Directory->moveInto @ /srv/www/htdocs/server/apps/dav/lib/Connector/Sabre/Directory.php:442
[8] Sabre\DAV\Tree->move @ /srv/www/htdocs/server/3rdparty/sabre/dav/lib/DAV/Tree.php:160
[9] Sabre\DAV\CorePlugin->httpMove @ /srv/www/htdocs/server/3rdparty/sabre/dav/lib/DAV/CorePlugin.php:612
[10] Sabre\DAV\Server->emit @ /srv/www/htdocs/server/3rdparty/sabre/event/lib/WildcardEmitterTrait.php:89
[11] Sabre\DAV\Server->invokeMethod @ /srv/www/htdocs/server/3rdparty/sabre/dav/lib/DAV/Server.php:472
[12] Sabre\DAV\Server->start @ /srv/www/htdocs/server/3rdparty/sabre/dav/lib/DAV/Server.php:253
[13] Sabre\DAV\Server->exec @ /srv/www/htdocs/server/3rdparty/sabre/dav/lib/DAV/Server.php:321
[14] OCA\DAV\Server->exec @ /srv/www/htdocs/server/apps/dav/lib/Server.php:360
[15] require_once @ /srv/www/htdocs/server/apps/dav/appinfo/v2/remote.php:35
[16] {main} @ /srv/www/htdocs/server/remote.php:171

reaching to https://github.com/nextcloud/server/blob/stable25/lib/private/Files/Storage/Common.php#L695 where a copy is done instead of a move, because it did not recognize the underlying same storage (primary object store, despite the share)

after the copy the file id isn't restored

it goes further into copyInner:

[0] OC\Files\ObjectStore\ObjectStoreStorage->copyInner @ /srv/www/htdocs/server/lib/private/Files/ObjectStore/ObjectStoreStorage.php:577
[1] OC\Files\ObjectStore\ObjectStoreStorage->copyFromStorage @ /srv/www/htdocs/server/lib/private/Files/ObjectStore/ObjectStoreStorage.php:553

am not sure what the right approach is here. somehow this code reminds me of the group folder issue where the shortcut was used but we didn't want to. and here we want it to use the shortcut and here we want it to use the shortcut

@icewind1991

PVince81 commented 1 year ago

some fileid action is happening here: https://github.com/nextcloud/server/blob/stable25/lib/private/Files/ObjectStore/ObjectStoreStorage.php#L607

but it's really just a copy from one urn to another one

in any case, when moving inside or outside a share, no copy is required, we should just update the oc_filecache entries and leave the fileid as is. And also make sure that this code path properly preserves encryption keys as we saw that shortcuts sometimes don't.

PVince81 commented 1 year ago

the bug might be in isSameStorage.

here's the stack for the source storage:

 ▾ $sourceStorage = (OCA\Files_Trashbin\Storage [13])
  \
   ▾ $sourceStorage->storage = (OCA\Files_Sharing\SharedStorage [20])
    \
     ▾ $sourceStorage->storage->storage = (OC\Files\Storage\Wrapper\PermissionsMask [7])
      \
       ▾ $sourceStorage->storage->storage->storage = (OCA\Files_Trashbin\Storage [13])
        \
         ▾ $sourceStorage->storage->storage->storage->storage = (OC\Files\Storage\Wrapper\Quota [10])
          \
           ▾ $sourceStorage->storage->storage->storage->storage->storage = (OC\Files\Storage\Wrapper\Availability [7])
            \
             ▾ $sourceStorage->storage->storage->storage->storage->storage->storage = (OC\Files\ObjectStore\HomeObjectStoreStorage [16])

and here for $this:

 ▾ $this = (OC\Files\ObjectStore\HomeObjectStoreStorage [16])
  \
   ▸ $this->cache = (OC\Files\Cache\Cache [8])
   |
   ▸ $this->scanner = (OC\Files\ObjectStore\NoopScanner [7])
   |
   ▸ $this->watcher = (OC\Files\Cache\Watcher [5])
...

hmmm, so it looks like the same instance

PVince81 commented 1 year ago

I've debugger further: the loop looks fine. and although both objects look the same, the === operator returns false

should we rather match them by the storage id string ?

PVince81 commented 1 year ago

can't use $this->getId() and $storage->getId() as they return the top level id.

$this->id and $storage->id however are the same, but seem not accessible: https://github.com/nextcloud/server/blob/stable25/lib/private/Files/ObjectStore/HomeObjectStoreStorage.php#L45 vs https://github.com/nextcloud/server/blob/stable25/lib/private/Files/ObjectStore/ObjectStoreStorage.php#L54

kind of messy :-/

should we expose the internal ? or is the approach completely wrong ?

nextcloud / server