fix: sync engine readonly folder file deletion - #10226
Conversation
|
I'll look at the test failure.. |
b3836a7 to
18bfc50
Compare
|
I've made the code distinct between totally new file and move of existing file from same sync folder to the readonly dir. I've also enabled the PermissionsTest on MacOS since they now all pass on my machine. Let's see if the CI sees it differently? (I also rebased on master) |
|
Artifact containing the AppImage: nextcloud-appimage-pr-10226.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
Tested OK on my setup (binary built on commit a910031 on a Debian unstable and using a kinda "custom" NC server): the Desktop Client patched with the content of this MR behaves exactly as told. |
|
CI failure seems unrelated: |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
sorry for the delay
a few questions or request for changes
| // Do NOT schedule the new local folder for deletion: it may contain data the user just | ||
| // created and that exists nowhere else. (see read-only folder regression, issues #7797/#10099). |
There was a problem hiding this comment.
should not this folder be read-only ?
I guess users can override the permissions and better be safe than sorry
| // Do NOT schedule the new local file for deletion: it may be data the user just created | ||
| // and that exists nowhere else. (see read-only folder regression, issues #7797/#10099). |
There was a problem hiding this comment.
same question (unless I miss something)
|
@guruz gentle reminder |
Sorry, on vacation now still, I'll have a look at your comments a bit later.. Regarding the code comments: It's about the read-only folder where users put files/folders in. Not about the new file/folder they put in. (users are locally are still able to put files into it, the read-only permissions can be overriden by the OS/file manager/user etc.. so we want to rescue thos files to be accidently deleted) |
|
If I may… IMO this MR is a (good) workaround to the bug (or lack of feature?) that if a directory has the permission flag "read-only" set, nothing prevents people to write in the directory client-side, or delete something, etc… To me the client should just do a IIRC (again, but I may be wrong) it was the fact that Windows does not support And it may be the very same reason that symlinks are not supported, too, IIRC again. Even if this one (symlinks) seems to be off topic here, that's also too bad and share, actually, the same origin, i.e. Windows support limiting features on other platforms. |
I'm back and will look at this this week @mgallien
It does for me on MacOS, but this can be easily acciently circumvented for example by pressing the right button in Finder copy dialog :-) |
Or by doing sudo or being a root user. The directory is (without this patch) a black hole for new client sides files dropped into it. (Other OS might be different/better/worse) |
This is for nextcloud#7797 and nextcloud#10099 Signed-off-by: Markus Goetz <markus@woboq.com> Assisted-by: ClaudeCode:claude-opus-4-8
|
Hi, Any news on this one? :) |
|
Hey! Is there anything that still needs doing for this to be merged? |
|
@guruz sorry for the delay |
|
/backport to stable-34.0 please |
I get it now |
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
|
@guruz we enabled compilation of the desktop client with Qt keywords only in upper case form |
Head branch was pushed to by a user without write access
|
OK, i pushed a possible fix, let's see the CI?
Sorry, I had not seen this message... |
Thanks I enabled the CI to check compilation despite this but we need the DCO to merge the PR
|
Signed-off-by: Markus Goetz <markus@woboq.com>
b04228c to
a7742d2
Compare
5 participants
Resolves
This is for #7797 and #10099
It would probably be good to backport this (if it's the correct fix and is merged...)
Summary
Show stray file in UI instead of agressively deleting it (= potential data loss)
Checklist
AI (if applicable)