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

Fix path option for the verify checksum command #35483

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

sharidas
Copy link
Contributor

Fix path option for the verify checksum command

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

Description

The verify checksum command was failing to fix the checksum when path option is provided as:

  • $user/files/foo or /$user/files/foo/bar

The reason for this is get() in the https://github.com/owncloud/core/blob/master/lib/private/Files/Node/Root.php#L180 will add extra entry in the cache. This PR tries to resolve the issue by analyzing the input by checking if the path contains $user/files, if so then getUserFolder would be used to access the file provided using -p option of the command.

Related Issue

  • Fixes <issue_link>

Motivation and Context

This PR solves the addition of the file path to the filecache table, when -p option is used with the command.

How Has This Been Tested?

Here is how the tests done:

  • Modify the checksum for the files welcome.txt and a.txt for the user admin.
  • Now execute the command to fix the checksum as shown:
sujith@sujith-ownCloud  ~/test/owncloud3   log-exception-console ●  ./occ files:checksums:verify -r -p /admin/files/welcome.txt
Cannot load Xdebug - it was already loaded
Mismatch for home::admin/files/welcome.txt:
 Filecache:     SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089a MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64
 Actual:        SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089d MD5:368e3a6cb99f88c3543123931d786e21 ADLER32:c5ad3a63
Repairing files/welcome.txt
 sujith@sujith-ownCloud  ~/test/owncloud3   log-exception-console ●  ./occ files:checksums:verify -r -p /admin/files/a.txt      
Cannot load Xdebug - it was already loaded
Mismatch for home::admin/files/a.txt:
 Filecache:     SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089a MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64
 Actual:        SHA1:ec223a7d316191091cd10fc04401e41de66ffee4 MD5:545ce406d805800b01fad61055c7d4d2 ADLER32:194703d6
Repairing files/a.txt
 sujith@sujith-ownCloud  ~/test/owncloud3   log-exception-console ● 

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

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)

@sharidas sharidas added this to the development milestone Jun 11, 2019
@sharidas sharidas self-assigned this Jun 11, 2019
@sharidas sharidas changed the title [WIP] Fix path option for the verify checksum command Fix path option for the verify checksum command Jun 11, 2019
@jvillafanez
Copy link
Member

Why do we use the user folder for one thing and the root folder for another?

If the user has to enter a path like /admin/files/welcome.txt he can also enter /admin/files_version/welcome.txt.v1872 and whatever path he wants. I expect the behaviour to be the same because it's the admin's responsability to provide the correct path.

If you're expecting to always access to the user's files, then assume that the path the admin will provide will always be under the user files. It would be way better to provide an additional (required?) option for the admin to provide the username. To be checked if we can change this.

I'm pretty sure the problem is caused by the prepended (or maybe the lack of) "/" char in the path. If this is really the problem, just focus on fixing this. No need to change anything else and make the fix more complex than it should.

@sharidas
Copy link
Contributor Author

I'm pretty sure the problem is caused by the prepended (or maybe the lack of) "/" char in the path. If this is really the problem, just focus on fixing this. No need to change anything else and make the fix more complex than it should.

No, unfortunately prepending "/" char is not the root cause of the problem. Because for instance on a scratch installation of oC and logged in as admin user here is the snapshot of the table filecache:

mysql> select fileid, path, checksum from oc_filecache;
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
| fileid | path                           | checksum                                                                                            |
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
|      1 |                                |                                                                                                     |
|      2 | cache                          |                                                                                                     |
|      3 | files                          |                                                                                                     |
|      4 | files/welcome.txt              | SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089c MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64 |
|      5 |                                |                                                                                                     |
|      6 | avatars                        |                                                                                                     |
|      7 | files_zsync                    |                                                                                                     |
|      8 | thumbnails                     |                                                                                                     |
|      9 | thumbnails/4                   |                                                                                                     |
|     10 | thumbnails/4/2048-2048-max.png | SHA1:2634fed078d1978f24f71892bf4ee0e4bd0c3c99 MD5:dd249372f7a68c551f7e6b2615d49463 ADLER32:821230d4 |
|     11 | thumbnails/4/32-32.png         | SHA1:a8ac4f223e7a44a9f8ff13da45abaa18fed7ccfb MD5:96cb8ec53fd2af54cca5536b4b3e2970 ADLER32:886ce34a |
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
11 rows in set (0.00 sec)

Now run verify checksum command:

sujith@sujith-ownCloud  ~/test/owncloud   fix-path-verify-checksum  ./occ files:checksums:verify -r -p /admin/files/welcome.txt
Cannot load Xdebug - it was already loaded
 sujith@sujith-ownCloud  ~/test/owncloud   fix-path-verify-checksum 

Again revisit the table:

mysql> select fileid, path, checksum from oc_filecache;
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
| fileid | path                           | checksum                                                                                            |
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
|      1 |                                |                                                                                                     |
|      2 | cache                          |                                                                                                     |
|      3 | files                          |                                                                                                     |
|      4 | files/welcome.txt              | SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089c MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64 |
|      5 |                                |                                                                                                     |
|      6 | avatars                        |                                                                                                     |
|      7 | files_zsync                    |                                                                                                     |
|      8 | thumbnails                     |                                                                                                     |
|      9 | thumbnails/4                   |                                                                                                     |
|     10 | thumbnails/4/2048-2048-max.png | SHA1:2634fed078d1978f24f71892bf4ee0e4bd0c3c99 MD5:dd249372f7a68c551f7e6b2615d49463 ADLER32:821230d4 |
|     11 | thumbnails/4/32-32.png         | SHA1:a8ac4f223e7a44a9f8ff13da45abaa18fed7ccfb MD5:96cb8ec53fd2af54cca5536b4b3e2970 ADLER32:886ce34a |
|     12 | admin                          |                                                                                                     |
|     13 | admin/files                    |                                                                                                     |
|     14 | admin/files/welcome.txt        |                                                                                                     |
+--------+--------------------------------+-----------------------------------------------------------------------------------------------------+
14 rows in set (0.00 sec)

mysql>

This happens because of the problem in the code flow mentioned here:

https://github.com/owncloud/core/blob/master/lib/private/Files/Node/Root.php#L188
https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1409
https://github.com/owncloud/core/blob/master/lib/private/Files/View.php#L1368
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L330
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L157
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L116
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Checksum.php#L187
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Encryption.php#L175
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Common.php#L650
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Encryption.php#L180
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Cache.php#L167 (false)
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Encryption.php#L196
https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Checksum.php#L199
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L129 (admin/files/welcome.txt)
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L176
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L157
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L129 (admin/files)
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L176
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L129 ( admin )
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L184
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Cache.php#L167
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L222 (adds admin to cache)
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L222 ( adds admin/files to cache)
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L222 ( adds admin/files/welcome.txt to cache )
https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Scanner.php#L341

Hence I was doubtful if to continue with getUserFolder only or use both,

@jvillafanez
Copy link
Member

I think we should clarify the help description first.

If the path the admin should input starts from the owncloud's data directory, the admin can use /admin/files/folder/file.txt as well as /joe/thumbnails/4/32-32.png. If this is the case, /joe/thumbnails/4/32-32.png should also work, which I don't think it is (it would generate a new entry in the DB without matching the one that is supposed to match).

If the case above is something we don't want because we want to fix always the checksums from the $user/files directory without caring about other folders (thumbnails, files_versions, etc), then maybe using both the username and path options, and also assuming that the path is relative to the user folder (not to the data directory) is a better choice.

@micbar
Copy link
Contributor

micbar commented Jun 25, 2019

@sharidas Could you give us an update on this ticket?

@sharidas
Copy link
Contributor Author

sharidas commented Jun 25, 2019

@sharidas Could you give us an update on this ticket?

The core of the issue is that we need to address #35483 (comment). There is a query from https://github.com/owncloud/enterprise/issues/3275#issuecomment-501417565 which asks how files:scan works. And that is something different. Because files:scan takes a different route to process the path. We need clarification, whether the path provided to this command:

  • is it files in the users data directory? (i.e, user1/files/file1.txt, user1/files/files2.txt etc)
  • Should it consider the path of users thumbnails or versions etc?

If we can get some insight over the same, it would be helpful.

@jvillafanez
Copy link
Member

Double check all possible options:

  • username + path
  • username without path
  • path without username
  • no username nor path.

@sharidas
Copy link
Contributor Author

The tested combinations:

  • Provide option path and `username
sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ●  ./occ files:checksums:verify -p welcome.txt -u admin
Cannot load Xdebug - it was already loaded
Mismatch for home::admin/files/welcome.txt:
 Filecache:     SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089b MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64
 Actual:        SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089d MD5:368e3a6cb99f88c3543123931d786e21 ADLER32:c5ad3a63
 ✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ● 
  • Provide option username only:
✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ●  ./occ files:checksums:verify -u admin               
Cannot load Xdebug - it was already loaded
Mismatch for home::admin/files/Documents/Example.odt:
 Filecache:     SHA1:a2c861e88db506c0deadb9f7d78a1e27212ce9fb MD5:12348c01fb20e0230c1afdacfe514307 ADLER32:85ffdff4
 Actual:        SHA1:a2c861e88db506c0deadb9f7d78a1e27212ce9fc MD5:12348c01fb20e0230c1afdacfe514308 ADLER32:85ffdff5
Mismatch for home::admin/files/Photos/Paris.jpg:
 Filecache:     SHA1:73e6f4c116ef8fa4568028c7a1e45917ffeea8d7 MD5:0959a73dbc91b7c4fa8daa511aa00bb6 ADLER32:1c41d5d3
 Actual:        SHA1:73e6f4c116ef8fa4568028c7a1e45917ffeea8d8 MD5:0959a73dbc91b7c4fa8daa511aa00bb7 ADLER32:1c41d5d2
Mismatch for home::admin/files/Photos/San Francisco.jpg:
 Filecache:     SHA1:f00431c8f8ac1530febe519ca413806be16e8e4b MD5:84d5d4f2e31ecc00680f5886645c59a3 ADLER32:3124fdc5
 Actual:        SHA1:f00431c8f8ac1530febe519ca413806be16e8e4c MD5:84d5d4f2e31ecc00680f5886645c59a2 ADLER32:3124fdc6
Mismatch for home::admin/files/Photos/Squirrel.jpg:
 Filecache:     SHA1:ed266b89065e74483248da7ff71cb80e3cca40a6 MD5:0f43fa5a262c476393018f7329080fa6 ADLER32:cad1073b
 Actual:        SHA1:ed266b89065e74483248da7ff71cb80e3cca40a5 MD5:0f43fa5a262c476393018f7329080fa7 ADLER32:cad1073a
Mismatch for home::admin/files/ownCloud Manual.pdf:
 Filecache:     SHA1:0c777cc6377bbd7d78da5aba6ea25fc2522dc435 MD5:dc63a3478f2f3b987c34089438743cb1 ADLER32:6de2fa25
 Actual:        SHA1:0c777cc6377bbd7d78da5aba6ea25fc2522dc436 MD5:dc63a3478f2f3b987c34089438743cb0 ADLER32:6de2fa26
Mismatch for home::admin/files/welcome.txt:
 Filecache:     SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089b MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64
 Actual:        SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089d MD5:368e3a6cb99f88c3543123931d786e21 ADLER32:c5ad3a63
 ✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ● 
  • Provide option path only and no username:
✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ●  ./occ files:checksums:verify -p welcome.txt         
Cannot load Xdebug - it was already loaded
Please provide user when path is provided as argument
 ✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ● 
  • Provide no username and path:
✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ●  ./occ files:checksums:verify               
Cannot load Xdebug - it was already loaded
This operation might take quite some time.
Mismatch for home::admin/files/Documents/Example.odt:
 Filecache:     SHA1:a2c861e88db506c0deadb9f7d78a1e27212ce9fb MD5:12348c01fb20e0230c1afdacfe514307 ADLER32:85ffdff4
 Actual:        SHA1:a2c861e88db506c0deadb9f7d78a1e27212ce9fc MD5:12348c01fb20e0230c1afdacfe514308 ADLER32:85ffdff5
Mismatch for home::admin/files/Photos/Paris.jpg:
 Filecache:     SHA1:73e6f4c116ef8fa4568028c7a1e45917ffeea8d7 MD5:0959a73dbc91b7c4fa8daa511aa00bb6 ADLER32:1c41d5d3
 Actual:        SHA1:73e6f4c116ef8fa4568028c7a1e45917ffeea8d8 MD5:0959a73dbc91b7c4fa8daa511aa00bb7 ADLER32:1c41d5d2
Mismatch for home::admin/files/Photos/San Francisco.jpg:
 Filecache:     SHA1:f00431c8f8ac1530febe519ca413806be16e8e4b MD5:84d5d4f2e31ecc00680f5886645c59a3 ADLER32:3124fdc5
 Actual:        SHA1:f00431c8f8ac1530febe519ca413806be16e8e4c MD5:84d5d4f2e31ecc00680f5886645c59a2 ADLER32:3124fdc6
Mismatch for home::admin/files/Photos/Squirrel.jpg:
 Filecache:     SHA1:ed266b89065e74483248da7ff71cb80e3cca40a6 MD5:0f43fa5a262c476393018f7329080fa6 ADLER32:cad1073b
 Actual:        SHA1:ed266b89065e74483248da7ff71cb80e3cca40a5 MD5:0f43fa5a262c476393018f7329080fa7 ADLER32:cad1073a
Mismatch for home::admin/files/ownCloud Manual.pdf:
 Filecache:     SHA1:0c777cc6377bbd7d78da5aba6ea25fc2522dc435 MD5:dc63a3478f2f3b987c34089438743cb1 ADLER32:6de2fa25
 Actual:        SHA1:0c777cc6377bbd7d78da5aba6ea25fc2522dc436 MD5:dc63a3478f2f3b987c34089438743cb0 ADLER32:6de2fa26
Mismatch for home::admin/files/welcome.txt:
 Filecache:     SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089b MD5:368e3a6cb99f88c3543123931d786e22 ADLER32:c5ad3a64
 Actual:        SHA1:eeb2c08011374d8ad4e483a4938e1aa1007c089d MD5:368e3a6cb99f88c3543123931d786e21 ADLER32:c5ad3a63
 ✘ sujith@sujith-ownCloud  ~/test/owncloud3   fix-path-verify-checksum ● 

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #35483 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #35483     +/-   ##
===========================================
- Coverage     65.77%   65.68%   -0.1%     
+ Complexity    18781    18780      -1     
===========================================
  Files          1222     1222             
  Lines         70870    70938     +68     
  Branches       1289     1289             
===========================================
- Hits          46618    46595     -23     
- Misses        23874    23965     +91     
  Partials        378      378
Flag Coverage Δ Complexity Δ
#javascript 53.7% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 67.05% <100%> (-0.11%) 18780 <0> (-1)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/VerifyChecksums.php 91.95% <100%> (-4.48%) 40 <0> (+3)
apps/dav/lib/Connector/Sabre/Node.php 75% <0%> (-6.15%) 49% <0%> (-1%)
lib/private/DB/MigrationService.php 85.29% <0%> (-5.15%) 53% <0%> (ø)
apps/files_external/lib/Lib/Storage/SMB.php 47.94% <0%> (-4.5%) 0% <0%> (ø)
lib/private/Files/View.php 84.6% <0%> (-0.05%) 398% <0%> (-3%)
...aring/appinfo/Migrations/Version20170804201125.php 0% <0%> (ø) 2% <0%> (ø) ⬇️

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 bdfd389...e8d37ad. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #35483 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35483      +/-   ##
============================================
- Coverage     65.77%   65.77%   -0.01%     
- Complexity    18781    18783       +2     
============================================
  Files          1222     1222              
  Lines         70870    70871       +1     
  Branches       1289     1289              
============================================
- Hits          46618    46615       -3     
- Misses        23874    23878       +4     
  Partials        378      378
Flag Coverage Δ Complexity Δ
#javascript 53.7% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 67.15% <100%> (-0.01%) 18783 <0> (+2)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/VerifyChecksums.php 91.76% <100%> (-4.67%) 39 <0> (+2)

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 bdfd389...e8d37ad. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #35483 into master will decrease coverage by <.01%.
The diff coverage is 78.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35483      +/-   ##
============================================
- Coverage     65.77%   65.77%   -0.01%     
+ Complexity    18781    18776       -5     
============================================
  Files          1222     1222              
  Lines         70870    70871       +1     
  Branches       1289     1289              
============================================
- Hits          46618    46615       -3     
- Misses        23874    23878       +4     
  Partials        378      378
Flag Coverage Δ Complexity Δ
#javascript 53.7% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 67.15% <78.94%> (-0.01%) 18776 <0> (-5)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/VerifyChecksums.php 91.76% <78.94%> (-4.67%) 32 <0> (-5)

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 29bbe99...53f697f. Read the comment docs.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

just a couple of minor comments to improve the readability.

apps/files/lib/Command/VerifyChecksums.php Outdated Show resolved Hide resolved
apps/files/lib/Command/VerifyChecksums.php Outdated Show resolved Hide resolved
The path option in the verify checksum command
should now be relative to the user folder.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas merged commit a0f0e3d into master Jul 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-path-verify-checksum branch July 1, 2019 08:29
@phil-davis
Copy link
Contributor

Backport stable10 #35694

@davitol davitol mentioned this pull request Sep 10, 2019
13 tasks
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.

None yet

4 participants