Fix encryption wrapper not seen by groupfolder cache

[danxuliu]

Fixes #2909

Besides the fix itself this pull request also adds E2E tests related to the issue (although it would have been better if they were integration tests, but those were not setup yet in the groupfolders app).

Jail wrappers reuse the cache of the wrapped storage even if another storage is explicitly given. Due to that, when the cache is got from an storage and that storage has a Jail all the wrappers above it are not known to the cache, and only those wrapped by the Jail are taken into account.

In general that works fine, as in most cases the cache does not need to know the details of a storage. However, it needs to know if an Encryption wrapper is present in the storage when moving files into it, as the file cache explicitly clears the encrypted flag when moving a file from an encrypted storage to a non encrypted storage.

When encryption is enabled the Encryption wrapper is applied on the storage of the GroupMountPoint. As that storage internally has a Jail, even if the cache is got on the outer storage wrapper the cache does not know about the Encryption wrapper, only about the storage wrapped by the Jail, so all files moved from an encrypted storage to an encrypted groupfolder ended wrongly marked as not encrypted.

To solve that now the Jail used by groupfolders does not reuse the inner cache when encryption is enabled, and instead passes the given storage. This is applied only when encryption is enabled, as reusing the inner cache was done as a performance optimization.

Note that it does not seem to be possible to move the Encryption wrapper below the Jail, as the Encryption constructor requires a mount point, and the GroupMountPoint storage includes the Jail (but maybe I am missing something :-) ).

An alternative fix would be, in server, adding an optional parameter to Jail wrappers to not reuse the internal cache and then, in groupfolders app, disable reusing the internal cache when encryption is enabled. This would make possible to easily adjust the behaviour if problematic also in other Jail uses.

However, note that reusing the inner cache when encryption is enabled does not seem to be a problem when using shared folders (at least when trying with just a folder shared by another user, maybe there are some fancy scenarios in which it fails too, I do not know 🤷), as in that case, although SharedStorage is a Jail, it is created with a null storage and then initialized when needed to wrap the storage of the shared file owner, which is already fully setup and therefore already includes an Encryption wrapper.

Independently of that, it is worth noting that, unlike other storage wrappers, GroupFolderStorage persists the Cache object once got (by default storage wrappers get it from the wrapped storage every time). Therefore, if a cache was got on the GroupFolderStorage before it was wrapped with the Encryption wrapper that cache would not seen the Encryption wrapper even with neither of the fixes.

However, as the storage passed to the GroupMountPoint constructor will be wrapped as needed (for example, to add the Encryption wrapper) and the outer wrapper will be the storage set in the GroupMountPoint that should not be a problem (unless the constructor of GroupMountPoint or MountPoint is eventually modified to explicitly or implicitly get the cache before wrapping the storage).

[come-nc]

Awesome work and investigation, thank you!

I’m pondering just reverting the perf optimisation on jail if it was the source of the problem, but I’ll let @icewind1991 decide on that.

[danxuliu]

While it is decided whether this should be fixed instead in server or not, would it be possible to merge the fix in groupfolders (including backports) and release new versions? :-)

Note that if in the end it is fixed in server that should not be a problem, as the fix here should also work without problems with the fix in server; it would just need to be reverted once no longer needed, but it should not cause an incompatibility due to having a non-reverted groupfolders version on a fixed server version, so it would not require an immediate groupfolders release once/if the fix is applied on server.

[come-nc]

Error: lib/Mount/GroupFolderEncryptionJail.php:42:14: UndefinedClass: Class, interface or enum named OC\Files\Cache\Wrapper\CacheJail does not exist (see https://psalm.dev/019)
-----------------------------
1 errors found
------------------------------

You need to add CacheJail to the stubs because it’s not part of OCP.

[danxuliu]

You need to add CacheJail to the stubs because it’s not part of OCP.

Done, thanks for the clarification.

The Cypress failures are unrelated to this pull request, so I opened another one to adjust the sidebar data attribute name to changes in server.

[danxuliu]

/backport to stable29

[danxuliu]

/backport to stable28

[danxuliu]

I have rebased on latest master, adjusted the license headers and changed some quotes in the tests from ` to ' (as no substitution was needed).

The E2E encryption failures are unrelated to this pull request (they can be reproduced on master). They are a regression introduced in nextcloud/server#47044.

[danxuliu]

/backport to stable30

[danxuliu]

After the fix in nextcloud/server#47346 CI is finally green 🎉 Would it be OK to merge it? :-)