Fix the regression caused due to usage of part file in decrypt-all #32401

sharidas · 2018-08-21T15:01:20Z

command

Fix the regression cuased due to the usage of part file in
decryptall command. An exception was thrown in the log
file during the rename operation.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

This change fixes the exception caused during rename operation in the decrypt-all. This issue was caused due to the introduction of using .part extension to the target. The .part file change in the decrypt-all, didn't affected the outcome of rename method. In fact the files were getting decrypted properly. The exception was happening because .part file wasn't added to the filecache, and the during the cod flow, fileinfo of the .part file was being accessed. Which was not there. Hence caused an exception which was logged. Basically, the exception didn't harmed the rename operation. In this change, we let the rename method know that this part file needs to be ignored.

Related Issue

Fixes recreating masterkey produces Exception messages in log file encryption#45

Motivation and Context

This change fixes the exception caused during rename operation in the decrypt-all. This issue was caused due to the introduction of using .part extension to the target.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...
With this PR I have tested with the shares as follows:
Encrypt the filesystem with user-keys. Create 2 users admin and user1
Login as user1 and logout.
Login as admin and create a group group1.
Add user1 to group1
create a file localshare.txt and share with user1
create a file groupshare.txt and share with group1
create a file publiclink.txt and create public link share of the file.
Now try to decrypt the filey system as follows:

➜  owncloud3 git:(master) ✗ ./occ maintenance:singleuser --on; ./occ encryption:decrypt-all -m recovery -c yes; ./occ encryption:disable; ./occ app:disable encryption;
Cannot load Xdebug - it was already loaded
Single user mode enabled
Cannot load Xdebug - it was already loaded
Disable server side encryption... done.


You are about to start to decrypt all files stored in your ownCloud.
It will depend on the encryption module and your setup if this is possible.
Depending on the number and size of your files this can take some time
Please make sure that no user access his files during this process!

prepare encryption modules...
 done.


 starting to decrypt files... 
 [->--------------------------]
Prepare "Default encryption module"

Configuring encryption module for decryption with user based keys
 decrypt files for user admin (1 of 2): /admin/files/welcome.txt 
 [----->----------------------]
Prepare "Default encryption module"

Configuring encryption module for decryption with user based keys
 decrypt files for user user1 (2 of 2): /user1/files/welcome.txt 
 [------>---------------------]

 starting to decrypt files... finished 
 [============================]


all files could be decrypted successfully!
Cannot load Xdebug - it was already loaded
Encryption is already disabled
Cannot load Xdebug - it was already loaded
encryption disabled
➜  owncloud3 git:(master) ✗

Now update the database as follows:

➜  owncloud3 git:(master) ✗ sqlite3 data/owncloud.db                                                                  
SQLite version 3.22.0 2018-01-22 18:45:57
Enter ".help" for usage hints.
sqlite> select * from oc_appconfig where appid='encryption';
encryption|recoveryKeyId|recoveryKey_09771295
encryption|publicShareKeyId|pubShare_09771295
encryption|masterKeyId|master_09771295
encryption|installed_version|1.3.1
encryption|types|filesystem
encryption|enabled|no
encryption|userSpecificKey|1
encryption|recoveryAdminEnabled|1
sqlite> delete from oc_appconfig where appid='encryption';
sqlite> select * from oc_appconfig where appid='encryption';
sqlite> 
➜  owncloud3 git:(master) ✗

delete the folders files_encryption in the data folder

➜  owncloud3 git:(master) ✗ rm -fr ./data/files_encryption ./data/user1/files_encryption ./data/admin/files_encryption 
➜  owncloud3 git:(master) ✗

re-encrypt the file system with master key as follows:

➜  owncloud3 git:(master) ✗ ./occ app:enable encryption; ./occ encryption:enable; ./occ encryption:select-encryption-type masterkey -y; ./occ encryption:encrypt-all -vvv; ./occ maintenance:singleuser --off
Cannot load Xdebug - it was already loaded
encryption enabled
Cannot load Xdebug - it was already loaded
Encryption enabled

Default module: OC_DEFAULT_MODULE
Cannot load Xdebug - it was already loaded
Master key successfully enabled.
Cannot load Xdebug - it was already loaded


You are about to encrypt all files stored in your ownCloud installation.
Depending on the number of available files, and their size, this may take quite some time.
Please ensure that no user accesses their files during this time!
Note: The encryption module you use determines which files get encrypted.

Do you really want to continue? (y/n) y


Encrypt all files with the Default encryption module
====================================================


