Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(SSE compatibility) Fix SSE files being mistaken for E2EE #495

Closed
wants to merge 4 commits into from

Conversation

fluse1367
Copy link

Based on my observations, I'm pretty sure to have found a somewhat reliable way to tell SSE and E2EE files apart, however i do not know if this is 100% reliable. I've tested this fix a bit and it seems like it's working fine.

What I've learned is, that with SSE, a folder will never be marked as encrypted in the filecache. With E2EE a file can only be E2EE if it's inside an E2EE folder. So, if a folder is marked as encrypted, it and all of its contents have to be E2EE.

Based on that, its fairly easy to check if a file/folder is SSE or E2EE: If a node has any parent marked as encrypted, it is E2EE. If that node however has no parent marked as encrypted but the node itself is, it is SSE.

I know this plugin is not compatible with SSE, however i still did this fix to at least increase compatibility (also, i have E2EE alongside with SSE and i do not want to disable either, so yeah this is the outcome).
This also addresses #489.

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the reviewing 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 reviewing 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!

@fluse1367
Copy link
Author

so, i've tested it on my own installation since i made this PR, and so far everything just works fine. i didn't encounter any issues. i think i successfully made E2EE and SSE compatible

@fluse1367 fluse1367 changed the title Fix SSE files being mistaken for E2EE (SSE compatibility) Fix SSE files being mistaken for E2EE Jan 30, 2024
Copy link
Collaborator

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.
I left some minor code style comments :)

lib/EncryptionManager.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
@fluse1367
Copy link
Author

Makes sense, thanks. I left some minor code style comments :)

thanks! :)
do i just accept the suggestions via the github web ui? like clicking "sign off and commit suggestion"? so adding 3 commits?
sorry never did such a code review ^^"

@artonge
Copy link
Collaborator

artonge commented Jan 30, 2024

do i just accept the suggestions via the github web ui? like clicking "sign off and commit suggestion"? so adding 3 commits?

I would recommend doing it locally. There are other places where you use spaces instead of spaces that should be updated too.

@fluse1367
Copy link
Author

alright, done!

@artonge
Copy link
Collaborator

artonge commented Feb 5, 2024

Looks good @fluse1367 :).
Looks like there is a conflict with master in E2EEnabledPathCache. Can you fix it?

# Conflicts:
#	lib/E2EEnabledPathCache.php
@fluse1367
Copy link
Author

done @artonge :)

@artonge
Copy link
Collaborator

artonge commented Feb 13, 2024

@fluse1367 some unit tests are failing due to your changes. Can you take a look?

@artonge
Copy link
Collaborator

artonge commented Mar 14, 2024

Closing in favor of #593 due to inactivity

@fluse1367
Copy link
Author

hey @artonge, sorry for not answering :( thanks for taking over ❤️
happy to see the changes merged :D

@artonge
Copy link
Collaborator

artonge commented Apr 9, 2024

hey @artonge, sorry for not answering :( thanks for taking over ❤️ happy to see the changes merged :D

Thanks for your investigation and work :)

@artonge
Copy link
Collaborator

artonge commented Aug 26, 2024

@fluse1367 Have you been using both encryption since this PR got merged? Do you have any feedback? Is there some limitation pending?

@fluse1367
Copy link
Author

@fluse1367 Have you been using both encryption since this PR got merged? Do you have any feedback? Is there some limitation pending?

@artonge yes i have and everything's working quite well ;P
at least, its working just like if i had either, SSE or E2EE enabled, not both

one time tho (i dont know how it happened, and it didnt happen again) my instance got into a weird state where some temporary part files (from uploading stuff) remained on the FS and didnt get deleted, that caused NC to be unable to interact with the target files AT ALL. however i just deleted the part files and then everything went back to normal. idk if that has something to do with the patch tho, may be just a regular bug 🤷‍♀️

i didnt need to recover from any of my NC backups, and as far as i can tell, all data is still intact

however, i'm still doing regular external backups of my e2ee stuff from the source (using gnome deja dup), just in case ^^

@artonge
Copy link
Collaborator

artonge commented Sep 2, 2024

Thanks for the feedback @fluse1367.

We'll keep on eye open for such kind of bug :). Was it when uploading E2EE files? Which files did you delete?

@fluse1367
Copy link
Author

Thanks for the feedback @fluse1367.

We'll keep on eye open for such kind of bug :). Was it when uploading E2EE files? Which files did you delete?

@artonge
honestly i cant remember that well 😅 i think the affected files were e2ee files. i deleted the .part files (the ones that are being created as temporary files during any upload) for whatever reason this one time they didnt get deleted automatically 🤷‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants