encryption 2.0 app

[th3fallen]

Depends on #14376

Very much in development. Please point out any missed opportunities to use new methods/interfaces @schiesbn

• The encryption module needs a upgrade script to migrate the keys from the old encryption app to the new one, unfortunately we had to make some small changes to the folder structure (added by @schiesbn)

TODOs:

• Throw exception in case a file cannot be decrypted because enc module could not be loaded - 503 on clients
• Throw exception in case a file cannot be decrypted because encryption is disabled - 503 on clients - @schiesbn this is going to be funny - as soon as there is no enc manager we have no way to detect if a file is encrypted
• Throw exception if keys are missing - 503 on clients
• Prevent sharing in case keys are missing - pre hook
• File size is lost on upload (browser and webdav)

To Test:

• Versions
• desktop sync
• mount option

Broken scenarios:

• share by link
• delete, restore from trash, download -> corrupted - workaround - f7c9066
• previews in trash bin are broken - works as soon as the trash wrapper is properly in place - see f7c9066

[LukasReschke]

🙈

[th3fallen]

yes needs refactoring, a lot of things are not in di either.

[LukasReschke]

lowStrength? 🙈

[th3fallen]

Yes, its just an identifier for the key, it used to be an md5 of the time so security isnt a big concern.

[LukasReschke]

http://crypto.junod.info/2012/05/24/owncloud-4-0-and-encryption/ – that's how we generated the key in the past, so we had quite some mistakes 🙈

But you are absolutely right: In this case this is okay. Valid point 👍

[th3fallen]

But you are absolutely right

Never get tired of hearing that 😉 Lol (as infrequently as i do...)

[DeepDiver1975]

please get rid of this - that's what different exception classes are for

[th3fallen]

Was copying functionality over, already have a note to update. (will be in next update)

[DeepDiver1975]

2.6k SLOC and not one single unit test?

@schiesbn @th3fallen did you miss to add them? (Let there be hope ....)

[schiessle]

@DeepDiver1975 can't speak for @th3fallen but I assume that he is still in the state where he copied a lot of stuff from the old encryption app, restructure it, start adding new code etc. So it is probably to early for unit tests (similar to my pr). I'm quite sure unit test will come if it is the right time for it.

@th3fallen of course you will add new unit tests for your classes and methods. But because most of the existing test in apps/files_encryption are quite high level you can probably reuse most of it as a addition to the new tests. I would also recommend to keep most of them around. Over time we added a lot of corner cases to the tests. So if the old (high-level) tests will pass on the new encryption we can already be quite sure that a lot of the new stuff works as expected.

[schiessle]

you no longer need to register a stream wrapper. The stream wrapper is now in core

[th3fallen]

thanks removed, will push with next update

[DeepDiver1975]

But because most of the existing test in apps/files_encryption are quite high level you can probably reuse most of it as a addition to the new tests. I would also recommend to keep most of them around.

Well - in the first place we can keep the existing unit integration test - just to ensure that the high level operations continue to work. But these tests have to die and be replace by proper unit tests.

Furthermore unit test are to be written TOGETHER with the code and not afterwards.

Feel free to ping me anytime to get into the details - maybe a personal talk is better suited then commenting on github.

Thanks

[schiessle]

You probably also don't need all the file and sharing related hooks. Rename and delete operations will update the key storage right here https://github.com/owncloud/core/pull/14376/files#diff-a36b9c0307e0126dd50baa38fb4bb74aR1

And I think I will also listen to share hooks in core calculate the new list of users and then use the update() call to inform you that a file/folder needs to be re-encrypted for a new set of users https://github.com/owncloud/core/pull/14376/files#diff-8d7e0ca7514556cd6929b4c073c9629dR90

[th3fallen]

thanks for the notes @schiesbn will update, @DeepDiver1975 Since this is much more of a refactor than a rewrite i was waiting until i go the functionality copied over before i assessed the current status of tests. So i don't end up writing tests that are already written.

[DeepDiver1975]

So i don't end up writing tests that are already written.

well - this is only partially valid - the existing test are more of an integration test type - we require unit tests.

I'm bugging you all that much - because code needs to written and structured in a way that true unit testing is possible. Just coding right away and applying unit tests afterwards will break.

So these are my requirement:

• I want one unit test class for each introduced class in core
• no static calls of what soever kind
• no hard dependencies between classes - inject dependencies and use mocks on unit tests

Feel free to ping me anytime to get into the details - maybe a personal talk is better suited then commenting on github.

^ Still applies 😉

[th3fallen]

fair enough... adds another thing to my list

[MorrisJobke]

@DeepDiver1975 can't speak for @th3fallen but I assume that he is still in the state where he copied a lot of stuff from the old encryption app, restructure it, start adding new code etc. So it is probably to early for unit tests (similar to my pr). I'm quite sure unit test will come if it is the right time for it.

Write the unit tests in advance. Design the way you want to interact with the encryption in the unit/acceptance test and then write the implement the code that does the work ;)

Trust me: This will make the code a lot better! ;)

[DeepDiver1975]

From a unit test and performance POV using an Storage interface here makes more sense instead of the View - do I miss anything?

[icewind1991]

It would be better to have seperate key stores for system keys and user keys (where you inject the $uid so you have one keystore object per user)

Additionally instead of working on a View you should work directly on a Storage
The storage should have the keys in the root of the storage (create a new local storage for $user->getHome() .'/files/encryption' or $dataDir . '/files_encryption)

Working on a view wont work since you need to access the keyfiles without going trough encryption.

[DeepDiver1975]

@icewind1991 can I ask you to come up with a POC on this approach? A PR against this branch would be best I guess - THX

[DeepDiver1975]

And unit tested of course 😉

[th3fallen]

@DeepDiver1975 should setupServerSide() be a part of the app or core? Also ready()

[DeepDiver1975]

should setupServerSide()

seems specific to the encryption implementation as it creates the keys

[DeepDiver1975]

Also ready()

the same I guess

[DeepDiver1975]

that might be better suited in core - as the keystore implementation know about the location of that folder: https://github.com/owncloud/core/blob/master/apps/files_encryption/lib/util.php#L156

[th3fallen]

what i thought but wanted a second opinion. Thanks, boy this util class is a mess.

[th3fallen]

i thought that was the case but i assumed there was a reason it was in the code.... What i get for assuming...

[DeepDiver1975]

i thought that was the case but i assumed there was a reason it was in the code.... What i get for assuming...

dependencies in info.xml are relative new (since 8.0) - that's why 😉

[scrutinizer-notifier]

A new inspection was created.

[scrutinizer-notifier]

The inspection completed: 32 new issues, 273 updated code elements

[karlitschek]

yes. awesome work guys :-)

[scrutinizer-notifier]

A new inspection was created.

[scrutinizer-notifier]

A new inspection was created.

[DeepDiver1975]

[th3fallen]

🎉 🍺 🙌

[DeepDiver1975]

👍

[th3fallen]

@DeepDiver1975 missing migrator shouldn't we wait for that?

[DeepDiver1975]

@DeepDiver1975 missing migrator shouldn't we wait for that?

we do this isolated in order to get going with other open prs

[schiessle]

👍 migration will come in a seperate pull request later today

[MorrisJobke]

Don't use weird date formats

https://xkcd.com/1179/

[th3fallen]

You know us crazy Americans and our crazy date formats....

[DeepDiver1975]

[LukasReschke]

[th3fallen]

[MorrisJobke]

This feels totally wrong :(

Returns boolean, string and can throw errors 🙈 🙉 🙊

[DeepDiver1975]

file headers will be fixed with the license script .. pr is #15441