Use master key to encrypt all files.


Start to encrypt users files
----------------------------



 all files encrypted 
 [============================]

Cannot load Xdebug - it was already loaded
Single user mode disabled
➜  owncloud3 git:(master) ✗

All the shares created were accessible for user1. The public link was also accessible.

test recreate master-key command

➜  owncloud3 git:(master) ✗ ./occ encryption:recreate-master-key -y                                                                                                       
Cannot load Xdebug - it was already loaded
Decryption started

    1 [->--------------------------]prepare encryption modules...
 done.


 decrypt files for user user1 (2 of 2): /user1/files/welcome.txt 
 [------>---------------------]

 starting to decrypt files... finished 
 [============================]


all files could be decrypted successfully!
    1 [============================]
Decryption completed

Encryption started

Waiting for creating new masterkey

New masterkey created successfully



Encrypt all files with the Default encryption module
====================================================


Use master key to encrypt all files.


Start to encrypt users files
----------------------------



 all files encrypted 
 [============================]


Encryption completed successfully

➜  owncloud3 git:(master) ✗

Did not found the data loss. The shares were again available.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2018-08-21T15:30:36Z

Codecov Report

Merging #32401 into master will decrease coverage by 0.77%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #32401      +/-   ##
============================================
- Coverage     64.11%   63.34%   -0.78%     
+ Complexity    18641    18636       -5     
============================================
  Files          1176     1176              
  Lines         70185    70104      -81     
  Branches       1270     1267       -3     
============================================
- Hits          45002    44406     -596     
- Misses        24813    25328     +515     
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	`52.84% <ø> (-0.06%)`	`0 <ø> (ø)`
#phpunit	`64.53% <100%> (-0.86%)`	`18636 <1> (-5)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/View.php	`84.6% <100%> (+0.03%)`	`398 <1> (+2)`	⬆️
lib/private/Encryption/DecryptAll.php	`95.9% <100%> (+0.06%)`	`37 <0> (ø)`	⬇️
lib/private/DB/PostgreSqlMigrator.php	`0% <0%> (-100%)`	`6% <0%> (ø)`
lib/private/DB/SQLiteSessionInit.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
lib/private/DB/SQLiteMigrator.php	`0% <0%> (-100%)`	`7% <0%> (ø)`
...ilder/ExpressionBuilder/PgSqlExpressionBuilder.php	`0% <0%> (-88.89%)`	`3% <0%> (ø)`
...ivate/Files/ObjectStore/HomeObjectStoreStorage.php	`0% <0%> (-86.67%)`	`8% <0%> (ø)`
lib/private/DB/AdapterOCI8.php	`0% <0%> (-86.67%)`	`4% <0%> (ø)`
lib/private/DB/AdapterPgSql.php	`0% <0%> (-85.72%)`	`2% <0%> (ø)`
lib/private/Repair/SqliteAutoincrement.php	`0% <0%> (-85.19%)`	`9% <0%> (ø)`
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e55e530...6f88726. Read the comment docs.

PVince81 · 2018-08-22T15:07:58Z

lib/private/Encryption/DecryptAll.php

@@ -271,7 +271,9 @@ protected function decryptFile($path) {
 			\OC\Files\Storage\Wrapper\Encryption::setDisableWriteEncryption(true);
 			$this->rootView->copy($source, $target);
 			\OC\Files\Storage\Wrapper\Encryption::setDisableWriteEncryption(false);
+			\OC::$server->getConfig()->setAppValue('core', 'ignorepartfile', 'true');


why not use a static function like above with Encryption::setDisableWriteEncryption, considering that this disabling only needs to happen in a single call and not be shared ?

So if I understood correctly, it would be better to have a static function like Encryption::setDisableWriteEncryption. All we have to do is block this call when rename operation is performed during decrypt-all : https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L843 and execute the else part. Would it be really bad if I have a static method in View.php to ignore the part file? So that user calls the method View::ignorePartFile(true) , and the variable holding true|false can be used to check the same.

a static method would be better than writing to the config in the DB

PVince81 · 2018-08-28T08:22:47Z

lib/private/Files/View.php

+	 * User can create part files example to a call for rename(), in effect
+	 * it might not be a part file. So for better control in such cases this
+	 * method would help to let the method in rename() to know if it is a
+	 * part file.


I see there are other locations in rename() where isPartialFile has a different logic.

Are you sure we shouldn't also exclude those ?

I tried to debug, based on the code flow which was causing the exception. I will post my update here after debugging further whether we need to apply exclude part file logic in those places or not.

After debugging around code https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L853-L869, lets say if I modify line https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L853 by having a check to ignore part file, the code flow will reach the else part. In the else part the shouldEmitHooks() method is called. And the path1 here will be considered as a part file by that function. Which means no signal will be emitted.

Now lets say if we don't make any changes to the PR and debug the above section where ignore part file change is missing. In this case, the code flow would call shouldEmitHooks of the if section. Which will return false, because of missing or empty path. Which means https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L855 will not be called.

Hence in my observation the result remains same, if we add the change of ignore part in the remaining part of the rename or not.

he exception was happening because .part file wasn't added to the filecache

would this change add the part file to the file cache ??

I'm still not clear why we need this hacky magic

Also, if your goal is to avoid writeUpdate then the flag needs to be checked around that and avoid it going to else as it would trigger another update in the else block.

No this change will not add the part file to the cache.

Yes, the intention here is to skip the writeUpdate and call renameUpdate. The else block update would not cause issue. For testing I tried to exclude the creation of .part file and then the code flow took the renameUpdate which didn't caused the exception. That's the reason I added the change in the if block of writeUpdate. If the code flow takes the writeUpdate path, that's where the whole problem for exception occurs.

It would have been too generic if I try to move this logic into the Files/Cache/Scanner.php and then check in isPartialFile method if the file is not part file. That would cause another exception at

core/lib/private/Files/View.php

Line 796 in 370f0a6

\OC_Hook::emit(

. The reason for that exception would be due to Files_Version pre_renameOrCopy_hook().

Let me know if more information needs to be added, then I can explain with the code paths without this change and with the change.

PVince81

👍 ugly but I don't have any better ideas...

sharidas · 2018-08-29T10:11:17Z

ugly but I don't have any better ideas..

I agree.. sigh.

command Fix the regression cuased due to the usage of part file in decryptall command. An exception was thrown in the log file during the rename operation. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2018-08-29T10:42:01Z

Backport PR #32504

sharidas added the 2 - Developing label Aug 21, 2018

sharidas added this to the development milestone Aug 21, 2018

sharidas self-assigned this Aug 21, 2018

ownclouders added Type:Bug p2-high Escalation, on top of current planning, release blocker regression labels Aug 21, 2018

sharidas requested a review from PVince81 August 21, 2018 16:34

sharidas added 3 - To Review and removed 2 - Developing labels Aug 21, 2018

PVince81 reviewed Aug 22, 2018

View reviewed changes

sharidas force-pushed the fix-regression-decryptall-partfile branch from 295add3 to 370f0a6 Compare August 27, 2018 15:06

PVince81 reviewed Aug 28, 2018

View reviewed changes

PVince81 approved these changes Aug 29, 2018

View reviewed changes

PVince81 added the backport-request label Aug 29, 2018

Fix the regression caused due to usage of part file in decrypt-all

6f88726

command Fix the regression cuased due to the usage of part file in decryptall command. An exception was thrown in the log file during the rename operation. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the fix-regression-decryptall-partfile branch from 370f0a6 to 6f88726 Compare August 29, 2018 10:24

sharidas mentioned this pull request Aug 29, 2018

[stable10] Backport of Fix the regression caused due to usage of part… #32504

Merged

10 tasks

PVince81 merged commit c0bb702 into master Aug 29, 2018

PVince81 deleted the fix-regression-decryptall-partfile branch August 29, 2018 15:01

phil-davis removed the backport-request label Aug 29, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the regression caused due to usage of part file in decrypt-all #32401

Fix the regression caused due to usage of part file in decrypt-all #32401

sharidas commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading

PVince81 Aug 22, 2018

sharidas Aug 27, 2018

PVince81 Aug 27, 2018

sharidas Aug 28, 2018

PVince81 Aug 28, 2018

sharidas Aug 28, 2018

sharidas Aug 28, 2018

PVince81 Aug 28, 2018

sharidas Aug 29, 2018 •

edited

Loading

PVince81 Aug 29, 2018

PVince81 left a comment

sharidas commented Aug 29, 2018

sharidas commented Aug 29, 2018

Fix the regression caused due to usage of part file in decrypt-all #32401

Fix the regression caused due to usage of part file in decrypt-all #32401

Conversation

sharidas commented Aug 21, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

codecov bot commented Aug 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Aug 29, 2018

sharidas commented Aug 29, 2018

sharidas commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading

sharidas Aug 29, 2018 •

edited

Loading